Augment NullAway error / fix serialization for Annotator auto fix mode#1322
Open
nimakarimipour wants to merge 10 commits intouber:masterfrom
Open
Augment NullAway error / fix serialization for Annotator auto fix mode#1322nimakarimipour wants to merge 10 commits intouber:masterfrom
nimakarimipour wants to merge 10 commits intouber:masterfrom
Conversation
Contributor
|
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:
WalkthroughIntroduces a nullable-inquiry interface and threads an optional nullable ExpressionTree plus infos map through ErrorBuilder and many call sites. NullAway gains a mayBeNullInquiry field and a SerializationAdapter. Serialization is bumped to version 4: ErrorInfo now carries origins (Set) and infos (Map<String,String]); Serializer emits a new errors.xml and a V4 adapter adds an infos TSV column. Adds ExpressionToSymbolScanner, OriginScanner, and OriginTrace to compute provenance for local nullable expressions. Updates numerous handlers, reports, and tests to use the new APIs. Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
nimakarimipour
force-pushed
the
nimak/autocodefix
branch
from
d0c7d83 to
2b21e00
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java (1)
131-142:⚠️ Potential issue | 🟠 Major
errors.xmlis not well-formed XML — missing prolog and root element.
initializeFile(errorOutputXmlPath, "")writes just"\n"as the file header, andserializeErrorInfothen appends oneErrorInfo.appendXml(...)fragment per error with no enclosing document element. The resulting file is a blank first line followed by N sibling elements — any standard XML parser (SAX/DOM/StAX) will reject it as having multiple roots / no document element.Because errors are appended incrementally across compilation units (and there is no end-of-analysis hook, per the existing comment in
appendToFile), you cannot trivially emit a closing tag. Two practical options:🛠️ Option A — write a root open tag up front and document that consumers must append `` (or tolerate its absence)
- initializeFile(errorOutputXmlPath, ""); + // NOTE: we cannot write a matching </errors> since there is no shutdown hook; + // consumers are expected to wrap the stream or tolerate a missing closing tag. + initializeFile(errorOutputXmlPath, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<errors>");🛠️ Option B — emit newline-delimited XML fragments but rename or document the file so downstream tooling doesn't try to parse it as a single XML document
e.g.
errors.xml.ndjson-style semantics, or add a comment in the javadoc ofserializeErrorInfostating that the output is a sequence of XML fragments, one per line.Secondary concern on the same code path:
escapeXmlat lines 214–226 handles&<>"'but not invalid XML 1.0 control characters (U+0000..U+001F except\t,\n,\r). If an error message contains such characters (possible when including source snippets or compiler output), the emitted XML will be invalid even with proper escaping elsewhere.🤖 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/fixserialization/Serializer.java` around lines 131 - 142, The errors.xml output is not a well-formed XML document because initializeOutputFiles writes an empty header and serializeErrorInfo appends XML fragments without an enclosing root; fix this by initializing errorOutputXmlPath with a proper XML prolog and opening root element (e.g. "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<errors>\n") in initializeOutputFiles (via initializeFile) so appended ErrorInfo.appendXml fragments become children of a single root, and document (in javadoc near serializeErrorInfo/appendToFile) that consumers may need to tolerate the missing closing "</errors>" when analysis is interrupted; additionally, harden escapeXml to strip or replace invalid XML 1.0 control characters (U+0000..U+001F except tab/newline/carriage return) before emission to avoid producing invalid XML.nullaway/src/test/java/com/uber/nullaway/SerializationTest.java (2)
1422-1429:⚠️ Potential issue | 🟠 MajorPlease include version 4 in the version-file test.
This helper still exercises only versions 1 and 3, so the new serialization version introduced here can regress without any test failing.
Suggested fix
`@Test` public void verifySerializationVersionIsSerialized() { // Check for serialization version 1. checkVersionSerialization(1); // Check for serialization version 3 (recall: 2 is skipped and was only used for an alpha // release of auto-annotator). checkVersionSerialization(3); + // Check for serialization version 4. + checkVersionSerialization(4); }🤖 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/SerializationTest.java` around lines 1422 - 1429, The test verifySerializationVersionIsSerialized currently only calls checkVersionSerialization(1) and checkVersionSerialization(3); add a call to checkVersionSerialization(4) in that test so the new serialization version is exercised by the helper method checkVersionSerialization and cannot regress unnoticed.
74-90:⚠️ Potential issue | 🟠 MajorThe new auto-fix metadata still isn't being asserted.
errorDisplayFactorynow accepts 13 columns but still drops the extra field, and the new tests still only readerrors.tsv. A brokeninfoscolumn orerrors.xml/origin payload would still pass this suite, even though that is the main behavior added by this PR.Also applies to: 2373-2509
🤖 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/SerializationTest.java` around lines 74 - 90, The factory/error-deserialization is accepting 13 columns but drops the new auto-fix metadata; update the deserialization in errorDisplayFactory (used in SerializationTest) to pass values[12] through to the ErrorDisplay constructor (or its builder) so ErrorDisplay stores the infos/auto-fix field instead of ignoring it, and extend SerializationTest to read/verify the 13th column (auto-fix/infos) from the errors.tsv (and where applicable the errors.xml/origin payload) using SerializationTestHelper.getRelativePathFromUnitTestTempDirectory so the new auto-fix metadata is asserted in the test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.gradle`:
- Line 83: The build currently has "-Werror" commented out in the main compiler
args which relaxes build enforcement; restore "-Werror" in the compiler
arguments (undo the commented-out "-Werror") or, if you must iterate on
XepPatchChecks, move the disabling into the existing temporary/dev-only profile
used for autocodefix (the comment block used for toggling checks) so master
keeps "-Werror"; update the compiler args list where "-Werror" appears and/or
add a Gradle profile flag to gate its removal during local runs.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.java`:
- Around line 44-52: getErrorsOutputFileHeader() adds a new "infos" column but
serializeError(ErrorInfo) still returns the V3 row shape, so update
serializeError(ErrorInfo) to append the infos value to the tab-separated row
(e.g., call super.serializeError(errorInfo) and then add "\t" + serializedInfos)
using ErrorInfo's accessor (e.g., getInfos() or the appropriate field) and
normalize/escape nulls, newlines and tabs so the output row count and ordering
match the header; alternatively, if you prefer not to emit infos in TSV, remove
the "\tinfos" suffix from getErrorsOutputFileHeader() and delete the redundant
serializeError override so header and rows stay consistent.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.java`:
- Around line 61-70: The current appendXmlFields method relies on
tabSeparatedToString(adapter).split("\t") which drops trailing empty fields and
can throw ArrayIndexOutOfBoundsException; fix it by calling split("\t", -1) to
preserve trailing empties and then defensively handle length < 6 (fill missing
entries with empty strings) before calling Serializer.appendXmlElement for
"target_kind","target_class","target_method","target_param","target_index","target_path";
reference appendXmlFields, tabSeparatedToString(adapter), and
Serializer.appendXmlElement and ensure any embedded tabs are considered a
longer-term issue (prefer emitting fields directly from the SymbolLocation
subclass state instead of round-tripping through TSV).
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java`:
- Around line 218-223: The isAnnotated() helper is recomputing “annotatedness”
from sourcefile presence which is incorrect; instead thread the actual
analysis-time annotatedness into ErrorInfo and use that stored flag. Add a
boolean field (e.g., annotatedAtAnalysis) to ErrorInfo, set it when ErrorInfo
instances are constructed (pass the Annotator/analysis result that determined
annotatedness), replace the current isAnnotated(Symbol) logic in ErrorInfo with
a simple accessor that returns the stored flag, and update all ErrorInfo
constructors/call sites to supply the real annotatedness instead of letting
ErrorInfo infer it (leave Serializer.pathToSourceFileFromURI and
ASTHelpers.enclosingClass untouched). Ensure Annotator’s decision is propagated
to wherever new ErrorInfo(...) is created so Annotator drives the value.
- Around line 190-191: The XML serialization uses
sym.getKind().toString().toLowerCase(Locale.getDefault()) which makes the
serialized enum value locale-dependent; update the call in ErrorInfo (where
Serializer.appendXmlElement is invoked with "kind" and sym.getKind()) to use
Locale.ROOT instead of Locale.getDefault() so enum names are lowercased in a
locale-independent, stable way.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/ExpressionToSymbolScanner.java`:
- Around line 67-82: The method defaultResult in ExpressionToSymbolScanner
currently prints to stdout for unhandled ExpressionTree shapes; remove the
System.out.println(...) call to avoid leaking messages on the compiler path and
instead simply return null (or use the existing null return path). Locate the
System.out.println in defaultResult and delete it (do not add stdout logging);
if needed for diagnostics, replace with a non-user-facing logger at trace/debug
level guarded behind a flag.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginTrace.java`:
- Around line 31-65: Replace the OriginTrace mutable class with a Java record to
remove boilerplate: change the declaration to a record named OriginTrace with
components (Symbol origin, Tree trace), remove the explicit constructor,
getOrigin(), getTrace(), equals(), and hashCode() methods, and rely on the
record-generated accessors origin() and trace() (update call sites in
OriginScanner and SerializationService from getOrigin()/getTrace() to
origin()/trace()); the generated toString(), equals, and hashCode will preserve
structural equality and improve debug output.
- Around line 38-41: Update the OriginTrace constructor to enforce non-null
parameters by calling Objects.requireNonNull on both inputs; inside the
OriginTrace(Symbol origin, Tree trace) constructor add
Objects.requireNonNull(origin) and Objects.requireNonNull(trace) so the class
(OriginTrace.origin and OriginTrace.trace) explicitly rejects nulls and fails
fast if a future caller passes null.
In `@nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java`:
- Around line 207-226: The current appendXmlElement and escapeXml methods
implement custom character escaping which can miss edge cases; replace this
manual logic by using a standard XML writer (e.g.,
javax.xml.stream.XMLStreamWriter or javax.xml.transform.stream.StreamResult with
Transformer) to produce elements and escape content safely: remove or stop using
escapeXml, update appendXmlElement to delegate to the chosen XMLStreamWriter
(writeStartElement, writeCharacters, writeEndElement) or equivalent, and ensure
callers that previously passed a StringBuilder now use the writer/stream API (or
adapt with a StringWriter wrapper) so all escaping and well-formedness are
handled by the JDK writer.
- Around line 76-83: serializeErrorInfo is allocating a new StringBuilder for
every error which is wasteful on hot paths; replace the per-call allocation by
reusing a single private StringBuilder instance (e.g., a field like xmlBuffer)
in Serializer, call xmlBuffer.setLength(0) before use, optionally
ensureCapacity(...) based on a rough expected size, pass xmlBuffer into
ErrorInfo.appendXml(xmlBuffer, serializationAdapter), and then append
xmlBuffer.toString() to errorOutputXmlPath; ensure any concurrent usage is
synchronized or use a ThreadLocal buffer if Serializer is used concurrently.
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 1983-1993: Replace locale-dependent lowercasing when serializing
symbol kinds: locate the calls that do
derefedSymbol.getKind().toString().toLowerCase(Locale.getDefault()) (and the
analogous call around line 2916) and change them to use Locale.ROOT instead of
Locale.getDefault(); update the expressions where "kind" metadata is populated
(the Map population around derefedSymbol and the other symbol-kind
serialization) to call toLowerCase(Locale.ROOT) so enum names are lowercased
deterministically.
---
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java`:
- Around line 131-142: The errors.xml output is not a well-formed XML document
because initializeOutputFiles writes an empty header and serializeErrorInfo
appends XML fragments without an enclosing root; fix this by initializing
errorOutputXmlPath with a proper XML prolog and opening root element (e.g.
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<errors>\n") in
initializeOutputFiles (via initializeFile) so appended ErrorInfo.appendXml
fragments become children of a single root, and document (in javadoc near
serializeErrorInfo/appendToFile) that consumers may need to tolerate the missing
closing "</errors>" when analysis is interrupted; additionally, harden escapeXml
to strip or replace invalid XML 1.0 control characters (U+0000..U+001F except
tab/newline/carriage return) before emission to avoid producing invalid XML.
In `@nullaway/src/test/java/com/uber/nullaway/SerializationTest.java`:
- Around line 1422-1429: The test verifySerializationVersionIsSerialized
currently only calls checkVersionSerialization(1) and
checkVersionSerialization(3); add a call to checkVersionSerialization(4) in that
test so the new serialization version is exercised by the helper method
checkVersionSerialization and cannot regress unnoticed.
- Around line 74-90: The factory/error-deserialization is accepting 13 columns
but drops the new auto-fix metadata; update the deserialization in
errorDisplayFactory (used in SerializationTest) to pass values[12] through to
the ErrorDisplay constructor (or its builder) so ErrorDisplay stores the
infos/auto-fix field instead of ignoring it, and extend SerializationTest to
read/verify the 13th column (auto-fix/infos) from the errors.tsv (and where
applicable the errors.xml/origin payload) using
SerializationTestHelper.getRelativePathFromUnitTestTempDirectory so the new
auto-fix metadata is asserted in the test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d779a1ab-f1de-4172-a69d-db6d3c30c6c5
📒 Files selected for processing (22)
build.gradlenullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/fixserialization/SerializationService.javanullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationAdapter.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.javanullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.javanullaway/src/main/java/com/uber/nullaway/fixserialization/location/SymbolLocation.javanullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/ExpressionToSymbolScanner.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginTrace.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullIfHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/FieldContractUtils.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.javanullaway/src/test/java/com/uber/nullaway/SerializationTest.java
| "-Xlint:rawtypes", | ||
| "-Xlint:unchecked", | ||
| "-Werror", | ||
| // "-Werror", |
Contributor
There was a problem hiding this comment.
Don't merge with -Werror disabled.
Removing -Werror silently lowers the quality bar for the entire build and lets any new compiler/Error Prone warning land unnoticed in master. The PR description notes this is an initial draft requiring follow-up, so please restore -Werror (or gate the removal behind a local/autocodefix profile) before this merges. The existing comment block at lines 110-113 is the idiomatic place to flip it off temporarily while iterating on -XepPatchChecks, not the main compiler args list.
🛠️ Proposed fix
options.compilerArgs += [
"-Xlint:deprecation",
"-Xlint:rawtypes",
"-Xlint:unchecked",
- // "-Werror",
+ "-Werror",
"-parameters",
]📝 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.
Suggested change
| // "-Werror", | |
| options.compilerArgs += [ | |
| "-Xlint:deprecation", | |
| "-Xlint:rawtypes", | |
| "-Xlint:unchecked", | |
| "-Werror", | |
| "-parameters", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build.gradle` at line 83, The build currently has "-Werror" commented out in
the main compiler args which relaxes build enforcement; restore "-Werror" in the
compiler arguments (undo the commented-out "-Werror") or, if you must iterate on
XepPatchChecks, move the disabling into the existing temporary/dev-only profile
used for autocodefix (the comment block used for toggling checks) so master
keeps "-Werror"; update the compiler args list where "-Werror" appears and/or
add a Gradle profile flag to gate its removal during local runs.
Collaborator
There was a problem hiding this comment.
yes @nimakarimipour we should undo this change and fix any issues
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
Author
There was a problem hiding this comment.
@msridhar Sorry I completely forgot that I added this to speed up local testing :) , Added -Werror back.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1322 +/- ##
============================================
- Coverage 88.45% 87.81% -0.64%
- Complexity 2878 2922 +44
============================================
Files 103 107 +4
Lines 9603 9851 +248
Branches 1935 1983 +48
============================================
+ Hits 8494 8651 +157
- Misses 532 595 +63
- Partials 577 605 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Extend the fix-serialization output with extra metadata needed by Annotator's automatic code-fix mode: - Add serialization version 4 with a new `infos` column. - Emit errors.xml alongside errors.tsv with per-error XML records, including origin traces for @nullable local variables (walking assignments / initializers back to the fields, parameters, or method returns that feed them). - Thread a Map<String, String> of site-specific metadata through ErrorBuilder for dereference / enhanced-for errors. - Thread nullable-expression + a MayBeNullableInquiry callback through every createErrorDescription call site (NullAway, GenericsChecks, contract handlers, field-contract handlers) so the origin scanner can run during reporting.
Restore -Werror and fix surfaced warnings. Serialize the infos column in V4 TSV rows so the header and row widths match. Use Locale.ROOT for machine-readable kind metadata. Preserve trailing empties when splitting tab-separated location strings. Remove a stray System.out.println from the compiler path.
nimakarimipour
force-pushed
the
nimak/autocodefix
branch
from
5751e46 to
ccdbc05
Compare
Removes the boilerplate constructor, getters, equals, and hashCode in favor of a Java 17 record. Call sites switch from getOrigin()/getTrace() to the generated origin()/trace() accessors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nullaway/src/test/java/com/uber/nullaway/SerializationTest.java (1)
72-90:⚠️ Potential issue | 🟡 MinorDon’t silently drop the v4
infoscolumn in tests.The factory accepts 13 fields but ignores
values[12], so v4 metadata can regress while tests still pass. Add a v4 display/assertion path for theinfoscolumn, and use the new dereference-origin tests to validate the expected metadata and/orerrors.xml.🤖 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/SerializationTest.java` around lines 72 - 90, The factory errorDisplayFactory currently accepts 12 or 13 values but ignores values[12] (the v4 infos column); update the factory and related test instantiation so the 13-field branch wires values[12] into the ErrorDisplay instance (either by calling a constructor overload or setting the infos field/property) instead of dropping it, and add a v4-specific assertion path in the serialization tests that validates the infos metadata (using SerializationTestHelper/getRelativePathFromUnitTestTempDirectory where appropriate) and the new dereference-origin tests to ensure errors.xml and expected metadata include the infos content.nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java (1)
100-147: 🧹 Nitpick | 🔵 TrivialCollapse duplicated
suppressibleNode+ delegate logic.The new
createErrorDescription(lines 100–117) andcreateErrorDescriptionWithInfo(lines 129–147) overloads are functionally identical apart from theargsargument — both recomputeenclosingSuppressTree = suppressibleNode(state.getPath())and forward to the 179 overload. Have the thin wrapper delegate to theWithInfovariant instead of duplicating the suppress-tree computation, which keeps a single source of truth for the "no explicit suggest tree" path.♻️ Proposed refactor
public Description createErrorDescription( ErrorMessage errorMessage, Description.Builder descriptionBuilder, VisitorState state, NullAway.MayBeNullableInquiry inquiry, `@Nullable` Symbol nonNullTarget, `@Nullable` ExpressionTree nullableExpression) { - Tree enclosingSuppressTree = suppressibleNode(state.getPath()); - return createErrorDescriptionWithInfo( - errorMessage, - enclosingSuppressTree, - descriptionBuilder, - state, - inquiry, - nonNullTarget, - nullableExpression, - Map.of()); + return createErrorDescriptionWithInfo( + errorMessage, + descriptionBuilder, + state, + inquiry, + nonNullTarget, + nullableExpression, + Map.of()); }🤖 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/ErrorBuilder.java` around lines 100 - 147, The two thin wrappers createErrorDescription and createErrorDescriptionWithInfo both call suppressibleNode(state.getPath()) and forward to the deeper overload; collapse this duplication by having the simpler createErrorDescription delegate to the WithInfo variant: remove the duplicate suppressibleNode call in createErrorDescription and replace its body with a call to createErrorDescriptionWithInfo(errorMessage, descriptionBuilder, state, inquiry, nonNullTarget, nullableExpression, Map.of()); keep the deeper overload (the full createErrorDescriptionWithInfo that accepts args) as the single implementation and reference suppressibleNode only there.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java (1)
226-231:⚠️ Potential issue | 🟠 MajorSerialize analysis-time annotatedness, not source-file presence.
This still treats “has a resolvable source path” as “is annotated,” which can flip both unannotated source code and annotated bytecode/source-elided symbols. Thread the real
codeAnnotationInfo/analysis decision intoErrorInfoinstead of recomputing it here.🤖 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/fixserialization/out/ErrorInfo.java` around lines 226 - 231, The current isAnnotated(Symbol) in ErrorInfo recomputes “annotated” by checking sourcefile URI; instead thread the actual analysis decision (codeAnnotationInfo / the analysis-time annotatedness flag) into ErrorInfo rather than recomputing; modify ErrorInfo to accept and store the real annotation boolean (or a CodeAnnotationInfo object) when constructed/serialized and replace usages of isAnnotated(Symbol) with that stored value (update the ErrorInfo constructor/factory and any serializer paths to propagate the provided codeAnnotationInfo).
🤖 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/ErrorBuilder.java`:
- Line 104: The parameter NullAway.MayBeNullableInquiry inquiry is currently
required and forced through all ErrorBuilder reporting call sites even though
it's only used when config.serializationIsActive(); make the parameter optional
instead of required by either annotating inquiry as `@Nullable` and null-checking
it before creating an OriginScanner in createErrorDescriptionWithInfo (or any
place that instantiates OriginScanner), or move retrieval of the inquiry into
ErrorBuilder (or pull it from state inside createErrorDescriptionWithInfo) so
callers like reportInitializerError and reportInitErrorOnField no longer must
pass it; ensure any use is guarded by config.serializationIsActive() and handle
a null inquiry as the sentinel case.
- Around line 208-219: The origin tracing is limited to the innermost enclosing
method because code calls state.findEnclosing(MethodTree.class) and passes that
to OriginScanner.retrieveOrigins when config.serializationIsActive() and
nullableExpression is a local; confirm with maintainers if this
captured-variable limitation (missing assignments from outer methods when
reporting inside lambdas/anonymous/nested classes) is acceptable for the initial
release, otherwise update the logic to walk outer enclosing MethodTree instances
(or otherwise aggregate scopes) before calling OriginScanner.retrieveOrigins so
assignments from outer scopes are considered for nullableExpression symbol
resolution.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.java`:
- Around line 51-53: serializeError currently uses
errorInfo.getInfos().toString() which produces an ambiguous Map.toString()
representation and only applies escapeSpecialCharacters to the whole string;
change it to emit a machine-readable, deterministic encoding (e.g., JSON or
URL-encoded key/value pairs) by iterating the Map returned from
ErrorInfo.getInfos(), handling null/empty maps explicitly (e.g., emit empty
object or sentinel), and applying escaping/quoting at the individual key and
value level (use a JSON library or a dedicated encodeKey/encodeValue routine)
before joining into the final serialized string; update serializeError (and any
helpers in SerializationService if needed) to build the structured
representation instead of using Map.toString(), ensuring insertion order is
preserved (LinkedHashMap) and the result is appended to
super.serializeError(errorInfo) with the same separator.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/ExpressionToSymbolScanner.java`:
- Around line 42-129: ExpressionToSymbolScanner is missing
visitMethodInvocation, so nullable method-call return symbols (METHOD) aren’t
captured; add an override for visitMethodInvocation(MethodInvocationTree node,
NullAway.MayBeNullableInquiry inquiry) that mirrors
visitMemberSelect/visitIdentifier: check inquiry.maybeNullable(node, state),
return empty set if false, otherwise call defaultResult(node) and return either
Set.of() or Set.of(symbol), and for compound invocations also recurse into
arguments/qualifier as needed using node.accept(this, inquiry) similar to
visitConditionalExpression; implement this inside the ExpressionToSymbolScanner
class referencing defaultResult, maybeNullable, and VisitorState.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.java`:
- Around line 76-122: The nullability checks use the enclosing VisitorState
(state) and thus can have stale TreePath; update OriginScanner to use a
TreePath-aware VisitorState for each inspected expression: either convert
OriginScanner into a TreePathScanner or, inside
visitAssignment/visitVariable/visitEnhancedForLoop, obtain a fresh TreePath for
the expression/initializer (e.g., TreePath for node.getExpression() or
node.getInitializer()) and derive a new VisitorState for that path before
calling inquiry.maybeNullable(...). Ensure you also pass that derived state into
ExpressionToSymbolScanner so inquiry and dataflow use the correct path; update
references to state/inquiry usage in visitAssignment, visitVariable, and
visitEnhancedForLoop accordingly.
- Around line 132-160: retrieveOrigins currently walks the whole MethodTree for
every queued symbol, letting assignments that occur after the diagnostic
location be considered origins; change retrieveOrigins to accept the diagnostic
expression position (or a tree/TreePath for that point) and bound the search to
only consider symbols/assignments that occur at or before that position: update
retrieveOrigins(Symbol target, int diagnosticPos) (or add an overload) and
modify the visitor logic (the tree.accept(...) implementation used here) to skip
or stop traversing statements whose start position is > diagnosticPos, and when
enqueuing origin symbols only enqueue those whose declaration/assignment
position <= diagnosticPos; alternatively, replace this limited-scan approach
with a reaching-definition/dataflow-based query to compute true reaching origins
for the diagnostic point.
In `@nullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.java`:
- Around line 76-82: The code currently creates/appends errors.xml for all
adapter versions and writes per-error XML fragments; change serializeErrorInfo
(and the other append point at the duplicate location) to only emit or append to
errorOutputXmlPath when the serializationAdapter version is V4 or when auto-fix
mode is enabled (check the adapter/version flag you already expose), and ensure
the XML is well-formed by either: (a) emitting a single root wrapper around
records via lifecycle hooks (emit an opening root element when the file is
created and a closing root element on shutdown) or (b) switching to a documented
fragment file name (not errors.xml) — implement this gating using the
serializationAdapter version check and use errorOutputXmlPath and appendToFile
only inside that guarded branch so older adapter versions keep previous artifact
behavior.
---
Outside diff comments:
In `@nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java`:
- Around line 100-147: The two thin wrappers createErrorDescription and
createErrorDescriptionWithInfo both call suppressibleNode(state.getPath()) and
forward to the deeper overload; collapse this duplication by having the simpler
createErrorDescription delegate to the WithInfo variant: remove the duplicate
suppressibleNode call in createErrorDescription and replace its body with a call
to createErrorDescriptionWithInfo(errorMessage, descriptionBuilder, state,
inquiry, nonNullTarget, nullableExpression, Map.of()); keep the deeper overload
(the full createErrorDescriptionWithInfo that accepts args) as the single
implementation and reference suppressibleNode only there.
In `@nullaway/src/test/java/com/uber/nullaway/SerializationTest.java`:
- Around line 72-90: The factory errorDisplayFactory currently accepts 12 or 13
values but ignores values[12] (the v4 infos column); update the factory and
related test instantiation so the 13-field branch wires values[12] into the
ErrorDisplay instance (either by calling a constructor overload or setting the
infos field/property) instead of dropping it, and add a v4-specific assertion
path in the serialization tests that validates the infos metadata (using
SerializationTestHelper/getRelativePathFromUnitTestTempDirectory where
appropriate) and the new dereference-origin tests to ensure errors.xml and
expected metadata include the infos content.
---
Duplicate comments:
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.java`:
- Around line 226-231: The current isAnnotated(Symbol) in ErrorInfo recomputes
“annotated” by checking sourcefile URI; instead thread the actual analysis
decision (codeAnnotationInfo / the analysis-time annotatedness flag) into
ErrorInfo rather than recomputing; modify ErrorInfo to accept and store the real
annotation boolean (or a CodeAnnotationInfo object) when constructed/serialized
and replace usages of isAnnotated(Symbol) with that stored value (update the
ErrorInfo constructor/factory and any serializer paths to propagate the provided
codeAnnotationInfo).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 095ad720-8c1a-465d-b610-7c34a8c6dfa0
📒 Files selected for processing (21)
nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/fixserialization/SerializationService.javanullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationAdapter.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.javanullaway/src/main/java/com/uber/nullaway/fixserialization/location/AbstractSymbolLocation.javanullaway/src/main/java/com/uber/nullaway/fixserialization/location/SymbolLocation.javanullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/ExpressionToSymbolScanner.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginTrace.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javanullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractUtils.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/EnsuresNonNullIfHandler.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/FieldContractUtils.javanullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.javanullaway/src/test/java/com/uber/nullaway/SerializationTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.java (2)
94-108: Redundant initializer null check.Line 96 already returns
Set.of()whennode.getInitializer() == null, so theinitializer == nulldisjunct on line 100 is dead. Simplify to a single guard and reuse the same local.♻️ Proposed simplification
public Set<OriginTrace> visitVariable(VariableTree node, Symbol target) { Symbol symbol = ASTHelpers.getSymbol(node); if (symbol != null && symbol.equals(target)) { - if (node.getInitializer() == null) { - return Set.of(); - } ExpressionTree initializer = node.getInitializer(); if (initializer == null || !inquiry.maybeNullable(initializer, state)) { return Set.of(); } return initializer.accept(new ExpressionToSymbolScanner(state), inquiry).stream() .map(input -> new OriginTrace(input, node)) .collect(Collectors.toSet()); } return super.visitVariable(node, target); }🤖 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/fixserialization/scanners/OriginScanner.java` around lines 94 - 108, Assign node.getInitializer() to a local (ExpressionTree initializer) once inside the Symbol match branch and remove the earlier separate null-return check so you have a single guard: if (initializer == null || !inquiry.maybeNullable(initializer, state)) return Set.of(); then proceed to call initializer.accept(new ExpressionToSymbolScanner(state), inquiry) and map results to new OriginTrace(input, node). Update the visitVariable logic in OriginScanner to reuse the initializer local and eliminate the redundant null check.
143-161: Minor: collapse visited contains+add.
Set#addalready returns whether the element was newly added; the explicitcontainscheck can be dropped.♻️ Proposed simplification
- Symbol current = queue.poll(); - if (visited.contains(current)) { - continue; - } - visited.add(current); + Symbol current = queue.poll(); + if (!visited.add(current)) { + continue; + }🤖 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/fixserialization/scanners/OriginScanner.java` around lines 143 - 161, In OriginScanner's traversal loop (the while over queue in the method containing variables queue, visited, tree.accept), simplify the visited check by using Set.add's boolean return value instead of contains; replace the pattern if (visited.contains(current)) { continue; } visited.add(current); with a single conditional using if (!visited.add(current)) continue; so you only add-and-check once and keep the rest of the loop logic (calling tree.accept and processing involvedSymbols) unchanged.
🤖 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/fixserialization/out/ErrorInfo.java`:
- Around line 199-202: Change the XML tag names to use the project's snake_case
convention: update the Serializer.appendXmlElement calls that currently emit
"class" and "isAnnotated" to emit "enc_class" (or "origin_class" if you prefer
consistency with other code) and "is_annotated" respectively; keep the symbol
sources the same (sym.enclClass() and isAnnotated(sym)) and leave the kind
emission as-is (sym.getKind().toString().toLowerCase(Locale.ROOT)) so only the
tag strings are modified.
- Around line 212-216: The infos map keys are written directly as XML element
names in ErrorInfo (using Serializer.appendXmlElement with entry.getKey()),
which can produce malformed XML; change the serialization to emit a safe wrapper
element (e.g. emit "<info name=\"...\">escapedValue</info>" for each entry) or
validate/assert keys against XML name rules before using them as element names,
and ensure values remain escaped via Serializer.appendXmlElement or equivalent;
also mirror the origins behavior by skipping the entire "<infos>...</infos>"
block when infos.isEmpty(). Reference: infos map in ErrorInfo,
Serializer.appendXmlElement, and the ErrorBuilder/SerializationService call
sites that populate infos.
---
Nitpick comments:
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.java`:
- Around line 94-108: Assign node.getInitializer() to a local (ExpressionTree
initializer) once inside the Symbol match branch and remove the earlier separate
null-return check so you have a single guard: if (initializer == null ||
!inquiry.maybeNullable(initializer, state)) return Set.of(); then proceed to
call initializer.accept(new ExpressionToSymbolScanner(state), inquiry) and map
results to new OriginTrace(input, node). Update the visitVariable logic in
OriginScanner to reuse the initializer local and eliminate the redundant null
check.
- Around line 143-161: In OriginScanner's traversal loop (the while over queue
in the method containing variables queue, visited, tree.accept), simplify the
visited check by using Set.add's boolean return value instead of contains;
replace the pattern if (visited.contains(current)) { continue; }
visited.add(current); with a single conditional using if (!visited.add(current))
continue; so you only add-and-check once and keep the rest of the loop logic
(calling tree.accept and processing involvedSymbols) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 124a6c72-3ca6-41f6-b1db-f2334d177672
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/fixserialization/out/ErrorInfo.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginTrace.java
✅ Files skipped from review due to trivial changes (1)
- nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginTrace.java
- V4 infos column now uses an explicit key=value,... codec with backslash escaping for \, =, and , so values can round-trip without being confused with entry separators. Previously relied on Map.toString() which produced ambiguous output. - ExpressionToSymbolScanner gains a visitMethodInvocation override so nullable method-return origins (e.g. foo() in Object f = foo()) are captured instead of dropped. - Serializer now gates errors.xml creation and appends behind serialization version >= 4, so V1/V3 users don't get a surprise artifact. Documented the fragment-stream contract. - OriginScanner accepts a diagnostic start offset and skips assignments, variable declarations, and for-each loops that start at or after it. ErrorBuilder passes the error tree position so origins from non-reaching future assignments are no longer serialized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.java (1)
104-148:⚠️ Potential issue | 🟠 MajorUse a TreePath-aware state for scanned expressions.
maybeNullable(..., state)andnew ExpressionToSymbolScanner(state)reuse the diagnosticVisitorStatewhile inspecting different initializer/assignment expressions. Dataflow-backed inquiries can then run at the wrong path and drop or misclassify origins. This is the same unresolved issue previously flagged; convert this scanner to aTreePathScanneror derive aVisitorStatefor each inspected expression before querying/scanning it.🤖 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/fixserialization/scanners/OriginScanner.java` around lines 104 - 148, The scanner reuses the top-level VisitorState when calling inquiry.maybeNullable(...) and new ExpressionToSymbolScanner(state) inside visitAssignment, visitVariable and visitEnhancedForLoop which can run dataflow queries at the wrong TreePath; update the scanner so each inspected expression uses a TreePath-aware state: either convert this class to a TreePathScanner or, before calling inquiry.maybeNullable(...) and creating ExpressionToSymbolScanner, derive a per-expression VisitorState (e.g., stateForPath or state.withPath) for the ExpressionTree, then use that derived state for both maybeNullable and new ExpressionToSymbolScanner so OriginTrace creation remains correct and startsAfterDiagnostic checks still use the original diagnostic context.
🤖 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/ErrorBuilder.java`:
- Around line 211-219: The diagnostic start position should use the
nullableExpression's start rather than the enclosing leaf; replace the diagPos
computation so it first attempts to get ((JCTree)
nullableExpression).getStartPosition() (with fallback to ((JCTree)
state.getPath().getLeaf()).getStartPosition() if nullableExpression's position
is not available) before passing diagPos into new
OriginScanner(...).retrieveOrigins(...) so OriginScanner uses the nullable
expression's actual position when locating assignments; update the diagPos
variable where it's computed in the block handling nullableExpression (near
nullableExpressionSymbol and OriginScanner usage).
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.java`:
- Around line 39-42: Update the Javadoc in SerializationV4Adapter describing the
`infos` column to explicitly state how an empty map is encoded (the current
implementation returns the empty string), and mention that this empty-string
value will appear as an empty field between TSV tabs after passing through
SerializationService#escapeSpecialCharacters; alternatively, if you prefer a
sentinel like "null" (consistent with V3's absent target_*), change the
serialization logic in the same adapter to emit that sentinel and update the
Javadoc accordingly so callers know which representation to expect.
---
Duplicate comments:
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.java`:
- Around line 104-148: The scanner reuses the top-level VisitorState when
calling inquiry.maybeNullable(...) and new ExpressionToSymbolScanner(state)
inside visitAssignment, visitVariable and visitEnhancedForLoop which can run
dataflow queries at the wrong TreePath; update the scanner so each inspected
expression uses a TreePath-aware state: either convert this class to a
TreePathScanner or, before calling inquiry.maybeNullable(...) and creating
ExpressionToSymbolScanner, derive a per-expression VisitorState (e.g.,
stateForPath or state.withPath) for the ExpressionTree, then use that derived
state for both maybeNullable and new ExpressionToSymbolScanner so OriginTrace
creation remains correct and startsAfterDiagnostic checks still use the original
diagnostic context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31d3ef9d-1624-4702-bbf4-c7ddf5e4f7be
📒 Files selected for processing (5)
nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/fixserialization/Serializer.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/ExpressionToSymbolScanner.javanullaway/src/main/java/com/uber/nullaway/fixserialization/scanners/OriginScanner.java
Contributor
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/ErrorBuilder.java`:
- Around line 219-225: The diagPos computed for OriginScanner can remain
negative if both ((JCTree) nullableExpression).getStartPosition() and ((JCTree)
state.getPath().getLeaf()).getStartPosition() return < 0, causing OriginScanner
to treat every assignment as after the diagnostic and yield empty origins;
change the fallback so that after checking nullableExpression and the current
leaf you set diagPos to OriginScanner.NO_BOUND when both positions are negative
before calling new OriginScanner(inquiry, state, diagPos).retrieveOrigins(...),
ensuring OriginScanner receives a valid non-negative bound constant instead of a
negative position.
In
`@nullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.java`:
- Around line 59-63: serializeInfos currently calls infos.isEmpty() and
appendEscaped assumes non-null strings, so null maps or null keys/values from
ErrorInfo.getInfos() will NPE; update serializeInfos(String) to first null-check
infos (treat null as empty map) and when iterating entries in
serializeInfos(Map<String,String>) guard each key/value against null (treat null
key/value as empty string) before calling appendEscaped, and update
appendEscaped(StringBuilder, String) to handle a null value by treating it as ""
to avoid calling value.length(); references: serializeInfos, appendEscaped,
ErrorInfo.getInfos.
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 323-329: The error reports created via
errorBuilder.createErrorDescription are passing null for the nullableExpression
argument, which prevents ErrorBuilder from computing OriginTrace for serialized
ASSIGN_GENERIC_NULLABLE / RETURN_NULLABLE_GENERIC / PASS_NULLABLE_GENERIC
diagnostics; update the calls in reportInvalidParametersNullabilityError to pass
paramExpression (instead of null) and similarly change the assignment and return
reporting calls to pass the RHS expression / returned expression respectively so
ErrorBuilder.createErrorDescription receives the real nullableExpression and can
compute provenance (look for usages around errorBuilder.createErrorDescription
and the methods reportInvalidParametersNullabilityError and the
assignment/return reporting paths).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9437fca-da6e-4618-ad76-e1b0483f24e9
📒 Files selected for processing (3)
nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.javanullaway/src/main/java/com/uber/nullaway/fixserialization/adapters/SerializationV4Adapter.javanullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Comment on lines
+219
to
+225
| int diagPos = ((JCTree) nullableExpression).getStartPosition(); | ||
| if (diagPos < 0) { | ||
| diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition(); | ||
| } | ||
| origins = | ||
| new OriginScanner(inquiry, state, diagPos) | ||
| .retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol); |
Contributor
There was a problem hiding this comment.
Use OriginScanner.NO_BOUND if both source positions are unavailable.
If both nullableExpression and the current leaf return a negative start position, diagPos stays negative and OriginScanner will treat every assignment as “after” the diagnostic, producing an empty origin set. Fall back to OriginScanner.NO_BOUND instead of leaving the bound invalid.
🐛 Proposed fix
int diagPos = ((JCTree) nullableExpression).getStartPosition();
if (diagPos < 0) {
diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition();
}
+ if (diagPos < 0) {
+ diagPos = OriginScanner.NO_BOUND;
+ }
origins =
new OriginScanner(inquiry, state, diagPos)
.retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol);📝 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.
Suggested change
| int diagPos = ((JCTree) nullableExpression).getStartPosition(); | |
| if (diagPos < 0) { | |
| diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition(); | |
| } | |
| origins = | |
| new OriginScanner(inquiry, state, diagPos) | |
| .retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol); | |
| int diagPos = ((JCTree) nullableExpression).getStartPosition(); | |
| if (diagPos < 0) { | |
| diagPos = ((JCTree) state.getPath().getLeaf()).getStartPosition(); | |
| } | |
| if (diagPos < 0) { | |
| diagPos = OriginScanner.NO_BOUND; | |
| } | |
| origins = | |
| new OriginScanner(inquiry, state, diagPos) | |
| .retrieveOrigins(state.findEnclosing(MethodTree.class), nullableExpressionSymbol); |
🤖 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/ErrorBuilder.java` around lines 219
- 225, The diagPos computed for OriginScanner can remain negative if both
((JCTree) nullableExpression).getStartPosition() and ((JCTree)
state.getPath().getLeaf()).getStartPosition() return < 0, causing OriginScanner
to treat every assignment as after the diagnostic and yield empty origins;
change the fallback so that after checking nullableExpression and the current
leaf you set diagPos to OriginScanner.NO_BOUND when both positions are negative
before calling new OriginScanner(inquiry, state, diagPos).retrieveOrigins(...),
ensuring OriginScanner receives a valid non-negative bound constant instead of a
negative position.
Comment on lines
323
to
+329
| errorBuilder.createErrorDescription( | ||
| errorMessage, analysis.buildDescription(tree), state, null)); | ||
| errorMessage, | ||
| analysis.buildDescription(tree), | ||
| state, | ||
| analysis.mayBeNullInquiry, | ||
| null, | ||
| null)); |
Contributor
There was a problem hiding this comment.
Thread the offending expression into serialized generic-mismatch reports.
These reporters now pass analysis.mayBeNullInquiry, but they still hardcode nullableExpression to null. That means ErrorBuilder can never compute OriginTraces for serialized ASSIGN_GENERIC_NULLABLE, RETURN_NULLABLE_GENERIC, or PASS_NULLABLE_GENERIC diagnostics, which leaves Annotator without the provenance this PR is adding. At minimum, reportInvalidParametersNullabilityError(...) should pass paramExpression; the assignment/return paths should similarly plumb the RHS / returned expression instead of dropping it.
Also applies to: 364-370, 407-413
🤖 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/generics/GenericsChecks.java` around
lines 323 - 329, The error reports created via
errorBuilder.createErrorDescription are passing null for the nullableExpression
argument, which prevents ErrorBuilder from computing OriginTrace for serialized
ASSIGN_GENERIC_NULLABLE / RETURN_NULLABLE_GENERIC / PASS_NULLABLE_GENERIC
diagnostics; update the calls in reportInvalidParametersNullabilityError to pass
paramExpression (instead of null) and similarly change the assignment and return
reporting calls to pass the RHS expression / returned expression respectively so
ErrorBuilder.createErrorDescription receives the real nullableExpression and can
compute provenance (look for usages around errorBuilder.createErrorDescription
and the methods reportInvalidParametersNullabilityError and the
assignment/return reporting paths).
…y into nimak/autocodefix
Replace hand-coded string concatenation and manual escape table with javax.xml.stream.XMLStreamWriter (JDK built-in StAX). ErrorInfo, AbstractSymbolLocation, and SymbolLocation now write to an XMLStreamWriter; Serializer wraps a per-error StringWriter and appends the result to errors.xml. The on-disk fragment-stream layout is unchanged.
Collaborator
|
@nimakarimipour can you update the PR summary to better describe the changes overall? I don't really understand what are the key new functionalities here, and why we need to switch from TSV to XML. Also, it seems like code coverage is pretty low for this patch (see the codecov comment). Can we bring that up with more tests? |
msridhar
reviewed
May 4, 2026
| private final @Nullable Path path; | ||
|
|
||
| /** Extra argument regarding the error required to generate a fix automatically. */ | ||
| private final Map<String, String> infos; |
Collaborator
There was a problem hiding this comment.
Do we know the shape of this map at all, like what keys are expected?
msridhar
reviewed
May 4, 2026
|
|
||
| @Override | ||
| public Set<Symbol> reduce(Set<Symbol> r1, Set<Symbol> r2) { | ||
| if (r2 == null && r1 == null) { |
Collaborator
There was a problem hiding this comment.
I see a few null checks on symbols not marked as @Nullable like these here. Given that all our code is checked with NullAway, these should not be necessary, correct?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR updates the NullAway error serialization to include additional information needed for Annotator’s automatic code-fix mode. The implementation is in an initial state and will require further refactoring.
Summary by CodeRabbit
New Features
Tests