Avoid unnecessary capture of PC variables when hoisting functions #1265
Conversation
mario-bucev
force-pushed
the
trim-innerfuns-signature
branch
from
968b62a
to
e70064a
Compare
mario-bucev
force-pushed
the
trim-innerfuns-signature
branch
from
e70064a
to
876f776
Compare
| val varsCond = exprOps.variablesOf(cond) | ||
| if (varsCond.intersect(fdsFreeVars).nonEmpty) varsCond | ||
| else Set.empty[Variable] | ||
| case _ => Set.empty[Variable] |
There was a problem hiding this comment.
Nit: what about dependent types? Shouldn't we also include their free variables?
This might also affect the CloseBound cases.
There was a problem hiding this comment.
Doesn't variablesOf also take into consideration the FVs occurring in types? Having a look at it, it calls typeOps.variablesOf, but maybe I've missed something.
There was a problem hiding this comment.
Yes, it does. But you need to actually call it on vd.tpe to get the free variables, e.g.
case Path.OpenBound(vd) => exprOps.variablesOf(vd.tpe)There was a problem hiding this comment.
I see, thanks for the clarification!
There was a problem hiding this comment.
Hopefully the pushed changes cover the dependent types. The fdsFreeVars now includes the FVs of types (if any).
For CloseBound (1st case), we do not need to take the FVs of vd.tpe as it's already in fdsFreeVars.
For the 2nd case, the FVs of vd.tpe will be picked up by the next iteration of step.
Otherwise, the vd is not relevant for this inner function and is dropped.
| case Path.CloseBound(vd, e) if fdsFreeVars.contains(vd.toVariable) => | ||
| exprOps.variablesOf(e) | ||
| case Path.CloseBound(vd, e) if exprOps.variablesOf(e).intersect(fdsFreeVars).nonEmpty => | ||
| // `vd` may occur in a constraint somewhere, which can also transitively constraint the FVs |
There was a problem hiding this comment.
Nit: the second 'constraint' should be 'constrain'
|
Thanks for the review! I'll address the remarks, in the meantime, I mark the PR as a draft to avoid any accidental merge... |
| exprOps.variablesOf(e) | ||
| case Path.CloseBound(vd, e) if exprOps.variablesOf(e).intersect(fdsFreeVars).nonEmpty => | ||
| // `vd` may occur in a constraint somewhere, which can also transitively constraint the FVs | ||
| // `vd` may occur in a constrain somewhere, which can also transitively constraint the FVs |
There was a problem hiding this comment.
Oups, I meant the other 'constraint' :) the sentence should be "vd may occur in a constraint somewhere, which can also transitively constrain the FVs"
Addresses #1259
Edit: marked as a draft as I've found some bugs (that seem to be related to not taking into account function specifications)
Edit 2: the bug I've encountered has nothing to do with this PR... so I'm going to re-ready it (I'll try to fix the big in a separate PR).