Skip to content

Fix false positive in AnnotateNullableParameters when parameter der…#868

Open
stefanodallapalma wants to merge 3 commits into
openrewrite:mainfrom
stefanodallapalma:fix-dereferncing-order-tracking-annotate-nullable-parameters
Open

Fix false positive in AnnotateNullableParameters when parameter der…#868
stefanodallapalma wants to merge 3 commits into
openrewrite:mainfrom
stefanodallapalma:fix-dereferncing-order-tracking-annotate-nullable-parameters

Conversation

@stefanodallapalma
Copy link
Copy Markdown
Contributor

@stefanodallapalma stefanodallapalma commented Apr 28, 2026

Fix false positive in AnnotateNullableParameters when parameter dereferenced before null check

Co-authored with Factory Droid 0.102.0.

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:

```java
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:

  1. Renamed visitor: NullCheckVisitorNullCheckAndDereferenceVisitor to reflect its expanded responsibilities

  2. 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
  3. Dereference detection: Added visitFieldAccess and enhanced visitMethodInvocation to detect when parameters are dereferenced via:

    • Method calls: param.method()
    • Field access: param.field
  4. 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)

  5. 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

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants