InstanceOfPatternMatch changes code meaning with multiple reassignments

[lukeu]

What version of OpenRewrite are you using?

I am using

• Gradle plugin: v7.2.0
• OpenRewrite: latest - 8.48.0(?)
• rewrite-migrate-java: latest - 3.4.0(?)

How are you running OpenRewrite?

I am running Gradle in a multi-module project:

plugins {
    id 'org.openrewrite.rewrite' version '7.2.0' apply false
}

def javaProjects = subprojects - project(":release").allprojects
configure (javaProjects) {
    apply plugin: 'java'
    apply plugin: 'checkstyle'
    apply plugin: 'org.openrewrite.rewrite'

    sourceCompatibility = 21
    targetCompatibility = 21

    rewrite {
        activeRecipe("org.openrewrite.staticanalysis.InstanceOfPatternMatch")
    }

    dependencies {
        rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:latest.release"))
        rewrite("org.openrewrite.recipe:rewrite-testing-frameworks")
        rewrite("org.openrewrite.recipe:rewrite-migrate-java")
    }

What is the smallest, simplest way to reproduce the problem?

    public static void main(String[] args) {
        Object o = -3;
        if (o instanceof Integer) {
            o = Math.abs((int) o) * 10;
            o = Math.max((int) o, 5);
        }
        System.out.println(o);
    }

What did you expect to see?

Perhaps unchanged? Regardless the code should print 30

What did you see instead?

The is modified to the following, and prints 5

    public static void main(String[] args) {
        Object o = -3;
        if (o instanceof Integer integer) {
            o = Math.abs(integer) * 10;
            o = Math.max(integer, 5);
        }
        System.out.println(o);
    }

Are you interested in contributing a fix to OpenRewrite?

Not at this stage sorry, but perhaps in the future if I become a more regular user. (From a glance at some rule implementations, the pattern matching engine indeed looks interesting and powerful!)

At this stage I'm just running some initial trials and was lucky to spot (the less simple variant of) this amongst a fairly large changeset.

[knutwannheden]

Good catch! If the instanceof checks a variable and the if statement's body contains any assignments to that variable, then the best thing would probably be to not apply the recipe to that instanceof check.

[lukeu]

Good catch! If the instanceof checks a variable and the if statement's body contains any assignments to that variable, then the best thing would probably be to not apply the recipe to that instanceof check.

I think not quite that simple, since I spotted many valid single-assignments updating the tested variable which would be nice to still be cleaned up. To pick one example:

        // Drop the optional Unary Plus token
        if (expr instanceof PlusUnary) {
            expr = ((PlusUnary) expr).single();
        }

So I think the ideal rule would be to consider multiple reassignments. I guess this could get complex when using the (!(o instanceof SomeType)) { ... } form this would need to consider the flow control of everything following the if / ternary

[knutwannheden]

So I think the ideal rule would be to consider multiple reassignments. I guess this could get complex when using the (!(o instanceof SomeType)) { ... } form this would need to consider the flow control of everything following the if / ternary

Agreed on both points.