Make subpackage recursion explicit where these renames depend on it - #141
Merged
Conversation
Both of these `ChangePackage` rules omit `recursive` while depending on it to
move types out of subpackages of `oldPackageName`.
`org.apache.commons.math` -> `org.apache.commons.math3` has to carry
`org.apache.commons.math.stat.StatUtils` along with it.
The httpclient rule is step 2 of a three-step chain:
org.apache.http.entity -> org.apache.hc.core5.http.io.entity (recursive: true)
org.apache.hc.core5.http.io.entity.mime -> org.apache.hc.client5.http.entity.mime
org.apache.hc.client5.http.entity.mime.content -> org.apache.hc.client5.http.entity.mime
Step 1 relocates `org.apache.http.entity.mime.content.StringBody` under
`core5`, step 2 has to move the whole `mime` subtree over to `client5`, and
step 3 flattens `.content`. Step 2 only reaches `.content.StringBody` by
recursing, so without it the chain stalls at
`org.apache.hc.core5.http.io.entity.mime.content.StringBody`, a package that
does not exist in httpclient5.
Relying on the default here was always fragile: `ChangePackage.recursive` is
`@Nullable` with no documented default, and a null was read as non-recursive by
the recipe's preconditions but as recursive by its visitor, so subpackage
renames only happened for files that also referenced a type sitting directly in
`oldPackageName`. openrewrite/rewrite#8382 settles null as non-recursive, which
makes that latent dependency visible. Stating `recursive: true` is correct
either way and is green both with and without that change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two
ChangePackagerules omitrecursivewhile depending on it to move types out of subpackages ofoldPackageName.apache-commons-math-2-3.yml—org.apache.commons.math→org.apache.commons.math3has to carryorg.apache.commons.math.stat.StatUtilsalong with it.apache-httpclient-5.yml— this one is step 2 of a deliberate three-step chain:Step 1 relocates
org.apache.http.entity.mime.content.StringBodyundercore5, step 2 has to move the wholemimesubtree over toclient5, and step 3 flattens.content. Step 2 only reaches.content.StringBodyby recursing, so without it the chain stalls atorg.apache.hc.core5.http.io.entity.mime.content.StringBody— a package that doesn't exist in httpclient5.Why now
ChangePackage.recursiveis@Nullablewithrequired = falseand no documented default, and a null was read two different ways inside the same recipe: non-recursive by its preconditions, recursive by its visitor. The upshot was that a subpackage type got renamed only if the file also referenced a type sitting directly inoldPackageName— so these two rules worked in the test fixtures (which happen to mix both) and silently did nothing for a file that only touched a subpackage.recursiveconsistently mean non-recursive inChangePackagerewrite#8382 settles null as non-recursive, which turns that latent dependency into a visible one. I found these two while validating that change downstream.Verification
Full
./gradlew teston this branch, four ways:origin/main