Opened 2 years ago

Last modified 14 months ago

#13766 new bug

Confusing "redundant pattern match" in 8.0, no warning at all in 8.2

Reported by: edsko Owned by:
Priority: normal Milestone:
Component: Compiler Version: 8.0.1
Keywords: PatternMatchWarnings Cc: Iceland_jack, mpickering
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #12466, #11066, #12694 Differential Rev(s):
Wiki Page:

Description

I couldn't find an existing ticket about this, so I figured I'd file this even though it's probably an instance of a more general problem. Consider

{-# LANGUAGE TypeFamilies #-}
{-# OPTIONS_GHC -Wall -Woverlapping-patterns #-}

module T where

class C a where
  c :: a -> a

instance Int ~ Bool => C Int where
  c = id

yields

T.hs:10:3: warning: [-Woverlapping-patterns]
    Pattern match is redundant
    In an equation for ‘c’: c = ...

which I suppose makes _some_ amount of sense but is highly confusing nonetheless :)

Now in 8.2 I don't get this warning, but in fact I don't get any warning at all, which I'm not entirely sure is better. In the real code obviously the superclass constraint was far more complicated and it was nice of ghc to warn me (albeit in a roundabout way) that it was unsatisfiable.

Change History (7)

comment:1 Changed 2 years ago by Iceland_jack

Cc: Iceland_jack added

comment:2 Changed 2 years ago by edsko

Summary: Confusing "reudundant pattern match" in 8.0, no warning at all in 8.2Confusing "redundant pattern match" in 8.0, no warning at all in 8.2

comment:3 Changed 2 years ago by RyanGlScott

Cc: mpickering added

Hoo boy, that's pretty scary. To make it worse, because of this bug you can write blatantly nonsensical things like this:

{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE TypeFamilies #-}
{-# OPTIONS_GHC -Wall -Woverlapping-patterns #-}

module T where

class C a where
  c :: a

instance (a ~ Bool, a ~ Int) => C a where
  c = 42 + True

And it'll compile on GHC 8.2.1-rc2 with no warnings. (I haven't found a way to actually use that bogus c definition, but still.)

The commit that caused this behavior is adb565aa74582969bbcc3b411d6d518b1c76c3cf (Don't return empty initial uncovered set for an unsat context). Matthew, do you know what is going on here?

comment:4 Changed 2 years ago by ezyang

This looks related to some changes to inaccessibility which happened for GHC 8.2. There's more context at #12466, #11066 and #12694, and possibly more (do a search for "inaccessible")

comment:5 Changed 2 years ago by mpickering

This is the expected behaviour, the commit message explains the reasoning.

Previously when the checker encountered an unsatisfiable term of type
context it would return an empty initial uncovered set. This caused all
pattern matches in the context to be reported as redudant.

This is arguably correct behaviour as they will never be reached but it
is better to recover and provide accurate warnings for these cases to
avoid error cascades. It would perhaps be better to report an error to
the user about an inacessible branch but this is certainly better than
many confusing redundant match warnings.

Warning that the RHS is inaccessible would be valuable but I don't think it's the domain of the pattern match checker to warn indirectly of this by warning of "redundant" clauses. So it looks like the tickets that Edward linked are more relevant.

comment:6 Changed 2 years ago by RyanGlScott

I talked with mpickering about this privately, but I'll recap the conversation here as well.

This may be expected behavior the sense of comment:5, but I still find it unsettling. The code that adb565aa74582969bbcc3b411d6d518b1c76c3cf targeted was of the form:

f = case [] of (_:_) -> case () of
                          a -> undefined

Here, it'll warn that the (_:_) match is redundant:

    Pattern match is redundant
    In a case alternative: (_ : _) -> ...

And thanks to the aforementioned commit, it will not warn about the inaccessible a -> undefined case. This is definitely a Good Thing.

A similar scenario arises in the code in this example:

instance Int ~ Bool => C Int where
  c = id

Because Int ~ Bool is insoluble (and thus all the code inside the instance is inaccessible), it won't bother printing out a warning that c is inaccessible. But this is a tad skeevy. In the former example, there was a separate warning that hinted that you were doing something questionable. In the latter example, however, there's no warning at all to point out the dubious nature of your code!

Therefore, I think a satisfactory conclusion to this bug would be to come up with a suitable warning about the Int ~ Bool constraint. Whether that's the purview of #12694 or some other ticket, I'm not sure.

comment:7 Changed 14 months ago by RyanGlScott

Keywords: PatternMatchWarnings added
Note: See TracTickets for help on using tickets.