Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSpecify: fix crash with calls to static methods #856

Merged
merged 6 commits into from
Nov 8, 2023
+30 −2
Merged

JSpecify: fix crash with calls to static methods #856

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25963a9
Adding com.google.common to annotatedpackages and fixing the NPE we g…
akulk022 Nov 7, 2023
ffb680e
Adding scenario for ImmutableSet.Of() with a param to the test case t…
akulk022 Nov 7, 2023
fff179d
Merge branch 'master' into jspecify_buildWithNullAway_guava_issue_abh…
msridhar Nov 8, 2023
f04f43c
changing the test case to be generic for static method calls instead …
akulk022 Nov 8, 2023
0d1c44f
Merge remote-tracking branch 'origin/jspecify_buildWithNullAway_guava…
akulk022 Nov 8, 2023
54bc881
Removing com.google.common from annotated packages to add in a future…
akulk022 Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nullaway/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I have removed it from this branch.

option("NullAway:CastToNonNullMethod", "com.uber.nullaway.NullabilityUtil.castToNonNull")
option("NullAway:CheckOptionalEmptiness")
option("NullAway:AcknowledgeRestrictiveAnnotations")
Expand Down
4 changes: 2 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/GenericsChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ public static Nullness getGenericReturnNullnessAtInvocation(
MethodInvocationTree tree,
VisitorState state,
Config config) {
if (!(tree.getMethodSelect() instanceof MemberSelectTree)) {
if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) {
return Nullness.NONNULL;
}
Type methodReceiverType =
Expand Down Expand Up @@ -834,7 +834,7 @@ public static Nullness getGenericParameterNullnessAtInvocation(
MethodInvocationTree tree,
VisitorState state,
Config config) {
if (!(tree.getMethodSelect() instanceof MemberSelectTree)) {
if (!(tree.getMethodSelect() instanceof MemberSelectTree) || invokedMethodSymbol.isStatic()) {
return Nullness.NONNULL;
}
Type enclosingType =
Expand Down
28 changes: 28 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1494,12 +1494,40 @@ public void interactionWithContracts() {
.doTest();
}

@Test
public void testForGuavaRawTypeBuildIssue() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@akulk022 akulk022 Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

makeHelperWithGuava()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.google.common.collect.ImmutableSet;",
"class Test {",
" static void funcImmutableSet(ImmutableSet<Object> is){",
" }",
" static void testNegative() {",
" funcImmutableSet(ImmutableSet.of());",
" }",
" static void testNegative2() {",
" funcImmutableSet(ImmutableSet.of(new Object()));",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
"-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:JSpecifyMode=true"));
}

private CompilationTestHelper makeHelperWithGuava() {
return makeTestHelperWithArgs(
Arrays.asList(
"-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common",
"-XepOpt:NullAway:JSpecifyMode=true"));
}

private CompilationTestHelper makeHelperWithoutJSpecifyMode() {
return makeTestHelperWithArgs(Arrays.asList("-XepOpt:NullAway:AnnotatedPackages=com.uber"));
}
Expand Down