Opened 18 months ago

Last modified 13 months ago

#14963 patch bug

ghci -fdefer-type-errors can't run IO action from another module

Reported by: elaforge Owned by: tdammers
Priority: high Milestone: 8.4.2
Component: GHCi Version: 8.4.1
Keywords: Cc: chak@…, oerjan, remi.turk@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: ghci/should_run/T14963
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D4833 Phab:D4830
Wiki Page:

Description

This is enough to trigger a crash on OS X and Linux:

Bug1.hs:

module Bug1.hs where
import qualified Bug2

test :: IO Bool
test = Bug2.failure

Bug2.hs:

module Bug2 where

failure :: IO Bool
failure = return False

Shell:

% ghci -fdefer-type-errors -ignore-dot-ghci
GHCi, version 8.4.1: http://www.haskell.org/ghc/  :? for help
Prelude> :load Bug
[1 of 2] Compiling Bug2             ( Bug2.hs, interpreted )
[2 of 2] Compiling Bug              ( Bug.hs, interpreted )
Ok, two modules loaded.
*Bug> test
ghc: panic! (the 'impossible' happened)
  (GHC version 8.4.1 for x86_64-apple-darwin):
	nameModule
  system $dShow_a1LX
  Call stack:
      CallStack (from HasCallStack):
        callStackDoc, called at compiler/utils/Outputable.hs:1150:37 in ghc:Outputable
        pprPanic, called at compiler/basicTypes/Name.hs:241:3 in ghc:Name

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

This is specific to 8.4.1, in 8.0.2 I get "False" as expected. If I leave off -fdefer-type-errors, it works. It also seems to be ghci only, compiling with -fdefer-type-errors doesn't have the problem.

Change History (40)

comment:1 Changed 18 months ago by chak

Cc: chak@… added

comment:2 Changed 18 months ago by monoidal

The same happens with just one module:

test :: IO Bool
test = return True

comment:3 Changed 18 months ago by RyanGlScott

I must be doing something wrong here. I created a file named Bug.hs with the contents of comment:2, and tried loading it like so:

$ ~/Software/ghc-8.4.1/bin/ghci -fdefer-type-errors -ignore-dot-ghciGHCi, version 8.4.1: http://www.haskell.org/ghc/  :? for help
Prelude> :load Bug
[1 of 1] Compiling Main             ( Bug.hs, interpreted )
Ok, one module loaded.

Which appears to work without issue. What am I missing?

comment:4 Changed 18 months ago by simonpj

I can repro the original 2-module report, with the GHC 8.4.2 branch, and with HEAD. Definite bug here!

comment:5 Changed 18 months ago by monoidal

After :load Bug you need to examine the value test in ghci.

comment:6 Changed 18 months ago by RyanGlScott

Oops, I overlooked that important detail!

Commit a211dca8236fb8c7ec632278f761121beeac1438 (Fix defer-out-of-scope-variables) is what caused this.

comment:7 Changed 18 months ago by RyanGlScott

Milestone: 8.4.2
Priority: normalhigh

Since this is a regression from 8.2, I'm opting to change the milestone and priority. Do change if you feel this isn't warranted.

comment:8 Changed 18 months ago by simonpj

It took me quite a while to find out what was going on here.

I started with a file containing only

test :: IO Bool
test = return True

Then load into GHCi, with -fdefer-type-errors

ghc --interactive -fdefer-type-errors Foo.hs

Now just evaluate test. Here's what I see from -ddump-tc -ddump-ds with a little extra debug tracing, when evaluating test at the GHCi prompt:

Typechecked expr do it_a1PP <- {>>=: GHC.Base.bindIO @ Bool
                                     @ [()]{let {EvBinds{[W] $dShow_a1Qg = GHC.Show.$fShowBool}} <>, <>}
                                           {<> |> <IO [()]>_R}
                                fail: "noSyntaxExpr"{}{<>}} /\(@ a_a1Q5).
                                                            let {EvBinds{[W] $dGHCiSandboxIO_a1Q7
                                                                           = GHC.GHCi.$fGHCiSandboxIOIO}}
                                                            GHC.GHCi.ghciStepIO @ IO
                                                              $dGHCiSandboxIO_a1Q7
                                                            @ a_a1Q5 ::
                     
  ic_tythings:
  ic_insts:
  ic_rn_gbl_env (LocalDef) [Foo.test defined at Foo.hs:11:1]
newTcEvBinds unique = a1Ql
checkSatisfiability { {}
checkSatisfiability } {}
unflattenGivens []
ds BindStmt
    /\(@ a_a1Q5).
    let {EvBinds{[W] $dGHCiSandboxIO_a1Q7 = $fGHCiSandboxIOIO}}
    ghciStepIO @ IO $dGHCiSandboxIO_a1Q7 @ a_a1Q5 ::
      forall a_a1PO. IO a_a1PO -> IO a_a1PO
  @ Bool
    test
    |> <IO Bool>_R
  ---
  (\ (@ a_a1Q5) ->
     let {
       $dGHCiSandboxIO_a1Q7 :: GHCiSandboxIO IO
       [LclId]
       $dGHCiSandboxIO_a1Q7 = $fGHCiSandboxIOIO } in
     ghciStepIO @ IO $dGHCiSandboxIO_a1Q7 @ a_a1Q5)
    @ Bool test
*** Core Lint errors : in result of desugar expression ***
<no location info>: warning:
    In the expression: print @ Bool $dShow_a1Qg it_a1PP
    $dShow_a1Qg :: Show Bool
    [LclId] is out of scope
*** Offending Program ***
bindIO
  @ Bool
  @ [()]
  (let {
     $dShow_a1Qg :: Show Bool      <----- Right binding, wrongly scoped!
     [LclId]
     $dShow_a1Qg = $fShowBool } in
   (\ (@ a_a1Q5) ->
      let {
        $dGHCiSandboxIO_a1Q7 :: GHCiSandboxIO IO
        [LclId]
        $dGHCiSandboxIO_a1Q7 = $fGHCiSandboxIOIO } in
      ghciStepIO @ IO $dGHCiSandboxIO_a1Q7 @ a_a1Q5)
     @ Bool test)
  (\ (it_a1PP :: Bool) ->
     thenIO
       @ ()
       @ [()]
       (print @ Bool $dShow_a1Qg it_a1PP)
       (returnIO
          @ [()]
          (: @ ()
             (unsafeCoerce# @ 'LiftedRep @ 'LiftedRep @ Bool @ () it_a1PP)
             ([] @ ()))))
*** End of Offense ***

Here's the deal.

  • When you type ghci> test to the GHCi prompt, GHC typechecks (roughly)
    do { it <- sandbox test
       ; print it
       ; return () }
    
    This is Plan A in TcRnDriver.tcUserStmt.
  • When typechecking the initial BindStmt of the do block, we end up invoking tcSyntaxOp in TcMatches.TcDoStmt
  • Bizarrely, we then typecheck the rest of the do block inside the thing_inside argument to tcSyntaxOp.
  • TcExpr.tcSyntaxOp ends up calling tcSyntaxArgE, which calls tcSkolemize, which builds an implication constraint.
  • This implication constraint gets wrapped around the first argument of the bind, namely sandbox test. But since the thing_inside includes the Show constraint arising from print it, the Show dictionary lands up in the evidence bindings for the implication, and hence gets wrapped around the sandbox test RHS only. Utterly bogus.
  • All this happens always, I think. Usually TcUnify.implicationNeeded ends up being false, so we don't actually create an implication, and so the evidence bindings don't end up in the wrong place. But in the special case of GHCi with -fdefer-type-errors we (unusually) you'll see that implicationNeeded returns True. And that's why the bug manifest only in GHCi, and even then only with -fdefer-type-errors.

Blimey.


All of this is a result of the impenetrable code in tcSyntaxOp, which Richard introduced in

commit 00cbbab3362578df44851442408a8b91a2a769fa
Author: Richard Eisenberg <eir@cis.upenn.edu>
Date:   Wed Jan 13 23:29:17 2016 -0500

...

    In addition, this patch does a significant reworking of
    RebindableSyntax, allowing much more freedom in the types
    of the rebindable operators. For example, we can now have
    `negate :: Int -> Bool` and
    `(>>=) :: m a -> (forall x. a x -> m b) -> m b`. The magic
    is in tcSyntaxOp.

To me it seems Utterly And Completely Wrong for tcSyntaxOp to take the continuation as a thing_inside argument. Not only is it extremely complicated, but it's also plain wrong.

Why can't it just decompose the function type to produce a bunch of types to use as the expected types for the aguments? No Notes explain. The code makes my head spin.

Richard, can't this all be radically simplified?

comment:9 Changed 18 months ago by simonpj

PS: using WpFun would mean you could return a single HsWrapper to wrap around the bind function, rather than returning wrappers for the function, the arguments, and the result. See its use in TcUnify.tc_sub_type_ds.

comment:10 Changed 18 months ago by goldfire

There's always room for simplification, of course, but I think the room here is limited. This whole mechanism came into being when ExpTypes were introduced. ExpTypes are "holes" -- places to write a type once we infer it; they replaced SigmaTv tyvars that were used previously. I forget what aspect of ExpTypes specifically forced the rewrite of rebindable syntax, but I do remember that this was more-or-less forced.

The reason for the complication is that we want to allow for the possibility that (>>=) :: blah -> (forall x. x -> x) -> wurble, where the arguments might have a higher-rank type. This, in turn, requires skolemization while type-checking. That problem with the current setup is that the two arguments are checked in the same thing_inside, where they should really be in different contexts. But that would make the whole scheme even more complicated.

So I'm a bit stuck really on how you would want to simplify this.

comment:11 Changed 18 months ago by goldfire

After a long discussion of design possibilities, Simon and I came to the following:

  1. The current design is broken, as described in this ticket.
  1. If we want to handle all rebindable syntax in a general fashion, we were unable to come up with anything simpler than the "impenetrable" code that exists. And, indeed, to fix the current flaw would likely require adding HLists or some such, making it even worse.
  1. Much of the current complication comes from the handling of >>=, which has an intricate type. Specifically, we need (>>=) :: ty1 -> (ty2 -> ty3) -> ty4. However, it would also be quite cool if something like (>>=) :: ty1 -> (forall a. ty2 -> ty3) -> ty4 were allowed. This effectively means that a <- operator in a do could bind an existential variable without using a pattern-match. And, if the user wrote (>>=) literally (without do), then this would indeed be possible. So it would be nice.

The complication specifically stems from the fact that the code processing BindStmts needs to know ty2 and ty3, so we must decompose the type of (>>=)'s second argument. In order to do this, we need to skolemize any foralld variables, and skolemization requires an implication constraint, causing the bug in this ticket.

  1. Rebindable syntax that decomposes list constructors suffers a similar fate, but that's not nearly as painful as (>>=).

Though I'm still not fully convinced, we resolved to make the treatment of rebindable syntax simpler and less general. Specifically:

  1. Reduce SyntaxOpType to have two constructors: SynAny :: SyntaxOpType and SynType :: ExpType -> SyntaxOpType.
  1. Make new functions tcBindOp, tcFromListOp, and tcFromListNOp that handle the special cases for (>>=) and the list functions. These won't use general mechanisms, but just be a bit repetitive with tcSyntaxOp.
  1. The SynRho case of tcSyntaxOp will be dropped. Instead, clients of tcSyntaxOp that need rho-types will arrange to instantiate/skolemize the sigma-types that tcSyntaxOp provides as needed. This will duplicate code, but it means that tcSyntaxOp no longer needs to be written in CPS style. It will also allow this ticket to be resolved.
  1. This still won't handle cases like fromListN :: (forall a. Int) -> [b] -> c, where one parameter of a bit of rebindable syntax has a known type, but that known type might be redundantly quantified. Handling such a case would require CPSing again, and so we won't. This means that rebindable syntax will be a bit less expressive than the manual has heretofore promised. But the only lost cases are surely of the form above, where there is an unused quantified type variable. We can arrange for a suitable error message in these cases. This change will require a user manual update.
  1. When all this is done, the extra flexibility in the SyntaxExpr type -- with its argument wrappers and result wrapper -- won't be needed. Instead, tcSyntaxOp can return a plain old HsExpr, suitably wrapped. Accommodations will have to be made in BindStmt and the places where list operators might crop up to store extra HsWrappers to be applied to arguments.
  1. A Note will be placed near tcSyntaxOp and the bind/list functions describing this design plan. If, in the future, we want more rebindable syntax, it might encourage us to re-adopt the more general -- but more complicated -- scheme currently in place.

comment:12 Changed 18 months ago by elaforge

This is just a peanut-gallery comment, so forgive the naivety, but my impression is that Idris treats do desugaring purely syntactically, so e.g. you don't even need a Monad class or the expected types, just something called (>>=) and pure in scope. Idris of course does a lot of things fundamentally differently, not the least of which is type-directed name overloading, but that seems orthogonal. What's the problem with treating do as a syntax macro before even getting to typechecking?

I gather the problem above is all rebindable syntax not just do and (>>=), but it made me curious about that one thing.

comment:13 Changed 18 months ago by goldfire

I actually proposed doing exactly that when working this out with Simon. The problem is that doing so would ruin error messages, because we could report errors only with respect to the expanded syntax, instead of what the user actually wrote.

comment:14 Changed 18 months ago by elaforge

Yeah, macros in general would need to store the original form and somehow get back to it for errors. I'm sure this has been thought about quite a bit over the decades and the devil is in that "somehow." I don't know anything about that previous work, so I'll leave it at that :)

comment:15 Changed 16 months ago by chak

Given that this didn't make it into 8.4.2, may I ask, what is the plan here?

comment:16 Changed 16 months ago by simonpj

As a short term fix how bad would it be to make -fdefer-type-errors incompatible with GHCi?

We could apply that fix to HEAD (until we fix this ticket properly) and perhaps even to 8.4.3.

comment:17 Changed 16 months ago by tdammers

Owner: set to tdammers

comment:18 in reply to:  13 Changed 16 months ago by tdammers

Replying to goldfire:

I actually proposed doing exactly that when working this out with Simon. The problem is that doing so would ruin error messages, because we could report errors only with respect to the expanded syntax, instead of what the user actually wrote.

My gut feeling (by all means correct me if I'm wrong!) is that this would still amount to a more elegant solution, everything considered. We would have to extend HsSyn to retain the original (sugared) notation, and treat such annotated syntax specially when printing error messages, but as far as type checking etc. are concerned, I would expect this to be mostly transparent.

Personally, I wouldn't even mind getting error messages in desugared form, but I can understand if others feel strongly about this.

comment:19 in reply to:  16 Changed 16 months ago by tdammers

Replying to simonpj:

As a short term fix how bad would it be to make -fdefer-type-errors incompatible with GHCi?

We could apply that fix to HEAD (until we fix this ticket properly) and perhaps even to 8.4.3.

I wouldn't be surprised to find that this breaks someone's workflow. Maybe we could get some opinions on this from the community?

comment:20 Changed 16 months ago by elaforge

Surely those people's workflows are already broken by the fact that you can't evaluate much of anything without causing a panic.

If they are just loading the modules to check types and not actually running them, then they don't need -fdefer-type-errors in the first place. So if someone's tool is thrown by an extra warning msg, they can probably just remove the flag. I also think it's reasonable to expect tools to be robust against unexpected messages at startup before the prompt comes.

comment:21 in reply to:  20 Changed 16 months ago by tdammers

Replying to elaforge:

Surely those people's workflows are already broken by the fact that you can't evaluate much of anything without causing a panic.

If they are just loading the modules to check types and not actually running them, then they don't need -fdefer-type-errors in the first place. So if someone's tool is thrown by an extra warning msg, they can probably just remove the flag. I also think it's reasonable to expect tools to be robust against unexpected messages at startup before the prompt comes.

The use case for -fdefer-type-errors is so that you can run (and test) your module even when parts of it don't typecheck yet. E.g. suppose you write this:

module Pluralize
where

suffix :: Int -> String
suffix 1 = ""
suffix _ = "s"

pluralize :: Int -> String -> String
pluralize i s = s ++ i -- TODO

...then you might want to load it into GHCi to play with the suffix function, or you may want to run unit tests on it, even though pluralize doesn't work yet (and doesn't typecheck).

Deferring type errors is *especially* (some would argue *only*) useful in an interactive REPL: there isn't really a sharp workflow distinction between "compile time" and "runtime", so it is less important to produce type errors at compile time.

comment:22 Changed 16 months ago by tdammers

As discussed in Hangouts, the problem is even more specific: it only appears for expressions entered at the GHCi prompt while defer-type-errors is active. Loading modules with defer-type-errors on, then turning it off and *then* evaluating expressions at the prompt works just fine.

Hence, the quick fix for 8.6 will be to selectively disable deferred type checking for expressions entered interactively. This way, we will not disrupt any workflows (the error would have appeared right there and then one way or another), while still avoiding the crash.

comment:23 in reply to:  22 Changed 16 months ago by tdammers

Replying to tdammers:

As discussed in Hangouts, the problem is even more specific: it only appears for expressions entered at the GHCi prompt while defer-type-errors is active. Loading modules with defer-type-errors on, then turning it off and *then* evaluating expressions at the prompt works just fine.

It actually turns out that this isn't the case, as the following sample GHCi session illustrates:

tobias@zoidberg:~/well-typed/devel/ghc-disable-defer-type-errors/ > ghc --interactive Foo.hs -fdefer-type-errors
GHCi, version 8.4.1: http://www.haskell.org/ghc/  :? for help
Loaded GHCi configuration from /home/tobias/dotfiles/ghc/.ghc/ghci.conf
[1 of 1] Compiling Main             ( Foo.hs, interpreted )
Ok, one module loaded.
λ> test
ghc: panic! (the 'impossible' happened)
  (GHC version 8.4.1 for x86_64-unknown-linux):
        nameModule
  system $dShow_a22M
  Call stack:
      CallStack (from HasCallStack):
        callStackDoc, called at compiler/utils/Outputable.hs:1150:37 in ghc:Outputable
        pprPanic, called at compiler/basicTypes/Name.hs:241:3 in ghc:Name

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

λ> :set -fno-defer-type-errors
λ> test
ghc: panic! (the 'impossible' happened)
  (GHC version 8.4.1 for x86_64-unknown-linux):
        nameModule
  system $dShow_a289
  Call stack:
      CallStack (from HasCallStack):
        callStackDoc, called at compiler/utils/Outputable.hs:1150:37 in ghc:Outputable
        pprPanic, called at compiler/basicTypes/Name.hs:241:3 in ghc:Name

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

However this probably just means that :set doesn't properly unset the fdefer-type-errors flag. Hmm.

comment:24 Changed 16 months ago by tdammers

The :set turns out to work just fine; I trapped the DynFlags right where GHCi runs those expressions, and both flags that control deferred type errors are now forced off; yet the panic still occurs when running ghci with -fdefer-type-errors.

From the error message, it seems to me like GHCi is trying to print a module name, but fails, and this only happens when starting up with deferred type errors. I assume the module in question is the Foo module just loaded.

Another data point is that the panic does not occur when I change the Foo module like so:

module Foo where

test :: IO ()
test = return ()

My hypothesis here is that because GHCi does not print the result of an IO action if its type is (), so the whole pretty-printing machinery doesn't run.

comment:25 Changed 16 months ago by tdammers

Another data point; the Core ghci generates with -fdefer-type-errors is slightly different from the normal output:

  • .dump-simpl

    old new  
    11
    22==================== Tidy Core ====================
    3 2018-06-06 15:53:34.440330463 UTC
     32018-06-06 17:44:00.38910864 UTC
    44
    55Result size of Tidy Core
    66  = {terms: 19, types: 9, coercions: 0, joins: 0/0}
    77
    8 -- RHS size: {terms: 4, types: 2, coercions: 0, joins: 0/0}
    9 test :: IO Int
    10 [GblId]
    11 test
    12   = break<0>() return @ IO GHC.Base.$fMonadIO @ Int (GHC.Types.I# 1#)
    13 
    148-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
    15 $trModule1_r1Ss :: GHC.Prim.Addr#
     9$trModule1_r1Sr :: GHC.Prim.Addr#
    1610[GblId, Caf=NoCafRefs, Unf=OtherCon []]
    17 $trModule1_r1Ss = "main"#
     11$trModule1_r1Sr = "main"#
    1812
    1913-- RHS size: {terms: 2, types: 0, coercions: 0, joins: 0/0}
    20 $trModule2_r1SD :: GHC.Types.TrName
     14$trModule2_r1SC :: GHC.Types.TrName
    2115[GblId, Caf=NoCafRefs, Unf=OtherCon []]
    22 $trModule2_r1SD = GHC.Types.TrNameS $trModule1_r1Ss
     16$trModule2_r1SC = GHC.Types.TrNameS $trModule1_r1Sr
    2317
    2418-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
    25 $trModule3_r1SE :: GHC.Prim.Addr#
     19$trModule3_r1SD :: GHC.Prim.Addr#
    2620[GblId, Caf=NoCafRefs, Unf=OtherCon []]
    27 $trModule3_r1SE = "Main"#
     21$trModule3_r1SD = "Main"#
    2822
    2923-- RHS size: {terms: 2, types: 0, coercions: 0, joins: 0/0}
    30 $trModule4_r1SF :: GHC.Types.TrName
     24$trModule4_r1SE :: GHC.Types.TrName
    3125[GblId, Caf=NoCafRefs, Unf=OtherCon []]
    32 $trModule4_r1SF = GHC.Types.TrNameS $trModule3_r1SE
     26$trModule4_r1SE = GHC.Types.TrNameS $trModule3_r1SD
    3327
    3428-- RHS size: {terms: 3, types: 0, coercions: 0, joins: 0/0}
    3529Main.$trModule :: GHC.Types.Module
    3630[GblId, Caf=NoCafRefs, Unf=OtherCon []]
    37 Main.$trModule = GHC.Types.Module $trModule2_r1SD $trModule4_r1SF
     31Main.$trModule = GHC.Types.Module $trModule2_r1SC $trModule4_r1SE
     32
     33-- RHS size: {terms: 4, types: 2, coercions: 0, joins: 0/0}
     34test :: IO Int
     35[GblId]
     36test
     37  = break<0>() return @ IO GHC.Base.$fMonadIO @ Int (GHC.Types.I# 1#)

The only differences however are due to uniques not matching up, and putting test first instead of last.

Last edited 16 months ago by tdammers (previous) (diff)

comment:26 Changed 16 months ago by tdammers

However the dumped Core for the interactive expression itself seems to depend only on whether type error deferring was active while loading the module.

Evaluating test interactively without -fdefer-type-errors:

==================== Simplified expression ====================
GHC.Base.returnIO
  @ [()]
  (GHC.Types.:
     @ ()
     ((GHC.Base..
         @ (GHC.Types.IO GHC.Base.String)
         @ (GHC.Types.IO GHC.Base.String)
         @ [GHC.Types.Char]
         (GHC.GHCi.ghciStepIO
            @ GHC.Types.IO GHC.GHCi.$fGHCiSandboxIOIO @ GHC.Base.String)
         (\ (s_a1z7 :: [GHC.Types.Char]) ->
            GHC.Base.$
              @ 'GHC.Types.LiftedRep
              @ [GHC.Types.Char]
              @ (GHC.Types.IO GHC.Base.String)
              (GHC.Base.return
                 @ GHC.Types.IO GHC.Base.$fMonadIO @ [GHC.Types.Char])
              (GHC.Base.++
                 @ GHC.Types.Char
                 (GHC.CString.unpackCString# ":! pointfree \""#)
                 (GHC.Base.++
                    @ GHC.Types.Char s_a1z7 (GHC.CString.unpackCString# "\""#)))))
      `cast` (UnsafeCo representational (GHC.Base.String
                                         -> GHC.Types.IO GHC.Base.String) ()
              :: (GHC.Base.String -> GHC.Types.IO GHC.Base.String) ~R# ()))
     (GHC.Types.[] @ ()))

With -fdefer-type-errors:

==================== Simplified expression ====================
GHC.Base.bindIO
  @ GHC.Types.Int
  @ [()]
  (GHC.GHCi.ghciStepIO
     @ GHC.Types.IO
     GHC.GHCi.$fGHCiSandboxIOIO
     @ GHC.Types.Int
     Main.test)
  (\ (it_a1CU :: GHC.Types.Int) ->
     GHC.Base.thenIO
       @ ()
       @ [()]
       (System.IO.print @ GHC.Types.Int $dShow_a1Rt it_a1CU)
       (GHC.Base.returnIO
          @ [()]
          (GHC.Types.:
             @ ()
             (it_a1CU
              `cast` (UnsafeCo representational GHC.Types.Int ()
                      :: GHC.Types.Int ~R# ()))
             (GHC.Types.[] @ ()))))

Whether I issue :set -fno-defer-type-errors before invoking test or not makes absolutely no difference. However, loading the module without -fdefer-type-errors, but :set -fdefer-type-errors before invoking test also produces the second Core, and subsequent panic.

Last edited 16 months ago by tdammers (previous) (diff)

comment:27 Changed 16 months ago by oerjan

Cc: oerjan added

comment:28 Changed 15 months ago by tdammers

I've done some more dumping. The situation can be traced back to the desugar phase at least.

In all 4 test cases, I did the same thing:

  • start up ghci on the Foo.hs module, with -fforce-recomp -ddump-ds -dsuppress-all; adding -fdefer-type-errors or not depending on the test case.
  • depending on the test case, :set -fno-defer-type-errors or -fdefer-type-errors to override the command-line setting
  • evaluate test in ghci

It turns out that the desugared output is almost entirely identical (save for mismatching Uniques), except for the final GHCI invocation of test. The desugared output for that is identical between the three cases that involve deferred type errors anywhere, but different for the case that doesn't defer type errors anywhere. To wit:

With deferred type errors in either or both module compilation and interactive session:

==================== Desugared ====================
bindIO
  (let {
     $dShow_a1Rt
     $dShow_a1Rt = $fShowInt } in
   (\ @ a_a1Dg ->
      let {
        $dGHCiSandboxIO_a1Di
        $dGHCiSandboxIO_a1Di = $fGHCiSandboxIOIO } in
      ghciStepIO $dGHCiSandboxIO_a1Di)
     test)
  (\ it_a1CU ->
     thenIO
       (print $dShow_a1Rt it_a1CU)
       (returnIO (: (unsafeCoerce# it_a1CU) [])))

Without deferred type errors:

==================== Desugared ====================
let {
  $dShow_a1Rq
  $dShow_a1Rq = $fShowInt } in
bindIO
  ((\ @ a_a1De ->
      let {
        $dGHCiSandboxIO_a1Dg
        $dGHCiSandboxIO_a1Dg = $fGHCiSandboxIOIO } in
      ghciStepIO $dGHCiSandboxIO_a1Dg)
     test)
  (\ it_a1CT ->
     thenIO
       (print $dShow_a1Rq it_a1CT)
       (returnIO (: (unsafeCoerce# it_a1CT) [])))

Ignoring uniques entirely, the only real difference is whether or not the dShow... binding is floated out.

So the questions are: why does it get floated out when type errors are deferred; and how does that lead to GHC calling nameModule on the dShow... part.

comment:29 Changed 15 months ago by tdammers

But wait! The version with -fdefer-type-errors is actually horribly wrong, because dShow_... is used outside of the let binding that defines it! This cannot possibly work, can it?

comment:30 Changed 15 months ago by simonpj

The code in TcUnify described in comment:8 is

implicationNeeded skol_tvs given
  | null skol_tvs
  , null given
  = -- Empty skolems and givens
    do { tc_lvl <- getTcLevel
       ; if not (isTopTcLevel tc_lvl)  -- No implication needed if we are
         then return False             -- already inside an implication
         else
    do { dflags <- getDynFlags       -- If any deferral can happen,
                                     -- we must build an implication
       ; return (gopt Opt_DeferTypeErrors dflags ||
                 gopt Opt_DeferTypedHoles dflags ||
                 gopt Opt_DeferOutOfScopeVariables dflags) } }

So you have to switch of all three of Opt_DeferTypeErrors, Opt_DeferTypedHoles and Opt_DeferOutOfScopeVariables. But NB that the first implies the other two; so I guess that you need to switch all three of them off in the workaround.

comment:31 Changed 15 months ago by tdammers

OK, disabling all three does indeed "fix" it. Patch incoming.

comment:32 Changed 15 months ago by tdammers

Differential Rev(s): D4833
Status: newpatch
Test Case: ghci/should_run/T14963

I have put up a patch (https://phabricator.haskell.org/D4833), however it turns out that while disabling these three flags for interactive statements avoids the panic, it also seems to break a number of other tests.

comment:33 Changed 15 months ago by tdammers

Differential Rev(s): D4833D4833 D4830

D4830 looks like a cleaner solution than D4833; suggest we run with the former.

comment:34 Changed 15 months ago by sighingnow

Differential Rev(s): D4833 D4830Phab:D4833 Phab:D4830

comment:35 Changed 15 months ago by tdammers

Quick thought on the side: -fdefer-type-errors currently implies -fdefer-type-holes and -fdefer-out-of-scope-variables, which makes sense; however, the way implied flags currently work in GHC leads to unexpected results in GHCi when you turn a flag on and then off again. You'd expect :set -fdefer-type-errors; :set -fno-defer-type-errors to be a no-op, but it's not, because it turns -fdefer-type-holes and -fdefer-out-of-scope-variables on and never off again.

So my thought was that maybe it would be better to use two sets of DynFlags here: one set to represent what the user explicitly requested, not setting any of the implied flags; and one set of "effective" flags, containing the explicitly requested ones plus all the implied ones. The latter would be calculated on the fly, just before compiling, based on the former. This way, we can trivially tell the difference between flags explicitly requested by the user, and flags that are active because some other flag implies them, and unsetting the explicit flag will take the implicit flags with it unless they were also requested explicitly.

If people think this would make sense, I'll file it as a separate ticket.

comment:36 Changed 15 months ago by simonpj

Let's not go overboard on this. At the moment we are just implementing a workaround for a bug that we will fix. Moreover the workaround is a one-line addition to Phab:D4830. DO we need to do more?

comment:37 in reply to:  36 Changed 15 months ago by tdammers

Replying to simonpj:

Let's not go overboard on this. At the moment we are just implementing a workaround for a bug that we will fix. Moreover the workaround is a one-line addition to Phab:D4830. DO we need to do more?

Oh, of course the proposal is not going to help with the issue at hand. I just noticed some unintuitive behavior, and a possible fix. If it's not deemed fix-worthy, then you'll never hear me about it again.

comment:38 Changed 15 months ago by Ben Gamari <ben@…>

In 4a931665/ghc:

Disable `-fdefer-out-of-scope-variables` in ghci.

We have already disabled `-fdefer-type-errors` and
`-fdefer-typed-holes` in ghci.
This patch disables `-fdefer-out-of-scope-variables` as well.

Fixes Trac #15259, as well as #14963.

Test Plan: make test TEST="T15259 T14963a T14963b T14963c"

Reviewers: bgamari, tdammers

Reviewed By: tdammers

Subscribers: tdammers, rwbarton, thomie, carter

GHC Trac Issues: #15259, #14963

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

comment:39 Changed 15 months ago by Remi

Cc: remi.turk@… added

comment:40 Changed 13 months ago by goldfire

See #15598 for another example of trouble in this water. That example might be simpler than this one.

Note: See TracTickets for help on using tickets.