Option that loads nullness information of JDK package#1472
Option that loads nullness information of JDK package#1472haewiful wants to merge 12 commits intouber:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Config API flag Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Line 1564: The constructor ExternalStubxLibraryModels(boolean
isJarInferEnabled, boolean isJDKInferEnabled) uses two positional booleans that
are easy to swap; change the API to accept a descriptive config object or
factory so callers pass named fields. Create a small value class (e.g.,
LibraryInferOptions or InferConfig) with boolean fields jarInferEnabled and
jdkInferEnabled, update ExternalStubxLibraryModels to take that single config
parameter, and update the existing call site to construct and pass the config
(or alternatively add clearly named static factory methods on
ExternalStubxLibraryModels like createWith(jarEnabled, jdkEnabled)). Ensure the
constructor and any usages are updated consistently.
- Around line 1588-1600: Fix the typo in the comment inside LibraryModelsHandler
near the JDK astubx loading block: change "infered" to "inferred" in the comment
that references hardcoded loading of stubx files when isJDKInferEnabled (the
block using getClass().getClassLoader().getResourceAsStream("output.astubx") and
cacheUtil.parseStubStream). Do not change resource names or logic—only update
the comment wording.
| private final Map<String, SetMultimap<Integer, NestedAnnotationInfo>> nestedAnnotationInfo; | ||
|
|
||
| ExternalStubxLibraryModels() { | ||
| ExternalStubxLibraryModels(boolean isJarInferEnabled, boolean isJDKInferEnabled) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Constructor now accepts two separate booleans — consider clarity.
Two positional boolean parameters (isJarInferEnabled, isJDKInferEnabled) can be accidentally swapped at call sites without a compiler error. This is a minor readability concern for now since there's only one call site (Line 483), but worth noting.
🤖 Prompt for AI Agents
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`
at line 1564, The constructor ExternalStubxLibraryModels(boolean
isJarInferEnabled, boolean isJDKInferEnabled) uses two positional booleans that
are easy to swap; change the API to accept a descriptive config object or
factory so callers pass named fields. Create a small value class (e.g.,
LibraryInferOptions or InferConfig) with boolean fields jarInferEnabled and
jdkInferEnabled, update ExternalStubxLibraryModels to take that single config
parameter, and update the existing call site to construct and pass the config
(or alternatively add clearly named static factory methods on
ExternalStubxLibraryModels like createWith(jarEnabled, jdkEnabled)). Ensure the
constructor and any usages are updated consistently.
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (2)
568-586:⚠️ Potential issue | 🟡 MinorWrap the Android
InputStreamin try-with-resources to prevent resource leaks.The
InputStreamobtained from the Android model class loader at line 1570 is never closed. AlthoughparseStubStreamdoes not close the stream internally, the JDK loading pattern at line 1590 correctly uses try-with-resources. Apply the same pattern to the Android code for consistency and to prevent the stream from leaking ifparseStubStreamthrows an exception.Proposed fix
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."); + 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) {
588-600:⚠️ Potential issue | 🟠 MajorGeneric resource name
output.astubxrisks classpath collisions.The resource name
"output.astubx"is not namespaced, so any other JAR on the classpath with the same resource name could shadow or be loaded instead of the intended JDK nullness data. Consider using a more specific name (e.g.,"com/uber/nullaway/jdk-nullness.astubx") to avoid ambiguity.The loading logic itself (try-with-resources, null check, exception handling) is well-structured.
#!/bin/bash # Check how/where output.astubx is generated and placed echo "=== Searching for output.astubx references ===" rg -n "output\.astubx" --type-not class -C 2 echo "" echo "=== Checking resource directories ===" fd "output.astubx" --type f echo "" echo "=== Checking build configs for resource placement ===" rg -n "output\.astubx" -g "*.gradle*" -g "*.xml" -C 3
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1472 +/- ##
============================================
- Coverage 88.46% 88.42% -0.04%
- Complexity 2757 2760 +3
============================================
Files 99 99
Lines 9196 9210 +14
Branches 1847 1850 +3
============================================
+ Hits 8135 8144 +9
- Misses 521 526 +5
Partials 540 540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java`:
- Around line 87-91: The FL_JSPECIFY_JDK_ENABLED constant is incorrectly grouped
under the JarInfer section; update the file to separate JSpecify-related flags
by either adding a new section comment like "/** --- JSpecify configs --- */"
above FL_JSPECIFY_JDK_ENABLED or renaming the existing JarInfer section to
reflect both features, and likewise move or add matching comments for the
jspecifyJDKModelsEnabled field and the constructor comment that references
JSpecify JDK models so all JSpecify symbols (FL_JSPECIFY_JDK_ENABLED,
jspecifyJDKModelsEnabled, and the constructor comment) are grouped under a
dedicated JSpecify section.
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Around line 1589-1599: When isJSpecifyJDKEnabled is true and
getClass().getClassLoader().getResourceAsStream("output.astubx") returns null or
cacheUtil.parseStubStream(...) throws, do not rely only on astubxLoadLog (which
is gated by DEBUG_ASTUBX_LOADING); instead emit an unconditional user-visible
warning. Update the block around isJSpecifyJDKEnabled /
getResourceAsStream("output.astubx") / cacheUtil.parseStubStream so that in the
in == null branch and in the catch (Exception e) branch you print a clear
warning (e.g., via System.err.println or the compiler/annotation-processing log
API such as Messager/Tree/Log) indicating JSpecifyJDKModels was enabled but
output.astubx was not found or failed to parse and include the exception message
in the parse-failure case; keep the existing astubxLoadLog calls for debug
logging.
- Around line 1568-1586: The InputStream androidStubxIS opened via
Class.forName(...).getClassLoader().getResourceAsStream(ANDROID_ASTUBX_LOCATION)
must be closed; change the code to obtain the Class (using
Class.forName(ANDROID_MODEL_CLASS)) and then open the resource inside a
try-with-resources so the stream is always closed (e.g. Class.forName(...) then
try (InputStream androidStubxIS =
clazz.getClassLoader().getResourceAsStream(ANDROID_ASTUBX_LOCATION)) { if
(androidStubxIS != null) { cacheUtil.parseStubStream(androidStubxIS,
"android.jar: " + ANDROID_ASTUBX_LOCATION); astubxLoadLog("Loaded Android RT
models."); } } ), while preserving the existing catches for
ClassNotFoundException and Exception and using the same references to
ANDROID_MODEL_CLASS, ANDROID_ASTUBX_LOCATION, cacheUtil.parseStubStream, and
astubxLoadLog.
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`:
- Line 1564: The constructor ExternalStubxLibraryModels(boolean
isJarInferEnabled, boolean isJSpecifyJDKEnabled) uses two same-typed booleans
that are easy to swap; make the API explicit by replacing the public constructor
with a private one and add a clearly named static factory or builder (e.g.,
ExternalStubxLibraryModels.createWithJarInferAndJSpecify(boolean
jarInferEnabled, boolean jSpecifyJDKEnabled) or a Builder class) and update all
call sites (calls to new ExternalStubxLibraryModels(...)) to use that
factory/builder so callers supply named intent and accidental argument
transposition is prevented; keep the class name ExternalStubxLibraryModels and
preserve the internal field names isJarInferEnabled/isJSpecifyJDKEnabled.
| /** --- JarInfer configs --- */ | ||
| static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled"; | ||
|
|
||
| static final String FL_JSPECIFY_JDK_ENABLED = EP_FL_NAMESPACE + ":JSpecifyJDKModels"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nitpick: FL_JSPECIFY_JDK_ENABLED is grouped under the JarInfer section comment.
FL_JSPECIFY_JDK_ENABLED (and its companion field jspecifyJDKModelsEnabled at 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
- /** --- JarInfer configs --- */
+ /** --- JarInfer and JDK model configs --- */
static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";
static final String FL_JSPECIFY_JDK_ENABLED = EP_FL_NAMESPACE + ":JSpecifyJDKModels";And likewise for the field comment at line 249 and the constructor comment at line 338.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** --- JarInfer configs --- */ | |
| static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled"; | |
| static final String FL_JSPECIFY_JDK_ENABLED = EP_FL_NAMESPACE + ":JSpecifyJDKModels"; | |
| /** --- JarInfer and JDK model configs --- */ | |
| static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled"; | |
| static final String FL_JSPECIFY_JDK_ENABLED = EP_FL_NAMESPACE + ":JSpecifyJDKModels"; | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java`
around lines 87 - 91, The FL_JSPECIFY_JDK_ENABLED constant is incorrectly
grouped under the JarInfer section; update the file to separate JSpecify-related
flags by either adding a new section comment like "/** --- JSpecify configs ---
*/" above FL_JSPECIFY_JDK_ENABLED or renaming the existing JarInfer section to
reflect both features, and likewise move or add matching comments for the
jspecifyJDKModelsEnabled field and the constructor comment that references
JSpecify JDK models so all JSpecify symbols (FL_JSPECIFY_JDK_ENABLED,
jspecifyJDKModelsEnabled, and the constructor comment) are grouped under a
dedicated JSpecify section.
| 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."); | ||
| } |
There was a problem hiding this comment.
Resource leak: androidStubxIS is never closed.
The Android InputStream obtained from getResourceAsStream on line 1570 is not closed in any code path. If cacheUtil.parseStubStream throws — caught by the outer catch (Exception e) — the stream is abandoned. Even on the happy path, no close() call exists.
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
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`
around lines 1568 - 1586, The InputStream androidStubxIS opened via
Class.forName(...).getClassLoader().getResourceAsStream(ANDROID_ASTUBX_LOCATION)
must be closed; change the code to obtain the Class (using
Class.forName(ANDROID_MODEL_CLASS)) and then open the resource inside a
try-with-resources so the stream is always closed (e.g. Class.forName(...) then
try (InputStream androidStubxIS =
clazz.getClassLoader().getResourceAsStream(ANDROID_ASTUBX_LOCATION)) { if
(androidStubxIS != null) { cacheUtil.parseStubStream(androidStubxIS,
"android.jar: " + ANDROID_ASTUBX_LOCATION); astubxLoadLog("Loaded Android RT
models."); } } ), while preserving the existing catches for
ClassNotFoundException and Exception and using the same references to
ANDROID_MODEL_CLASS, ANDROID_ASTUBX_LOCATION, cacheUtil.parseStubStream, and
astubxLoadLog.
| 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); | ||
| } |
There was a problem hiding this comment.
Silent failure when output.astubx is missing while JSpecifyJDKModels=true.
When the user explicitly opts in via NullAway:JSpecifyJDKModels=true but output.astubx is absent from the classpath, in is null (line 1591) and the only feedback is astubxLoadLog(...), which is gated on DEBUG_ASTUBX_LOADING = false (line 1770). The user silently gets no JDK nullness information without any indication of why.
Same concern applies to the catch (Exception e) path on line 1597 — a parse failure produces no visible warning.
Consider emitting an unconditional warning to System.err (or the compiler's Trees/Log) for the in == null case when the flag is explicitly enabled, matching how other missing-resource cases are handled in the ecosystem.
💡 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
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java`
around lines 1589 - 1599, When isJSpecifyJDKEnabled is true and
getClass().getClassLoader().getResourceAsStream("output.astubx") returns null or
cacheUtil.parseStubStream(...) throws, do not rely only on astubxLoadLog (which
is gated by DEBUG_ASTUBX_LOADING); instead emit an unconditional user-visible
warning. Update the block around isJSpecifyJDKEnabled /
getResourceAsStream("output.astubx") / cacheUtil.parseStubStream so that in the
in == null branch and in the catch (Exception e) branch you print a clear
warning (e.g., via System.err.println or the compiler/annotation-processing log
API such as Messager/Tree/Log) indicating JSpecifyJDKModels was enabled but
output.astubx was not found or failed to parse and include the exception message
in the parse-failure case; keep the existing astubxLoadLog calls for debug
logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java`:
- Around line 12-54: Add a third test method in JSpecifyJDKModelsTest (e.g.,
modelsEnabledWithJSpecifyModeLoadsAstubxModel) that calls makeTestHelperWithArgs
with both "-XepOpt:NullAway:JSpecifyMode=true" and
"-XepOpt:NullAway:JSpecifyJDKModels=true" (and
"-XepOpt:NullAway:AnnotatedPackages=foo"), add the same Test.java source as the
first test that expects a `@Nullable` diagnostic on attrs.get("key").toString(),
and call doTest(); this ensures the interaction between JSpecifyMode and
JSpecifyJDKModels is exercised.
- Around line 36-54: Add a short explanatory comment at the start of the test
method modelsDisabledDoesNotLoadAstubxModel stating the implicit assumption
being relied on: that no current NullAway model (built-in or JarInfer) marks
javax.naming.directory.Attributes.get() as `@Nullable`, and that if a future model
annotates that API the test will begin producing a diagnostic and should be
updated or adjusted; mention that the comment documents the fragility and what
to change if such a model is added.
| @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(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Negative test is implicitly fragile — explain the assumption.
modelsDisabledDoesNotLoadAstubxModel() passes today because no other NullAway model (built-in or JarInfer) annotates javax.naming.directory.Attributes.get() as @Nullable. If a future model is added for this API, the test will fail with an unexpected diagnostic and give no clear signal about why.
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
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java` around
lines 36 - 54, Add a short explanatory comment at the start of the test method
modelsDisabledDoesNotLoadAstubxModel stating the implicit assumption being
relied on: that no current NullAway model (built-in or JarInfer) marks
javax.naming.directory.Attributes.get() as `@Nullable`, and that if a future model
annotates that API the test will begin producing a diagnostic and should be
updated or adjusted; mention that the comment documents the fragility and what
to change if such a model is added.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java`:
- Around line 73-75: In JSpecifyJDKModelsTest, remove the unexplained dead-code
comment "// list.add(null);" (or replace it with a one-line explanation if it’s
intentionally omitted) and tighten the test assertion by replacing the empty "//
BUG: Diagnostic contains:" with an explicit expected fragment such as "// BUG:
Diagnostic contains: `@Nullable`" so the diagnostic on the following line
(list.get(0).toString();) is asserted precisely; update the comment near the
test method in JSpecifyJDKModelsTest to reflect this change.
- Around line 56-80: Add a companion test that mirrors
listContainingNullsWithModel() but omits the
"-XepOpt:NullAway:JSpecifyJDKModels=true" arg to verify the diagnostic depends
on the JSpecify JDK model flag; copy the same source snippet and create a new
test method (e.g., listContainingNullsWithoutModel) using
makeTestHelperWithArgs(JSpecifyJavacConfig.withJSpecifyModeArgs(List.of("-XepOpt:NullAway:AnnotatedPackages=foo")))
and assert the opposite outcome (adjust the BUG comment/expectation so this
variant does not report the diagnostic), keeping the test structure consistent
with modelsEnabledLoadsAstubxModel() / modelsDisabledDoesNotLoadAstubxModel().
---
Duplicate comments:
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java`:
- Around line 36-54: The test modelsDisabledDoesNotLoadAstubxModel assumes no
other NullAway model (built-in or JarInfer) marks
javax.naming.directory.Attributes.get() as `@Nullable`, which would cause this
negative test to fail; add a concise comment above the test (near
modelsDisabledDoesNotLoadAstubxModel / the
makeTestHelperWithArgs/CompilationTestHelper setup) stating this hidden
assumption and that adding any model making Attributes.get() nullable will cause
the diagnostic and break the test so future contributors understand the
dependency.
| @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 use(List<@Nullable String> list) { | ||
| // list.add(null); | ||
| // BUG: Diagnostic contains: | ||
| list.get(0).toString(); | ||
| } | ||
| } | ||
| """) | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the test file and examine its structure
find . -name "JSpecifyJDKModelsTest.java" -type fRepository: uber/NullAway
Length of output: 126
🏁 Script executed:
# Look for tests with similar names or related to listContainingNulls
rg -n "listContainingNulls" --type java -A 5 -B 2Repository: uber/NullAway
Length of output: 914
🏁 Script executed:
# Find JDK astubx files to see what's modeled
fd -e astubx -type f | head -20Repository: uber/NullAway
Length of output: 227
🏁 Script executed:
# Look for List interface modeling in astubx files
fd -e astubx -type f -exec grep -l "List" {} \;Repository: uber/NullAway
Length of output: 227
🏁 Script executed:
# Read the entire test file to see all tests and understand the structure
wc -l ./nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.javaRepository: uber/NullAway
Length of output: 129
🏁 Script executed:
# Read the test file
cat -n ./nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.javaRepository: uber/NullAway
Length of output: 3139
🏁 Script executed:
# Find astubx files using find command instead
find . -name "*.astubx" -type fRepository: uber/NullAway
Length of output: 408
🏁 Script executed:
# Check the output.astubx file for List.get annotation
cat ./nullaway/src/main/resources/output.astubx | grep -A 10 "class List"Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
# Also check if there's a pattern in astubx for java.util.List
cat ./nullaway/src/main/resources/output.astubx | grep -A 5 -B 2 "List.*get"Repository: uber/NullAway
Length of output: 100
🏁 Script executed:
# Check the file size and structure
head -50 ./nullaway/src/main/resources/output.astubxRepository: uber/NullAway
Length of output: 50370
🏁 Script executed:
# Search for strings in the binary astubx file related to List
strings ./nullaway/src/main/resources/output.astubx | grep -i "java.util.list\|List.*get" | head -20Repository: uber/NullAway
Length of output: 102
🏁 Script executed:
# Look for any other test methods related to JSpecifyJDKModels or similar patterns
rg "public void" nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.javaRepository: uber/NullAway
Length of output: 206
🏁 Script executed:
# Check if there are any comments or documentation about the test
rg -B 5 "listContainingNullsWithModel" nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.javaRepository: uber/NullAway
Length of output: 174
🏁 Script executed:
# Look for related tests that might be testing List behavior
rg -n "listContaining|List.*get" --type java -A 3 -B 1Repository: uber/NullAway
Length of output: 49500
Add a test variant without JSpecifyJDKModels=true to isolate the flag's necessity.
The test pattern established by modelsEnabledLoadsAstubxModel() and modelsDisabledDoesNotLoadAstubxModel() shows the proper way to validate that a flag is causally required. listContainingNullsWithModel() should have a corresponding variant without the flag to confirm that the diagnostic actually depends on JDK model loading. In JSpecify mode, List<@Nullable String>.get(0) returning @Nullable String may result from generic type-argument substitution alone, making the "WithModel" suffix misleading if the flag doesn't affect the outcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java` around
lines 56 - 80, Add a companion test that mirrors listContainingNullsWithModel()
but omits the "-XepOpt:NullAway:JSpecifyJDKModels=true" arg to verify the
diagnostic depends on the JSpecify JDK model flag; copy the same source snippet
and create a new test method (e.g., listContainingNullsWithoutModel) using
makeTestHelperWithArgs(JSpecifyJavacConfig.withJSpecifyModeArgs(List.of("-XepOpt:NullAway:AnnotatedPackages=foo")))
and assert the opposite outcome (adjust the BUG comment/expectation so this
variant does not report the diagnostic), keeping the test structure consistent
with modelsEnabledLoadsAstubxModel() / modelsDisabledDoesNotLoadAstubxModel().
| // list.add(null); | ||
| // BUG: Diagnostic contains: | ||
| list.get(0).toString(); |
There was a problem hiding this comment.
Remove commented-out line; tighten the empty Diagnostic contains: message.
Two issues:
- Line 73 —
// list.add(null);is unexplained dead code. Either remove it or add a comment explaining why it's intentionally excluded. - Line 74 —
// BUG: Diagnostic contains:has no expected text, so the assertion will pass for any diagnostic on the next line. The first test uses@Nullableas the expected fragment; applying the same precision here makes the assertion meaningful and catches regressions where the wrong diagnostic fires.
♻️ Proposed fix
void use(List<@Nullable String> list) {
- // list.add(null);
// BUG: Diagnostic contains: `@Nullable`
list.get(0).toString();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // list.add(null); | |
| // BUG: Diagnostic contains: | |
| list.get(0).toString(); | |
| void use(List<@Nullable String> list) { | |
| // BUG: Diagnostic contains: `@Nullable` | |
| list.get(0).toString(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/test/java/com/uber/nullaway/JSpecifyJDKModelsTest.java` around
lines 73 - 75, In JSpecifyJDKModelsTest, remove the unexplained dead-code
comment "// list.add(null);" (or replace it with a one-line explanation if it’s
intentionally omitted) and tighten the test assertion by replacing the empty "//
BUG: Diagnostic contains:" with an explicit expected fragment such as "// BUG:
Diagnostic contains: `@Nullable`" so the diagnostic on the following line
(list.get(0).toString();) is asserted precisely; update the comment near the
test method in JSpecifyJDKModelsTest to reflect this change.
|
@haewiful looking good overall. I made some cleanup changes and renamings. But I added another test that fails. The reason is that the parsing of the astubx file crashes in the middle. Right now we catch the exception at line 1598 of |
2 participants
This pull request adds some nullness check support for modules in the JDK package.
NullAway:JDKInferEnabledthat lets the users decide whether to load the astubx file to NullAway.Some related PRs are #1333.
The unit tests that are added to
LibraryModelsHandlerTest.javafile.Summary by CodeRabbit
New Features
Behavior
Tests