Rename variables if the name matches class name

[radoslaw-panuszewski]

What's your motivation?

When I rename a type in IntelliJ IDEA (for example SomeType to AnotherType) it will automatically rename variables with the same name as the class, but starting with lowercase character (so someType to anotherType).

This PR introduces the above feature to org.openrewrite.java.ChangeType recipe.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Neat addition, thanks! Couple small requests, but otherwise looks good.

[timtebeek]

Neat idea indeed to rename these as well!

[radoslaw-panuszewski]

Hey @timtebeek, could you ping rest of the team to take a look? 😉

[timtebeek]

Got back over the weekend, thanks! Having a look now.

Did a quick extra check that we only touch variables where the type changed:

    @Test
    void changeMethodVariableName() {
        rewriteRun(
          spec -> spec.recipe(new ChangeType("a.A1", "a.A2", true)),
          java(
            """
              package a;
              public class A1 {
              }
              """
          ),
          java(
            """
              package a;
              public class A2 {
              }
              """
          ),
          java(
            """
              package org.foo;
              
              import a.A1;
              import a.A2;
              
              public class Example {
                  public A1 method1(A1 a1) {
                      return a1;
                  }
                  public A2 method2(A2 a1) {
                      return a1; // Unchanged
                  }
              }
              """,
            """
              package org.foo;
              
              import a.A2;
              
              public class Example {
                  public A2 method1(A2 a2) {
                      return a2;
                  }
                  public A2 method2(A2 a1) {
                      return a1; // Unchanged
                  }
              }
              """
          )
        );
    }

[radoslaw-panuszewski]

Hi @timtebeek, did you take a look? I would love to proceed with this PR 😉

[timtebeek]

Thanks both; took a little longer to figure out what seemed "off" at the earlier implementation. Figured it out just now and applied changes to make better use of RenameVariable and keep all changes inside visitVariable. Previously only the printed name of the variable was changed, but the underlying type informations was not updated from the same point. That might then lead to problems down the line. With the changes made just now I'm more confident to merge this, as it's such a frequently used recipe. Thanks again for the suggestion, tests and initial implementation!

[radoslaw-panuszewski]

Hi @timtebeek, thanks for the changes! The only thing I don't understand is the commit Do not expect Kotlin changes just yet. Does it mean this recipe won't rename variables in Kotlin? That would be bad news for my company, which have most of the code written in Kotlin 😞

Or maybe the "just yet" means that more commits will follow? 😛

[timtebeek]

I've not yet worked out why we're not seeing change for Kotlin, and my IDE was throwing up some unrelated issues in trying to figure it out. It might be that RenameVariable visitor, or something else still. I'm definitely open to fixing that, but perhaps best done on a separate PR such that we have something to work from already. Appreciate any help debugging that addition!

[timtebeek]

Looks to fail on

[timtebeek]

Fixed in 3b1f9d1; thanks !

[radoslaw-panuszewski]

Thanks for your help @timtebeek, looking forward to the release! 😄

[timtebeek]

Unfortunately we saw some undesired incomplete changes downstream, so we've decided to revert this for now:

• 982ad13

It's at times hard to make guaranteed correct changes, so as much as we like the suggestion, in this case it's too risky.

[radoslaw-panuszewski]

Unfortunately we saw some undesired incomplete changes downstream, so we've decided to revert this for now:

• 982ad13

It's at times hard to make guaranteed correct changes, so as much as we like the suggestion, in this case it's too risky.

@timtebeek could you give some examples of the "undesired incomplete changes downstream"? If this change is too risky, I would love to understand why 🙏

[radoslaw-panuszewski]

@timtebeek maybe it would be possible to hide this feature under a boolean flag, something like renameVariables = true?

[timtebeek]

hi! So cases of undesired renames is anything that's public, and might then be referenced elsewhere, or implied public through the use of say Lombok annotations or (de)serialization. In short it'd only be safe if we know the exact scope, and even then might result in conflicts that need to be handled correctly. While we could potentially iterate on this, ChangeType is very much a corner stone recipe for a lot of migrations, and changes here are likely to ripple out to cause friction downstream. For that same reason we do not like to add boolean flags to recipes: those tend to accumulate over time, and result in a need to adopt ever more optional arguments in constructors. While we understand it's perhaps a desirable change on a small scale, the potential effects on a large scale mean we can not safely add this at this time. Appreciate the suggestion though!