diff --git a/jdk-javac-plugin/README.md b/jdk-javac-plugin/README.md new file mode 100644 index 0000000000..634e9ebb5b --- /dev/null +++ b/jdk-javac-plugin/README.md @@ -0,0 +1,39 @@ +# Javac plugin for generating JSpecify JDK astubx file + +This module and others contain logic to generate an astubx file from the +[annotated JSpecify JDK](https://github.com/jspecify/jdk). The generation works +in two stages: + +1. This module provides a javac plugin that gets injected into the build of the +JSpecify JDK. It generates `.json` files capturing the nullability annotations +in the JDK. +2. We have a separate astubx generator (main entrypoint: +`com.uber.nullaway.jdkannotations.AstubxGeneratorCLI`) that turns the `.json` +files into an `.astubx` file. + +Here are the current steps to (re-)generate the file (admittedly janky, we will +work to improve them). + +1. Build this module and the `astubx-generator-cli` module: +`./gradlew :jdk-javac-plugin:build :jdk-annotations:astubx-generator-cli:build` +2. Clone [this fork](https://github.com/msridhar/jdk) of the JSpecify JDK, and +check out the `test` branch. +3. In the jdk repo, edit these lines of `make/common/JavaCompilation.gmk`: + +```bash + $1_API_DIGEST_FLAGS += -Xplugin:"NullnessAnnotationSerializer /tmp" + $1_AUGMENTED_CLASSPATH += /Users/msridhar/git-repos/NullAway/jdk-javac-plugin/build/libs/jdk-javac-plugin-all.jar +``` + +On the first line, you can change `/tmp` to whatever directory should be used to +store the `.json` files generated by the javac plugin. On the second line, +change the absolute path to point to the `jdk-javac-plugin-all.jar` file under +your NullAway repo. +4. In the jdk repo, run: `make clean && make jdk`. (You may need to run +`configure` first.) This should exit successfully and generate the json files. +5. Run the `AstubxGeneratorCLI` main method (e.g., you can run it from within +IntelliJ). It takes two arguments. The first is the directory containing the +json files, and the second is where the output astubx file should be placed. +The output file will be named `output.astubx`. +6. Copy the `output.astubx` file from the previous step to +`nullaway/src/main/resources/jspecify-jdk.astubx` under the NullAway repo. diff --git a/nullaway/build.gradle b/nullaway/build.gradle index b191920257..502270287b 100644 --- a/nullaway/build.gradle +++ b/nullaway/build.gradle @@ -250,6 +250,7 @@ tasks.register('buildWithNullAway', JavaCompile) { option("NullAway:CheckContracts") option("NullAway:JSpecifyMode") option("NullAway:HandleWildcardGenerics") + option("NullAway:JSpecifyJDKModels") } // Make sure the jar has already been built dependsOn 'jar' diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java index 4e54d71386..3856151846 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Config.java +++ b/nullaway/src/main/java/com/uber/nullaway/Config.java @@ -285,6 +285,13 @@ public interface Config { */ boolean isJarInferEnabled(); + /** + * Checks if JSpecify JDK models should be enabled. + * + * @return true if JSpecify JDK models should be enabled, false otherwise + */ + boolean isJSpecifyJDKModels(); + /** * Gets the URL to show with NullAway error messages. * diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java index 94f9532dda..fa88033bee 100644 --- a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java @@ -204,6 +204,11 @@ public boolean isJarInferEnabled() { throw new IllegalStateException(ERROR_MESSAGE); } + @Override + public boolean isJSpecifyJDKModels() { + throw new IllegalStateException(ERROR_MESSAGE); + } + @Override public String getErrorURL() { throw new IllegalStateException(ERROR_MESSAGE); diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index 2d5514db34..0728d441d0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -87,6 +87,8 @@ final class ErrorProneCLIFlagsConfig implements Config { /** --- JarInfer configs --- */ static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled"; + static final String FL_JSPECIFY_JDK_ENABLED = EP_FL_NAMESPACE + ":JSpecifyJDKModels"; + static final String FL_ERROR_URL = EP_FL_NAMESPACE + ":ErrorURL"; /** --- Serialization configs --- */ @@ -247,6 +249,8 @@ final class ErrorProneCLIFlagsConfig implements Config { /** --- JarInfer configs --- */ private final boolean jarInferEnabled; + private final boolean jspecifyJDKModelsEnabled; + private final String errorURL; /** --- Fully qualified names of custom nonnull/nullable annotation --- */ @@ -325,6 +329,11 @@ final class ErrorProneCLIFlagsConfig implements Config { /* --- JarInfer configs --- */ jarInferEnabled = flags.getBoolean(FL_JI_ENABLED).orElse(false); + jspecifyJDKModelsEnabled = flags.getBoolean(FL_JSPECIFY_JDK_ENABLED).orElse(false); + if (jspecifyJDKModelsEnabled && !jspecifyMode) { + throw new IllegalStateException( + "-XepOpt:%s should only be set in JSpecify mode".formatted(FL_JSPECIFY_JDK_ENABLED)); + } errorURL = flags.get(FL_ERROR_URL).orElse(DEFAULT_URL); if (acknowledgeAndroidRecent && !isAcknowledgeRestrictive) { throw new IllegalStateException( @@ -581,6 +590,11 @@ public boolean isJarInferEnabled() { return jarInferEnabled; } + @Override + public boolean isJSpecifyJDKModels() { + return jspecifyJDKModelsEnabled; + } + @Override public String getErrorURL() { return errorURL; diff --git a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java index 03f13776dc..ce0b296cb1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java +++ b/nullaway/src/main/java/com/uber/nullaway/LibraryModels.java @@ -22,6 +22,8 @@ package com.uber.nullaway; +import static com.uber.nullaway.NullabilityUtil.castToNonNull; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -255,7 +257,7 @@ private MethodRef(String enclosingClass, String methodName, String fullMethodSig public static MethodRef methodRef(String enclosingClass, String methodSignature) { Matcher matcher = METHOD_SIG_PATTERN.matcher(methodSignature); if (matcher.find()) { - String methodName = matcher.group(2); + String methodName = castToNonNull(matcher.group(2)); if (methodName.equals(enclosingClass.substring(enclosingClass.lastIndexOf('.') + 1))) { // constructor methodName = ""; diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java index 676923b926..9a30555c8b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java @@ -24,6 +24,7 @@ import static com.uber.nullaway.LibraryModels.FieldRef.fieldRef; import static com.uber.nullaway.LibraryModels.MethodRef.methodRef; +import static com.uber.nullaway.NullabilityUtil.castToNonNull; import static com.uber.nullaway.Nullness.NONNULL; import static com.uber.nullaway.Nullness.NULLABLE; import static com.uber.nullaway.librarymodel.NestedAnnotationInfo.TypePathEntry.Kind.ARRAY_ELEMENT; @@ -55,7 +56,6 @@ import com.uber.nullaway.LibraryModels.MethodRef; import com.uber.nullaway.MethodParameterNullness; import com.uber.nullaway.NullAway; -import com.uber.nullaway.NullabilityUtil; import com.uber.nullaway.Nullness; import com.uber.nullaway.annotations.Initializer; import com.uber.nullaway.dataflow.AccessPath; @@ -65,7 +65,9 @@ import com.uber.nullaway.librarymodel.AddAnnotationToNestedTypeVisitor; import com.uber.nullaway.librarymodel.NestedAnnotationInfo; import com.uber.nullaway.librarymodel.NestedAnnotationInfo.Annotation; +import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.HashSet; import java.util.LinkedHashMap; @@ -532,8 +534,9 @@ private static LibraryModels loadLibraryModels(Config config) { ServiceLoader.load(LibraryModels.class, LibraryModels.class.getClassLoader()); ImmutableSet.Builder libModelsBuilder = new ImmutableSet.Builder<>(); libModelsBuilder.add(new DefaultLibraryModels(config)).addAll(externalLibraryModels); - if (config.isJarInferEnabled()) { - libModelsBuilder.add(new ExternalStubxLibraryModels()); + if (config.isJarInferEnabled() || config.isJSpecifyJDKModels()) { + libModelsBuilder.add( + new ExternalStubxLibraryModels(config.isJarInferEnabled(), config.isJSpecifyJDKModels())); } return new CombinedLibraryModels(libModelsBuilder.build(), config); } @@ -1587,8 +1590,7 @@ private NameIndexedMap makeOptimizedBoolLookup( makeOptimizedNestedAnnotationLookup( Names names, ImmutableMap> refs) { - return makeOptimizedLookup( - names, refs.keySet(), ref -> NullabilityUtil.castToNonNull(refs.get(ref))); + return makeOptimizedLookup(names, refs.keySet(), ref -> castToNonNull(refs.get(ref))); } private NameIndexedMap makeOptimizedLookup( @@ -1640,6 +1642,9 @@ private static class ExternalStubxLibraryModels implements LibraryModels { /** astubx file name used in our Android SDK JarInfer models */ private static final String ANDROID_ASTUBX_LOCATION = "jarinfer.astubx"; + /** astubx file name used for the JSpecify JDK models */ + private static final String JSPECIFY_JDK_ASTUBX_FILENAME = "jspecify-jdk.astubx"; + /** Class we expect to be present in a jar containing Android SDK JarInfer models */ private static final String ANDROID_MODEL_CLASS = "com.uber.nullaway.jarinfer.AndroidJarInferModels"; @@ -1650,26 +1655,44 @@ private static class ExternalStubxLibraryModels implements LibraryModels { private final Multimap methodTypeParamNullableUpperBoundCache; private final Map> nestedAnnotationInfo; - ExternalStubxLibraryModels() { + ExternalStubxLibraryModels(boolean isJarInferEnabled, boolean isJSpecifyJDKEnabled) { String libraryModelLogName = "LM"; StubxCacheUtil cacheUtil = new StubxCacheUtil(libraryModelLogName); - // hardcoded loading of stubx files from android-jarinfer-models-sdkXX artifacts - try { - InputStream androidStubxIS = - Class.forName(ANDROID_MODEL_CLASS) - .getClassLoader() - .getResourceAsStream(ANDROID_ASTUBX_LOCATION); - if (androidStubxIS != null) { - cacheUtil.parseStubStream(androidStubxIS, "android.jar: " + ANDROID_ASTUBX_LOCATION); - astubxLoadLog("Loaded Android RT models."); + if (isJarInferEnabled) { + // hardcoded loading of stubx files from android-jarinfer-models-sdkXX artifacts + try (InputStream androidStubxIS = + castToNonNull(Class.forName(ANDROID_MODEL_CLASS).getClassLoader()) + .getResourceAsStream(ANDROID_ASTUBX_LOCATION)) { + if (androidStubxIS != null) { + cacheUtil.parseStubStream(androidStubxIS, "android.jar: " + ANDROID_ASTUBX_LOCATION); + astubxLoadLog("Loaded Android RT models."); + } + } catch (ClassNotFoundException e) { + astubxLoadLog( + "Cannot find Android RT models locator class." + + " This is expected if not in an Android project, or the Android SDK JarInfer models Jar has not been set up for this build."); + + } catch (IOException e) { + astubxLoadLog("Loading Android RT models failed: " + e.getMessage()); } - } catch (ClassNotFoundException e) { - astubxLoadLog( - "Cannot find Android RT models locator class." - + " This is expected if not in an Android project, or the Android SDK JarInfer models Jar has not been set up for this build."); + } - } catch (Exception e) { - astubxLoadLog("Cannot load Android RT models."); + if (isJSpecifyJDKEnabled) { + // hardcoded loading of JSpecify JDK astubx from jspecify-jdk.astubx + try (InputStream in = + castToNonNull(getClass().getClassLoader()) + .getResourceAsStream(JSPECIFY_JDK_ASTUBX_FILENAME)) { + if (in == null) { + throw new IllegalStateException( + "JDK astubx model not found on classpath: %s" + .formatted(JSPECIFY_JDK_ASTUBX_FILENAME)); + } else { + cacheUtil.parseStubStream(in, JSPECIFY_JDK_ASTUBX_FILENAME); + astubxLoadLog("Loaded JDK astubx model."); + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } } argAnnotCache = cacheUtil.getArgAnnotCache(); diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java b/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java index 51907f5335..fd10c80c90 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/StubxCacheUtil.java @@ -127,9 +127,11 @@ private void loadStubxFiles() { for (JarInferStubxProvider provider : astubxProviders) { for (String astubxPath : provider.pathsToStubxFiles()) { Class providerClass = provider.getClass(); - InputStream stubxInputStream = providerClass.getResourceAsStream(astubxPath); String stubxLocation = providerClass + ":" + astubxPath; - try { + try (InputStream stubxInputStream = providerClass.getResourceAsStream(astubxPath)) { + if (stubxInputStream == null) { + throw new RuntimeException("could not get input stream for " + astubxPath); + } parseStubStream(stubxInputStream, stubxLocation); LOG(DEBUG, "DEBUG", "loaded stubx file " + stubxLocation); } catch (IOException e) { diff --git a/nullaway/src/main/resources/jspecify-jdk.astubx b/nullaway/src/main/resources/jspecify-jdk.astubx new file mode 100644 index 0000000000..e7e62220d3 Binary files /dev/null and b/nullaway/src/main/resources/jspecify-jdk.astubx differ diff --git a/nullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.java b/nullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.java index ece58dff55..ac808052f4 100644 --- a/nullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/ErrorProneCLIFlagsConfigTest.java @@ -72,4 +72,15 @@ public void missingTypeAnnotationSymbolFlagForJSpecifyModeOnOlderJDK() { assertTrue( e.getMessage().contains("Running NullAway in JSpecify mode requires either JDK 22+")); } + + @Test + public void jspecifyJDKOutsideJSpecifyMode() { + CompilationTestHelper compilationTestHelper = + makeTestHelperWithArgs( + List.of( + "-XepOpt:NullAway:OnlyNullMarked", "-XepOpt:NullAway:JSpecifyJDKModels=true")) + .addSourceLines("Stub.java", "package com.uber; class Stub {}"); + AssertionError e = assertThrows(AssertionError.class, () -> compilationTestHelper.doTest()); + assertTrue(e.getMessage().contains("should only be set in JSpecify mode")); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java b/nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java new file mode 100644 index 0000000000..5fe77cf758 --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java @@ -0,0 +1,94 @@ +package com.uber.nullaway; + +import com.google.errorprone.CompilationTestHelper; +import com.uber.nullaway.generics.JSpecifyJavacConfig; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class JSpecifyJDKModelsTest extends NullAwayTestsBase { + + @Test + public void modelsDisabledDoesNotLoadAstubxModel() { + CompilationTestHelper compilationTestHelper = + makeTestHelperWithArgs(List.of("-XepOpt:NullAway:AnnotatedPackages=foo")) + .addSourceLines( + "Test.java", + """ + package foo; + import javax.naming.directory.Attributes; + import org.jspecify.annotations.NullMarked; + @NullMarked + class Test { + void use(Attributes attrs) { + // Attributes.get returns @Nullable in the models, but since we don't load + // models here, we get no warning + attrs.get("key").toString(); + } + } + """); + compilationTestHelper.doTest(); + } + + @Test + public void listContainingNullsWithModel() { + makeTestHelperWithArgs( + JSpecifyJavacConfig.withJSpecifyModeArgs( + List.of( + "-XepOpt:NullAway:AnnotatedPackages=foo", + "-XepOpt:NullAway:JSpecifyJDKModels=true"))) + .addSourceLines( + "Test.java", + """ + package foo; + import java.util.List; + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.Nullable; + @NullMarked + class Test { + void testNullableContents(List<@Nullable String> list) { + list.add(null); + // BUG: Diagnostic contains: dereferenced expression 'list.get(0)' is @Nullable + list.get(0).toString(); + } + void testNonNullContents(List list) { + // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required + list.add(null); + list.get(0).toString(); + } + } + """) + .doTest(); + } + + @Test + public void listContainingNullsWithoutModel() { + makeTestHelperWithArgs( + JSpecifyJavacConfig.withJSpecifyModeArgs( + List.of("-XepOpt:NullAway:AnnotatedPackages=foo"))) + .addSourceLines( + "Test.java", + """ + package foo; + import java.util.List; + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.Nullable; + @NullMarked + class Test { + void use(List<@Nullable String> list) { + list.add(null); + // no warning, since List.get() is unmarked without the model + list.get(0).toString(); + } + void testNonNullContents(List list) { + // no warning, since List.add() is unmarked without the model + list.add(null); + list.get(0).toString(); + } + } + """) + .doTest(); + } +}