SONARPY-2202: Migrate S5727 ComparisonToNoneCheck to the new type model #2085

Seppli11 · 2024-10-18T13:35:59Z

No description provided.

Seppli11 · 2024-10-18T14:01:10Z

python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java


+    @Override
+    public TriBool test(PythonType otherMaybeWrappedType) {


I'm not entirely sure that the behaviour of this method and InferredType.isIdentityComparableWith is the same.

My understanding of this check is that it checks if two types are the same, or, if one is a union, one of its candidates is the same as the other type. Additionally, if one of the two types is unknown, the result is always TriBool.UNKNOWN.

Seppli11 · 2024-10-23T08:43:15Z

python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeCheckerBuilderTest.java

was moved to types/v2

Seppli11 · 2024-10-23T09:35:04Z

python-frontend/src/main/java/org/sonar/python/types/v2/TypeUtils.java

+    return Set.of(type);
+  }
+
+  public static PythonType unwrapType(PythonType type) {


I think a decent argument can be made that UnionType#unwrappedType() should have this behavior. I'm not sure if there are unwanted side-effects from this and aired on the side of caution,

The disabled test dependent on SONARPY-2241

sonarqube-next · 2024-10-23T13:28:48Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
2 Accepted issues

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

See analysis details on SonarQube

ghislainpiot

The utility methods seem to be handling a lot of edge cases that should never happen in the first place.
I think it would be good to create tickets to investigate (and eliminate) the creation of those impossible types.

I didn't review the TypeChecker changes as discussed, I don't think handling the identify case is worth the added complexity on the main predicate.

ghislainpiot · 2024-10-24T09:07:33Z

python-frontend/src/main/java/org/sonar/python/types/v2/TypeUtils.java

+   * @return a set of all possible effective types. However, {@link ObjectType}s are not unwrapped, as they are used to represent an
+   * instance of a type, rather than a type itself, and this should be done explicitly.
+   */
+  public static Set<PythonType> getNestedEffectiveTypes(PythonType type) {


I am not sure about the naming of the method, maybe something like flattenedTypes would be clearer ?

ghislainpiot · 2024-10-24T09:24:12Z

python-frontend/src/test/java/org/sonar/python/types/v2/TypeUtilsTest.java

+    var nestedUnions = UnionType.or(
+      intObjType,
+      UnionType.or(
+        floatObjType,
+        NONE_TYPE
+      )
+    );
+
+    assertThat(TypeUtils.unwrapType(nestedUnions))
+      .isEqualTo(
+        UnionType.or(INT_TYPE, UnionType.or(FLOAT_TYPE, NONE_TYPE))
+      );
+  }


UnionTypes cannot be nested, and the variable does not have any nesting.

ghislainpiot · 2024-10-24T09:33:00Z

python-frontend/src/test/java/org/sonar/python/types/v2/TypeUtilsTest.java

+    assertThat(TypeUtils.getNestedEffectiveTypes(INT_TYPE)).containsExactlyInAnyOrder(INT_TYPE);
+
+    assertThat(TypeUtils.getNestedEffectiveTypes(UnionType.or(INT_TYPE, FLOAT_TYPE))).containsExactlyInAnyOrder(INT_TYPE, FLOAT_TYPE);
+    assertThat(TypeUtils.getNestedEffectiveTypes(new UnionType(Set.of(INT_TYPE, UnionType.or(FLOAT_TYPE, BOOL_TYPE))))).containsExactlyInAnyOrder(INT_TYPE, FLOAT_TYPE, BOOL_TYPE);


This is also an UnionType that should not be able to exist

ghislainpiot · 2024-10-24T09:42:17Z

python-frontend/src/test/java/org/sonar/python/types/v2/TypeUtilsTest.java

+    assertThat(TypeUtils.getNestedEffectiveTypes(
+      new UnionType(Set.of(
+        PROJECT_LEVEL_TYPE_TABLE.lazyTypesContext().getOrCreateLazyType("int"),
+        new LazyUnionType(Set.of(FLOAT_TYPE, BOOL_TYPE))


This also probably shouldn't exist, the creation of the UnionType should resolve its candidates

ghislainpiot · 2024-10-24T09:43:46Z

python-frontend/src/test/java/org/sonar/python/types/v2/TypeUtilsTest.java

+    var floatObjType = new ObjectType(FLOAT_TYPE);
+    assertThat(TypeUtils.unwrapType(INT_TYPE)).isEqualTo(INT_TYPE);
+    assertThat(TypeUtils.unwrapType(intObjType)).isEqualTo(INT_TYPE);
+    assertThat(TypeUtils.unwrapType(new ObjectType(new ObjectType(INT_TYPE)))).isEqualTo(INT_TYPE);


Is there an example of this happening ? If yes, we should investigate if that is correct

ghislainpiot · 2024-10-24T10:02:12Z

python-frontend/src/main/java/org/sonar/python/types/v2/TypeCheckBuilder.java

+      if (type2 instanceof ResolvableType resolvableType) {
+        type2 = resolvableType.resolve();
+      }
+      if (type2 instanceof ObjectType objectType && typeStrictness == TypeStrictness.NON_STRICT) {


Is this the same as SONARPY-2229 ?

ghislainpiot · 2024-10-24T10:27:40Z

python-checks/src/main/java/org/sonar/python/checks/ComparisonToNoneCheck.java

+  }
+
+  private boolean isNone(PythonType type) {
+    return isNoneTypeCheck.check(TypeUtils.unwrapType(type)) == TriBool.TRUE;


Is the unwrapType needed ?

Seppli11 force-pushed the SONARPY-2202 branch from 4d37b04 to 3d389b0 Compare October 18, 2024 13:41

Seppli11 commented Oct 18, 2024

View reviewed changes

Seppli11 force-pushed the SONARPY-2202 branch 3 times, most recently from 4e9ea82 to 49c3d6c Compare October 18, 2024 14:18

Seppli11 marked this pull request as ready for review October 18, 2024 14:20

Seppli11 force-pushed the SONARPY-2202 branch 4 times, most recently from 1d7a71b to 601d05d Compare October 21, 2024 12:35

Seppli11 marked this pull request as draft October 22, 2024 07:12

Seppli11 force-pushed the SONARPY-2202 branch 5 times, most recently from f2e4d76 to 8bcf3f3 Compare October 23, 2024 07:57

Seppli11 commented Oct 23, 2024

View reviewed changes

python-frontend/src/test/java/org/sonar/python/semantic/v2/TypeCheckerBuilderTest.java Outdated

Copy link

Contributor Author

Seppli11 Oct 23, 2024

Choose a reason for hiding this comment

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

was moved to types/v2

Seppli11 force-pushed the SONARPY-2202 branch 5 times, most recently from bc18ff4 to 494711d Compare October 23, 2024 09:31

Seppli11 commented Oct 23, 2024

View reviewed changes

Seppli11 marked this pull request as ready for review October 23, 2024 09:40

Seppli11 requested a review from ghislainpiot October 23, 2024 09:46

Seppli11 force-pushed the SONARPY-2202 branch 2 times, most recently from c77f94e to 35eb809 Compare October 23, 2024 12:36

Seppli11 added 3 commits October 23, 2024 15:01

add util functions in TriBool and TypeUtils

36d529b

add TypeChecker functions and move tests to correct package

2bf29e8

SONARPY-2202 Migrate S5727 ComparisonToNoneCheck to the new type model

62d8821

update ruling and add disabled test

cea811f

The disabled test dependent on SONARPY-2241

Seppli11 force-pushed the SONARPY-2202 branch from 35eb809 to cea811f Compare October 23, 2024 13:17

SONARPY-2202 fix renaming error

9fd7c68

ghislainpiot requested changes Oct 24, 2024

View reviewed changes

Seppli11 closed this Nov 14, 2024

Seppli11 deleted the SONARPY-2202 branch December 10, 2024 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARPY-2202: Migrate S5727 ComparisonToNoneCheck to the new type model #2085

SONARPY-2202: Migrate S5727 ComparisonToNoneCheck to the new type model #2085

Seppli11 commented Oct 18, 2024

Seppli11 Oct 18, 2024

Seppli11 Oct 23, 2024

Seppli11 Oct 23, 2024

sonarqube-next bot commented Oct 23, 2024

ghislainpiot left a comment

ghislainpiot Oct 24, 2024

ghislainpiot Oct 24, 2024

ghislainpiot Oct 24, 2024

ghislainpiot Oct 24, 2024

ghislainpiot Oct 24, 2024

ghislainpiot Oct 24, 2024

ghislainpiot Oct 24, 2024


		@Override
		public TriBool test(PythonType otherMaybeWrappedType) {

SONARPY-2202: Migrate S5727 ComparisonToNoneCheck to the new type model #2085

SONARPY-2202: Migrate S5727 ComparisonToNoneCheck to the new type model #2085

Conversation

Seppli11 commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqube-next bot commented Oct 23, 2024

Quality Gate passed

ghislainpiot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment