Fix nasty infelicity: do not short-cut empty substitution in the simplifier

I was perplexed about why an arity-related WARN was tripping. It took
me _day_ (sigh) to find that it was because SimplEnv.substExpr was taking
a short cut when the substitution was empty, thereby not subsituting for
Ids in scope, which must be done (CoreSubst Note [Extending the Subst]).

The fix is a matter of deleting the "optimisation".  Same with
CoreSubst.substSpec, although I don't know if that actually caused a
probem.

diff --git a/compiler/coreSyn/CoreSubst.lhs b/compiler/coreSyn/CoreSubst.lhs
index 582ece2..e08cdb8 100644 (file)
--- a/compiler/coreSyn/CoreSubst.lhs
+++ b/compiler/coreSyn/CoreSubst.lhs
@@ -89,8 +89,8 @@ data Subst
        --              Types.TvSubstEnv
        --
        -- INVARIANT 3: See Note [Extending the Subst]
        --              Types.TvSubstEnv
        --
        -- INVARIANT 3: See Note [Extending the Subst]

+\end{code}

 
 

-{-

 Note [Extending the Subst]
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 For a core Subst, which binds Ids as well, we make a different choice for Ids
 Note [Extending the Subst]
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 For a core Subst, which binds Ids as well, we make a different choice for Ids

@@ -118,6 +118,13 @@ In consequence:
   so we only extend the in-scope set.  Then we must look up in the in-scope
   set when we find the occurrence of x.
 
   so we only extend the in-scope set.  Then we must look up in the in-scope
   set when we find the occurrence of x.
 

+* The requirement to look up the Id in the in-scope set means that we
+  must NOT take no-op short cut in the case the substitution is empty.
+  We must still look up every Id in the in-scope set.
+
+* (However, we don't need to do so for expressions found in the IdSubst
+  itself, whose range is assumed to be correct wrt the in-scope set.)
+

 Why do we make a different choice for the IdSubstEnv than the TvSubstEnv?
 
 * For Ids, we change the IdInfo all the time (e.g. deleting the
 Why do we make a different choice for the IdSubstEnv than the TvSubstEnv?
 
 * For Ids, we change the IdInfo all the time (e.g. deleting the

@@ -129,8 +136,8 @@ Why do we make a different choice for the IdSubstEnv than the TvSubstEnv?
 
 * For TyVars, only coercion variables can possibly change, and they are 
   easy to spot
 
 * For TyVars, only coercion variables can possibly change, and they are 
   easy to spot

--}

 
 

+\begin{code}

 -- | An environment for substituting for 'Id's
 type IdSubstEnv = IdEnv CoreExpr
 
 -- | An environment for substituting for 'Id's
 type IdSubstEnv = IdEnv CoreExpr
 

@@ -256,6 +263,9 @@ instance Outputable Subst where
 \begin{code}
 -- | Apply a substititon to an entire 'CoreExpr'. Rememeber, you may only 
 -- apply the substitution /once/: see "CoreSubst#apply_once"
 \begin{code}
 -- | Apply a substititon to an entire 'CoreExpr'. Rememeber, you may only 
 -- apply the substitution /once/: see "CoreSubst#apply_once"

+--
+-- Do *not* attempt to short-cut in the case of an empty substitution!
+-- See Note [Extending the Subst]

 substExpr :: Subst -> CoreExpr -> CoreExpr
 substExpr subst expr
   = go expr
 substExpr :: Subst -> CoreExpr -> CoreExpr
 substExpr subst expr
   = go expr

@@ -493,11 +503,7 @@ substWorker subst (HasWorker w a)
 ------------------
 -- | Substitutes for the 'Id's within the 'WorkerInfo' given the new function 'Id'
 substSpec :: Subst -> Id -> SpecInfo -> SpecInfo
 ------------------
 -- | Substitutes for the 'Id's within the 'WorkerInfo' given the new function 'Id'
 substSpec :: Subst -> Id -> SpecInfo -> SpecInfo

-
-substSpec subst new_fn spec@(SpecInfo rules rhs_fvs)
-  | isEmptySubst subst
-  = spec
-  | otherwise
+substSpec subst new_fn (SpecInfo rules rhs_fvs)

   = seqSpecInfo new_rules `seq` new_rules
   where
     new_name = idName new_fn
   = seqSpecInfo new_rules `seq` new_rules
   where
     new_name = idName new_fn

diff --git a/compiler/simplCore/SimplEnv.lhs b/compiler/simplCore/SimplEnv.lhs
index 3cdbb32..70e0fa1 100644 (file)
--- a/compiler/simplCore/SimplEnv.lhs
+++ b/compiler/simplCore/SimplEnv.lhs
@@ -284,10 +284,6 @@ setSubstEnv env tvs ids = env { seTvSubst = tvs, seIdSubst = ids }
 
 mkContEx :: SimplEnv -> InExpr -> SimplSR
 mkContEx (SimplEnv { seTvSubst = tvs, seIdSubst = ids }) e = ContEx tvs ids e
 
 mkContEx :: SimplEnv -> InExpr -> SimplSR
 mkContEx (SimplEnv { seTvSubst = tvs, seIdSubst = ids }) e = ContEx tvs ids e

-
-isEmptySimplSubst :: SimplEnv -> Bool
-isEmptySimplSubst (SimplEnv { seTvSubst = tvs, seIdSubst = ids })
-  = isEmptyVarEnv tvs && isEmptyVarEnv ids

 \end{code}
 
 
 \end{code}
 
 

@@ -713,8 +709,8 @@ mkCoreSubst (SimplEnv { seInScope = in_scope, seTvSubst = tv_env, seIdSubst = id
     fiddle (ContEx tv id e) = CoreSubst.substExpr (mk_subst tv id) e
 
 substExpr :: SimplEnv -> CoreExpr -> CoreExpr
     fiddle (ContEx tv id e) = CoreSubst.substExpr (mk_subst tv id) e
 
 substExpr :: SimplEnv -> CoreExpr -> CoreExpr

-substExpr env expr
-  | isEmptySimplSubst env = expr
-  | otherwise            = CoreSubst.substExpr (mkCoreSubst env) expr
+substExpr env expr = CoreSubst.substExpr (mkCoreSubst env) expr
+  -- Do *not* short-cut in the case of an empty substitution
+  -- See CoreSubst: Note [Extending the Subst]

 \end{code}
 
 \end{code}