Deprecate MonadZero about purescript-control HOT 21 CLOSED

On reflection, I think whatever we do on #63, it's unlikely to have direct consequences for this issue. In particular, nobody seems to be defending MonadZero or proposing an alternative hierarchy which does include it. So I think we can go ahead and do this (and mark this library as 0.14-ready in the checklist on that compiler issue).

from purescript-control.

I'm on board with either --censor-user-defined-warnings=\"'MonadZero' is deprecated\" or --censor-codes="UserDefinedWarning" and an issue flagging the warning for removal in the next breaking release for any libraries which would otherwise fail CI due to the presence of the deprecation warning.

from purescript-control.

I agree with @kl0tl that the visibility is much better if we can keep the warning in place. I rarely look at pursuit for functions and classes I already use regularly and could easily miss something like this change.

I’m on board with a hybrid approach where we try to filter out warnings as narrowly as possible and leave issues to remove those filters later. I think that would be preferable to removing the warnings altogether from the source.

from purescript-control.

Sounds reasonable.

from purescript-control.

I think I might post about this on Discourse, since this feels like a big enough change that we ought to solicit feedback on it.

from purescript-control.

Based on the discussion in the Discourse post, what changes need to be made here to close this issue?

Seems like

• change the definition of MonadZero to class (Alternative m, Monad m) => MonadZero m
• deprecate the class via docs

from purescript-control.

That, plus changing the definition of MonadPlus to have Alternative and Monad superclasses. However I'd like to see if we can get closer to a consensus on #63 first, since I think the result of that discussion might have consequences for the MonadZero class.

from purescript-control.

As mentioned in #177, should other libraries in core which use MonadZero (at least lists and maybe, and perhaps others) continue to do so, or should the instance be removed? The question is pertinent for at least CI, which will not pass as implemented now if there are any library warnings.

from purescript-control.

One way to get around the CI issue is to censor the UserDefinedWarnings because we're using a custom compiler warning.

spago build -u "--strict --censor-codes=UserDefinedWarning"

We'll want to censor it in some situations (e.g. updating repos) but not others (updating the package set).

from purescript-control.

I think deprecating the class but removing the existing instances doesn’t make a whole lot of sense, because I’d guess most usages of the class probably rely on at least one of the instances in core; if we were going to remove the instances we might as well just remove the whole class. So for me it’s between skipping a deprecation cycle and just removing the whole class now, or removing the warning so that existing instances can compile without warnings. I’m leaning towards the latter.

I think the policy of “all libraries should always compile without warnings” is often incompatible with backwards compatibility and deprecation cycles - if you look at the warnings produced by Haskell dependencies I think the majority are to do with ongoing deprecation cycles - so perhaps in the longer term we might want to try to find a better compromise, perhaps allowing certain warnings but not others.

from purescript-control.

I think censoring UserDefinedWarning is probably not appropriate in general because of e.g

undefined :: forall a. Warn “undefined remains in code” => a

from purescript-control.

The right thing to do is probably to have a deprecation cycle. Yet due to how (I suspect) rarely MonadZero is used and how straightforward the fix is I am tempted to just remove it.

In the long run, though, we’ll have some other deprecation warning in a library crop up and we’ll have to figure out what to do about warnings in CI.

We could temporarily at least use @JordanMartinez’s solution or a similar flag — it’s better than disabling the warnings check altogether in CI.

from purescript-control.

If we are talking about temporary approaches, I think keeping the MonadZero class and the instances but removing the Warn constraint on it is preferable to adding a flag to CI scripts to ignore custom warnings; I think it is very likely that we’d forget to remove the flag again, especially if we had to make that change across a few libraries.

from purescript-control.

There are open issues on purescript-lists and purescript-maybe to track the future removal of their MonadZero instances, we could amend them to remember to uncensor user defined warnings.

We could also add support for a --censor-user-defined-warnings flag to psa so that we can censor only the deprecation message for MonadZero with:

spago build -u "--strict --censor-user-defined-warnings=\"'MonadZero' is deprecated\""

I’m not sure people will notice the deprecation of the class without a warning so given how little it is probably used I think we may as well remove the class and its instances now if we’re not going to warn before removing it later.

from purescript-control.

People will notice if they've seen the notice on Discourse, or if they see the docs for MonadZero on Pursuit. I think it's plausible that people will see the docs on Pursuit even if they have missed the Discourse announcement - if you spot that guard's type signature has changed and wonder why, you'll probably find the deprecation notice quickly enough.

from purescript-control.

I opened natefaubion/purescript-psa-utils#10 and natefaubion/purescript-psa#45 to add support for a --censor-user-defined-warnings flag to psa.

from purescript-control.

I think we should consider adding this filtering in a bespoke script, or using a fork of psa temporarily, rather than adding it directly to psa. It's hacky and not something I would want to support if I was maintaining the tool.

from purescript-control.

Alternatively, I guess we could justify censoring all UserDefinedWarning warnings if it's a temporary thing and we have an issue to track removing them further down the line. I think we're unlikely to encounter a UserDefinedWarning which isn't a deprecation warning within the core libraries; we shouldn't be using an undefined like that in core anyway, and I think we can probably catch any attempts to introduce one in code review.

from purescript-control.

I disagree on filtering user defined warnings in yet another tool and I think it would be useful in general to be able to temporarily ignore deprecations until one is ready to address them. Better compiler support for deprecations would of course be great but I’d rather think this through when integrating psa into the compiler and be able to do it with psa in the meantime.

Regardless, I’m less confident than you in our ability to catch Warn constraints during reviews (I bet nobody will check newly imported bindings for Warn constraints) but I would happily settle for censoring all user defined warnings in some core libraries as long as we keep the deprecation warning.

from purescript-control.

Of course in general the less tools you need to achieve something, the better, but I am specifically concerned about encouraging people other than us to depend on this behaviour in a widely used tool such as psa, because I think there's a good chance we would want to change or remove it at some point. That's much easier to do if the functionality only exists in a fork which isn't widely used. But yeah I think just filtering all UserDefinedWarning warnings is probably the way to go for now.

To clarify I think we're likely to catch any attempts to introduce undefined specifically, because that doesn't currently exist anywhere in the core libraries to my knowledge (and for the record I don't think it should exist in the core libraries). I agree that accidentally using something which has been deprecated is more likely to slip past unnoticed, but that feels like an acceptable risk to me (we can probably just fix any of those when they get removed upstream).

from purescript-control.

I've implemented that in purescript/purescript-lists#177

from purescript-control.