Prevent marker-only changes from producing phantom diffs#7358
Merged
steve-aom-elliott merged 1 commit intomainfrom Apr 13, 2026
Merged
Prevent marker-only changes from producing phantom diffs#7358steve-aom-elliott merged 1 commit intomainfrom
steve-aom-elliott merged 1 commit intomainfrom
Conversation
Dependency-modifying recipes now update JavaSourceSet markers (added in #7202), which can produce Results with identical before/after text. This caused downstream test failures in rewrite-dropwizard, rewrite-hibernate, rewrite-migrate-java, and rewrite-spring. Three fixes: 1. JavaSourceSet.addTypesForGav returns `this` when the gavKey already exists with the same types, preventing unnecessary allocations for LSTs with empty gavToTypes maps. 2. The cached overload of JavaSourceSet.updateOnSourceFile now checks whether the source file's current marker is already the cached instance before replacing it, preventing O(n) phantom diffs across files in the same source set. 3. RewriteTest accepts marker-only changes (identical printed text) instead of failing with "An empty diff was generated", since these represent legitimate internal state updates. Closes #7349
steve-aom-elliott
added
bug
timtebeek
reviewed
Apr 13, 2026
| */ | ||
| public JavaSourceSet addTypesForGav(String gavKey, List<JavaType.FullyQualified> types) { | ||
| List<JavaType.FullyQualified> existing = gavToTypes.get(gavKey); | ||
| if (existing != null && existing.equals(types)) { |
Member
There was a problem hiding this comment.
I suppose there's no way to prevent the deep equality comparison here? Are there any shortcuts like comparing the sizes? Or is that already built into equals?
Member
There was a problem hiding this comment.
Looks like the size check is one of the first things it does, thankfully. Then this is less of a concern.
timtebeek
approved these changes
Apr 13, 2026
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
2 participants
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.
Summary
Dependency-modifying recipes now update
JavaSourceSetmarkers (Expand star imports in ChangePackage and related recipes when they would create ambiguity #7202), which can produceResultobjects with identical before/after printed text. This caused "An empty diff was generated" test failures in downstream repos:moderneinc/rewrite-dropwizard —
addsSpringStarterJdbcWhenDropwizardDbIsUsedopenrewrite/rewrite-hibernate —
groupIdHibernateOrmRenamedopenrewrite/rewrite-migrate-java —
changeAndUpgradeDependencyIfAnnotationJsr250Presentopenrewrite/rewrite-spring —
flywayStarterOmitsVersionWhenManagedByParentThree fixes, each addressing a different layer:
JavaSourceSet.addTypesForGavidempotency — returnsthiswhen the gavKey already exists with the same types, preventing unnecessary object allocation (especially for LSTs with emptygavToTypesmaps where the existing guard inchangeDependencydoesn't fire).JavaSourceSet.updateOnSourceFile(cached overload) — checks whether the source file's current marker is already the cached instance before callingwithMarkers, preventing O(n) new tree references across all files in a source set when only the first file truly changed.RewriteTestsafety net — accepts marker-only changes (identical before/after text) instead of failing, since these represent legitimate internal state updates. CallsafterRecipeso marker assertions still work.All four downstream test failures were verified to pass with these changes.