Fix false positive in `AnnotateNullableParameters` when parameter der… by stefanodallapalma · Pull Request #868 · openrewrite/rewrite-static-analysis

stefanodallapalma · 2026-04-28T15:29:40Z

Fix false positive in `AnnotateNullableParameters` when parameter dereferenced before null check

Co-authored with Factory Droid 0.102.0.

Relates to: bugfix: false positive in AnnotateNullableParameters when parameter … #865

Problem

The AnnotateNullableParameters recipe had a false positive: it would annotate parameters as @Nullable even when they were dereferenced (via method calls or field access) before being null-checked. This could lead to incorrect annotations that suggest a parameter can be null when it would actually cause a NullPointerException.

Example of the false positive from actual code:

public SchemeFeeResult calculateSchemeFee(
            String acquirerAccountCode,
            McSmsBatchDataCoreRecord coreRecord,
 +         @CheckForNull McSmsBatchDataPostingAddendum postingAddendumRecord,  // WRONG ANNOTATION DUE TO postingAddendumRecord.getLineNumber()
            McSmsBatchDataAdditionalDataAddendum additionalDataAddendumRecord,
            @CheckForNull McSmsBatchDataFeeAddendum feeAddendumRecord)
            throws ParseException {
        final Account acquirerAccount = PspAccountTypes.AcquirerAccount.getAccount(acquirerAccountCode);

        if (acquirerAccount == null) {
            throw new ParseExceptionWithContext(
                            "Failed to calculate scheme fee, acquirer account not found",
                            postingAddendumRecord.getLineNumber()) // PARAM DEREFERENCED HERE
                    .with(LogFields.acquirerAccountCode, acquirerAccountCode);
        }
        // ...
        context.merchantCountryCode(
                postingAddendumRecord == null ? null : postingAddendumRecord.getMerchantCountryCode()); // CORRECT NULL-CHECK, BUT NOT ENOUGH FOR THE PARAM TO BE ANNOTATED
        if (null != postingAddendumRecord) { // CORRECT NULL-CHECK, BUT NOT ENOUGH FOR THE PARAM TO BE ANNOTATED
            String posData = postingAddendumRecord.getPOSData();
            //...

Previously, the recipe would incorrectly add @CheckForNull to postingAddendumRecord even though postingAddendumRecord.getLineNumber() is called before the null check, making the annotation misleading.

Solution

Refactored the internal visitor to track both null checks and dereferences in a single pass:

Renamed visitor: NullCheckVisitor → NullCheckAndDereferenceVisitor to reflect its expanded responsibilities
State tracking via Cursor messaging: Replaced the reduce pattern with stateful tracking using three keys:
- NULL_CHECKED_KEY - parameters that have been null-checked
- DEREFERENCED_KEY - parameters that have been dereferenced before null-check
- NULL_CHECK_STATE_KEY - tracks null-check status during AST traversal
Dereference detection: Added visitFieldAccess and enhanced visitMethodInvocation to detect when parameters are dereferenced via:
- Method calls: param.method()
- Field access: param.field
Refined null-checking method handling: Separated Objects.requireNonNullElse and Objects.requireNonNullElseGet into FIRST_ARG_NULLABLE_MATCHERS since only their first argument can be null (second is the non-null fallback)
Final filtering: getNullCheckedIdentifiers() now filters out parameters that were dereferenced before being null-checked

Testing

noChangeWhenParameterDereferencedBeforeNullCheck() - verifies the recipe doesn't annotate parameters dereferenced before null checks

All existing tests pass, confirming backward compatibility.

Requested reviewer

@timtebeek

…eferenced before null check

…ers` Replace cursor-message state with plain instance fields keyed by `JavaType.Variable` so scope-shadowed identifiers stay distinct, drop the redundant method-invocation/unary arms of `handleCondition` (now reached via the standard visitor traversal), and remove the narrative comments that duplicated what the code already says.

…lable-parameters

…notateNullableParameters

… 2.36.0 to 2.37.0 [skip ci] Bumps [org.openrewrite.recipe:rewrite-static-analysis](https://github.com/openrewrite/rewrite-static-analysis) from 2.36.0 to 2.37.0. Release notes *Sourced from [org.openrewrite.recipe:rewrite-static-analysis's releases](https://github.com/openrewrite/rewrite-static-analysis/releases).* > 2.37.0 > ------ > > What's Changed > -------------- > > * Preserve raw supertype cast on parameterized argument in `RemoveRedundantTypeCast` by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#915](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/915) > * Preserve space when InstanceOfPatternMatch replaces a cast after a keyword by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#913](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/913) > * `InstanceOfPatternMatch` not to remove double primitive cast by [`@greg-at-moderne`](https://github.com/greg-at-moderne) in [openrewrite/rewrite-static-analysis#918](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/918) > * Fix UseDiamondOperator throwing array index out of bounds when operating on a varargs method by [`@sambsnyd`](https://github.com/sambsnyd) in [openrewrite/rewrite-static-analysis#919](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/919) > * Fix false positive in `AnnotateNullableParameters` when parameter der… by [`@stefanodallapalma`](https://github.com/stefanodallapalma) in [openrewrite/rewrite-static-analysis#868](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/868) > > **Full Changelog**: <openrewrite/rewrite-static-analysis@v2.36.0...v2.37.0> Commits * [`3d2b173`](openrewrite/rewrite-static-analysis@3d2b173) Fix false positive in `AnnotateNullableParameters` when parameter der… ([#868](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/868)) * [`6d76624`](openrewrite/rewrite-static-analysis@6d76624) Fix UseDiamondOperator throwing array index out of bounds when operating on a... * [`dfc6176`](openrewrite/rewrite-static-analysis@dfc6176) `InstanceOfPatternMatch` not to remove double primitive cast ([#918](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/918)) * [`49d1bdc`](openrewrite/rewrite-static-analysis@49d1bdc) Preserve space when InstanceOfPatternMatch replaces a cast after a keyword (#... * [`23503ce`](openrewrite/rewrite-static-analysis@23503ce) Preserve raw supertype cast on parameterized argument in `RemoveRedundantType... * [`64e3203`](openrewrite/rewrite-static-analysis@64e3203) git-ignore .context/ * See full diff in [compare view](openrewrite/rewrite-static-analysis@v2.36.0...v2.37.0) [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility\_score?dependency-name=org.openrewrite.recipe:rewrite-static-analysis&package-manager=maven&previous-version=2.36.0&new-version=2.37.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Fix false positive in AnnotateNullableParameters when parameter der…

8a5a958

…eferenced before null check

moderne-meeseeks Bot added this to OpenRewrite Apr 28, 2026

moderne-meeseeks Bot assigned stefanodallapalma Apr 28, 2026

github-project-automation Bot moved this to In Progress in OpenRewrite Apr 28, 2026

timtebeek self-requested a review April 28, 2026 15:44

timtebeek added 3 commits April 30, 2026 14:26

Merge branch 'main' into fix-dereferncing-order-tracking-annotate-nul…

f6f9903

…lable-parameters

Document lexical dereference tracking and add guard-clause test in An…

22ed69d

…notateNullableParameters

timtebeek merged commit 3d2b173 into openrewrite:main Jun 16, 2026
1 check passed

github-project-automation Bot moved this from In Progress to Done in OpenRewrite Jun 16, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix false positive in `AnnotateNullableParameters` when parameter der…#868

Fix false positive in `AnnotateNullableParameters` when parameter der…#868
timtebeek merged 4 commits into
openrewrite:mainfrom
stefanodallapalma:fix-dereferncing-order-tracking-annotate-nullable-parameters

stefanodallapalma commented Apr 28, 2026 •

edited by timtebeek

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

stefanodallapalma commented Apr 28, 2026 • edited by timtebeek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix false positive in AnnotateNullableParameters when parameter dereferenced before null check

Problem

Solution

Testing

Requested reviewer

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stefanodallapalma commented Apr 28, 2026 •

edited by timtebeek

Loading

Fix false positive in `AnnotateNullableParameters` when parameter dereferenced before null check