Skip to content

Keep annotated and strictfp overrides that only call super - #988

Merged
timtebeek merged 2 commits into
openrewrite:mainfrom
martinfrancois:repro/remove-methods-only-call-super-annotations
Aug 17, 2026
Merged

Keep annotated and strictfp overrides that only call super#988
timtebeek merged 2 commits into
openrewrite:mainfrom
martinfrancois:repro/remove-methods-only-call-super-annotations

Conversation

@martinfrancois

@martinfrancois martinfrancois commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Suggested review order: 8 of 52 (Score: 8)
Review first: openrewrite/rewrite-migrate-java#1205

What's changed?

Updates RemoveMethodsOnlyCallSuper to preserve forwarding overrides when a parameter or any nested part of the method signature carries an annotation other than @Override.

The change also preserves a strictfp override when the invoked super method is not strictfp. It still removes the override when both methods are strictfp, matching the recipe's existing handling of synchronized.

The three former failing reproductions are now passing regression tests, and a positive control verifies the safe strictfp removal. All 20 tests in RemoveMethodsOnlyCallSuperTest pass.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.RemoveMethodsOnlyCallSuper.

#971 taught this recipe to keep overrides that add declaration annotations or the synchronized modifier. Its method-level annotation query does not include annotations on parameters or type-use annotations nested inside the return type, so the recipe still deletes overrides that carry those contracts.

Case 1: parameter annotation

Before

class Child extends Parent {
    @Override
    void foo(@Nullable String s) {
        super.foo(s);
    }
}

Actual after the recipe

class Child extends Parent {
}

Expected after the recipe

(unchanged)

Removing the method silently removes the parameter contract read by annotation processors and static-analysis tools. The same loss occurs for a nested type-use annotation such as public String @Nullable [] foo().

Case 2: strictfp modifier

Before

class Child extends Parent {
    @Override
    strictfp void foo() {
        super.foo();
    }
}

Actual after the recipe

The override is deleted, leaving the same empty Child class shown in Case 1.

Expected after the recipe

(unchanged) when the super method is not strictfp.

The previous transformation drops a reflection-visible modifier. Before Java 17, it also changes floating-point semantics. When the super method is already strictfp, removing this forwarding override remains safe and the recipe still removes it.

Confirmed real-world execution

The released recipe deletes a delegating override whose parameter annotations intentionally create the invalid constraint declaration tested by Hibernate Validator. The deletion removes the diagnostic condition and weakens the test.

Affected code in real projects

  • didi/DoKit CpuMainPageFragment.java: an onViewCreated override that only forwards to super but carries @NonNull and @Nullable parameter annotations. The recipe from main deletes the whole override, silently dropping the parameter nullability contracts it declares.
  • Justson/AgentWeb AgentActionFragment.java: the same shape in a library fragment, super.onViewCreated(view, savedInstanceState) with @NonNull/@Nullable annotated parameters. The recipe from main removes the method together with those annotations.

Anything in particular you'd like reviewers to focus on?

Please review the conservative signature traversal, which intentionally stops before the method body, and the decision to mirror the existing synchronized parent-method comparison for strictfp.

Have you considered any alternatives or workarounds?

One alternative is to enumerate specific signature locations. Traversing the complete signature is safer because an override carrying annotation metadata is not redundant, regardless of whether the annotation is attached to a parameter, type parameter, return type, or nested type use.

For strictfp, preserving every marked override would also retain behavior. Comparing the super method retains the recipe's cleanup when the modifier is redundant.

Any additional context

Pre-existing tests changed: None.

Related: #971 added the synchronized guard and the declaration-level annotation guard. This change extends the same behavior to the remaining signature locations and to strictfp.

This change was prepared with AI assistance. I reviewed the implementation, regression tests, focused test results, and contribution text.

Checklist

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Aug 11, 2026
@martinfrancois martinfrancois changed the title RemoveMethodsOnlyCallSuper: add failing tests for unseen annotations and strictfp Preserve annotated and strictfp overrides in RemoveMethodsOnlyCallSuper Aug 15, 2026
@martinfrancois
martinfrancois force-pushed the repro/remove-methods-only-call-super-annotations branch 2 times, most recently from 5a8030e to 134b35a Compare August 16, 2026 02:53
@martinfrancois
martinfrancois force-pushed the repro/remove-methods-only-call-super-annotations branch from 134b35a to b0a9d77 Compare August 16, 2026 12:23
@martinfrancois martinfrancois changed the title Preserve annotated and strictfp overrides in RemoveMethodsOnlyCallSuper Keep annotated and strictfp overrides that only call super Aug 16, 2026
@martinfrancois
martinfrancois marked this pull request as ready for review August 17, 2026 08:08

@timtebeek timtebeek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the double fixes here!

@github-project-automation github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Aug 17, 2026
@timtebeek
timtebeek merged commit bee07a4 into openrewrite:main Aug 17, 2026
1 check passed
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants