Fix quality issues (#1808)

guillaume-dequenne · guillaume-dequenne · commit c63eae2a9e4f · 2024-05-21T17:09:08.000+02:00
diff --git a/python-checks/src/test/java/org/sonar/python/checks/NonCallableCalledCheckTest.java b/python-checks/src/test/java/org/sonar/python/checks/NonCallableCalledCheckTest.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.python.checks;
 
+import java.util.List;
 import org.junit.jupiter.api.Test;
 import org.sonar.python.checks.utils.PythonCheckVerifier;
 
@@ -29,4 +30,9 @@ void test() {
     PythonCheckVerifier.verify("src/test/resources/checks/nonCallableCalled.py", new NonCallableCalledCheck());
   }
 
+  @Test
+  void test_multiple_files() {
+    PythonCheckVerifier.verify(List.of("src/test/resources/checks/nonCallableCalledImporter.py", "src/test/resources/checks/nonCallableCalledImported.py"), new NonCallableCalledCheck());
+  }
+
 }
diff --git a/python-checks/src/test/resources/checks/nonCallableCalledImported.py b/python-checks/src/test/resources/checks/nonCallableCalledImported.py
@@ -0,0 +1,7 @@
+class MyNonCallableClass:
+    ...
+
+
+class MyCallableClass:
+    def __call__(self):
+        ...
diff --git a/python-checks/src/test/resources/checks/nonCallableCalledImporter.py b/python-checks/src/test/resources/checks/nonCallableCalledImporter.py
@@ -0,0 +1,15 @@
+from nonCallableCalledImported import MyNonCallableClass, MyCallableClass
+
+
+imported_non_callable = MyNonCallableClass()
+imported_callable = MyCallableClass()
+
+imported_non_callable()  # Noncompliant
+imported_callable()
+
+
+class LocallyDefinedNonCallable:
+    ...
+
+local_non_callable = LocallyDefinedNonCallable()
+local_non_callable()  # Noncompliant
diff --git a/python-frontend/src/main/java/org/sonar/python/semantic/v2/ScopeV2.java b/python-frontend/src/main/java/org/sonar/python/semantic/v2/ScopeV2.java
@@ -54,7 +54,7 @@ public Tree root() {
     return rootTree;
   }
 
-  void addBindingUsage(Name nameTree, UsageV2.Kind kind, @Nullable String fullyQualifiedName) {
+  void addBindingUsage(Name nameTree, UsageV2.Kind kind) {
     String symbolName = nameTree.name();
     if (!isExistingSymbol(symbolName)) {
       SymbolV2 symbol = new SymbolV2(symbolName);
@@ -90,7 +90,7 @@ void createSelfParameter(Parameter parameter) {
       return;
     }
     String symbolName = nameTree.name();
-    //TODO: Check with SelfSymbolImpl
+    //TODO: SONARPY-1865 Represent "self"
     SymbolV2 symbol = new SymbolV2(symbolName);
     symbols.add(symbol);
     symbolsByName.put(symbolName, symbol);
diff --git a/python-frontend/src/main/java/org/sonar/python/semantic/v2/SymbolsModuleTypeProvider.java b/python-frontend/src/main/java/org/sonar/python/semantic/v2/SymbolsModuleTypeProvider.java
@@ -98,11 +98,6 @@ private ModuleType createModuleFromSymbols(@Nullable String name, @Nullable Modu
     return module;
   }
 
-  private static PythonType convertToObjectType(Symbol symbol) {
-    // What should we have here?
-    return PythonType.UNKNOWN;
-  }
-
   private static PythonType convertToFunctionType(FunctionSymbol symbol, Map<Symbol, PythonType> createdTypesBySymbol) {
     if (createdTypesBySymbol.containsKey(symbol)) {
       return createdTypesBySymbol.get(symbol);
@@ -163,7 +158,8 @@ private PythonType convertToType(Symbol symbol, Map<Symbol, PythonType> createdT
       case CLASS -> convertToClassType((ClassSymbol) symbol, createdTypesBySymbol);
       case FUNCTION -> convertToFunctionType((FunctionSymbol) symbol, createdTypesBySymbol);
       case AMBIGUOUS -> convertToUnionType((AmbiguousSymbol) symbol, createdTypesBySymbol);
-      case OTHER -> convertToObjectType(symbol);
+      // Symbols that are neither classes or function nor ambiguous symbols whose alternatives are all classes or functions are considered of unknown type
+      case OTHER -> PythonType.UNKNOWN;
     };
   }
 
diff --git a/python-frontend/src/main/java/org/sonar/python/semantic/v2/TypeInferenceV2.java b/python-frontend/src/main/java/org/sonar/python/semantic/v2/TypeInferenceV2.java
@@ -55,7 +55,7 @@ public TypeInferenceV2(ProjectLevelTypeTable projectLevelTypeTable, PythonFile p
   }
 
   public void inferTypes(FileInput fileInput) {
-    TrivialTypeInferenceVisitor trivialTypeInferenceVisitor = new TrivialTypeInferenceVisitor(projectLevelTypeTable, pythonFile, symbolTable);
+    TrivialTypeInferenceVisitor trivialTypeInferenceVisitor = new TrivialTypeInferenceVisitor(projectLevelTypeTable, pythonFile);
     fileInput.accept(trivialTypeInferenceVisitor);
 
     inferTypesAndMemberAccessSymbols(fileInput);
@@ -88,7 +88,7 @@ private void inferTypesAndMemberAccessSymbols(FileInput fileInput) {
 
   private void inferTypesAndMemberAccessSymbols(FunctionDef functionDef) {
     Set<Name> parameterNames = TreeUtils.nonTupleParameters(functionDef).stream()
-      // TODO: it probably doesn't make sense to restrict to annotated parameters here
+      // TODO SONARPY-1866: it probably doesn't make sense to restrict to annotated parameters here
       .filter(parameter -> parameter.typeAnnotation() != null)
       .map(Parameter::name)
       .collect(Collectors.toSet());
diff --git a/python-frontend/src/main/java/org/sonar/python/semantic/v2/WriteUsagesVisitor.java b/python-frontend/src/main/java/org/sonar/python/semantic/v2/WriteUsagesVisitor.java
@@ -114,7 +114,7 @@ public void visitPyListOrSetCompExpression(ComprehensionExpression tree) {
 
   @Override
   public void visitFunctionDef(FunctionDef functionDef) {
-    currentScope().addBindingUsage(functionDef.name(), UsageV2.Kind.FUNC_DECLARATION, null);
+    currentScope().addBindingUsage(functionDef.name(), UsageV2.Kind.FUNC_DECLARATION);
     createAndEnterScope(functionDef, currentScope());
     createTypeParameters(functionDef.typeParams());
     createParameters(functionDef);
@@ -127,7 +127,7 @@ private void createTypeParameters(@Nullable TypeParams typeParams) {
       .map(TypeParams::typeParamsList)
       .stream()
       .flatMap(Collection::stream)
-      .forEach(typeParam -> currentScope().addBindingUsage(typeParam.name(), UsageV2.Kind.TYPE_PARAM_DECLARATION, null));
+      .forEach(typeParam -> currentScope().addBindingUsage(typeParam.name(), UsageV2.Kind.TYPE_PARAM_DECLARATION));
   }
 
 
@@ -151,7 +151,7 @@ private void createParameters(FunctionLike function) {
       .skip(hasSelf ? 1 : 0)
       .map(Parameter::name)
       .filter(Objects::nonNull)
-      .forEach(param -> currentScope().addBindingUsage(param, UsageV2.Kind.PARAMETER, null));
+      .forEach(param -> currentScope().addBindingUsage(param, UsageV2.Kind.PARAMETER));
 
     parameterList.all().stream()
       .filter(param -> param.is(Tree.Kind.TUPLE_PARAMETER))
@@ -163,7 +163,7 @@ private void addTupleParamElementsToBindingUsage(TupleParameter param) {
     param.parameters().stream()
       .filter(p -> p.is(Tree.Kind.PARAMETER))
       .map(p -> ((Parameter) p).name())
-      .forEach(name -> currentScope().addBindingUsage(name, UsageV2.Kind.PARAMETER, null));
+      .forEach(name -> currentScope().addBindingUsage(name, UsageV2.Kind.PARAMETER));
     param.parameters().stream()
       .filter(p -> p.is(Tree.Kind.TUPLE_PARAMETER))
       .map(TupleParameter.class::cast)
@@ -172,13 +172,13 @@ private void addTupleParamElementsToBindingUsage(TupleParameter param) {
 
   @Override
   public void visitTypeAliasStatement(TypeAliasStatement typeAliasStatement) {
-    currentScope().addBindingUsage(typeAliasStatement.name(), UsageV2.Kind.TYPE_ALIAS_DECLARATION, null);
+    currentScope().addBindingUsage(typeAliasStatement.name(), UsageV2.Kind.TYPE_ALIAS_DECLARATION);
     super.visitTypeAliasStatement(typeAliasStatement);
   }
 
   @Override
   public void visitClassDef(ClassDef classDef) {
-    currentScope().addBindingUsage(classDef.name(), UsageV2.Kind.CLASS_DECLARATION, null);
+    currentScope().addBindingUsage(classDef.name(), UsageV2.Kind.CLASS_DECLARATION);
     createAndEnterScope(classDef, currentScope());
     createTypeParameters(classDef.typeParams());
     super.visitClassDef(classDef);
@@ -198,7 +198,7 @@ public void visitImportFrom(ImportFrom importFrom) {
       ? moduleTree.names().stream().map(Name::name).collect(Collectors.joining("."))
       : null;
     if (importFrom.isWildcardImport()) {
-      // FIXME: handle wildcard import
+      // TODO: SONARPY-1781 handle wildcard import
     } else {
       createImportedNames(importFrom.importedNames(), moduleName, importFrom.dottedPrefixForModule());
     }
@@ -212,15 +212,15 @@ private void createImportedNames(List<AliasedName> importedNames, @Nullable Stri
       String targetModuleName = fromModuleName;
       Name alias = module.alias();
       if (targetModuleName != null) {
-        addBindingUsage(alias == null ? nameTree : alias, UsageV2.Kind.IMPORT, null);
+        addBindingUsage(alias == null ? nameTree : alias, UsageV2.Kind.IMPORT);
       } else if (alias != null) {
-        addBindingUsage(alias, UsageV2.Kind.IMPORT, null);
+        addBindingUsage(alias, UsageV2.Kind.IMPORT);
       } else if (dottedPrefix.isEmpty() && dottedNames.size() > 1) {
         // Submodule import
-        addBindingUsage(nameTree, UsageV2.Kind.IMPORT, null);
+        addBindingUsage(nameTree, UsageV2.Kind.IMPORT);
       } else {
         // It's a simple case - no "from" imports or aliasing
-        addBindingUsage(nameTree, UsageV2.Kind.IMPORT, null);
+        addBindingUsage(nameTree, UsageV2.Kind.IMPORT);
       }
     });
   }
@@ -238,12 +238,12 @@ public void visitComprehensionFor(ComprehensionFor tree) {
   }
 
   private void addCompDeclarationParam(Tree tree) {
-    boundNamesFromExpression(tree).forEach(name -> currentScope().addBindingUsage(name, UsageV2.Kind.COMP_DECLARATION, null));
+    boundNamesFromExpression(tree).forEach(name -> currentScope().addBindingUsage(name, UsageV2.Kind.COMP_DECLARATION));
   }
 
   private void createLoopVariables(ForStatement loopTree) {
     loopTree.expressions().forEach(expr ->
-      boundNamesFromExpression(expr).forEach(name -> currentScope().addBindingUsage(name, UsageV2.Kind.LOOP_DECLARATION, null)));
+      boundNamesFromExpression(expr).forEach(name -> currentScope().addBindingUsage(name, UsageV2.Kind.LOOP_DECLARATION)));
   }
 
   @Override
@@ -260,7 +260,7 @@ public void visitAssignmentStatement(AssignmentStatement pyAssignmentStatementTr
   public void visitAnnotatedAssignment(AnnotatedAssignment annotatedAssignment) {
     if (annotatedAssignment.variable().is(Tree.Kind.NAME)) {
       Name variable = (Name) annotatedAssignment.variable();
-      addBindingUsage(variable, UsageV2.Kind.ASSIGNMENT_LHS, null);
+      addBindingUsage(variable, UsageV2.Kind.ASSIGNMENT_LHS);
     }
     super.visitAnnotatedAssignment(annotatedAssignment);
   }
@@ -283,7 +283,7 @@ public void visitAssignmentExpression(AssignmentExpression assignmentExpression)
   public void visitGlobalStatement(GlobalStatement globalStatement) {
     // Global statements are not binding usages, but we consider them as such for symbol creation
     globalStatement.variables().forEach(name -> {
-      moduleScope.addBindingUsage(name, UsageV2.Kind.GLOBAL_DECLARATION, null);
+      moduleScope.addBindingUsage(name, UsageV2.Kind.GLOBAL_DECLARATION);
       currentScope().addGlobalName(name);
     });
     super.visitGlobalStatement(globalStatement);
@@ -314,10 +314,6 @@ public void visitCapturePattern(CapturePattern capturePattern) {
   }
 
   private void addBindingUsage(Name nameTree, UsageV2.Kind usage) {
-    addBindingUsage(nameTree, usage, null);
-  }
-
-  private void addBindingUsage(Name nameTree, UsageV2.Kind usage, @Nullable String fullyQualifiedName) {
-    currentScope().addBindingUsage(nameTree, usage, fullyQualifiedName);
+    currentScope().addBindingUsage(nameTree, usage);
   }
 }
diff --git a/python-frontend/src/main/java/org/sonar/python/semantic/v2/types/FlowSensitiveTypeInference.java b/python-frontend/src/main/java/org/sonar/python/semantic/v2/types/FlowSensitiveTypeInference.java
@@ -89,7 +89,7 @@ public void updateProgramState(Tree element, ProgramState programState) {
     } else if (element instanceof FunctionDef functionDef) {
       handleDefinition(functionDef, state);
     } else {
-      // TODO: isinstance visitor
+      // Here we should run "isinstance" visitor when we handle declared types, to avoid FPs when type guard checks are made
       updateTree(element, state);
     }
   }
diff --git a/python-frontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java b/python-frontend/src/main/java/org/sonar/python/semantic/v2/types/TrivialTypeInferenceVisitor.java
@@ -54,7 +54,6 @@
 import org.sonar.python.semantic.v2.ClassTypeBuilder;
 import org.sonar.python.semantic.v2.FunctionTypeBuilder;
 import org.sonar.python.semantic.v2.ProjectLevelTypeTable;
-import org.sonar.python.semantic.v2.SymbolTable;
 import org.sonar.python.semantic.v2.SymbolV2;
 import org.sonar.python.semantic.v2.UsageV2;
 import org.sonar.python.tree.DictionaryLiteralImpl;
@@ -79,14 +78,12 @@
 public class TrivialTypeInferenceVisitor extends BaseTreeVisitor {
 
   private final ProjectLevelTypeTable projectLevelTypeTable;
-  private final SymbolTable symbolTable;
   private final String fileId;
 
   private final Deque<PythonType> typeStack = new ArrayDeque<>();
 
-  public TrivialTypeInferenceVisitor(ProjectLevelTypeTable projectLevelTypeTable, PythonFile pythonFile, SymbolTable symbolTable) {
+  public TrivialTypeInferenceVisitor(ProjectLevelTypeTable projectLevelTypeTable, PythonFile pythonFile) {
     this.projectLevelTypeTable = projectLevelTypeTable;
-    this.symbolTable = symbolTable;
     Path path = pathOf(pythonFile);
     this.fileId = path != null ? path.toString() : pythonFile.toString();
   }
@@ -101,7 +98,7 @@ public void visitFileInput(FileInput fileInput) {
   @Override
   public void visitStringLiteral(StringLiteral stringLiteral) {
     ModuleType builtins = this.projectLevelTypeTable.getModule();
-    // TODO: multiple object types to represent str instance?
+    // TODO: SONARPY-1867 multiple object types to represent str instance?
     PythonType strType = builtins.resolveMember("str").orElse(PythonType.UNKNOWN);
     ((StringLiteralImpl) stringLiteral).typeV2(new ObjectType(strType, new ArrayList<>(), new ArrayList<>()));
   }
@@ -149,7 +146,7 @@ public void visitNumericLiteral(NumericLiteral numericLiteral) {
   @Override
   public void visitNone(NoneExpression noneExpression) {
     ModuleType builtins = this.projectLevelTypeTable.getModule();
-    // TODO: multiple object types to represent str instance?
+    // TODO: SONARPY-1867 multiple object types to represent str instance?
     PythonType noneType = builtins.resolveMember("NoneType").orElse(PythonType.UNKNOWN);
     ((NoneExpressionImpl) noneExpression).typeV2(new ObjectType(noneType, new ArrayList<>(), new ArrayList<>()));
   }
@@ -159,7 +156,6 @@ public void visitListLiteral(ListLiteral listLiteral) {
     ModuleType builtins = this.projectLevelTypeTable.getModule();
     scan(listLiteral.elements());
     List<PythonType> pythonTypes = listLiteral.elements().expressions().stream().map(Expression::typeV2).distinct().toList();
-    // TODO: cleanly reduce attributes
     PythonType listType = builtins.resolveMember("list").orElse(PythonType.UNKNOWN);
     ((ListLiteralImpl) listLiteral).typeV2(new ObjectType(listType, pythonTypes, new ArrayList<>()));
   }
@@ -195,7 +191,7 @@ static void resolveTypeHierarchy(ClassDef classDef, ClassTypeBuilder classTypeBu
 
   private static void addParentClass(ClassTypeBuilder classTypeBuilder, RegularArgument regularArgument) {
     Name keyword = regularArgument.keywordArgument();
-    // TODO: store names if not resolved properly
+    // TODO: SONARPY-1871 store names if not resolved properly
     if (keyword != null) {
       if ("metaclass".equals(keyword.name())) {
         PythonType argumentType = getTypeV2FromArgument(regularArgument);
@@ -205,7 +201,7 @@ private static void addParentClass(ClassTypeBuilder classTypeBuilder, RegularArg
     }
     PythonType argumentType = getTypeV2FromArgument(regularArgument);
     classTypeBuilder.superClasses().add(argumentType);
-    // TODO: handle generics
+    // TODO: SONARPY-1869 handle generics
   }
 
   private static PythonType getTypeV2FromArgument(RegularArgument regularArgument) {
@@ -364,7 +360,7 @@ public void visitName(Name name) {
       .filter(Expression.class::isInstance)
       .map(Expression.class::cast)
       .map(Expression::typeV2)
-      // FIXME: classes and functions should be propagated like other types
+      // TODO: classes (SONARPY-1829) and functions should be propagated like other types
       .filter(t -> (t instanceof ClassType) || (t instanceof FunctionType))
       .ifPresent(type -> setTypeToName(name, type));
   }
diff --git a/python-frontend/src/main/java/org/sonar/python/tree/CallExpressionImpl.java b/python-frontend/src/main/java/org/sonar/python/tree/CallExpressionImpl.java
@@ -47,7 +47,6 @@
 import org.sonar.python.types.HasTypeDependencies;
 import org.sonar.python.types.InferredTypes;
 import org.sonar.python.types.v2.ClassType;
-import org.sonar.python.types.v2.FunctionType;
 import org.sonar.python.types.v2.ObjectType;
 import org.sonar.python.types.v2.PythonType;
 
diff --git a/python-frontend/src/main/java/org/sonar/python/tree/ListLiteralImpl.java b/python-frontend/src/main/java/org/sonar/python/tree/ListLiteralImpl.java
@@ -79,6 +79,7 @@ public InferredType type() {
     return InferredTypes.LIST;
   }
 
+  @Override
   public PythonType typeV2() {
     return this.typeV2;
   }
diff --git a/python-frontend/src/main/java/org/sonar/python/tree/NumericLiteralImpl.java b/python-frontend/src/main/java/org/sonar/python/tree/NumericLiteralImpl.java
@@ -102,7 +102,7 @@ private InferredType computeType() {
     return InferredTypes.INT;
   }
 
-
+  @Override
   public PythonType typeV2() {
     return this.typeV2;
   }
diff --git a/python-frontend/src/main/java/org/sonar/python/tree/StringLiteralImpl.java b/python-frontend/src/main/java/org/sonar/python/tree/StringLiteralImpl.java
@@ -80,6 +80,7 @@ public InferredType type() {
     return InferredTypes.STR;
   }
 
+  @Override
   public PythonType typeV2() {
     return this.typeV2;
   }
diff --git a/python-frontend/src/main/java/org/sonar/python/types/v2/ClassType.java b/python-frontend/src/main/java/org/sonar/python/types/v2/ClassType.java
@@ -91,7 +91,7 @@ public boolean isASubClassFrom(ClassType other) {
   }
 
   public boolean areAttributesCompatible(ClassType other) {
-    return attributes.stream().allMatch(attr -> other.attributes.stream().anyMatch(otherAttr -> attr.isCompatibleWith(otherAttr)));
+    return attributes.stream().allMatch(attr -> other.attributes.stream().anyMatch(attr::isCompatibleWith));
   }
 
   @Override
diff --git a/python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeInferenceV2Test.java b/python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeInferenceV2Test.java
@@ -24,7 +24,6 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Disabled;
@@ -351,7 +350,7 @@ def foo(param: int):
 
     var functionDef = (FunctionDef) root.statements().statements().get(0);
     var lastExpressionStatement = (ExpressionStatement) functionDef.body().statements().get(functionDef.body().statements().size() -1);
-    // TODO: should be declared int
+    // TODO SONARPY-1773: should be declared int
     Assertions.assertThat(lastExpressionStatement.expressions().get(0).typeV2().unwrappedType()).isEqualTo(PythonType.UNKNOWN);
   }
 
@@ -778,8 +777,6 @@ class MyClass:
       MyClass()
       """);
 
-    ClassDef classDef = ((ClassDef) fileInput.statements().statements().get(0));
-
     List<CallExpression> calls = PythonTestUtils.getAllDescendant(fileInput, tree -> tree.is(Tree.Kind.CALL_EXPR));
     PythonType calleeType1 = calls.get(0).callee().typeV2();
     PythonType calleeType2 = calls.get(1).callee().typeV2();
diff --git a/python-frontend/src/test/java/org/sonar/python/types/v2/ClassTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/v2/ClassTypeTest.java
diff --git a/python-frontend/src/test/java/org/sonar/python/types/v2/FunctionTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/v2/FunctionTypeTest.java
diff --git a/python-frontend/src/test/java/org/sonar/python/types/v2/ObjectTypeTest.java b/python-frontend/src/test/java/org/sonar/python/types/v2/ObjectTypeTest.java

Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@`
`19`	`19`	`*/`
`20`	`20`	`package org.sonar.python.checks;`
`21`	`21`
	`22`	`+import java.util.List;`
`22`	`23`	`import org.junit.jupiter.api.Test;`
`23`	`24`	`import org.sonar.python.checks.utils.PythonCheckVerifier;`
`24`	`25`
`@@ -29,4 +30,9 @@ void test() {`
`29`	`30`	`PythonCheckVerifier.verify("src/test/resources/checks/nonCallableCalled.py", new NonCallableCalledCheck());`
`30`	`31`	`}`
`31`	`32`
	`33`	`+ @Test`
	`34`	`+ void test_multiple_files() {`
	`35`	`+ PythonCheckVerifier.verify(List.of("src/test/resources/checks/nonCallableCalledImporter.py", "src/test/resources/checks/nonCallableCalledImported.py"), new NonCallableCalledCheck());`
	`36`	`+ }`
	`37`	`+`
`32`	`38`	`}`
Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,7 @@ public void updateProgramState(Tree element, ProgramState programState) {`
`89`	`89`	`} else if (element instanceof FunctionDef functionDef) {`
`90`	`90`	`handleDefinition(functionDef, state);`
`91`	`91`	`} else {`
`92`		`- // TODO: isinstance visitor`
	`92`	`+ // Here we should run "isinstance" visitor when we handle declared types, to avoid FPs when type guard checks are made`
`93`	`93`	`updateTree(element, state);`
`94`	`94`	`}`
`95`	`95`	`}`