Clean up Middle IR and variable assignment/reassignment #293
Conversation
|
@CAG2Mark The lifter is not really updated. There are tests failing as it does the wrong thing in some cases (see the diff of |
| val vars = t.definedVars.toSeq.filter(scope.lookup(_).isEmpty).sortBy(_.uid).iterator.map(l => | ||
|
|
||
| // TODO: now should be possible to just write: (currently causes problems to handlers) | ||
| // val vars = t.definedVars.toSeq.sortBy(_.uid).iterator.map(l => |
There was a problem hiding this comment.
@CAG2Mark @AnsonYeung There are probably many instances where the effect handler transformation reassigns non-mutable symbols instead of simply assigning them. In principle it should be possible to make this change, but it breaks code that uses handlers.
There was a problem hiding this comment.
Indeed the continuation class implicitly captures the variables so all assignments within continuation should be reassignments really (including the this.pc assignments), even though they might not be assigned or initialized in the first place yet.
A brief remark of why capturing is needed: any function could define other nested function that capture the same variable, and it is important for the continuation class to capture the same one.
There was a problem hiding this comment.
Indeed the continuation class implicitly captures the variables so all assignments within continuation should be reassignments really (including the this.pc assignments), even though they might not be assigned or initialized in the first place yet.
I'm not following. Doesn't the continuation class just duplicate the function's implementation? If the function was assigning the variable for the first time (maybe the variable is even immutable), then the continuation class should also be assigning that variable for the first time in the corresponding part of its duplicated code. No?
There was a problem hiding this comment.
Oh right, so we need proper binding site probably. With the changes in the comment above it would make the initial assignment to create a local variable at the function declaration which is incorrect as the resume function can return a effectsig and resume later which uses the same variable again.
There was a problem hiding this comment.
Ah right, I see. That's probably why it doesn't work currently.
We should list the variables that are bound within a given MIR block body.
| @@ -486,7 +489,7 @@ class HandlerLowering(paths: HandlerPaths)(using TL, Raise, Elaborator.State, El | |||
|
|
|||
| val resumedVal = VarSymbol(Tree.Ident("value$")) | |||
|
|
|||
| def createAssignment(sym: Local) = Assign(sym, resumedVal.asPath, End()) | |||
| def createAssignment(sym: Local) = Reassign(sym, resumedVal.asPath, End()) // TODO: or is this `Assign`? | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
This is always a reassignment, as the old symbol would hold the previous EffectSig
| case _ => initial | ||
|
|
||
| val remaining = rewritten match | ||
| case Assign(lhs: InnerSymbol, rhs, rest) => ctx.getIsymPath(lhs) match | ||
|
|
||
| // TODO (LP:) not sure about the semantics of these "Reassign" cases |
|
In today's meeting, we figured that we currently don't actually need this change. We just need to change the way |
I'm trying to refactor the currently messy handling of variables in MIR.
vals (that have no owner), private fields defined withletandval-less class parameters, etc.