SONARPY-247 Support variable annotations (SonarSource#128)

alban-auzeill · pynicolas · commit 77ec9198dc91 · 2018-01-16T14:01:10.000+01:00
* SONARPY-247 Parse variable annotations

* SONARPY-247 Support variable annotations in symbol table
diff --git a/python-checks/src/main/java/org/sonar/python/checks/CheckUtils.java b/python-checks/src/main/java/org/sonar/python/checks/CheckUtils.java
@@ -86,6 +86,12 @@ public static boolean classHasInheritance(AstNode classDef) {
   }
 
   public static boolean isAssignmentExpression(AstNode expression) {
+    if (expression.is(PythonGrammar.EXPRESSION_STMT)) {
+      AstNode assignNode = expression.getFirstChild(PythonGrammar.ANNASSIGN);
+      if (assignNode != null && assignNode.getFirstChild(PythonPunctuator.ASSIGN) != null) {
+        return true;
+      }
+    }
     int numberOfChildren = expression.getNumberOfChildren();
     int numberOfAssign = expression.getChildren(PythonPunctuator.ASSIGN).size();
     if (numberOfChildren == 3 && numberOfAssign == 1) {
diff --git a/python-checks/src/main/java/org/sonar/python/checks/NewSymbolsAnalyzer.java b/python-checks/src/main/java/org/sonar/python/checks/NewSymbolsAnalyzer.java
@@ -25,6 +25,7 @@
 import java.util.LinkedList;
 import java.util.List;
 import org.sonar.python.api.PythonGrammar;
+import org.sonar.python.api.PythonPunctuator;
 
 public class NewSymbolsAnalyzer {
 
@@ -60,11 +61,14 @@ private void addFieldsInMethod(AstNode method) {
   }
 
   private static List<AstNode> getTestsFromLongAssignmentExpression(AstNode expression) {
-    List<AstNode> assignedExpressions = expression.getChildren(PythonGrammar.TESTLIST_STAR_EXPR);
-    assignedExpressions.remove(assignedExpressions.size() - 1);
     List<AstNode> tests = new LinkedList<>();
-    for (AstNode assignedExpression : assignedExpressions) {
-      tests.addAll(assignedExpression.getDescendants(PythonGrammar.TEST));
+    AstNode lastAssign = expression.getLastChild(PythonPunctuator.ASSIGN);
+    for (AstNode node : expression.getChildren()) {
+      if (node.is(PythonGrammar.TESTLIST_STAR_EXPR)) {
+        tests.addAll(node.getDescendants(PythonGrammar.TEST));
+      } else if (node == lastAssign) {
+        break;
+      }
     }
     return tests;
   }
diff --git a/python-checks/src/main/java/org/sonar/python/checks/SelfAssignmentCheck.java b/python-checks/src/main/java/org/sonar/python/checks/SelfAssignmentCheck.java
@@ -66,12 +66,22 @@ public void visitNode(AstNode node) {
       addImportedName(node, importedName);
 
     } else {
-      for (AstNode assignOperator : node.getChildren(PythonPunctuator.ASSIGN)) {
-        AstNode assigned = assignOperator.getPreviousSibling();
-        if (CheckUtils.equalNodes(assigned, assignOperator.getNextSibling()) && !isException(node, assigned)) {
-          addIssue(assignOperator, MESSAGE);
+      checkExpressionStatement(node);
+    }
+  }
+
+  private void checkExpressionStatement(AstNode node) {
+    for (AstNode assignOperator : node.getChildren(PythonPunctuator.ASSIGN, PythonGrammar.ANNASSIGN)) {
+      AstNode assigned = assignOperator.getPreviousSibling();
+      if (assignOperator.is(PythonGrammar.ANNASSIGN)) {
+        assignOperator = assignOperator.getFirstChild(PythonPunctuator.ASSIGN);
+        if (assigned.is(PythonGrammar.TESTLIST_STAR_EXPR) && assigned.getNumberOfChildren() == 1) {
+          assigned =  assigned.getFirstChild();
         }
       }
+      if (assignOperator != null && CheckUtils.equalNodes(assigned, assignOperator.getNextSibling()) && !isException(node, assigned)) {
+        addIssue(assignOperator, MESSAGE);
+      }
     }
   }
 
diff --git a/python-checks/src/test/java/org/sonar/python/checks/CheckUtilsTest.java b/python-checks/src/test/java/org/sonar/python/checks/CheckUtilsTest.java
@@ -146,4 +146,16 @@ public void invalid_string_literal_content() throws Exception {
     CheckUtils.stringLiteralContent(PARSER.parse("2").getTokens().get(0).getOriginalValue());
   }
 
+  @Test
+  public void is_assignment_expression() throws Exception {
+    Function<String, Boolean> firstStatement = (source) ->
+      CheckUtils.isAssignmentExpression(PARSER.parse(source).getFirstDescendant(PythonGrammar.SIMPLE_STMT).getFirstChild());
+
+    assertThat(firstStatement.apply("a()")).isFalse();
+    assertThat(firstStatement.apply("a = 2")).isTrue();
+    assertThat(firstStatement.apply("a: int")).isFalse();
+    assertThat(firstStatement.apply("a: int = 2")).isTrue();
+    assertThat(firstStatement.apply("a.b = (1, 2)")).isTrue();
+    assertThat(firstStatement.apply("a.b: int = (1, 2)")).isTrue();
+  }
 }
diff --git a/python-checks/src/test/resources/checks/duplicatedMethodFieldNames.py b/python-checks/src/test/resources/checks/duplicatedMethodFieldNames.py
@@ -25,6 +25,9 @@ def hello(self):
     fOo = 1            # Noncompliant {{Rename field "fOo" to prevent any misunderstanding/clash with field "foo" defined on line 23}}
 #   ^^^
 
+    foO: int = 1       # Noncompliant {{Rename field "foO" to prevent any misunderstanding/clash with field "fOo" defined on line 25}}
+#   ^^^
+
     baz = 100
 
     bat = 1000
@@ -36,7 +39,9 @@ class B:
         bar = 10
 
         def __init__(self):
-            self.baR = 10      # Noncompliant {{Rename field "baR" to prevent any misunderstanding/clash with field "bar" defined on line 36}}
+            self.baR = 10      # Noncompliant {{Rename field "baR" to prevent any misunderstanding/clash with field "bar" defined on line 39}}
+#                ^^^
+            self.FoO: int = 10 # Noncompliant
 #                ^^^
             self.baz = 100
             self.baT = 1000
diff --git a/python-checks/src/test/resources/checks/fieldDuplicatesClassName.py b/python-checks/src/test/resources/checks/fieldDuplicatesClassName.py
@@ -2,7 +2,7 @@ class MyClass:
   myClass = 3      # Noncompliant {{Rename field "myClass"}}
 # ^^^^^^^
   def __int__(self, myclass):
-    self.myclass = myclass # Noncompliant [[secondary=-4]]
+    self.myclass: type = myclass # Noncompliant [[secondary=-4]]
 
   def fun(self):
     self.myClass += 1
@@ -30,3 +30,6 @@ class MyClass:
     def foo(self):
       self.myCLASS = 1 # Noncompliant
       myClass = 1      # compliant, local var
+
+class MyClass2:
+  myClass2: int = 3      # Noncompliant {{Rename field "myClass2"}}
diff --git a/python-checks/src/test/resources/checks/fieldName.py b/python-checks/src/test/resources/checks/fieldName.py
@@ -2,6 +2,8 @@
 class MyClass:
     myField = 4 # Noncompliant {{Rename this field "myField" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
 #   ^^^^^^^
+    myField2: int = 4 # Noncompliant {{Rename this field "myField2" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
+#   ^^^^^^^^
     my_field = 4
 
     def __init__(self):
diff --git a/python-checks/src/test/resources/checks/localVariableAndParameterNameIncompatibility.py b/python-checks/src/test/resources/checks/localVariableAndParameterNameIncompatibility.py
@@ -2,6 +2,8 @@
 def fun(input_par1, inputPar2, inputPar3 = 3): # Noncompliant {{Rename this parameter "inputPar3" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
     someName = 1 # Noncompliant {{Rename this local variable "someName" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
 #   ^^^^^^^^
+    CamelName: int = 1 # Noncompliant {{Rename this local variable "CamelName" to match the regular expression ^[_a-z][a-z0-9_]+$.}}
+#   ^^^^^^^^^
     another_name = someName
     someName = 3
     inputPar2 = 2
diff --git a/python-checks/src/test/resources/checks/selfAssignment.py b/python-checks/src/test/resources/checks/selfAssignment.py
@@ -4,6 +4,9 @@
 x = 1
 x = y
 x = x # Noncompliant
+# ^
+x: int = x # Noncompliant
+#      ^
 x += x
 
 def f():
@@ -23,6 +26,7 @@ def f():
 
 a.x = a.x # Noncompliant
 a[x] = a[x] # Noncompliant
+a[x]: str = a[x] # Noncompliant
 a[sideEffect()] = a[sideEffect()]
 
 import1 = import1
diff --git a/python-squid/src/main/java/org/sonar/python/api/PythonGrammar.java b/python-squid/src/main/java/org/sonar/python/api/PythonGrammar.java
@@ -100,6 +100,7 @@ public enum PythonGrammar implements GrammarRuleKey {
   EXEC_STMT,
   ASSERT_STMT,
 
+  ANNASSIGN,
   AUGASSIGN,
 
   PASS_STMT,
@@ -172,11 +173,17 @@ public static LexerfulGrammarBuilder create() {
 
   public static void grammar(LexerfulGrammarBuilder b) {
 
+    // https://github.com/python/cpython/blob/v3.6.4/Grammar/Grammar#L41
     b.rule(EXPRESSION_STMT).is(
       TESTLIST_STAR_EXPR,
       b.firstOf(
+        ANNASSIGN,
         b.sequence(AUGASSIGN, b.firstOf(YIELD_EXPR, TESTLIST)),
         b.zeroOrMore("=", b.firstOf(YIELD_EXPR, TESTLIST_STAR_EXPR))));
+
+    // https://github.com/python/cpython/blob/v3.6.4/Grammar/Grammar#L43
+    b.rule(ANNASSIGN).is(":", TEST, b.optional( "=", TEST));
+
     b.rule(TESTLIST_STAR_EXPR).is(b.firstOf(TEST, STAR_EXPR), b.zeroOrMore(",", b.firstOf(TEST, STAR_EXPR)), b.optional(","));
     b.rule(AUGASSIGN).is(b.firstOf("+=", "-=", "*=", "/=", "//=", "%=", "**=", ">>=", "<<=", "&=", "^=", "|=", "@="));
 
diff --git a/python-squid/src/main/java/org/sonar/python/semantic/SymbolTableBuilderVisitor.java b/python-squid/src/main/java/org/sonar/python/semantic/SymbolTableBuilderVisitor.java
@@ -124,21 +124,30 @@ public void visitNode(AstNode node) {
         createScope(node, currentScope);
 
       } else if (node.is(PythonGrammar.EXPRESSION_STMT)) {
-        for (AstNode assignOperator : node.getChildren(PythonPunctuator.ASSIGN, PythonGrammar.AUGASSIGN)) {
+        visitAssignment(node);
+
+      } else if (node.is(PythonGrammar.GLOBAL_STMT)) {
+        node.getChildren(PythonGrammar.NAME).forEach(name -> currentScope().addGlobalName(name.getTokenValue()));
+
+      } else if (node.is(PythonGrammar.NONLOCAL_STMT)) {
+        node.getChildren(PythonGrammar.NAME).forEach(name -> currentScope().addNonlocalName(name.getTokenValue()));
+      }
+    }
+
+    private void visitAssignment(AstNode node) {
+      for (AstNode assignOperator : node.getChildren(PythonPunctuator.ASSIGN, PythonGrammar.AUGASSIGN, PythonGrammar.ANNASSIGN)) {
+        AstNode target = assignOperator.getPreviousSibling();
+        if (assignOperator.is(PythonGrammar.ANNASSIGN)) {
+          assignOperator = assignOperator.getFirstChild(PythonPunctuator.ASSIGN);
+        }
+        if (assignOperator != null) {
           if (currentScopeRootTree().is(PythonGrammar.CLASSDEF)) {
             new ClassVariableAssignmentVisitor(currentScopeRootTree()).scanNode(assignOperator.getNextSibling());
           }
-          AstNode target = assignOperator.getPreviousSibling();
           if (target.getTokens().size() == 1) {
             addWriteUsage(target.getFirstDescendant(PythonGrammar.NAME));
           }
         }
-
-      } else if (node.is(PythonGrammar.GLOBAL_STMT)) {
-        node.getChildren(PythonGrammar.NAME).forEach(name -> currentScope().addGlobalName(name.getTokenValue()));
-
-      } else if (node.is(PythonGrammar.NONLOCAL_STMT)) {
-        node.getChildren(PythonGrammar.NAME).forEach(name -> currentScope().addNonlocalName(name.getTokenValue()));
       }
     }
 
diff --git a/python-squid/src/test/java/org/sonar/python/parser/compound_statements/ClassDefTest.java b/python-squid/src/test/java/org/sonar/python/parser/compound_statements/ClassDefTest.java
@@ -39,6 +39,8 @@ public void realLife() {
     assertThat(p).matches(PythonTestUtils.appendNewLine("class Foo: pass"));
     assertThat(p).matches(PythonTestUtils.appendNewLine("class Foo(argument): pass"));
     assertThat(p).matches(PythonTestUtils.appendNewLine("class Foo(argument=value): pass"));
+    assertThat(p).matches(PythonTestUtils.appendNewLine("class Foo: x: int"));
+    assertThat(p).matches(PythonTestUtils.appendNewLine("class Foo: x: int = 3"));
   }
 
 }
diff --git a/python-squid/src/test/java/org/sonar/python/parser/simple_statements/ExpressionStatementTest.java b/python-squid/src/test/java/org/sonar/python/parser/simple_statements/ExpressionStatementTest.java
@@ -38,6 +38,13 @@ public void realLife() {
     assertThat(p).matches("i = 10");
     assertThat(p).matches("list[1] = 10");
     assertThat(p).matches("self.balance = initial_balance");
+    assertThat(p).matches("var1: int = 5");
+    assertThat(p).matches("var2: [int, str]");
+    assertThat(p).matches("st: str = 'Hello'");
+    assertThat(p).matches("a.b: int = (1, 2)");
+    assertThat(p).matches("x: int");
+    assertThat(p).matches("self.x: int = x");
+    assertThat(p).matches("lst: List[int] = []");
   }
 
 }
diff --git a/python-squid/src/test/java/org/sonar/python/semantic/SymbolTableBuilderVisitorTest.java b/python-squid/src/test/java/org/sonar/python/semantic/SymbolTableBuilderVisitorTest.java
@@ -19,11 +19,13 @@
  */
 package org.sonar.python.semantic;
 
+import com.google.common.base.Functions;
 import com.sonar.sslr.api.AstNode;
 import java.io.File;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import org.junit.Before;
 import org.junit.Test;
 import org.sonar.python.PythonVisitor;
@@ -55,26 +57,31 @@ public void non_scope_tree() throws Exception {
 
   @Test
   public void module_variable() {
-    assertThat(symbolTable.symbols(rootTree)).extracting(Symbol::name).containsOnly("a", "b");
+    assertThat(symbolTable.symbols(rootTree)).extracting(Symbol::name).containsOnly("a", "b", "t1");
     assertThat(symbolTable.symbols(rootTree)).extracting(Symbol::scopeTree).containsOnly(rootTree);
   }
 
   @Test
   public void local_variable() {
     AstNode functionTree = functionTreesByName.get("function_with_local");
     Set<Symbol> symbols = symbolsInFunction("function_with_local");
-    assertThat(symbols).extracting(Symbol::name).containsOnly("a");
-    Symbol a = symbols.iterator().next();
+    Map<String, Symbol> symbolByName = symbols.stream().collect(Collectors.toMap(Symbol::name, Functions.identity()));
+    assertThat(symbolByName.keySet()).containsOnly("a", "t2");
+    Symbol a = symbolByName.get("a");
     assertThat(a.scopeTree()).isEqualTo(functionTree);
     assertThat(a.writeUsages()).extracting(AstNode::getTokenLine).containsOnly(functionTree.getTokenLine() + 1);
     assertThat(a.readUsages()).extracting(AstNode::getTokenLine).containsOnly(
       functionTree.getTokenLine() + 2,
       functionTree.getTokenLine() + 3);
+    Symbol t2 = symbolByName.get("t2");
+    assertThat(t2.scopeTree()).isEqualTo(functionTree);
+    assertThat(t2.writeUsages()).extracting(AstNode::getTokenLine).containsOnly(functionTree.getTokenLine() + 4);
+    assertThat(t2.readUsages()).isEmpty();
   }
 
   @Test
   public void global_variable() {
-    assertThat(symbolsInFunction("function_with_global")).extracting(Symbol::name).containsOnly("c");
+    assertThat(symbolsInFunction("function_with_global")).extracting(Symbol::name).containsOnly("c", "t3");
   }
 
   @Test
@@ -125,9 +132,11 @@ public void dotted_name() {
   @Test
   public void class_variable() {
     AstNode classC = rootTree.getFirstDescendant(PythonGrammar.CLASSDEF);
-    assertThat(symbolTable.symbols(classC)).hasSize(2);
+    assertThat(symbolTable.symbols(classC)).hasSize(3);
     Symbol classVariableA = lookup(classC, "a");
     assertThat(classVariableA.readUsages()).extracting(AstNode::getTokenLine).containsOnly(classC.getTokenLine() + 2);
+    Symbol classVariableB = lookup(classC, "b");
+    assertThat(classVariableB.readUsages()).extracting(AstNode::getTokenLine).containsOnly(classC.getTokenLine() + 3);
   }
 
   @Test
diff --git a/python-squid/src/test/resources/parser/examples/ann_module.py b/python-squid/src/test/resources/parser/examples/ann_module.py
@@ -0,0 +1,53 @@
+
+
+"""
+The module for testing variable annotations.
+Empty lines above are for good reason (testing for correct line numbers)
+"""
+
+from typing import Optional
+
+__annotations__[1] = 2
+
+class C:
+
+    x = 5; y: Optional['C'] = None
+
+from typing import Tuple
+x: int = 5; y: str = x; f: Tuple[int, int]
+
+class M(type):
+
+    __annotations__['123'] = 123
+    o: type = object
+
+(pars): bool = True
+
+class D(C):
+    j: str = 'hi'; k: str= 'bye'
+
+from types import new_class
+h_class = new_class('H', (C,))
+j_class = new_class('J')
+
+class F():
+    z: int = 5
+    def __init__(self, x):
+        pass
+
+class Y(F):
+    def __init__(self):
+        super(F, self).__init__(123)
+
+class Meta(type):
+    def __new__(meta, name, bases, namespace):
+        return super().__new__(meta, name, bases, namespace)
+
+class S(metaclass = Meta):
+    x: str = 'something'
+    y: str = 'something else'
+
+def foo(x: int = 10):
+    def bar(y: List[str]):
+        x: str = 'yes'
+    bar()
diff --git a/python-squid/src/test/resources/parser/examples/ann_module2.py b/python-squid/src/test/resources/parser/examples/ann_module2.py
@@ -0,0 +1,36 @@
+"""
+Some correct syntax for variable annotation here.
+More examples are in test_grammar and test_parser.
+"""
+
+from typing import no_type_check, ClassVar
+
+i: int = 1
+j: int
+x: float = i/10
+
+def f():
+    class C: ...
+    return C()
+
+f().new_attr: object = object()
+
+class C:
+    def __init__(self, x: int) -> None:
+        self.x = x
+
+c = C(5)
+c.new_attr: int = 10
+
+__annotations__ = {}
+
+
+@no_type_check
+class NTC:
+    def meth(self, param: complex) -> None:
+        ...
+
+class CV:
+    var: ClassVar['CV']
+
+CV.var = CV()
diff --git a/python-squid/src/test/resources/parser/examples/ann_module3.py b/python-squid/src/test/resources/parser/examples/ann_module3.py
diff --git a/python-squid/src/test/resources/semantic/symbols.py b/python-squid/src/test/resources/semantic/symbols.py
