Duplication between `CoreSubst` and `SimplEnv` is very unfortunate

There are lots of very similar functions in SimplEnv and CoreSubst.

For example, in SimpleEnv there is a function substIdType and likewise in CoreSubst but they are different to each other.

There is also a substIdBndr function in both modules but the one in SimplEnv calls SimpleEnv.substIdType, the one in CoreSubst doesn't call CoreSubst.substIdType but has its own inlined version. This means there's at least three different versions of substIdType floating around. The correctness of this function is crucial so it would be better not to duplicate it.

Can we remove this duplication? Help me Simon!

Trac metadata

Trac field	Value
Version	8.4.3
Type	Task
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

changed milestone to %8.6.1

changed weight to 5

added Ttask Trac import labels

And I found another function which is rather like substIdBnd, CoreOpt.subst_opt_id_bndr. I don't think there should be four functions doing this very similar task of applying a substitution to a binder.

changed milestone to %8.8.1

These won't be fixed for in GHC 8.6.

Help me Simon!

I agree this is vary far from ideal

• CoreSubst.substIdBndr takes a Subst

• It substitutes in the IdInfo

• CoreOpt.subst_opt_id_bndr is similar to CoreSubst.substIdBndr, but

  * It zaps the IdInfo.

• SimplEnv.substIdBndr takes a SimplEnv.

• For CoVars it makes a TCvSubst, and calls TyCoRep.substCoVarBndr

• For Ids it calls SimplEnv.substNonCoVarBndr, which

• zaps the IdInfo (because the simplifier will simplify and re-add it)

• and for Ids it can cope with join points; I'll add a Note to explain this.

• Why are CoVars treated differently to other Ids? Because they can occur in types, so their bindings must be in the TCvSubst.

Things we could improve relatively easily:

• SimplEnv.substIdBndr does the Id/CoVar split, calling substCoVarBndr and substNonCoVarIdBndr resp. But CoreSubst.substIdBndr assumes a non-CoVar Id; the split is done by CoreSubst.substBndr. This is inconsistent.
• CoreSubst.substIdBndr should call CoreSubst.substIdType rather than copying its code.
• SimplEnv.substIdType could probably just call CoreSubst.substIdType
• I suppose that CoreOpt.subst_opt_id_bndr could zap the IdInfo and then call CoreSubst.substIdBndr. Slightly less efficient but more modular; and this is not heavily used code.

Does any of that help?

changed milestone to %8.10.1

Bumping milestones of low-priority tickets.

added Pnormal label

removed milestone