diff --git a/python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java b/python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java index 4e0a72c829..22f527b787 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java +++ b/python-checks/src/main/java/org/sonar/python/checks/IdentityComparisonWithCachedTypesCheck.java @@ -19,24 +19,25 @@ */ package org.sonar.python.checks; -import java.util.HashSet; import java.util.List; -import java.util.Set; import org.sonar.check.Rule; import org.sonar.plugins.python.api.PythonSubscriptionCheck; import org.sonar.plugins.python.api.SubscriptionContext; -import org.sonar.plugins.python.api.symbols.Symbol; -import org.sonar.plugins.python.api.symbols.Usage; +import org.sonar.plugins.python.api.quickfix.PythonQuickFix; import org.sonar.plugins.python.api.tree.CallExpression; import org.sonar.plugins.python.api.tree.Expression; import org.sonar.plugins.python.api.tree.IsExpression; import org.sonar.plugins.python.api.tree.Name; import org.sonar.plugins.python.api.tree.Tree; -import org.sonar.plugins.python.api.types.InferredType; -import org.sonar.plugins.python.api.quickfix.PythonQuickFix; -import org.sonar.python.checks.utils.CheckUtils; +import org.sonar.plugins.python.api.types.BuiltinTypes; import org.sonar.python.quickfix.TextEditUtils; +import org.sonar.python.semantic.v2.SymbolV2; +import org.sonar.python.semantic.v2.UsageV2; import org.sonar.python.tree.TreeUtils; +import org.sonar.python.types.v2.PythonType; +import org.sonar.python.types.v2.TriBool; +import org.sonar.python.types.v2.TypeCheckBuilder; +import org.sonar.python.types.v2.TypeChecker; import static java.util.Arrays.asList; @@ -49,12 +50,40 @@ public class IdentityComparisonWithCachedTypesCheck extends PythonSubscriptionCh public static final String IS_QUICK_FIX_MESSAGE = "Replace with \"==\""; public static final String IS_NOT_QUICK_FIX_MESSAGE = "Replace with \"!=\""; + /** + * Fully qualified names of constructors and functions that are guaranteed to create fresh objects with + * references not shared anywhere else. + *

+ * If a reference arises from a call to one of these functions, and it does not escape anywhere else, then an + * is-comparison with such a reference will always return False. + *

+ * Note that these are fully qualified names of (value-level) expressions, not types. + */ + private static final List FQNS_CONSTRUCTORS_RETURNING_UNIQUE_REF = + asList("frozenset", "bytes", "int", "float", "str", "tuple", "hash"); + + /** + * Names of types that usually should not be compared with is. + *

+ * Note that these are names of types, not `fqn`s of expressions. + */ + private static final List NAMES_OF_TYPES_UNSUITABLE_FOR_COMPARISON = + asList("frozenset", "bytes", "int", "float", "tuple"); + + private TypeChecker typeChecker; + private TypeCheckBuilder isNoneTypeChecker; + @Override public void initialize(Context context) { - context.registerSyntaxNodeConsumer(Tree.Kind.IS, IdentityComparisonWithCachedTypesCheck::checkIsComparison); + context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, ctx -> { + typeChecker = ctx.typeChecker(); + isNoneTypeChecker = ctx.typeChecker().typeCheckBuilder().isBuiltinWithName(BuiltinTypes.NONE_TYPE); + }); + + context.registerSyntaxNodeConsumer(Tree.Kind.IS, this::checkIsComparison); } - private static void checkIsComparison(SubscriptionContext ctx) { + private void checkIsComparison(SubscriptionContext ctx) { IsExpression isExpr = (IsExpression) ctx.syntaxNode(); // Comparison to none is checked by S5727 @@ -83,60 +112,56 @@ private static void checkIsComparison(SubscriptionContext ctx) { } } - private static boolean isComparisonToNone(IsExpression isExpr) { - return CheckUtils.isNone(isExpr.leftOperand().type()) || CheckUtils.isNone(isExpr.rightOperand().type()); + private boolean isComparisonToNone(IsExpression isExpr) { + return isNone(isExpr.leftOperand().typeV2()) || isNone(isExpr.rightOperand().typeV2()); } - /** - * Fully qualified names of constructors and functions that would are guaranteed to create fresh objects with - * references not shared anywhere else. - * - * If a reference arises from an call to one of these functions, and it does not escape anywhere else, then an - * is-comparison with such a reference will always return False. - * - * Note that these are fully qualified names of (value-level) expressions, not types. - */ - private static final Set FQNS_CONSTRUCTORS_RETURNING_UNIQUE_REF = new HashSet<>( - asList("frozenset", "bytes", "int", "float", "str", "tuple", "hash")); - - /** - * Names of types that usually should not be compared with is. - * - * Note that these are names of types, not `fqn`s of expressions. - */ - private static final Set NAMES_OF_TYPES_UNSUITABLE_FOR_COMPARISON = new HashSet<>( - asList("frozenset", "bytes", "int", "float", "tuple")); + private boolean isNone(PythonType type) { + return isNoneTypeChecker.check(type) == TriBool.TRUE; + } /** * Checks whether an expression is either of a type that should generally be compared with ==, or * whether it holds a non-escaping reference that stems from a function that guarantees that the * returned reference is not shared anywhere. */ - private static boolean isUnsuitableOperand(Expression expr) { - InferredType tpe = expr.type(); - if (isUnsuitableType(tpe)) { - if (expr.is(Tree.Kind.NAME)) { - Symbol symb = ((Name) expr).symbol(); - // Impossible to cover. If the type could be inferred, then the `symb` cannot be null. - return symb != null && !isVariableThatCanEscape(symb); + private boolean isUnsuitableOperand(Expression expr) { + PythonType type = expr.typeV2(); + if (isUnsuitableType(type)) { + if (expr instanceof Name name) { + SymbolV2 symbol = name.symbolV2(); + return symbol != null && !isVariableThatCanEscape(symbol); } else { return true; } } if (expr.is(Tree.Kind.STRING_LITERAL)) { return true; - } else if (expr.is(Tree.Kind.CALL_EXPR)) { - Symbol calleeSymbol = ((CallExpression) expr).calleeSymbol(); - if (calleeSymbol != null) { - String calleeFqn = calleeSymbol.fullyQualifiedName(); - return FQNS_CONSTRUCTORS_RETURNING_UNIQUE_REF.contains(calleeFqn); + } else if (expr instanceof CallExpression callExpr) { + PythonType calleeType = callExpr.callee().typeV2(); + return isConstructorReturningUniqueRef(calleeType); + } + return false; + } + + private boolean isUnsuitableType(PythonType type) { + for (String builtinName : NAMES_OF_TYPES_UNSUITABLE_FOR_COMPARISON) { + TypeCheckBuilder builtinWithNameChecker = typeChecker.typeCheckBuilder().isBuiltinWithName(builtinName); + if (builtinWithNameChecker.check(type) == TriBool.TRUE) { + return true; } } return false; } - private static boolean isUnsuitableType(InferredType tpe) { - return NAMES_OF_TYPES_UNSUITABLE_FOR_COMPARISON.stream().anyMatch(tpe::canOnlyBe); + private boolean isConstructorReturningUniqueRef(PythonType type) { + for (String constructorName : FQNS_CONSTRUCTORS_RETURNING_UNIQUE_REF) { + TypeCheckBuilder constructorChecker = typeChecker.typeCheckBuilder().isTypeWithName(constructorName); + if (constructorChecker.check(type) == TriBool.TRUE) { + return true; + } + } + return false; } /** @@ -144,14 +169,14 @@ private static boolean isUnsuitableType(InferredType tpe) { * assignment. In such cases, we say that the reference to the value of the variable could have escaped, * and don't raise an issue. We do not check whether it could actually arrive in the other operand. */ - private static boolean isVariableThatCanEscape(Symbol symb) { - List usages = symb.usages(); + private static boolean isVariableThatCanEscape(SymbolV2 symb) { + List usages = symb.usages(); if (usages.size() > 2) { // Check whether there are any usages except assignments and `is`-comparisons. // (Assignments override, they don't let the reference escape. Comparisons with `is` also don't let it escape, // because they only compare, they don't copy the reference anywhere.) return usages.stream().anyMatch(u -> !u.isBindingUsage() && - TreeUtils.firstAncestorOfKind(u.tree(), Tree.Kind.IS) == null); + TreeUtils.firstAncestorOfKind(u.tree(), Tree.Kind.IS) == null); } // Must be definition and usage within is-comparison itself. return false; diff --git a/python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py b/python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py index 58823bea7f..b96fbf3c66 100644 --- a/python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py +++ b/python-checks/src/test/resources/checks/identityComparisonWithCachedTypes.py @@ -18,6 +18,9 @@ def literal_comparison(param): (4, 5) is param # Noncompliant {{Replace this "is" operator with "=="; identity operator is not reliable here.}} # ^^ + param is -1 # Noncompliant + param is 1 + 1 # Noncompliant + def literal_comparison_compliant(param): param is param # ok from the point of this rule param is someUnknownFunction(42) # ok @@ -149,3 +152,7 @@ def comparison_to_none(): "str" is None 123 is None None is "str" + +def potential_null_symbols(): + type(arr) is tuple + some_thing is other_thing diff --git a/python-frontend/src/main/java/org/sonar/plugins/python/api/tree/Name.java b/python-frontend/src/main/java/org/sonar/plugins/python/api/tree/Name.java index 9db8594df2..3841e6ea70 100644 --- a/python-frontend/src/main/java/org/sonar/plugins/python/api/tree/Name.java +++ b/python-frontend/src/main/java/org/sonar/plugins/python/api/tree/Name.java @@ -19,6 +19,7 @@ */ package org.sonar.plugins.python.api.tree; +import javax.annotation.CheckForNull; import org.sonar.api.Beta; import org.sonar.python.semantic.v2.SymbolV2; @@ -33,5 +34,6 @@ public interface Name extends Expression, HasSymbol { boolean isVariable(); @Beta + @CheckForNull SymbolV2 symbolV2(); } 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 index 844992d38c..28e75d2273 100644 --- 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,9 +54,12 @@ import org.sonar.plugins.python.api.tree.SetLiteral; import org.sonar.plugins.python.api.tree.StringLiteral; import org.sonar.plugins.python.api.tree.SubscriptionExpression; +import org.sonar.plugins.python.api.tree.Token; import org.sonar.plugins.python.api.tree.Tree; import org.sonar.plugins.python.api.tree.Tuple; import org.sonar.plugins.python.api.tree.TypeAnnotation; +import org.sonar.plugins.python.api.tree.UnaryExpression; +import org.sonar.plugins.python.api.types.BuiltinTypes; import org.sonar.python.semantic.v2.ClassTypeBuilder; import org.sonar.python.semantic.v2.FunctionTypeBuilder; import org.sonar.python.semantic.v2.SymbolV2; @@ -72,12 +75,15 @@ import org.sonar.python.tree.SetLiteralImpl; import org.sonar.python.tree.StringLiteralImpl; import org.sonar.python.tree.TupleImpl; +import org.sonar.python.tree.UnaryExpressionImpl; import org.sonar.python.types.v2.ClassType; import org.sonar.python.types.v2.FunctionType; import org.sonar.python.types.v2.Member; import org.sonar.python.types.v2.ModuleType; import org.sonar.python.types.v2.ObjectType; import org.sonar.python.types.v2.PythonType; +import org.sonar.python.types.v2.TriBool; +import org.sonar.python.types.v2.TypeCheckBuilder; import org.sonar.python.types.v2.TypeOrigin; import org.sonar.python.types.v2.TypeSource; import org.sonar.python.types.v2.UnionType; @@ -152,6 +158,36 @@ public void visitNumericLiteral(NumericLiteral numericLiteral) { numericLiteralImpl.typeV2(new ObjectType(pythonType, new ArrayList<>(), new ArrayList<>())); } + @Override + public void visitUnaryExpression(UnaryExpression unaryExpr) { + super.visitUnaryExpression(unaryExpr); + + var builtins = projectLevelTypeTable.getBuiltinsModule(); + Token operator = unaryExpr.operator(); + PythonType exprType = switch (operator.value()) { + case "~" -> builtins.resolveMember(BuiltinTypes.INT).orElse(PythonType.UNKNOWN); + case "not" -> builtins.resolveMember(BuiltinTypes.BOOL).orElse(PythonType.UNKNOWN); + case "+", "-" -> getTypeWhenUnaryPlusMinus(unaryExpr); + default -> unaryExpr.expression().typeV2(); + }; + + if (unaryExpr instanceof UnaryExpressionImpl unaryExprImpl) { + unaryExprImpl.typeV2(exprType); + } + } + + private PythonType getTypeWhenUnaryPlusMinus(UnaryExpression unaryExpr) { + var builtins = projectLevelTypeTable.getBuiltinsModule(); + var isBooleanTypeCheck = new TypeCheckBuilder(projectLevelTypeTable).isBuiltinWithName(BuiltinTypes.BOOL); + var innerExprType = unaryExpr.expression().typeV2(); + + if (isBooleanTypeCheck.check(innerExprType) == TriBool.TRUE) { + return builtins.resolveMember(BuiltinTypes.INT).orElse(PythonType.UNKNOWN); + } else { + return innerExprType; + } + } + @Override public void visitNone(NoneExpression noneExpression) { var builtins = this.projectLevelTypeTable.getBuiltinsModule(); diff --git a/python-frontend/src/main/java/org/sonar/python/tree/UnaryExpressionImpl.java b/python-frontend/src/main/java/org/sonar/python/tree/UnaryExpressionImpl.java index db001fbed0..dcb67968be 100644 --- a/python-frontend/src/main/java/org/sonar/python/tree/UnaryExpressionImpl.java +++ b/python-frontend/src/main/java/org/sonar/python/tree/UnaryExpressionImpl.java @@ -31,6 +31,7 @@ import org.sonar.plugins.python.api.tree.UnaryExpression; import org.sonar.plugins.python.api.types.InferredType; import org.sonar.python.types.InferredTypes; +import org.sonar.python.types.v2.PythonType; public class UnaryExpressionImpl extends PyTree implements UnaryExpression { @@ -39,6 +40,7 @@ public class UnaryExpressionImpl extends PyTree implements UnaryExpression { private final Kind kind; private final Token operator; private final Expression expression; + private PythonType type = PythonType.UNKNOWN; private static Map kindsByOperator() { Map map = new HashMap<>(); @@ -90,4 +92,14 @@ public InferredType type() { } return InferredTypes.anyType(); } + + public UnaryExpression typeV2(PythonType type) { + this.type = type; + return this; + } + + @Override + public PythonType typeV2() { + return type; + } } diff --git a/python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java b/python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java index c4db10fb8c..f75fa486fc 100644 --- a/python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java +++ b/python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java @@ -24,14 +24,14 @@ import java.util.HashSet; import java.util.List; import java.util.Set; -import org.sonar.python.semantic.v2.ProjectLevelTypeTable; +import org.sonar.python.semantic.v2.TypeTable; public class TypeCheckBuilder { - ProjectLevelTypeTable projectLevelTypeTable; + TypeTable projectLevelTypeTable; List predicates = new ArrayList<>(); - public TypeCheckBuilder(ProjectLevelTypeTable projectLevelTypeTable) { + public TypeCheckBuilder(TypeTable projectLevelTypeTable) { this.projectLevelTypeTable = projectLevelTypeTable; } diff --git a/python-frontend/src/test/java/org/sonar/python/semantic/v2/SymbolTableBuilderV2Test.java b/python-frontend/src/test/java/org/sonar/python/semantic/v2/SymbolTableBuilderV2Test.java index 2ec857b9bd..f2278b9aed 100644 --- a/python-frontend/src/test/java/org/sonar/python/semantic/v2/SymbolTableBuilderV2Test.java +++ b/python-frontend/src/test/java/org/sonar/python/semantic/v2/SymbolTableBuilderV2Test.java @@ -22,6 +22,7 @@ import java.util.List; import java.util.Set; import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.sonar.plugins.python.api.tree.AssignmentStatement; import org.sonar.plugins.python.api.tree.FileInput; @@ -40,23 +41,23 @@ void testSymbolTableModuleSymbols() { FileInput fileInput = PythonTestUtils.parse( """ import lib - + v = lib.foo() a = lib.A() b = a.do_something() x = 42 - + def script_do_something(param): c = 42 return c - + script_do_something() """ ); var symbolTable = new SymbolTableBuilderV2(fileInput) .build(); - + var moduleSymbols = symbolTable.getSymbolsByRootTree(fileInput); Assertions.assertThat(moduleSymbols) .hasSize(6) @@ -97,7 +98,7 @@ void testSymbolTableGlobalSymbols() { def script_do_something(param): global b b = 42 - + """ ); @@ -130,7 +131,7 @@ def inner(param): nonlocal a a = "hello" print(a) - + """ ); @@ -223,6 +224,44 @@ def script_do_something(param): } + + @Test + @Disabled("SONARPY-2259 primitives do not have a symbol") + void primitives_have_symbol() { + FileInput fileInput = PythonTestUtils.parse(""" + x = 3 + x is int + + float(x) + """); + + var symbolTable = new SymbolTableBuilderV2(fileInput) + .build(); + + var moduleSymbols = symbolTable.getSymbolsByRootTree(fileInput); + Assertions.assertThat(moduleSymbols) + .extracting(SymbolV2::name) + .containsExactlyInAnyOrder("x", "int", "float"); + } + + @Test + @Disabled("SONARPY-2260 variables which are never written to do not have a symbol") + void never_written_variables_have_symbol() { + FileInput fileInput = PythonTestUtils.parse(""" + x + 3 + if x == 3: pass + """); + + var symbolTable = new SymbolTableBuilderV2(fileInput) + .build(); + + var moduleSymbols = symbolTable.getSymbolsByRootTree(fileInput); + Assertions.assertThat(moduleSymbols) + .extracting(SymbolV2::name) + .containsExactlyInAnyOrder("x"); + } + + private static void assertNameSymbol(Name name, String expectedSymbolName, int expectedUsagesCount) { var symbol = name.symbolV2(); Assertions.assertThat(symbol).isNotNull(); 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 index f80424457e..62c0110746 100644 --- 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 @@ -25,11 +25,15 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Function; import java.util.stream.Stream; import org.assertj.core.api.Assertions; import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mockito; import org.sonar.plugins.python.api.LocationInFile; import org.sonar.plugins.python.api.PythonFile; @@ -77,6 +81,7 @@ import static org.sonar.python.PythonTestUtils.parse; import static org.sonar.python.PythonTestUtils.parseWithoutSymbols; import static org.sonar.python.PythonTestUtils.pythonFile; +import static org.sonar.python.types.v2.TypesTestUtils.BOOL_TYPE; import static org.sonar.python.types.v2.TypesTestUtils.DICT_TYPE; import static org.sonar.python.types.v2.TypesTestUtils.EXCEPTION_TYPE; import static org.sonar.python.types.v2.TypesTestUtils.FLOAT_TYPE; @@ -2229,6 +2234,44 @@ void return_type_of_call_expression_2() { ).typeV2().unwrappedType()).isEqualTo(NONE_TYPE); } + @Test + void unary_expression() { + Function exprToType = expr -> expr.typeV2().unwrappedType(); + + assertThat(lastExpression("-1")).extracting(exprToType).isEqualTo(INT_TYPE); + assertThat(lastExpression("+1")).extracting(exprToType).isEqualTo(INT_TYPE); + assertThat(lastExpression("-1.0")).extracting(exprToType).isEqualTo(FLOAT_TYPE); + assertThat(lastExpression("+1.0")).extracting(exprToType).isEqualTo(FLOAT_TYPE); + assertThat(lastExpression("-True")).extracting(exprToType).isEqualTo(INT_TYPE); + assertThat(lastExpression("+True")).extracting(exprToType).isEqualTo(INT_TYPE); + assertThat(lastExpression("~1")).extracting(exprToType).isEqualTo(INT_TYPE); + assertThat(lastExpression("~True")).extracting(exprToType).isEqualTo(INT_TYPE); + assertThat(lastExpression("not True")).extracting(exprToType).isEqualTo(BOOL_TYPE); + assertThat(lastExpression("not 1")).extracting(exprToType).isEqualTo(BOOL_TYPE); + } + + static Stream unary_expression_of_variables() { + return Stream.of( + Arguments.of("x = 1; -x", INT_TYPE), + Arguments.of("x = 1; +x", INT_TYPE), + Arguments.of("x = True; -x", INT_TYPE), + Arguments.of("x = True; +x", INT_TYPE), + Arguments.of("x = True; ~x", INT_TYPE), + Arguments.of("x = True; not x", BOOL_TYPE), + Arguments.of("x = 1; not x", BOOL_TYPE), + + Arguments.of("x = 1; -(x + 1)", INT_TYPE) + ); + } + + @Disabled("SONARPY-2257 unary expressions of non-literals do not propagate their type") + @ParameterizedTest + @MethodSource("unary_expression_of_variables") + void unary_expression_of_variables(String code, PythonType expectedType) { + assertThat(lastExpression(code)).extracting(expr -> expr.typeV2().unwrappedType()).isEqualTo(expectedType); + } + + @Test void imported_ambiguous_symbol() { FileInput fileInput = inferTypes("""