fix(compiler-core): identifiers in switch-case should not be inferred as references #13923
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds switch/case-aware traversal to babelUtils: imports Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Transformer as TemplateTransform
participant WalkIDs as walkIdentifiers
participant WalkSwitch as walkSwitchStatement
participant WalkBlock as walkBlockDeclarations
participant Codegen as Codegen
Transformer->>WalkIDs: traverse AST
WalkIDs->>WalkSwitch: encounter SwitchStatement (if no scopeIds)
rect rgb(235,245,235)
WalkSwitch->>WalkBlock: iterate cases, collect declarations (isVar? -> hoist)
WalkBlock-->>WalkIDs: report identifiers (case-local vs hoisted)
end
WalkIDs-->>Codegen: supply scope id mappings
Codegen->>Codegen: emit references (omit `_ctx.` for case-local non-var consts)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/compiler-core/src/babelUtils.ts (1)
195-220: Fix scoping: avoid collectingvarin block scopes and use strict equality
- Block-level traversal should skip
vardeclarations; otherwisevarbecomes block-scoped (incorrect). Only collectvarfrom loop initializers (function-scoped).- Use
===instead of==for type checks.Apply:
-export function walkBlockDeclarations( - block: BlockStatement | SwitchCase | Program, - onIdent: (node: Identifier) => void, -): void { - const body = block.type === 'SwitchCase' ? block.consequent : block.body - for (const stmt of body) { - if (stmt.type === 'VariableDeclaration') { - if (stmt.declare) continue - for (const decl of stmt.declarations) { - for (const id of extractIdentifiers(decl.id)) { - onIdent(id) - } - } - } else if ( +export function walkBlockDeclarations( + block: BlockStatement | SwitchCase | Program, + onIdent: (node: Identifier) => void, +): void { + const body = block.type === 'SwitchCase' ? block.consequent : block.body + for (const stmt of body) { + if (stmt.type === 'VariableDeclaration') { + if (stmt.declare) continue + // Only collect block-scoped bindings here; 'var' is function-scoped. + if (stmt.kind !== 'var') { + for (const decl of stmt.declarations) { + for (const id of extractIdentifiers(decl.id)) { + onIdent(id) + } + } + } + } else if ( stmt.type === 'FunctionDeclaration' || stmt.type === 'ClassDeclaration' ) { if (stmt.declare || !stmt.id) continue onIdent(stmt.id) } else if (isForStatement(stmt)) { - walkForStatement(stmt, true, onIdent) - } else if (stmt.type == 'SwitchStatement') { - walkSwitchStatement(stmt, true, onIdent) + // Capture loop initializer bindings; include 'var' so it leaks to the surrounding scope. + walkForStatement(stmt, true, onIdent) + } else if (stmt.type === 'SwitchStatement') { + // Delegate; inside this block we only want 'var' to bubble to this enclosing scope. + walkSwitchStatement(stmt, true, onIdent) } } }
🧹 Nitpick comments (1)
packages/compiler-core/__tests__/transforms/transformExpressions.spec.ts (1)
720-765: Nice coverage for case bodies; please addvar-in-switch and case-test reference checks
- Add a test asserting
vardeclared in a case is visible after the switch and not prefixed with_ctx.- Add a test asserting identifiers used in
case <expr>are still prefixed with_ctxwhen not locally declared.I can draft the exact tests to drop in this suite if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-core/__tests__/transforms/transformExpressions.spec.ts(1 hunks)packages/compiler-core/src/babelUtils.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/compiler-core/__tests__/transforms/transformExpressions.spec.ts (1)
packages/compiler-core/src/ast.ts (1)
InterpolationNode(249-252)
packages/compiler-core/src/babelUtils.ts (1)
packages/compiler-core/src/ast.ts (1)
BlockStatement(444-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (2)
packages/compiler-core/src/babelUtils.ts (2)
13-15: LGTM on type importsAdding SwitchCase and SwitchStatement types is correct.
85-88: Correct to introduce SwitchStatement handling hereThis integrates switch traversal into identifier walking as needed for the fix.
edison1105
added
scope: compiler
🔨 p3-minor-bug
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/compiler-core/src/babelUtils.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/src/babelUtils.ts (1)
packages/compiler-core/src/ast.ts (1)
BlockStatement(444-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (4)
packages/compiler-core/src/babelUtils.ts (4)
13-14: LGTM!The import of
SwitchCaseandSwitchStatementtypes from @babel/types is correct and necessary for the new switch-case handling functionality.
200-203: LGTM!The signature extension to include
SwitchCaseis correct, and the conditional body extraction logic properly handles the different node types.
85-92: Fix identifier scoping logic in switch statements.The current implementation has critical scoping issues that cause local variables in switch cases to be incorrectly prefixed with
_ctx.when referenced. Based on the past review comments from edison1105, this needs to follow the established pattern of checking for pre-computedscopeIdsfirst.Apply this diff to fix the scoping logic:
- } else if (node.type === 'SwitchStatement') { - if (node.scopeIds) { - node.scopeIds.forEach(id => markKnownIds(id, knownIds)) - } - // record switch case block-level local variables - walkSwitchStatement(node, false, id => - markScopeIdentifier(node, id, knownIds), - ) + } else if (node.type === 'SwitchStatement') { + if (node.scopeIds) { + node.scopeIds.forEach(id => markKnownIds(id, knownIds)) + } else { + // record switch case block-level local variables + walkSwitchStatement(node, false, id => + markScopeIdentifier(node, id, knownIds), + ) + }
255-275: Critical: Fix switch statement scoping to avoid incorrect _ctx prefixing.The current implementation has multiple severe scoping issues that will cause the exact bug this PR is meant to fix:
- Missing case test handling: The function doesn't collect identifiers from case test expressions (
cs.test), but these should be treated as references, not local declarations- Incorrect var scoping:
vardeclarations inside case bodies are being treated as block-scoped instead of function-scoped- Incomplete recursion: The function doesn't handle nested statements like function/class declarations or nested control structures
Based on the previous review feedback, apply this complete fix:
function walkSwitchStatement( stmt: SwitchStatement, isVar: boolean, onIdent: (id: Identifier) => void, ) { for (const cs of stmt.cases) { - for (const stmt of cs.consequent) { - if ( - stmt.type === 'VariableDeclaration' && - (stmt.kind === 'var' ? isVar : !isVar) - ) { - for (const decl of stmt.declarations) { - for (const id of extractIdentifiers(decl.id)) { - onIdent(id) + // Only collect declarations from the case body; case tests are references. + for (const s of cs.consequent) { + if (s.type === 'VariableDeclaration') { + if (s.declare) continue + if (s.kind === 'var' ? isVar : !isVar) { + for (const decl of s.declarations) { + for (const id of extractIdentifiers(decl.id)) { + onIdent(id) + } } } + } else if ( + s.type === 'FunctionDeclaration' || + s.type === 'ClassDeclaration' + ) { + if (s.declare || !s.id) continue + onIdent(s.id) + } else if (isForStatement(s)) { + // Respect 'var' vs block-scoped loop initializers. + walkForStatement(s, isVar, onIdent) + } else if (s.type === 'SwitchStatement') { + // Nested switch + walkSwitchStatement(s, isVar, onIdent) } } - walkBlockDeclarations(cs, onIdent) } }Please add tests to verify:
varin switch case remains function-scoped:switch (0) { case 0: var x = 1; } console.log(x)—xshould not be prefixed with_ctx.- Case test references are treated as references:
switch (v) { case foo: break }—fooshould be_ctx.foounless locally declared- Block-scoped declarations work correctly:
switch (0) { case 0: const y = 1; console.log(y); }—yshould not be prefixed with_ctx.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/compiler-core/src/babelUtils.ts (1)
199-224: Do not recordvarin block/switch scopes; fix walkBlockDeclarationswalkBlockDeclarations currently records function-scoped
varand passestrueto for/switch walkers — only block-scoped bindings should be recorded.File: packages/compiler-core/src/babelUtils.ts (walkBlockDeclarations)
export function walkBlockDeclarations( block: BlockStatement | SwitchCase | Program, onIdent: (node: Identifier) => void, ): void { const body = block.type === 'SwitchCase' ? block.consequent : block.body for (const stmt of body) { if (stmt.type === 'VariableDeclaration') { if (stmt.declare) continue + // Only block-scoped declarations here; 'var' is function-scoped. + if (stmt.kind === 'var') continue for (const decl of stmt.declarations) { for (const id of extractIdentifiers(decl.id)) { onIdent(id) } } } else if ( stmt.type === 'FunctionDeclaration' || stmt.type === 'ClassDeclaration' ) { if (stmt.declare || !stmt.id) continue onIdent(stmt.id) } else if (isForStatement(stmt)) { - walkForStatement(stmt, true, onIdent) + // Only collect block-scoped loop initializers here. + walkForStatement(stmt, false, onIdent) } else if (stmt.type === 'SwitchStatement') { - walkSwitchStatement(stmt, true, onIdent) + // Collect case-local block-scoped declarations. + walkSwitchStatement(stmt, false, onIdent) } } }Tests to add:
switch (0) { case 0: var x = 1 } x→ ensure no_ctx.xprefix.switch (0) { case 0: for (var i = 0; i < 1; i++) {} } i→ ensure no_ctx.iprefix.
🧹 Nitpick comments (2)
packages/compiler-core/src/babelUtils.ts (2)
85-92: Avoid re-walking when scopeIds already computed on SwitchStatementSkip traversal if scopeIds exist to prevent duplicate marking and extra work.
Apply:
} else if (node.type === 'SwitchStatement') { - if (node.scopeIds) { - node.scopeIds.forEach(id => markKnownIds(id, knownIds)) - } - // record switch case block-level local variables - walkSwitchStatement(node, false, id => - markScopeIdentifier(node, id, knownIds), - ) + if (node.scopeIds) { + node.scopeIds.forEach(id => markKnownIds(id, knownIds)) + } else { + // record switch case block-level local variables + walkSwitchStatement(node, false, id => + markScopeIdentifier(node, id, knownIds), + ) + } }
255-275: Simplify switch-case scanning and avoid misclassifying declarationsOnly collect declarations from case bodies; don’t pre-scan, and rely on walkBlockDeclarations (which should ignore
var) to handle functions/classes/let/const and nested structures.Apply:
-function walkSwitchStatement( - stmt: SwitchStatement, - isVar: boolean, - onIdent: (id: Identifier) => void, -) { - for (const cs of stmt.cases) { - for (const stmt of cs.consequent) { - if ( - stmt.type === 'VariableDeclaration' && - (stmt.kind === 'var' ? isVar : !isVar) - ) { - for (const decl of stmt.declarations) { - for (const id of extractIdentifiers(decl.id)) { - onIdent(id) - } - } - } - } - walkBlockDeclarations(cs, onIdent) - } -} +function walkSwitchStatement( + stmt: SwitchStatement, + _isVar: boolean, + onIdent: (id: Identifier) => void, +) { + for (const cs of stmt.cases) { + // Case test expressions are references, not declarations. + walkBlockDeclarations(cs, onIdent) + } +}Also add tests to assert that
case foo:treatsfooas a reference (gets_ctx.foounless locally declared).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/compiler-core/__tests__/transforms/transformExpressions.spec.ts(1 hunks)packages/compiler-core/src/babelUtils.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/compiler-core/tests/transforms/transformExpressions.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/src/babelUtils.ts (1)
packages/compiler-core/src/ast.ts (1)
BlockStatement(444-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (1)
packages/compiler-core/src/babelUtils.ts (1)
13-15: Type imports for SwitchCase/SwitchStatement: LGTMAccurate additions for switch traversal utilities.
…t scopeIds correctly
My brother @PBK-B likes to write JSX in Vue. He came across this problem.
Variables declared under a switch-case clause are added
_ctx.prefix when used.See Playground
Expected to show
Hello World.Summary by CodeRabbit
Bug Fixes
Tests