It’s a question of how oldish it is, and whether we can give some nicer compiler errors. I doubt we’re going to finish this proposal in the immediate future (in fact, I’d recommend we hold off on the last phase for a handful of years), and we can hopefully add some compiler errors that inform the user to downgrade if they want this code.
With such timing, if after 2-3 years of warning on lawful definitions and about 12 of warning/erroring on unlawful, remaining packages must be rather old and unmaintained indeed.
(Edit: I am still in favor of making the new warning (-Wredundant-canonical-methods) enabled by default, but in this MR I merely put it in -Wcompat instead; I didn’t want to smuggle that assumption along with the implementation choices I made. It would be easy to promote to a default warning, should someone in the right committee decree that we can do so.)
In the modern division of labour between Haskell committees, this change likely falls under purview of GHC SC, not CLC. You are not changing base, you are changing the language itself.
I’m in favour of making any explicit (canonical or not) implementation of return and (>>) a warning. This would hopefully give a renewed push to clean up old code.
I’m not so confident about making anything an error until a) either 95% of Stackage are unaffected, b) or there is a reason going beyond “aesthetics”. Is return in class Monad a wart? Definitely. Is it harmful? Unlikely.
Something I realized while preparing my MR is that the current default implementation of (>>) is actually:
m >> k = m >>= \_ -> k
and not (>>) = (*>), which is what we’ve been telling people is ‘canonical’. So some code could be using the canonical definition and, if we warn them to remove it, it might affect their program. (Perhaps going via (>>=) has different performance characteristics from (*>); or perhaps their monad is subtly law-bending.)
Fixing that by changing the default implementation of (>>)would be a change to base. And there’s a somewhat dire warning in the source against doing just that. But I think that the warning can be dismissed if the compiler makes it an actual error to implement an Applicative with (*>) = (>>). So that’s an argument for promoting -Wnoncanonical-monad-instances to an error sooner.
I think we’re pretty close to this threshold! I’ve been compiling sampled projects from the latest Stackage nightly with my patched GHC and, while compilation errors in dependencies will prevent me from doing anything close to the entire package set, so far I have gotten to 1186 packages attempted (out of 3084), from which 70 builds included one or both of the promoted errors, for an unaffected rate of 94.1%.
Here's a list of the packages that need fixing so far, if folks want to start submitting patches to them. If my estimate is to be believed, we only need about 28 of them to be patched to get us to 95%!
Thank you so much for the work you’ve put in so far! I’ve got no further comments, you’re doing great on progressing the warnings and errors of phases 2 and 3. As mentioned previously, I’ll ask the GHC SC (not sure what that is, @Bodigrim can you elaborate?) later this week, but I’ll also bring this up in the GHC weekly meeting.
I agree with this, except I think 5% breakage (ie. 95% unaffected) is too much breakage to justify implementing this proposal. I think it would be a shame to break so many existing Hackage packages for newer GHC versions just to fix this wart.
It’s hard to tell how many packages are stable purely because they’ve not changed at all. While -Wall shouldn’t be required for hackage or stackage, these warnings have existed for about ten years, and been enabled by default for four. I don’t know what we could do for any more of a soft lead in to these errors. Rhendric has been making changes for maintainers, and on one occasion had PRs rejected because the maintainer doesn’t want those changes until defining these methods are errors.
Making these changes makes Haskell better, I think we can all agree. I hope that we can agree that this change shouldn’t be held back because maintainers haven’t looked at their package warnings in a while.
Besides, just because those packages won’t compile with newer GHCs doesn’t mean they’re unusable; because of the long lead up, every version of GHC from 2015 onwards would be able to compile the code made after this change, so if you want to keep using old packages and new ones, you can simply use a slightly older GHC until the package is updated.
I think such packages can be legitimately excluded from the analysis. If they’re actively maintained and the maintainer has explicitly rejected prospective fixes, then that’s on them.
Haskell the language, I agree. Haskell the ecosystem, I would need more persuasion.
Definitely don’t agree with that one! (That’s not to say I agree with the opposite either.)
I think it is fair to remove those packages and their reverse dependencies from the analysis. Those packages are these and strict, which each have more than 7800 indirect reverse dependencies (across all their versions, I assume), which would block all of their compilation as well. (EDIT: on stackage, these has 25 direct reverse dependencies, versus the 73 on hackage. This includes lens and aeson, which have 100+ and 300+ reverse dependencies respectively, out of 3084 stackage packages at time of writing)
I’d like to hear further elaboration on what you need convincing on.
So what is the alternative? We wait four more years and check in again?
I’m ready to be convinced that we could take things slower, but without actionable objectives this proposal (and the monoid one as well) will stagnate even more than they currently have.
As a general point, we could avoid having to make these difficult trade-offs if more people got involved in this sort of work. Often the thing that is blocking us is capacity.
For instance my suggestion in Monad of No Return: Next Steps - #7 by TeofilC requires more work but means that no code would be broken by the change (though it still might not be the right choice for other reasons).
A lot of this work is much more accessible than people assume, and there’s a wide variety of tasks that can help move these things along. You don’t have to be a GHC or Haskell expert to get involved in these things.
Though of course, many people just don’t have capacity to get involved and that’s OK too.
I’m fully paged-out on this discussion, so bear with me.
Phase 1 introduced a warning when non-lawful instances of return and (>>) are defined.
I think “non-lawful” and “non-canonical” in this discussion mean the same.
And I think that a “non-canonical” definition of return is anything other than (literally)
instance ... => Monad T where
...
return = pure -- RHS must be precisely `pure`
or omitting the definition of return entirely, since pure is the default decl.
And similarly a non-canonical defn of (>>) is anything other than
(>>) = (*>)
or, again, omitted.
@TeofilC has kindly offered to write PRs to the two relevant GHC proposals, along with a crisp summary of what changes are proposed and why. I think the GHC SC will approve them quickly, if they are clearly explained. (Without relying on readers paging in a decade-old debate.)
Copying in the CLC would go wise, since this affects the base API, and we cannot change that without CLC agreement.
Hmm, perhaps I’m misunderstanding what you mean, but isn’t m1 >> m2 = m1 >>= \_ -> m2 lawful but non-canonical?
In fact, I seem to recall circumstances where the latter can actually be preferable, when m2 retains large amounts of memory, and being behind a lambda can allow it to be freed early (but I can’t work out how this goes now – perhaps I imagined it).
(We do need to be careful when messing around with definitions of (*>) = (>>), and either bring an explicit definition down from Monad or, as I did in the conduit PR, inline the default (>>) unless/until we can be sure from other reasoning that doing something else is safe.)
I’d recommend to work through potentially affected packages as much as possible before pushing a GHC proposal forwards. It’s far better optics if you can say “people happily accept PRs removing redundant methods” and “only that many packages remain affected” instead of simply stating “if someone does lots of work across dozens of packages, it is possible to clean up the errors”.
At risk of sounding like I’m complaining—I don’t mean to be, but—when GHC contributors design a warning to say, ‘Hey, this isn’t an error now but we want it to be one later; you should change this when you get a chance,’ it seems like bad incentivizing to then recommend to the GHC contributors that they should do as much work as possible to clean up any packages whose maintainers didn’t heed the warning.
We have a pretty good community here and it’s clear from the numbers that there are plenty of maintainers who would look at that system of incentives and still take proactive action when they see warnings like this instead of punting and relying on the people pushing the change to pick up the slack, but also social systems do degrade when incentives are misaligned. Being intentional about who is responsible for what, and communicating that consistently through both warning messages and conversations around when to move on from warnings, is all part of ensuring ecosystem stability in the face of an evolving language.