Conversation
WalkthroughAdds cached hashCode fields and private computeHashCode() methods to AccessPath and FieldOrMethodCallElement, initializing cachedHashCode in constructors and returning it from hashCode(). Refactors NullnessStore.leastUpperBound for single-pass merging (iterating the smaller map), adds an identity fast-path in equals, and switches Builder to ImmutableMap.Builder, removing null-checks and using buildKeepingLast(). Updates an error message formatting in EnclosingEnvironmentNullness. Bumps Guava to 31.1-jre and adds Guava to oldest Error Prone test classpaths in related Gradle files. Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1358 +/- ##
============================================
+ Coverage 88.39% 88.47% +0.07%
- Complexity 2594 2602 +8
============================================
Files 98 98
Lines 8705 8727 +22
Branches 1732 1736 +4
============================================
+ Hits 7695 7721 +26
+ Misses 505 503 -2
+ Partials 505 503 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java(3 hunks)
🧰 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/dataflow/NullnessStore.java
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java (1)
223-231: LGTM!The reference equality fast-path is a standard and effective optimization for
equalsmethods.
…e.java var renaming Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java (2)
202-214: Please remove the commented-out legacyleastUpperBoundimplementation.The old implementation is fully preserved in version control; keeping it commented out here adds noise to a performance-sensitive method and makes the current implementation harder to scan.
- // NullnessStore.Builder result = NullnessStore.empty().toBuilder(); - // for (AccessPath ap : intersection(contents.keySet(), other.contents.keySet())) { - // Nullness apContents = contents.get(ap); - // if (apContents == null) { - // throw new RuntimeException("null contents for " + ap); - // } - // Nullness otherAPContents = other.contents.get(ap); - // if (otherAPContents == null) { - // throw new RuntimeException("null other contents for " + ap); - // } - // result.contents.put(ap, apContents.leastUpperBound(otherAPContents)); - // } - // return result.build();
355-356: TODO aboutImmutableMap.Builderis reasonable; consider tracking as a follow-up.Leaving the TODO here is fine for this PR, but since this class is on hot paths, switching the builder to
ImmutableMap.Builder(with explicit semantics for duplicate keys) seems worth tracking in a separate optimization issue.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/dataflow/NullnessStore.java
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java (1)
224-226:this == ofast-path inequalsis safe and appropriate.Given
NullnessStoreis immutable, the reference-equality short-circuit is correct and avoids the map equality check in the common self-comparison case (e.g., in hash-based collections).
These were showing up in profiles. Note that one of the fixes requires updating our minimum supported Guava version to 31.1. This was released in February 2022, and mostly Guava is backward compatible these days, so hopefully it's fine for users.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.