loop capture per iteration - #11596
Conversation
|
(Eric Anderson (@humanapp) in case you want to take a look, had some notable gpt assistance but took a stab at both this and #11597 based off your last two issues, still validating / need to test hardware but so far think got it) |
tested on office gamego
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Positionless synthetic captures can bypass loop-block hoisting and cause compilation or capture failures.
Get a fresh assessment by requesting another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
pxtcompiler/emitter/emitter.ts — Handle positionless captures in loop-block hoisting View comment |
What changed in this PR
Fixes per-iteration capture semantics for let bindings in for and for...of loops.
Changes:
- Creates fresh captured bindings at correct loop boundaries.
- Corrects loop-local hoisted function placement and labeled-statement emission.
- Adds semantic, native assembly, and hardware regression coverage.
| File | Description |
|---|---|
pxtcompiler/emitter/emitter.ts |
Implements per-iteration bindings and loop-local function hoisting. |
tests/compile-test/lang-test0/58loopcapture.ts |
Adds comprehensive semantic tests. |
tests/compile-test/lang-test0/README-codegen.md |
Documents loop-capture coverage. |
tests/thumb-test/cases/loopcapture.ts |
Adds native compiler probe. |
tests/thumb-test/asmchecks.ts |
Verifies generated assembly structure. |
tests/hw-ab/gen-project.js |
Adds hardware A/B coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| info.capturedVars.sort((a, b) => b.pos - a.pos) | ||
| const vinfo = getVarInfo(info.capturedVars[0]) | ||
| if (!vinfo.functionsToDefine) | ||
| vinfo.functionsToDefine = [] | ||
| vinfo.functionsToDefine.push(node) | ||
| const scope = node.parent; | ||
| if (scope.kind === SK.Block && inLoop(scope) && info.capturedVars[0].pos < scope.pos) { |
There was a problem hiding this comment.
narrow but looks like real edge case / right, build after fix https://arcade.makecode.com/app/7537abbebd27997ef4e4312ef1c218991a9add97-28ed852db5
|
Small nit: This PR and #11597 both add a test file with numeric prefix |
|
Fix looks good! The additional work of boxing/unboxing the captured values is unfortunate, but unavoidable and is the expected cost here. |
| function emitLabeledStatement(node: LabeledStatement) { | ||
| let l = getLabels(node.statement) | ||
| emit(node.statement) | ||
| proc.emitLblDirect(l.brk) |
There was a problem hiding this comment.
This is interesting. Did you find a different compiler bug? Were labeled loops broken?
There was a problem hiding this comment.
yes, this was a small side case of this emitting the same label a second time while going through it. Probably a case 0 people will ever hit but might as well fix, repro i came up with from description:
let count: number = 0;
outer: for (let i = 0; i < 3; i++) {
count++;
if (i === 1) break outer;
}
game.splash("Count: " + count);
on live hw compilation breaks, fixed in branch
(and re: test file, i'll swap 11597 to be 59 before merging yeah / after incorporating your suggested change on that one)
close #11563
build here https://arcade.makecode.com/app/48bf6ee7932fd01859f48cc3cadc2211a3ba2253-baa5c5f82b#pub:S11575-26979-25273-96005; should see rainbow diagonal of numbers, vs https://arcade.makecode.com/#pub:S11575-26979-25273-96005 being set at 14 / black so it shows nothing on screen