Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted parser fix that mirrors the existing parseBlockStatement() pattern.
What was reviewed:
- Confirmed
m_insideSwitchCaseBodyis only read at the twosemanticFailIfTruesites inparseStatementListItem(), so clearing it inparseFunctionBody()affects nothing else. - Verified
SetForScoperestores the flag so ausingafter the nested function in the same clause (and in subsequent clauses via thewhileloop inparseSwitchClauses) is still rejected — both covered by the new tests. - Checked the reset sits alongside the
m_statementDepth = 0reset for the same fresh-statement-list reason, and that the empty-body early return above it needs no change.
Extended reasoning...
Overview
This PR fixes a false-positive SyntaxError in the JSC parser: using / await using declarations at the top level of a function body were incorrectly rejected when that function was lexically nested inside a switch case/default clause. The Explicit Resource Management proposal only forbids using directly in a CaseClause's StatementList, not inside nested function bodies.
The production change is 3 lines in Parser.cpp:2381-2383: a SetForScope switchCaseScope(m_insideSwitchCaseBody, false) in parseFunctionBody(), placed immediately after the existing DepthManager / m_statementDepth = 0 reset that exists for the identical reason (a function body is a fresh statement list regardless of what it's nested in). This is the same RAII idiom already used at Parser.cpp:2073 in parseBlockStatement().
The other file is a new 148-line stress test covering 25 kinds of function bodies × 6 wrapper contexts (all must parse), 19 negative cases confirming the flag is correctly restored (direct-in-clause, after-nested-function, cross-clause, nested-switch-in-nested-function), and two runtime checks that disposal actually happens.
Security risks
None. This change only relaxes an over-restrictive syntax error to match spec/V8 behavior. It doesn't touch codegen, memory, JIT, or any security-sensitive path — it's a boolean flag reset in the parser's semantic-error checking.
Level of scrutiny
Low-to-medium. The mechanism is trivial (one RAII boolean reset matching an existing pattern 300 lines up), but parser correctness matters. I traced every read and write of m_insideSwitchCaseBody: it's written in parseSwitchClauses (1940), parseSwitchDefaultClause (1971), parseBlockStatement (2073), and now parseFunctionBody (2383); it's read only at the two semanticFailIfTrue checks (810, 845). The SetForScope in parseSwitchClauses at 1940 lives across the while (match(CASE)) loop, so restoration after a nested function body correctly leaves the flag true for the remainder of the current clause and subsequent clauses — the test case 0: function f() { } case 1: using x = null; exercises exactly this.
Other factors
- The PR description is exceptionally detailed, including cross-verification of all 169 test expectations against V8 (node 26).
- The fix is placed after the empty-body early return (
match(CLOSEBRACE)), which is fine — an empty body has no statements to check. - Placing the reset before the
ArrowFunctionBodyExpressionbranch is harmless (expression bodies can't contain declarations) and keeps it adjacent to them_statementDepthreset. - The author notes
Parser.cppmatches upstream WebKit here, so this is upstreamable. - No prior reviews or comments on the timeline; bug hunting system found nothing.
|
Warning Review limit reached
Next review available in: 22 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between caad865 and 23e89c28f7f09075deb3be2dc8da33e5a44075bd. 📒 Files selected for processing (2)
WalkthroughThe parser now permits top-level ChangesNested resource declarations
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
Preview Builds
|
|
Verification with this PR's preview build (
Once this merges, the Bun PR moves its pin to the merge commit. |
…d in a switch case clause m_insideSwitchCaseBody is set while the statements of a case/default clause are parsed and only cleared again by parseBlockStatement(). A function body is parsed through parseFunctionBody(), which goes straight to parseSourceElements(), so a `using` or `await using` declaration at the top level of a function, arrow, method, getter, setter, generator or class field initializer function that is lexically inside a case clause was rejected with "'using' declaration is not allowed directly in a switch case or default clause" while the enclosing code was parsed. Clear the flag for the duration of parseFunctionBody(), next to the m_statementDepth reset that exists for the same reason. The flag is restored when the body has been parsed, so a declaration directly in the clause after the nested function is still rejected.
23e89c2 to
c9731a2
Compare
There was a problem hiding this comment.
LGTM — a one-line SetForScope reset that mirrors the existing pattern in parseBlockStatement().
What was reviewed:
- Confirmed
m_insideSwitchCaseBodyis only read at the twosemanticFailIfTruesites inparseStatementListItem(), and only otherwise reset inparseBlockStatement()— so scoping the reset toparseFunctionBody()is the right place with no side effects. - Checked that
SetForScoperestores the flag on exit, and the tests cover the after-function-in-clause and nested-switch-in-nested-function cases that would break if it didn't. - Verified the reset sits alongside the
m_statementDepth = 0reset for the same reason (fresh statement list); the arrow-expression-body branch is unaffected since it can't contain a statement.
Extended reasoning...
Overview
This PR fixes a false-positive syntax error in the JSC parser: a using / await using declaration at the top level of a function body was being rejected when that function was lexically nested inside a switch case: / default: clause. The fix is a single line in Source/JavaScriptCore/parser/Parser.cpp (parseFunctionBody()): a SetForScope switchCaseScope(m_insideSwitchCaseBody, false); next to the existing m_statementDepth = 0 reset. A new 148-line stress test in JSTests/stress/ covers 25 function-body forms × 6 wrapper contexts (150 accept cases), 19 must-still-reject cases, and two runtime disposal-order checks.
Security risks
None. This is a parse-time acceptance change: previously-rejected valid code is now accepted, matching the Explicit Resource Management spec (the CaseClause restriction applies only to the clause's own StatementList) and V8's behavior. It doesn't touch codegen, memory, or any privileged path — the flag it resets is read exclusively at two semanticFailIfTrue guard sites and nowhere else.
Level of scrutiny
Low-to-moderate. Parser.cpp is a critical file, but this change is one effective line using an established RAII idiom (SetForScope) applied to the same flag in exactly the way parseBlockStatement() already does at line ~2073. There is no new logic, no new state, no control-flow change. The placement is correct: after the empty-body early return (irrelevant — no statements) and before both the block-body and arrow-expression-body parse paths. The RAII restore guarantees the flag is back for statements that follow the function in the enclosing clause, and the test suite explicitly verifies that (e.g. case 0: function f() { using y = null; } using x = null; still throws).
Other factors
- Grep of
m_insideSwitchCaseBodyconfirms it is set inparseSwitchClauses()/parseSwitchDefaultClause(), cleared inparseBlockStatement(), and now cleared inparseFunctionBody()— those are the only writers; the only readers are the twousing/await usingsemantic checks. The blast radius is fully understood. - Verification is thorough: the PR description and follow-up comment record that the new test fails on the current pin, passes on the preview build, the 41 existing
using/await using/DisposableStack stress tests still pass, and the 150/19 expectations were cross-checked against V8 (node 26). A companion Bun PR pins the preview build and passes. - The test file is comprehensive and well-structured — it covers restoration after every function form, nested switches inside nested functions, and actual runtime dispose ordering, not just parse acceptance.
- No outstanding reviewer requests; CodeRabbit had no findings; no prior claude[bot] review on this PR.
- The inherited upstream CODEOWNERS lists
@WebKit/jsc-reviewersfor this path, but that file is explicitly for auto-assigning reviewers ("Contributors do not 'own' WebKit components"), and this fork's recent JSC/WTF merges (#432, #428, #424) follow the same flow.
1 participant
Symptom
A
usingorawait usingdeclaration at the top level of a function that is lexically inside acase:/default:clause is rejected while the enclosing code is parsed, although it is not directly in the clause:Both are valid (the restriction in the proposal applies to the CaseClause's own StatementList only), and V8 accepts them. Every kind of nested body is affected: function declarations and expressions, arrows, methods, getters/setters, generators, async functions, class methods, the function in a class field initializer and a function in a parameter default. Wrapping the declaration in a block inside the function is the only workaround. Bun hit this with
await usinginside an async arrow in a switch case (the sync form is hit just the same once the initializer is not a constant).Cause
m_insideSwitchCaseBodyis set byparseSwitchClauses()/parseSwitchDefaultClause()for the statements of the clause and is only cleared again byparseBlockStatement().parseFunctionBody()parses the body withparseSourceElements()directly, never throughparseBlockStatement(), so the flag is still set when the nested body's statements are parsed and the checks inparseStatementListItem()fire. The body is only checked this way during the enclosing parse (a later lazy reparse of the function starts with a fresh parser), which is why the error appears at load time of the enclosing code.Change
parseFunctionBody()clears the flag with aSetForScopefor the duration of the body, next to them_statementDepthreset that exists for the same reason (a function body is a fresh statement list, whatever it is nested in). The flag is restored afterwards, socase 0: function f() {} using x = r;is still rejected. Class static blocks already go throughparseBlockStatement()and were fine.Parser.cppis identical to upstreammainhere, so the same fix applies upstream.JSTests/stress/using-declaration-in-function-inside-switch-case.jsevaluates every kind of nested body containingusingandawait usinginside case and default clauses (all must parse), keeps the declarations directly in a clause rejected (including right after a nested function has been parsed, and in a switch nested in such a function), and checks that the declarations in the nested functions actually dispose their resources.Related, not fixed here
Two more parser flags leak into nested function bodies the same way, independently of this one (both reproduce identically on the current pin and on this PR's preview; V8 accepts all of them). They are tracked separately and would each get the same kind of reset:
m_parserState.allowAwait: a non-async function nested in an async function's parameter defaults,async function f(x = function () { var await = 1; }) {}, is rejected with "Cannot use 'await' as a variable name in an async function" (the override atparseFunctionInfoonly covers the nested function's own parameters, not its body).m_allowsIn: a block-bodied arrow in a for-loop init,for (var f = () => { return 'a' in {}; }; false;) {}, is rejected at thein(function expressions are fine becauseparseUnaryExpressionre-enables it; arrows never go through that path).Verification
jscshell of the currentautobuild-caad865eprebuilt (unfixed): fails on the first body; taken apart, all 75 nested-body cases it checks are rejected with the message above and all 19 should-still-throw cases throw.autobuild-preview-pr-423-23e89c28): the new test passes on itsjsc, the 41 existingusing/await using/ DisposableStack stress tests still pass, and Bump WebKit: allow using declarations in a function nested in a switch case clause bun#38285 (which pins the build) passes the same test as a jsc-stress fixture plus the user-facing shapes run by Bun; both fail on the current pin.