Issue #740: Adding visitors for handling different types along with ClassType in Generic Type invariance check #806

akulk022 · 2023-08-14T19:37:06Z

Description: Only ClassTypes are supported for checking the invariance of the nullability annotations for Generic types and hence TypeVisitors and a TreeVisitor are added to handle different scenarios like for example:
Foo<Foo<@Nullable String>[]> x = new Foo<Foo<String>[]>();
Issue Number: 740
All the tests in NullAwayJSpecifyGenericTests.java have passed for these changes.

Fixes #740

CLAassistant · 2023-08-14T19:37:13Z

All committers have signed the CLA.

msridhar

Looks very promising! These are a couple of quick comments from a very quick initial review.

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

msridhar · 2023-08-16T13:51:43Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+  public class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor<Type, Boolean> {
+    @Override
+    public Type visitArrayType(ArrayTypeTree tree, Boolean p) {
+      ParameterizedTypeTree paramTree = (ParameterizedTypeTree) tree.getType();


Are we sure tree.getType() will always be a ParameterizedTypeTree?

I am not sure, I tried running a test with a regular ArrayType inside the generic like A<String[]> and that ran fine. Maybe its better to just do Type.ClassType classType = (Type.ClassType) tree.getType().accept(new PreservedAnnotationTreeVisitor(), null); return new Type.ArrayType(classType, classType.tsym); instead of assuming?

Can you try a type like A<int[]>? If we can't find a counterexample we can leave this code as is

It gives a ClassCastException in the TYPE_ARG_VISITOR since it cannot cast PrimitiveType to ClassType

Ok then we need a fix somewhere. Please add that as another test case as well

It was a simple fix to not cast it into ClassType on those lines, as far as this piece of code is concerned, I just realised that we do not accept this Visitor unless we have generic type arguments so we never even reach this method in those situations.

Not sure what the latest is here, but let's see what happens after we add the other tests I suggested

msridhar · 2023-08-16T13:53:06Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+      };
+
+  /** Visitor For Handling Different Tree Types */
+  public class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor<Type, Boolean> {


Why is it SimpleTreeVisitor<Type, Boolean> and not SimpleTreeVisitor<Type, Void>? What is the Boolean used for?

It should be Void, I do not recall why it was Boolean but it certainly doesn't make sense now, Thanks for catching that.

Co-authored-by: Manu Sridharan <[email protected]>

…rns true

…022/NullAway into generic_type_issue_#740_ak # Conflicts: # nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

…#740_ak # Conflicts: # nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

…s static

msridhar

Here are some more detailed comments

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

msridhar · 2023-08-18T14:54:51Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+      List<Type> genericArgType = currentTypeArgType.accept(TYPE_ARG_VISITOR, null);
+      if (genericArgType.size() > 0) {
+        currentTypeArgType = curTypeArg.accept(new PreservedAnnotationTreeVisitor(state), null);
      }


I think this logic is checking, if there type arguments nested at any depth within currentTypeArgType, then we need to recurse and preserve type annotations there. It makes sense at a high level, but I'm not sure this is really an optimization (since we are adding another traversal of the type), and I'm not sure the logic in TYPE_ARG_VISITOR is correct (it doesn't seem to recursively apply the visitor). For now, how about we remove this optimization, and just unconditionally always do currentTypeArgType = curTypeArg.accept(new PreservedAnnotationTreeVisitor(state), null);?

the purpose of TYPE_ARG_VISITOR is just to be able to get the type arguments even if the Argument is not a ClassType, but the if condition checking the size is greater than 0 was already there as given below, do you want me to remove that logic?

if (currentTypeArgType.getTypeArguments().size() > 0) { // nested generic type; recursively preserve its nullability type argument annotations currentTypeArgType = typeWithPreservedAnnotations((ParameterizedTypeTree) curTypeArg, state); }

You mentioned you get an NPE without this check. What is the stack trace for that?

One of the test cases that failed is: A<A<@Nullable String>[]> var = new A<A<String>[]>();, when we traverse the ParametrizedTypeTree on the RHS (A<A[]>), when we get to the String, since we don't check if there are further nested arguments and accept the visitor, we get a null for currentTypeArgType, so we would certainly need some type of a check to make sure we don't run the logic again if we don't have further generic types.

Stack Trace:
java.lang.NullPointerException: Cannot invoke "com.sun.tools.javac.code.Type.cloneWithMetadata(com.sun.tools.javac.code.TypeMetadata)" because "currentTypeArgType" is null
at com.uber.nullaway.GenericsChecks$PreservedAnnotationTreeVisitor.visitParameterizedType(GenericsChecks.java:654)
at com.uber.nullaway.GenericsChecks$PreservedAnnotationTreeVisitor.visitParameterizedType(GenericsChecks.java:595)

I fixed this by overriding the defaultAction method in b9eb8fc

That is a good idea, Thanks!

msridhar · 2023-08-18T14:55:26Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

@@ -577,6 +529,182 @@ public static void compareGenericTypeParameterNullabilityForCall(
    }
  }

+  /** To dispatch the logic to obtain the generic type arguments for different Types */


Assuming we agree with my suggestion above, this visitor can be deleted

msridhar · 2023-08-18T14:55:43Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+        }
+      };
+
+  /** To dispatch the logic to0 obtain the generic type arguments for different Types */


This comment seems wrong

Yes it is wrong, it should be fixed in the newer commits, Thanks!

msridhar · 2023-08-18T14:55:55Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+      // The base type of rhsType may be a subtype of lhsType's base type.  In such cases, we
+      // must
+      // compare lhsType against the supertype of rhsType with a matching base type.


Nit: formatting

msridhar · 2023-08-18T14:56:18Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+      // This is impossible, considering the fact that standard Java subtyping succeeds before
+      // running
+      // NullAway


Nit: formatting

msridhar · 2023-08-18T14:57:20Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+        List<Type> genericArgs = lhsTypeArgument.accept(TYPE_ARG_VISITOR, null);
+        if (genericArgs.size() > 0) {


Again let's get rid of this optimization and just unconditionally do a recursive traversal

msridhar · 2023-08-18T14:57:41Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+    }
+  }
+
+  /** Visitor For Handling Different Tree Types */


We need a more descriptive comment

I agree, I have added a more descriptive comment

msridhar · 2023-08-18T14:58:13Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+  public class PreservedAnnotationTreeVisitor extends SimpleTreeVisitor<Type, Boolean> {
+    @Override
+    public Type visitArrayType(ArrayTypeTree tree, Boolean p) {
+      ParameterizedTypeTree paramTree = (ParameterizedTypeTree) tree.getType();


Not sure what the latest is here, but let's see what happens after we add the other tests I suggested

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

…n certain comments

… generic type arguments before recursing further

msridhar

very minor

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

Co-authored-by: Manu Sridharan <[email protected]>

msridhar

I think this is ready to land! Will check if @lazaroclapp wants to take a look

lazaroclapp

Overall looks good to me. One suggestion regarding test cases and a possible follow-up refactoring option, but otherwise this looks fine by me :)

lazaroclapp · 2023-08-29T02:49:07Z

nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

+            "  static class A<T extends @Nullable Object> { }",
+            "  static void testPositive() {",
+            "    // BUG: Diagnostic contains: Cannot assign from type",
+            "    A<A<@Nullable String>[]> var = new A<A<String>[]>();",


How about adding the:

A<A<String>[]> var = new A<A<@Nullable String>[]>();

and

A<A<String>[]> var = new A<A<String>[]>();

cases?

lazaroclapp · 2023-08-29T02:53:00Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

+   * Nullability annotations for the nested generic type arguments. Compares the Type it is called
+   * upon, i.e. the LHS type and the Type passed as an argument, i.e. The RHS type.
+   */
+  public static class CompareNullabilityVisitor extends Types.DefaultTypeVisitor<Boolean, Type> {


Fine with these being inner classes for now, but a good follow up PR would be a simple refactor of this into a com.uber.nullaway.generics package with three top level classes (preferably in its own PR without any other changes, so it's easier to review).

Created #817, agreed this is good for a follow up

msridhar · 2023-08-29T22:11:15Z

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

@@ -908,6 +964,7 @@ public String visitCapturedType(Type.CapturedType t, Void s) {

        @Override
        public String visitArrayType(Type.ArrayType t, Void unused) {
+          // TODO properly print cases like int @Nullable[]


We should also fix printing of these types in a follow-up

lazaroclapp

LGTM! 🚀

akulk022 added 4 commits June 25, 2023 05:29

adding failing test case for issue 740

a0b76ea

adding visitors for handling ArrayTypes

b2c533a

clean-up of redundant code in visitors

6ae742b

fixing indentation

15e1601

msridhar marked this pull request as draft August 14, 2023 19:38

msridhar and others added 4 commits August 14, 2023 14:30

Merge branch 'master' into generic_type_issue_#740_ak

9fae10c

updating visitor methods returning null

47dd926

making default Type method Nullable in visitor

f5b6c1d

formatting

e9d969b

msridhar reviewed Aug 16, 2023

View reviewed changes

akulk022 and others added 7 commits August 16, 2023 20:12

changing TreeVisitor second argument type to Void since it is unused

20d747a

Update default return type for Compare Nullability Visitor

0d3c2f0

Co-authored-by: Manu Sridharan <[email protected]>

removing cast from TYPE_ARG_VISITOR

4273601

removing unnecessary @nullable annotation for method that always retu…

9e96103

…rns true

Merge branch 'generic_type_issue_#740_ak' of https://github.com/akulk…

d1e61bf

…022/NullAway into generic_type_issue_#740_ak # Conflicts: # nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java

Merge remote-tracking branch 'origin/master' into generic_type_issue_…

d2db3cc

…#740_ak # Conflicts: # nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

After merging with changes made by PR#805 making GenericChecks method…

d7c9812

…s static

msridhar reviewed Aug 18, 2023

View reviewed changes

akulk022 and others added 9 commits August 23, 2023 21:56

Adding test cases

aed09fd

Merge branch 'master' into generic_type_issue_#740_ak

1a12cef

Merge branch 'master' into generic_type_issue_#740_ak

a4bd9bc

Removing redundant code from TypeWithPreservedAnnotations

680a17c

Adding more descriptive comments for visitors and fixing formatting o…

d314952

…n certain comments

fix NPE issue

b9eb8fc

fix other test failure

989a755

copy type symbol for array type from original

4e5adf0

removing TYPE_ARG_VISITOR and the check to see if we have more nested…

657f38e

… generic type arguments before recursing further

msridhar reviewed Aug 28, 2023

View reviewed changes

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java Outdated Show resolved Hide resolved

nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java Outdated Show resolved Hide resolved

Update nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java

6556379

Co-authored-by: Manu Sridharan <[email protected]>

using the existing visitor instead of allocating a new one

ce51560

Co-authored-by: Manu Sridharan <[email protected]>

msridhar marked this pull request as ready for review August 28, 2023 15:31

msridhar added 3 commits August 28, 2023 08:34

fix position of annotations

2e14f16

add todo

6f5c722

restore

b419103

msridhar approved these changes Aug 28, 2023

View reviewed changes

lazaroclapp reviewed Aug 29, 2023

View reviewed changes

more tests

394ff52

msridhar mentioned this pull request Aug 29, 2023

Create a new com.uber.nullaway.generics package with generics-checking-related code #817

Closed

msridhar reviewed Aug 29, 2023

View reviewed changes

msridhar mentioned this pull request Aug 29, 2023

Fix printing of array types in generics error messages #818

Open

msridhar requested a review from lazaroclapp August 29, 2023 22:12

Merge branch 'master' into generic_type_issue_#740_ak

1438bbd

lazaroclapp approved these changes Aug 30, 2023

View reviewed changes

msridhar merged commit 0431a90 into uber:master Aug 30, 2023
7 checks passed

		List<Type> genericArgs = lhsTypeArgument.accept(TYPE_ARG_VISITOR, null);
		if (genericArgs.size() > 0) {

Issue #740: Adding visitors for handling different types along with ClassType in Generic Type invariance check #806

Issue #740: Adding visitors for handling different types along with ClassType in Generic Type invariance check #806

Conversation

akulk022 commented Aug 14, 2023 • edited by msridhar Loading

CLAassistant commented Aug 14, 2023 • edited Loading

msridhar 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

akulk022 Aug 16, 2023 • edited Loading

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

msridhar 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

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

msridhar left a comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar Aug 29, 2023 • edited Loading

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

lazaroclapp left a comment

Choose a reason for hiding this comment

akulk022 commented Aug 14, 2023 •

edited by msridhar

Loading

CLAassistant commented Aug 14, 2023 •

edited

Loading

akulk022 Aug 16, 2023 •

edited

Loading

msridhar Aug 29, 2023 •

edited

Loading