Proper handling of local variables capturing inside a loop body #351
Conversation
|
Nice! Please first resolve the conflicts and then request my review. |
LPTK
left a comment
LPTK
left a comment
There was a problem hiding this comment.
Good work, but let's not claim victory too fast.
| case Label(lbl, loop, bod, rst) => | ||
| scope.allocateName(lbl) | ||
|
|
||
| // [fixme:0] TODO check scope and allocate local variables here (see: https://github.com/hkust-taco/mlscript/pull/293#issuecomment-2792229849) |
There was a problem hiding this comment.
This is not actually fixed, as while loops aren't always rewritten yet...
There was a problem hiding this comment.
In the latest commit I've added this back, but it shouldn't be an issue after loop rewriting. If a loop does not contain any local function, loop local variables can be treated as function locals since there is no capturing.
There was a problem hiding this comment.
Yes, that's right, but it's better to keep the doc/tests in sync with what's actually implemented at the moment.
AnsonYeung
left a comment
AnsonYeung
left a comment
There was a problem hiding this comment.
I've addressed the comments.
| case Label(lbl, loop, bod, rst) => | ||
| scope.allocateName(lbl) | ||
|
|
||
| // [fixme:0] TODO check scope and allocate local variables here (see: https://github.com/hkust-taco/mlscript/pull/293#issuecomment-2792229849) |
There was a problem hiding this comment.
In the latest commit I've added this back, but it shouldn't be an issue after loop rewriting. If a loop does not contain any local function, loop local variables can be treated as function locals since there is no capturing.
| //│ lambda$here = runtime.safeCall(lambda2(foo$capture6)); | ||
| //│ return lambda$here | ||
| //│ } else { | ||
| //│ foo$capture6.tmp$capture$1 = runtime.Unit; |
There was a problem hiding this comment.
I just noticed this crazy capture of a temporary variable that just stores the unit value. It's pretty crazy. I wonder why Unit is bound to a temporary variable in the first place.
2 participants
Previously, variables inside a loop body are treated the same as that of the function body, which leads to bad semantics when a function captures a local variable defined within a loop body.
As mentioned in #293 (comment), this PR lowers while loop into tail recursive function and rely on the scoping of the function for the correct semantics. This is done during lowering as we already know the location of inserting Continue statements and we just need to convert these into tail calls.
runtime.LoopEndis introduced to handle the case when a return statement occur inside the loop body. This value is returned when the loop exits without early return and is tested to determine whether to return from the outer function.