Opened 23 months ago

Last modified 8 months ago

#11369 new bug

Suppress redundant-constraint warnings in case of empty classes

Reported by: hvr Owned by:
Priority: normal Milestone: 8.4.1
Component: Compiler (Type checker) Version: 8.1
Keywords: Cc: edsko, bgamari, ekmett, RyanGlScott
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #11370 Differential Rev(s):
Wiki Page:

Description

The technique described in

http://www.well-typed.com/blog/2015/07/checked-exceptions/

makes use of a class without any methods, i.e.

class Throws e

However, this by definition results in redundant constraints, e.g.

readFoo :: Throws IOError => FilePath -> IO Foo
readFoo fn = {- ...  -}

which GHC 8.0 warns about.

I propose to suppress warnings in case a class which has no methods is used as seemingly redundant constraint.

Change History (20)

comment:1 Changed 23 months ago by edsko

We could probably work around this by adding a dummy class method in Throws and call that in throwChecked.

(But I think there are other use cases of method-less classes out there in the wild :)

comment:2 Changed 23 months ago by simonpj

I'd be ok with that. But I suggest suppressing only if it has no methods and no superclasses.

comment:3 Changed 23 months ago by ekmett

Note: Suppressing only in the absence of superclasses means that any class constraint that just gives you a law will always spring this warning.

class Bifunctor p => Braided p where
  braid :: p a b -> p b a

-- | @braid . braid = id@
class Braided p => Symmetric p

swap :: Symmetric p => p a b -> p b a
swap = braid

(This code is slightly simplified from http://hackage.haskell.org/package/categories-1.0.7/docs/Control-Category-Braided.html)

One "solution" here is to move swap into the Symmetric class, but then you need to worry about it being overridden to be different than braid, which operationally can't happen in the current approach.

comment:4 Changed 23 months ago by ekmett

comment:5 Changed 23 months ago by ekmett

Cc: ekmett added

comment:6 Changed 23 months ago by edsko

I wonder if some kind of pragma would make more sense than trying to come up with a clever heuristic.

comment:7 Changed 23 months ago by simonpj

Indeed, a pragma would be good.

If someone is going to add class-specific pragmas, making UndecidableSuperClasses work on a per-class (rather than per-module) basis would be good.

Simon

comment:8 Changed 23 months ago by rwbarton

Priority: highestnormal

comment:9 Changed 23 months ago by rwbarton

Demoting priority since there is an obvious workaround here: disable the warning in the module that defines unthrow.

comment:10 Changed 22 months ago by RyanGlScott

Cc: RyanGlScott added

comment:11 Changed 22 months ago by bgamari

If someone is going to add class-specific pragmas, making UndecidableSuperClasses work on a per-class (rather than per-module) basis would be good.

What would this look like? Any class in the cycle being marked with the pragma would allow undecidable superclasses?

If we are going to do this we may want to consider doing it before the 8.0 release. Moving from per-module to per-class modules after the per-class pragmas are in the wild may be rather painful (judging by the move from OverlappingInstances to {-# OVERLAPPING #-}, although perhaps UndecidableSuperClasses won't see as wide use).

comment:12 Changed 22 months ago by simonpj

What would this look like? Any class in the cycle being marked with the pragma would allow undecidable superclasses?

This issue is when we report a class cycle. If any class in the cycle has a "UndecidableSuperClasses" pragma, then yes, I suggest we do not warn.

It'd be great to put this per-class stuff in 8.0, but someone would have to step up and implement it.

comment:13 Changed 22 months ago by thomie

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

comment:14 Changed 20 months ago by simonpj

Owner: simonpj deleted

comment:15 Changed 20 months ago by bgamari

Milestone: 8.0.18.0.2

Bumping to 8.0.2.

comment:16 Changed 15 months ago by bgamari

Milestone: 8.0.28.2.1

Sadly this won't be happening for 8.0.2.

comment:17 Changed 13 months ago by RyanGlScott

Another use case for this emerged in #12810. Now that we can use GND for deriving instances of classes with associated type families, it's quite easy to trigger -Wredundant-constraints with code like this:

{-# LANGUAGE GeneralizedNewtypeDeriving, TypeFamilies #-}
class C a where
  type T a

newtype Identity a = Identity a deriving C

as it will generate the following instance:

instance C a => C (Identity a) where
  type T (Identity a) = T a

and GHC will emit this warning:

• Redundant constraint: C a
• In the instance declaration for ‘C (Identity a)’

Now we have examples where GHC-generated code is producing warnings, which is disconcerting.

comment:18 Changed 13 months ago by simonpj

I think this is just a bug in the code that infers the context for the instance declaration that arises from GND. We should tet

instance {- Empty context!! -} C (Identity a) where
  type T (Identity a) = T a

Where is that code? I can help with this after the PLDI deadline.

Anyway, I think this is unrelated to the original topic of the ticket.

comment:19 Changed 13 months ago by RyanGlScott

simonpj, I've opened #12814 to discuss the design you propose in comment:18.

comment:20 Changed 8 months ago by bgamari

Milestone: 8.2.18.4.1

Given that 8.2.1-rc1 is imminent, I'm bumping these off to the 8.4

Note: See TracTickets for help on using tickets.