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.

2 files changed, 16 insertions, 14 deletions

diff --git a/compiler/coreSyn/CoreSubst.lhs b/compiler/coreSyn/CoreSubst.lhs
index 582ece2124..e08cdb8faa 100644
--- 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]
+\end{code}
 
-{-
 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.
 
+* 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
@@ -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
--}
 
+\begin{code}
 -- | 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"
+--
+-- 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
@@ -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
-
-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
diff --git a/compiler/simplCore/SimplEnv.lhs b/compiler/simplCore/SimplEnv.lhs
index 3cdbb3291d..70e0fa1149 100644
--- 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
-
-isEmptySimplSubst :: SimplEnv -> Bool
-isEmptySimplSubst (SimplEnv { seTvSubst = tvs, seIdSubst = ids })
-  = isEmptyVarEnv tvs && isEmptyVarEnv ids
 \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
-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}