Skip to content

Commit

Permalink
SONARPY-2332 Fix FP on S5845 when comparing enum objects with types
Browse files Browse the repository at this point in the history
  • Loading branch information
@thomas-serre-sonarsource
thomas-serre-sonarsource committed Nov 22, 2024
1 parent 2e55165 commit 14a8153
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 190 deletions.
196 changes: 6 additions & 190 deletions python-checks/src/test/resources/checks/tests/assertOnDissimilarTypes.py
View file
Original file line number Diff line number Diff line change
@@ -1,188 +1,5 @@
import unittest

class A:
pass

class B:
pass

class C:
pass

class D:
pass

class ClassWithEq:
def __eq__(self, other):
pass

class ClassWithNe:
def __ne__(self, other):
pass

class MyTest(unittest.TestCase):
def test_assert_is_non_compliant(self):
myString = "toto"
myInt = 3
a = A()
b = B()
aInd = A()
# ^^^^^^^^^^> {{Last assignment of "aInd"}}
bInd = B()
# ^^^^^^^^^^> {{Last assignment of "bInd"}}

self.assertIs(aInd, bInd) # Noncompliant {{Change this assertion to not compare dissimilar types (A and B).}}
# ^^^^^^^^^^
aInd = A()
self.assertEqual(first="string", second=True) # Noncompliant
self.assertIs(msg="my message", first="myString", second=a) # Noncompliant
self.assertIs(a, myString) # Noncompliant
self.assertIs(a, "string") # Noncompliant
self.assertIs(a, myInt) # Noncompliant
self.assertIs(a, 5) # Noncompliant
self.assertIs(a, 42.42) # Noncompliant
self.assertIs(a, 42j) # Noncompliant
self.assertIs('foo'.partition('f'), "42") # Noncompliant

def test_assert_is_compliant(self):
a = A()
b = a
c = None

self.assertIs() # OK
self.assertIs(a, A()) # OK
self.assertIs(a, a) # OK
self.assertIs(a, b) # OK
self.assertIs(a, None) # OK
self.assertIs(None, None) # OK
self.assertIs(c, None) # OK
self.assertIs(a, c) # OK
self.assertIs("my string", "my string") # OK
self.assertIs(42, 42) # OK
self.assertIs("my string", "another string") # OK
self.assertIs(42, 0) # OK
self.assertIs(msg="my message", first=a, second=a) # OK

def test_assert_equal_non_compliant(self):
ab = A()
cd = C()
if x == 42:
ab = B()
cd = D()
myString = "toto"
myInt = 3
a = A()
b = B()
classWithEq = ClassWithEq()
classWithNe = ClassWithNe()
mydict = {"x": a}

self.assertEqual(a, ab)
self.assertEqual("string", True) # Noncompliant
self.assertEqual(A(), []) # Noncompliant
self.assertEqual(A(), ()) # Noncompliant
self.assertEqual(A(), {}) # Noncompliant
self.assertEqual({42:''}, {1}) # Noncompliant
self.assertEqual(A(), B()) # Noncompliant
self.assertEqual(a, b) # Noncompliant
self.assertEqual(a, myString) # Noncompliant
self.assertEqual(a, "string") # Noncompliant
self.assertEqual(a, myInt) # Noncompliant
self.assertEqual(a, 5) # Noncompliant
self.assertEqual(a, 42.42) # Noncompliant
self.assertEqual(a, 42j) # Noncompliant
self.assertEqual('foo'.partition('f'), "42") # Noncompliant

# Test with UnionType argument for error message without type indication
self.assertEqual(5, ab) # Noncompliant {{Change this assertion to not compare dissimilar types.}}
self.assertEqual(ab, 5) # Noncompliant {{Change this assertion to not compare dissimilar types.}}
self.assertEqual(ab, cd) # Noncompliant {{Change this assertion to not compare dissimilar types.}}

self.assertEqual(acos(1), "0") # OK

tuple = "name", "passwd", 123, 456, "gecos", "dir", "shell"
passwd = pwd.struct_passwd(tuple)
self.assertEqual(passwd, "something")
passwd_2 = pwd.struct_passwd(tuple)
self.assertEqual(passwd, passwd_2) # OK
self.assertEqual(passwd, pwd.getpwuid(1)) # OK


def test_assert_equal_compliant(self):
myString = "toto"
myInt = 3
a = A()
b = B()
aa = a
classWithEq = ClassWithEq()
classWithNe = ClassWithNe()
mydict = {"x": a}

self.assertEqual(a, classWithEq) # OK
self.assertEqual(a, classWithNe) # OK
self.assertEqual(a, a) # OK
self.assertEqual(a, None) # OK
self.assertEqual(None, a) # OK
self.assertEqual(None, None) # OK
self.assertEqual(d, None) # OK
self.assertEqual(a, aa) # OK
self.assertEqual(classWithEq, classWithEq) # OK
self.assertEqual(classWithEq, myString) # OK
self.assertEqual(classWithEq, "string") # OK
self.assertEqual(classWithEq, myInt) # OK
self.assertEqual(classWithEq, 5) # OK
self.assertEqual(classWithEq, 42.42) # OK
self.assertEqual(classWithEq, 42j) # OK
self.assertEqual(myString, classWithEq) # OK
self.assertEqual("string", classWithEq) # OK
self.assertEqual(myInt, classWithEq) # OK
self.assertEqual(5, classWithEq) # OK
self.assertEqual(42.42, classWithEq) # OK
self.assertEqual(42j, classWithEq) # OK
self.assertEqual('foo'.partition('f'), classWithEq) # OK
self.assertEqual(A(), None) # OK
self.assertEqual(None, A()) # OK
self.assertEqual(A(), A()) # OK
self.assertEqual(set([1, 2]), frozenset([1, 2])) # OK
self.assertEqual({}, collections.OrderedDict()) # OK

def test_other(self):
*a, = 1, 2 # test unpacking argument instead of regular argument
b = 5
b += 1 # test compound assignment
self.assertTrue(5 == 3)
self.assertEqual(5)
self.assertEqual()
self.assertEqual(*a, 5)
self.assertEqual(5, *a)
self.assertEqual("5", b) # Noncompliant
a.assertEqual("string", True)


class AmbiguousSymbolsNoType(unittest.TestCase):
def test_signature_on_class(self):
class ClassWithMultipleDefinitions:
def __init__(self, a):
pass

class CM(type):
def __call__(cls, a):
pass
class ClassWithMultipleDefinitions(metaclass=CM):
def __init__(self, b):
pass

with ...:
class CM(type):
@classmethod
def __call__(cls, a):
return a
class ClassWithMultipleDefinitions(metaclass=CM):
def __init__(self, b):
pass

self.assertEqual(ClassWithMultipleDefinitions(1), 1) # OK, ambiguous type


class MyClass:
...
Expand All @@ -198,8 +15,7 @@ class ComparingTypeAndEnum(unittest.TestCase):
def test_type_of_enum_and_enum_class(self):
EnumType = Enum("EnumType", names=["one", "two"])
enum_member = EnumType(1)
# FP SONARPY-2332: EnumType is a proper "type" object since the Enum constructor with argument creates a new Enum type
self.assertIs(type(enum_member), EnumType) # Noncompliant
self.assertIs(type(enum_member), EnumType)

class DerivedEnumWithMembers(Enum):
ONE = 1
Expand All @@ -208,7 +24,7 @@ class DerivedEnumWithMembers(Enum):
class ComparingTypeAndDerivedEnumWithMembers(unittest.TestCase):
def test_type_of_derived_enum_and_derived_enum_class(self):
derived_enum_member = DerivedEnumWithMembers(1)
self.assertIs(type(enum_member), EnumType)
self.assertIs(type(derived_enum_member), EnumType)

class DerivedEnumWithoutMembers(Enum):
...
Expand All @@ -218,12 +34,12 @@ class ComparingTypeAndDerivedEnumWithoutMembers(unittest.TestCase):
def test_type_of_derived_enum_and_derived_enum_class(self):
DerivedEnumType = DerivedEnumWithoutMembers("DerivedEnumType", names=["one", "two"])
derived_enum_member = DerivedEnumType(1)
# FP SONARPY-2332: DerivedEnumType is a proper "type" object because `DerivedEnumWithoutMembers(str, list)` will create a new enum,
# DerivedEnumType is a proper "type" object because `DerivedEnumWithoutMembers(str, list)` will create a new enum,
# just like `Enum(str, list)` would
self.assertIs(type(derived_enum_member), DerivedEnumType) # Noncompliant
self.assertIs(type(derived_enum_member), DerivedEnumType)

DerivedEnumTypeFromDict = DerivedEnumWithoutMembers("DerivedEnumType", OrderedDict([("one", 1), ("two", 2)]))
derived_enum_member_from_dict = DerivedEnumTypeFromDict(1)
# FP SONARPY-2332: DerivedEnumTypeFromDict is a proper "type" object because `DerivedEnumWithoutMembers(str, list)` will create a new enum,
# DerivedEnumTypeFromDict is a proper "type" object because `DerivedEnumWithoutMembers(str, list)` will create a new enum,
# just like `Enum(str, list)`
self.assertIs(type(derived_enum_member_from_dict), DerivedEnumTypeFromDict) # Noncompliant
self.assertIs(type(derived_enum_member_from_dict), DerivedEnumTypeFromDict)
21 changes: 21 additions & 0 deletions python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@

public class RuntimeType implements InferredType {

// Name of the type returned by the type() function
private static String TYPE_CLASS_NAME = "type";

private ClassSymbol typeClass;
private String builtinFullyQualifiedName;
private Set<String> typeClassSuperClassesFQN = null;
Expand All @@ -52,9 +55,27 @@ public boolean isIdentityComparableWith(InferredType other) {
if (other instanceof UnionType) {
return other.isIdentityComparableWith(this);
}
if (isComparingTypeWithMetaclass(other)) {
return true;
}
return this.equals(other);
}

private boolean isComparingTypeWithMetaclass(InferredType other) {
if (other instanceof RuntimeType otherRuntimeType) {
boolean hasOtherMetaClass = hasMetaclassInHierarchy(otherRuntimeType);
boolean hasThisMetaClass = hasMetaclassInHierarchy(this);
return (getTypeClass().name().equals(TYPE_CLASS_NAME) && hasOtherMetaClass)
|| (hasThisMetaClass && otherRuntimeType.getTypeClass().name().equals(TYPE_CLASS_NAME));
}
return false;
}

private static boolean hasMetaclassInHierarchy(RuntimeType runtimeType) {
return runtimeType.getTypeClass().hasMetaClass()
|| runtimeType.getTypeClass().superClasses().stream().filter(c -> c instanceof ClassSymbol).anyMatch(c -> ((ClassSymbol) c).hasMetaClass());
}

@Override
public boolean canHaveMember(String memberName) {
if (MOCK_FQNS.stream().anyMatch(this::mustBeOrExtend)){
Expand Down
25 changes: 25 additions & 0 deletions python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,31 @@ void isIdentityComparableWith() {
assertThat(aType.isIdentityComparableWith(new DeclaredType(b))).isTrue();
}

@Test
void isIdentityComparableWithMetaclass() {
RuntimeType typeType = new RuntimeType(new ClassSymbolImpl("type", "type"));

ClassSymbolImpl metaclassSymbol = new ClassSymbolImpl("Meta", "Meta");
metaclassSymbol.setHasMetaClass();
RuntimeType metaClassType = new RuntimeType(metaclassSymbol);

ClassSymbolImpl classSymbolWithSuperMetaClass = new ClassSymbolImpl("SuperMeta", "SuperMeta");
classSymbolWithSuperMetaClass.addSuperClass(metaclassSymbol);
RuntimeType superMetaClassType = new RuntimeType(classSymbolWithSuperMetaClass);

assertThat(typeType.isIdentityComparableWith(typeType)).isTrue();
assertThat(typeType.isIdentityComparableWith(metaClassType)).isTrue();
assertThat(typeType.isIdentityComparableWith(superMetaClassType)).isTrue();

assertThat(metaClassType.isIdentityComparableWith(typeType)).isTrue();
assertThat(metaClassType.isIdentityComparableWith(metaClassType)).isTrue();
assertThat(metaClassType.isIdentityComparableWith(superMetaClassType)).isFalse();

assertThat(superMetaClassType.isIdentityComparableWith(typeType)).isTrue();
assertThat(superMetaClassType.isIdentityComparableWith(metaClassType)).isFalse();
assertThat(superMetaClassType.isIdentityComparableWith(superMetaClassType)).isTrue();
}

@Test
void member() {
ClassSymbolImpl x = new ClassSymbolImpl("x", "x");
Expand Down

0 comments on commit 14a8153

Please sign in to comment.