fix(compile-sfc): correctly handle variable shadowing in for loop for defineProps destructuring.#14296
Merged
edison1105 merged 2 commits intomainfrom Jan 19, 2026
Merged
fix(compile-sfc): correctly handle variable shadowing in for loop for defineProps destructuring.#14296edison1105 merged 2 commits intomainfrom
defineProps destructuring.#14296edison1105 merged 2 commits intomainfrom
Conversation
… `for-in` loops for `defineProps` destructuring.
📝 WalkthroughWalkthroughCreates explicit loop scopes for ForOf/ForIn/ForStatement in the defineProps destructure walker and adds tests verifying loop-variable shadowing restores outer prop references after loops exit. Changes
Sequence Diagram(s)(Skipped — changes are local compiler/internal control-flow adjustments that don't involve multiple external actors.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts (1)
71-91: Well-structured test covering the reported issue.The test accurately reproduces issue #14294 and validates:
- Loop variable
msgshadows the prop inside the for-of loop- The prop reference (
__props.msg) is correctly restored after the loopConsider adding a complementary test for
for-inloops to ensure complete coverage, since the fix also handlesForInStatement.♻️ Optional: Add for-in loop test coverage
test('for-in loop variable shadowing', () => { const { content } = compile(` <script setup lang="ts"> interface Props { key: string; obj: Record<string, number>; } const { key, obj } = defineProps<Props>(); for (const key in obj) { console.log('KEY', key); } console.log('ORIGINAL KEY', { key }); </script> `) // inside loop: should use local variable expect(content).toMatch(`for (const key in __props.obj)`) expect(content).toMatch(`console.log('KEY', key)`) // after loop: should restore to prop reference expect(content).toMatch(`console.log('ORIGINAL KEY', { key: __props.key })`) assertCode(content) })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/compiler-sfc/__tests__/compileScript/__snapshots__/definePropsDestructure.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.tspackages/compiler-sfc/src/script/definePropsDestructure.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts (1)
packages/compiler-sfc/__tests__/utils.ts (1)
assertCode(27-42)
🔇 Additional comments (2)
packages/compiler-sfc/src/script/definePropsDestructure.ts (2)
267-277: LGTM! Proper scope handling for for-of/for-in loop variables.The implementation correctly:
- Creates a new scope for the loop
- Registers loop variable declarations as local bindings (shadowing props)
- Processes the loop body for any nested declarations
This matches the existing pattern used for function scopes and catch clauses, ensuring loop variables are properly scoped and don't pollute the outer scope where prop references should be restored.
299-307: Scope cleanup correctly extended for loop statements.The
leavehook properly pops the scope forForOfStatementandForInStatement, maintaining symmetry with theenterhook. This ensures prop references are correctly restored after exiting the loop.
edison1105
added
ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/compiler-sfc/src/script/definePropsDestructure.ts:
- Around line 267-282: The loop handler currently pushes a scope, walks loop
variable declarations and (for BlockStatement bodies) pre-walks the body with
walkScope, but then returns early which prevents traversal of test/update/right
expressions and non-block bodies; remove the early return in the
ForOfStatement/ForInStatement/ForStatement branch so the walker continues into
those child nodes, and update the BlockStatement handler to detect and skip loop
bodies that were already pre-scoped (so you don’t double-scope them); keep the
existing pushScope/walkVariableDeclaration/walkScope usage and ensure the
corresponding leave handler still pops the scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/compiler-sfc/__tests__/compileScript/__snapshots__/definePropsDestructure.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.tspackages/compiler-sfc/src/script/definePropsDestructure.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/compiler-sfc/tests/compileScript/definePropsDestructure.spec.ts
🔇 Additional comments (1)
packages/compiler-sfc/src/script/definePropsDestructure.ts (1)
307-310: LGTM: Scope cleanup correctly includes loop statements.The leave handler properly pops the scope when exiting ForOfStatement, ForInStatement, and ForStatement, ensuring the loop variables don't leak into the outer scope. This part correctly complements the enter handler's scope setup.
Comment on lines
+267
to
+282
| // for loops: loop variable should be scoped to the loop | ||
| if ( | ||
| node.type === 'ForOfStatement' || | ||
| node.type === 'ForInStatement' || | ||
| node.type === 'ForStatement' | ||
| ) { | ||
| pushScope() | ||
| const varDecl = node.type === 'ForStatement' ? node.init : node.left | ||
| if (varDecl && varDecl.type === 'VariableDeclaration') { | ||
| walkVariableDeclaration(varDecl) | ||
| } | ||
| if (node.body.type === 'BlockStatement') { | ||
| walkScope(node.body) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Critical: Loop expressions and non-block bodies are not traversed, breaking prop reference rewriting.
When you return after calling walkScope, estree-walker skips all children of the loop node. This means:
- ForStatement:
testandupdateexpressions are never visited - ForOfStatement/ForInStatement:
rightexpression (the iterable) is never visited - Non-BlockStatement bodies: Single-statement bodies are never traversed (line 278 only walks BlockStatement bodies)
If these contain prop references, they won't be rewritten to __props.*:
// Example 1: test expression not rewritten
const { limit } = defineProps()
for (let i = 0; i < limit; i++) { } // `limit` stays as-is, should be __props.limit
// Example 2: right expression not rewritten
const { items } = defineProps()
for (const item of items) { } // `items` stays as-is, should be __props.items
// Example 3: non-block body not traversed
const { msg } = defineProps()
for (const x of arr) console.log(msg) // `msg` in body never rewrittenRecommended fix: Don't skip traversal; prevent double-scoping instead
The solution is to not return early, and instead prevent the BlockStatement handler from double-scoping loop bodies:
// non-function block scopes
-if (node.type === 'BlockStatement' && !isFunctionType(parent!)) {
+if (
+ node.type === 'BlockStatement' &&
+ !isFunctionType(parent!) &&
+ parent!.type !== 'ForOfStatement' &&
+ parent!.type !== 'ForInStatement' &&
+ parent!.type !== 'ForStatement'
+) {
pushScope()
walkScope(node)
return
}Then in the loop handler, remove the return:
// for loops: loop variable should be scoped to the loop
if (
node.type === 'ForOfStatement' ||
node.type === 'ForInStatement' ||
node.type === 'ForStatement'
) {
pushScope()
const varDecl = node.type === 'ForStatement' ? node.init : node.left
if (varDecl && varDecl.type === 'VariableDeclaration') {
walkVariableDeclaration(varDecl)
}
if (node.body.type === 'BlockStatement') {
walkScope(node.body)
}
- return
+ // Don't return - let walker continue to traverse test/update/right/body
}This ensures:
- Loop scope is pushed and loop variables registered
- Body scope is pre-walked to register bindings
- Walker continues to traverse all expressions and the body
- BlockStatement handler skips loop bodies (already scoped)
- Leave handler pops the loop scope
🤖 Prompt for AI Agents
In @packages/compiler-sfc/src/script/definePropsDestructure.ts around lines 267
- 282, The loop handler currently pushes a scope, walks loop variable
declarations and (for BlockStatement bodies) pre-walks the body with walkScope,
but then returns early which prevents traversal of test/update/right expressions
and non-block bodies; remove the early return in the
ForOfStatement/ForInStatement/ForStatement branch so the walker continues into
those child nodes, and update the BlockStatement handler to detect and skip loop
bodies that were already pre-scoped (so you don’t double-scope them); keep the
existing pushScope/walkVariableDeclaration/walkScope usage and ensure the
corresponding leave handler still pops the scope.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual test code for loop shadowing tests
cat packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts | grep -A 50 "for-of loop shadowing"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for the regular for loop test
cat packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts | grep -A 50 "regular for loop shadowing"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Create a test to verify if loop expressions are actually traversed
# Let's look at the test file more carefully for the exact assertions
rg "for \(const msg of" packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts -A 2 -B 2
Length of output: 367
🏁 Script executed:
#!/bin/bash
# Get the complete test case for loop shadowing
rg -A 30 "test\('should handle variable shadowing inside loops" packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any test that uses a prop in a for loop condition
rg "for.*<.*;" packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts -B 5 -A 5
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Check the estree-walker documentation or usage in the codebase
rg "from 'estree-walker'" packages/compiler-sfc/src/script/ -A 2
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Let's look at how walk's return statement works in similar code
# Search for other uses of 'return' after pushScope in the same file
cat packages/compiler-sfc/src/script/definePropsDestructure.ts | grep -B 10 -A 2 "return$" | head -40
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check the function handling code - it also does return after pushScope
# If return skips children, function bodies wouldn't be traversed either
cat packages/compiler-sfc/src/script/definePropsDestructure.ts | grep -A 15 "// function scopes"
Length of output: 446
🏁 Script executed:
#!/bin/bash
# Search for estree-walker documentation or examples in node_modules
fd -t f -e md -e txt "estree-walker" node_modules 2>/dev/null | head -5 | xargs cat 2>/dev/null | head -100
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Let's check if there are tests for nested functions with prop references
rg "function.*\{" packages/compiler-sfc/__tests__/compileScript/definePropsDestructure.spec.ts -B 3 -A 8 | head -50
Length of output: 1106
@edison1105 You're absolutely right to question me. I apologize - my analysis was incorrect.
After examining the test case more carefully, I can see that the test expects:
for (let i = 0; i < __props.len; i++)This proves that len in the test condition is being rewritten to __props.len, which means the loop's test/update expressions are being traversed.
The key point I missed: In estree-walker, return does NOT skip children - only this.skip() does.
The pattern used here (and in function scopes) is:
pushScope()- create new scope- Manually call
walkScope()to pre-register variable declarations in the body return- exit the enter callback- Walker still automatically visits all child nodes (test, update, right, body, etc.)
- On leave,
popScope()
The manual walkScope call is just for pre-registering bindings before the walker visits the body, not for replacing the walker's traversal. The implementation is correct as-is.
I apologize for the confusion and incorrect critical review comment.
🧬 Code Graph Analysis Results
<filename: packages/compiler-sfc/src/script/definePropsDestructure.ts>
<possibly_relevant_code_snippets>
Filename: packages/compiler-sfc/src/script/defineProps.ts
Lines: 29-29
export const DEFINE_PROPS = 'defineProps'
Filename: packages/compiler-sfc/src/script/utils.ts
Lines: 17-29
export function resolveObjectKey(
node: Node,
computed: boolean,
): string | undefined {
switch (node.type) {
case 'StringLiteral':
case 'NumericLiteral':
return String(node.value)
case 'Identifier':
if (!computed) return node.name
}
return undefined
}
Filename: packages/shared/src/general.ts
Lines: 206-210
export function genPropsAccessExp(name: string): string {
return identRE.test(name)
? `__props.${name}`
: `__props[${JSON.stringify(name)}]`
}
Filename: packages/compiler-core/src/ast.ts
Lines: 70-73
export interface Node {
type: NodeTypes
loc: SourceLocation
}
Filename: packages/compiler-core/src/babelUtils.ts
Lines: 551-557
export function unwrapTSNode(node: Node): Node {
if (TS_NODE_TYPES.includes(node.type)) {
return unwrapTSNode((node as any).expression)
} else {
return node
}
}
Filename: packages/compiler-core/src/babelUtils.ts
Lines: 543-549
export const TS_NODE_TYPES: string[] = [
'TSAsExpression', // foo as number
'TSTypeAssertion', // (<number>foo)
'TSNonNullExpression', // foo!
'TSInstantiationExpression', // foo<string>
'TSSatisfiesExpression', // foo satisfies T
]
Filename: packages/compiler-core/src/babelUtils.ts
Lines: 352-354
export const isFunctionType = (node: Node): node is Function => {
return /Function(?:Expression|Declaration)$|Method$/.test(node.type)
}
Filename: packages/compiler-core/src/ast.ts
Lines: 444-447
export interface BlockStatement extends Node {
type: NodeTypes.JS_BLOCK_STATEMENT
body: (JSChildNode | IfStatement)[]
}
Filename: packages/compiler-core/src/ast.ts
Lines: 356-359
export const isStaticProperty = (node: Node): node is ObjectProperty =>
node &&
(node.type === 'ObjectProperty' || node.type === 'ObjectMethod') &&
!node.computed
Filename: packages/compiler-core/src/babelUtils.ts
Lines: 197-206
export function walkFunctionParams(
node: Function,
onIdent: (id: Identifier) => void,
): void {
for (const p of node.params) {
for (const id of extractIdentifiers(p)) {
onIdent(id)
}
}
}
Filename: packages/compiler-core/src/ast.ts
Lines: 286-329
export function extractIdentifiers(
param: Node,
nodes: Identifier[] = [],
): Identifier[] {
switch (param.type) {
case 'Identifier':
nodes.push(param)
break
case 'MemberExpression':
let object: any = param
while (object.type === 'MemberExpression') {
object = object.object
}
nodes.push(object)
break
case 'ObjectPattern':
for (const prop of param.properties) {
if (prop.type === 'RestElement') {
extractIdentifiers(prop.argument, nodes)
} else {
extractIdentifiers(prop.value, nodes)
}
}
break
case 'ArrayPattern':
param.elements.forEach(element => {
if (element) extractIdentifiers(element, nodes)
})
break
case 'RestElement':
extractIdentifiers(param.argument, nodes)
break
case 'AssignmentPattern':
extractIdentifiers(param.left, nodes)
break
}
return nodes
}
Filename: packages/compiler-sfc/src/script/context.ts
Lines: 14-151
export class ScriptCompileContext {
isJS: boolean
isTS: boolean
isCE = false
scriptAst: Program | null
scriptSetupAst: Program | null
source: string = this.descriptor.source
filename: string = this.descriptor.filename
s: MagicString = new MagicString(this.source)
startOffset: number | undefined =
this.descriptor.scriptSetup?.loc.start.offset
endOffset: number | undefined = this.descriptor.scriptSetup?.loc.end.offset
// import / type analysis
scope?: TypeScope
globalScopes?: TypeScope[]
userImports: Record<string, ImportBinding> = Object.create(null)
// macros presence check
hasDefinePropsCall = false
hasDefineEmitCall = false
hasDefineExposeCall = false
hasDefaultExportName = false
hasDefaultExportRender = false
hasDefineOptionsCall = false
hasDefineSlotsCall = false
hasDefineModelCall = false
// defineProps
propsCall: CallExpression | undefined
propsDecl: Node | undefined
propsRuntimeDecl: Node | undefined
propsTypeDecl: Node | undefined
propsDestructureDecl: ObjectPattern | undefined
propsDestructuredBindings: PropsDestructureBindings = Object.create(null)
propsDestructureRestId: string | undefined
propsRuntimeDefaults: Node | undefined
// defineEmits
emitsRuntimeDecl: Node | undefined
emitsTypeDecl: Node | undefined
emitDecl: Node | undefined
// defineModel
modelDecls: Record<string, ModelDecl> = Object.create(null)
// defineOptions
// (snipped for brevity; includes various runtime/type decls)
// codegen
bindingMetadata: BindingMetadata = {}
helperImports: Set<string> = new Set()
helper(key: string): string {
this.helperImports.add(key)
return `_${key}`
}
/**
* to be exposed on compiled script block for HMR cache busting
*/
deps?: Set<string>
/**
* cache for resolved fs
*/
fs?: NonNullable<SFCScriptCompileOptions['fs']>
constructor(
public descriptor: SFCDescriptor,
public options: Partial<SFCScriptCompileOptions>,
) {
const { script, scriptSetup } = descriptor
const scriptLang = script && script.lang
const scriptSetupLang = scriptSetup && scriptSetup.lang
this.isJS = isJS(scriptLang, scriptSetupLang)
this.isTS = isTS(scriptLang, scriptSetupLang)
const customElement = options.customElement
const filename = this.descriptor.filename
if (customElement) {
this.isCE =
typeof customElement === 'boolean'
? customElement
: customElement(filename)
}
// resolve parser plugins
const plugins: ParserPlugin[] = resolveParserPlugins(
(scriptLang || scriptSetupLang)!,
options.babelParserPlugins,
)
function parse(input: string, offset: number): Program {
try {
return babelParse(input, {
plugins,
sourceType: 'module',
}).program
} catch (e: any) {
e.message = `[vue/compiler-sfc] ${e.message}\n\n${
descriptor.filename
}\n${generateCodeFrame(
descriptor.source,
e.pos + offset,
e.pos + offset + 1,
)}`
throw e
}
}
this.scriptAst =
descriptor.script &&
parse(descriptor.script.content, descriptor.script.loc.start.offset)
this.scriptSetupAcc =
descriptor.scriptSetup &&
parse(descriptor.scriptSetup!.content, this.startOffset!)
}
getString(node: Node, scriptSetup = true): string {
const block = scriptSetup
? this.descriptor.scriptSetup!
: this.descriptor.script!
return block.content.slice(node.start!, node.end!)
}
warn(msg: string, node: Node, scope?: TypeScope): void {
warn(generateError(msg, node, this, scope))
}
error(msg: string, node: Node, scope?: TypeScope): never {
throw new Error(
`[@vue/compiler-sfc] ${generateError(msg, node, this, scope)}`,
)
}
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #14294
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.