ChangeType not working with types provided to the parser using dependsOn.

[ammachado]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v8.17.1
• Maven/Gradle plugin v5.23.1
• rewrite-java v8.17.1

How are you running OpenRewrite?

I am using the following unit test:

    @Test
    void renamePackageWithDependsOn() {
        @Language("java")
        final String schemaClass1 = """
          package my.example.xprivate.x1.MySchema;
          
          public class MySchemaType {}
          """;

        @Language("java")
        final String schemaClass2 = """
          package my.example._private._1.MySchema;
          
          public class MySchemaType {}
          """;

        rewriteRun(
          spec -> spec
            .parser(JavaParser.fromJavaVersion().dependsOn(schemaClass1, schemaClass2))
            .recipe(new ChangeType(
              "my.example.xprivate.x1.MySchema.MySchemaType",
              "my.example._private._1.MySchema.MySchemaType",
              true
            )),
          //language=java
          java("""
            package my.example.app;
            
            import my.example.xprivate.x1.MySchema.MySchemaType;
            
            class MyApp {
                void doWork(MySchemaType mySchema) {}
            }
            """, """
            package my.example.app;
            
            import my.example._private._1.MySchema.MySchemaType;
            
            class MyApp {
                void doWork(MySchemaType mySchema) {}
            }
            """)
        );
    }

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

Using the provided test.

What did you expect to see?

package my.example.app;
            
import my.example._private._1.MySchema.MySchemaType;
            
class MyApp {
    void doWork(MySchemaType mySchema) {}
}

What did you see instead?

diff --git a/my/example/app/MyApp.java b/my/example/app/MyApp.java
index 5a97edd..9ff90dd 100644
--- a/my/example/app/MyApp.java
+++ b/my/example/app/MyApp.java
@@ -1,7 +1,5 @@ 
 package my.example.app;
 
-import my.example._private._1.MySchema.MySchemaType;
-
 class MyApp {
     void doWork(MySchemaType mySchema) {}
 }
\ No newline at end of file

What is the full stack trace of any errors you encountered?

N/A

Are you interested in contributing a fix to OpenRewrite?

Yes.

[timtebeek]

That's a strange case indeed; thanks for the detailed runnable example. I wouldn't quite know why that's missed here. I'm guessing you ran into this when doing something similar in an actual recipe beyond this test case?

Thanks for the offer to look into this as well!

[ammachado]

Findings:

1. Compilation errors being swallowed for classes provided via dependsOn, if a class is not declared on the default package:

1255073806633000:3: error: class MySchemaType is public, should be declared in a file named MySchemaType.java
public class MySchemaType {}
       ^

There's a fix for this issue: #4040

2. Identified a possible small optimization on ChangeTypeVisitor - There's a fix for this issue: Save identified type on cache for future lookups on ChangeType. #4039

3. Package names used the failing test does not follow the the conventions, although their values are valid. For example, ChangeType uses JavaType.ShallowClass.build(oldFullyQualifiedTypeName) to create type references. The build method identifies names based on conventions; it considers that if an uppercase letter comes after a dot, the next identifier is a class name.

[ammachado]

Updated the last comment with the fixes for the first 2 findings.

[timtebeek]

Thanks for the fixes so far! I've given the test that you provided above another go and I can replicate it using only a capital letter in the package name already 🤔

@Test
void renamePackageWithDependsOn() {
    @Language("java") final String schemaClass1 = """
      package foo.Capital;
      public class MySchemaType {}
      """;
    @Language("java") final String schemaClass2 = """
      package bar.Capital;
      public class MySchemaType {}
      """;

    rewriteRun(
      spec -> spec
        .parser(JavaParser.fromJavaVersion().dependsOn(schemaClass1, schemaClass2))
        .recipe(new ChangeType(
          "foo.Capital.MySchemaType",
          "bar.Capital.MySchemaType",
          true
        )),
      //language=java
      java(
        """
          package my.example.app;
          
          import foo.Capital.MySchemaType;
          
          class MyApp {
              void doWork(MySchemaType mySchema) {}
          }
          """,
        """
          package my.example.app;
          
          import bar.Capital.MySchemaType;
          
          class MyApp {
              void doWork(MySchemaType mySchema) {}
          }
          """
      )
    );
}

Which again fails with

diff --git a/my/example/app/MyApp.java b/my/example/app/MyApp.java
index 7fe83d7..9ff90dd 100644
--- a/my/example/app/MyApp.java
+++ b/my/example/app/MyApp.java
@@ -1,7 +1,5 @@ 
 package my.example.app;
 
-import bar.Capital.MySchemaType;
-
 class MyApp {
     void doWork(MySchemaType mySchema) {}
 }

[timtebeek]

Look like we misidentify an owning class where there is none, just because the package contains an uppercase letter. :/

[ammachado]

Here's a simpler test:

@Test
void changeTypeWithCapitalLettersInPackageName() {
    @Language("java") final String schemaClass = """
      package bar.Capital;
      public class MySchemaType {}
      """;

    rewriteRun(
      java(schemaClass, spec -> spec.afterRecipe(cu -> {
            JavaType.FullyQualified typeFromParser = cu.getClasses().get(0).getType();

            // Called on ChangeClassDefinition constructor
            JavaType.ShallowClass shallowClass = JavaType.ShallowClass
              .build(typeFromParser.getFullyQualifiedName());

            assertThat(shallowClass)
              .hasFieldOrPropertyWithValue("fullyQualifiedName", typeFromParser.getFullyQualifiedName())
              .hasFieldOrPropertyWithValue("owningClass", typeFromParser.getOwningClass());
        }
      ))
    );
}

And a variant for classes starting with lowercase letters, also failing:

@Test
void changeTypeWithClassNamesStartingWithLowercaseLetters() {
    @Language("java") final String schemaClass = """
      package bar.capital;
      public class mySchemaType {}
      """;

    rewriteRun(
      java(schemaClass, spec -> spec.afterRecipe(cu -> {
            JavaType.FullyQualified typeFromParser = cu.getClasses().get(0).getType();

            // Called on ChangeClassDefinition constructor
            JavaType.ShallowClass shallowClass = JavaType.ShallowClass
              .build(typeFromParser.getFullyQualifiedName());

            assertThat(shallowClass)
              .hasFieldOrPropertyWithValue("fullyQualifiedName", typeFromParser.getFullyQualifiedName())
              .hasFieldOrPropertyWithValue("owningClass", typeFromParser.getOwningClass());
        }
      ))
    );
}