SONARPY-672e Improve issue reporting for S5797 (#741)

* Add ClassSymbol#definitionLocation API

* SONARPY-672 Improve issue reporting for S5797

Author: guillaume-dequenne

python-checks/src/main/java/org/sonar/python/checks/ConstantConditionCheck.java

@@ -23,8 +23,10 @@
 import java.util.List;
 import java.util.Set;
 import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.LocationInFile;
 import org.sonar.plugins.python.api.PythonVisitorCheck;
 import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
+import org.sonar.plugins.python.api.symbols.ClassSymbol;
 import org.sonar.plugins.python.api.symbols.FunctionSymbol;
 import org.sonar.plugins.python.api.symbols.Symbol;
 import org.sonar.plugins.python.api.tree.BinaryExpression;
@@ -182,7 +184,7 @@ private void checkExpression(Expression expression) {
     if (expression.is(NAME) || expression.is(QUALIFIED_EXPR)) {
       Symbol symbol = ((HasSymbol) expression).symbol();
       if (symbol != null && isClassOrFunction(symbol)) {
-        addIssue(expression, MESSAGE);
+        raiseIssueOnClassOrFunction(expression, symbol);
         return;
       }
     }
@@ -197,6 +199,15 @@ private void checkExpression(Expression expression) {
     }
   }
 
+  private void raiseIssueOnClassOrFunction(Expression expression, Symbol symbol) {
+    PreciseIssue issue = addIssue(expression, MESSAGE);
+    LocationInFile locationInFile = locationForClassOrFunction(symbol);
+    if (locationInFile != null) {
+      String type = symbol.is(Symbol.Kind.CLASS) ? "Class" : "Function";
+      issue.secondary(locationInFile, String.format("%s definition.", type));
+    }
+  }
+
   private static boolean isClassOrFunction(Symbol symbol) {
     if (symbol.is(Symbol.Kind.CLASS)) {
       return true;
@@ -205,6 +216,10 @@ private static boolean isClassOrFunction(Symbol symbol) {
       // Avoid potential FPs with properties: only report on limited selection of "safe" decorators
       return ACCEPTED_DECORATORS.containsAll(((FunctionSymbol) symbol).decorators());
     }
-    return symbol.is(Symbol.Kind.AMBIGUOUS) && ((AmbiguousSymbol) symbol).alternatives().stream().allMatch(ConstantConditionCheck::isClassOrFunction);
+    return false;
+  }
+
+  private static LocationInFile locationForClassOrFunction(Symbol symbol) {
+    return symbol.is(Symbol.Kind.CLASS) ? ((ClassSymbol) symbol).definitionLocation() : ((FunctionSymbol) symbol).definitionLocation();
   }
 }

python-checks/src/test/resources/checks/constantCondition.py

@@ -152,11 +152,14 @@ def myfunction():
 
   if myfunction:  # Noncompliant
       pass
-  elif round:  # Noncompliant
+  elif round:  # FN (ambiguous symbol)
+      pass
+  elif float.__add__: # Noncompliant
       pass
 
 def class_and_methods():
   class MyClass:
+  #     ^^^^^^^> {{Class definition.}}
     def mymethod(self): ...
     @staticmethod
     def mystaticmethod(): ...
@@ -169,7 +172,8 @@ def othercachedproperty(self): ...
 
   myinstance = MyClass()
 
-  if MyClass: ... # Noncompliant
+  if MyClass: ... # Noncompliant {{Replace this expression; used as a condition it will always be constant.}}
+  #  ^^^^^^^
   elif MyClass.mymethod: ...   # Noncompliant
   elif myinstance.mymethod: ...   # Noncompliant
   elif myinstance.mystaticmethod: ...   # Noncompliant
@@ -182,7 +186,7 @@ def ambiguous_symbols():
     class ambiguous_class_or_function: ...
   else:
     def ambiguous_class_or_function(): ...
-  if ambiguous_class_or_function: ... # Noncompliant
+  if ambiguous_class_or_function: ... # OK
 
   if cond():
       class true_ambiguous: ...

python-frontend/src/main/java/org/sonar/plugins/python/api/symbols/ClassSymbol.java

@@ -23,6 +23,8 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import javax.annotation.CheckForNull;
+import org.sonar.plugins.python.api.LocationInFile;
 
 public interface ClassSymbol extends Symbol {
   List<Symbol> superClasses();
@@ -31,6 +33,9 @@ public interface ClassSymbol extends Symbol {
 
   Set<Symbol> declaredMembers();
 
+  @CheckForNull
+  LocationInFile definitionLocation();
+
   @Beta
   Optional<Symbol> resolveMember(String memberName);
 

python-frontend/src/main/java/org/sonar/python/semantic/ClassSymbolImpl.java

@@ -20,6 +20,7 @@
 package org.sonar.python.semantic;
 
 
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -32,9 +33,15 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
+import org.sonar.plugins.python.api.LocationInFile;
+import org.sonar.plugins.python.api.PythonFile;
 import org.sonar.plugins.python.api.symbols.AmbiguousSymbol;
 import org.sonar.plugins.python.api.symbols.ClassSymbol;
 import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.tree.ClassDef;
+
+import static org.sonar.python.semantic.SymbolUtils.pathOf;
+import static org.sonar.python.tree.TreeUtils.locationInFile;
 
 public class ClassSymbolImpl extends SymbolImpl implements ClassSymbol {
 
@@ -46,15 +53,32 @@ public class ClassSymbolImpl extends SymbolImpl implements ClassSymbol {
   private Map<String, Symbol> membersByName = null;
   private boolean hasAlreadyReadSuperClasses = false;
   private boolean hasAlreadyReadMembers = false;
+  private final LocationInFile classDefinitionLocation;
+
+  public ClassSymbolImpl(ClassDef classDef, @Nullable String fullyQualifiedName, PythonFile pythonFile) {
+    super(classDef.name().name(), fullyQualifiedName);
+    this.setKind(Kind.CLASS);
+    String fileId = null;
+    if (!SymbolUtils.isTypeShedFile(pythonFile)) {
+      Path path = pathOf(pythonFile);
+      fileId = path != null ? path.toString() : pythonFile.toString();
+    }
+    classDefinitionLocation = locationInFile(classDef.name(), fileId);
+  }
 
   public ClassSymbolImpl(String name, @Nullable String fullyQualifiedName) {
+    this(name, fullyQualifiedName, null);
+  }
+
+  public ClassSymbolImpl(String name, @Nullable String fullyQualifiedName, @Nullable LocationInFile definitionLocation) {
     super(name, fullyQualifiedName);
-    this.setKind(Kind.CLASS);
+    classDefinitionLocation = definitionLocation;
+    setKind(Kind.CLASS);
   }
 
   @Override
   ClassSymbolImpl copyWithoutUsages() {
-    ClassSymbolImpl copiedClassSymbol = new ClassSymbolImpl(name(), fullyQualifiedName());
+    ClassSymbolImpl copiedClassSymbol = new ClassSymbolImpl(name(), fullyQualifiedName(), definitionLocation());
     for (Symbol superClass : superClasses()) {
       if (superClass == this) {
         copiedClassSymbol.superClasses.add(copiedClassSymbol);
@@ -123,6 +147,11 @@ public Optional<Symbol> resolveMember(String memberName) {
     return Optional.empty();
   }
 
+  @Override
+  public LocationInFile definitionLocation() {
+    return classDefinitionLocation;
+  }
+
   @Override
   public boolean isOrExtends(String fullyQualifiedClassName) {
     return allSuperClasses(false).stream().anyMatch(c -> c.fullyQualifiedName() != null && Objects.equals(fullyQualifiedClassName, c.fullyQualifiedName()));

python-frontend/src/main/java/org/sonar/python/semantic/FunctionSymbolImpl.java

@@ -39,10 +39,10 @@
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.tree.TypeAnnotation;
 import org.sonar.plugins.python.api.types.InferredType;
-import org.sonar.python.TokenLocation;
 import org.sonar.python.types.InferredTypes;
 
 import static org.sonar.python.semantic.SymbolUtils.pathOf;
+import static org.sonar.python.tree.TreeUtils.locationInFile;
 
 public class FunctionSymbolImpl extends SymbolImpl implements FunctionSymbol {
   private final List<Parameter> parameters = new ArrayList<>();
@@ -102,16 +102,6 @@ public FunctionSymbolImpl(String name, @Nullable String fullyQualifiedName, bool
     this.isStub = true;
   }
 
-  @CheckForNull
-  private static LocationInFile locationInFile(Tree tree, @Nullable String fileId) {
-    if (fileId == null) {
-      return null;
-    }
-    TokenLocation firstToken = new TokenLocation(tree.firstToken());
-    TokenLocation lastToken = new TokenLocation(tree.lastToken());
-    return new LocationInFile(fileId, firstToken.startLine(), firstToken.startLineOffset(), lastToken.endLine(), lastToken.endLineOffset());
-  }
-
   @Override
   FunctionSymbolImpl copyWithoutUsages() {
     FunctionSymbolImpl copy = new FunctionSymbolImpl(name(), this);

python-frontend/src/main/java/org/sonar/python/semantic/Scope.java

@@ -41,6 +41,7 @@
 import org.sonar.python.tree.FunctionDefImpl;
 import org.sonar.python.types.InferredTypes;
 import org.sonar.python.types.TypeShed;
+
 import static org.sonar.python.semantic.SymbolUtils.isTypeShedFile;
 
 class Scope {
@@ -129,8 +130,9 @@ private static Symbol copySymbol(String symbolName, Symbol symbol, Map<String, S
     if (symbol.is(Symbol.Kind.FUNCTION)) {
       return new FunctionSymbolImpl(symbolName, (FunctionSymbol) symbol);
     } else if (symbol.is(Symbol.Kind.CLASS)) {
-      ClassSymbolImpl classSymbol = new ClassSymbolImpl(symbolName, symbol.fullyQualifiedName());
       ClassSymbolImpl originalClassSymbol = (ClassSymbolImpl) symbol;
+      // Must use symbolName to preserve import aliases
+      ClassSymbolImpl classSymbol = new ClassSymbolImpl(symbolName, originalClassSymbol.fullyQualifiedName(), originalClassSymbol.definitionLocation());
       for (Symbol originalSymbol : originalClassSymbol.superClasses()) {
         Symbol globalSymbol = globalSymbolsByFQN.get(originalSymbol.fullyQualifiedName());
         if (globalSymbol != null && globalSymbol.kind() == Symbol.Kind.CLASS) {
@@ -254,7 +256,7 @@ void addClassSymbol(ClassDef classDef, @Nullable String fullyQualifiedName) {
     if (isExistingSymbol(symbolName)) {
       addBindingUsage(classDef.name(), Usage.Kind.CLASS_DECLARATION, fullyQualifiedName);
     } else {
-      ClassSymbolImpl classSymbol = new ClassSymbolImpl(symbolName, fullyQualifiedName);
+      ClassSymbolImpl classSymbol = new ClassSymbolImpl(classDef, fullyQualifiedName, pythonFile);
       symbols.add(classSymbol);
       symbolsByName.put(symbolName, classSymbol);
       classSymbol.addUsage(classDef.name(), Usage.Kind.CLASS_DECLARATION);

python-frontend/src/main/java/org/sonar/python/semantic/SymbolTableBuilder.java

@@ -188,8 +188,8 @@ private Set<Symbol> getAlternativeDefinitions(Symbol symbol, List<Usage> binding
           alternativeDefinitions.add(functionSymbol);
           break;
         case CLASS_DECLARATION:
-          ClassSymbolImpl classSymbol = new ClassSymbolImpl(symbol.name(), symbol.fullyQualifiedName());
           ClassDef classDef = (ClassDef) bindingUsage.tree().parent();
+          ClassSymbolImpl classSymbol = new ClassSymbolImpl(classDef, symbol.fullyQualifiedName(), pythonFile);
           resolveTypeHierarchy(classDef, classSymbol, pythonFile, scopesByRootTree.get(fileInput).symbolsByName);
           Scope classScope = scopesByRootTree.get(classDef);
           classSymbol.addMembers(getClassMembers(classScope.symbolsByName, classScope.instanceAttributesByName));

python-frontend/src/main/java/org/sonar/python/tree/TreeUtils.java

@@ -30,6 +30,7 @@
 import java.util.stream.Stream;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
+import org.sonar.plugins.python.api.LocationInFile;
 import org.sonar.plugins.python.api.symbols.ClassSymbol;
 import org.sonar.plugins.python.api.symbols.FunctionSymbol;
 import org.sonar.plugins.python.api.symbols.Symbol;
@@ -48,6 +49,7 @@
 import org.sonar.plugins.python.api.tree.Tree;
 import org.sonar.plugins.python.api.tree.Tree.Kind;
 import org.sonar.plugins.python.api.tree.Tuple;
+import org.sonar.python.TokenLocation;
 import org.sonar.python.api.PythonTokenType;
 
 public class TreeUtils {
@@ -252,4 +254,14 @@ public static boolean isBooleanLiteral(Tree tree) {
     }
     return false;
   }
+
+  @CheckForNull
+  public static LocationInFile locationInFile(Tree tree, @Nullable String fileId) {
+    if (fileId == null) {
+      return null;
+    }
+    TokenLocation firstToken = new TokenLocation(tree.firstToken());
+    TokenLocation lastToken = new TokenLocation(tree.lastToken());
+    return new LocationInFile(fileId, firstToken.startLine(), firstToken.startLineOffset(), lastToken.endLine(), lastToken.endLineOffset());
+  }
 }

python-frontend/src/test/java/org/sonar/python/semantic/FullyQualifiedNameTest.java

@@ -19,6 +19,7 @@
  */
 package org.sonar.python.semantic;
 
+import java.net.URI;
 import javax.annotation.Nullable;
 import org.junit.Test;
 import org.mockito.Mockito;
@@ -429,6 +430,7 @@ public void init_module_relative_import() {
     FileInput fileInput = new PythonTreeMaker().fileInput(PythonParser.create().parse(code));
     PythonFile pythonFile = Mockito.mock(PythonFile.class, "__init__.py");
     when(pythonFile.fileName()).thenReturn("__init__.py");
+    when(pythonFile.uri()).thenReturn(URI.create("mod/__init__.py"));
     PythonVisitorContext context = new PythonVisitorContext(fileInput, pythonFile, null, "foo.bar");
     fileInput = context.rootTree();
     CallExpression callExpression = (CallExpression) getAllDescendant(fileInput, tree -> tree.is(Tree.Kind.CALL_EXPR)).get(0);