rewrite-migrate-java icon indicating copy to clipboard operation
rewrite-migrate-java copied to clipboard

PreferJavaUtilObjectsEquals does not check for existing equals method

Open timo-abele opened this issue 1 year ago • 2 comments

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

                <plugin>
                    <groupId>org.openrewrite.maven</groupId>
                    <artifactId>rewrite-maven-plugin</artifactId>
                    <version>5.23.1</version>
                    <configuration>
                        <activeRecipes>
                            <recipe>org.openrewrite.java.migrate.guava.NoGuavaJava11</recipe>
                        </activeRecipes>
                        <failOnDryRunResults>true</failOnDryRunResults>
                    </configuration>
                    <dependencies>
                        <dependency>
                            <groupId>org.openrewrite.recipe</groupId>
                            <artifactId>rewrite-migrate-java</artifactId>
                            <version>2.10.0</version>
                        </dependency>
                    </dependencies>
                </plugin>

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

I haven't compiled this, but I hope the point gets across: the unconditional static import of java.util.Objects.equals is shadowed by the existing equals method

import static com.google.common.base.Objects.equal;

class A {
    int foo;
    int ba;

    @Override
    boolean equals(A other) {
        //omitting the nullchecks
        return equal(foo, other.foo) && equal(ba, other.ba);
    }
}

What did you expect to see?

import java.util.Objects;

class A {
    int foo;
    int ba;

    @Override
    boolean equals(A other) {
        //omitting the nullchecks
        return Objects.equals(foo, other.foo) && Objects.equals(ba, other.ba);
    }
}

What did you see instead?

import static java.util.Objects.equals; //unused!

class A {
    int foo;
    int ba;

    @Override
    boolean equals(A other) {
        //omitting the nullchecks
        return equals(foo, other.foo) && equals(ba, other.ba);
    }
}

Are you interested in contributing a fix to OpenRewrite?

No, just letting you know.

timo-abele avatar Mar 20 '24 17:03 timo-abele

Hi! Thanks for the report; I'm looking at your samples and how we've implemented this recipe https://github.com/openrewrite/rewrite-migrate-java/blob/c82c73a85d7f5f13523d09a4709bf61dba9f7b53/src/main/resources/META-INF/rewrite/no-guava.yml#L189-L194

With the example above I see one equals(Object) method and an import of an equals(Object, Object) method, that I think would not clash here because of the differing number of arguments; Does your experience there differ?

timtebeek avatar May 06 '24 10:05 timtebeek

I found the original code again! The number of arguments does seem to be an issue for Java, the (IntelliJ) error message is "Expected 1 arguments but found 2", so I assume java cannot handle the same method name, defined in a class and statically imported from somewhere else with a different number of parameters. Maven error is:

[ERROR] /C:/Users/.../A.java:[63,16] method equals in class com.myorg.A cannot be applied to given types;
  required: java.lang.Object
  found: com.myorg.Foo,com.myorg.Foo
  reason: actual and formal argument lists differ in length

I've observed this in the original code with a custom type (Foo) and Strings. I hope this helps, let me know if you can't reproduce this.

timo-abele avatar May 06 '24 14:05 timo-abele