Opened 4 years ago

Closed 4 years ago

#11370 closed bug (fixed)

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

Reported by: ekmett Owned by: bgamari
Priority: highest Milestone: 8.0.1
Component: Compiler (Type checker) Version: 7.10.3
Keywords: Cc: ekmett, quchen
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #11369, #11429 Differential Rev(s): Phab:D1956
Wiki Page:

Description

These redundant constraint warnings being in -Wall make it seemingly impossible for us to live up to the '3 Release Policy' in many situations.

https://prime.haskell.org/wiki/Libraries/3-Release-Policy

Consider the AMP: (Monad m, Functor m) gets used as a replacement for Monad m in many situations for compatibility with code before GHC 7.8, where that wants to call both fmap and something from Monad, but from 7.10+ that Functor is a redundant constraint. We have no backwards-facing 3-release-compatible constraint that someone could use in a type signature without warning to get both fmap and return that won't trigger this warning.

Similarly if we split a class we can't advocate that folks use the subclass until the superclass is widely available or they'll get spammed with warnings. This affects splitting out Semigroup from Monoid.

A similar issue arises with the MonadFail proposal as there is a stage around 8.4 where the 3-release-compatible advice would be to incur a MonadFail constraint, even when the active desguaring will be through Monad.

I don't actually have a fix from the libraries side for the fact that with these showing up in -Wall, the 3 release policy doesn't work.

Every single proposal the core libraries committee currently has in the works (except for adding log1p, expm1, etc. to Floating) is affected by this concern.

One possible fix is to demote the redundant superclass warnings out of -Wall.

This would resolve #11369 as well.

Change History (36)

comment:1 Changed 4 years ago by ekmett

Component: CompilerCompiler (Type checker)

comment:2 Changed 4 years ago by nomeata

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.

comment:3 Changed 4 years ago by simonpj

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

comment:4 Changed 4 years ago by hvr

Cc: quchen added

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)

comment:5 Changed 4 years ago by gershomb

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.

comment:6 Changed 4 years ago by quchen

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.

comment:7 Changed 4 years ago by ekmett

I think Herbert was leaning towards -Weverything.

comment:8 Changed 4 years ago by ekmett

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

1) Just remove it from the default constraint set. This is the minimum we should do.

2) 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.

comment:9 in reply to:  8 Changed 4 years ago by hvr

Replying to 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 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 -- 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

Last edited 4 years ago by hvr (previous) (diff)

comment:10 Changed 4 years ago by simonpj

2) 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

comment:11 Changed 4 years ago by ekmett

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 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.

comment:12 Changed 4 years ago by gershomb

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.

comment:13 Changed 4 years ago by simonpj

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!

comment:14 Changed 4 years ago by gershomb

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

comment:15 in reply to:  13 Changed 4 years ago by simonpj

Replying to 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

comment:16 Changed 4 years ago by bgamari

Owner: set to bgamari

comment:17 Changed 4 years ago by gershomb

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.

comment:18 Changed 4 years ago by simonpj

Thanks Gershom. I have revised Design/Warnings to express comment:10 too.

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

comment:19 Changed 4 years ago by bgamari

One open question here is how this should interact with #10632, 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.

comment:20 Changed 4 years ago by Ben Gamari <ben@…>

In 8e9a870/ghc:

Remove -Wredundant-superclasses from standard warnings

It is impossible to write warning-free code under the three-release
policy with this flag enabled by default. See #11370 for details.

comment:21 Changed 4 years ago by bgamari

Status: newmerge

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

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

Last edited 4 years ago by bgamari (previous) (diff)

comment:22 Changed 4 years ago by thomie

Type of failure: None/UnknownIncorrect warning at compile-time

comment:23 Changed 4 years ago by bgamari

Resolution: fixed
Status: mergeclosed

comment:24 Changed 4 years ago by simonmar

Owner: bgamari deleted
Resolution: fixed
Status: closednew

-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.

comment:25 Changed 4 years ago by ekmett

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.

comment:26 Changed 4 years ago by nomeata

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.

comment:27 Changed 4 years ago by ekmett

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.

comment:28 Changed 4 years ago by gershomb

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.

comment:29 Changed 4 years ago by simonmar

@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.

comment:30 Changed 4 years ago by nomeata

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 _.

comment:31 Changed 4 years ago by simonmar

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.

comment:32 Changed 4 years ago by simonmar

Owner: set to bgamari

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.

comment:33 Changed 4 years ago by ekmett

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.

comment:34 Changed 4 years ago by bgamari

Differential Rev(s): Phab:D1956
Status: newpatch

comment:35 Changed 4 years ago by Ben Gamari <ben@…>

In 2e49a316/ghc:

DynFlags: Add -Wredundant-constraints to -Wall

Test Plan: It works, I promise.

Reviewers: austin

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D1956

GHC Trac Issues: #11370

comment:36 Changed 4 years ago by bgamari

Resolution: fixed
Status: patchclosed

Merged to ghc-8.0.

Note: See TracTickets for help on using tickets.