Partial handling for restrictive annotations on varargs in unannotated code #1029

msridhar · 2024-09-02T22:57:08Z

Partially addresses #1027. Most importantly, handles the case with unannotated methods and no restrictive annotations; we were getting warnings in some cases with the main branch, which impacts backward compatibility.

I started implementing full support for restrictive annotations on varargs parameters and then realized it would take some refactoring (since there can be a restrictive annotation on the varargs array itself or on its contents, and we have no way to express this in the current code structure). This PR contains some incomplete code towards that change, which I figured does no harm so I left it in for now. If desired I can remove it and put it in a separate PR.

codecov · 2024-09-02T23:06:06Z

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.23%. Comparing base (fe9476a) to head (118dc17).
Report is 1 commits behind head on master.

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

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1029      +/-   ##
============================================
- Coverage     87.28%   87.23%   -0.05%     
- Complexity     2117     2129      +12     
============================================
  Files            83       83              
  Lines          6943     6965      +22     
  Branches       1346     1356      +10     
============================================
+ Hits           6060     6076      +16     
- Misses          456      458       +2     
- Partials        427      431       +4

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

msridhar · 2024-09-02T23:07:21Z

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

@@ -200,7 +200,8 @@ public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
            "    ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
            "    ThirdParty.takesNullableVarargs(o1, o4);",
            "    ThirdParty.takesNullableVarargs(o1, (Object) null);",
-            "    // BUG: Diagnostic contains: passing @Nullable parameter '(Object[]) null' where @NonNull",
+            "    // No error: we require a type-use annotation for nullability of the varargs array itself",
+            // TODO should we preserve the old behavior in legacy mode?


@lazaroclapp with this change, we no longer detect an error when passing a null array in the varargs position to a third-party method with a JetBrains @NotNull annotation on the varargs parameter. On the one hand, getting an error here is the "desired" behavior, in that the annotation is supposed to apply to the array itself (see #720). But, everywhere else, a declaration annotation on the varargs argument is treated as applying to the contents of the array, not the array itself. To me, adding the extra code complexity to detect these errors in legacy mode for this corner case is not worth it; hopefully Kotlinc will be convinced to put annotations in the "right" place soon. But I'd be interested in your opinion.

I think without a coordinated change on kotlinc, I would prefer to keep the hack. But I don't feel super strongly about it. It's just an additional test of (a) is the annotation in bytecode, (b) is it the jetbrains annotation, (c) is it in var args... if all that is true, hacky override with a comment linking to the issue. No? Would this be hard to maintain?

Unfortunately, Java code interfacing with Kotlin code is a big portion of what we will see in Android for the foreseeable future, so kotlinc quirks might be the kind of thing we can't ignore for the sake of uniformity. Though if you can convince them to generate the "right" bytecode, I'd be happy to see this hack disappear (hopefully we don't need then to analyze magic numbers to see which kotlinc version produced the jar)

You're right; it looks like for now kotlinc will be emitting similar bytecodes. Fixed in 7330a20

lazaroclapp

Definitely an incomplete fix, as the description clearly states, but also a step in the right direction. See comments below. All addressable in follow ups, of course.

lazaroclapp · 2024-09-06T19:58:02Z

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

+    if ((arraySymbol.flags() & Flags.VARARGS) != 0) {
+      return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config);
+    }
+    return false;


Is codecov correct here that we never test the case where this method returns false?

It is correct. We should get coverage of this line when we add logic to process restrictive @NonNull annotations on array elements in positions other than the varargs position, which we will do in a follow up.

lazaroclapp · 2024-09-06T20:06:58Z

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

@@ -200,7 +200,8 @@ public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
            "    ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
            "    ThirdParty.takesNullableVarargs(o1, o4);",
            "    ThirdParty.takesNullableVarargs(o1, (Object) null);",
-            "    // BUG: Diagnostic contains: passing @Nullable parameter '(Object[]) null' where @NonNull",
+            "    // No error: we require a type-use annotation for nullability of the varargs array itself",
+            // TODO should we preserve the old behavior in legacy mode?


I think without a coordinated change on kotlinc, I would prefer to keep the hack. But I don't feel super strongly about it. It's just an additional test of (a) is the annotation in bytecode, (b) is it the jetbrains annotation, (c) is it in var args... if all that is true, hacky override with a comment linking to the issue. No? Would this be hard to maintain?

Unfortunately, Java code interfacing with Kotlin code is a big portion of what we will see in Android for the foreseeable future, so kotlinc quirks might be the kind of thing we can't ignore for the sake of uniformity. Though if you can convince them to generate the "right" bytecode, I'd be happy to see this hack disappear (hopefully we don't need then to analyze magic numbers to see which kotlinc version produced the jar)

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

msridhar

Thanks for the review!

msridhar · 2024-09-07T17:04:16Z

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

+    if ((arraySymbol.flags() & Flags.VARARGS) != 0) {
+      return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config);
+    }
+    return false;


It is correct. We should get coverage of this line when we add logic to process restrictive @NonNull annotations on array elements in positions other than the varargs position, which we will do in a follow up.

msridhar · 2024-09-07T17:17:15Z

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

@@ -200,7 +200,8 @@ public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
            "    ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
            "    ThirdParty.takesNullableVarargs(o1, o4);",
            "    ThirdParty.takesNullableVarargs(o1, (Object) null);",
-            "    // BUG: Diagnostic contains: passing @Nullable parameter '(Object[]) null' where @NonNull",
+            "    // No error: we require a type-use annotation for nullability of the varargs array itself",
+            // TODO should we preserve the old behavior in legacy mode?


You're right; it looks like for now kotlinc will be emitting similar bytecodes. Fixed in 7330a20

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

msridhar added 6 commits September 2, 2024 11:43

Properly handle varargs array parameter for unannotated methods

e0be617

more tests

2309531

more

fcfeb94

add TODO

14ae632

fix comment

18a7002

comment tweaks

c644768

msridhar requested a review from lazaroclapp September 2, 2024 23:04

msridhar marked this pull request as ready for review September 2, 2024 23:04

msridhar commented Sep 2, 2024

View reviewed changes

more comment fixes

e5d7d1b

msridhar marked this pull request as draft September 2, 2024 23:45

msridhar added 2 commits September 2, 2024 16:59

more tests

4f07f82

fixes for legacy mode

e13dc58

msridhar marked this pull request as ready for review September 3, 2024 03:03

Merge branch 'master' into varargs-unannotated

e312e44

lazaroclapp approved these changes Sep 6, 2024

View reviewed changes

msridhar added 3 commits September 7, 2024 10:03

add test

32e9aa2

fully restore workarounds for JetBrains @NotNull from kotlinc

7330a20

more tests

118dc17

msridhar commented Sep 7, 2024

View reviewed changes

msridhar enabled auto-merge (squash) September 7, 2024 18:45

msridhar merged commit e2e5394 into uber:master Sep 7, 2024
9 of 11 checks passed

msridhar deleted the varargs-unannotated branch September 7, 2024 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial handling for restrictive annotations on varargs in unannotated code #1029

Partial handling for restrictive annotations on varargs in unannotated code #1029

msridhar commented Sep 2, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading

msridhar Sep 2, 2024

lazaroclapp Sep 6, 2024

msridhar Sep 7, 2024

lazaroclapp left a comment

lazaroclapp Sep 6, 2024

msridhar Sep 7, 2024

lazaroclapp Sep 6, 2024

msridhar left a comment

msridhar Sep 7, 2024

msridhar Sep 7, 2024

Partial handling for restrictive annotations on varargs in unannotated code #1029

Partial handling for restrictive annotations on varargs in unannotated code #1029

Conversation

msridhar commented Sep 2, 2024

codecov bot commented Sep 2, 2024 • edited Loading

Codecov Report

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

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

codecov bot commented Sep 2, 2024 •

edited

Loading