Opened 19 months ago

Last modified 8 months ago

#14838 new bug

missing "incomplete-patterns" warning for TH-generated functions

Reported by: gelisam Owned by:
Priority: normal Milestone: 8.10.1
Component: Template Haskell Version: 8.2.2
Keywords: PatternMatchWarnings Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect error/warning at compile-time Test Case: T14838
Blocked By: Blocking:
Related Tickets: #16169 Differential Rev(s): Phab:D4440
Wiki Page:

Description

"incomplete-patterns" warnings are generated for TH-generated case expressions, but not for TH-generated functions, so the behaviour is inconsistent.

For example:

{-# LANGUAGE TemplateHaskell #-}
module Lib where
import Language.Haskell.TH

qIncompleteCase :: Q [Dec]
qIncompleteCase = [d|
  incompleteCase :: Bool -> ()
  incompleteCase b = case b of
    True -> () |]

qIncompleteFunction :: Q [Dec]
qIncompleteFunction =[d|
  incompleteFunction :: Bool -> ()
  incompleteFunction True = () |]
{-# LANGUAGE TemplateHaskell #-}
module Bug where
import Lib

$qIncompleteCase

$qIncompleteFunction

incompleteCase' :: Bool -> ()
incompleteCase' b = case b of
  True -> ()

incompleteFunction' :: Bool -> ()
incompleteFunction' True = ()

When compiling the above two files with -Wall, GHC 8.2.2 produces an "incomplete-patterns" warning for qIncompleteCase, incompleteCase', and incompleteFunction', but not for qIncompleteFunction. I would prefer to get a warning for qIncompleteFunction as well.

My use case is the surjective package, in which I intentionally generate code which produces warnings in order to warn the user about corresponding issues in their code. I could generate better error messages if GHC generated warnings for TH-generated functions as well.

Change History (15)

comment:1 Changed 19 months ago by RyanGlScott

Amazing find. It turns out that this can all be tied back to this line of code (introduced in GHC 8.0, in commit 8a506104d5b5b71d5640afc69c992e0af40f2213):

        ; unless (isGenerated origin) $
          when (isAnyPmCheckEnabled dflags (DsMatchContext ctxt locn)) $
          addTmCsDs (genCaseTmCs1 mb_scr new_vars) $
              -- See Note [Type and Term Equality Propagation]
          checkMatches dflags (DsMatchContext ctxt locn) new_vars matches

Specifically, the unless (isGenerated origin) part. It turns out that -Wincomplete-patterns is completely suppressed for any code that has an Origin of Generated, and Template Haskell–spliced code happens to fall under that bucket.

I see two ways out of this:

  1. Go through the code in Convert and change every use of Generated to FromSource.
  2. Come up with a separate origin for Template Haskell code (perhaps FromTH?). I'm not sure if it's worth the trouble, though.

comment:2 Changed 19 months ago by RyanGlScott

Keywords: PatternMatchWarnings added

comment:3 Changed 19 months ago by simonpj

Nothing in the commit message explains why we suppress pattern match checks for generated code. Perhaps we shouldn't?

I favour (1) until we see why it's a bad idea.

comment:4 Changed 19 months ago by mpickering

I think the current behaviour of suppressing warnings might be better? If you see a warning arising from spliced code, what should you do about it?

comment:5 Changed 19 months ago by simonpj

But there shouldn't be any warnings! And indeed this ticket is all about a warning that the user wanted but wasn't there.

comment:6 Changed 19 months ago by RyanGlScott

I agree with Simon's reasoning. Moreover, it indeed appears that this was intended to be the case already, judging from this comment:

          --We use FromSource as the origin of the bind
          -- because the TH declaration is user-written

comment:7 Changed 19 months ago by RyanGlScott

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

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

In ffb2738f/ghc:

Fix #14838 by marking TH-spliced code as FromSource

Previously, any Template Haskell code that was spliced would
be marked as `Generated`, which would completely suppress pattern-
match coverage warnings for it, which several folks found confusing.
Indeed, Template Haskell-spliced code is "source" code in some sense,
as users specifically request that it be put into their program, so
changing its designation to `FromSource` makes sense from that
perspective.

Test Plan: make test TEST=T14838

Reviewers: goldfire, bgamari

Reviewed By: bgamari

Subscribers: rwbarton, thomie, carter

GHC Trac Issues: #14838

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

comment:9 Changed 19 months ago by bgamari

Milestone: 8.6.1
Resolution: fixed
Status: patchclosed
Test Case: T14838

comment:10 Changed 17 months ago by Ben Gamari <ben@…>

In c054162/ghc:

Revert "Fix #14838 by marking TH-spliced code as FromSource"

This reverts commit ffb2738f86c4e4c3f0eaacf0a95d7326fdd2e383.

Due to #14987.

Reviewers: goldfire, RyanGlScott

Reviewed By: RyanGlScott

Subscribers: RyanGlScott, thomie, carter

GHC Trac Issues: #14987, #14838

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

comment:11 Changed 17 months ago by bgamari

Resolution: fixed
Status: closednew

comment:12 Changed 15 months ago by bgamari

Milestone: 8.6.18.8.1

These won't be addressed in GHC 8.6.

comment:13 Changed 9 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

comment:14 Changed 8 months ago by mpickering

No idea why the original patch was reverted here but I still think close as working as intended. You wouldn't want a warning if an imported library function had an incomplete pattern so why is code generated by splices any different?

comment:15 Changed 8 months ago by RyanGlScott

For the sake of historical context, the original patch was in reverted in light of #14899, where it was discovered that the only reason that singletons compiles in a reasonable amount of time is because pattern-match coverage checking is disabled for generated code. Technically, the problem still exists if you were to write this code by hand, but we deemed singletons to be an important-enough reason not to keep the patch :)

Note: See TracTickets for help on using tickets.