Run NullAway in JSpecify mode #1608

Stephan202 · 2025-03-22T14:45:00Z

This is a draft PR on top of #1598, to demonstrate the benefits and gotchas of running NullAway in JSpecify mode.

Suggested commit message.

Run NullAway in JSpecify mode (#1608)

github-actions · 2025-03-22T14:47:55Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added some context. CC @msridhar. Let me know if for any/all of the flagged issues I should file a GitHub issue over at NullAway; happy to do so!

Stephan202 · 2025-03-22T14:47:08Z

...tion-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java

-        MethodInvocationTree inputTree = (MethodInvocationTree) ASTHelpers.getReceiver(tree);
+        MethodInvocationTree inputTree =
+            (MethodInvocationTree)
+                requireNonNull(
+                    ASTHelpers.getReceiver(tree), "Instance method invocation must have receiver");


All the places in which I had to add requireNonNull are genuinely cases where we can't reasonably expect NullAway to infer the non-nullness invariant. 👍

This is cool but I'm not sure why it's not detected outside JSpecify mode 🤔 Is there anything related to generics here?

If you want I can file an issue for this one as well, though since it's not a bug as far as this project is concerned: also happy to leave it.

I think I figured this one out; it's probably uber/NullAway#1144.

Stephan202 · 2025-03-22T14:48:48Z

error-prone-contrib/pom.xml

+        <!-- XXX: Drop this dependency declaration once NullAway in JSpecify
+        mode no longer requires it when building with JDK 23+. -->
+        <dependency>
+            <groupId>com.google.code.findbugs</groupId>
+            <artifactId>jsr305</artifactId>
+            <scope>provided</scope>
+        </dependency>


Without this version declaration builds with JDK 23+ fail with:

[WARNING] Cannot find annotation method 'when()' in type 'javax.annotation.Nonnull': class file for javax.annotation.Nonnull not found [WARNING] /home/sschroevers/workspace/picnic/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java: unknown enum constant javax.annotation.meta.When.MAYBE reason: class file for javax.annotation.meta.When not found

(This causes a failure because we build with -Werror.)

It's rather ironic that enabling JSpecify mode forces one to add a "competing" dependency to the classpath. 🙃

Hrm, this is a weird one! Particularly that it's tied to JSpecify mode, I have no idea what's going on. Can you file an issue for this one?

Sure! I'll first try to narrow down which aspect of ReactorRules triggers the issue.

Alright, filed uber/NullAway#1171.

Stephan202 · 2025-03-22T14:52:29Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableTableRules.java

+    // XXX: Drop the warning suppression annotation once NullAway in JSpecify mode understands that
+    // the method references do not have a nullable return type.
    @BeforeTemplate
+    @SuppressWarnings("NullAway")


As the type parameters aren't @Nullable, I would expect NullAway to understand that Table.Cell::getRowKey and do not return null. But without the suppression we get:

[WARNING] /home/sschroevers/workspace/picnic/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableTableRules.java:[64,23] [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull (see http://t.uber.com/nullaway ) [WARNING] /home/sschroevers/workspace/picnic/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableTableRules.java:[64,46] [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull (see http://t.uber.com/nullaway ) [WARNING] /home/sschroevers/workspace/picnic/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableTableRules.java:[64,72] [NullAway] referenced method returns @Nullable, but functional interface method java.util.function.Function.apply(T) returns @NonNull (see http://t.uber.com/nullaway )

This one could be a combination of uber/NullAway#1075 and uber/NullAway#1128. I'm hoping that uber/NullAway#1128 will fix it. Could you somehow like this from uber/NullAway#1128 so we can test if a fix for that issue fixes this one?

Alright, I added a comment here. Based on the additional details I shared there, I suspect that this may be a JSpecify-specific edge case to a more general issue with @ParametricNullness handling.

Stephan202 · 2025-03-22T14:55:19Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+    // XXX: Drop the warning suppression annotation once NullAway in JSpecify mode understands that
+    // `Mono<Void>` is a subtype of `Mono<@Nullable Void>`.
    @BeforeTemplate
+    @SuppressWarnings("NullAway")
    Mono<@Nullable Void> before(Mono<T> mono) {


There are many similar expressions. NullAway can be appeased by simply replacing Mono<@Nullable Void> with Mono<Void>, but then VoidMissingNullable complains.

Please file an issue on this one. We have hacks for @Nullable Void but we should disable them in JSpecify mode.

Check! I filed uber/NullAway#1172.

github-actions · 2025-03-22T18:37:53Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2025-03-29T13:28:49Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2025-03-29T13:48:39Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2025-03-29T13:58:14Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2025-03-29T14:07:44Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2025-03-29T14:13:31Z

I rebased the branch onto #1616 and added a commit. Two notes:

Locally I could remove the workaround for this issue, while on GitHub Actions it did persist. After reinstating the workaround, pushing to GitHub, and dropping it again, the issue was gone also on GHA. Weird caching bug? 🤔 (Perhaps with Upgrade Guava 33.4.0-jre -> 33.4.7-jre #1598 it'll have to be reintroduced again (TBD), but if so, the change belongs in that PR.)

The reason for rebasing this PR on top of Upgrade NullAway 0.12.5 -> 0.12.6 #1616, is that with NullAway 0.12.5 on master I got an error:

java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
  at jdk.compiler/com.sun.tools.javac.util.List.get(List.java:476)
  at com.uber.nullaway.generics.InferSubstitutionViaAssignmentContextVisitor.visitClassType(InferSubstitutionViaAssignmentContextVisitor.java:33)
  at com.uber.nullaway.generics.InferSubstitutionViaAssignmentContextVisitor.visitClassType(InferSubstitutionViaAssignmentContextVisitor.java:15)
  at jdk.compiler/com.sun.tools.javac.code.Type$ClassType.accept(Type.java:1053)
  at com.uber.nullaway.generics.GenericsChecks.checkTypeParameterNullnessForAssignability(GenericsChecks.java:459)
  at com.uber.nullaway.NullAway.matchVariable(NullAway.java:1515)
  ...

Thanks for all the help @msridhar! 🙏

msridhar · 2025-03-29T14:17:45Z

The reason for rebasing this PR on top of Upgrade NullAway 0.12.5 -> 0.12.6 #1616, is that with NullAway 0.12.5 on master I got an error:

Yeah this is the main reason we cut 0.12.6 🙂 uber/NullAway#1176 led to the same crash

Stephan202 · 2025-03-29T14:19:57Z

(Pitest correctly complains about a lack of coverage in AnnotationAttributeMatcher. That relates to this comment. Something we should fix, but I consider it out of scope for this PR. Up to @rickie and @mohamedsamehsalah to more carefully review the change in that particular class 😉.)

mohamedsamehsalah

LGTM 👍

(Pitest correctly complains about a lack of coverage in AnnotationAttributeMatcher. That relates to this comment. Something we should fix, but I consider it out of scope for this PR. Up to @rickie and @mohamedsamehsalah to more carefully review the change in that particular class 😉.)

Refactored logic looks sound to me ✌️

rickie

Changes LGTM! Nice work ✨

sonarqubecloud · 2025-04-02T11:58:19Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
70.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2025-04-02T11:58:47Z

Surviving mutants in this change: 4
Killed mutants in this change: 4

class	surviving	killed
🧟`tech.picnic.errorprone.utils.AnnotationAttributeMatcher`	4	0
🎉`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	0	2
🎉`tech.picnic.errorprone.experimental.bugpatterns.MethodReferenceUsage`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

This reverts commit c14e128.

Stephan202 · 2025-04-12T13:42:06Z

This PR was accidentally merged into #1616. There it was amended to include the following changes:

diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
index 58384d2c..955d3753 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
@@ -515,8 +515,13 @@ final class ReactorRules {
           mono.switchIfEmpty(Mono.empty()), mono.flux().next(), mono.flux().singleOrEmpty());
     }
 
+    // XXX: Consider filing a SonarCloud issue for the S2637 false positive.
     @BeforeTemplate
-    @SuppressWarnings("java:S4968" /* Result may be `Mono<Void>`. */)
+    @SuppressWarnings({
+      "java:S2637" /* False positive: result is never `null`. */,
+      "java:S4968" /* Result may be `Mono<Void>`. */,
+      "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict"
+    })
     Mono<? extends @Nullable Void> before2(Mono<@Nullable Void> mono) {
       return Refaster.anyOf(mono.ignoreElement(), mono.then());
     }
@@ -970,8 +975,13 @@ final class ReactorRules {
       return flux.ignoreElements().then();
     }
 
+    // XXX: Consider filing a SonarCloud issue for the S2637 false positive.
     @BeforeTemplate
-    @SuppressWarnings("java:S4968" /* Result may be `Mono<Void>`. */)
+    @SuppressWarnings({
+      "java:S2637" /* False positive: result is never `null`. */,
+      "java:S4968" /* Result may be `Mono<Void>`. */,
+      "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict"
+    })
     Mono<? extends @Nullable Void> before2(Flux<@Nullable Void> flux) {
       return flux.ignoreElements();
     }

Stephan202 · 2025-04-14T07:45:41Z

This change made its way into master through #1616, as aa988ef.

Stephan202 commented Mar 22, 2025

View reviewed changes

Stephan202 force-pushed the sschroevers/guava-33.4.5-upgrade-with-jspecify-mode branch from 3accab7 to 7eb979d Compare March 22, 2025 18:34

Stephan202 mentioned this pull request Mar 27, 2025

Upgrade NullAway 0.12.4 -> 0.12.5 #1615

Merged

1 task

Stephan202 force-pushed the sschroevers/guava-33.4.5-upgrade-with-jspecify-mode branch from 7eb979d to 95c0a22 Compare March 29, 2025 13:25

Stephan202 changed the base branch from renovate/guava-33.x to renovate/version.nullaway March 29, 2025 13:26

Stephan202 force-pushed the sschroevers/guava-33.4.5-upgrade-with-jspecify-mode branch from 4330736 to 60d9f56 Compare March 29, 2025 14:04

Stephan202 added this to the 0.22.0 milestone Mar 29, 2025

Stephan202 marked this pull request as ready for review March 29, 2025 14:13

Stephan202 requested review from mohamedsamehsalah and rickie March 29, 2025 14:13

Picnic-DevPla-Bot force-pushed the renovate/version.nullaway branch 2 times, most recently from 9369cd3 to 248ac4b Compare April 2, 2025 01:19

mohamedsamehsalah approved these changes Apr 2, 2025

View reviewed changes

rickie approved these changes Apr 2, 2025

View reviewed changes

Stephan202 added 2 commits April 2, 2025 13:55

Run NullAway in JSpecify mode

97dae56

Simplify

6ef1c6e

rickie force-pushed the sschroevers/guava-33.4.5-upgrade-with-jspecify-mode branch from 60d9f56 to 6ef1c6e Compare April 2, 2025 11:55

rickie merged commit aeb9587 into renovate/version.nullaway Apr 2, 2025
17 checks passed

rickie deleted the sschroevers/guava-33.4.5-upgrade-with-jspecify-mode branch April 2, 2025 12:23

rickie added the chore A task not related to code (build, formatting, process, ...) label Apr 2, 2025

Stephan202 added a commit that referenced this pull request Apr 3, 2025

Run NullAway in JSpecify mode (#1608)

14a1ee1

Stephan202 mentioned this pull request Apr 3, 2025

Upgrade NullAway 0.12.5 -> 0.12.6 #1616

Merged

1 task

Stephan202 added a commit that referenced this pull request Apr 12, 2025

Run NullAway in JSpecify mode (#1608)

c14e128

Stephan202 added a commit that referenced this pull request Apr 12, 2025

Revert "Run NullAway in JSpecify mode (#1608)"

2232531

This reverts commit c14e128.

Stephan202 added a commit that referenced this pull request Apr 12, 2025

Run NullAway in JSpecify mode (#1608)

a1dd062

Stephan202 added a commit that referenced this pull request Apr 14, 2025

Run NullAway in JSpecify mode (#1608)

cb8b432

Stephan202 added a commit that referenced this pull request Apr 14, 2025

Run NullAway in JSpecify mode (#1608)

aa988ef

Run NullAway in JSpecify mode #1608

Run NullAway in JSpecify mode #1608

Uh oh!

Conversation

Stephan202 commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 22, 2025

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 22, 2025

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

Stephan202 commented Mar 29, 2025

Uh oh!

msridhar commented Mar 29, 2025

Uh oh!

Stephan202 commented Mar 29, 2025

Uh oh!

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 2, 2025

Quality Gate passed

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

Uh oh!

Stephan202 commented Apr 12, 2025

Uh oh!

Stephan202 commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

Stephan202 commented Mar 22, 2025 •

edited

Loading

msridhar Mar 22, 2025 •

edited

Loading