SONARPY-154 False Positive on rules considering nested class properties (SonarSource#37)

ivandalbosco · vilchik-elena · commit b1177cf49958 · 2016-07-06T13:28:53.000+02:00
diff --git a/its/ruling/src/test/resources/expected/python-S116.json b/its/ruling/src/test/resources/expected/python-S116.json
@@ -120,7 +120,6 @@
 ],
 'project:buildbot-0.8.6p1/buildbot/test/unit/test_process_users_manual.py':[
 36,
-36,
 ],
 'project:buildbot-0.8.6p1/buildbot/test/unit/test_schedulers_basic.py':[
 46,
@@ -508,8 +507,8 @@
 479,
 480,
 482,
-933,
-935,
+907,
+909,
 1114,
 ],
 'project:twisted-12.1.0/twisted/internet/defer.py':[
@@ -1395,7 +1394,6 @@
 'project:twisted-12.1.0/twisted/web/test/test_webclient.py':[
 1084,
 1349,
-1349,
 ],
 'project:twisted-12.1.0/twisted/web/test/test_wsgi.py':[
 112,
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
@@ -27,6 +27,7 @@
 import org.sonar.python.api.PythonGrammar;
 
 public class NewSymbolsAnalyzer {
+
   private List<Token> symbols;
 
   public List<Token> getClassFields(AstNode classDef) {
@@ -36,11 +37,18 @@ public List<Token> getClassFields(AstNode classDef) {
     List<AstNode> methods = classDef.getFirstChild(PythonGrammar.SUITE).getDescendants(PythonGrammar.FUNCDEF);
 
     for (AstNode method : methods) {
-      addFieldsInMethod(method);
+      // ignore any nested class
+      if (belongToClass(classDef, method)) {
+        addFieldsInMethod(method);
+      }
     }
     return symbols;
   }
 
+  private static boolean belongToClass(AstNode classDef, AstNode methodOrStmt) {
+    return classDef.equals(methodOrStmt.getFirstAncestor(PythonGrammar.CLASSDEF, PythonGrammar.FUNCDEF));
+  }
+
   private void addFieldsInMethod(AstNode method) {
     AstNode suite = method.getFirstChild(PythonGrammar.SUITE);
     List<AstNode> expressions = suite.getDescendants(PythonGrammar.EXPRESSION_STMT);
@@ -100,12 +108,10 @@ private List<Token> findFieldsInClassBody(AstNode classDef) {
     List<AstNode> statements = classDef.getFirstChild(PythonGrammar.SUITE).getChildren(PythonGrammar.STATEMENT);
     List<AstNode> expressions = new LinkedList<>();
     for (AstNode statement : statements) {
-      if (!statement.hasDescendant(PythonGrammar.FUNCDEF)) {
-        expressions.addAll(statement.getDescendants(PythonGrammar.EXPRESSION_STMT));
-      }
+      expressions.addAll(statement.getDescendants(PythonGrammar.EXPRESSION_STMT));
     }
     for (AstNode expression : expressions) {
-      if (CheckUtils.isAssignmentExpression(expression)) {
+      if (CheckUtils.isAssignmentExpression(expression) && belongToClass(classDef, expression)) {
         addSimpleIdentifiersFromLongAssignmentExpression(expression);
       }
     }
@@ -117,4 +123,5 @@ public List<Token> getVariablesFromLongAssignmentExpression(List<Token> varNames
     addSimpleIdentifiersFromLongAssignmentExpression(expression);
     return symbols;
   }
+
 }
diff --git a/python-checks/src/test/resources/checks/duplicatedMethodFieldNames.py b/python-checks/src/test/resources/checks/duplicatedMethodFieldNames.py
@@ -12,7 +12,34 @@ def GO_DOWN(self):  # Noncompliant [[secondary=-2]]
     def NAME(self):
         pass
 
-    NAME = foo(NAME)
+    NAME = func(NAME)
 
     def NAME(self):
         pass
+
+    def hello(self):
+        pass
+
+    foo = 1
+    
+    fOo = 1            # Noncompliant {{Rename field "fOo" to prevent any misunderstanding/clash with field "foo" defined on line 23}}
+#   ^^^
+
+    baz = 100
+
+    bat = 1000
+
+    class B:
+
+        Foo = 1
+
+        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.baz = 100
+            self.baT = 1000
+
+        def HEllo(self):
+            pass
diff --git a/python-checks/src/test/resources/checks/fieldDuplicatesClassName.py b/python-checks/src/test/resources/checks/fieldDuplicatesClassName.py
@@ -17,3 +17,16 @@ def fun(self):
 
 class MyClass1(MyClass):
   myClass1 = 1
+
+class MyClass:
+  class B:
+     myClass = 1
+     def foo(self):
+         self.myClass = 1
+
+class MyClass:
+  if True:
+    MYCLASS = 1 # Noncompliant
+    def foo(self):
+      self.myCLASS = 1 # Noncompliant
+      myClass = 1      # compliant, local var
diff --git a/python-checks/src/test/resources/checks/fieldName.py b/python-checks/src/test/resources/checks/fieldName.py
@@ -37,7 +37,16 @@ def fun(self):
         self.Field4 = self.Field5 = 6
 
 class MyClass3(object):
-  myField = 4 # Noncompliant
+    myField = 4 # Noncompliant
 
 class MyClass4(MyClass3):
-  myField = 4
+    myField = 4
+
+class MyClass5:
+    class MyClass6:
+        myField1 = 1            # Noncompliant
+
+class MyClass7:
+    class MyClass8:
+        def foo(self):
+            self.myField2 = 2   # Noncompliant
