Centralize loop guard injection into AST transformation#399
Centralize loop guard injection into AST transformation#399tracygardner wants to merge 1 commit intomainfrom
Conversation
…enerators Add `injectLoopGuards(src)` to the flock object, which uses acorn/acorn-walk to parse generated code and automatically insert `await wait(0)` before the closing brace of any loop body that doesn't already contain an await expression. This centralises the loop-guard concern so individual generators no longer need to embed it inline. Remove the explicit `await wait(0)` from controls_whileUntil, controls_doWhile, controls_repeat_ext, and controls_forEach. The more sophisticated timing/RAF-based guards in controls_for and for_loop are preserved; because those bodies already contain an AwaitExpression, injectLoopGuards skips them automatically. The injection runs in runCode() after AST validation, so it also protects any hand-written user code that contains unguarded loops. https://claude.ai/code/session_01Dvex7RparGEMpvAAR2iF1H
📝 WalkthroughWalkthroughThe PR refactors loop delay injection by introducing a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flock.js`:
- Around line 666-692: The loop instrumentation currently inserts "await
wait(0)" everywhere (in walk.simple handlers for WhileStatement,
DoWhileStatement, ForStatement, ForInStatement, ForOfStatement), which yields
invalid syntax inside non-async scopes; update the AST traversal to track
async-capable ancestry (push/pop a flag when entering FunctionDeclaration,
FunctionExpression, ArrowFunctionExpression, MethodDefinition, and
GeneratorFunction nodes and consider top-level/module await rules) and only call
addGuardIfNeeded when the current ancestry indicates await is legal (i.e.,
inside an async function or allowed top-level); keep the insertion logic
(insertions sort + splice into result) unchanged but ensure you reference the
existing addGuardIfNeeded and the walk/simple handlers so guards are only added
where await is syntactically valid.
- Around line 689-691: The injected guard currently calls the bare identifier
wait which can be shadowed by user code; change the injected call to a reserved
helper name like __flockLoopGuardWait__ (replace the "await wait(0);" injection
with "await __flockLoopGuardWait__(0);"), export that identifier from
createWhitelist() so it is available to the runtime, and add
__flockLoopGuardWait__ to the REJECT_IDENTIFIERS set checked by
validateUserCodeAST() so user code cannot define or shadow it.
- Around line 642-663: The loop-guardging logic must (1) normalize
single-statement loop bodies into BlockStatement so constructs like while(true)
foo(); get guarded, (2) inject the guard as the first statement of the block
(not just before the closing brace) so a continue still executes the guard on
the next iteration, and (3) make bodyHasAwait ignore awaits inside nested
function declarations/expressions/arrow functions so an inner async function
containing await does not cause skipping; to implement: update addGuardIfNeeded
to wrap non-BlockStatement loop bodies in a BlockStatement (preserving the
original node) and record insertion positions at the block start (or better,
record the AST block node to insert the guard as its first statement instead of
using end-1), and change bodyHasAwait to use walk.simple but override/skipping
handlers for FunctionDeclaration/FunctionExpression/ArrowFunctionExpression (and
similar) so traversal does not descend into nested functions when detecting
AwaitExpression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc0ef1b7-f320-4d4a-b7be-cde880f57f6d
📒 Files selected for processing (2)
flock.jsgenerators/generators.js
💤 Files with no reviewable changes (1)
- generators/generators.js
| // Check whether a BlockStatement already contains any await expression | ||
| // (at any nesting depth). Loops that already yield don't need an extra guard. | ||
| function bodyHasAwait(node) { | ||
| let found = false; | ||
| walk.simple(node, { | ||
| AwaitExpression() { | ||
| found = true; | ||
| }, | ||
| }); | ||
| return found; | ||
| } | ||
|
|
||
| // Collect positions (just before the closing '}') where we'll inject | ||
| const insertions = []; | ||
|
|
||
| function addGuardIfNeeded(loopBody) { | ||
| if ( | ||
| loopBody?.type === "BlockStatement" && | ||
| !bodyHasAwait(loopBody) | ||
| ) { | ||
| insertions.push(loopBody.end - 1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure and locate the file
git ls-files | grep -E "flock\.js|\.js$" | head -20Repository: flipcomputing/flock
Length of output: 354
🏁 Script executed:
# Read the full context of flock.js around the lines mentioned
wc -l flock.jsRepository: flipcomputing/flock
Length of output: 76
🏁 Script executed:
# Get the context around lines 642-663 and 684-692
sed -n '620,710p' flock.js | cat -nRepository: flipcomputing/flock
Length of output: 4632
🏁 Script executed:
# Search for walk.simple usage to understand its behavior
rg "walk\." flock.js -A 3 -B 3 | head -100Repository: flipcomputing/flock
Length of output: 912
🏁 Script executed:
# Look for any test files or documentation about this feature
fd -e test -e spec -e example flock | head -20Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Search for continue statements in test context and loop examples
rg "continue" flock.js -B 3 -A 3Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Look for async function examples within loops in the codebase
rg "while|for.*{" flock.js -A 10 | grep -E "(while|for|async|await)" | head -30Repository: flipcomputing/flock
Length of output: 150
🏁 Script executed:
# Let's see the broader context around the problematic lines and the second location
sed -n '630,700p' flock.js | cat -nRepository: flipcomputing/flock
Length of output: 3397
🏁 Script executed:
# Search for imports to understand which walk library is being used
head -50 flock.js | grep -E "import|require|walk"Repository: flipcomputing/flock
Length of output: 1256
🏁 Script executed:
# Look for any comments or documentation about loop guards
rg "guard|yield|busy" flock.js -B 2 -A 2 | head -50Repository: flipcomputing/flock
Length of output: 2029
🏁 Script executed:
# Search for examples of loops with await to understand the pattern
rg "await" flock.js -B 5 -A 5 | head -80Repository: flipcomputing/flock
Length of output: 3561
🏁 Script executed:
# Check the second location mentioned (684-692)
sed -n '675,700p' flock.js | cat -nRepository: flipcomputing/flock
Length of output: 1266
🏁 Script executed:
# Let's verify the behavior of acorn-walk's simple function with a quick test
python3 << 'EOF'
# Let's reason through the acorn-walk behavior:
# walk.simple() is a recursive visitor that traverses the entire AST
# Test case 1: continue statement
# Code: while(true) { x++; continue; }
# Guard inserted at loopBody.end - 1 (before closing })
# Result: while(true) { x++; continue; await wait(0); }
# But continue jumps to next iteration, skipping the guard
# TRUE: continue escapes the guard
# Test case 2: single-statement
# Code: while(true) foo();
# loopBody.type would be ExpressionStatement, NOT BlockStatement
# addGuardIfNeeded checks: loopBody?.type === "BlockStatement"
# No guard is added
# TRUE: single-statement bodies not guarded
# Test case 3: nested async
# Code: while(true) { async function f() { await x; } }
# walk.simple traverses entire tree, finds AwaitExpression inside async function
# bodyHasAwait returns true, so NO guard is added
# But the loop itself doesn't yield - only the function yields if called
# If f() is never called, loop blocks UI
# TRUE: nested async awaits incorrectly treated as loop yields
print("All three escape conditions appear to be valid based on code analysis")
EOFRepository: flipcomputing/flock
Length of output: 132
🏁 Script executed:
# Let's also check if there's a "wait" function defined to understand the guard
rg "function wait|const wait|=.*wait.*=" flock.js | head -10Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
# Check for any PR context or comments about this feature
git log --oneline -20 2>/dev/null | head -10Repository: flipcomputing/flock
Length of output: 149
Loop guards can be bypassed in three ways, reintroducing UI-blocking loops.
The current implementation has three bypass mechanisms that defeat the guard:
-
continuestatements skip the tail-inserted guard — Since the guard is injected just before the closing}, acontinuejumps to the next iteration without executing it. -
Single-statement loop bodies are never guarded — The code checks
loopBody?.type === "BlockStatement", so loops likewhile (true) foo();are never instrumented. -
bodyHasAwait()treats awaits in nested async functions as loop yields — Sincewalk.simple()traverses the entire AST, anasync function() { await x; }inside the loop body causes the guard to be skipped, even though the loop itself doesn't yield if the function is never called.
All three reintroduce the "busy loop blocks the UI" failure mode. Normalizing eligible loop bodies to blocks and injecting the guard as the first statement of each iteration would close these gaps.
Also applies to: 684-692
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flock.js` around lines 642 - 663, The loop-guardging logic must (1) normalize
single-statement loop bodies into BlockStatement so constructs like while(true)
foo(); get guarded, (2) inject the guard as the first statement of the block
(not just before the closing brace) so a continue still executes the guard on
the next iteration, and (3) make bodyHasAwait ignore awaits inside nested
function declarations/expressions/arrow functions so an inner async function
containing await does not cause skipping; to implement: update addGuardIfNeeded
to wrap non-BlockStatement loop bodies in a BlockStatement (preserving the
original node) and record insertion positions at the block start (or better,
record the AST block node to insert the guard as its first statement instead of
using end-1), and change bodyHasAwait to use walk.simple but override/skipping
handlers for FunctionDeclaration/FunctionExpression/ArrowFunctionExpression (and
similar) so traversal does not descend into nested functions when detecting
AwaitExpression.
| walk.simple(ast, { | ||
| WhileStatement(node) { | ||
| addGuardIfNeeded(node.body); | ||
| }, | ||
| DoWhileStatement(node) { | ||
| addGuardIfNeeded(node.body); | ||
| }, | ||
| ForStatement(node) { | ||
| addGuardIfNeeded(node.body); | ||
| }, | ||
| ForInStatement(node) { | ||
| addGuardIfNeeded(node.body); | ||
| }, | ||
| ForOfStatement(node) { | ||
| addGuardIfNeeded(node.body); | ||
| }, | ||
| }); | ||
|
|
||
| // Apply from last to first so earlier positions stay valid | ||
| insertions.sort((a, b) => b - a); | ||
|
|
||
| let result = src; | ||
| for (const pos of insertions) { | ||
| result = | ||
| result.slice(0, pos) + | ||
| "await wait(0);\n" + | ||
| result.slice(pos); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, is await syntactically valid inside a non-async function body when the surrounding program is executed from an async IIFE?
💡 Result:
No. await is not syntactically valid inside the body of a non-async function, even if that function is called from (or nested inside) an async IIFE—the code will fail at parse time with a syntax error. await is only allowed inside async functions (and async generators), or at the top level of an ES module (“top-level await”). [1] [2]
Only inject guards inside async-capable scopes.
This code instruments loops anywhere in the AST, producing invalid JavaScript. For example, function tick() { while (true) {} } becomes syntactically invalid after transformation because await is not legal inside non-async function bodies—the code fails at parse time. The wrapper async IIFE does not change the syntax rules for nested functions, methods, or generators. Track async ancestry before inserting guards to ensure guards are only injected where await is syntactically valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flock.js` around lines 666 - 692, The loop instrumentation currently inserts
"await wait(0)" everywhere (in walk.simple handlers for WhileStatement,
DoWhileStatement, ForStatement, ForInStatement, ForOfStatement), which yields
invalid syntax inside non-async scopes; update the AST traversal to track
async-capable ancestry (push/pop a flag when entering FunctionDeclaration,
FunctionExpression, ArrowFunctionExpression, MethodDefinition, and
GeneratorFunction nodes and consider top-level/module await rules) and only call
addGuardIfNeeded when the current ancestry indicates await is legal (i.e.,
inside an async function or allowed top-level); keep the insertion logic
(insertions sort + splice into result) unchanged but ensure you reference the
existing addGuardIfNeeded and the walk/simple handlers so guards are only added
where await is syntactically valid.
| result = | ||
| result.slice(0, pos) + | ||
| "await wait(0);\n" + |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | head -20Repository: flipcomputing/flock
Length of output: 437
🏁 Script executed:
fd -t f "flock.js" | head -5Repository: flipcomputing/flock
Length of output: 71
🏁 Script executed:
wc -l flock.jsRepository: flipcomputing/flock
Length of output: 76
🏁 Script executed:
sed -n '680,700p' flock.jsRepository: flipcomputing/flock
Length of output: 875
🏁 Script executed:
rg "wait" flock.js -A 2 -B 2 | head -50Repository: flipcomputing/flock
Length of output: 2176
🏁 Script executed:
rg "createWhitelist\|wait" flock.js -B 3 -A 5 | head -100Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
rg "function wait\|const wait\|=.*wait" flock.js | head -20Repository: flipcomputing/flock
Length of output: 45
🏁 Script executed:
rg "wait" flock.js -B 5 -A 5 | grep -A 10 -B 10 "await wait"Repository: flipcomputing/flock
Length of output: 756
🏁 Script executed:
rg "function createWhitelist|createWhitelist" flock.js -B 2 -A 20Repository: flipcomputing/flock
Length of output: 2343
🏁 Script executed:
sed -n '900,1100p' flock.js | grep -A 50 "createWhitelist"Repository: flipcomputing/flock
Length of output: 2747
🏁 Script executed:
rg "return.*\{" flock.js -A 30 | grep -B 10 -A 30 "wait"Repository: flipcomputing/flock
Length of output: 2170
🏁 Script executed:
sed -n '900,1100p' flock.jsRepository: flipcomputing/flock
Length of output: 11739
🏁 Script executed:
sed -n '1000,1200p' flock.jsRepository: flipcomputing/flock
Length of output: 11072
🏁 Script executed:
rg "validateUserCodeAST" flock.js -B 3 -A 30Repository: flipcomputing/flock
Length of output: 2926
🏁 Script executed:
sed -n '500,700p' flock.js | grep -A 100 "REJECT_IDENTIFIERS"Repository: flipcomputing/flock
Length of output: 7628
🏁 Script executed:
rg "REJECT_IDENTIFIERS" flock.js -A 100 | head -150Repository: flipcomputing/flock
Length of output: 6659
Use a hygienic helper name for the injected guard.
The injected code calls bare wait, which is exposed as a public API in the whitelist. User code can shadow this identifier (const wait = 0, function wait() {}), causing the guard to malfunction at runtime. Route this through a reserved name like __flockLoopGuardWait__ that is both exposed from createWhitelist() and rejected in validateUserCodeAST() so user code cannot redefine it.
Suggested fix
- "await wait(0);\n" +
+ "await __flockLoopGuardWait__(0);\n" +Then expose __flockLoopGuardWait__ from createWhitelist() and add it to the REJECT_IDENTIFIERS set in validateUserCodeAST().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flock.js` around lines 689 - 691, The injected guard currently calls the bare
identifier wait which can be shadowed by user code; change the injected call to
a reserved helper name like __flockLoopGuardWait__ (replace the "await wait(0);"
injection with "await __flockLoopGuardWait__(0);"), export that identifier from
createWhitelist() so it is available to the runtime, and add
__flockLoopGuardWait__ to the REJECT_IDENTIFIERS set checked by
validateUserCodeAST() so user code cannot define or shadow it.
2 participants
Summary
Refactored loop guard injection from being hardcoded in individual code generators into a centralized AST transformation pass. This improves maintainability and ensures consistent loop yielding behavior across all loop types.
Key Changes
Added
injectLoopGuards()method inflock.jsthat:await wait(0);guards into loop bodies that don't already contain await expressionsUpdated
runCode()method to callinjectLoopGuards()on user code before execution, passing the instrumented code to the SES compartmentRemoved hardcoded loop guards from
generators/generators.js:await wait(0);from while loop generatorawait wait(0);from do-while loop generatorawait wait(0);from repeat/for loop generatorawait wait(0);from forEach loop generatorImplementation Details
bodyHasAwait()helper uses AST walking to detect if a loop body already contains await expressions, avoiding redundant guardshttps://claude.ai/code/session_01Dvex7RparGEMpvAAR2iF1H
Summary by CodeRabbit
New Features
Refactor