JSpecify: fix crash with calls to static methods #856

akulk022 · 2023-11-07T23:05:54Z

After adding com.google.common to annotated packages for buildWithNullAway in JSpecify Mode, we got the following exception:

error: An unhandled exception was thrown by the Error Prone static analysis plugin.
    return findEnclosingMethodOrLambdaOrInitializer(path, ImmutableSet.of());

com.google.common.util.concurrent.UncheckedExecutionException: java.lang.NullPointerException: castToNonNull failed!
        at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2086)
        at com.google.common.cache.LocalCache.get(LocalCache.java:4012)
        at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4035)
        at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5011)

Caused by: java.lang.NullPointerException: castToNonNull failed!
        at com.uber.nullaway.NullabilityUtil.castToNonNull(NullabilityUtil.java:409)
        at com.uber.nullaway.GenericsChecks.getGenericReturnNullnessAtInvocation(GenericsChecks.java:800)
        at com.uber.nullaway.dataflow.AccessPathNullnessPropagation.genericReturnIsNullable(AccessPathNullnessPropagation.java:1031)
        at com.uber.nullaway.dataflow.AccessPathNullnessPropagation.returnValueNullness(AccessPathNullnessPropagation.java:1008)
        at com.uber.nullaway.dataflow.AccessPathNullnessPropagation.visit

With the call to java ImmutableSet.of() which is a static call, we were wrongly trying to return null from getTreeType in GenericChecks.java for this case which caused the above Exception.

A unit test which reproduces the issue has been added to NullAwayJSpecifyGenerickChecks.java:

import org.jspecify.annotations.Nullable;
import com.google.common.collect.ImmutableSet;
class Test {
    static void funcImmutableSet(ImmutableSet<Object> is){
    }
    static void testNegative() {
		//We were getting the issue on this line
        funcImmutableSet(ImmutableSet.of());
    }
}

All the unit tests from the NullAway build have passed for these changes.

…ot from method invocation raw type

codecov · 2023-11-07T23:11:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5fc285b) 86.93% compared to head (54bc881) 86.95%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #856      +/-   ##
============================================
+ Coverage     86.93%   86.95%   +0.01%     
- Complexity     1886     1889       +3     
============================================
  Files            74       74              
  Lines          6215     6215              
  Branches       1208     1208              
============================================
+ Hits           5403     5404       +1     
  Misses          405      405              
+ Partials        407      406       -1

Files	Coverage Δ
...rc/main/java/com/uber/nullaway/GenericsChecks.java	`90.28% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

…estForGuavaRawTypeBuildIssue

…ijitk

msridhar

Thanks for the fix! Couple of comments

msridhar · 2023-11-08T18:04:02Z

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

@@ -1494,12 +1494,40 @@ public void interactionWithContracts() {
        .doTest();
  }

+  @Test
+  public void testForGuavaRawTypeBuildIssue() {


Rather than pulling in Guava here, let's just test for the core issue of static method calls of the form X.foo() where X is there explicitly. You should be able to write a standalone test case that exposes the previous crash, without importing Guava or adding the makeHelperWithGuava() method

I agree. I have changed the test case to reproduce this scenario without using importing guava and ImmutableSet. Let me know if this works. I have removed the helper which imports guava too.

msridhar · 2023-11-08T18:04:24Z

nullaway/build.gradle

@@ -152,7 +152,7 @@ tasks.register('buildWithNullAway', JavaCompile) {
            sourceSets.main.compileClasspath)
    options.errorprone.enabled = true
    options.errorprone {
-        option("NullAway:AnnotatedPackages", "com.uber,org.checkerframework.nullaway")
+        option("NullAway:AnnotatedPackages", "com.uber,org.checkerframework.nullaway,com.google.common")


Now that I think of it, let's not add this change here, and instead do it in a separate follow-up PR, just to make things cleaner

Makes sense, I have removed it from this branch.

…of being specific to guava ImmutableSet

…_issue_abhijitk' into jspecify_buildWithNullAway_guava_issue_abhijitk

… commit

msridhar · 2023-11-08T21:56:56Z

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

@@ -1494,6 +1494,34 @@ public void interactionWithContracts() {
        .doTest();
  }

+  @Test
+  public void testForStaticMethodCallAsAParam() {


@akulk022 have you confirmed that this test fails with two unexpected errors if you remove the fixes in GenericsChecks?

Yes, I verified it for both scenarios by removing the fix and seeing if the test case fails with the same exception as before.

…dle (#857) Based on the fixes made in #856 , we should no longer get that exception after adding com.google.common in the annotated packages when running in JSpecify mode.

Adding com.google.common to annotatedpackages and fixing the NPE we g…

25963a9

…ot from method invocation raw type

Adding scenario for ImmutableSet.Of() with a param to the test case t…

ffb680e

…estForGuavaRawTypeBuildIssue

msridhar changed the title ~~JSpecify: fixing buildWithNullAway com.google.common package issue~~ JSpecify: fix crash with calls to static methods Nov 8, 2023

msridhar marked this pull request as ready for review November 8, 2023 18:01

Merge branch 'master' into jspecify_buildWithNullAway_guava_issue_abh…

fff179d

…ijitk

msridhar changed the title ~~JSpecify: fix crash with calls to static methods~~ JSpecify: fix crash with calls to static methods, and treat Guava as annotated when running buildWithNullAway Nov 8, 2023

msridhar changed the title ~~JSpecify: fix crash with calls to static methods, and treat Guava as annotated when running buildWithNullAway~~ JSpecify: fix crash with calls to static methods Nov 8, 2023

msridhar reviewed Nov 8, 2023

View reviewed changes

akulk022 added 3 commits November 8, 2023 11:29

changing the test case to be generic for static method calls instead …

f04f43c

…of being specific to guava ImmutableSet

Merge remote-tracking branch 'origin/jspecify_buildWithNullAway_guava…

0d1c44f

…_issue_abhijitk' into jspecify_buildWithNullAway_guava_issue_abhijitk

Removing com.google.common from annotated packages to add in a future…

54bc881

… commit

msridhar reviewed Nov 8, 2023

View reviewed changes

msridhar approved these changes Nov 8, 2023

View reviewed changes

msridhar merged commit 5aeb32c into uber:master Nov 8, 2023
9 checks passed

msridhar mentioned this pull request Nov 8, 2023

JSpecify mode: number of types arguments mismatch #791

Closed

akulk022 mentioned this pull request Nov 9, 2023

JSpecify: adding com.google.common to annotated packages in build.gradle #857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSpecify: fix crash with calls to static methods #856

JSpecify: fix crash with calls to static methods #856

akulk022 commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

msridhar left a comment

msridhar Nov 8, 2023

akulk022 Nov 8, 2023 •

edited

Loading

msridhar Nov 8, 2023

akulk022 Nov 8, 2023

msridhar Nov 8, 2023

akulk022 Nov 8, 2023

JSpecify: fix crash with calls to static methods #856

JSpecify: fix crash with calls to static methods #856

Conversation

akulk022 commented Nov 7, 2023 • edited Loading

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

msridhar left a comment

Choose a reason for hiding this comment

msridhar Nov 8, 2023

Choose a reason for hiding this comment

akulk022 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

msridhar Nov 8, 2023

Choose a reason for hiding this comment

akulk022 Nov 8, 2023

Choose a reason for hiding this comment

msridhar Nov 8, 2023

Choose a reason for hiding this comment

akulk022 Nov 8, 2023

Choose a reason for hiding this comment

akulk022 commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

akulk022 Nov 8, 2023 •

edited

Loading