Commit a57d5c4d authored by Simon Peyton Jones's avatar Simon Peyton Jones

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.
parent 66ce7de1
......@@ -2041,12 +2041,15 @@ lintUnliftedCoVar cv
data LintEnv
= LE { le_flags :: LintFlags -- Linting the result of this pass
, le_loc :: [LintLocInfo] -- Locations
, le_subst :: TCvSubst -- Current type substitution; we also use this
-- to keep track of all the variables in scope,
-- both Ids and TyVars
, le_joins :: IdSet -- Join points in scope that are valid
-- A subset of teh InScopeSet in le_subst
-- See Note [Join points]
, le_subst :: TCvSubst -- Current type substitution
-- We also use le_subst to keep track of
-- /all variables/ in scope, both Ids and TyVars
, le_joins :: IdSet -- Join points in scope that are valid
-- A subset of the InScopeSet in le_subst
-- See Note [Join points]
, le_dynflags :: DynFlags -- DynamicFlags
}
......@@ -2304,17 +2307,30 @@ applySubstCo :: InCoercion -> LintM OutCoercion
applySubstCo co = do { subst <- getTCvSubst; return (substCo subst co) }
lookupIdInScope :: Id -> LintM Id
lookupIdInScope id
| not (mustHaveLocalBinding id)
= return id -- An imported Id
| otherwise
= do { subst <- getTCvSubst
; case lookupInScope (getTCvInScope subst) id of
Just v -> return v
Nothing -> do { addErrL out_of_scope
; return id } }
lookupIdInScope id_occ
= do { subst <- getTCvSubst
; case lookupInScope (getTCvInScope subst) id_occ of
Just id_bnd -> do { checkL (not (bad_global id_bnd)) global_in_scope
; return id_bnd }
Nothing -> do { checkL (not is_local) local_out_of_scope
; return id_occ } }
where
out_of_scope = pprBndr LetBind id <+> text "is out of scope"
is_local = mustHaveLocalBinding id_occ
local_out_of_scope = text "Out of scope:" <+> pprBndr LetBind id_occ
global_in_scope = hang (text "Occurrence is GlobalId, but binding is LocalId")
2 (pprBndr LetBind id_occ)
bad_global id_bnd = isGlobalId id_occ
&& isLocalId id_bnd
&& not (isWiredInName (idName id_occ))
-- 'bad_global' checks for the case where an /occurrence/ is
-- a GlobalId, but there is an enclosing binding fora a LocalId.
-- NB: the in-scope variables are mostly LocalIds, checked by lintIdBndr,
-- but GHCi adds GlobalIds from the interactive context. These
-- are fine; hence the test (isLocalId id == isLocalId v)
-- NB: when compiling Control.Exception.Base, things like absentError
-- are defined locally, but appear in expressions as (global)
-- wired-in Ids after worker/wrapper
-- So we simply disable the test in this case
lookupJoinId :: Id -> LintM (Maybe JoinArity)
-- Look up an Id which should be a join point, valid here
......@@ -2325,14 +2341,11 @@ lookupJoinId id
Just id' -> return (isJoinId_maybe id')
Nothing -> return Nothing }
lintTyCoVarInScope :: Var -> LintM ()
lintTyCoVarInScope v = lintInScope (text "is out of scope") v
lintInScope :: SDoc -> Var -> LintM ()
lintInScope loc_msg var =
do { subst <- getTCvSubst
; lintL (not (mustHaveLocalBinding var) || (var `isInScope` subst))
(hsep [pprBndr LetBind var, loc_msg]) }
lintTyCoVarInScope :: TyCoVar -> LintM ()
lintTyCoVarInScope var
= do { subst <- getTCvSubst
; lintL (var `isInScope` subst)
(pprBndr LetBind var <+> text "is out of scope") }
ensureEqTys :: OutType -> OutType -> MsgDoc -> LintM ()
-- check ty2 is subtype of ty1 (ie, has same structure but usage
......
......@@ -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
......
......@@ -84,57 +84,61 @@ defined in module B.
How do we ensure that we maintain the necessary consistency?
* Call a module which defines at least one type family instance a
"family instance module". This flag `mi_finsts` is recorded in the
interface file.
"family instance module". This flag `mi_finsts` is recorded in the
interface file.
* For every module we calculate the set of all of its direct and
indirect dependencies that are family instance modules. This list
`dep_finsts` is also recorded in the interface file so we can compute
this list for a module from the lists for its direct dependencies.
indirect dependencies that are family instance modules. This list
`dep_finsts` is also recorded in the interface file so we can compute
this list for a module from the lists for its direct dependencies.
* When type checking a module M we check consistency of all the type
family instances that are either provided by its `dep_finsts` or
defined in the module M itself. This is a pairwise check, i.e., for
every pair of instances we must check that they are consistent.
family instances that are either provided by its `dep_finsts` or
defined in the module M itself. This is a pairwise check, i.e., for
every pair of instances we must check that they are consistent.
- For family instances coming from `dep_finsts`, this is checked in
checkFamInstConsistency, called from tcRnImports. See Note
[Checking family instance consistency] for details on this check (and
in particular how we avoid having to do all these checks for every
module we compile).
- For family instances coming from `dep_finsts`, this is checked in
checkFamInstConsistency, called from tcRnImports. See Note
[Checking family instance consistency] for details on this check
(and in particular how we avoid having to do all these checks for
every module we compile).
- That leaves checking the family instances defined in M itself
against instances defined in either M or its `dep_finsts`. This is
checked in `tcExtendLocalFamInstEnv'.
- That leaves checking the family instances defined in M itself
against instances defined in either M or its `dep_finsts`. This is
checked in `tcExtendLocalFamInstEnv'.
There are two subtle points in this scheme which have not been
There are four subtle points in this scheme which have not been
addressed yet.
* We have checked consistency of the family instances *defined* by M
or its imports, but this is not by definition the same thing as the
family instances *used* by M or its imports. Specifically, we need to
ensure when we use a type family instance while compiling M that this
instance was really defined from either M or one of its imports,
rather than being an instance that we happened to know about from
reading an interface file in the course of compiling an unrelated
module. Otherwise, we'll end up with no record of the fact that M
depends on this family instance and type safety will be compromised.
See #13102.
or its imports, but this is not by definition the same thing as the
family instances *used* by M or its imports. Specifically, we need to
ensure when we use a type family instance while compiling M that this
instance was really defined from either M or one of its imports,
rather than being an instance that we happened to know about from
reading an interface file in the course of compiling an unrelated
module. Otherwise, we'll end up with no record of the fact that M
depends on this family instance and type safety will be compromised.
See #13102.
* It can also happen that M uses a function defined in another module
which is not transitively imported by M. Examples include the
desugaring of various overloaded constructs, and references inserted
by Template Haskell splices. If that function's definition makes use
of type family instances which are not checked against those visible
from M, type safety can again be compromised. See #13251.
which is not transitively imported by M. Examples include the
desugaring of various overloaded constructs, and references inserted
by Template Haskell splices. If that function's definition makes use
of type family instances which are not checked against those visible
from M, type safety can again be compromised. See #13251.
* When a module C imports a boot module B.hs-boot, we check that C's
type family instances are compatible with those visible from
B.hs-boot. However, C will eventually be linked against a different
module B.hs, which might define additional type family instances which
are inconsistent with C's. This can also lead to loss of type safety.
See #9562.
type family instances are compatible with those visible from
B.hs-boot. However, C will eventually be linked against a different
module B.hs, which might define additional type family instances which
are inconsistent with C's. This can also lead to loss of type safety.
See #9562.
* The call to checkFamConsistency for imported functions occurs very
early (in tcRnImports) and that causes problems if the imported
instances use type declared in the module being compiled.
See Note [Loading your own hi-boot file] in LoadIface.
-}
{-
......
This diff is collapsed.
F.hs:1:1: error:
instance O.O F.F -- Defined at F.hs-boot:6:10
is defined in the hs-boot file, but not in the module itself
F.hs-boot:5:1: error:
‘F.F’ is exported by the hs-boot file, but not exported by the module
F.hs-boot:6:10: error:
instance O.O F.F
is defined in the hs-boot file, but not in the module itself
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment