-
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
Merged
Merged
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
fe3fe77
make new option for jdk infer
haewiful 1a103a1
make library model load jdk nullness data when option is true
haewiful 5f46f67
test for new option
haewiful 8afe137
jdk nullness data astubx file
haewiful 3efa027
typo
haewiful 128234b
new output.astubx
haewiful 817d8fa
rename flag
msridhar 6a23e9f
rename test
msridhar f71c02c
switch to text blocks
msridhar ba387fe
failing test
msridhar ac21f02
new output.astubx
haewiful 83741b9
only catch IOException when parsing jdk astubx file
haewiful 59c53ea
more tests
msridhar cd954a8
re-throw IOException rather than swallowing it
msridhar 595fc60
WIP
msridhar cece53e
remove now unnecessary hacks
msridhar f5df46f
update astubx file
msridhar 89c37e6
fixes
msridhar 06c0b91
more
msridhar b1ef9c1
only allow models in JSpecify mode
msridhar 495c522
narrower catch
msridhar a112c0e
extract constant and relocate astubx file
msridhar ba233ad
add README file and tweaks
msridhar b80333f
comment in test
msridhar 2a1b233
coderabbit feedback
msridhar cb47be6
coderabbit
msridhar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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>"; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) { | ||
| 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); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
msridhar marked this conversation as resolved.
|
||
|
|
||
| argAnnotCache = cacheUtil.getArgAnnotCache(); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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(); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.