-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Treat assignments in unreachable code as local bindings #11666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3071,13 +3071,35 @@ export class Binder extends ParseTreeWalker { | |
| if (!this._moduleSymbolOnly) { | ||
| const dummyScopeGenerator = new DummyScopeGenerator(this._currentScope, this._nodeInfo); | ||
| dummyScopeGenerator.walk(statement); | ||
|
|
||
| // Assignments in unreachable code still make names local, matching | ||
| // CPython. Bind those names without type-checking the dead code. | ||
| this._bindNamesInUnreachableCode(statement); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private _bindNamesInUnreachableCode(node: ParseNode) { | ||
| // Directives apply to the entire scope, so process global and | ||
| // nonlocal declarations before binding any names. | ||
| new UnreachableDirectiveFinder( | ||
| (globalNode) => this.visitGlobal(globalNode), | ||
| (nonlocalNode) => this.visitNonlocal(nonlocalNode) | ||
| ).walk(node); | ||
|
|
||
| const bindTarget = (target: ExpressionNode) => { | ||
| this._bindPossibleTupleNamedTarget(target); | ||
| }; | ||
| const bindName = (name: NameNode) => { | ||
| this._bindNameToScope(this._currentScope, name); | ||
| }; | ||
|
|
||
| new UnreachableNameBinder(bindTarget, bindName).walk(node); | ||
| } | ||
|
|
||
| private _createStartFlowNode() { | ||
| const flowNode: FlowNode = { | ||
| flags: FlowFlags.Start, | ||
|
|
@@ -4865,6 +4887,196 @@ export class ReturnFinder extends ParseTreeWalker { | |
| } | ||
| } | ||
|
|
||
| // Discovers and processes `global` and `nonlocal` directives in unreachable code. | ||
| // Directives must be evaluated before binding names to ensure that later | ||
| // assignments do not bind as locals in the current scope. Nested function, | ||
| // class, and lambda bodies are skipped because their directives belong to | ||
| // nested scopes. | ||
| class UnreachableDirectiveFinder extends ParseTreeWalker { | ||
| constructor( | ||
| private readonly _bindGlobal: (node: GlobalNode) => void, | ||
| private readonly _bindNonlocal: (node: NonlocalNode) => void | ||
| ) { | ||
| super(); | ||
| } | ||
|
|
||
| override visitGlobal(node: GlobalNode): boolean { | ||
| this._bindGlobal(node); | ||
| return false; | ||
| } | ||
|
|
||
| override visitNonlocal(node: NonlocalNode): boolean { | ||
| this._bindNonlocal(node); | ||
| return false; | ||
| } | ||
|
|
||
| override visitFunction(node: FunctionNode): boolean { | ||
| return false; | ||
| } | ||
|
|
||
| override visitClass(node: ClassNode): boolean { | ||
| return false; | ||
| } | ||
|
|
||
| override visitLambda(node: LambdaNode): boolean { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Binds assignment targets in unreachable code so they still create local | ||
| // symbols, matching CPython (an assignment after `return` makes the name | ||
| // local for the entire function). | ||
| // Header expressions for nested functions, classes, and lambdas (decorators, | ||
| // parameter defaults/annotations, type parameters, and class bases/arguments) | ||
| // are evaluated in the enclosing scope, so they are walked to catch assignment | ||
| // expressions (:=). Nested bodies are skipped because DummyScopeGenerator | ||
| // already created their scopes and they belong to nested scopes. | ||
| class UnreachableNameBinder extends ParseTreeWalker { | ||
| constructor( | ||
| private readonly _bindTarget: (target: ExpressionNode) => void, | ||
| private readonly _bindName: (name: NameNode) => void | ||
| ) { | ||
| super(); | ||
| } | ||
|
|
||
| override visitAssignment(node: AssignmentNode): boolean { | ||
| this._bindTarget(node.d.leftExpr); | ||
| return true; | ||
| } | ||
|
|
||
| override visitAugmentedAssignment(node: AugmentedAssignmentNode): boolean { | ||
| this._bindTarget(node.d.leftExpr); | ||
| return true; | ||
| } | ||
|
|
||
| override visitTypeAnnotation(node: TypeAnnotationNode): boolean { | ||
| this._bindTarget(node.d.valueExpr); | ||
| return true; | ||
| } | ||
|
|
||
| override visitAssignmentExpression(node: AssignmentExpressionNode): boolean { | ||
| this._bindName(node.d.name); | ||
| return true; | ||
| } | ||
|
|
||
| override visitFor(node: ForNode): boolean { | ||
| this._bindTarget(node.d.targetExpr); | ||
| return true; | ||
| } | ||
|
|
||
| override visitWith(node: WithNode): boolean { | ||
| node.d.withItems.forEach((item) => { | ||
| if (item.d.target) { | ||
| this._bindTarget(item.d.target); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| override visitDel(node: DelNode): boolean { | ||
| node.d.targets.forEach((target) => { | ||
| this._bindTarget(target); | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| override visitExcept(node: ExceptNode): boolean { | ||
| if (node.d.name) { | ||
| this._bindName(node.d.name); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| override visitImportAs(node: ImportAsNode): boolean { | ||
| if (node.d.alias) { | ||
| this._bindName(node.d.alias); | ||
| } else if (node.d.module.d.nameParts.length > 0) { | ||
| this._bindName(node.d.module.d.nameParts[0]); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| override visitImportFrom(node: ImportFromNode): boolean { | ||
| node.d.imports.forEach((importSymbolNode) => { | ||
| this._bindName(importSymbolNode.d.alias || importSymbolNode.d.name); | ||
| }); | ||
| return false; | ||
| } | ||
|
|
||
| override visitFunction(node: FunctionNode): boolean { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This walker skips function/class headers but descends into lambda bodies. That misses assignment expressions in decorators, defaults, and class bases that bind in the enclosing scope, while incorrectly treating assignment expressions in a lambda body as enclosing-scope bindings. Please handle those scope boundaries explicitly and add regressions for both cases. |
||
| this._bindName(node.d.name); | ||
|
|
||
| this.walkMultiple(node.d.decorators); | ||
|
|
||
| node.d.params.forEach((param) => { | ||
| if (param.d.defaultValue) { | ||
| this.walk(param.d.defaultValue); | ||
| } | ||
| if (param.d.annotation) { | ||
| this.walk(param.d.annotation); | ||
| } | ||
| if (param.d.annotationComment) { | ||
| this.walk(param.d.annotationComment); | ||
| } | ||
| }); | ||
|
|
||
| if (node.d.typeParams) { | ||
| this.walk(node.d.typeParams); | ||
| } | ||
|
|
||
| if (node.d.returnAnnotation) { | ||
| this.walk(node.d.returnAnnotation); | ||
| } | ||
|
|
||
| if (node.d.funcAnnotationComment) { | ||
| this.walk(node.d.funcAnnotationComment); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| override visitClass(node: ClassNode): boolean { | ||
| this._bindName(node.d.name); | ||
|
|
||
| this.walkMultiple(node.d.decorators); | ||
|
|
||
| if (node.d.typeParams) { | ||
| this.walk(node.d.typeParams); | ||
| } | ||
|
|
||
| this.walkMultiple(node.d.arguments); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| override visitLambda(node: LambdaNode): boolean { | ||
| node.d.params.forEach((param) => { | ||
| if (param.d.defaultValue) { | ||
| this.walk(param.d.defaultValue); | ||
| } | ||
| }); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| override visitTypeAlias(node: TypeAliasNode): boolean { | ||
| this._bindName(node.d.name); | ||
| return false; | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Returning |
||
|
|
||
| override visitPatternAs(node: PatternAsNode): boolean { | ||
| if (node.d.target) { | ||
| this._bindName(node.d.target); | ||
| } | ||
| return true; | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| override visitPatternCapture(node: PatternCaptureNode): boolean { | ||
| this._bindName(node.d.target); | ||
| return false; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new visitor duplicates the primary binder's syntax and scope classification, and the missing directive and scope-boundary cases already demonstrate drift. Reuse or centralize the scope-aware binding classification so future syntax additions do not update only one path.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[verified] |
||
|
|
||
| // Creates dummy scopes for classes or functions within a parse tree. | ||
| // This is needed in cases where the parse tree has been determined | ||
| // to be unreachable. There are code paths where the type evaluator | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| # This sample tests that assignments in unreachable code still make | ||
| # a name local, matching CPython. | ||
|
|
||
| variable = "global" | ||
|
|
||
|
|
||
| def example_with_local(): | ||
| # This should generate an error because the later assignment makes | ||
| # "variable" a local, so this read is unbound. | ||
| return variable | ||
| variable = "local" | ||
|
|
||
|
|
||
| def example_without_local(): | ||
| return variable | ||
|
|
||
|
|
||
| def outer(): | ||
| def inner(): | ||
| # This should not generate an error; the assignment below binds | ||
| # "variable" in outer even though it is unreachable. | ||
| nonlocal variable | ||
|
|
||
| return | ||
| variable = "local" | ||
|
|
||
|
|
||
| g_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_global(): | ||
| # This should not generate an error; the unreachable global directive makes | ||
| # g_var global rather than local. | ||
| return g_var | ||
| global g_var | ||
| g_var = "local" | ||
|
|
||
|
|
||
| def example_with_unreachable_nonlocal(): | ||
| n_var = "outer" | ||
|
|
||
| def inner(): | ||
| # This should not generate an error; the unreachable nonlocal directive makes | ||
| # n_var nonlocal rather than local. | ||
| return n_var | ||
| nonlocal n_var | ||
| n_var = "local" | ||
|
|
||
|
|
||
| lambda_body_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_lambda_body(): | ||
| # This should not generate an error; the assignment expression belongs to the | ||
| # lambda's scope rather than the enclosing function. | ||
| return lambda_body_var | ||
| _ = lambda: (lambda_body_var := "lambda") | ||
|
|
||
|
|
||
| lambda_default_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_lambda_default(): | ||
| # This should generate an error because the default value is evaluated in the | ||
| # enclosing scope, making lambda_default_var a local. | ||
| return lambda_default_var | ||
| _ = lambda a=(lambda_default_var := "default"): a | ||
|
|
||
|
|
||
| func_default_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_func_default(): | ||
| # This should generate an error because parameter defaults are evaluated in the | ||
| # enclosing scope, making func_default_var a local. | ||
| return func_default_var | ||
|
|
||
| def nested_func(a=(func_default_var := "default")): | ||
| pass | ||
|
|
||
|
|
||
| func_decorator_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_func_decorator(): | ||
| # This should generate an error because decorators are evaluated in the enclosing | ||
| # scope, making func_decorator_var a local. | ||
| return func_decorator_var | ||
|
|
||
| @(func_decorator_var := (lambda fn: fn)) | ||
| def nested_func(): | ||
| pass | ||
|
|
||
|
|
||
| class_base_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_class_base(): | ||
| # This should generate an error because class bases are evaluated in the enclosing | ||
| # scope, making class_base_var a local. | ||
| return class_base_var | ||
|
|
||
| class NestedClass((class_base_var := object)): | ||
| pass | ||
|
|
||
|
|
||
| class_decorator_var = "global" | ||
|
|
||
|
|
||
| def example_with_unreachable_class_decorator(): | ||
| # This should generate an error because class decorators are evaluated in the | ||
| # enclosing scope, making class_decorator_var a local. | ||
| return class_decorator_var | ||
|
|
||
| @(class_decorator_var := (lambda cls: cls)) | ||
| class NestedClass: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable
globalandnonlocaldirectives are not processed, so a later assignment can be bound to the current local scope rather than the declared scope. Handle these directives before binding targets and add focused unreachable-directive coverage.