Request for feedback on GHC proposal to change warning severity

[adamgundry]

The GHC Steering Committee is considering a proposal to enable -Werror=missing-methods -Werror=missing-fields by default. See ghc-proposals/ghc-proposals#571 (rendered version).

This is a breaking change to existing code (which has been impact assessed). Moreover, it means that future changes to libraries will more easily lead to breaking changes, because the addition of a method to a class or a field to a record will trigger a compile-time error (rather than a warning). Thus while it does not directly involve changes to base, we would appreciate feedback from the CLC as to whether this change is desirable.

[parsonsmatt]

Massive +1 from me!

[tomjaguarpaw]

I too think this is a good idea.

[simonpj]

Massive +1 from me!

That's interesting. The GHC SC is worried about making a change that will outright-break some (not much but some) existing code, causing an error where there would currently be a warning.

An alternative would be to create the severe category of warnings, but not make them errors by default. A client of Haskell could choose to make -Werror=severe to make all those "severe" warnings into errors; but they would not be forced to.

On the other hand, an ignorant client might simply ignore the warnings (they are only warnings after all) and then get hard to debug non-termination errors.

A reason for consulting the CLC is to get your views about balancing:

• Breaking some existing code, or
• Allowing existing and future code to contain nasty missing-field land mines.

Any views?

[parsonsmatt]

Any package that is broken by promoting this from a warning to an error is already seriously broken. The debug/diagnosis time for fixing a missing method in a dependency, especially when the class and instance may be buried deep in a dependency graph, is far worse than fixing it up pre-emptively, esp since a fix can be a package version bound and Hackage revision, at smallest, or setting -Wwarn=missing-methods.

I can't imagine a scenario where a class author puts a new method on a class and I don't want that to be a breaking change for all downstream users.

EDIT:

I guess to elaborate - this kind of error isn't the sort of error that can really live very long in the wild anyway. Package A defines class C, package B defines instance C Foo, and then package A releases a new version with a new method on C. Package B is broken and this will bite the first person that uses it, at runtime, with a mysterious error that may even originate in some library code that expects to treat instances of C with the new method.

Contrasting with W-x-partial, where you can write if null xs then ... else ... head xs ..., and locally verify that an ordinarily dangerous function is safe in that context. You can only avoid the problem by not calling the offending method, but the locality of that cannot be guaranteed. If you have a missing method then you will have a problem, and the library should not build.

I think GHC should strive to make fewer breaking changes, and make those changes as easy-to-adopt as possible. But this is not a question of GHC breaking things, but rather revealing and providing early-diagnosis for already broken things.

In a perfect world, B has strict enough bounds on A that this won't happen, but in the world we actually live in, that's usually not the case.

[angerman]

My stance is the same as the one I posted to the SC. So a copy here too:

---

Just to clarify: I am not against change, or evolution. I'm actually looking forward to progress. What I am against ist sudden breakage.
As such, if there is breakage (clc stackage is a subset), we have to assume there will be breakage in production codebases, most
of which are likely not public.

Can't we have -Wcompat enable -Werror=missing-methods, and -Werror=missing-fields (I guess that's the same as -Werror=sever?)
Advertise this prominently in the release notes for GHC 9.10? And then enable this fully in GHC 9.14? Though I guess the flag we want
is really -Wcompat-error, or we rather change the notion of -Wcompat to also promote warnings to errors early? In any case either the
current documentation for -Wcompat would need to be adjusted, or we'd need something that signals new errors.

Ideally I'd like to see something like a warning for missing-methods, with an additional note that this will become an error in GHC X.Y,
and that one can opt into this behaviour by enabling -Wcompat.

My test for support is generally: can I take existing code unmodified, swap out the compiler, and it will still compile? That way I can report
back regressions, bugs, ... early on during alphas, betas, and release candidates. Right now I can't. I usually have to wait for x.y.4+. That
also means the feedback for anyone working on GHC is terrible. You won't hear about bugs until late in the release cycle where the
master branch has moved forward by a lot. At the same time it's painful for integrators who end up having to backport and patch old
branches. https://gitlab.haskell.org/ghc/ghc/-/wikis/GHC-status already states anything but 9.4 and 9.6 won't see any further releases.
Our current production compiler is 8.10, we could not switch to 9.2 due to performance regressions. And finally have almost everything
compiling with 9.6, but are far from having any form of performance profile feedback on 9.6 yet.

Again, I'm not against breakage per-se. I'm against sudden breakage. Managed evolution or however we want to call it, is something
I'd absolutely support!

---

I have been and I will continue to be against any form of sudden breakage. The test will always be: can I swap out the compiler trivially to test a new compiler?

[angerman]

+1 iif given a transition/migration period.

-1 without.

[hasufell]

@angerman since this type of breakage does not require you to change any code, but only adjusting your cabal/stack/GHC flags configuration if you don't care about the errors... is it not enough to put it prominently in the release migration guide?

GHCup can print post install messages. We could start linking to the migration guide or warn about this instance directly.

I feel the cost is low.

[angerman]

@hasufell so the release notes will say:

Please put ghc-options: -Werror=no-... into your cabal.project file to keep your existing code compiling.

And then those flags will bitrot there?

That's an ever worse outcome imo. Cranking up the warnings/loudness of the warnings, sure. But requiring changes to your project to swap out the compiler? No. It's bad enough as it is already. I'm not willing to agree to this just because it's small. The next time this will be used as a pretext to make me agree to just so slightly larger changes.

Again, changing this is fine. I'm just opposed to changing this abruptly. We already have releases ever ~6mo.

[phadej]

Ideally I'd like to see something like a warning for missing-methods,

-Wmissing-methods and -Wmissing-fields are enabled by default. People have seen these warnings for a decade if not longer.

% ghci-7.4.2
GHCi, version 7.4.2: http://www.haskell.org/ghc/  :? for help
Loading package ghc-prim ... linking ... done.
Loading package integer-gmp ... linking ... done.
Loading package base ... linking ... done.
Prelude> instance Num () where

<interactive>:2:10:
    Warning: No explicit method nor default method for `+'
    In the instance declaration for `Num ()'

(GHC-7.4 was released in 2012).

EDIT: We don't need -Wall, the warning was enabled by default already then.

[angerman]

@phadej yes. And we have other warnings too. So every warning can become an error now? Please do not selectively quote me like that.

This is what I wrote:

Ideally I'd like to see something like a warning for missing-methods, with an additional note that this will become an error in GHC X.Y,
and that one can opt into this behaviour by enabling -Wcompat.

I've added additional emphasis to highlight what I tried to convey.

[nomeata]

So the assumption is that there are people reading warnings for their dependencies after each upgrade to GHC, but are not acting on them, unless the warning says it will become an error?

[angerman]

@nomeata i have honestly no idea if

<interactive>:2:10:
    Warning: No explicit method nor default method for `+'
    In the instance declaration for `Num ()'

Is an informative warning or a severe one. It looks mostly informative. The warning doesn't even say why this is a problem. Ok, so there is no default method, so what? GHC seems to suggest I should probably add one, but it doesn't say in any form what so ever that it will start rejecting this by default going forward.

Again, I have no objection to introducing severity. I have an objection to turning a warning into a sudden error without prior notice.

[hasufell]

And we have other warnings too. So every warning can become an error now?

We're talking about warnings that can lead to runtime errors. At least that's what @parsonsmatt was getting at afaiu.

This is indeed an interesting discussion. I'm not perfectly sold on either side.

To me the cost is low enough to just toggle it. If others feel strongly against it, a more elaborate deprecation can take place too.

[angerman]

@adamgundry suggested

---

M.hs:8:10: warning: [-Wmissing-methods]
     • No explicit implementation for
         ‘<>’
     • In the instance declaration for ‘Semigroup T’
     • Warning: this may lead to an infinite loop or runtime exception.
     • This will become an error by default in a future GHC release;
       use -Werror=severe to make severe warnings into errors now.

Then in a future release:

M.hs:8:10: error: [-Wmissing-methods, -Werror=missing-methods]
     • No explicit implementation for
         ‘<>’
     • In the instance declaration for ‘Semigroup T’
     • Warning: this may lead to an infinite loop or runtime exception.
     • Use -Wwarn=missing-methods to permit this anyway.

---

which I consider a good solution. It adds more context to the warning. It informs the user that this is severe enough to become an error going forward and then follows this by turning it into an error in a later GHC release.

[hasufell]

I think there are two things to discuss:

1. what type of policy do we want in general for changing defaults in a breaking manner? What exceptions are we willing to make to the policy?
2. this specific instance

I can easily get behind @angerman wrt point 1. The policy should be that there can't be breaking changes to defaults without clear communication and deprecation.

Then: does the currently discussed instance qualify for an exception? The brought forward arguments were:

1. it's very easy to unbreak (one-liner in your project config)
2. it "escalates" a possible runtime error to a compile time error
3. the warning has existed for very long

@angerman if that is not sufficient for making an exception to a stricter policy, what would be? Nothing?

[angerman]

@hasufell for a warning that has been there for 10 years (and the associated issues have been there as long then as well), we now need an exception to go from instead 6mo month with a significantly more informative warning, to an error? Where does this rush come from that doesn't permit even a 6mo transition period? As @phadej pointed out, this has been a warning in GHC since 2012. If we are going to make this an error in ghc 9.10 (~6mo out), that would be in 2024 or 12 years (144mo) vs making it an error in ghc 9.12 (150mo; a ~5% increase).

We could potentially even compromise on idk. GHC 9.8.2 starting to have improved warnings and then 9.10 throwing an error, if we feel this really needs to be rushed.

This whole discussion also feels a bit like "our users are stupid, we need to force their hand". I'm not very comfortable with that line of thought. I'd be much more comfortable with "our users appear stupid, we need to think hard how to educate them better".

@hasufell Let's assume we had a policy in place. From the arguments you brought forward I can see the merit for 2, but I don't see why a more informative warning text can't address this and it needs to be a sudden error. I believe argument 3 actually supports a transition period, if we were able to live with a warning for so long, we can surely live just a little longer with a warning. And while 1 seems like a small change, I don't see how this warrants skipping a transition period.

Again, @adamgundry has in my opinion suggest a very excellent path forward. That

• improves the warning to be clearer.
• also notes that this is considered severe enough to become an error down the line.
• provides an opt-in mechanism.

[simonpj]

Again, @adamgundry has in my opinion suggest a very excellent path forward. That

Let's see if that suggestion (summarised here) resonates with other CLC members. It looks good to me.

[tomjaguarpaw]

I would be happy with that suggestion.

[phadej]

Where does this rush come from

I opened a ghc-proposal issue ghc-proposals/ghc-proposals#544 roughly a year ago. If the change is going to happen only in GHC-9.10, it's at best 1.5 years; but probably closer to two years.

I added -Werror=missing-methods to be on by default to haskell-ci in 2020: haskell-CI/haskell-ci@f00352a, and it was fine not having it enabled by default in GHC. The need to enable it by default, from my perspective, was to be able to make impact assessments for base changes.
(Enabling -Werror=missing-methods now on Stackage means fixing/disabling too much unrelated stuff).

OTOH two years from a need to an implementation is not long, but OTOH that issue being a prerequisite for another stuff so it has to wait until then.

It took too long already, so I dropped an idea for base proposal (yay, no breaking change proposals!).

IMHO, GHC evolution is halted to death. I probably should learn to be fine with the current state of affairs in GHC/base/... or/and contribute somewhere else where I can feel I get something done.

[parsonsmatt]

I'm happy with a longer migration period, though I'm not sure I see the point. It would be interesting to see the impact assessment.

After sleeping on this, I have one reservation.

An idiom I like is to define, say,

instance TypeError ('Text "Informative and useful error message") => Cls T

Where defining the methods is unnecessary - after all, you can't call them without incurring a type error anyway. But this does still trigger missing-methods warning, which is promoted into -Werror in my codebase, so we end up needing to write error "unreachable" a bunch. Adding a new class member in this case would incur an unnecessary breaking change.

We can reduce the impact of breaking changes here if we can also reduce the warnings on TypeError or Unsatisfiable instances to classes. I imagine that requires a GHC proposal, though.

[phadej]

@parsonsmatt Nope. With GHC-9.8:

with TypeError you indeed need to specify the methods:

Prelude GHC.TypeError> instance TypeError ('Text "No") => Num () where

<interactive>:5:10: warning: [GHC-06201] [-Wmissing-methods]
    • No explicit implementation for
        ‘+’, ‘*’, ‘abs’, ‘signum’, ‘fromInteger’, and (either ‘negate’
                                                              or
                                                              ‘-’)
    • In the instance declaration for ‘Num ()’

but with Unsatisfiable you don't:

Prelude GHC.TypeError> instance Unsatisfiable ('Text "No") => Num () where
-- no warnings.

That is one of the benefits of Unsatisfiable over TypeError.

[Bodigrim]

FTR I'm strongly in favor of the proposed change and I don't see much point in delaying it (I'm not against delaying, I just don't see why). There is a warning already (which means that you can fix all occurencies without upgrading GHC), and one can undo the change globally with a single line in cabal.project.

A reason for consulting the CLC is to get your views about balancing:

• Breaking some existing code, or

• Allowing existing and future code to contain nasty missing-field land mines.

I appreciate the ask, but as a matter of principle, I'm worried about erosion of responsibilities. Following this line of thinking, one should consult CLC on every GHC proposal, because, well, each proposal can break something and affects long-term maintainability of the language. We are not particularly well equipped to discuss such questions and do not hold a community mandate to decide on. I wish GHC SC felt more confidence to make decisions within their purview.

Maintaining two parallel discussions (here and at the GHC proposals tracker) is not helpful. I strongly urge everyone to continue the discussion at ghc-proposals/ghc-proposals#571.

[simonpj]

We are not particularly well equipped to discuss such questions and do not hold a community mandate to decide on. I wish GHC SC felt more confidence to make decisions within their purview.

I think we do, but thought it worth consulting you as a matter of politeness. While many GHC proposals could break something, this one is unusual in that it is primarily designed to break something (albeit things that probably have bugs), and the CLC has a lot of experience in the consequences of such breakage.

I agree that it'd be better to invite CLC members to contribute to ghc-proposals/ghc-proposals#571 rather than to have two paralel threads.

Contributing is a matter of choice, not obligation! We are not seeking formal approval, for reasons you mention.

[Bodigrim]

The GHC proposal has been sent for revision, so I guess we can close this discussion for now.