diff options
author | Simon Peyton Jones <simonpj@microsoft.com> | 2018-12-20 17:49:34 +0000 |
---|---|---|
committer | Simon Peyton Jones <simonpj@microsoft.com> | 2018-12-21 16:54:17 +0000 |
commit | a57d5c4d3e39ab9ac2c31431b5e38818359fa5b5 (patch) | |
tree | 1a048f360c7ab359703ae869dcdd8e693094becb /compiler/iface/LoadIface.hs | |
parent | 66ce7de15dbc594e6890b5651dba3aa669c8d5fc (diff) | |
download | haskell-a57d5c4d3e39ab9ac2c31431b5e38818359fa5b5.tar.gz |
Fix treatment of hi-boot files and dfuns
Trac #16038 exposed the fact that TcRnDriver.checkHiBootIface
was creating a binding, in the module being compiled, for
$fxBlah = $fBlah
but $fxBlah was a /GlobalId/. But all bindings should be for
/LocalIds/ else dependency analysis goes down the tubes.
* I added a CoreLint check that an occurrence of a GlobalId
is not bound by an binding of a LocalId. (There is already
a binding-site check that no binding binds a GlobalId.)
* I refactored (and actually signficantly simplified) the
tricky code for dfuns in checkHiBootIface to ensure that
we get LocalIds for those boot-dfuns.
Alas, I then got "duplicate instance" messages when compiling
HsExpr. It turns out that this is a long-standing, but extremely
delicate, bug: even before this patch, if you compile HsExpr
with -ddump-tc-trace, you get "duplicate instance". Without
-ddump-tc-trace, it's OK. What a mess!
The reason for the duplicate-instance is now explained in
Note [Loading your own hi-boot file] in LoadIface. I fixed
it by a Gross Hack in LoadIface.loadInterface. This is at
least no worse than before.
But there should be a better way. I have opened #16081 for this.
Diffstat (limited to 'compiler/iface/LoadIface.hs')
-rw-r--r-- | compiler/iface/LoadIface.hs | 93 |
1 files changed, 63 insertions, 30 deletions
diff --git a/compiler/iface/LoadIface.hs b/compiler/iface/LoadIface.hs index bff507f973..87a6beb3ff 100644 --- a/compiler/iface/LoadIface.hs +++ b/compiler/iface/LoadIface.hs @@ -418,15 +418,7 @@ loadInterface doc_str mod from -- READ THE MODULE IN ; read_result <- case (wantHiBootFile dflags eps mod from) of Failed err -> return (Failed err) - Succeeded hi_boot_file -> - -- Stoutly warn against an EPS-updating import - -- of one's own boot file! (one-shot only) - --See Note [Do not update EPS with your own hi-boot] - -- in MkIface. - WARN( hi_boot_file && - fmap fst (if_rec_types gbl_env) == Just mod, - ppr mod ) - computeInterface doc_str hi_boot_file mod + Succeeded hi_boot_file -> computeInterface doc_str hi_boot_file mod ; case read_result of { Failed err -> do { let fake_iface = emptyModIface mod @@ -488,9 +480,20 @@ loadInterface doc_str mod from } } - ; updateEps_ $ \ eps -> + ; let bad_boot = mi_boot iface && fmap fst (if_rec_types gbl_env) == Just mod + -- Warn warn against an EPS-updating import + -- of one's own boot file! (one-shot only) + -- See Note [Loading your own hi-boot file] + -- in MkIface. + + ; WARN ( bad_boot, ppr mod ) + updateEps_ $ \ eps -> if elemModuleEnv mod (eps_PIT eps) || is_external_sig dflags iface - then eps else + then eps + else if bad_boot + -- See Note [Loading your own hi-boot file] + then eps { eps_PTE = addDeclsToPTE (eps_PTE eps) new_eps_decls } + else eps { eps_PIT = extendModuleEnv (eps_PIT eps) mod final_iface, eps_PTE = addDeclsToPTE (eps_PTE eps) new_eps_decls, @@ -525,26 +528,56 @@ loadInterface doc_str mod from ; return (Succeeded res) }}}} +{- Note [Loading your own hi-boot file] +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Generally speaking, when compiling module M, we should not +load M.hi boot into the EPS. After all, we are very shortly +going to have full information about M. Moreover, see +Note [Do not update EPS with your own hi-boot] in MkIface. + +But there is a HORRIBLE HACK here. + +* At the end of tcRnImports, we call checkFamInstConsistency to + check consistency of imported type-family instances + See Note [The type family instance consistency story] in FamInst + +* Alas, those instances may refer to data types defined in M, + if there is a M.hs-boot. + +* And that means we end up loading M.hi-boot, because those + data types are not yet in the type environment. + +But in this wierd case, /all/ we need is the types. We don't need +instances, rules etc. And if we put the instances in the EPS +we get "duplicate instance" warnings when we compile the "real" +instance in M itself. Hence the strange business of just updateing +the eps_PTE. + +This really happens in practice. The module HsExpr.hs gets +"duplicate instance" errors if this hack is not present. + +This is a mess. + + +Note [HPT space leak] (#15111) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +In IfL, we defer some work until it is demanded using forkM, such +as building TyThings from IfaceDecls. These thunks are stored in +the ExternalPackageState, and they might never be poked. If we're +not careful, these thunks will capture the state of the loaded +program when we read an interface file, and retain all that data +for ever. + +Therefore, when loading a package interface file , we use a "clean" +version of the HscEnv with all the data about the currently loaded +program stripped out. Most of the fields can be panics because +we'll never read them, but hsc_HPT needs to be empty because this +interface will cause other interfaces to be loaded recursively, and +when looking up those interfaces we use the HPT in loadInterface. +We know that none of the interfaces below here can refer to +home-package modules however, so it's safe for the HPT to be empty. +-} - --- Note [HPT space leak] (#15111) --- --- In IfL, we defer some work until it is demanded using forkM, such --- as building TyThings from IfaceDecls. These thunks are stored in --- the ExternalPackageState, and they might never be poked. If we're --- not careful, these thunks will capture the state of the loaded --- program when we read an interface file, and retain all that data --- for ever. --- --- Therefore, when loading a package interface file , we use a "clean" --- version of the HscEnv with all the data about the currently loaded --- program stripped out. Most of the fields can be panics because --- we'll never read them, but hsc_HPT needs to be empty because this --- interface will cause other interfaces to be loaded recursively, and --- when looking up those interfaces we use the HPT in loadInterface. --- We know that none of the interfaces below here can refer to --- home-package modules however, so it's safe for the HPT to be empty. --- dontLeakTheHPT :: IfL a -> IfL a dontLeakTheHPT thing_inside = do let |