Don't modify package declaration of siblings in ChangeType

[kennytv]

What's changed?

ChangeClassDefinition in ChangeType previously changed package declaration of sibling types, this PR restricts these to only the classes that should actually be changed

What's your motivation?

Closes #4182

Anything in particular you'd like reviewers to focus on?

I'm not exactly sure as to whether this is the proper way to do it (tracking required modification in JavaSourceFile visits), but at the very least this should show what the root cause is. And I suppose this breaks the immutability rule should be addressed now

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]

Thanks a lot for getting this started. Helpful to know we might now inadvertently make changes, and a good start at getting that resolved. I've added a note about the use of cursor messaging over mutable fields; I hope that's clear enough. If not do let us know!

[timtebeek]

Looking good ; quick suggestion to rename the message key and variable, as path can mean the source file path too, which might be confusing.

[timtebeek]

Approved already; let me know how you feel about those suggestions and then we can get this merged.

[kennytv]

Should be all good 👍

[timtebeek]

Thanks a lot for the quick turn around on this; really helpful to get this sorted. I'll merge as soon as the tests pass and from there it'll become available in our snapshot versions until the next release, typically in a week or two.

[timtebeek]

Hmm; seems two other tests have started failing; This might need some work still. 🤔

rewrite/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java

Lines 1310 to 1332 in 2988966

	@Issue("https://github.com/openrewrite/rewrite/issues/1904")
	@Test
	void onlyChangeTypeWithoutMatchedFilePath() {
	rewriteRun(
	spec -> spec.recipe(new ChangeType("a.b.Original", "x.y.Target", false)),
	java(
	"""
	package a.b;
	public class Original {
	}
	""",
	"""
	package x.y;
	public class Target {
	}
	""",
	spec -> spec.path("a/b/NoMatch.java").afterRecipe(cu -> {
	assertThat(PathUtils.separatorsToUnix(cu.getSourcePath().toString())).isEqualTo("a/b/NoMatch.java");
	assertThat(TypeUtils.isOfClassType(cu.getClasses().get(0).getType(), "x.y.Target")).isTrue();
	})
	)
	);
	}

rewrite/rewrite-java-test/src/test/java/org/openrewrite/java/ChangeTypeTest.java

Lines 1426 to 1449 in 2988966

	@Issue("https://github.com/openrewrite/rewrite/issues/2528")
	@Test
	void changePathOfNonPublicClass() {
	rewriteRun(
	spec -> spec.recipe(new ChangeType("a.b.C", "x.y.Z", false)),
	java(
	"""
	package a.b;
	class C {
	}
	class D {
	}
	""",
	"""
	package x.y;
	class Z {
	}
	class D {
	}
	""",
	spec -> spec.path("x/y/Z.java")
	)
	);
	}

[kennytv]

Hmm that's strange, they run and pass locally, both individually through IntelliJ and the full module via gradlew :rewrite-java-test:test. I'm running Windows, if that matters

[timtebeek]

I did see them fail for me locally; I've triggered the tests to run again to see if that shows any difference.

[kennytv]

After switching machines I got the same lol, a fresh clone might have done it as well. In any case, I'm not sure I understand the spec -> spec.path("x/y/Z.java") lines. Docs say this marks the path after running the recipe, but I'm already getting x/y/Z.java as the source path in the initial ChangeClassDefinition visit - is that intended behavior? Changing the spec.path call to the origin path makes the test pass again

[timtebeek]

Oh wow, I'd never noticed the Java docs on those methods; they seem to have been added deliberately, although I'd always taken those to mean the source paths before the recipe is run.

rewrite/rewrite-test/src/main/java/org/openrewrite/test/SourceSpec.java

Lines 108 to 124 in 2122964

	/**
	* @param sourcePath The source path after the recipe is run.
	* @return This source spec.
	*/
	public SourceSpec<T> path(Path sourcePath) {
	this.sourcePath = sourcePath;
	return this;
	}
	/**
	* @param sourcePath The source path after the recipe is run.
	* @return This source spec.
	*/
	public SourceSpec<T> path(String sourcePath) {
	this.sourcePath = Paths.get(sourcePath);
	return this;
	}

We have other mechanisms to verify the source path is changed after a recipe run, which hints at the JavaDoc above being wrong.

rewrite/rewrite-test/src/test/java/org/openrewrite/RenameFileTest.java

Lines 41 to 43 in 2122964

	spec
	.path("a/b/hello.txt")
	.afterRecipe(pt -> assertThat(pt.getSourcePath()).isEqualTo(Paths.get("a/b/goodbye.txt")));

So with that, the spec -> spec.path("a/b/NoMatch.java") and spec -> spec.path("x/y/Z.java") should set the paths on the input source files, not their moved equivalents.

The onlyChangeTypeWithoutMatchedFilePath test then verifies that if the filename does not match any of the contained classes, that we do not change the filename. I think that's valid and probably best to retain to avoid surprises. What do you think? The linked issue is this one

• ChangeType should also change the source path and constructors when modifying the class definition #1904

The changePathOfNonPublicClass test verifies that we do actually change the path, even when the class is not public, as defined in this linked issue:

• ChangeType: rename java file for non-public class too if the java file name matches the class name #2528

With that I think it would be best if we don't change the behavior of any of the existing cases, and maybe even fix the confusing JavaDocs. What are your thoughts on the same?

[kennytv]

That makes sense, thanks for the help! I pushed a commit here with how I'd go about fixing them, let me know if that should be a separate PR. The fix for the jd and the changePathOfNonPublicClass test are pretty straight forward, just for onlyChangeTypeWithoutMatchedFilePath I split the test for the non-matching class into its own call, so that the source path of the a.b.Original case stays correct

Though for the jd, I don't know the other uses, so let me know if "before" isn't correct either

[timtebeek]

Thanks for the quick work; I had a slightly different idea come to mind after I'd already replied above; I should have updated my comment but was in the middle of other work.

Before any of your changes, we handled the cases where there's multiple classes in a single compilation unit (file) and changed their package and source path if the recipe indicates that's necessary. The NoMatch.java test highlighted a separate case where neither non-public class matched the filename, and where the filename should thus not change for that test (at least up to now).

The issue you've reported, and are tackling here is a different variation: there's again two classes in the same package, but in different compilation units (files). That's where the recipe up to now appears to have been making incorrect changes, based on the new test you added where the sibling in a different file was changed as well.

I think ideally we don't modify the tests we had before, and keep their behavior the same (unless we feel that's incorrect), and instead work out how to get the new test to pass by handling the above cases separately: sibling classes in the same compilation unit versus in different compilation units.

And note that I'm certainly not trying to complicate the work you're doing! It's much appreciated; we're both learning what the recipe did before, and how that relates to what we'd like it to do going forward, and hope we agree on the approach.

[kennytv]

Ah I see now. Technically the provided and expected results aren't invalid Java code, but in practice you get misplaced class files that don't match their package declaration before compilation, which doesn't seem to be part of the linked issues (the provided class in changePathOfNonPublicClass, and the result in onlyChangeTypeWithoutMatchedFilePath are misplaced). So at least the path spec in changePathOfNonPublicClass looks wrong to me

An alternative to checking the source path for the sibling fix (as pushed now) would be to go through the classes in JavaSourceFile to check their fully qualified names, ignoring the actual source path. That should work and makes all tests pass, but I'm not sure of other implications

[timtebeek]

Thanks a lot for working through all these iterations. I think this now works as we'd like it to.