Enforce correct type-use annotation locations for nested types #1045

armughan11 · 2024-09-25T07:09:51Z

Enforce correct type-use annotation locations for nested types as per JSpecify norms. We enforce type-use annotations to be on the inner class and raise an error if they are not. For annotations that are both type use and declaration, we raise an error at an invalid location.

Current behavior

// All three are ok

@Nullable A.B.C foo1 = null;
A.@Nullable B.C foo2 = null;
A.B.@Nullable C foo3 = null;

New behavior

// BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class
@Nullable A.B.C foo1 = null;
// BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class
A.@Nullable B.C foo2 = null;
A.B.@Nullable C foo3 = null;

For annotations which are both declaration and annotation and type-use, only foo2 throws an error since the location isn't apt for either scenarios

codecov · 2024-09-25T07:23:19Z

Codecov Report

Attention: Patch coverage is 85.48387% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.61%. Comparing base (9eea2be) to head (cddd77b).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...away/src/main/java/com/uber/nullaway/NullAway.java	80.95%	3 Missing and 5 partials ⚠️
...c/main/java/com/uber/nullaway/NullabilityUtil.java	94.73%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1045      +/-   ##
============================================
- Coverage     87.62%   87.61%   -0.02%     
- Complexity     2160     2183      +23     
============================================
  Files            85       85              
  Lines          7103     7161      +58     
  Branches       1386     1404      +18     
============================================
+ Hits           6224     6274      +50     
- Misses          450      453       +3     
- Partials        429      434       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msridhar

This is a great start! I think we are missing checking on various occurrences of annotated types, like method return types. We should be doing checking in all the places that this checker does it. Can we add that, and also tests?

msridhar · 2024-09-26T21:43:28Z

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

+          if (hasNestedClass(state.getTypes(), symbol.type)) {
+            errorMessage =
+                new ErrorMessage(
+                    MessageTypes.NULLABLE_ON_WRONG_NESTED_CLASS_LEVEL,
+                    "Type-use nullability annotations should be applied on inner class");


I can't understand this check and why it occurs here? I think the check should occur independent of whether the initializer assignment is valid. If we need to possibly report multiple error messages from this method, we can use the state.reportMatch API to report the errors and then just always return Description.NO_MATCH.

Changed the logic here to adhere to the ErrorProne flow.

msridhar · 2024-09-26T21:44:04Z

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

+    if (types == null || type == null) {
+      return false;
+    }


If these null checks are necessary the parameters should be @Nullable. But I don't think they are needed?

msridhar

Looking really promising! I still need to check the tests more carefully, but here are some comments on the code

msridhar · 2024-10-07T20:36:02Z

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

+        || TYPE_USE_OR_TYPE_PARAMETER.containsAll(elementTypes));
+  }
+
+  private void handleNullabilityOnNestedClass(


This method should probably be renamed and it needs Javadoc. For renaming, how about:

Suggested change

private void handleNullabilityOnNestedClass(

private void checkNullableAnnotationPositionInType(

msridhar · 2024-10-07T20:39:09Z

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

+      Tree typeTree,
+      Tree errorReportingTree,
+      VisitorState state) {
+    if (!(typeTree instanceof JCTree.JCFieldAccess)) {


The public version of JCTree.JCFieldAccess is com.sun.source.tree.MemberSelectTree; can we just use that here?

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

msridhar · 2024-10-07T20:40:50Z

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

@@ -1487,6 +1509,70 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
    return Description.NO_MATCH;
  }

+  private static boolean isOnlyTypeAnnotation(Symbol anno) {


Rename?

Suggested change

private static boolean isOnlyTypeAnnotation(Symbol anno) {

private static boolean isTypeUseAnnotation(Symbol anno) {

I think this will return true for annotations that are both type use and declaration.

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

msridhar · 2024-10-07T20:44:45Z

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

+      return true;
+    }
+    ImmutableSet<ElementType> elementTypes = ImmutableSet.copyOf(target.value());
+    // Return true only if annotation is not type-use only


Maybe:

Suggested change

// Return true only if annotation is not type-use only

// Return true for any annotation that is not exclusively a type-use annotation

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

msridhar · 2024-10-07T20:50:05Z

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

@@ -340,6 +343,9 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi
    // In JSpecify mode and without the LegacyAnnotationLocations flag, annotations on array
    // dimensions are *not* treated as applying to the top-level type, consistent with the JSpecify
    // spec.
+    // Outside of JSpecify mode, annotations which are *not* on the inner type are not treated as


Do we mean "Outside of JSpecify mode", or independent of JSpecify mode? If it's true no matter what, we can just delete "Outside of JSpecify mode". Or maybe the part about outside JSpecify mode just relates to the use of the legacy annotations mode?

That's correct. Since we had earlier blocked using JSpecify mode and Legacy annotations together, this probably is a bit confusing. I'll remove it for the sake of it brevity

msridhar · 2024-10-07T20:52:26Z

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

+    if (!hasNestedClass(symbol.type)) {
+      if (innerTypeCount > 0) {
+        return true;
+      }
+    }
+    return innerTypeCount == nestingDepth - 1;


need a comment explaining this logic

msridhar · 2024-10-08T20:37:26Z

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

+            "    // @Nullable does not apply to the inner type",
+            "    // BUG: Diagnostic contains: @NonNull field f2 not initialized",


Can we fix up indentation in all the tests for readability?

msridhar

couple more comments on the tests

msridhar · 2024-10-08T20:39:22Z

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

+            "  // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
+            "    @Nullable A.B foo4 = null;",
+            "  // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field",
+            "  A.B.@Nullable C [][] foo5 = null;",


If we are handling multi-dim arrays can we add a test where the field is actually @Nullable? I think you need to write A.B.C @Nullable [][]

msridhar · 2024-10-08T20:40:58Z

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

+            "  // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
+            "  public @Nullable A.B method4() { return null; }",
+            "  public @Nullable A method5() { return null; }",
+            "  public @Nullable int method6() { return 0; }",


remove this one? eventually this should be an error (but not for this PR). @Nullable makes no sense on a primitive type

msridhar

one question

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

msridhar · 2024-10-09T20:51:13Z

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

+   * @param state The visitor state
+   */
+  private void checkNullableAnnotationPositionInType(
+      List<? extends AnnotationTree> annotations, Tree type, Tree tree, VisitorState state) {


I don't understand why we need the separate tree parameter. Why just not report errors directly on the type tree? Won't that be better?

…into type-use-location-nested-class

msridhar

Looks great now, thanks a lot!

msridhar linked an issue Sep 26, 2024 that may be closed by this pull request

Enforce correct type-use annotation locations for nested types #1026

Closed

msridhar reviewed Sep 26, 2024

View reviewed changes

fix comment

213bc44

armughan11 force-pushed the type-use-location-nested-class branch from 61b0cdc to 213bc44 Compare October 7, 2024 10:45

Merge branch 'master' into type-use-location-nested-class

cd9acfb

armughan11 requested a review from msridhar October 7, 2024 10:51

armughan11 and others added 6 commits October 7, 2024 16:26

add declaration and type use check comment

41a0371

fix nullability of new methods

a08ae75

Merge branch 'master' into type-use-location-nested-class

1364c2b

rename variables

d1d534d

reflow comment

d1203af

tweak comment

49bb92a

msridhar reviewed Oct 7, 2024

View reviewed changes

msridhar reviewed Oct 8, 2024

View reviewed changes

fix review comments

b1e3bc1

armughan11 requested a review from msridhar October 9, 2024 09:28

armughan11 and others added 4 commits October 9, 2024 14:58

Merge branch 'master' into type-use-location-nested-class

0e9bbbe

docs

3ed138f

tweaks

d705317

tweak comment

18dd6b9

msridhar reviewed Oct 9, 2024

View reviewed changes

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java Show resolved Hide resolved

remove unnecessary local

5ded427

msridhar reviewed Oct 9, 2024

View reviewed changes

armughan11 added 2 commits October 10, 2024 02:46

fix review comments

9aadf2b

Merge remote-tracking branch 'origin/type-use-location-nested-class' …

da3f7f0

…into type-use-location-nested-class

armughan11 requested a review from msridhar October 9, 2024 21:18

msridhar added 2 commits October 9, 2024 15:32

rename method

82f3813

minor tweaks to tests

cddd77b

msridhar approved these changes Oct 9, 2024

View reviewed changes

msridhar enabled auto-merge (squash) October 9, 2024 22:35

msridhar merged commit cc5ef65 into uber:master Oct 9, 2024
9 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce correct type-use annotation locations for nested types #1045

Enforce correct type-use annotation locations for nested types #1045

armughan11 commented Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

msridhar left a comment

msridhar Sep 26, 2024

armughan11 Oct 7, 2024

msridhar Sep 26, 2024

msridhar left a comment

msridhar Oct 7, 2024

msridhar Oct 7, 2024

msridhar Oct 7, 2024

msridhar Oct 7, 2024

msridhar Oct 7, 2024

armughan11 Oct 9, 2024

msridhar Oct 7, 2024

msridhar Oct 8, 2024

msridhar left a comment

msridhar Oct 8, 2024

msridhar Oct 8, 2024

msridhar left a comment

msridhar Oct 9, 2024

armughan11 Oct 9, 2024

msridhar left a comment

	private void handleNullabilityOnNestedClass(
	private void checkNullableAnnotationPositionInType(

	private static boolean isOnlyTypeAnnotation(Symbol anno) {
	private static boolean isTypeUseAnnotation(Symbol anno) {

	// Return true only if annotation is not type-use only
	// Return true for any annotation that is not exclusively a type-use annotation

		" // @Nullable does not apply to the inner type",
		" // BUG: Diagnostic contains: @NonNull field f2 not initialized",

Enforce correct type-use annotation locations for nested types #1045

Enforce correct type-use annotation locations for nested types #1045

Conversation

armughan11 commented Sep 25, 2024

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

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

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

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

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

msridhar left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2024 •

edited

Loading