#14773 closed bug (fixed)

MultiWayIf makes it easy to write partial programs that are not catched by -Wall

Reported by: SimonHengel Owned by: sighingnow
Priority: normal Milestone: 8.6.1
Component: Compiler Version: 8.2.2
Keywords: PatternMatchWarnings Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: T14773a,T14773b
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D4400
Wiki Page:

Description

The following partial function will pass -Wall

-- program 1
{-# LANGUAGE MultiWayIf #-}
foo :: Bool -> Int
foo b = if | b -> 23

While the following two alternatives will not:

-- program 2
foo :: Bool -> Int 
foo b | b = 23
-- program 3
foo :: Bool -> Int
foo b = case () of
  () | b -> 23

Note that the GHC User's Guide states that "program 1" and "program 3" are equivalent.

  1. Is this a bug or by design?
  2. I guess at the very least we would want to update the User's Guide

Change History (14)

comment:1 Changed 20 months ago by simonpj

I think it's a bug, or at least an inconsistency. Would someone like to fix it? I doubt it's difficult.

comment:2 Changed 20 months ago by RyanGlScott

Keywords: PatternMatchWarnings added

comment:3 Changed 20 months ago by RyanGlScott

Sigh. I don't see an easy way to fix this. The issue is that the entry-point into the pattern match coverage checker, checkMatches, requires a list of LPats as its arguments. However, the AST for multi-way-if (HsMultiIf) has GRHSes, not LPats. That's the same reason why this program doesn't emit any warnings under -Wall:

b :: Bool
Just b | False = Nothing

comment:4 Changed 20 months ago by simonpj

The pattern-match overlap checker can do some simple things with guards, if memory serves; but it would indeed need a new entry point.

At the moment we lack anyone with the time and inclination to pay attention to the pattern-match overlap checker. There's a great paper about it, but the actual implementation needs love and care.

comment:5 Changed 20 months ago by jmct

Is this the "GADTs Meet Their Match" (great title) paper?

I can try to take this on if no one else has cycles for it.

comment:6 Changed 20 months ago by simonpj

Yes, that's the paper!

comment:7 Changed 20 months ago by RyanGlScott

On second thought, I was too hasty in my earlier assessment—this might be more tractable than I thought. It turns out that when we're checking patterns from function bindings (as in program 2 above), we don't directly call checkMatches, but instead go through an auxiliary function called matchWrapper. Instead of taking a list of LPats an an argument, matchWrapper takes a MatchGroup, and sets up the necessary scaffolding to feed that into checkMatches.

Now, we can't directly feel an HsMultiIf's guards into matchWrapper, since HsMultiIf has an LGRHS instead of a MatchGroup. However, if we look at the definition of MatchGroup (and (L)Match):

data MatchGroup p body
  = MG { mg_alts    :: Located [LMatch p body]
       , mg_arg_tys :: [PostTc p Type]
       , mg_res_ty  :: PostTc p Type
       , mg_origin  :: Origin }

type LMatch id body = Located (Match id body)

data Match p body
  = Match {
        m_ctxt :: HsMatchContext (NameOrRdrName (IdP p)),
        m_pats :: [LPat p], -- The patterns
        m_grhss :: (GRHSs p body)
  }

We can see that we're actually very close to having what we need, since we can slide the GRHS right into a Match, and put that into the mg_alts of a MatchGroup. The trick is to then come up with suitable things to put in the remaining fields of MatchGroup and Match.

comment:8 in reply to:  7 Changed 20 months ago by sighingnow

Owner: set to sighingnow

I already have some tries on this. We can construct the LMatch manually in ds_expr, the use checkMatches directly, without feed to matchWrapper. We can do some like this:

ds_expr _ (HsMultiIf res_ty alts)
  | null alts
  = mkErrorExpr

  | otherwise
  = do { match_result <- liftM (foldr1 combineMatchResults)
                               (mapM (dsGRHS IfAlt res_ty) alts)
       ; dflags <- getDynFlags
       ; vanillaId <- mkPmId boolTy
       ; let vanillaLPat = mkLHsVarPatTup [vanillaId]
             matches = [ L (getLoc pattern) $
                            Match { m_ctxt = IfAlt
                                  , m_pats = [vanillaLPat]
                                  , m_grhss = GRHSs [pattern] (noLoc emptyLocalBinds) }
                                  -- mkSimpleMatch IfAlt [vanillaLPat] m
                       | pattern@(L _ (GRHS p m)) <- alts]
       ; checkMatches dflags dsMatchContext [vanillaId] matches
       
       ; error_expr   <- mkErrorExpr
       ; extractMatchResult match_result error_expr }
  where
    mkErrorExpr = mkErrorAppDs nON_EXHAUSTIVE_GUARDS_ERROR_ID res_ty
                               (text "multi-way if")
    
    combinedLoc = foldr1 combineSrcSpans (map getLoc alts)
    dsMatchContext = DsMatchContext IfAlt combinedLoc

Patch coming!

comment:9 Changed 20 months ago by sighingnow

I'm confused with the following comments at Check.hs: https://ghc.haskell.org/trac/ghc/browser/ghc/compiler/deSugar/Check.hs#L1904

1904    exhaustiveWarningFlag (StmtCtxt {}) = Nothing -- Don't warn about incomplete patterns
1905                                           -- in list comprehensions, pattern guards
1906                                           -- etc. They are often *supposed* to be
1907                                           -- incomplete

Edit:

Is the incomplete pattern matching warning of False in RyanGlScott's example ignored by design ?

b :: Bool
Just b | False = Nothing

When compile it with -Wall -Wincomplete-uni-patterns, I think we should get the following two warnings:

T.hs:47:1: warning: [-Wincomplete-uni-patterns]^^^^^^^^^^^^^^^^^^^^^^^

    Pattern match(es) are non-exhaustive
    In a pattern binding: Patterns not matched: Nothing
   |
47 | (Just b) | False = Nothing
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^

T.hs:47:10: warning: [-Wincomplete-patterns]^^^^^^^^^^^^^^^^^^^^^^^

    Pattern match(es) are non-exhaustive
    In a pattern binding in
         pattern guard for
         a pattern binding:
        Patterns not matched: _
   |
47 | (Just b) | False = Nothing
   |          ^^^^^^^^^^^^^^^^^

Note that -Wincomplete-uni-patterns is not implied by -Wall.

Last edited 20 months ago by sighingnow (previous) (diff)

comment:10 Changed 20 months ago by sighingnow

Differential Rev(s): Phab:D4400
Status: newpatch
Test Case: T14773a,T14773b

comment:11 Changed 20 months ago by simonpj

Why do you need vanillaLPat? Why not just have an empty list of patterns?

Also

b :: Bool
Just b | False = Nothing

No that is not ignored by design. The thing that is ignored is this:

  [ e | False <- blah, blah blah ]
and
  do { False <- blah; ... }

I hope that StmtCxt is true of the latter two but not of the former.

comment:12 in reply to:  11 Changed 20 months ago by sighingnow

Replying to simonpj:

Why do you need vanillaLPat? Why not just have an empty list of patterns?

I have misunderstood the m_pats field of Match and the vars arguments of checkMatches. I'll fix it.

I hope that StmtCxt is true of the latter two but not of the former.

Now I see that we can't use StmtCtx here. Could I add another context for the False guards in ?

b :: Bool
Just b | False = Nothing

Edit: Perhaps I should add a new match context PatBindGuards for the guards in the above example ?

Edit: Phab updated.

Last edited 20 months ago by sighingnow (previous) (diff)

comment:13 Changed 19 months ago by Ben Gamari <ben@…>

In e8e9f6a7/ghc:

Improve exhaustive checking for guards in pattern bindings and MultiIf.

Previously we didn't do exhaustive checking on MultiIf expressions
and guards in pattern bindings.

We can construct the `LMatch` directly from GRHSs or [LHsExpr]
(MultiIf's alts) then feed it to checkMatches, without construct the
MatchGroup and using function `matchWrapper`.

Signed-off-by: HE, Tao <sighingnow@gmail.com>

Test Plan: make test TEST="T14773a T14773b"

Reviewers: bgamari, RyanGlScott, simonpj

Reviewed By: bgamari, simonpj

Subscribers: rwbarton, thomie, carter

GHC Trac Issues: #14773

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

comment:14 Changed 19 months ago by bgamari

Milestone: 8.6.1
Resolution: fixed
Status: patchclosed
Note: See TracTickets for help on using tickets.