Skip to content

Apply library models to functional interface methods - #1903

Closed
subhramit wants to merge 4 commits into
uber:masterfrom
subhramit:modeled-functional-interface-method-type
Closed

subhramit wants to merge 4 commits into
uber:masterfrom
subhramit:modeled-functional-interface-method-type

Conversation

@subhramit

@subhramit subhramit commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #1722
Closes #1724
Refs. #1718

I was in context of #1724 so thought of finishing it.

Use library-modeled functional-interface method types when checking lambda and method-reference parameter and return compatibility.
Preserve substituted and inferred types so nullable parameters and returns are handled consistently.

Before pressing the "Create Pull Request" button, please provide the following:

  • A description about what and why you are contributing, even if it's trivial.

  • The issue number(s) or PR number(s) in the description if you are contributing in response to those.

  • If applicable, unit tests.

AI usage - Zed + GPT 5.6 Sol, mnaually driven. I verified the code and ran all the tests.

Summary by CodeRabbit

  • Bug Fixes
    • In JSpecify mode, nullability checks for lambdas and method references now account for modeled functional-interface types. This helps flag nullable values passed where non-null values are expected, nullable parameters that are dereferenced, and implementations whose parameter or return nullability conflicts with the interface model. Checks cover both inferred and explicitly typed lambda parameters. Outside JSpecify mode, these checks retain their previous behavior.

Signed-off-by: subhramit <subhramit.bb@live.in>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: uber/NullAway/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: a4e1cadc-64f4-4501-a157-670825616950
📥 Commits

Reviewing files that changed from the base of the PR and between 5ae6a9e and 160b578.

📒 Files selected for processing (4)
  • nullaway/src/main/java/com/uber/nullaway/NullAway.java
  • nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
  • test-library-models/src/main/java/com/uber/nullaway/testlibrarymodels/TestLibraryModels.java
  • test-library-models/src/test/java/com/uber/nullaway/CustomLibraryModelsTests.java

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


Walkthrough

In JSpecify mode, lambda and method-reference checks now use the modeled functional-interface method type during override validation. GenericsChecks resolves that type from the inferred-type cache or javac and applies handler-provided method models. The change also adds a modeled functional interface and a test for modeled nullability in method references and lambdas.

Suggested reviewers: msridhar, dbwiddis

Priority: ⬇️ Low

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 160b5

No actionable merge-blocking issue remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 5ae6a

The change affects compile-time nullness compatibility and preserves existing analysis gates. No introduced security weakness was identified in the inspected paths. The assessment remains bounded because broader security coverage is incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly affected exposure is compile-time analysis of eligible lambdas and method references under the existing checker configuration. The traced outcome is nullness compatibility validation; the inspected call chain does not introduce a runtime privilege or cross-service authority path.

Trust Boundaries and Controls

  • observed — Compiler input and inferred types are interpreted using the existing library-model handler. Nullable functional-interface parameters remain subject to compatibility checks. An unbound instance-method reference with a nullable receiver parameter still produces a diagnostic before the remaining parameters are mapped to referenced-method arguments.

Resilience and Maintainability Implications

  • observed — The new resolver reads the inferred-type cache without writing it. LibraryModelsHandler collects updated argument and return types locally and returns either the unchanged method type or a newly constructed MethodType. The resolver preserves generic variables around the modeled result.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: applying library models to functional-interface methods.
Description check ✅ Passed The description explains the use of modeled functional-interface method types and relates the change to the linked issue.
Linked Issues check ✅ Passed Issue [#1724] asks whether lambda and method-reference checks should use library models for the functional-interface method type. NullAway.java now supplies a modeled method type for those checks in…
Out of Scope Changes check ✅ Passed The whole-PR diff changes functional-interface method-type handling and adds a modeled functional interface with focused library-model tests. These changes implement and verify issue [#1724]. The diff…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@msridhar

msridhar commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

So I think this one overlaps with #1740, which has been sitting in a draft state for a long time. The reason is that one is stacked on #1739, which causes new warnings on all the integration tests, and I haven't had time to go through them. Really those two changes are kind of independent though.

So, we could push this one through in place of #1740. But, we'd need to dig in a bit as #1740 makes changes in more places than this one. Or, we can wait and get #1740 through along with #1739. WDYT @subhramit ?

@subhramit

subhramit commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor Author

So I think this one overlaps with #1740, which has been sitting in a draft state for a long time. The reason is that one is stacked on #1739, which causes new warnings on all the integration tests, and I haven't had time to go through them. Really those two changes are kind of independent though.

So, we could push this one through in place of #1740. But, we'd need to dig in a bit as #1740 makes changes in more places than this one. Or, we can wait and get #1740 through along with #1739. WDYT @subhramit ?

Hey, thanks for lmk. I went through the stack, and it seems to be the more complete solution. Unless this is a burning issue (which I assume it's not) I would be in favour of waiting for them and closing this.
However, only if you think if its important and that work there may take much more time, we can also consider independently landing this one to currently fix the issue and then rebasing the stack when you resume work there.

@subhramit

Copy link
Copy Markdown
Contributor Author

Closing in favor of a more complete solution in progress as described in #1903 (comment)
Can reopen if there are prolonged blockers.

@subhramit subhramit closed this Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Look into call sites passing null for overridden method type where they should be consulting library models

2 participants