-
Notifications
You must be signed in to change notification settings - Fork 330
Option that loads nullness information of JDK package #1472
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
02ec987
4bccdd0
53b9ffc
c34a866
f6c1c63
4eaf834
e2c2952
146c824
a4b081f
03bd28a
cad6808
3857cf3
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 |
|---|---|---|
|
|
@@ -478,8 +478,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); | ||
| } | ||
|
|
@@ -1560,26 +1561,42 @@ 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); | ||
| // 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) { | ||
| 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."); | ||
| } | ||
| } 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."); | ||
| } | ||
| } 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."); | ||
| } | ||
|
Comment on lines
+1568
to
+1586
Contributor
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. Resource leak: The Android This contrasts with the JDK loading block immediately below (lines 1590–1596) which correctly uses try-with-resources. Streams backed by JAR entries hold open JAR file connections and file descriptors at the JDK level; in a long-running Gradle daemon these accumulate. 🔒 Proposed fix — use nested try-with-resources to match the JDK path if (isJarInferEnabled) {
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.");
+ ClassLoader androidLoader = Class.forName(ANDROID_MODEL_CLASS).getClassLoader();
+ try (InputStream androidStubxIS =
+ androidLoader.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 (Exception e) {
astubxLoadLog("Cannot load Android RT models.");
}
}🤖 Prompt for AI Agents |
||
|
|
||
| } catch (Exception e) { | ||
| astubxLoadLog("Cannot load Android RT models."); | ||
| // hardcoded loading of stubx files from jdk nullness inferred output.astubx | ||
| if (isJSpecifyJDKEnabled) { | ||
| try (InputStream in = getClass().getClassLoader().getResourceAsStream("output.astubx")) { | ||
| if (in == null) { | ||
| astubxLoadLog("JDK astubx model not found on classpath: output.astubx"); | ||
| } else { | ||
| cacheUtil.parseStubStream(in, "output.astubx"); | ||
| astubxLoadLog("Loaded JDK astubx model."); | ||
| } | ||
| } catch (Exception e) { | ||
| astubxLoadLog("Failed to load JDK astubx: " + e); | ||
| } | ||
|
Comment on lines
+1589
to
+1599
Contributor
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. Silent failure when When the user explicitly opts in via Same concern applies to the Consider emitting an unconditional warning to 💡 Suggested minimal fix if (isJSpecifyJDKEnabled) {
try (InputStream in = getClass().getClassLoader().getResourceAsStream("output.astubx")) {
if (in == null) {
- astubxLoadLog("JDK astubx model not found on classpath: output.astubx");
+ // Always warn: user explicitly enabled the flag but the resource is absent
+ System.err.println(
+ "[NullAway] WARNING: JSpecifyJDKModels is enabled but output.astubx was not found on the classpath.");
} else {
cacheUtil.parseStubStream(in, "output.astubx");
astubxLoadLog("Loaded JDK astubx model.");
}
} catch (Exception e) {
- astubxLoadLog("Failed to load JDK astubx: " + e);
+ System.err.println("[NullAway] WARNING: Failed to load JDK astubx model: " + e);
}
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| argAnnotCache = cacheUtil.getArgAnnotCache(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| package com.uber.nullaway; | ||
|
|
||
| import com.google.errorprone.CompilationTestHelper; | ||
| 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 modelsEnabledLoadsAstubxModel() { | ||
| CompilationTestHelper compilationTestHelper = | ||
| makeTestHelperWithArgs( | ||
| List.of( | ||
| "-XepOpt:NullAway:AnnotatedPackages=foo", | ||
| "-XepOpt:NullAway:JSpecifyJDKModels=true")) | ||
| .addSourceLines( | ||
| "Test.java", | ||
| """ | ||
| package foo; | ||
| import javax.naming.directory.Attributes; | ||
| import org.jspecify.annotations.NullMarked; | ||
| @NullMarked | ||
| class Test { | ||
| void use(Attributes attrs) { | ||
| // BUG: Diagnostic contains: @Nullable | ||
| attrs.get("key").toString(); | ||
| } | ||
| } | ||
| """); | ||
| compilationTestHelper.doTest(); | ||
| } | ||
|
|
||
| @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) { | ||
| attrs.get("key").toString(); | ||
| } | ||
| } | ||
| """); | ||
| compilationTestHelper.doTest(); | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+36
to
+54
Contributor
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. 🧹 Nitpick | 🔵 Trivial Negative test is implicitly fragile — explain the assumption.
Consider adding a brief comment to document the assumption: 📝 Suggested comment to document assumption `@Test`
public void modelsDisabledDoesNotLoadAstubxModel() {
+ // Without the JSpecifyJDKModels flag, no NullAway model annotates
+ // Attributes.get() as `@Nullable`, so the dereference is not flagged.
CompilationTestHelper compilationTestHelper =🤖 Prompt for AI Agents |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Nitpick:
FL_JSPECIFY_JDK_ENABLEDis grouped under the JarInfer section comment.FL_JSPECIFY_JDK_ENABLED(and its companion fieldjspecifyJDKModelsEnabledat line 252) is placed under/** --- JarInfer configs --- */, but JSpecify JDK models are a conceptually distinct feature. Consider either adding a separate section comment or renaming the existing one.♻️ Suggested section update
And likewise for the field comment at line 249 and the constructor comment at line 338.
📝 Committable suggestion
🤖 Prompt for AI Agents