Skip to content

fix: scope leakage in getOutVarNames for lambda, try-catch, and traditional for-loop - #468

Open
chenjunwenhao wants to merge 1 commit into
alibaba:mainfrom
chenjunwenhao:fix/getOutVarNames-scope-leakage
Open

fix: scope leakage in getOutVarNames for lambda, try-catch, and traditional for-loop#468
chenjunwenhao wants to merge 1 commit into
alibaba:mainfrom
chenjunwenhao:fix/getOutVarNames-scope-leakage

Conversation

@chenjunwenhao

Copy link
Copy Markdown
Contributor

Summary

  • Lambda parameter leakage: Lambda parameters (both (a, b) -> formal params and x -> single-param shorthand) were not scoped to the lambda body, causing them to pollute the outer scope. This led to false negatives (external a masked by leaked lambda param) and false positives (single-param x never registered, flagged as external).
  • Catch variable not registered: The catch exception variable (e.g. e in catch(e) { ... }) was never added to the catch block's scope, so it was incorrectly reported as an external variable.
  • Traditional for-loop variable leakage: Variables declared in traditional for-loops (e.g. i in for(int i = 0; i < n; i++)) leaked into the outer scope, causing false negatives when i was used after the loop.

Root Cause

ScopeStackVisitor (the base class for OutVarNamesVisitor and OutVarAttrsVisitor) was missing scope push/pop for lambda expressions and traditional for-loops, and the existing visitTryCatch pushed a scope but never registered the catch variable name.

Fix

All three fixes are applied in ScopeStackVisitor (the base class) following the push/pop pattern from PR #463, so both OutVarNamesVisitor and OutVarAttrsVisitor benefit:

  • visitLambdaExpr: Push scope → register params (handling both varId and FormalOrInferredParameterList) → visit body → pop scope
  • visitTryCatch: Push scope → register catch variable → visit catch body → pop scope
  • visitTraditionalForStatement: Push scope → visit init/condition/update/body → pop scope
  • visitForEachStatement: Moved from OutVarNamesVisitor to the base class (same logic)

Tests

7 new test cases added in Express4RunnerTest:

  • getOutVarNamesLambdaFormalParamsTest - formal params (a, b) scoped correctly
  • getOutVarNamesLambdaSingleParamTest - single param x -> scoped correctly
  • getOutVarNamesLambdaParamNotLeakTest - lambda param doesn't leak to outer scope
  • getOutVarNamesCatchVariableTest - catch variable e not reported as external
  • getOutVarNamesCatchVariableMultipleCatchesTest - multiple catch blocks
  • getOutVarNamesTraditionalForLoopTest - loop variable i scoped correctly
  • getOutVarNamesTraditionalForLoopVarNotLeakTest - loop variable doesn't leak after loop

All existing getOutVarNames* and getOutVarAttrs* tests continue to pass.

…tional for-loop

Three scope management bugs in ScopeStackVisitor caused incorrect
results from getOutVarNames():

1. Lambda parameters leaked into the outer scope (both formal params
   and single-param shorthand), causing false negatives and false
   positives in external variable detection.

2. Catch exception variables (e.g. 'e' in 'catch(e)') were never
   registered in the catch block scope, causing false positives.

3. Traditional for-loop variables (e.g. 'i' in 'for(int i = 0; ...)')
   leaked into the outer scope, causing false negatives.

All three fixes follow the push/pop scope pattern established by
the for-each fix in PR alibaba#463, and are applied in the base
ScopeStackVisitor class so both OutVarNamesVisitor and
OutVarAttrsVisitor benefit.

Co-authored-by: QoderWork
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant