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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Nov 22, 2024
Merged

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

merged 2 commits into from
Nov 22, 2024

Conversation

thomas-serre-sonarsource
Copy link
Contributor

@thomas-serre-sonarsource thomas-serre-sonarsource commented Nov 20, 2024 •
edited by jira bot
Loading

SONARPY-2332

Part of

thomas-serre-sonarsource
thomas-serre-sonarsource commented Nov 22, 2024
View reviewed changes
python-checks/src/test/resources/checks/tests/assertOnDissimilarTypes.py
@@ -208,7 +207,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.

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 22, 2024
View reviewed changes
python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java Outdated
@@ -31,6 +31,9 @@

public class RuntimeType implements InferredType {

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

Choose a reason for hiding this comment

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

Did you try to use org.sonar.python.types.InferredTypes#TYPE?
It looks like that for the v1 type inference we have this constant representing the type class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I ll use it. thanks

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 22, 2024
View reviewed changes
python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java Outdated

private static boolean hasMetaclassInHierarchy(RuntimeType runtimeType) {
return runtimeType.getTypeClass().hasMetaClass()
|| runtimeType.getTypeClass().superClasses().stream().filter(ClassSymbol.class::isInstance).anyMatch(c -> ((ClassSymbol) c).hasMetaClass());
Copy link
Contributor

Choose a reason for hiding this comment

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

should we look only for current type parents for a metaclass, or should we go deeper in the types hierarchy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intend to go deeper as the original issue contained a child class from enum.Enum

Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed - we need to check whole hierarchy, not nearest parents

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 22, 2024
View reviewed changes
python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java Outdated
if (other instanceof RuntimeType otherRuntimeType) {
boolean hasOtherMetaClass = hasMetaclassInHierarchy(otherRuntimeType);
boolean hasThisMetaClass = hasMetaclassInHierarchy(this);
return (TYPE_CLASS_NAME.equals(getTypeClass().name()) && hasOtherMetaClass)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there could be just a comparison with the org.sonar.python.types.InferredTypes#TYPE constant.

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 22, 2024
View reviewed changes
python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java Outdated
@@ -52,9 +55,27 @@ public boolean isIdentityComparableWith(InferredType other) {
if (other instanceof UnionType) {
return other.isIdentityComparableWith(this);
}
if (isComparingTypeWithMetaclass(other)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be like this? maybe it will be cleaner to return:

return isComparingTypeWithMetaclass(other) || this.equals(other);

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 22, 2024
View reviewed changes
python-frontend/src/test/java/org/sonar/python/types/RuntimeTypeTest.java Outdated
@@ -64,6 +64,34 @@ void isIdentityComparableWith() {
assertThat(aType.isIdentityComparableWith(new DeclaredType(b))).isTrue();
}

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

Choose a reason for hiding this comment

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

here you can use org.sonar.python.types.InferredTypes#TYPE

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource requested changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, I left some comments, seems that the implementation could be simplified

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource reviewed Nov 22, 2024
View reviewed changes
python-frontend/src/main/java/org/sonar/python/types/RuntimeType.java
return isComparingTypeWithMetaclass(other) || this.equals(other);
}

private boolean isComparingTypeWithMetaclass(InferredType other) {
Copy link
Contributor

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;
  }

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource requested changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

After a discussion some changes requested

@sonarqube-next SonarQube-Next
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

maksim-grebeniuk-sonarsource
maksim-grebeniuk-sonarsource approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for the fixes

@thomas-serre-sonarsource thomas-serre-sonarsource merged commit 3be15cc into master Nov 22, 2024
12 checks passed
@thomas-serre-sonarsource thomas-serre-sonarsource deleted the ts/SONARPY-2332 branch November 22, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource maksim-grebeniuk-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomas-serre-sonarsource @maksim-grebeniuk-sonarsource