ChangeType causing unexpected import changes for inner types.

[ryan-hudson]

What version of OpenRewrite are you using?

8.41.2

Test Case

In this example, the change type recipe incorrectly removes the existing web client import and unnecessarily adds a rest client builder import.

@Test
void addsUnnecessaryBuilderImport() {
  rewriteRun(recipeSpec -> recipeSpec.recipe(
      new ChangeType("org.springframework.web.reactive.function.client.WebClient$Builder",
              "org.springframework.web.client.RestClient$Builder", null))
                  .parser(JavaParser.fromJavaVersion()
                      .classpath("spring-webflux")
                      .logCompilationWarningsAndErrors(true)),
      java("""
        import org.springframework.web.reactive.function.client.WebClient;
  
        class Foo {
            public WebClient foo() {
                return WebClient.builder().build();
            }
        }
      """));
}

Expected Result

No file change. (Possibly a type change to the tree)

Actual Result

import org.springframework.web.client.RestClient.Builder;

class Foo {
    public WebClient foo() {
        return WebClient.builder().build();
    }
}

Additional Info:

This issue appears to be a recently introduced bug, as rewrite version 8.37.0 does not have this issue.

[timtebeek]

Thanks for the report @ryan-hudson ! Might this have been caused by

• Fix AddImport match variable's name #4747 ?

That's only in 8.41.2; do you see the same issue with 8.41.1?

[ryan-hudson]

I have, its unfortunately the same behavior with both 8.41.1 and 8.41.0.

[timtebeek]

Thanks for the quick check; then it must be going back further; weird that we haven't encountered this elsewhere. Would you mind sharing where that ChangeType recipe you're using is coming from? Or are the classes used in that test just to illustrate the issue? Seems like it would fit rewrite-spring.

[nmck257]

(I work with Ryan) This was originally encountered in proprietary recipes targeting proprietary types; the types shown in this issue were picked to demonstrate the behavior as they have comparable "shapes" (ie types where you can statically call .builder() and receive an instance of an inner Builder class and then .build() that to an object implementing the original type/interface)

[nmck257]

PR #4648 looks like the most substantial set of changes in this area which line up with the impacted versions; cc @Laurens-W if you happen to have insights :)

[Laurens-W]

Hmm nothing comes to mind immediately but I will keep this on my radar to look into after rounding up what I'm working on rn!

[nmck257]

FYI, narrowed it further. Here's a raw-java test case to demonstrate the odd behaviors:

    @Test
    void changeTypeOfInnerClass() {
        rewriteRun(
          spec -> spec.recipe(new ChangeType("foo.A$Builder", "bar.A$Builder", true))
            .parser(JavaParser.fromJavaVersion().dependsOn(
              """
              package foo;
              
              public class A {
                public A.Builder builder() {
                  return new A.Builder();
                }
              
                public static class Builder {
                  public A build() {
                    return new A();
                  }
                }
              }
              """,
              """
              package bar;
              
              public class A {
                public A.Builder builder() {
                  return new A.Builder();
                }
              
                public static class Builder {
                  public A build() {
                    return new A();
                  }
                }
              }
              """
            )),
          java("""
          import foo.A;
          import foo.A.Builder;
          
          class Test {
            A test() {
                A.Builder b = A.builder();
                return b.build();
            }
          }
          """, """
          import foo.A;
          import bar.A.Builder;
          
          class Test {
            A test() {
                A.Builder b = A.builder();
                return b.build();
            }
          }
          """)
// this scenario is commented-out because its expected behavior is *also* a failure, for the recipe making a type-metadata-only change
//          ,
//          java("""
//          import foo.A;
//
//          class Test2 {
//            A test() {
//                return A.builder().build();
//            }
//          }
//          """
//          )
        );
    }

The issue begins with 8.41.0, and this PR looks like the most-relevant one now: #4714

The issue also seems to be specific to using the $ syntax to identify inner types; swapping to . seems to resolve it. So optimistically, this is just an unexpected change in behavior for which syntax option is supported.

[timtebeek]

This looks related to

• Correct J.FieldAccess#isFullyQualifiedClassReference() #4774

[knutwannheden]

Interesting test case. To me it isn't quite clear what is to be expected when changing foo.A$Builder to bar.A$Builder in the example:

              import foo.A;
              import foo.A.Builder;
              
              class Test {
                A test() {
                    A.Builder b = A.builder();
                    return b.build();
                }
              }

With my changes in #4774 the result is:

              import foo.A;
              
              class Test {
                A test() {
                    bar.A.Builder b = A.builder();
                    return b.build();
                }
              }

To me that seems logical. An alternative would have been to add an import for bar.A.Builder but then also replace A.Builder by Builder.

[nmck257]

Yeah, the test case is a bit contrived, but the results after your changes seem reasonable.
For context, in our original composite recipe, the ChangeType invocation described here would accompany a ChangeType invocation for foo.A directly, which I think is a more intuitive use case. But the test case aimed to be minimal.

Thanks for the resolution!