Update handling of annotations on varargs argument #1025

msridhar · 2024-08-24T02:16:22Z

See #708 (comment) for an overview, and #674 (comment) for all the details of how annotations on the varargs argument are now checked by NullAway. Reproduced from #674 (comment), here are the updated behaviors for different types of annotations on the varargs formal parameter (Update the table is slightly tweaked from the previous comment; this table now matches the current thinking / implementation):

Annotation Type	Annotation Position	Mode	`@Nullable` varargs array	`@Nullable` array elements	`@Nullable` args at calls
declaration	-	standard	❌	❌	✅
declaration	-	legacy	✅	❌	✅
declaration	-	JSpecify	❌	✅	✅
type use	before `...`	standard	✅	❌	❌
type use	before `...`	legacy	✅	❌	✅
type use	before `...`	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	before `...`	standard	✅	❌	❌
both	before `...`	legacy	✅	❌	✅
both	before `...`	JSpecify	✅	❌	❌
both	elements	standard	❌	❌	✅
both	elements	legacy	✅	❌	✅
both	elements	JSpecify	❌	✅	✅
both	both	standard	✅	❌	✅
both	both	legacy	✅	❌	✅
both	both	JSpecify	✅	✅	✅

"@Nullable varargs array" means that the varargs array is treated as @Nullable within the method body, and a @Nullable array can be passed at call sites. "@Nullable array elements" means that elements of the varargs array are treated as @Nullable within the method body (only relevant in JSpecify mode). And, "@Nullable args at calls" means that individual arguments passed in the varargs position at call sites may be @Nullable.

Fixes #674

codecov · 2024-08-24T02:23:06Z

Codecov Report

Attention: Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.06%. Comparing base (0d500cc) to head (535af4f).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...away/src/main/java/com/uber/nullaway/NullAway.java	95.45%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1025      +/-   ##
============================================
+ Coverage     86.00%   86.06%   +0.06%     
- Complexity     2091     2110      +19     
============================================
  Files            83       83              
  Lines          6908     6932      +24     
  Branches       1331     1344      +13     
============================================
+ Hits           5941     5966      +25     
+ Misses          551      550       -1     
  Partials        416      416

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

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

lazaroclapp

Ok, left comments/nits as I went along, including suggestions of comments/naming that make things more readable for me, but which you can ultimately accept or ignored as desired.

Overall this LGTM, a few questions inline, that can be addressed in follow ups and also:

Do we have any tests checking that when given a JSpecify annotation we always act as in JSpecify mode? I saw that in one of the discussion comments, but not the tests.

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

lazaroclapp · 2024-08-30T23:18:15Z

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

-      // This statement should be unreachable without assigning actual beforehand:
-      Preconditions.checkNotNull(actual);


This is still the case, right? Is just that now NullAway can prove this statically?

Correct, javac's initialized variables check now proves it (along with the fact that we never assign null to the variable)

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

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

lazaroclapp · 2024-08-30T23:47:36Z

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

+            "public class Utilities {",
+            " public static String takesNullableVarargs(Object o, @Nullable Object... others) {",
+            "  String s = o.toString() + \" \" + others.toString();",
+            "  return s;",


Shouldn't we be checking in here that the elements are @Nullable? By adding the for loop and trying to deref `other? Or is the table saying that in JSpecify mode (and every mode other than legacy) we don't check declaration annotations (but acknowledge them for call sites)?

I did add the test in bc46f2e. And I have now changed the logic so that we get an error, in 6b622c7. Great catch! My table didn't have this case as an error before, due to an attempt to make treatment of array types identical for regular arrays and varargs arrays. But there are already other cases where we don't have identical treatment. And the previous table made a @Nullable declaration annotation a no-op within the method body, which could be confusing. So this is now fixed, and I will update the table accordingly.

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

lazaroclapp · 2024-08-30T23:56:19Z

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

+            "    Object[] x = new Object[10];",
+            "    takesVarargs(x, o);",


Again, why isn't this an error in JSpecify mode? Isn't the initialization here an array of 10 null references?

Yes, this is again due to unsound handling of array initializers, see previous comment

Co-authored-by: Lázaro Clapp <[email protected]>

… in JSpecify mode

msridhar

Thanks a lot for the detailed review @lazaroclapp!

msridhar · 2024-09-01T00:54:22Z

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

-      // This statement should be unreachable without assigning actual beforehand:
-      Preconditions.checkNotNull(actual);


Correct, javac's initialized variables check now proves it (along with the fact that we never assign null to the variable)

msridhar · 2024-09-01T01:00:37Z

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

+            "    Utilities.takesNullableVarargs(o1, (java.lang.Object) null);",
+            "    Object[] x = null;",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
+            "    Utilities.takesNullableVarargs(o1, x);",


Yup, added in 71301a3

msridhar · 2024-09-01T01:17:20Z

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

+            "  String s = o.toString() + \" \";",
+            "  // BUG: Diagnostic contains: enhanced-for expression others is @Nullable",
+            "  for (Object other : others) {",
+            "    s += (other == null) ? \"(null) \" : other.toString() + \" \";",


That reasoning only occurs in JSpecify mode. It is tested in JSpecifyVarargsTests#typeUseAndDeclarationOnBoth and in other methods in that class.

msridhar · 2024-09-01T01:19:56Z

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

+            "    Object[] x = new Object[10];",
+            "    takesVarargs(x, o);",


This is a separate issue, which is around checking of array initialization. By default, thus far, we have been unsound and not checked for initialization of array elements at all. Checking that array elements (for an array of @NonNull references) are initialized before they are used would be challenging and likely lead to many false positives. The Checker Framework Nullness Checker also handles array initialization unsoundly by default; see here. We should probably document this in our JSpecify mode documentation.

msridhar · 2024-09-01T01:30:41Z

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

+            "public class Utilities {",
+            " public static String takesNullableVarargs(Object o, @Nullable Object... others) {",
+            "  String s = o.toString() + \" \" + others.toString();",
+            "  return s;",


I did add the test in bc46f2e. And I have now changed the logic so that we get an error, in 6b622c7. Great catch! My table didn't have this case as an error before, due to an attempt to make treatment of array types identical for regular arrays and varargs arrays. But there are already other cases where we don't have identical treatment. And the previous table made a @Nullable declaration annotation a no-op within the method body, which could be confusing. So this is now fixed, and I will update the table accordingly.

msridhar · 2024-09-01T01:48:05Z

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

+            "    Object[] x = new Object[10];",
+            "    takesVarargs(x, o);",


Yes, this is again due to unsound handling of array initializers, see previous comment

lazaroclapp

Changes make sense to me!

msridhar added 16 commits August 23, 2024 10:44

WIP

145f0e3

WIP

50862eb

format

fbd131e

test case

db9c3b1

tweak test

d25d350

WIP

a466b27

WIP

9f37626

more

c45b44c

WIP

3d086f3

WIP

e75c938

WIP

98b3d12

fix for nullable varargs arrays

f32dd87

JSpecify mode tests

65dd597

initial legacy mode tests

c85fec1

WIP

0878e5b

fixes

91fe3fe

msridhar added 2 commits August 23, 2024 20:14

comments

c524432

tweaks

252e6c3

msridhar marked this pull request as ready for review August 24, 2024 03:20

msridhar commented Aug 24, 2024

View reviewed changes

nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyArrayTests.java Show resolved Hide resolved

msridhar requested a review from lazaroclapp August 24, 2024 03:23

msridhar added 2 commits August 24, 2024 07:49

clarify comment

925265d

comments

58ece61

lazaroclapp approved these changes Aug 30, 2024

View reviewed changes

msridhar and others added 5 commits August 31, 2024 17:49

suggested comment

4068568

Co-authored-by: Lázaro Clapp <[email protected]>

comment suggestion

fe92b7d

Co-authored-by: Lázaro Clapp <[email protected]>

Apply suggestions from code review

62a7df5

Co-authored-by: Lázaro Clapp <[email protected]>

formatting

893fabe

Apply suggestions from code review

fee21e4

Co-authored-by: Lázaro Clapp <[email protected]>

msridhar added 3 commits August 31, 2024 18:00

more tests of passing null varargs array

71301a3

another test

bc46f2e

@nullable declaration annotation makes varargs array elements @nullable…

6b622c7

… in JSpecify mode

msridhar commented Sep 1, 2024

View reviewed changes

logic simplification

535af4f

lazaroclapp reviewed Sep 1, 2024

View reviewed changes

msridhar merged commit b7756ea into uber:master Sep 1, 2024
11 checks passed

msridhar deleted the manu/issue-674 branch September 1, 2024 16:38

msridhar mentioned this pull request Sep 2, 2024

Update handling of varargs in unannotated / unmarked code #1027

Open

msridhar mentioned this pull request Dec 12, 2024

Another varargs @Nullable issue #1090

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update handling of annotations on varargs argument #1025

Update handling of annotations on varargs argument #1025

msridhar commented Aug 24, 2024 •

edited

Loading

codecov bot commented Aug 24, 2024 •

edited

Loading

lazaroclapp left a comment

lazaroclapp Aug 30, 2024

msridhar Sep 1, 2024 •

edited

Loading

lazaroclapp Aug 30, 2024

msridhar Sep 1, 2024

lazaroclapp Aug 30, 2024

msridhar Sep 1, 2024

msridhar left a comment

msridhar Sep 1, 2024 •

edited

Loading

msridhar Sep 1, 2024

msridhar Sep 1, 2024

msridhar Sep 1, 2024

msridhar Sep 1, 2024

msridhar Sep 1, 2024

lazaroclapp left a comment

		// This statement should be unreachable without assigning actual beforehand:
		Preconditions.checkNotNull(actual);

Update handling of annotations on varargs argument #1025

Update handling of annotations on varargs argument #1025

Conversation

msridhar commented Aug 24, 2024 • edited Loading

codecov bot commented Aug 24, 2024 • edited Loading

Codecov Report

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar Sep 1, 2024 • 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

msridhar left a comment

Choose a reason for hiding this comment

msridhar Sep 1, 2024 • 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

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

msridhar commented Aug 24, 2024 •

edited

Loading

codecov bot commented Aug 24, 2024 •

edited

Loading

msridhar Sep 1, 2024 •

edited

Loading

msridhar Sep 1, 2024 •

edited

Loading