Add explicit imports to avoid conflicts with classes added to `java.lang`, like `Record`

[timtebeek]

What problem are you trying to solve?

Folks using com.something.Record through import com.something.*; might find that the java.lang.Record is picked up when they migrate to Java 16+. Same for other classes added to the JDK:

• Record added in Java 16
  • https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Record.html
  • https://bugs.openjdk.org/browse/JDK-8233436
• WrongThreadException added in Java 19
  • https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/WrongThreadException.html
  • https://bugs.openjdk.org/browse/JDK-8284169
• MatchException added in Java 21
  • https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/MatchException.html
  • https://bugs.openjdk.org/browse/JDK-8284528

What precondition(s) should be checked before applying this recipe?

Using any conflicting classes on older Java versions

Describe the situation before applying the recipe

import com.something.*;
class A {
    void foo(Record r) {
    }
}

Describe the situation after applying the recipe

import com.something.Record;
class A {
    void foo(Record r) {
    }
}

Any additional context

Reported via email, with an example use case in https://github.com/fmcdevops/medrec/blob/master/src/common/value/com/bea/medrec/value/Record.java

[junior]

This is something to be included on the "Migrate to Java 17" recipe or will be standalone recipe?

[timtebeek]

I'd implement this as a recipe that takes in an argument, which we then add to the Java 17 and 21 migrations.

[timtebeek]

As a quick starter; here's how we'd test this type of recipe

import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.java.JavaParser;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.java.Assertions.javaVersion;

class AvoidConflictingImportsTest implements RewriteTest {

    @Override
    public void defaults(RecipeSpec spec) {
        try {
            JavaParser.Builder<? extends JavaParser, ?> parser = ((JavaParser.Builder<? extends JavaParser, ?>) Class
              .forName("org.openrewrite.java.Java11Parser")
              .getDeclaredMethod("builder")
              .invoke(null));
            spec.recipe(new AvoidConflictingImports("java.lang.Record"))
              .parser(parser);
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }

    @DocumentExample
    @Test
    void importFromSamePackage() {
        rewriteRun(
          //language=java
          java(
            """
              package com.acme.bank;
              public class Record {}
              """
          ),
          //language=java
          java(
            """
              package com.acme.bank;
              
              class Foo {
                  Record r;
              }
              """,
            """
              package com.acme.bank;
              
              import com.acme.bank.Record;
              
              class Foo {
                  Record r;
              }
              """,
            spec -> spec.markers(javaVersion(11))
          )
        );
    }

    private static JavaParser.Builder<? extends JavaParser, ?> java8Parser() throws Exception {
        return ((JavaParser.Builder<? extends JavaParser, ?>) Class
          .forName("org.openrewrite.java.Java11Parser")
          .getDeclaredMethod("builder")
          .invoke(null));
    }

    @Test
    void importFromDifferentPackage() {
        rewriteRun(
          //language=java
          java(
            """
              package com.acme.bank;
              public class Record {}
              """
          ),
          //language=java
          java(
            """
              import com.acme.bank.*;
              
              class Foo {
                  Record r;
              }
              """,
            """
              import com.acme.bank.Record;
              
              class Foo {
                  Record r;
              }
              """
          )
        );
    }
}

And then beyond that we'd need to implement the visitor to actually add the unambiguous import that avoids java.lang.Record.

package org.openrewrite.java.migrate.lang;

import lombok.EqualsAndHashCode;
import lombok.Value;
import org.openrewrite.*;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.search.UsesType;
import org.openrewrite.java.tree.J;

@Value
@EqualsAndHashCode(callSuper = false)
public class AvoidConflictingImports extends Recipe {

    @Option(displayName = "Import",
            description = "The fully qualified name of the class introduced.",
            example = "java.lang.Record")
    String fullyQualifiedClassName;

    @Override
    public String getDisplayName() {
        return "Avoid conflicting imports";
    }

    @Override
    public String getDescription() {
        return "Add explicit imports for classes that might otherwise conflict with the argument FQCN.";
    }

    @Override
    public TreeVisitor<?, ExecutionContext> getVisitor() {
        return Preconditions.check(Preconditions.and(
                new UsesType<>("*..Record", false),
                Preconditions.not(new UsesType<>("java.lang.Record", false))
        ), new JavaIsoVisitor<ExecutionContext>() {
            @Override
            public J.CompilationUnit visitCompilationUnit(J.CompilationUnit compilationUnit, ExecutionContext ctx) {
                J.CompilationUnit cu = super.visitCompilationUnit(compilationUnit, ctx);
                // TODO Enforce addition of import for classes that conflict with java.lang.Record
                maybeAddImport("com.acme.bank.Record", false);
                return cu;
            }
        });
    }
}

[junior]

That's the best TODO for this feature. How do we vote? ;)

TODO find any use of `*..Record` and replace wildcard import with explicit import

[timtebeek]

Had a quick look at what's necessary here, and updated the tests above already. We likely have to make a change here to fix one of the tests, as we right now fail to add an import when the import and class are in the same package, whereas we likely need to disambiguate the import even if in the same package: https://github.com/openrewrite/rewrite/blob/a98d83d698dda9971c93a0767b7f85d5866f306d/rewrite-java/src/main/java/org/openrewrite/java/AddImport.java#L108-L112

[BramliAK]

hello @timtebeek what do you think about replacing

import com.something.*;
class A {
    void foo(Record r) {
    }
}

by this

import com.something.*;
class A {
    void foo(com.something.Record r) {
    }
}

[timtebeek]

Thanks for offering an alternative @BramliAK ; That could work as well, but I worry a bit about the readability of the code when an import would be closer to the original. Typically I like to only use fully qualified names throughout the class body when two classes of the same name are in use.