[GHC] #15345: Duplication between `CoreSubst` and `SimplEnv` is very unfortunate
#15345: Duplication between `CoreSubst` and `SimplEnv` is very unfortunate -------------------------------------+------------------------------------- Reporter: mpickering | Owner: (none) Type: task | Status: new Priority: normal | Milestone: 8.6.1 Component: Compiler | Version: 8.4.3 Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Test Case: | Blocked By: Blocking: | Related Tickets: Differential Rev(s): | Wiki Page: -------------------------------------+------------------------------------- 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! -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/15345 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#15345: Duplication between `CoreSubst` and `SimplEnv` is very unfortunate -------------------------------------+------------------------------------- Reporter: mpickering | Owner: (none) Type: task | Status: new Priority: normal | Milestone: 8.6.1 Component: Compiler | Version: 8.4.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by mpickering): 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. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/15345#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#15345: Duplication between `CoreSubst` and `SimplEnv` is very unfortunate -------------------------------------+------------------------------------- Reporter: mpickering | Owner: (none) Type: task | Status: new Priority: normal | Milestone: 8.8.1 Component: Compiler | Version: 8.4.3 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj):
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? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/15345#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC