Redundant superclass warnings being included in -Wall destroys the "3 Release Policy"

mentioned in issue #11369

changed milestone to %8.0.1

changed weight to 10

Trac metadata

Trac field	Value
Component	Compiler → Compiler (Type checker)

I’m not sure if the 3-release-policy should get in the way of useful compilier output, and -Wall is (rightfully) used by developers to ask the compiler to comment the code as much as sensibly possible.

We could refine the policy to promise (or thrive) to deliver -Wall -Wno-redundant-constraints cleanliness, and similarly disable other warnings that get in the way of a nice backward-facing compatibility story.

From a GHC point of view, I think we can do whatever the Core Libraries Committee wants. Eg. switch off -Wno-redundant-constraints for 8.0 and switch it on for 8.2. Or whatever.

But I have always felt uncomfortable with the way that warnings are being essentially treated like errors: you must not have any. After all, if we switch it off, the exact same issue will arise the moment we switch it on!

Maybe we need three categories:

**Errors**: we can't compile your program
**Warnings**: we can compile it but your library should probably be warning-free
**Advisories**: there's something fishy, but no expectation that a library will be advisory-free. However advisories may become warnings in the next release, so you may want to invest a bit of effort at your convenience in getting rid of them.

I see our users (embodied in the Core Libraries Committee) as being in the driving seat here.

Simon

NB: Currently -Wredundant-constraints is part of the default warning set (i.e. it's active even w/o -W or -Wall)

(also, a tentative plan for GHC 8.2 is to overhaul the warning system to support warning-sets more uniformly, see also Design/Warnings)

I definitely think it shouldn't be in the default warning set.

I suspect making an "advisory" category is too late for 8.0. But I think keeping -Wredundant-constraints out of -Wall for now is the right thing. Any future refactors of the hierarchy will also still trip it up, so in the future we should probably move it into -Wall and then follow nomeata's suggestion of modifying the policy to be -Wall -Wno-redundant-constraints clean.

I agree with Simon that things like redundant constraints are not "probably broken" (warnings) or broken (errors).

I'm a big fan of compilers educating users to best practices (advisories), and redundant constraints fall into this category. It's as bad as trailing whitespace, literal tabs, or redundant parentheses.

That said, I think we need a more strict "-Wall", I named it "-WALL" for the lack of a better name (-Wpedantic akin to GCC would also work, or -Wlint) where we enable such suggestions. This flag would be useful for building-and-fixing once-ish per release, and would make no compatibility guarantees.

I think Herbert was leaning towards -Weverything.

I've talked with some folks on the committee informally, and with Gershom while I'm visiting New York at the moment:

At the very least, I definitely think the warning for redundant constraints shouldn't be in the default warning set. It goes off in situations where there is no good way to mask it at this time.

Somewhat ironically the "3 release compatible" version of adding redundant-constraints would be to add it as an option now and consider turning it on in -Wall in 8.4 when such a -Wno-redundant-constraints option would be supported by 3 releases of the compiler. Possibly with Gershom's suggestion from the mailing list of making unrecognized -Wfoo options emit a single warning about an unrecognized option rather than have the compiler emit an error and die. This wouldn't preclude adding it to -Wlint or -Weverything when the warning overhaul goes in in 8.2.

Putting it in -Wall weakens the 3 release policy in a way that requires users to use cabal trickery to pass ghc options to avoid warnings in an uncomfortable way -- since old GHCs won't recognize a -Wno-redundant-constraints option, but if you feel it really belongs there I guess that is just what we'd have to do.

In a perfect world this'd be relegated to a sort of -Wlint which checks style.

So, this seems to suggest a few ways forward

Just remove it from the default constraint set. This is the minimum we should do.
Remove it from both the default constraint set and -Wall. Implement Gershom's suggestion that unrecognized -Wfoo options passed to the compiler simply cause it to warn about an unrecognized option. This would give us the option to add it to whatever -Wlint equivalent is added in 8.2 and an option to bring it into -Wall in 8.4 and weaken the 3 release policy accordingly, but now in a way that doesn't require the user to play with cabal flags.

1 is easy but damages the already weak 3 release policy almost to the point of irreparability: few people seem willing to use if clauses in cabal files, the reaction is much the same as to CPP hacks.

2 is a fair bit more work, but would serve to placate the folks for whom we put the 3 release policy in place in the first place.

I'd prefer 2, but we could live with 1 if the amount of work involved was too prohibitive and GHC HQ felt strongly about the issue.

Replying to [ticket:11370#comment:113790 ekmett]:

I think Herbert was leaning towards -Weverything.

Indeed, because we have a precedent in clang which uses -Weverything for that very purpose (since -Wall traditionally doesn't comprise *all* warnings for gcc/clang, which is probably also the origin from where GHC copied this tradition...)

Replying to [ticket:11370#comment:113791 ekmett]:

Possibly with Gershom's suggestion from the mailing list of making unrecognized -Wfoo options emit a single warning about an unrecognized option rather than have the compiler emit an error and die.

Filed as #11429 (closed) -- if we agree this is a sensible thing to do, I think we should try to get this done in time for GHC 8.0 to extend the compat-window as far back as possible, i.e. starting at GHC 8.0 for a -Wno-unrecognised-warning-flags (modulo bikeshed) support

Trac metadata

Trac field	Value
Related	#11369 → #11369, #11429 (closed)

Remove it from both the default constraint set and -Wall.

Doesn't that mean that 8.2 will be in precisely the same situation as 8.0, and we'll just have the identical conversation then?

What is -Wlint? Is it the same as -Weverything? What does it mean? Perhaps this:

Default warnings are what you get by default. Is there a flag -Wdefault?
-Wall means a larger set.
-Weverything means the entire set.

Plus there is some protocol that a new warning always starts in -Weverything; may move in the next release to -Wall; and may move in the release after that to the default set. Both moves are a judgement call.

Maybe I have this wrong. It would be super-helpful if someone could lay our the entire proposal. It doesn't look hard to implement in 8.0 if I have it roughly right, and someone is willing to step up to do it.

Simon

Doesn't that mean that 8.2 will be in precisely the same situation as 8.0, and we'll just have the identical conversation then?

The main difference is that in 8.2 we'd have at least 2 releases with #11429 (closed) resolved, so if users used -Wno-whatever and the compiler didn't understand it, it'd not be a hard error. That makes increasing the length of the options list that the 3 release policy applies to a fair bit more palatable. By 8.4 users wouldn't even have to use cabal flags, so in many ways this is the same conversation, but it gets easier to resolve the further out we push.

Plus there is some protocol that a new warning always starts in -Weverything; may move in the next release to -Wall; and may move in the release after that to the default set. Both moves are a judgement call.

That sounds like a good rubric to me. All of the warnings we'd need for the items on the current roadmap (https://prime.haskell.org/wiki/Libraries/Proposals) seem to fit these guidelines.

As for -Wlint it was just a suggestion as to what one of the extended warning sets might be, if it existed at all, it'd be included in -Weverything. Herbert has given a lot more thought to what groupings of flags make sense than I have, and I'm happy to defer to him there.

I think Ed's comments on the two proposed plans of action are exactly right.

Note that two things by the way are intersecting here: 1) GHC adding new warnings in general, which is largely unrelated to the 3-release policy but should be generally managed with some degree of rollout and 2) The presence of warnings at all such as redundant superclasses, which by their very nature warn against code that is necessary and desirable in migration paths. So lots of the details here don't apply to most warnings added -- they apply to this particular one, and adding features so that it and similar warnings can more easily not stand in the way of library evolution, in general.

It would be super-helpful if someone could lay our the entire proposal.

Since you are clear, might one of you do this? On a wiki page where we can refine the text to clear up ambiguities. I'm not capable of composing all these comments in my head to get the end result. Thanks!

I will try to get to that this weekend or early next week at the latest.

Replying to [ticket:11370#comment:113817 simonpj]:

It would be super-helpful if someone could lay our the entire proposal.

Still hoping for this? Since a spec precedes the implementation, and we want the implementation in 8.0.1, getting the spec is pretty urgent.

Thanks

Simon

assigned to @bgamari

Okay, here's my stab at a minimal description of the plan:

https://ghc.haskell.org/trac/ghc/wiki/Design/Redundant_Superclass_Warning_Plan

Herbert and Edward can fill in if they think I got it correct.

Thanks Gershom. I have revised Design/Warnings to express ticket:11370#comment:113813 too.

I suggest that others concentrate on Design/Warnings, including the timing aspects.

One open question here is how this should interact with #10632 (closed), which requests warnings for unused implicit parameters. This was addressed with -Wredundant-constraints, which we are now disabling by default. Is it reasonable to expect users to enable -Wredundant-constraints to be warned of unused implicit parameters? Implicit parameters "feel" a lot more like arguments than constraints.

mentioned in commit 8e9a8702

Resolved with 8e9a8702. Needs merging to ghc-8.0.

Handling of unrecognised warnings (#11429 (closed)) is implemented in D1830.

added 1 deleted label

Trac metadata

Trac field	Value
TypeOfFailure	OtherFailure → IncorrectWarning

closed

Merged to ghc-8.0 as 57bce48f.

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

removed 1 deleted label

mentioned in issue #11494 (closed)

reopened

-Wredundant-constraints was removed from the standard warning set, but we should put it in -Wall. We explicitly allow warnings in -Wall (and to a lesser extent the standard warning set) to change between releases, and this warning should really be in -Wall.

Trac metadata

Trac field	Value
Resolution	ResolvedFixed → Unresolved

Very well.

I'll admit I'm sad to see this go into -Wall, as this is the first warning in -Wall that sometimes just absolutely can not be worked around without explicitly turning it off, but I see your reasoning.

this is the first warning in -Wall that sometimes just absolutely can not be worked around without explicitly turning it off

Does this sentence have an implicit “without losing 3-release compatibility”? Or is there really code that triggers this warning and cannot be worked around for a single set of GHC and library versions?

We should stay explicit about these things. I conjecture that most users of GHC do not attempt to write 3-release-compatible code and are just interested in the most tidy code for their current target.

Does this sentence have an implicit “without losing 3-release compatibility”?

No.

I mean explicitly that there is code that I have in the wild that can only work around this by turning off the warning. If you write any class that only supplies a law then it will always appear to be a "redundant constraint". GHC doesn't know about the laws.

Examples from real math: Upgrading a braided monoidal category to a symmetric monoidal category, or needing to know that the Cayley-Dickson construction that yields the complex numbers from the reals needs a the ring structure it builds over to be trivially involutive for the lifted ring structure to be commutative.

These will simply always trigger this warning. In some cases not just for my library but for users of my library.

Now, I've already patched most of that code to include an explicit, messy

#if __GLASGOW_HASKELL__ >= 800
{-# OPTIONS_GHC -fno-warn-redundant-constraints #-}
#endif

But it is telling to me that this is the first situation where I've had to hack in explicit {-# OPTIONS_GHC #-} to work around a warning going haywire.

At present the only other code I can think of where I turn on explicit {-# OPTIONS_GHC #-} is to hack around changes in sharing behavior across GHC versions that breaks particularly tricky uses of {-# NOINLINE #-}. To get reflection to work across a wide enough array of GHC versions I wind up with a cocktail that looks like:

{-# OPTIONS_GHC -fno-cse -fno-full-laziness -fno-float-in #-}

but that isn't hacking around warnings, but rather buggy code motion that changes in behavior across GHC versions.

Note that there's another silliness here. If we have both -Wcompat and -Wredundant-constraints in the -Wall set, then there will be cases where the "correct" fix to a compat warning is precisely to introduce something that will trigger a redundant-constraints warning! I don't think we want to have actively contradictory warnings in the same -Wall set -- even if there is a way to fix this (i.e. explicitly turn off one or the other set of warnings) it will send users into a flurry of confusion.

Also, I may have missed something, but where is the discussion that overturned the prior resolution of this ticket? I thought we had a good plan of action, arrived at after a fair amount of discussion. Now I see that there's been a 180-degree-turn, but I don't see any discussion surrounding it outside of "this warning really should be in -Wall." In particular, if this is moved into the -Wall set a few releases down the line, then at a minimum users won't need a version-specific-flag to disable it, which was the motivation for the prior plan to begin with.

@gershomb - re the missing discussion, this came up during the GHC Skype meeting yesterday, I was concerned that we had stopped adding new warnings to -Wall which is exactly what I wanted to avoid (see https://mail.haskell.org/pipermail/ghc-devs/2016-January/010955.html).

I've also argued that we should put -Wcompat into -Wall for the same reason. However, I wasn't aware that there would be a conflict between the compat warnings and -Wredundant-constraints, nor was I aware that we planned to have compat warnings that were very difficult to act upon. So in the light of that, I suggest that we don't add warnings to -Wall that are not actionable or that conflict with other warnings in -Wall. Sadly that means that a lot of people won't see some of the compat warnings, but maybe that's ok.

My apologies for diving into the discussion with partial information. But I do feel strongly about one thing: that -Wall should always be the set of all warnings that we believe it is both possible and sensible for programmers to act upon. We shouldn't try to freeze -Wall under a misguided desire for stability.

If you write any class that only supplies a law then it will always appear to be a "redundant constraint".

One (not necessarily me) could argue that in a language where laws are just convention, one should not use superclasses to document such a convention. It is a bit similar to annotating functions with an unused argument of some type Deprecated instead of documenting that with a pragma or in the documentation. But OTOH, that would work by matching it with _.

It's perfectly fine for -Wall to contain warnings that sometimes need to be disabled, for reasons of personal taste or necessity. A lot of our code has -Wall -fno-warn-unused-do-binds -fno-warn-name-shadowing, and it's quite often necessary to throw in a -fno-warn-orphans when there's no reasonable way to avoid an orphan.

The plan is:

-Wall will include -Wredundant-constraints
If at some point we have a compat warning that requires the addition of a redundant constraint to suppress it, then when this warning is enabled at the same time as -Wredundant-constraints, we will arrange to not emit a redundant constraint warning for constraints that are required to suppress the compatibility warning.

Please let us know if this will cause a problem. @bgamari is going to action the first change.

Including -fwarn-redundant-constraints but not -Wcompat in -Wall to avoid contradictory suggestions, does indeed seem like a reasonable compromise.

Going through all of the code I've updated for GHC 8, -fwarn-redundant-constraints actually gave useful information in 18/20 cases, and had to be manually disabled because there was no appropriate "fix" in 2 places, putting it on similar footing with -fwarn-orphans, where most times you can slide the instance around to live with the data type or class in question, but sometimes you get boxed in.

Trac metadata

Trac field	Value
Differential revisions	- → D1956

mentioned in commit 2e49a316

closed

Merged to ghc-8.0.

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

mentioned in issue #12700

added Phighest label

Redundant superclass warnings being included in -Wall destroys the "3 Release Policy"

Child items ...

Activity

Redundant superclass warnings being included in -Wall destroys the "3 Release Policy"

Relates to

Activity