Enforce Strict Interpretation Of Type Use Annotation Locations Outside of JSpecify mode #1010

armughan11 · 2024-07-29T03:31:26Z

Currently, outside of JSpecify, both top-level and element annotations are treated the same, i.e., considered as top-level annotations. This changes the default behavior by ignoring annotations on elements and adds a flag to revert back to legacy behavior.

Example:

@Nullable Object[] foo1 = null;
Object @Nullable[] foo2 = null;

Current Behavior:
Both assignments are fine

New Behavior:

The first assignment raises an error since annotations on elements are ignored altogether and NOT treated as top-level annotations. Fixes #1007

msridhar · 2024-07-29T15:26:31Z

Looks promising! But I don't see any checks in the code as to whether the relevant annotation is type use or declaration. Is that not needed?

armughan11 · 2024-07-29T15:34:33Z

@msridhar declaration annotation seems to work as expected, added a test for that.

I am not sure but i think we already filter it out here so declaration annotations don't call isDirecTypeUseAnnotation? So we should be fine I guess?

codecov · 2024-07-29T15:40:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.00%. Comparing base (a863221) to head (1d6c0dd).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1010      +/-   ##
============================================
- Coverage     86.00%   86.00%   -0.01%     
- Complexity     2087     2091       +4     
============================================
  Files            83       83              
  Lines          6903     6908       +5     
  Branches       1330     1331       +1     
============================================
+ Hits           5937     5941       +4     
  Misses          551      551              
- Partials        415      416       +1

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

msridhar · 2024-08-04T16:18:31Z

Thanks for this @armughan11! Related to #708 (comment), I wanted to document the test scenarios we want to go through here.

Annotation Type	Annotation Position	Mode	`@Nullable` array ref	`@Nullable` elements
declaration	-	standard	✅	❌
declaration	-	legacy	✅	❌
declaration	-	JSpecify	✅	❌
type use	top-level	standard	✅	❌
type use	top-level	legacy	✅	❌
type use	top-level	JSpecify	✅	❌
type use	elements	standard	❌	❌
type use	elements	legacy	✅	❌
type use	elements	JSpecify	❌	✅
type use	both	standard	✅	❌
type use	both	legacy	✅	❌
type use	both	JSpecify	✅	✅
both	top-level	standard	✅	❌
both	top-level	legacy	✅	❌
both	top-level	JSpecify	✅	❌
both	elements	standard	✅	❌
both	elements	legacy	✅	❌
both	elements	JSpecify	✅	❌
both	both	standard	✅	❌
both	both	legacy	✅	❌
both	both	JSpecify	✅	✅

I wrote down what I expect the nullability behavior to be in each case. For those cases with ❓ I think we should just preserve current behavior but I'm not 100% sure what that is.

@armughan11 could you go through this table and make sure we have coverage of these cases?

msridhar

Looking great! I have a few comments.

Also, I think we are missing some tests of the nullability of array elements in JSpecify mode. E.g., we need to test if we have a "both" annotation for the top level array reference (Object @Nullable []), the array elements should not be treated as @Nullable.

msridhar · 2024-08-15T14:55:47Z

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

+        .doTest();
+  }
+
+  private CompilationTestHelper makeHelper() {


Suggested change

private CompilationTestHelper makeHelper() {

private CompilationTestHelper makeLegacyModeHelper() {

msridhar · 2024-08-15T14:57:21Z

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

+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",


Let's use the JSpecify @Nullable when testing pure type use annotations

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

msridhar · 2024-08-15T15:00:22Z

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

+            "  @Nullable Object [][] foo4 = null;",
+            "  // ok according to spec",
+            "  Object @Nullable [][] foo5 = null;",
+            "  // NOT ok; @Nullable applies to first array dimension not the elements or the array ref",


What does "NOT ok" mean? I think in legacy mode we're just trying to preserve NullAway's legacy behavior? "NOT ok" makes it sound like we should report an error

msridhar · 2024-08-15T15:02:01Z

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

+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",


use jspecify annotation

msridhar · 2024-08-15T15:02:56Z

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

+            "  Object @Nullable[] foo2 = null;",
+            "  // ok, but @Nullable is not applied on top-level of array ",
+            "  @Nullable Object @Nullable [] foo3 = null;",
+            "  // ok only for backwards compat",


delete this line?

msridhar · 2024-08-15T15:04:52Z

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

+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jetbrains.annotations.Nullable;",


I saw somewhere that the targets for JetBrains annotations have changed across releases. Maybe we should just declare our own annotation com.uber.Nullable in the test that targets all the locations, to avoid issues?

Here is a code snippet for adding our own "both" annotation to a test:

.addSourceLines( "Nullable.java", "package com.uber;", "import java.lang.annotation.ElementType;", "import java.lang.annotation.Target;", "@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})", "public @interface Nullable {}")

Fixed, thanks for the snippet!

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

msridhar · 2024-08-15T15:12:16Z

nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java

@@ -608,6 +608,25 @@ public void mismatchedIndexUse() {
        .doTest();
  }

+  @Test
+  public void typeUseAndDeclarationLegacyAnnotationOnArray() {


This is not testing legacy mode. In fact, is there a way for us to fail if the config says that both JSpecify mode and legacy mode are enabled? That shouldn't be allowed.

Our JSpecify tests run fine when both are enabled. Do we still want to restrict this?

@armughan11 I think yes, since it's just invalid, and probably the user made a mistake.

nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java

msridhar · 2024-08-15T16:58:12Z

@ben-manes FYI this change may impact Caffeine. I think Caffeine uses Checker Framework annotations (which are type use) and might put them in the "wrong" place for declaring that array references are @Nullable. We are adding a LegacyAnnotationLocations flag to preserve backward compatibility. I am curious, though; do you see any issues with Caffeine updating these annotations to be in the right place (i.e., writing Object @Nullable [] x to say that x itself may be null), like might it lead to incompatibility with some other checking tool Caffeine uses?

ben-manes · 2024-08-15T19:06:31Z

I'd be happy to make any updates and its an internal only change. I only see one case where an array is annotated

/** Table of buffers. When non-null, size is a power of 2. */
volatile Buffer<E> @Nullable[] table;

The array or its elements may be null, and the array is resized as needed. If that now prefers to be double annotated that's fine. I think the array elements being null was assumed by NullAway so I only had to specify that the array reference was, which is the correct syntax?

msridhar · 2024-08-15T19:09:08Z

That annotation looks correct and should work fine. Maybe I was thinking of an old version. I'll try to run a snapshot version of NullAway with this change on Caffeine and see if there are any unexpected errors.

msridhar · 2024-08-18T00:05:38Z

I've temporarily converted this PR to a draft. #1015, #1020, and #1021 should land before this one, and then we should cut a bug-fix release, and then we can land this. @armughan11 you can continue to work on the review comments and I will re-review

Co-authored-by: Manu Sridharan <msridhar@gmail.com>

msridhar

Almost set! Just a couple of minor things.

Also can you confirm we have good coverage of whether array elements are treated as @Nullable according to the table? I realize some of these tests may have existed already and are not part of this PR.

nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java

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

msridhar · 2024-08-21T19:23:24Z

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

+            "import java.lang.annotation.ElementType;",
+            "import java.lang.annotation.Target;",
+            "@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})",
+            "public @interface Nullable {}",


Let's use two test source files for clean-ness. Rough suggestion:

Suggested change

"public @interface Nullable {}",

"public @interface Nullable {}")

.addSourceLines("Test.java",

"package com.uber;",

Please make the change in other places as well.

armughan11 · 2024-08-22T04:55:33Z

@msridhar For the three cases where we expect @Nullable to be applied on the elements in JSpecify mode, we have covered the elements position for Type use Annotations in jspecify/ArrayTests/arrayContentsAnnotationDereference here and when it's applied to both, that is covered in jspecify/ArrayTests/arrayContentsAndTopLevelAnnotation here

For annotations which are both in JSpecify mode, i have added that test as part of the latest commit.

msridhar · 2024-08-23T17:43:27Z

That annotation looks correct and should work fine. Maybe I was thinking of an old version. I'll try to run a snapshot version of NullAway with this change on Caffeine and see if there are any unexpected errors.

@ben-manes I confirmed there are no new errors due to this change on the master branch of Caffeine.

msridhar

Thanks for the great contribution!

armughan11 added 2 commits July 28, 2024 22:24

Added legacy annotation flag

5756d1a

fixed comment

6bb33b5

armughan11 marked this pull request as draft July 29, 2024 03:48

armughan11 added 2 commits July 28, 2024 22:51

Fixed comment.

f9f863e

Fix args

6b4ab88

armughan11 added 2 commits July 29, 2024 10:26

Added legacy flag for caffeine compiler

ccc7b17

Added declaration annotation test

819fdfc

armughan11 marked this pull request as ready for review July 29, 2024 15:33

armughan11 added 2 commits July 29, 2024 13:19

Merge branch 'master' into legacy-annotation-flag

c595ce6

Merge branch 'master' into legacy-annotation-flag

2fbc225

Added tests

e497e1b

msridhar reviewed Aug 15, 2024

View reviewed changes

msridhar mentioned this pull request Aug 17, 2024

Not-null varargs argument with Nullable elements - considered as Nullable #674

Closed

msridhar marked this pull request as draft August 18, 2024 00:04

Address review comments

4b6187f

armughan11 force-pushed the legacy-annotation-flag branch from 8e94fd2 to 4b6187f Compare August 20, 2024 05:36

armughan11 and others added 2 commits August 20, 2024 00:36

Update nullaway/src/test/java/com/uber/nullaway/ArrayTests.java

69b81ce

Co-authored-by: Manu Sridharan <msridhar@gmail.com>

Merge branch 'master' into legacy-annotation-flag

3d65db3

armughan11 requested a review from msridhar August 20, 2024 05:37

msridhar reviewed Aug 21, 2024

View reviewed changes

msridhar marked this pull request as ready for review August 21, 2024 19:26

Address review comments

6dce3c6

armughan11 requested a review from msridhar August 22, 2024 04:39

Address review comments

3545a59

armughan11 and others added 3 commits August 23, 2024 02:44

Added legacy and jspecify flag test

6105a3d

add / update copyright notices

a6a79e0

revert change

1d6c0dd

msridhar approved these changes Aug 23, 2024

View reviewed changes

msridhar merged commit 0d500cc into uber:master Aug 23, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce Strict Interpretation Of Type Use Annotation Locations Outside of JSpecify mode #1010

Enforce Strict Interpretation Of Type Use Annotation Locations Outside of JSpecify mode #1010

armughan11 commented Jul 29, 2024 •

edited

Loading

msridhar commented Jul 29, 2024

armughan11 commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

msridhar commented Aug 4, 2024 •

edited

Loading

msridhar left a comment

msridhar Aug 15, 2024

msridhar Aug 15, 2024

armughan11 Aug 20, 2024

msridhar Aug 15, 2024

armughan11 Aug 20, 2024

msridhar Aug 15, 2024

armughan11 Aug 20, 2024

msridhar Aug 15, 2024

armughan11 Aug 20, 2024

msridhar Aug 15, 2024

msridhar Aug 18, 2024

armughan11 Aug 20, 2024

msridhar Aug 15, 2024

armughan11 Aug 20, 2024

msridhar Aug 21, 2024

msridhar commented Aug 15, 2024

ben-manes commented Aug 15, 2024

msridhar commented Aug 15, 2024

msridhar commented Aug 18, 2024

msridhar left a comment

msridhar Aug 21, 2024

armughan11 Aug 22, 2024

armughan11 commented Aug 22, 2024

msridhar commented Aug 23, 2024

msridhar left a comment

	private CompilationTestHelper makeHelper() {
	private CompilationTestHelper makeLegacyModeHelper() {

Enforce Strict Interpretation Of Type Use Annotation Locations Outside of JSpecify mode #1010

Enforce Strict Interpretation Of Type Use Annotation Locations Outside of JSpecify mode #1010

Conversation

armughan11 commented Jul 29, 2024 • edited Loading

msridhar commented Jul 29, 2024

armughan11 commented Jul 29, 2024 • edited Loading

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

msridhar commented Aug 4, 2024 • 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

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 commented Aug 15, 2024

ben-manes commented Aug 15, 2024

msridhar commented Aug 15, 2024

msridhar commented Aug 18, 2024

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

armughan11 commented Aug 22, 2024

msridhar commented Aug 23, 2024

msridhar left a comment

Choose a reason for hiding this comment

armughan11 commented Jul 29, 2024 •

edited

Loading

armughan11 commented Jul 29, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading

msridhar commented Aug 4, 2024 •

edited

Loading