Opened 4 years ago

Closed 3 years ago

#10600 closed bug (fixed)

-fwarn-incomplete-patterns doesn't work with -fno-code

Reported by: akio Owned by: ezyang
Priority: normal Milestone: 8.4.1
Component: Compiler Version: 7.10.1
Keywords: Cc: duog
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect warning at compile-time Test Case: driver/T8101b
Blocked By: Blocking:
Related Tickets: #8101 Differential Rev(s): Phab:D1278
Wiki Page:

Description (last modified by bgamari)

-fwarn-incomplete-patterns doesn't seem to generate any warnings when -fno-code is specified.

To reproduce, save this module as foo.hs.

module Foo where
foo True = 4

and compile with -fwarn-incomplete-patterns, with and without -fno-code.

% ghc -fwarn-incomplete-patterns -fforce-recomp foo.hs
[1 of 1] Compiling Foo              ( foo.hs, foo.o )

foo.hs:2:1: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for ‘foo’: Patterns not matched: False
% ghc -fwarn-incomplete-patterns -fforce-recomp -fno-code foo.hs
[1 of 1] Compiling Foo              ( foo.hs, nothing )
%

Change History (27)

comment:1 Changed 4 years ago by thomie

Milestone: 7.12.1

This has been addressed before in #8101. The fix there doesn't seem to work, and is missing a test.

comment:2 Changed 4 years ago by rwbarton

Actually the fix in #8101 works in one-shot mode (-c), but not in make mode (the default).

comment:3 Changed 4 years ago by Reid Barton <rwbarton@…>

In aa230540/ghc:

Add test for #10600 (exhaustiveness check with --make and -fno-code)

comment:4 Changed 4 years ago by rwbarton

Test Case: driver/T8101b

comment:5 Changed 4 years ago by ntc2

This causes Emacs Flycheck to not warn for non-exhaustive patterns, because it runs GHC with -fno-code: https://github.com/flycheck/flycheck/issues/722.

comment:6 Changed 4 years ago by simonpj

This can't be hard to fix... would anyone like to look at it?

Simon

comment:7 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:8 Changed 4 years ago by thomie

Keywords: newcomer added

comment:9 Changed 4 years ago by ezyang

Owner: set to ezyang

comment:10 Changed 4 years ago by ezyang

So... I implemented this. But then I got a lint error!

"/home/hs01/ezyang/ghc-validate/inplace/bin/haddock" --verbosity=0 --odir="libraries/ghc-prim/dist-install/doc/html/ghc-prim" --no-tmp-comp-dir --dump-interface=libraries/ghc-prim/dist-install/doc/html/ghc-prim/ghc-prim.haddock --html --hoogle --title="ghc-prim-0.4.0.0: GHC primitives" --prologue="libraries/ghc-prim/dist-install/haddock-prologue.txt"   --optghc=-hisuf --optghc=dyn_hi --optghc=-osuf --optghc=dyn_o --optghc=-hcsuf --optghc=dyn_hc --optghc=-fPIC --optghc=-dynamic --optghc=-O0 --optghc=-H64m --optghc=-Werror --optghc=-this-package-key --optghc=8TmvWUcS1U1IKHT0levwg3 --optghc=-hide-all-packages --optghc=-i --optghc=-ilibraries/ghc-prim/. --optghc=-ilibraries/ghc-prim/dist-install/build --optghc=-ilibraries/ghc-prim/dist-install/build/autogen --optghc=-Ilibraries/ghc-prim/dist-install/build --optghc=-Ilibraries/ghc-prim/dist-install/build/autogen --optghc=-Ilibraries/ghc-prim/. --optghc=-optP-include --optghc=-optPlibraries/ghc-prim/dist-install/build/autogen/cabal_macros.h --optghc=-package-id --optghc=builtin_rts --optghc=-this-package-key --optghc=ghc-prim --optghc=-XHaskell2010 --optghc=-O --optghc=-dcore-lint --optghc=-no-user-package-db --optghc=-rtsopts --optghc=-fno-warn-trustworthy-safe --optghc=-fno-warn-deprecated-flags --optghc=-fno-warn-tabs --optghc=-odir --optghc=libraries/ghc-prim/dist-install/build --optghc=-hidir --optghc=libraries/ghc-prim/dist-install/build --optghc=-stubdir --optghc=libraries/ghc-prim/dist-install/build --source-module=src/%{MODULE/./-}.html --source-entity=src/%{MODULE/./-}.html#%{NAME}   libraries/ghc-prim/./GHC/CString.hs  libraries/ghc-prim/./GHC/Classes.hs  libraries/ghc-prim/./GHC/Debug.hs  libraries/ghc-prim/./GHC/IntWord64.hs  libraries/ghc-prim/./GHC/Magic.hs  libraries/ghc-prim/dist-install/build/GHC/PrimopWrappers.hs  libraries/ghc-prim/./GHC/Tuple.hs  libraries/ghc-prim/./GHC/Types.hs libraries/ghc-prim/dist-install/build/autogen/GHC/Prim.hs +RTS -t"libraries/ghc-prim/dist-install/haddock.t" --machine-readable
*** Core Lint errors : in result of Desugar (after optimization) ***
<wired into compiler>: warning:
    [RHS of proxy# :: forall a_a3EW. Proxy# a_a3EW]
    The type of this binder is primitive: proxy#
    Binder's type: forall a_a3EW. Proxy# a_a3EW
Version 0, edited 4 years ago by ezyang (next)

comment:11 Changed 4 years ago by bgamari

Either we could explicitly blacklist GHC.Prim from desugaring or we could forego Core linting when we aren't going to codegen. Neither option seems terribly great although the first seems a bit more straightforward.

comment:12 Changed 4 years ago by thoughtpolice

Differential Rev(s): Phab:D1278

comment:13 Changed 4 years ago by bgamari

Description: modified (diff)
Status: newpatch

comment:14 Changed 4 years ago by bgamari

We discussed this during the weekly meeting; no one was terribly excited about any of the alternatives,

  • Fix GHC.Prim to pass Core Lint (perhaps not even possible)
  • Add a special case to skip desugaring for GHC.Prim
  • Add a special case to skip Core Linting GHC.Prim
  • Disable Core Linting when not generating code
  • Add a command line option to specifically disable desugaring (which could be used by Haddock)

The point also came up that we should really try to avoid desugaring when invoked by Haddock as this is otherwise wasted effort.

comment:15 Changed 4 years ago by Edward Z. Yang <ezyang@…>

In 427f8a15/ghc:

Deduplicate one-shot/make compile paths.

Summary:
We had a duplicate copy of the code for --make and for -c
which was a pain.  The call graph looked something like this:

    compileOne -> genericHscCompileGetFrontendResult -> genericHscFrontend
                                   hscCompileOneShot ---^

with genericHscCompileGetFrontendResult and hscCompileOneShot
duplicating logic for deciding whether or not recompilation
was needed.

This patchset fixes it, so now everything goes through this call-chain:

    compileOne (--make entry point)
        Calls hscIncrementCompile, invokes the pipeline to do codegen
        and sets up linkables.
    hscIncrementalCompile (-c entry point)
        Calls hscIncrementalFrontend, and then simplifying,
        desugaring, and writing out the interface.
    hscIncrementalFrontend
        Performs recompilation avoidance, if recompilation needed,
        does parses typechecking.

I also cleaned up some of the MergeBoot nonsense by introducing
a FrontendResult type.

NB: this BREAKS #8101 again, because I can't unconditionally desugar
due to Haddock barfing on lint, see #10600

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Test Plan: validate

Reviewers: simonpj, bgamari, simonmar, austin

Subscribers: thomie

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

comment:16 Changed 4 years ago by bgamari

Milestone: 8.0.18.2.1

This won't happen for 8.0. Punting.

comment:17 Changed 4 years ago by mpickering

Keywords: newcomer removed

comment:18 Changed 3 years ago by bgamari

Milestone: 8.2.18.4.1
Owner: ezyang deleted
Status: patchnew

The patch has some shortcomings (e.g. what to do with GHC.Prim) and it looks unlikely that this will happen for 8.2. Punting.

Last edited 3 years ago by bgamari (previous) (diff)

comment:19 Changed 3 years ago by bgamari

Cc: duog added
Owner: set to ezyang

comment:20 Changed 3 years ago by duog

I have had a look at this issue and I think the situation must have changed somewhat since the last time a fix was attempted. Several errors can be thrown during desugaring now, including at least

  • Top-level bindings for unlifted types aren't allowed:
  • Top-level strict pattern bindings aren't allowed:

which I assume must not have previously been the case?

If I add an unconditional desugaring in HscMain.finishTypecheckOnly then haddock errors on Ghc.Prim with "Top-level bindings for unlifted types aren't allowed:"

This means that currently these desugaring errors are not thrown for modules compiled with -fno-code.

I have pushed D3533 which includes a test for this.

Let's always desugar with -fno-code, except for Ghc.Prim?

Last edited 3 years ago by duog (previous) (diff)

comment:21 Changed 3 years ago by Ben Gamari <ben@…>

In baa18de/ghc:

testsuite: add new test for desugar warnings/errors with -fno-code

Add a new (expect_broken) test T10600 that checks that the error:
Top-level bindings for unlifted types aren't allowed: is thrown when
compiling with -fno-code. This test currently fails because modules
compiled with -fno-code aren't desugared. There are several other errors
which can be thrown during desugaring that aren't tested for,
discoverable by grepping for "errDs".

Update .stderr files T8101 and T8101b. Presumably the compilation output
has changed slightly since they were written.

Reviewers: austin, bgamari

Reviewed By: bgamari

Subscribers: rwbarton, thomie

GHC Trac Issues: #10600, #8101

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

comment:22 Changed 3 years ago by bgamari

  • Top-level bindings for unlifted types aren't allowed:
  • Top-level strict pattern bindings aren't allowed:

While these were never really "allowed", Richard Eisenberg did tighten the checks to ensure that they are caught between 8.0 and 8.2.

Let's always desugar with -fno-code, except for Ghc.Prim?

Sounds like a reasonable first approximation to me. Let me know if you get stuck.

comment:23 Changed 3 years ago by mpickering

Is there a conceptual problem of moving the pattern match checker to the end of typechecking? There is the additional fiddlyness to make sure all pattern occurrences are checked but this seems preferable than making -fno-code slower for everyone.

comment:24 in reply to:  23 Changed 3 years ago by duog

Replying to mpickering:

Is there a conceptual problem of moving the pattern match checker to the end of typechecking? There is the additional fiddlyness to make sure all pattern occurrences are checked but this seems preferable than making -fno-code slower for everyone.

I'm not qualified to answer that, but it's a bigger problem than just pattern match checking(Maybe we should change the ticket name?). The whole point of -fno-code is to check code for errors and warnings, and many errors and warnings are currently thrown by the desugarer.

I would expect that correctness is the most important concern, and that a slower, more correct -fno-code is preferable. I also expect that desugaring is in general a much less expensive operation than typechecking. Please correct me if I'm wrong!

Below I give a rough enumeration of the warnings and errors that the desugarer can currently throw, and some ideas about how to proceed.

The warnings listed in D1278 (comment 12, 20 months ago) as throwable by the desugarer are:

  • Opt_WarnIncompletePatterns (-W)
  • Opt_WarnIncompleteUniPatterns
  • Opt_WarnIncompletePatternsRecUpd
  • Opt_WarnIdentities
  • Opt_WarnOverflowedLiterals (def)
  • Opt_WarnEmptyEnumerations (def)
  • Opt_WarnInlineRuleShadowing (def)
  • Opt_WarnOverlappingPatterns (def)
  • Opt_WarnUnusedDoBind (-Wall)
  • Opt_WarnWrongDoBind (def)

I haven't verified that this list is up to date. I have noted in brackets default warnings (def), -W warnings and -Wall warnings.

The errors throwable by desugaring are a bit hard to find. Grepping and chasing errDs, I can see at least:

  • "A levity-polymorphic type is not allowed here:" : DsMonad.dsNoLevPoly
  • "Malformed constructor signature" : updateGadtResult called by DsMeta.repC
  • "Top level bindings for unlifted types" : DsBinds.dsTopLevelBinds
  • "Top level strict pattern bindings" : DsBinds.dsTopLevelBinds
  • "You can't mix polymorphic and unlifted bindings:" : DsExpr.ds_val_bind
  • "Recursive bindings for unlifted types aren't allowed:" : DsExpr.ds_val_bind
  • "Cannot use primitive with levity-polymorphic arguments:" : DsExpr.levPolyPrimopErr
  • "A lazy (~) pattern cannot bind variables of unlifted type." : Match.tidy1

Perhaps there is a list of extensions, where if none were enabled then some or all of these errors could be excluded? Say

  • TypeInType
  • MagicHash
  • UnboxedTuples
  • UnboxedSums
  • GADTs
  • BangPatterns

One would have to think carefully and understand this better than I do.

Then the first, easiest, solution is to unconditionally desugar with -fno-code. This has no maintenance burden and is easy to verify for correctness. D3542 implements this.

Then next improvement could be reducing the work done by the desugarer in -fno-code, I intend to look at this.

Finally one could implement a predicate to determine whether desugaring could produce warnings and errors, for example by checking enabled warnings and extensions, and desugar only when necessary. One could potentially move warnings/errors from desugaring to typechecking to facilitate this. This seems like a bit of a maintenance nightmare, but maybe it's not even so difficult to get them all out? Or at least everything except the TypeInType stuff.

comment:25 Changed 3 years ago by mpickering

Thanks Doug, that's a useful summary.

My main concern is a code hygiene. It is a fundamental design principle that we typecheck the code before desugaring so that error messages are better. By checking these things in the desugarer we might be trading good error messages for an easier implementation which I think is in general undesirable.

comment:26 Changed 3 years ago by Ben Gamari <ben@…>

In c9eb4385/ghc:

Desugar modules compiled with -fno-code

Previously modules with hscTarget == HscNothing were not desugared.
This patch changes behavior so that all modules HsSrcFile Modules except GHC.Prim
are desugared. Modules with hscTarget == HscNothing are not simplified.

Warnings and errors produced by the desugarer will now be produced when
compiling with -fno-code.

HscMain.finishTypecheckingOnly is removed, HscMain.hscIncrementalCompile is
simplified a bit, and HscMain.finish takes in the removed logic. I think this
is easier to follow.

Updates haddock submodule.

Tests T8101, T8101b, T10600 are no longer expect_broken.

Reviewers: ezyang, austin, bgamari

Subscribers: rwbarton, thomie

GHC Trac Issues: #10600

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

comment:27 Changed 3 years ago by bgamari

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.