Fixed bug in binder logic that resulted in infinite recursion in the …

…type evaluator. It occurred in the case where the target of an assignment was a tuple and the elements in the tuple depended on assignment ordering. The binder was not assigning the correct control flow nodes to the individual elements of the target tuple.
heejaechang · Feb 14, 2020 · ec63949 · ec63949
1 parent 04a01c7
commit ec63949
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 21 deletions.
diff --git a/server/src/analyzer/binder.ts b/server/src/analyzer/binder.ts
@@ -264,7 +264,7 @@ export class Binder extends ParseTreeWalker {
         // to do more passes to resolve classes.
         this._addSymbolToCurrentScope(node.name.value, true);
 
-        this._createAssignmentTargetFlowNodes(node.name);
+        this._createAssignmentTargetFlowNodes(node.name, false, false);
 
         return false;
     }
@@ -392,7 +392,7 @@ export class Binder extends ParseTreeWalker {
             AnalyzerNodeInfo.setCodeFlowExpressions(node, this._currentExecutionScopeReferenceMap);
         });
 
-        this._createAssignmentTargetFlowNodes(node.name);
+        this._createAssignmentTargetFlowNodes(node.name, false, false);
 
         // We'll walk the child nodes in a deferred manner, so don't walk them now.
         return false;
@@ -468,8 +468,7 @@ export class Binder extends ParseTreeWalker {
         this.walk(node.rightExpression);
         this._addInferredTypeAssignmentForVariable(node.leftExpression, node.rightExpression);
 
-        this._createAssignmentTargetFlowNodes(node.leftExpression);
-        this.walk(node.leftExpression);
+        this._createAssignmentTargetFlowNodes(node.leftExpression, true, false);
 
         return false;
     }
@@ -510,8 +509,7 @@ export class Binder extends ParseTreeWalker {
             this._bindNameToScope(containerScope, node.name.value);
         }
 
-        this._createAssignmentTargetFlowNodes(node.name);
-        this.walk(node.name);
+        this._createAssignmentTargetFlowNodes(node.name, true, false);
 
         return false;
     }
@@ -523,7 +521,7 @@ export class Binder extends ParseTreeWalker {
         this._addInferredTypeAssignmentForVariable(node.destExpression, node.rightExpression);
 
         this._bindPossibleTupleNamedTarget(node.destExpression);
-        this._createAssignmentTargetFlowNodes(node.destExpression);
+        this._createAssignmentTargetFlowNodes(node.destExpression, false, false);
 
         return false;
     }
@@ -532,7 +530,7 @@ export class Binder extends ParseTreeWalker {
         node.expressions.forEach(expr => {
             this._bindPossibleTupleNamedTarget(expr);
             this.walk(expr);
-            this._createAssignmentTargetFlowNodes(expr, true);
+            this._createAssignmentTargetFlowNodes(expr, false, true);
         });
 
         return false;
@@ -561,8 +559,7 @@ export class Binder extends ParseTreeWalker {
         this._addAntecedent(preForLabel, this._currentFlowNode);
         this._currentFlowNode = preForLabel;
         this._addAntecedent(preElseLabel, this._currentFlowNode);
-        this._createAssignmentTargetFlowNodes(node.targetExpression);
-        this.walk(node.targetExpression);
+        this._createAssignmentTargetFlowNodes(node.targetExpression, true, false);
 
         this._bindLoopStatement(preForLabel, postForLabel, () => {
             this.walk(node.forSuite);
@@ -733,8 +730,8 @@ export class Binder extends ParseTreeWalker {
 
         if (node.name) {
             const symbol = this._bindNameToScope(this._currentScope, node.name.value);
-            this._createAssignmentTargetFlowNodes(node.name);
-            this.walk(node.name);
+            this._createAssignmentTargetFlowNodes(node.name, true, false);
+
             if (symbol) {
                 const declaration: VariableDeclaration = {
                     type: DeclarationType.Variable,
@@ -1245,8 +1242,7 @@ export class Binder extends ParseTreeWalker {
             if (item.target) {
                 this._bindPossibleTupleNamedTarget(item.target);
                 this._addInferredTypeAssignmentForVariable(item.target, item);
-                this._createAssignmentTargetFlowNodes(item.target);
-                this.walk(item.target);
+                this._createAssignmentTargetFlowNodes(item.target, true, false);
             }
         });
 
@@ -1381,8 +1377,7 @@ export class Binder extends ParseTreeWalker {
 
                     this.walk(compr.iterableExpression);
 
-                    this._createAssignmentTargetFlowNodes(compr.targetExpression);
-                    this.walk(compr.targetExpression);
+                    this._createAssignmentTargetFlowNodes(compr.targetExpression, true, false);
                 } else {
                     const trueLabel = this._createBranchLabel();
                     this._bindConditional(compr.testExpression, trueLabel, falseLabel);
@@ -1672,37 +1667,52 @@ export class Binder extends ParseTreeWalker {
         return false;
     }
 
-    private _createAssignmentTargetFlowNodes(target: ExpressionNode, unbound = false) {
+    private _createAssignmentTargetFlowNodes(target: ExpressionNode, walkTargets: boolean, unbound: boolean) {
         switch (target.nodeType) {
             case ParseNodeType.Name:
             case ParseNodeType.MemberAccess: {
                 this._createFlowAssignment(target, unbound);
+                if (walkTargets) {
+                    this.walk(target);
+                }
                 break;
             }
 
             case ParseNodeType.Tuple: {
                 target.expressions.forEach(expr => {
-                    this._createAssignmentTargetFlowNodes(expr, unbound);
+                    this._createAssignmentTargetFlowNodes(expr, walkTargets, unbound);
                 });
                 break;
             }
 
             case ParseNodeType.TypeAnnotation: {
-                this._createAssignmentTargetFlowNodes(target.valueExpression, unbound);
+                this._createAssignmentTargetFlowNodes(target.valueExpression, false, unbound);
+                if (walkTargets) {
+                    this.walk(target);
+                }
                 break;
             }
 
             case ParseNodeType.Unpack: {
-                this._createAssignmentTargetFlowNodes(target.expression, unbound);
+                this._createAssignmentTargetFlowNodes(target.expression, false, unbound);
+                if (walkTargets) {
+                    this.walk(target);
+                }
                 break;
             }
 
             case ParseNodeType.List: {
                 target.entries.forEach(entry => {
-                    this._createAssignmentTargetFlowNodes(entry, unbound);
+                    this._createAssignmentTargetFlowNodes(entry, walkTargets, unbound);
                 });
                 break;
             }
+
+            default: {
+                if (walkTargets) {
+                    this.walk(target);
+                }
+            }
         }
     }
 

diff --git a/server/src/tests/checker.test.ts b/server/src/tests/checker.test.ts
@@ -659,6 +659,12 @@ test('Assignment4', () => {
     validateResults(analysisResults, 0);
 });
 
+test('Assignment5', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['assignment5.py']);
+
+    validateResults(analysisResults, 0);
+});
+
 test('AugmentedAssignment1', () => {
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['augmentedAssignment1.py']);
 

diff --git a/server/src/tests/samples/assignment5.py b/server/src/tests/samples/assignment5.py
@@ -0,0 +1,15 @@
+# This sample tests the handling of tuple assignments
+# where the order of assignment within the tuple is important.
+
+from typing import Optional
+
+class Node:
+    key: str
+    next: Optional['Node'] = None
+
+node = Node()
+
+# This should analyze fine because node.next should be assigned
+# None before node is assigned None.
+node.next, node = None, None
+