Migrate AutoValue to Records #1415
Conversation
|
|
|
Hi @msridhar, |
WalkthroughThis PR removes AutoValue usages and dependency entries, converts multiple AutoValue-backed types to Java records or explicit classes (e.g., MethodAnnotationsRecord, MethodClassAndName, FieldRef, FieldInitEntities, CfgParams, AnalysisParams, CollectRecordAndInnerMethod, CollectLikeMethodRecord), updates factory methods and call sites accordingly, removes auto-value entries from the Gradle version catalog and bumps Error Prone versions, re-enables ErrorProne warnings for generated code by deleting the suppression, and inlines contract test sources while deleting the former testdata files. 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
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: 4
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/NullAway.java (1)
2819-2878: FieldInitEntities record correctly replaces the AutoValue holderThe
FieldInitEntitiesrecord mirrors the old AutoValue structure and continues to provide all the accessors relied on by the initialization logic (nonnullInstanceFields(),constructors(), initializer methods/blocks, etc.). Using thecreate(...)factory to defensively copy intoImmutable*collections maintains the previous immutability guarantees.You might consider trimming the “Generates by Record” text and the inline method-signature commentary in the Javadoc in favor of a shorter description of each component, but that’s stylistic and not blocking.
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (1)
92-105: CfgParams must override equals/hashCode for cfgCache to work correctly
CfgParamsis used as the key type forcfgCache, but the manual implementation lacksequals/hashCodeoverrides. Each call toCfgParams.create(path, env)produces a fresh instance with different identity, so cache lookups always use Object's identity-based equality and always miss. This prevents CFG reuse and causes unnecessary recomputation—a significant performance regression from the previous@AutoValueimplementation, which generated value-based equality on thecodePathfield.The commented placeholder at lines 380-387 indicates this was recognized as incomplete work. Restore value-based semantics by overriding these methods based on
codePathandenvironment, or simplify by makingenvironmenta final constructor parameter. Either approach will restore cache hits and eliminate the performance regression.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
build.gradlegradle/libs.versions.tomllibrary-model/library-model-generator/build.gradlelibrary-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.javanullaway/build.gradlenullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.javanullaway/src/main/java/com/uber/nullaway/LibraryModels.javanullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javanullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.javanullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java
💤 Files with no reviewable changes (3)
- nullaway/build.gradle
- gradle/libs.versions.toml
- library-model/library-model-generator/build.gradle
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.java
🔇 Additional comments (3)
build.gradle (1)
87-88: Comment update correctly reflects AutoValue removalThe updated comment drops the AutoValue-specific mention and stays accurate given the migration; no behavioral impact.
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (1)
390-395: AnalysisParams record migration is consistent with prior AutoValue usageSwitching
AnalysisParamsto a record with(ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg)and keeping thecreate(...)factory is a straightforward replacement for the AutoValue version. It preserves the previous key semantics foranalysisCache(value-based on transfer function and CFG reference), so no additional changes seem necessary here.nullaway/src/main/java/com/uber/nullaway/LibraryModels.java (1)
328-333: FieldRef record keeps previous value semanticsConverting
FieldRefto a record withenclosingClassNameandfieldNameand updating the factory to call the canonical constructor is behaviorally equivalent to the old AutoValue-based holder. Equality and hashing over both components remain correct for its use inImmutableSet<FieldRef>and related collections.
...ibrary-model-generator/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.java
Outdated
Show resolved
Hide resolved
| record MethodClassAndName(String enclosingClass, | ||
| String methodName) { | ||
|
|
||
| static MethodClassAndName create(String enclosingClass, String methodName) { | ||
| return new AutoValue_ErrorProneCLIFlagsConfig_MethodClassAndName(enclosingClass, methodName); | ||
| return new MethodClassAndName(enclosingClass, methodName); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MethodClassAndName record migration preserves value semantics
Using a record here keeps the previous AutoValue-style value semantics (field-based equals/hashCode) and works cleanly with the knownInitializers ImmutableSet. The factory and fromClassDotMethod logic remain unchanged in behavior.
If this type is only used inside ErrorProneCLIFlagsConfig, you could optionally mark the record private to keep the helper out of the public surface, but that's non-blocking.
🤖 Prompt for AI Agents
In nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java around
lines 611 to 616, the new MethodClassAndName record preserves value semantics
but should be made private if it’s only used inside ErrorProneCLIFlagsConfig;
change the declaration to a private record MethodClassAndName(...) and keep the
existing static factory and fromClassDotMethod logic unchanged so behavior
remains the same.
nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java
Outdated
Show resolved
Hide resolved
| record CollectRecordAndInnerMethod(CollectLikeMethodRecord collectlikeMethodRecord, | ||
| Tree innerMethodOrLambda) { | ||
|
|
||
| static CollectRecordAndInnerMethod create( | ||
| CollectLikeMethodRecord collectlikeMethodRecord, Tree innerMethodOrLambda) { | ||
| return new AutoValue_StreamNullabilityPropagator_CollectRecordAndInnerMethod( | ||
| return new CollectRecordAndInnerMethod( | ||
| collectlikeMethodRecord, innerMethodOrLambda); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CollectRecordAndInnerMethod record is a clean AutoValue replacement
The record-based CollectRecordAndInnerMethod plus the updated call sites using collectlikeMethodRecord() / innerMethodOrLambda() keep the previous semantics (immutable pair, correct equals/hashCode) while simplifying the implementation. No functional issues spotted.
Optionally, consider renaming the component to collectLikeMethodRecord for readability and consistency with the CollectLikeMethodRecord type name.
Also applies to: 375-382
🤖 Prompt for AI Agents
In
nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.java
around lines 130-137 (and similarly 375-382), the record components use names
`collectlikeMethodRecord` and `innerMethodOrLambda`; rename the first component
to `collectLikeMethodRecord` to match the `CollectLikeMethodRecord` type and
improve readability, update the record constructor/factory and all call sites to
use the new accessor `collectLikeMethodRecord()` (and leave
`innerMethodOrLambda()` as-is), and ensure imports and usages compile after the
rename.
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Thanks for this great contribution! I have some initial comments
build.gradle
Outdated
| options.errorprone { | ||
| // disable warnings in generated code; AutoValue code fails UnnecessaryParentheses check | ||
| // disable warnings in generated code; fails UnnecessaryParentheses check | ||
| disableWarningsInGeneratedCode = true |
There was a problem hiding this comment.
Probably we can just get rid of this line now?
| /* | ||
| * If needed, need to override these methods | ||
| * @Override | ||
| * public boolean equals(Object o) { ...codePath... } | ||
| * | ||
| * @Override | ||
| * public int hashCode() { ...codePath... } | ||
| */ |
There was a problem hiding this comment.
Yes, important to override these and only use the codePath field. Also can we override toString?
|
|
||
| /** | ||
| /* | ||
| * Generate by Record |
There was a problem hiding this comment.
Again, these docs should appear as Javadoc for the record parameters
|
@rishikraj990 let me know if you need further help / feedback here |
|
will update with accommodating all the suggested changes :) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java (1)
28-42: Move parameter documentation to record Javadoc.The
@paramdocumentation in thecreate()method should be moved to the record declaration itself using@paramtags in the record's Javadoc. This is the standard way to document record components and ensures the documentation appears for all accessors.🔎 Suggested Javadoc format
/** * An immutable model describing a collect-like method from a stream-based API, * such as {@link java.util.stream.Stream#collect(Collector)}. * * <p>Collect-like methods are distinguished from map-like methods in that they * accept a {@link Collector} instance as an argument. This record captures the * metadata necessary to identify such methods, match specific collector factory * methods, and track how stream elements flow into those factories. + * + * @param collectorFactoryMethodClass The fully qualified name of the class that contains the collector factory method + * @param collectorFactoryMethodSignature The signature of the factory method that creates the {@link Collector} instance + * @param argsToCollectorFactoryMethod The indices of the arguments to the collector factory method that are lambdas + * @param innerMethodName Name of the method that gets passed the elements of the stream + * @param argsFromStream Argument indices to which stream elements are directly passed */nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2784-2801: Move parameter documentation to record Javadoc.The
@paramdocumentation should be added to the record declaration's Javadoc rather than (or in addition to) thecreate()factory method. This ensures documentation appears for the record component accessors.🔎 Suggested Javadoc format
/** * Aggregates all entities involved in field initialization for a class. * * <p>This record captures information needed to reason about initialization of * {@code @NonNull} fields, including initializer blocks, constructors, and * initializer methods, both instance-level and static. * * <p>The data is extracted from a class symbol and used during NullAway analysis * to verify that all required fields are properly initialized. + * + * @param classSymbol symbol for class + * @param nonnullInstanceFields {@code @NonNull} instance fields not directly initialized at declaration + * @param nonnullStaticFields {@code @NonNull} static fields not directly initialized at declaration + * @param instanceInitializerBlocks instance initializer blocks in declaration order + * @param staticInitializerBlocks static initializer blocks in declaration order + * @param constructors constructors in the class + * @param instanceInitializerMethods non-static initializer methods + * @param staticInitializerMethods static initializer methods */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.javanullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javanullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java
🔇 Additional comments (5)
library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.java (1)
9-11: Excellent migration from AutoValue to Record.The record declaration is well-formed with all components using Guava immutable types, ensuring proper immutability. The record automatically provides accessors,
equals(),hashCode(), andtoString(), matching AutoValue's behavior.nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java (1)
61-73: LGTM!The factory method correctly delegates to the record constructor, maintaining API compatibility with existing callers.
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (2)
354-476: LGTM! Well-designed manual class for cache key semantics.The
CfgParamsclass correctly excludesenvironmentfromequals/hashCode/toStringsince it's not part of the logical identity. The thorough Javadoc clearly explains why this design was chosen over a record.
478-485: LGTM!The
AnalysisParamsrecord migration is correct. The privatecreate()factory method maintains the existing internal API.nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2822-2840: LGTM!The
create()factory method correctly converts mutable collection inputs (Set,List) to their immutable counterparts (ImmutableSet,ImmutableList), ensuring the record's immutability guarantees are preserved.
...ibrary-model-generator/src/main/java/com/uber/nullaway/libmodel/MethodAnnotationsRecord.java
Show resolved
Hide resolved
|
@rishikraj990 your changes are failing CI due to formatting and other issues. For the formatting issues you can run |
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Changes look good but we need CI to pass
|
@rishikraj990 do you know when you might be able to finish this one up? Also, if you check the box to allow contributions from committers, then we could do some of the easier cleanup. Right now I cannot push to your branch. |
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/dataflow/DataFlow.java:
- Around line 479-486: The AnalysisParams.record defines a redundant static
factory create() that only delegates to the record constructor; remove the
create(ForwardTransferFunction<?, ?>, ControlFlowGraph) method and update all
call sites that use AnalysisParams.create(...) to use the record constructor
directly (new AnalysisParams(transferFunction, cfg)) or the canonical record
construction form with the same parameters, ensuring you update references in
the code paths that previously called create (e.g., the sites where
transferFunction and cfg are supplied).
- Around line 355-477: Change CfgParams to take ProcessingEnvironment in the
constructor and make the environment field final: replace the single-arg private
CfgParams(TreePath) and the static create(TreePath, ProcessingEnvironment)
factory with a constructor CfgParams(TreePath, ProcessingEnvironment) that
assigns both final fields, update environment() to return the final field as
before, and remove the post-construction mutation paths (and calls to create) so
callers (e.g., code that calls CfgParams.create(...) before passing to the cache
load) instead call new CfgParams(codePath, environment); keep
equals/hashCode/toString based only on codePath.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
gradle/libs.versions.tomlnullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java
💤 Files with no reviewable changes (1)
- gradle/libs.versions.toml
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java (1)
355-486: Migration from AutoValue to Records/manual classes is well-executed.The conversion strategy is appropriate:
AnalysisParams→ Java record (simple value type)CfgParams→ manual class (needed to excludeenvironmentfrom equality/hashing)This aligns with the PR's objective to migrate away from AutoValue while respecting the constraints of Java records. The use of a manual class for
CfgParamscorrectly handles the requirement thatenvironmentshould not contribute to cache key equality while still being accessible for CFG construction.
| static final class CfgParams { | ||
|
|
||
| /** | ||
| * The {@link TreePath} representing the code location for which CFG-related | ||
| * parameters are being computed. | ||
| * | ||
| * <p>This field defines the logical identity of {@code CfgParams}. Equality, | ||
| * hash code computation, and string representation are all based solely on | ||
| * this field. | ||
| */ | ||
| private final TreePath codePath; | ||
|
|
||
| /** | ||
| * The {@link ProcessingEnvironment} associated with the current analysis. | ||
| * | ||
| * <p>This field is intentionally <em>not</em> final and is excluded from | ||
| * {@link #equals(Object)} and {@link #hashCode()} because: | ||
| * <ul> | ||
| * <li>It is injected after construction</li> | ||
| * <li>It is mutable</li> | ||
| * <li>It does not contribute to logical identity</li> | ||
| * </ul> | ||
| */ | ||
| private @Nullable ProcessingEnvironment environment; | ||
|
|
||
| private static CfgParams create(TreePath codePath, ProcessingEnvironment environment) { | ||
| CfgParams cp = new AutoValue_DataFlow_CfgParams(codePath); | ||
| /** | ||
| * Private constructor used internally to enforce controlled creation via | ||
| * {@link #create(TreePath, ProcessingEnvironment)}. | ||
| * | ||
| * @param codePath the {@link TreePath} identifying the analyzed code location | ||
| */ | ||
| private CfgParams(TreePath codePath) { | ||
| this.codePath = codePath; | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new {@code CfgParams} instance. | ||
| * | ||
| * <p>The {@link ProcessingEnvironment} is assigned after construction to | ||
| * preserve immutability of the identity-defining fields while still allowing | ||
| * environment injection. | ||
| * | ||
| * @param codePath the {@link TreePath} identifying the analyzed code location | ||
| * @param environment the {@link ProcessingEnvironment} for the current | ||
| * annotation processing round | ||
| * @return a fully initialized {@code CfgParams} instance | ||
| */ | ||
| static CfgParams create(TreePath codePath, ProcessingEnvironment environment) { | ||
| CfgParams cp = new CfgParams(codePath); | ||
| cp.environment = environment; | ||
| return cp; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the {@link ProcessingEnvironment} associated with this instance. | ||
| * | ||
| * <p>This method guarantees a non-null return value. It is an error to call | ||
| * this method before the environment has been set via {@link #create}. | ||
| * | ||
| * @return the non-null {@link ProcessingEnvironment} | ||
| */ | ||
| ProcessingEnvironment environment() { | ||
| return castToNonNull(environment); | ||
| } | ||
|
|
||
| abstract TreePath codePath(); | ||
| /** | ||
| * Returns the {@link TreePath} that defines the identity of this instance. | ||
| * | ||
| * @return the {@link TreePath} associated with this configuration | ||
| */ | ||
| TreePath codePath() { | ||
| return codePath; | ||
| } | ||
|
|
||
| /** | ||
| * Indicates whether some other object is equal to this one. | ||
| * | ||
| * <p>Equality is based solely on {@link #codePath}. The | ||
| * {@link ProcessingEnvironment} is intentionally excluded. | ||
| * | ||
| * @param o the reference object with which to compare | ||
| * @return {@code true} if this object is equal to the given object; | ||
| * {@code false} otherwise | ||
| */ | ||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof CfgParams)) { | ||
| return false; | ||
| } | ||
| CfgParams that = (CfgParams) o; | ||
| return java.util.Objects.equals(this.codePath, that.codePath); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a hash code value for this object. | ||
| * | ||
| * <p>The hash code is computed solely from {@link #codePath} to remain | ||
| * consistent with {@link #equals(Object)}. | ||
| * | ||
| * @return a hash code value for this object | ||
| */ | ||
| @Override | ||
| public int hashCode() { | ||
| return java.util.Objects.hash(codePath); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a string representation of this object. | ||
| * | ||
| * <p>The returned string includes only the identity-defining fields and omits | ||
| * the {@link ProcessingEnvironment} to keep the output stable and readable. | ||
| * | ||
| * @return a string representation of this {@code CfgParams} | ||
| */ | ||
| @Override | ||
| public String toString() { | ||
| return "CfgParams{codePath=" + codePath + "}"; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cd nullaway && find . -name "DataFlow.java" -type fRepository: uber/NullAway
Length of output: 113
🏁 Script executed:
wc -l nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway
Length of output: 124
🏁 Script executed:
sed -n '355,477p' nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway
Length of output: 4056
🏁 Script executed:
# Check for CfgParams usage throughout the file
rg -n "CfgParams" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway
Length of output: 1063
🏁 Script executed:
# Check for LoadingCache and synchronization in the file
rg -n "LoadingCache|synchronized|volatile" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java | head -20Repository: uber/NullAway
Length of output: 277
🏁 Script executed:
# Check imports to see what caching framework is used
head -50 nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java | grep -E "^import"Repository: uber/NullAway
Length of output: 1545
🏁 Script executed:
sed -n '93,155p' nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway
Length of output: 3507
🏁 Script executed:
# Check what happens in the CacheLoader.load() method
sed -n '97,155p' nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway
Length of output: 3349
🏁 Script executed:
# Check for all accesses to environment field or environment() method
rg -n "\.environment|environment\(\)" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.javaRepository: uber/NullAway
Length of output: 211
🏁 Script executed:
# Check if CfgParams instances are stored/retrieved outside of immediate cache usage
rg -n "CfgParams" nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java -A 2 -B 2Repository: uber/NullAway
Length of output: 2940
Simplify CfgParams by initializing environment in the constructor.
The environment field is mutable and non-final, but this is not a practical thread-safety concern here. Each call creates a fresh CfgParams instance that is immediately passed to the cache's load() method, where the environment field is read synchronously. The Guava LoadingCache handles all necessary synchronization internally, and CfgParams instances are never shared across threads.
That said, the post-construction initialization pattern (lines 402-406) adds unnecessary complexity. Passing environment as a constructor parameter would be simpler and clearer without affecting caching behavior, since only codePath contributes to the cache key.
🤖 Prompt for AI Agents
In @nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java around lines
355 - 477, Change CfgParams to take ProcessingEnvironment in the constructor and
make the environment field final: replace the single-arg private
CfgParams(TreePath) and the static create(TreePath, ProcessingEnvironment)
factory with a constructor CfgParams(TreePath, ProcessingEnvironment) that
assigns both final fields, update environment() to return the final field as
before, and remove the post-construction mutation paths (and calls to create) so
callers (e.g., code that calls CfgParams.create(...) before passing to the cache
load) instead call new CfgParams(codePath, environment); keep
equals/hashCode/toString based only on codePath.
| record AnalysisParams(ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) { | ||
|
|
||
| private static AnalysisParams create( | ||
| ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) { | ||
| AnalysisParams ap = new AutoValue_DataFlow_AnalysisParams(transferFunction, cfg); | ||
| return ap; | ||
| return new AnalysisParams(transferFunction, cfg); | ||
| } | ||
|
|
||
| abstract ForwardTransferFunction<?, ?> transferFunction(); | ||
|
|
||
| abstract ControlFlowGraph cfg(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing the redundant factory method.
The create() method simply wraps the record constructor without adding any logic. While it may preserve API compatibility with the previous AutoValue implementation, direct constructor usage is more idiomatic for records.
♻️ Optional: Call-sites can use the record constructor directly
At lines 158 and 350, replace:
-AnalysisParams aparams = AnalysisParams.create(transfer, cfg);
+AnalysisParams aparams = new AnalysisParams(transfer, cfg);Then remove the create() method entirely:
record AnalysisParams(ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) {
-
- private static AnalysisParams create(
- ForwardTransferFunction<?, ?> transferFunction, ControlFlowGraph cfg) {
- return new AnalysisParams(transferFunction, cfg);
- }
-
}🤖 Prompt for AI Agents
In @nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java around lines
479 - 486, The AnalysisParams.record defines a redundant static factory create()
that only delegates to the record constructor; remove the
create(ForwardTransferFunction<?, ?>, ControlFlowGraph) method and update all
call sites that use AnalysisParams.create(...) to use the record constructor
directly (new AnalysisParams(transferFunction, cfg)) or the canonical record
construction form with the same parameters, ensuring you update references in
the code paths that previously called create (e.g., the sites where
transferFunction and cfg are supplied).
|
Hi @msridhar, apologies for the delayed response. I was traveling over the past week with limited access to my development setup, which caused the slowdown on my side. |
|
@rishikraj990 thanks for the fixes! I think I fixed the test failure in 06ccd25. The JMH NullAway release benchmark runs the current version of NullAway to check the source code of an older release of NullAway. Since that older release still used AutoValue we need to preserve it in the benchmark configuration. |
msridhar
left a comment
msridhar
left a comment
There was a problem hiding this comment.
Things are looking great, just a couple more minor comments and then I think this will be good to go!
nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1415 +/- ##
============================================
- Coverage 88.44% 88.39% -0.05%
Complexity 2672 2672
============================================
Files 97 97
Lines 8852 8861 +9
Branches 1772 1774 +2
============================================
+ Hits 7829 7833 +4
- Misses 507 510 +3
- Partials 516 518 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2827-2845: Consider removing the factory method to eliminate redundancy.The factory method's only call site (lines 2472-2480) already creates immutable copies before passing arguments, making the
copyOf()calls in the factory redundant. WhileImmutableSet.copyOf()optimizes this case internally, it still adds unnecessary method call overhead.Since the factory is only used once, consider calling the record's canonical constructor directly at the call site and removing this factory method entirely.
♻️ Proposed refactor to call constructor directly
Remove the factory method and update the call site to use the canonical constructor:
At lines 2472-2480, change:
- return FieldInitEntities.create( + return new FieldInitEntities( classSymbol, ImmutableSet.copyOf(nonnullInstanceFields), ImmutableSet.copyOf(nonnullStaticFields), ImmutableList.copyOf(instanceInitializerBlocks), ImmutableList.copyOf(staticInitializerBlocks), ImmutableSet.copyOf(constructors), ImmutableSet.copyOf(instanceInitializerMethods), ImmutableSet.copyOf(staticInitializerMethods));Then remove lines 2821-2845 (the factory method).
🤖 Fix all issues with AI agents
In
@nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java:
- Around line 61-79: The create(...) factory on CollectLikeMethodRecord is a
simple pass-through to the record constructor; update call sites to instantiate
the record directly with new
CollectLikeMethodRecord(collectorFactoryMethodClass,
collectorFactoryMethodSignature, argsToCollectorFactoryMethod, innerMethodName,
argsFromStream) to remove the indirection, then remove or mark the static
create(...) method as deprecated once all usages are migrated; search for usages
of CollectLikeMethodRecord.create and replace them with the direct constructor
call and run tests to ensure no behavioral changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java
🔇 Additional comments (3)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2811-2846: LGTM! Clean migration from AutoValue to record.The
FieldInitEntitiesrecord is well-structured with comprehensive Javadoc. All components use immutable collection types, maintaining type safety and immutability guarantees.nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java (2)
28-52: LGTM: Excellent documentation.The Javadoc is comprehensive and well-structured, clearly explaining the purpose of the record and each component. Good use of {@link} references and HTML formatting.
53-59: This suggestion conflicts with NullAway's established patterns. All records in the codebase (MethodAnnotationsRecord, NestedAnnotationInfo, MapLikeMethodRecord, etc.) follow the same convention: no nullability annotations and no validation in constructors. The record is safe in practice because the ImmutableSet fields enforce non-null constraints via Guava's contracts, and records are constructed from controlled internal sources.Likely an incorrect or invalid review comment.
| /** | ||
| * Creates an immutable {@link CollectLikeMethodRecord} instance. | ||
| * | ||
| * <p>This factory method exists primarily for call-site clarity and symmetry with other model | ||
| * types. | ||
| */ | ||
| public static CollectLikeMethodRecord create( | ||
| String collectorFactoryMethodClass, | ||
| String collectorFactoryMethodSignature, | ||
| ImmutableSet<Integer> argsToCollectorFactoryMethod, | ||
| String innerMethodName, | ||
| ImmutableSet<Integer> argsFromStream) { | ||
| return new AutoValue_CollectLikeMethodRecord( | ||
| return new CollectLikeMethodRecord( | ||
| collectorFactoryMethodClass, | ||
| collectorFactoryMethodSignature, | ||
| argsToCollectorFactoryMethod, | ||
| innerMethodName, | ||
| argsFromStream); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider eventual migration to direct constructor usage.
The create() factory method is a pass-through to the record constructor, maintained for call-site clarity and API compatibility during the AutoValue-to-Record migration. While this is acceptable for now, consider eventually migrating call sites to use the record constructor directly (new CollectLikeMethodRecord(...)) to reduce indirection and maintenance overhead.
🤖 Prompt for AI Agents
In
@nullaway/src/main/java/com/uber/nullaway/handlers/stream/CollectLikeMethodRecord.java
around lines 61 - 79, The create(...) factory on CollectLikeMethodRecord is a
simple pass-through to the record constructor; update call sites to instantiate
the record directly with new
CollectLikeMethodRecord(collectorFactoryMethodClass,
collectorFactoryMethodSignature, argsToCollectorFactoryMethod, innerMethodName,
argsFromStream) to remove the indirection, then remove or mark the static
create(...) method as deprecated once all usages are migrated; search for usages
of CollectLikeMethodRecord.create and replace them with the direct constructor
call and run tests to ensure no behavioral changes.
Following up on my previous PR and the goal of issue uber#1425, I have refactored ContractsTests.java. - Migrated test data from external files to inline text blocks using addSourceLines. - Deleted physical test files: CheckContractPositiveCases.java and CheckContractNegativeCases.java. - Removed unnecessary @SuppressWarnings("deprecation") annotations. - Part of the ongoing effort to modernize tests (uber#1425). Thank you for contributing to NullAway! Please note that once you click "Create Pull Request" you will be asked to sign our [Uber Contributor License Agreement](https://cla-assistant.io/uber/NullAway) via [CLA assistant](https://cla-assistant.io/). Before pressing the "Create Pull Request" button, please provide the following: - [x] A description about what and why you are contributing, even if it's trivial. - [x] The issue number(s) or PR number(s) in the description if you are contributing in response to those. - [x] If applicable, unit tests. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Consolidated external test-data into inline test cases within the test suite for simpler maintenance and clearer test flow. * Removed separate public test-data classes that previously existed as external resources. * Dropped a deprecation-suppression annotation from the public test class to reflect current warnings handling. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
To stay up to date. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated development dependencies to their latest versions for improved code quality and analysis capabilities. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Went ahead and did the last edits on this one @rishikraj990 . Thanks again for the contribution!! |
4 participants
A description about what and why you are contributing: Migrate AutoValue to Java Records and for some place direct replacement as there are some strict guidelines to use Records
The issue number: #1400
Fixes #1400
Summary by CodeRabbit
Chores
Tests
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.