Preserve assertion owners and receiver evaluation in assertInstanceOf migration

[martinfrancois]

Suggested review order: 47 of 52 (Score: 1)
Review first: openrewrite/rewrite-static-analysis#994

What's changed?

Adds 2 known-failing tests to AssertTrueInstanceofToAssertInstanceOfTest that reproduce two defects in AssertTrueInstanceofToAssertInstanceOf:

• qualifyWhenAssertInstanceOfDeclaredInClass: the migrated unqualified call binds to a same-name method declared in the class instead of JUnit's Assertions.assertInstanceOf.
• noChangeWhenInstanceReceiverHasSideEffect: migrating getAssertions().assertTrue(...) deletes the receiver expression, so its side effects are lost.

No recipe code changes. The tests are marked @Disabled so the suite stays green; removing the annotation shows the failure. The class had 6 tests before and has 8 now; all 6 pre-existing tests still pass.

What's your motivation?

Recipe: the JUnit assertTrue(instanceof) to assertInstanceOf migration.

Case 1: keep the JUnit assertion owner

Before

This code calls JUnit's statically imported assertTrue, and the class also declares its own assertInstanceOf:

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertTrue;

class ATest {
    static void assertInstanceOf(Class<?> type, Object value) {
        throw new AssertionError("wrong owner");
    }

    @Test
    void test() {
        Object obj = "example";
        assertTrue(obj instanceof String);
    }
}

Actual after the recipe

Using current main.

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertInstanceOf;

class ATest {
    static void assertInstanceOf(Class<?> type, Object value) {
        throw new AssertionError("wrong owner");
    }

    @Test
    void test() {
        Object obj = "example";
        assertInstanceOf(String.class, obj);
    }
}

Expected after the recipe

Assertions.assertInstanceOf(String.class, obj);

Per JLS 6.5.7.1, a method declared in the class shadows a single static import, so the new unqualified call binds to the local method. A compiled probe against junit-jupiter-api 5.13.3 confirmed it and printed "local method wins over static import". The output compiles, so the behavior change is silent. After the recipe runs, the code should call the qualified Assertions.assertInstanceOf(String.class, obj), which is what the test pins.

Case 2: preserve receiver evaluation

Before

The input contains getAssertions().assertTrue(obj instanceof String);, where getAssertions() prints "side effect" and returns null.

Actual after the recipe

Output from current main:

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertInstanceOf;

class ATest {
    Assertions getAssertions() {
        System.out.println("side effect");
        return null;
    }

    @Test
    void test() {
        Object obj = "example";
        assertInstanceOf(String.class, obj);
    }
}

Expected after the recipe

(unchanged)

The receiver expression is gone. Java evaluates the receiver of an instance call even when the resolved method is static (JLS 15.12.4.1), so the original code prints before asserting and the migrated code does not. A compiled probe confirmed both forms. The test pins no change as the safe behavior.

The cause is the recipe's JavaTemplate: mi.getCoordinates().replace() with an unqualified assertInstanceOf(...) template plus maybeAddImport replaces the whole invocation, including any select.

Found while preparing #1083, which fixes a related defect in this recipe (the qualified Assertions.assertTrue(...) path) and lists these two remaining paths in its description.

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

I think both are genuine bugs: the output compiles, but runtime behavior changes, either by calling a different method or by skipping the receiver's evaluation. If you agree this should change, I would gladly prepare the fix. If this behavior is intended, feel free to close this and I know it is settled.

Have you considered any alternatives or workarounds?

For the second defect, a receiver-preserving migration is possible, but it would keep the static-via-instance call style. Pinning no change looked safer; I am happy to implement either.

Any additional context

Pre-existing tests changed: None.

The tests fail the same way on current main (ded10c4) and on top of #1083. If #1083 merges first, the second test's wrong output becomes org.junit.jupiter.api.Assertions.assertInstanceOf(...), with the receiver still dropped, so the reproduction stays valid. The merged #1044 and the issues #459 and #515 it addressed show that same-name shadowing is treated as a real bug class here. The tests use @Disabled with the failure reason, because junit-pioneer's @ExpectedToFail is not on this repository's test classpath.

This reproduction was prepared with AI assistance (Claude Code). I reviewed the tests and this description.

The added reproduction tests and the existing suite together cover changed and unchanged behavior. The known-failing tests remain disabled until implementation. The formatter run was calibrated per file; untouched lines were not reformatted.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch