Remove BaseNoOpHandler and replace with default methods in Handler by msridhar · Pull Request #1376 · uber/NullAway

msridhar · 2025-12-14T18:31:52Z

Cleanup refactoring, no behavior changes

Summary by CodeRabbit

Refactor
- Internal architectural improvements to handler implementation structure.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

yuxincs

Nice!

coderabbitai · 2025-12-14T18:38:28Z

Walkthrough

This pull request refactors the handler architecture by eliminating the intermediate BaseNoOpHandler abstract base class and promoting its default no-op implementations into the Handler interface as default methods. Approximately 16 concrete handler classes are updated to implement Handler directly instead of extending BaseNoOpHandler. The Handler interface now provides default implementations for all previously abstract methods, allowing implementers to selectively override only the hooks they need rather than extending a base class.

Possibly related PRs

PR #1332: Modifies handler codebase including ContractCheckHandler.java to add \@nullable annotations to visitor generics and methods in handler classes.

Suggested reviewers

yuxincs
lazaroclapp

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main change: removing BaseNoOpHandler class and moving its no-op method implementations into the Handler interface as default methods.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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 no-more-basenoophandler

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 938a897 and 27cdd0c.

📒 Files selected for processing (17)

nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java (2 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java (0 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/FieldInitializationSerializationHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/GrpcHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/GuavaAssertionsHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java (20 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/SynchronousCallbackHandler.java (1 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.java (2 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java (2 hunks)
nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java (2 hunks)

💤 Files with no reviewable changes (1)

nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java

🧰 Additional context used

🧠 Learnings (3)

📚 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
nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

📚 Learning: 2025-08-29T18:41:43.584Z

Learnt from: msridhar
Repo: uber/NullAway PR: 1259
File: jdk-recent-unit-tests/src/test/java/com/uber/nullaway/jdk17/SwitchTests.java:318-321
Timestamp: 2025-08-29T18:41:43.584Z
Learning: Classes annotated with NullMarked are analyzed by NullAway even if they are not in packages specified by the AnnotatedPackages configuration. The NullMarked annotation guarantees NullAway analysis.

Applied to files:

nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.java

📚 Learning: 2025-08-14T18:50:06.159Z

Learnt from: msridhar
Repo: uber/NullAway PR: 1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.

Applied to files:

nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.java

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Build and test on windows-latest
GitHub Check: Build caffeine with snapshot
GitHub Check: Build and test on macos-latest
GitHub Check: Build and test on ubuntu-latest

🔇 Additional comments (20)

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java (2)

61-473: Default no-op/passthrough implementations in Handler look consistent and preserve handler chaining semantics
Void hooks no-op; override hooks return the incoming value; dataflow hooks return UNKNOWN / passthrough builder/node; predicates default to FALSE_AP_PREDICATE; dereference returns Optional.empty()—all consistent with “opt-in overrides.”

61-473: No action needed. The Handler interface correctly provides default no-op implementations for all methods, allowing handlers to implement only the methods they require. No BaseNoOpHandler class exists or needs to be maintained.

Likely an incorrect or invalid review comment.

nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java (1)

38-38: Migration to implements Handler is clean here (only required hooks overridden)

nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java (1)

69-69: implements Handler switch is straightforward; overrides remain explicit

nullaway/src/main/java/com/uber/nullaway/handlers/GuavaAssertionsHandler.java (1)

17-17: implements Handler migration looks correct; CFG hook override still matches the interface

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)

75-75: Implements Handler directly: looks consistent with the refactor goal.

No concerns with the signature swap itself.

nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.java (1)

80-80: Implements Handler directly: LGTM.

nullaway/src/main/java/com/uber/nullaway/handlers/FieldInitializationSerializationHandler.java (1)

53-53: Implements Handler directly: LGTM.

nullaway/src/main/java/com/uber/nullaway/handlers/LombokHandler.java (1)

23-23: Implements Handler directly: LGTM.

nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java (1)

43-43: Implements Handler directly: LGTM.

nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractCheckHandler.java (1)

40-56: Import + implements Handler: LGTM.

nullaway/src/main/java/com/uber/nullaway/handlers/SynchronousCallbackHandler.java (1)

25-25: Implements Handler directly: LGTM.

nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java (2)

41-83: Import + implements Handler: LGTM.

83-83: Verified: Java source/target compatibility confirmed and BaseNoOpHandler fully removed.

✓ No BaseNoOpHandler references remain in the codebase
✓ Java compatibility set to VERSION_11 in build.gradle (lines 111-112), supporting default interface methods

nullaway/src/main/java/com/uber/nullaway/handlers/AbstractFieldContractHandler.java (3)

49-49: LGTM! Class signature correctly updated.

The change from extending BaseNoOpHandler to implementing Handler is syntactically correct and aligns with the PR's refactoring objective.

95-95: LGTM! Correct syntax for invoking interface default method.

The change to Handler.super.onMatchMethod(...) is the proper Java syntax for calling a default method from an implemented interface.

49-95: Handler refactoring is complete and consistent across all implementations.

All 16 handler classes have been successfully updated to implement Handler directly with no remaining references to BaseNoOpHandler. The Handler.super.onMatchMethod() syntax is correctly applied in AbstractFieldContractHandler, and subclasses properly use standard super() calls to invoke parent behavior. No further action required.

nullaway/src/main/java/com/uber/nullaway/handlers/ApacheThriftIsSetHandler.java (1)

51-51: LGTM! Class signature correctly updated.

The change from extending BaseNoOpHandler to implementing Handler is correct. Since this class doesn't invoke any super methods, no additional changes are required.

nullaway/src/main/java/com/uber/nullaway/handlers/temporary/FluentFutureHandler.java (2)

12-12: LGTM! Import correctly updated.

The import statement has been correctly updated from BaseNoOpHandler to Handler to match the class signature change.

35-35: LGTM! Class signature correctly updated.

The change from extending BaseNoOpHandler to implementing Handler is correct. Since this class doesn't invoke any super methods, no additional changes are required.

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 · 2025-12-14T18:40:03Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.47%. Comparing base (9c41467) to head (7345f99).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1376      +/-   ##
============================================
- Coverage     88.47%   88.47%   -0.01%     
+ Complexity     2619     2618       -1     
============================================
  Files            98       97       -1     
  Lines          8774     8772       -2     
  Branches       1750     1750              
============================================
- Hits           7763     7761       -2     
  Misses          504      504              
  Partials        507      507

☔ 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 added 2 commits December 14, 2025 10:29

Remove BaseNoOpHandler and replace with default methods in Handler

ff1cf53

comment

27cdd0c

msridhar marked this pull request as ready for review December 14, 2025 18:33

msridhar requested a review from yuxincs December 14, 2025 18:33

yuxincs approved these changes Dec 14, 2025

View reviewed changes

Merge branch 'master' into no-more-basenoophandler

7345f99

msridhar enabled auto-merge (squash) December 14, 2025 18:47

msridhar merged commit ce56ccd into master Dec 14, 2025
10 of 11 checks passed

msridhar deleted the no-more-basenoophandler branch December 14, 2025 18:58

coderabbitai bot mentioned this pull request Dec 27, 2025

Enable PatternMatchingInstanceOf check and fix all warnings #1412

Merged

coderabbitai bot mentioned this pull request Mar 2, 2026

Fix library modeling for varargs arrays #1485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove BaseNoOpHandler and replace with default methods in Handler#1376

Remove BaseNoOpHandler and replace with default methods in Handler#1376
msridhar merged 3 commits intomasterfrom
no-more-basenoophandler

msridhar commented Dec 14, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

yuxincs left a comment

Uh oh!

coderabbitai bot commented Dec 14, 2025

Uh oh!

codecov bot commented Dec 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

msridhar commented Dec 14, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

yuxincs left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Dec 14, 2025

Walkthrough

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

msridhar commented Dec 14, 2025 •

edited by coderabbitai bot

Loading

codecov bot commented Dec 14, 2025 •

edited

Loading