Opened 2 years ago

Closed 11 months ago

Last modified 6 months ago

#10635 closed feature request (fixed)

-fwarn-redundant-constraints should not be part of -Wall

Reported by: Lemming Owned by:
Priority: highest Milestone: 8.0.2
Component: Compiler (Type checker) Version: 7.11
Keywords: Cc: mboes, edgar.zhavoronkov, RyanGlScott
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #9939, #9973, #10100, #10183, #11370 Differential Rev(s): Phab:D2498
Wiki Page:

Description

When I compile existing code with GHC-7.11.20150707 I get lot of "redundant constraints" warnings. Generally I think that this warning can be very useful for minimizing constraints, nonetheless there are good reasons not choose minimal constraints for a type signature. E.g. if I have to implement Data.Map.singleton I do not need the Ord k constraint for the key type. However, I might add it anyway in order to be able to change the implementation in future such that it uses Ord k dictionary.

Thus I suggest to exclude -fwarn-redundant-constraints from -Wall.

Change History (38)

comment:1 Changed 2 years ago by Lemming

comment:2 Changed 18 months ago by mboes

Cc: mboes added

I disagree.

Here's a data point for you to take into consideration. At Tweag I/O we've had at least one instance of a serious bug that shipped into production and several days worth of wasted effort, because a constraint maintaining a key invariant was mistakenly unused. -fwarn-redundant-constraints would of caught this bug very early and saved us all the trouble (a test would have worked too, but as it happens a test did exist but had a bug too).

If this warning is not turned on with -Wall, then we'd have to discover somehow that there is this extra flag that we should pass in even though we already said -Wall, to really get all warnings, including one that could be life saving.

I think -Wall should do what it says on the tin: include all warnings. That many such warnings may be cause for false positives is why we have -Wall not turned on by default in the first place.

comment:3 Changed 18 months ago by Lemming

I understand -Wall as the option that allows reasonably designed code to pass without warnings. There are good reasons to have redundant constraints as I sketched in the ticket description. It's crucial to be able to make code -Wall warning-free otherwise relevant warnings are easily overlooked in a flood of irrelevant warnings. According to my programming style I often add e.g. -fwarn-incomplete-uni-patterns -fwarn-tabs -fwarn-missing-import-lists. They match my style of programming but seem to interfere with other's style, thus they are not part of -Wall.

comment:4 Changed 18 months ago by bgamari

Lemming, Perhaps it would help have introduce a mechanism for telling the compiler "I intentionally do not use this constraint" on either a per-context or per-class basis?

comment:5 in reply to:  4 Changed 18 months ago by Lemming

Replying to bgamari:

Lemming, Perhaps it would help have introduce a mechanism for telling the compiler "I intentionally do not use this constraint" on either a per-context or per-class basis?

On the one hand, I would say, Yes, because we have ways to flag intentionally unused imports and unused identifiers and that works nicely. On the other hand, I wonder whether we can treat constraints and imports and identifiers the same way. What does it mean at all to have a redundant constraint? E.g.

asTypeOf :: a -> a -> a
asTypeOf x _ = x

has not the most general type. Is this a redundant constraint? I can write it more explicitly:

asTypeOf1 :: (a ~ b) => a -> b -> a
asTypeOf1 x _ = x

GHC-8.0 does not warn about any of these two. Should it?

comment:6 Changed 18 months ago by bgamari

GHC-8.0 does not warn about any of these two. Should it?

Indeed GHC 8.0 warns that the a ~ b constraint is redundant. This is arguably wrong and I've opened #11582 for this case. This is almost certainly not the last example of this sort that we will find.

However, I suspect that -Wredundant-constraints will help more users in the situation that mboes describes than it would hurt users with the cases you describe if enabled in -Wall. It doesn't seem unreasonable to me to require users writing code like asTypeOf1 to explicitly disable this warning. At the moment this can only be done on with module-level granularity but hopefully in the future we will have mechanisms for silencing warnings with greater precision.

Regardless, your points have demonstrated that there are some tricky issues here. It would be helpful if you could collect these thoughts on Design/Warnings/RedundantConstraints.

comment:7 in reply to:  6 Changed 18 months ago by Lemming

Replying to bgamari:

GHC-8.0 does not warn about any of these two. Should it?

Indeed GHC 8.0 warns that the a ~ b constraint is redundant. This is arguably wrong and I've opened #11582 for this case. This is almost certainly not the last example of this sort that we will find.

I just re-checked: ghci-8.0.0.20160214 warns with -fwarn-redundant-constraints but not with -Wall. That is, -fwarn-redundant-constraints is no longer part of -Wall? But this is the subject of this ticket.

comment:8 in reply to:  6 ; Changed 18 months ago by Lemming

Replying to bgamari:

It doesn't seem unreasonable to me to require users writing code like asTypeOf1 to explicitly disable this warning. At the moment this can only be done on with module-level granularity but hopefully in the future we will have mechanisms for silencing warnings with greater precision.

If would prefer to disable the warning per constraint. E.g. I could wrap a not strictly needed constraint in a magic Used :: Constraint -> Constraint. However, this would not be possible in Haskell 98.

comment:9 in reply to:  8 Changed 17 months ago by Lemming

Replying to Lemming:

If would prefer to disable the warning per constraint. E.g. I could wrap a not strictly needed constraint in a magic Used :: Constraint -> Constraint. However, this would not be possible in Haskell 98.

A Haskell 98 compliant pragma might look like this:

{-# MINIMAL_CONSTRAINTS singleton () #-}
singleton :: Ord a => a -> Set a

The pragma means: Type check singleton two times, once with the constraints () and once with the constraint Ord a. Warn about redundant constraints in the pragma but not in the signature of singleton.

If you also want to warn about not maximally general types, then the pragma should even mention the whole type signature, e.g.

{-# MOST_GENERAL singleton :: a -> Set a #-}
singleton :: Ord a => a -> Set a

{-# MOST_GENERAL asTypeOf :: a -> b -> a #-}
asTypeOf :: a -> a -> a
Last edited 17 months ago by Lemming (previous) (diff)

comment:10 Changed 17 months ago by simonpj

I think it might be simpler just to say

{-# NO_WARN_REDUNDANT_CONSTRAINTS singleton #-}

or something of that ilk. Or put it in the signature itself

singleton :: {-# NO_WARN_REDUNDANT_CONSTRAINTS #-}
             Ord a => a -> Set a

comment:11 in reply to:  10 Changed 17 months ago by Lemming

Replying to simonpj:

I think it might be simpler just to say

{-# NO_WARN_REDUNDANT_CONSTRAINTS singleton #-}

My goal was to somehow mark the redundant constraints, as we do with unused identifiers. I would not like to let the type checker ignore all redundant constraints at once in a signature.

comment:12 Changed 17 months ago by simonpj

My goal was to somehow mark the redundant constraints, as we do with unused identifiers. I would not like to let the type checker ignore all redundant constraints at once in a signature.

I hear you. But as compiler writers we have a limited complexity budget and number of implementation cycles, and we need to think how best to spend them in the service of our users. Both in the initial cost of design and implementation and in the ongoing cost of maintenance. I think what you want is hard, so there's a risk of making the best be the enemy of the good.

(By all means try; I'm just saying.)

comment:13 in reply to:  12 Changed 17 months ago by Lemming

Replying to simonpj:

I hear you. But as compiler writers we have a limited complexity budget and number of implementation cycles, and we need to think how best to spend them in the service of our users. Both in the initial cost of design and implementation and in the ongoing cost of maintenance. I think what you want is hard, so there's a risk of making the best be the enemy of the good.

Bgamari suggested a general mechanism like Design/LocalWarningPragmas that would certainly make NO_WARN_REDUNDANT_CONSTRAINTS unnecessary.

comment:14 Changed 17 months ago by hvr

For the record, in comment:ticket:11370:32 it was decided to have -Wredundant-constraint included in -Wall

comment:15 Changed 14 months ago by acfoltzer

Now that I'm getting some hands on 8.0.1 experience, I'm pretty quickly running into situations where I need to disable this warning. I'm inclined to agree with the suggestion to take it out of -Wall, but being able to mark certain instances and classes as exempt from the warning would work for my purposes.

I also wonder if it might make sense to distinguish between certain types of redundancy: for example, in our code a redundancy arising from a superclass relationship (e.g., (Functor f, Applicative f)) is almost always something we want to fix, but unused classes are almost always an API design choice rather than a bug we wish to avoid.

I'm not sure where mboes' example falls between these cases, but maybe distinguishing the two cases and only including the superclass redundancy in -Wall could make folks happier?

comment:16 in reply to:  15 Changed 14 months ago by Lemming

Replying to acfoltzer:

I also wonder if it might make sense to distinguish between certain types of redundancy: for example, in our code a redundancy arising from a superclass relationship (e.g., (Functor f, Applicative f)) is almost always something we want to fix, but unused classes are almost always an API design choice rather than a bug we wish to avoid.

I'm not sure where mboes' example falls between these cases, but maybe distinguishing the two cases and only including the superclass redundancy in -Wall could make folks happier?

I would prefer that.

comment:17 in reply to:  15 ; Changed 14 months ago by mboes

Replying to acfoltzer:

I also wonder if it might make sense to distinguish between certain types of redundancy: for example, in our code a redundancy arising from a superclass relationship (e.g., (Functor f, Applicative f)) is almost always something we want to fix, but unused classes are almost always an API design choice rather than a bug we wish to avoid.

Our particular bug (and I think there are many situations like it), fell in the latter category (it was a mistakenly unused constraint). If only the former category is included in -Wall, then -Wall will be useless to us and others to catch similar bugs in the future. We'll have to remember to -Wall -Wsome-other-warning-flag in all projects.

I think what would be nice though is if there existed a pragma to mark a constraint as explicitly redundant, for those who want that. In general, it's nicer to be able to specify things at fine granularity - disabling redundant constraint checking at the module level is too coarse a granularity. Probably a matter for a separate ticket.

comment:19 Changed 14 months ago by simonpj

The proposal on the thread is

  • -Wredundant-constraints reports when a user writes a constraint that is fully equivalent to some other, strictly smaller constraint, like suggesting simplifying (Eq a, Ord a) to (Ord a).
  • -Wtype-overly-specific warns about any type signature that's more specific than it needs to be. That would include both

(a) When a constraint is specified but not used

f :: Eq a => a -> a
f x = x

(b) When a type variable is instantiated to something more specific than necessary

g :: [a] -> [a]
g x = x

That sounds attractive but (b) isn't an easy fit with the current implementation. GHC treats a type signature as normative and "pushes it inward". So when type checking

g :: [a] -> [a]
g x = x && True

we immediately given an error "can't unify [a] with Bool". We do not infer the most general type of g, namely g :: Bool -> Bool, and then compare with the signature.

So I don't know an easy way to do (b). But (a) would be fine. Patches welcome.

comment:20 Changed 14 months ago by acfoltzer

Replying to simonpj:

I would be happy with this proposal, but given the difficulty of (b) I wonder if it's realistic in the short term. A -Wtype-overly-specific that only checks for unused constraints would be quite confusing.

A pairing of -Wredundant-constraints as described in Richard's proposal, and a more narrowly-scoped -Wunused-constraints would be more straightforward to implement, but the hair-splitting between "redundant" and "unused" might be a bit much for someone who's not in the weeds already.

Replying to mboes:

Our particular bug (and I think there are many situations like it), fell in the latter category (it was a mistakenly unused constraint).

Could you say a bit more about what this case looks like? I can't recall ever hitting something like it, but it's sounding like I probably have and just haven't noticed.

I think what would be nice though is if there existed a pragma to mark a constraint as explicitly redundant, for those who want that. In general, it's nicer to be able to specify things at fine granularity - disabling redundant constraint checking at the module level is too coarse a granularity. Probably a matter for a separate ticket.

+1; this would be valuable even if -Wunused-constraints gets lifted out of -Wall.

comment:21 in reply to:  20 Changed 14 months ago by goldfire

Replying to acfoltzer:

the hair-splitting between "redundant" and "unused" might be a bit much for someone who's not in the weeds already.

Sometimes I feel like I live in the weeds, but I can't actually figure out which warning on the table is "redundant" and which is "unused". -Wconstraint-overly-specific? I don't like that name much myself, though.

comment:22 in reply to:  17 Changed 14 months ago by thomie

Cc: edgar.zhavoronkov added

Replying to mboes:

I think what would be nice though is if there existed a pragma to mark a constraint as explicitly redundant

See #602 (opened 12 years ago!).

comment:23 Changed 12 months ago by konn

I need more flexible control on -Wredundant-constraints, too. I'm currently maintaining library to provide algebraic structures. In such a situation, we need empty class without any member function to express the additional algebraic constraint.

Suppose we have following classes:

class Multiplicative r where
  (*) :: r -> r -> r

class Commutative r -- without any member functions!

Until here, everything goes well. Suppose we want to write algorithm relying on the commutativity of multiplication:

myGreatAlgorithm :: (Commutative r) => r -> r
myGreatAlgorithm = ...

Then GHC warns about Commutative r as a "redundant constraint", although it is essentially needed to work algorithm properly. Since Commutative class doesn't have any member functions, workaround adding "dead code", which is introduced in GHC users' guide, doesn't work at all.

comment:24 Changed 12 months ago by Lemming

I have another example: The package llvm-tf (derived from llvm) uses type classes without methods in order to formalize the type restrictions set by the LLVM framework. E.g. IsPrimitive is the class for types of elements of vectors, IsArithmetic is the class for types supporting addition, multiplication and so on. However, only their super class IsType has a method typeDesc :: Proxy a -> TypeDesc that returns the LLVM structure for the according type. The LLVM function for addition takes this TypeDesc and it could in principle hold a non-Arithmetic type. We exclude this using the IsArithmetic class, but there is no IsArithmetic method to call that would force us to restrict the LLVM additon function to Arithmetic types.

comment:25 Changed 12 months ago by ross

I would like to see this turned off for exported functions, like the Data.Map.singleton example. Avoiding this warning leads to implementation details leaking from the abstraction.

comment:26 Changed 12 months ago by simonpj

It makes some sense to warn, by default, when a signature is less polymorphic than it could be. After all, it's quite possible that the programmer didn't realise that the function could be more polymorphic. And indeed I found half a dozen cases of redundant constrains in the base package when I first implemented it.

But

  • -fwarn-redundant-constraints only does part of the job. It does not warn when you use a tighter constraint than necessary, or indeed instantiate what could be a polymorphic variable to a specific type.
  • People have argued persuasively that they sometimes want a less polymorphic type.

So:

  • It looks as if we should take it out of -Wall. Would someone like to do that, it it hasn't happened already?
  • I'd be open to a design (and a patch) making it possible to say that a specific type signature deliberately has redundant constraint.

Simon

comment:27 Changed 12 months ago by konn

I think that -fwarn-redundant-constraints should be still a part of -Wall, but we need some way to suppress warning for intensionally tightened constraints.

In such a viewpoint, I like something like Simon's {-# NO_WARN_REDUNDANT_CONSTRAINTS #-} annotation. I think it's likely to happen that, in the same function, some constraint is intentionally tightened but other is not. So I think that it's ideal that one can specify necessity for each constraint.

Example:

flipAdd :: (Multiplicative r, {-# NON_REDUNDANT #-} Abelian r) => r -> r -> r
flipAdd x y = x + y

Compiler should warn Multiplicative r as redundant since it doesn't use any multiplication, but Abelian r should be unwarned because it's marked as non-redundant.

comment:28 in reply to:  27 Changed 12 months ago by Lemming

Replying to konn:

Example:

flipAdd :: (Multiplicative r, {-# NON_REDUNDANT #-} Abelian r) => r -> r -> r
flipAdd x y = x + y

Compiler should warn Multiplicative r as redundant since it doesn't use any multiplication, but Abelian r should be unwarned because it's marked as non-redundant.

I like this approach, too, but I would prefer a positive attribute, e.g. {-# REQUIRED #-} or {-# INTENDED #-}.

comment:29 Changed 12 months ago by svenpanne

Just a word of warning from a library maintainer's point of view: Including new flags into -Wall puts some non-trivial burden onto maintainers wanting a warning-free build. Simply adding a new pragma is not enough: Old GHCs don't know that pragma, so you have to put that into some ugly #ifdefs, which in turn oftern implies adding another (language) pragma to allow the preprocessor plus perhaps some changes in the .cabal file. So including -fwarn-redundant-constraints in -Wall is far from free for real-world code.

I'm not saying that this shouldn't be done, but at least I want to bring up the maintenance issue. From my personal experience, Haskell code is becoming more and more cluttered with CPP stuff because of seemingly innocent changes like this. Of course this isn't a problem if you only consider the latest and greatest GHC, but this isn't an option for most maintainers.

comment:30 in reply to:  29 ; Changed 12 months ago by Lemming

Replying to svenpanne:

Just a word of warning from a library maintainer's point of view: Including new flags into -Wall puts some non-trivial burden onto maintainers wanting a warning-free build. Simply adding a new pragma is not enough: Old GHCs don't know that pragma, so you have to put that into some ugly #ifdefs, which in turn oftern implies adding another (language) pragma to allow the preprocessor plus perhaps some changes in the .cabal file.

I don't like CPP and would prefer that GHC gets a switch to disable warnings about certain pragmas. This was proposed in #2867. It should be possible to disable those warnings globally in the Cabal file.

comment:31 in reply to:  30 Changed 12 months ago by svenpanne

Replying to Lemming:

I don't like CPP and would prefer that GHC gets a switch to disable warnings about certain pragmas. This was proposed in #2867. It should be possible to disable those warnings globally in the Cabal file.

While this might be a worthwhile goal, it doesn't help with the current situation at all: Current and past GHCs don't have that feature, so even if this is implemented, we will have to live with CPP for this purpose for quite a few years to come...

comment:32 Changed 11 months ago by acfoltzer

In light of Ben's last call for 8.0.2 patches, would it be feasible to get one together in time to remove the warning from -Wall? It seems like we reached a tentative decision to do so.

I'd be happy to take a look myself, but if time is of the essence I might not be able to get a development environment up and running quickly.

comment:33 Changed 11 months ago by simonpj

Milestone: 8.0.2
Priority: normalhighest

In comment:26 I suggest taking it out of -Wall. That is simple and low risk. If it's causing pain having it in, let's just do that.

Ben: can you execute? If we consider putting it into -Wall as a bug, this is a bug-fix :-). I'll milestone as 8.0.2/highest to make sure it gets done.

More elaborate solutions could be possible (see thread above), but not for 8.0.2.

Simon

comment:34 Changed 11 months ago by acfoltzer

Differential Rev(s): Phab:D2498

I've just added a patch for 8.0.2. I definitely agree that we want more elaborate solutions in the future, or at least ways to recover the parts of the warning that don't cause as many folks pain.

comment:35 Changed 11 months ago by RyanGlScott

Cc: RyanGlScott added

comment:36 Changed 11 months ago by Ben Gamari <ben@…>

In e9b0bf4e/ghc:

Remove redundant-constraints from -Wall (#10635)

Removes -Wredundant-constraints from -Wall, as per the
discussion in #10635.

Reviewers: austin, bgamari

Reviewed By: bgamari

Subscribers: thomie

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

GHC Trac Issues: #10635

comment:37 Changed 11 months ago by bgamari

Resolution: fixed
Status: newclosed

comment:38 Changed 6 months ago by Joachim Breitner <mail@…>

In 5ce39f6/ghc:

Add Wredundant-constraints to list of flags excluded from -Wall

In light of #10635
Note: See TracTickets for help on using tickets.