-
Notifications
You must be signed in to change notification settings - Fork 356
Enable JSpecify JDK models (under a flag) #1641
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.
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
Changes from 24 commits
fe3fe77
1a103a1
5f46f67
8afe137
3efa027
128234b
817d8fa
6a23e9f
f71c02c
ba387fe
ac21f02
83741b9
59c53ea
cd954a8
595fc60
cece53e
f5df46f
89c37e6
06c0b91
b1ef9c1
495c522
a112c0e
ba233ad
b80333f
2a1b233
cb47be6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # 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`: | ||
| ``` | ||
| $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. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is due to the new JDK models (which model the return of |
||
| if (methodName.equals(enclosingClass.substring(enclosingClass.lastIndexOf('.') + 1))) { | ||
| // constructor | ||
| methodName = "<init>"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<LibraryModels> 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<Boolean> makeOptimizedBoolLookup( | |
| makeOptimizedNestedAnnotationLookup( | ||
| Names names, | ||
| ImmutableMap<MethodRef, ImmutableSetMultimap<Integer, NestedAnnotationInfo>> refs) { | ||
| return makeOptimizedLookup( | ||
| names, refs.keySet(), ref -> NullabilityUtil.castToNonNull(refs.get(ref))); | ||
| return makeOptimizedLookup(names, refs.keySet(), ref -> castToNonNull(refs.get(ref))); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is due to the new static import |
||
| } | ||
|
|
||
| private <T> NameIndexedMap<T> 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<String, Integer> methodTypeParamNullableUpperBoundCache; | ||
| private final Map<String, SetMultimap<Integer, NestedAnnotationInfo>> nestedAnnotationInfo; | ||
|
|
||
| ExternalStubxLibraryModels() { | ||
| ExternalStubxLibraryModels(boolean isJarInferEnabled, boolean isJSpecifyJDKEnabled) { | ||
| String libraryModelLogName = "LM"; | ||
| StubxCacheUtil cacheUtil = new StubxCacheUtil(libraryModelLogName); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| // 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) { | ||
| astubxLoadLog( | ||
| "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); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
msridhar marked this conversation as resolved.
|
||
|
|
||
| argAnnotCache = cacheUtil.getArgAnnotCache(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,9 @@ private void loadStubxFiles() { | |
| for (String astubxPath : provider.pathsToStubxFiles()) { | ||
| Class<? extends JarInferStubxProvider> providerClass = provider.getClass(); | ||
| InputStream stubxInputStream = providerClass.getResourceAsStream(astubxPath); | ||
| if (stubxInputStream == null) { | ||
| throw new RuntimeException("could not get input stream for " + astubxPath); | ||
| } | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is due to the new JDK models (which model the return of |
||
| String stubxLocation = providerClass + ":" + astubxPath; | ||
| try { | ||
| parseStubStream(stubxInputStream, stubxLocation); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
|
msridhar marked this conversation as resolved.
|
||
| } | ||
|
|
||
| @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<String> 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<String> list) { | ||
| // no warning, since List.add() is unmarked without the model | ||
| list.add(null); | ||
| list.get(0).toString(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.