Skip to content

Use passed-in TreePath in one more place when available#1329

Merged
msridhar merged 2 commits intomasterfrom
minor-followup-path-fix
Nov 6, 2025
Merged

Use passed-in TreePath in one more place when available#1329
msridhar merged 2 commits intomasterfrom
minor-followup-path-fix

Conversation

@msridhar
Copy link
Copy Markdown
Collaborator

@msridhar msridhar commented Nov 5, 2025 •
edited by coderabbitai bot
Loading

Minor follow-up for #1328. When available, using path is more correct than state.getPath().

Summary by CodeRabbit

  • Bug Fixes
    • Improved nullability inference accuracy for non-static method calls through enhanced path resolution handling.

@msridhar msridhar enabled auto-merge (squash) November 5, 2025 00:41
@msridhar msridhar requested a review from yuxincs November 5, 2025 00:41
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 5, 2025

Walkthrough

The getEnclosingTypeForCallExpression method in GenericsChecks.java was updated to prioritize an explicitly provided path over the current error-prone state path when resolving the enclosing class during nullability inference at non-static method calls. The method now computes basePath as either the supplied path parameter (if non-null) or state.getPath(), then uses this basePath to locate the enclosing ClassTree. The resulting enclosing type remains cast to non-null as before. No changes were made to public/exported entity signatures or error handling logic.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: using a passed-in TreePath parameter instead of state.getPath() in the getEnclosingTypeForCallExpression method.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch minor-followup-path-fix
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6fa9f6 and 7e823b4.

📒 Files selected for processing (1)
  • nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1 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/generics/GenericsChecks.java
🔇 Additional comments (1)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java (1)

1602-1603: LGTM! Clean correctness improvement.

The change correctly prioritizes the explicitly provided path parameter over state.getPath(), which is consistent with how path is already used elsewhere in this method (e.g., line 1611). This follows the documented intent and improves correctness when a specific TreePath is available.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 5, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.42%. Comparing base (6c78a05) to head (38b2693).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ava/com/uber/nullaway/generics/GenericsChecks.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #1329      +/-   ##
============================================
- Coverage     88.43%   88.42%   -0.02%     
  Complexity     2566     2566              
============================================
  Files            96       96              
  Lines          8605     8605              
  Branches       1707     1708       +1     
============================================
- Hits           7610     7609       -1     
  Misses          500      500              
- Partials        495      496       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@msridhar msridhar merged commit a59df24 into master Nov 6, 2025
9 of 11 checks passed
@msridhar msridhar deleted the minor-followup-path-fix branch November 6, 2025 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuxincs yuxincs yuxincs approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@msridhar @yuxincs