PY-11882 Assignment to for loop inspection false positive

PY-3569 Inspection to warn if a loop variable is assigned inside the loop
2026-09-27 10:03:11 +07:00 · 2014-01-23 03:38:23 +04:00
parent 11a158b7c4
commit b55ee30dc1
12 changed files with 262 additions and 67 deletions
@@ -15,12 +15,21 @@
 */
 package com.jetbrains.python.psi;

+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

 /**
 * @author yole
 */
 public interface PySubscriptionExpression extends PyQualifiedExpression, PyReferenceOwner {
+
+  /**
+   * @return For <code>spam[x][y][n]</code> will return <code>spam</code> regardless number of its dimensions
+   */
+  @NotNull
+  PyExpression getRootOperand();
+
+  @NotNull
  PyExpression getOperand();

  @Nullable
@@ -1,7 +1,7 @@
 <html>
 <body>
 <span style="font-family: verdana,serif;">
-  This inspection checks for cases when loop variable is redeclared inside of loop:
+  Checks for cases when you rewrite loop variable with inner loop
 </span>
 <pre style="font-family: monospace">
    for i in xrange(5):
@@ -15,7 +15,7 @@
 <pre style="font-family: monospace">
    with open("file") as f:
      f.read()
-      f = open("another file")
+      with open("file") as f:
  </pre>
 </body>
 </html>
@@ -17,31 +17,23 @@ package com.jetbrains.python.inspections;

 import com.intellij.codeInspection.LocalInspectionToolSession;
 import com.intellij.codeInspection.ProblemsHolder;
+import com.intellij.openapi.util.Condition;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiElementVisitor;
+import com.intellij.psi.PsiReference;
 import com.intellij.psi.util.PsiTreeUtil;
 import com.jetbrains.python.PyBundle;
-import com.jetbrains.python.psi.PyForPart;
-import com.jetbrains.python.psi.PyTargetExpression;
-import com.jetbrains.python.psi.PyUtil;
-import com.jetbrains.python.psi.PyWithStatement;
+import com.jetbrains.python.codeInsight.controlflow.ScopeOwner;
+import com.jetbrains.python.psi.*;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

 //TODO: Try to share logic with AssignmentToForLoopParameterInspection

 /**
- * Checks for cases like
- * <pre>
- *   for i in range(1, 10):
- *    i = "new value"
- * </pre>
- * and
- * <pre>
- * with open("file") as f:
- *  f.read()
- *  f = open("another file")
- * </pre>
+ * Checks for cases when you rewrite loop variable with inner loop.
+ * It finds all <code>with</code> and <code>for</code> statements, takes variables declared by them and ensures none of parent
+ * <code>with</code> or <code>for</code> declares variable with the same name
 *
 * @author link
 */
@@ -70,24 +62,97 @@ public class PyAssignmentToLoopOrWithParameterInspection extends PyInspection {
    }

    @Override
-    public void visitPyTargetExpression(PyTargetExpression node) {
-      PsiElement variableDeclaration = node.getReference().resolve();
-      if (variableDeclaration == null) {
-        return;
-      }
-      if (!PyUtil.inSameFile(node, variableDeclaration)) {
-        return;
-      }
-      PsiElement variableFirstTimeDeclaration = variableDeclaration.getParent();
+    public void visitPyWithStatement(PyWithStatement node) {
+      checkNotReDeclaringUpperLoopOrStatement(node);
+    }

-      if (variableFirstTimeDeclaration.equals(node.getParent())) {
-        return; //We are checking first time declaration
-      }
+    @Override
+    public void visitPyForStatement(PyForStatement node) {
+      checkNotReDeclaringUpperLoopOrStatement(node);
+    }

-      //Check if variable declared in "for" or "with" statement
-      if (PsiTreeUtil.getNonStrictParentOfType(variableFirstTimeDeclaration, PyForPart.class, PyWithStatement.class) != null) {
-        registerProblem(node, MESSAGE);
+    /**
+     * Finds first parent of specific type (See {@link #isRequiredStatement(com.intellij.psi.PsiElement)})
+     * that declares one of names, declared in this statement
+     */
+    private void checkNotReDeclaringUpperLoopOrStatement(NameDefiner statement) {
+      for (PsiElement declaredVar : statement.iterateNames()) {
+        Filter filter = new Filter(handleSubscriptionsAndResolveSafely(declaredVar));
+        PsiElement firstParent = PsiTreeUtil.findFirstParent(statement, true, filter);
+        if (firstParent != null && isRequiredStatement(firstParent)) {
+          registerProblem(declaredVar, MESSAGE);
+        }
      }
    }
  }
+
+  /**
+   * Filters list of parents trying to find parent that declares var that refers to {@link #node}
+   * Returns {@link com.jetbrains.python.codeInsight.controlflow.ScopeOwner} if nothing found.
+   * Returns parent otherwise.
+   */
+  private static class Filter implements Condition<PsiElement> {
+    private final PsiElement node;
+
+    private Filter(PsiElement node) {
+      this.node = node;
+    }
+
+    @Override
+    public boolean value(PsiElement psiElement) {
+      if (psiElement instanceof ScopeOwner) {
+        return true; //Do not go any further
+      }
+      if (!(isRequiredStatement(psiElement))) {
+        return false; //Parent has wrong type, skip
+      }
+      Iterable<PyElement> varsDeclaredInStatement = ((NameDefiner)psiElement).iterateNames();
+      for (PsiElement varDeclaredInStatement : varsDeclaredInStatement) {
+        //For each variable, declared by this parent take first declaration and open subscription list if any
+        PsiReference reference = handleSubscriptionsAndResolveSafely(varDeclaredInStatement).getReference();
+        if (reference != null && reference.isReferenceTo(node)) {
+          return true; //One of variables declared by this parent refers to node
+        }
+      }
+      return false;
+    }
+
+  }
+
+  /**
+   * Opens subscription list (<code>i[n][q][f] --&gt; i</code>) and resolves ref recursively to the topmost element,
+   * but not further than file borders (to prevent Stub to AST conversion)
+   *
+   * @param element element to open and resolve
+   * @return opened and resolved element
+   */
+  private static PsiElement handleSubscriptionsAndResolveSafely(PsiElement element) {
+    assert element != null;
+    if (element instanceof PySubscriptionExpression) {
+      element = ((PySubscriptionExpression)element).getRootOperand();
+    }
+    while (true) {
+      PsiReference reference = element.getReference();
+      if (reference == null) {
+        break;
+      }
+      PsiElement resolve = reference.resolve();
+      if (resolve == null || resolve.equals(element) || !PyUtil.inSameFile(resolve, element)) {
+        break;
+      }
+      element = resolve;
+    }
+    return element;
+  }
+
+  /**
+   * Checks if element is statement this inspection should work with
+   *
+   * @param element to check
+   * @return true if inspection should work with this element
+   */
+  private static boolean isRequiredStatement(PsiElement element) {
+    assert element != null;
+    return element instanceof PyWithStatement || element instanceof PyForStatement;
+  }
 }
@@ -37,10 +37,21 @@ public class PySubscriptionExpressionImpl extends PyElementImpl implements PySub
    super(astNode);
  }

+  @NotNull
  public PyExpression getOperand() {
    return childToPsiNotNull(PythonDialectsTokenSetProvider.INSTANCE.getExpressionTokens(), 0);
  }

+  @NotNull
+  @Override
+  public PyExpression getRootOperand() {
+    PyExpression operand = getOperand();
+    while (operand instanceof PySubscriptionExpression) {
+      operand = ((PySubscriptionExpression)operand).getOperand();
+    }
+    return operand;
+  }
+
  @Nullable
  public PyExpression getIndexExpression() {
    return childToPsi(PythonDialectsTokenSetProvider.INSTANCE.getExpressionTokens(), 1);
@@ -74,7 +85,7 @@ public class PySubscriptionExpressionImpl extends PyElementImpl implements PySub
          res = ((PySubscriptableType)type).getElementType(indexExpression, context);
        }
        else if (type instanceof PyCollectionType) {
-          res = ((PyCollectionType) type).getElementType(context);
+          res = ((PyCollectionType)type).getElementType(context);
        }
      }
    }
@@ -0,0 +1,62 @@
+i = []
+for i[0] in xrange(5):
+    for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i[0]</warning> in xrange(20, 25):
+        print("Inner", i)
+        for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i</warning> in xrange(20, 25):
+            pass
+    print("Outer", i)
+
+for i in xrange(5):
+    for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i</warning> in xrange(20, 25):
+        print("Inner", i)
+    print("Outer", i)
+
+for i in xrange(5):
+    i = []
+    for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i[0]</warning> in xrange(20, 25):
+        print("Inner", i)
+    print("Outer", i)
+
+i = [0]
+for i[0] in xrange(5):
+    for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i[0]</warning> in xrange(20, 25):
+        print("Inner", i)
+    print("Outer", i)
+
+i = [[]]
+for i[0] in xrange(5):
+    for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i</warning> in xrange(20, 25):
+        print("Inner", i)
+    print("Outer", i)
+
+with open("a") as f:
+    spam(f)
+    f.eggs()
+    with open("b") as <warning descr="Assignment to 'for' loop or 'with' statement parameter">f</warning>: #
+        pass
+
+with open("a") as z, open("A") as f:
+    spam(f)
+    f.eggs()
+    for (a,b,c,d,(e,<warning descr="Assignment to 'for' loop or 'with' statement parameter">f</warning>)) in []:
+        pass
+
+
+with open("a") as f:
+    spam(f)
+    f.eggs()
+    for z in []:
+        with open("b") as q:
+            with open("a") as <warning descr="Assignment to 'for' loop or 'with' statement parameter">f</warning>: #
+                pass
+
+
+class Foo(object):
+    def __init__(self):
+        super(Foo, self).__init__()
+        self.data = "ddd"
+
+    def foo(self):
+        for self.data in [1,2,3]:
+            for <warning descr="Assignment to 'for' loop or 'with' statement parameter">self.data</warning> in [1,2,3]:
+                pass
@@ -1,3 +1,8 @@
+from spam import eggs
+
+for eggs in (1, 12):
+    eggs = 12
+
 for a in (1, 12):
    for b in (2, 24):
        for (c, d) in {"C": "D"}.items():
@@ -6,4 +11,79 @@ for a in (1, 12):
 i = 12
 print(i)
 (z, x) = (i, 12)
-print(z)
+print(z)
+
+for root in settings.STATICFILES_DIRS:
+    if isinstance(root, (list, tuple)):
+        prefix, root = root
+
+
+for field, model in self.model._meta.get_concrete_fields_with_model():
+    if model is None:
+        model = self.model
+
+with open('a', 'w') as a, open('b', 'w') as b:
+    do_something()
+
+
+for f in [1,2,3]:
+    f = f + 1
+
+for f in [1,2,3]:
+    f = spam(f)
+
+for f in [1,2,3]:
+    f = eggs(lambda x: x + f)
+
+q = []
+for q[0] in [1,2,3]:
+    q[0] = eggs(q)
+
+q = []
+for q[0] in [1,2,3]:
+    q[0] = eggs(q)
+
+for f in [1,2,3]:
+    f = eggs(lambda x: x, f)
+
+for a in [1,2]:
+    pass
+
+for a in [1,2]:
+    pass
+
+b = 12
+for b in [1,2]:
+    pass
+
+for item in range(5):
+    want_to_import = False
+    print want_to_import
+    want_to_import = 2          #No error should be here
+    if True:
+        pass
+
+for ((a, b), (c, d)) in {(1, 2): (3, 4)}.items():
+    print b
+
+x = [1]
+for x[0] in range(1,2):
+    print i
+
+for x[i] in range(1,2):
+    print i
+
+x = [[1]]
+for x[0][0] in range(1,2):
+    x[0][1] = 1
+
+class Foo(object):
+    def __init__(self):
+        super(Foo, self).__init__()
+        self.data = "ddd"
+
+    def foo(self):
+        data, self.data = self.data
+        for data in [1,2,3]:
+            for self.data in [1,2,3]:
+                pass
@@ -1,3 +0,0 @@
-for i in range(1, 2):
-    print(i)
-    <warning descr="Assignment to 'for' loop or 'with' statement parameter">i</warning> = 12
@@ -1,3 +0,0 @@
-for i in [1, 2, 3]:
-    print(i)
-    (<warning descr="Assignment to 'for' loop or 'with' statement parameter">i</warning>, f) = (1, 2)
@@ -1,3 +0,0 @@
-for (k, v) in {"K": "V"}.items():
-    print(k)
-    <warning descr="Assignment to 'for' loop or 'with' statement parameter">k</warning> = "12"
@@ -1,4 +0,0 @@
-for i in range(5):
-    for <warning descr="Assignment to 'for' loop or 'with' statement parameter">i</warning> in range(20, 25):
-        print("Inner", i)
-    print("Outer", i) 
@@ -1,3 +0,0 @@
-with open("file") as f:
-    f.read()
-    <warning descr="Assignment to 'for' loop or 'with' statement parameter">f</warning> = open("another file")
@@ -27,26 +27,10 @@ public class PyAssignmentToLoopOrWithParameterInspectionTest extends PyInspectio
    doTest();
  }

-  public void testSimpleReassignment() {
+  public void testBad() {
    doTest();
  }

-  public void testTupleAssignment() {
-    doTest();
-  }
-
-  public void testTupleDeclaration() {
-    doTest();
-  }
-
-  public void testTwoLoops() {
-    doTest();
-  }
-  public void testWithStatement() {
-    doTest();
-  }
-
-
  @NotNull
  @Override
  protected Class<? extends PyInspection> getInspectionClass() {