From bcd2b37cadb257e6f1ee6ca477f9ae9471a77b14 Mon Sep 17 00:00:00 2001 From: Sebastian Zumbrunn Date: Wed, 23 Oct 2024 16:18:01 +0200 Subject: [PATCH] SONARPY-2204 Migrate S5795 IdentityComparisonWithCachedTypesCheck to the new type model --- ...dentityComparisonWithCachedTypesCheck.java | 114 +++++++++++------- 1 file changed, 71 insertions(+), 43 deletions(-) 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..26fe377c67 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 @@ -25,18 +25,21 @@ 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 +52,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 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 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,40 +114,24 @@ 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(); + private boolean isUnsuitableOperand(Expression expr) { + PythonType type = expr.typeV2(); + if (isUnsuitableType(type)) { + if (expr instanceof Name name) { + SymbolV2 symb = name.symbolV2(); // Impossible to cover. If the type could be inferred, then the `symb` cannot be null. return symb != null && !isVariableThatCanEscape(symb); } else { @@ -125,18 +140,31 @@ private static boolean isUnsuitableOperand(Expression expr) { } 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 +172,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;