From 75a5427071c20292f83cf00b8258f45beaaf2b17 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Tue, 2 Aug 2022 16:17:46 -0400 Subject: [PATCH 1/5] Add Guava 31+ support by treating @ParametricNullness as @Nullable Guava versions from 31.0 onwards do away with annotating methods as returning or taking `@Nullable` if nullability depends on a type parameter. Instead adopting JSpecify semantics. For compatibility (with Kotlin, among other tools), Guava still marks such returns and arguments specially as `@ParametricNullness`. While imprecise, we can handle Guava as annotated code to at least the same level as we did previously, by treating `@ParametricNullness` as an alias for `@Nullable`, until we have full type parameter / generics support within NullAway. To test this change without bumping our own Guava dependency, we intoduce a new test target: `guava-recent-unit-tests`. Additionally, since there are actually multiple instances of `@ParametricNullness` in Guava (one per subpackage), we hard-code a check for `com.google.common.*.ParametricNullness` in our core `isNullableAnnotation(...)` check. An alternative would be to extend `-XepOpt:NullAway:CustomNullableAnnotations` to allow simple names or regular expressions, but that would make the mechanism more costly in common case. See: --- guava-recent-unit-tests/build.gradle | 49 ++++++++++ .../NullAwayGuavaParametricNullnessTests.java | 92 +++++++++++++++++++ .../main/java/com/uber/nullaway/Nullness.java | 3 + settings.gradle | 1 + 4 files changed, 145 insertions(+) create mode 100644 guava-recent-unit-tests/build.gradle create mode 100644 guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java diff --git a/guava-recent-unit-tests/build.gradle b/guava-recent-unit-tests/build.gradle new file mode 100644 index 0000000000..abc6304038 --- /dev/null +++ b/guava-recent-unit-tests/build.gradle @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2021. Uber Technologies + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +plugins { + id 'java-library' + id 'nullaway.jacoco-conventions' +} + +// We need this separate build target to test newer versions of Guava +// (e.g. 31+) than that which NullAway currently depends on. + +dependencies { + testImplementation project(":nullaway") + testImplementation deps.test.junit4 + testImplementation(deps.build.errorProneTestHelpers) { + exclude group: "junit", module: "junit" + } + testImplementation deps.build.jsr305Annotations + testImplementation "com.google.guava:guava:31.1-jre" +} + +test { + maxHeapSize = "1024m" + // to expose necessary JDK types on JDK 9+; see https://errorprone.info/docs/installation#java-9-and-newer + jvmArgs += [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + ] +} diff --git a/guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java b/guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java new file mode 100644 index 0000000000..9dc64e6c4c --- /dev/null +++ b/guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2022 Uber Technologies, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +import com.google.errorprone.CompilationTestHelper; +import com.uber.nullaway.NullAway; +import java.util.Arrays; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +public class NullAwayGuavaParametricNullnessTests { + @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder(); + + private CompilationTestHelper defaultCompilationHelper; + + @Before + public void setup() { + defaultCompilationHelper = + CompilationTestHelper.newInstance(NullAway.class, getClass()) + .setArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common")); + } + + @Test + public void testFutureCallbackParametricNullness() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.google.common.util.concurrent.FutureCallback;", + "import javax.annotation.Nullable;", + "class Test {", + " public static FutureCallback wrapFutureCallback(FutureCallback futureCallback) {", + " return new FutureCallback() {", + " @Override", + " public void onSuccess(@Nullable T result) {", + " futureCallback.onSuccess(result);", + " }", + " @Override", + " public void onFailure(Throwable throwable) {", + " futureCallback.onFailure(throwable);", + " }", + " };", + " }", + "}") + .doTest(); + } + + @Test + public void testIterableParametricNullness() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.Iterables;", + "import javax.annotation.Nullable;", + "class Test {", + " public static String test1() {", + " // BUG: Diagnostic contains: returning @Nullable expression", + " return Iterables.getFirst(ImmutableList.of(), null);", + " }", + " public static @Nullable String test2() {", + " return Iterables.getFirst(ImmutableList.of(), null);", + " }", + "}") + .doTest(); + } +} diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index d8a042ae56..7292adcde8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -156,6 +156,9 @@ public static boolean isNullableAnnotation(String annotName, Config config) { || annotName.endsWith(".checkerframework.checker.nullness.compatqual.NullableDecl") // matches javax.annotation.CheckForNull and edu.umd.cs.findbugs.annotations.CheckForNull || annotName.endsWith(".CheckForNull") + // matches any of the multiple @ParametricNullness annotations used within Guava + // (see https://github.com/google/guava/issues/6126) + || (annotName.endsWith(".ParametricNullness") && annotName.startsWith("com.google.common.")) || (config.acknowledgeAndroidRecent() && annotName.equals("androidx.annotation.RecentlyNullable")) || config.isCustomNullableAnnotation(annotName); diff --git a/settings.gradle b/settings.gradle index 6d5619e670..6ac4955a6e 100644 --- a/settings.gradle +++ b/settings.gradle @@ -22,6 +22,7 @@ include ':jar-infer:jar-infer-cli' include ':jar-infer:test-java-lib-jarinfer' include ':jar-infer:nullaway-integration-test' include ':jmh' +include ':guava-recent-unit-tests' include ':jdk17-unit-tests' // The following modules require JDK 11 and fail during Gradle configuration on JDK 8 From 5d6c1c930888d22966db59db1e0349d987ed80a1 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Tue, 2 Aug 2022 16:25:15 -0400 Subject: [PATCH 2/5] Minor fix --- nullaway/src/main/java/com/uber/nullaway/Nullness.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index 7292adcde8..4587064972 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -158,6 +158,9 @@ public static boolean isNullableAnnotation(String annotName, Config config) { || annotName.endsWith(".CheckForNull") // matches any of the multiple @ParametricNullness annotations used within Guava // (see https://github.com/google/guava/issues/6126) + // We check the simple name first and the package prefix second for boolean short + // circuiting, as Guava includes + // many annotations || (annotName.endsWith(".ParametricNullness") && annotName.startsWith("com.google.common.")) || (config.acknowledgeAndroidRecent() && annotName.equals("androidx.annotation.RecentlyNullable")) From 76b23de69d90de0459b2d134314b7e9b0f319253 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Tue, 2 Aug 2022 16:30:49 -0400 Subject: [PATCH 3/5] Un-time-travel build.gradle --- guava-recent-unit-tests/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guava-recent-unit-tests/build.gradle b/guava-recent-unit-tests/build.gradle index abc6304038..0a9227a59c 100644 --- a/guava-recent-unit-tests/build.gradle +++ b/guava-recent-unit-tests/build.gradle @@ -1,5 +1,5 @@ /* - * Copyright (C) 2021. Uber Technologies + * Copyright (C) 2022. Uber Technologies * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 8c5a38d4324937b419d181951ccd98432d8e99b6 Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Tue, 2 Aug 2022 16:41:15 -0400 Subject: [PATCH 4/5] Build/CI fixes, incl. code cov --- code-coverage-report/build.gradle | 1 + guava-recent-unit-tests/build.gradle | 17 ----------------- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/code-coverage-report/build.gradle b/code-coverage-report/build.gradle index c7cb38eb71..4dc990a048 100644 --- a/code-coverage-report/build.gradle +++ b/code-coverage-report/build.gradle @@ -91,5 +91,6 @@ dependencies { implementation project(':nullaway') implementation project(':jar-infer:jar-infer-lib') implementation project(':jar-infer:nullaway-integration-test') + implementation project(':guava-recent-unit-tests') implementation project(':jdk17-unit-tests') } diff --git a/guava-recent-unit-tests/build.gradle b/guava-recent-unit-tests/build.gradle index 0a9227a59c..73baaa9977 100644 --- a/guava-recent-unit-tests/build.gradle +++ b/guava-recent-unit-tests/build.gradle @@ -30,20 +30,3 @@ dependencies { testImplementation deps.build.jsr305Annotations testImplementation "com.google.guava:guava:31.1-jre" } - -test { - maxHeapSize = "1024m" - // to expose necessary JDK types on JDK 9+; see https://errorprone.info/docs/installation#java-9-and-newer - jvmArgs += [ - "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", - "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", - "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", - ] -} From 78d06d0d72aa73ece2008b158853702a74e6ee0c Mon Sep 17 00:00:00 2001 From: Lazaro Clapp Date: Tue, 2 Aug 2022 16:44:38 -0400 Subject: [PATCH 5/5] CI fixes. --- guava-recent-unit-tests/build.gradle | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/guava-recent-unit-tests/build.gradle b/guava-recent-unit-tests/build.gradle index 73baaa9977..f831222651 100644 --- a/guava-recent-unit-tests/build.gradle +++ b/guava-recent-unit-tests/build.gradle @@ -30,3 +30,26 @@ dependencies { testImplementation deps.build.jsr305Annotations testImplementation "com.google.guava:guava:31.1-jre" } + +test { + maxHeapSize = "1024m" + if (!JavaVersion.current().java9Compatible) { + jvmArgs "-Xbootclasspath/p:${configurations.errorproneJavac.asPath}" + } else { + // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer + jvmArgs += [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + // Accessed by Lombok tests + "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", + ] + } +}