Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SONARPY-2332 Fix FP on S5845 when comparing enum objects with types #2170

Merged
merged 2 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ 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
# EnumType is a proper "type" object since the Enum constructor with argument creates a new Enum type
self.assertIs(type(enum_member), EnumType)

class DerivedEnumWithMembers(Enum):
ONE = 1
Expand All @@ -208,7 +208,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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistake that was present in the previous ticket. I corrected it here.


class DerivedEnumWithoutMembers(Enum):
...
Expand All @@ -218,12 +218,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)
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,21 @@ public boolean isIdentityComparableWith(InferredType other) {
if (other instanceof UnionType) {
return other.isIdentityComparableWith(this);
}
return this.equals(other);
return isComparingTypeWithMetaclass(other) || this.equals(other);
}

private boolean isComparingTypeWithMetaclass(InferredType other) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, could be simplified like this :

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

if (other instanceof RuntimeType otherRuntimeType) {
boolean hasOtherMetaClass = hasMetaclassInHierarchy(otherRuntimeType.getTypeClass());
boolean hasThisMetaClass = hasMetaclassInHierarchy(getTypeClass());
return (InferredTypes.TYPE.equals(this) && hasOtherMetaClass)
|| (hasThisMetaClass && InferredTypes.TYPE.equals(otherRuntimeType));
}
return false;
}

private static boolean hasMetaclassInHierarchy(ClassSymbol classSymbol) {
return classSymbol.hasMetaClass() || classSymbol.superClasses().stream().filter(ClassSymbol.class::isInstance).anyMatch(c -> hasMetaclassInHierarchy((ClassSymbol) c));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,32 @@ void isIdentityComparableWith() {
assertThat(aType.isIdentityComparableWith(new DeclaredType(b))).isTrue();
}

@Test
void isIdentityComparableWithMetaclass() {
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);

UnknownClassType unknownClassType = new UnknownClassType(metaclassSymbol);

assertThat(InferredTypes.TYPE.isIdentityComparableWith(InferredTypes.TYPE)).isTrue();
assertThat(InferredTypes.TYPE.isIdentityComparableWith(metaClassType)).isTrue();
assertThat(InferredTypes.TYPE.isIdentityComparableWith(superMetaClassType)).isTrue();
assertThat(InferredTypes.TYPE.isIdentityComparableWith(unknownClassType)).isFalse();

assertThat(metaClassType.isIdentityComparableWith(InferredTypes.TYPE)).isTrue();
assertThat(metaClassType.isIdentityComparableWith(metaClassType)).isTrue();
assertThat(metaClassType.isIdentityComparableWith(superMetaClassType)).isFalse();

assertThat(superMetaClassType.isIdentityComparableWith(InferredTypes.TYPE)).isTrue();
assertThat(superMetaClassType.isIdentityComparableWith(metaClassType)).isFalse();
assertThat(superMetaClassType.isIdentityComparableWith(superMetaClassType)).isTrue();
}

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