Skip to content

Expand star imports in ChangePackage and related recipes when they would create ambiguity#7202

Merged
steve-aom-elliott merged 7 commits intomainfrom
fix/changepackage-ambiguous-star-imports
Apr 10, 2026
Merged

Expand star imports in ChangePackage and related recipes when they would create ambiguity#7202
steve-aom-elliott merged 7 commits intomainfrom
fix/changepackage-ambiguous-star-imports

Conversation

@steve-aom-elliott
Copy link
Copy Markdown
Contributor

@steve-aom-elliott steve-aom-elliott commented Mar 30, 2026

Summary

  • When ChangePackage renames a star import (e.g., import javax.validation.constraints.*import jakarta.validation.constraints.*), it now detects if the new package contains types whose simple names clash with types from other star imports (e.g., org.hibernate.validator.constraints.* also has NotBlank)
  • If ambiguity is detected, only the changed star import is expanded into explicit imports for the types actually referenced in the source, leaving other star imports untouched
  • Uses JavaSourceSet classpath to enumerate types in each package, then walks AST identifiers to determine which types are actually referenced
  • ChangeType now also detects when its target type is covered by a star import that would become ambiguous with another star import, and inserts an explicit import to disambiguate
  • ChangeDependencyGroupIdAndArtifactId (Maven) and AddDependency (Maven and Gradle) now update the JavaSourceSet marker when changing or adding dependencies, so downstream recipes like ChangePackage and ChangeType have accurate classpath data for ambiguity detection
  • JavaSourceSetUpdater.addDependency now tries the project's actual Maven repositories instead of hardcoding Maven Central, fixing failures in air-gapped environments or projects using private repositories
  • Shared withSourceTypesOnClasspath test helper extracted to Assertions.java for reuse across test classes

Test plan

  • changePackageExpandsStarImportWhenItWouldCreateAmbiguity — uses real javax.validation, jakarta.validation, and org.hibernate.validator types; verifies star import is expanded to explicit imports when another star import contains a type with the same simple name
  • changePackagePreservesStarImportWhenNoAmbiguity — verifies star import is preserved when no ambiguity exists
  • changeTypeAddsExplicitImportWhenStarImportsWouldBeAmbiguous — verifies ChangeType inserts an explicit import when two star imports would both provide the target type's simple name
  • composedWithChangePackageUpdatesImports — end-to-end test composing ChangeDependencyGroupIdAndArtifactId + ChangePackage to verify import renaming works when the JavaSourceSet is updated by the dependency change
  • Existing ChangePackageTest, ChangeTypeTest, and ChangeDependencyGroupIdAndArtifactIdTest suites pass

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Mar 30, 2026
@steve-aom-elliott steve-aom-elliott force-pushed the fix/changepackage-ambiguous-star-imports branch from a6ee247 to c7942f8 Compare March 30, 2026 18:22
@steve-aom-elliott steve-aom-elliott force-pushed the fix/changepackage-ambiguous-star-imports branch 4 times, most recently from e50f48f to 48f89f5 Compare April 7, 2026 19:57
@steve-aom-elliott steve-aom-elliott changed the title Expand star imports in ChangePackage when they would create ambiguity Expand star imports in ChangePackage and related recipes when they would create ambiguity Apr 7, 2026
@steve-aom-elliott steve-aom-elliott marked this pull request as ready for review April 7, 2026 20:55
@steve-aom-elliott steve-aom-elliott added bug Something isn't working enhancement New feature or request test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail java maven gradle labels Apr 7, 2026
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Apr 7, 2026
Comment thread rewrite-gradle/src/main/java/org/openrewrite/gradle/AddDependency.java Outdated
Comment thread rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependency.java Outdated
Comment thread rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependency.java Outdated
@steve-aom-elliott steve-aom-elliott force-pushed the fix/changepackage-ambiguous-star-imports branch from 7566269 to 7bc9f18 Compare April 9, 2026 13:22
Copy link
Copy Markdown
Member

@sambsnyd sambsnyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JavaSourceSet updating functionality within AddDependency , ChangeDependnecy, etc., are all very similar. Perhaps that functionality could be moved into JavaSourceSet itself to simplify and keep standard the recipes which update it

When ChangeType moves a type into a package covered by a star import,
and another star import provides a type with the same simple name,
add an explicit import to disambiguate. This mirrors the ambiguity
handling already present in ChangePackage.
…ntegration test

Move the withSourceTypesOnClasspath test helper from private methods in
ChangePackageTest and ChangeTypeTest to a public static method in
Assertions.java so it can be reused across test classes.

Add an end-to-end integration test in ChangeDependencyGroupIdAndArtifactIdTest
that composes ChangeDependencyGroupIdAndArtifactId with ChangePackage to verify
that import renaming works correctly when the JavaSourceSet is updated by the
dependency change recipe.
…rce set updates

- Attach Markup.warn to build files when version resolution for JavaSourceSet
  updates fails, so users know the classpath won't be updated
- Remove counter-productive G.CompilationUnit/K.CompilationUnit exclusion in
  Gradle AddDependency and ChangeDependency — Groovy/Kotlin source files can
  have JavaSourceSet markers; Gradle build files won't have one and are already
  filtered by the maybeSourceSet.isPresent() check
- Cache computed JavaSourceSet per source set in ChangeDependency (Gradle) and
  ChangeDependencyGroupIdAndArtifactId (Maven) to avoid redundant per-file
  recomputation when all files in a source set share identical markers
… ChangeDependency

Capture Maven repositories from GradleProject in the scanner and use
them when downloading JARs for JavaSourceSet updates, falling back to
Maven Central only if no remote repository is available. This matches
the approach already used in Maven ChangeDependencyGroupIdAndArtifactId
and both AddDependency recipes.
@steve-aom-elliott steve-aom-elliott force-pushed the fix/changepackage-ambiguous-star-imports branch from a688b74 to a5abf62 Compare April 10, 2026 19:08
@steve-aom-elliott
Copy link
Copy Markdown
Contributor Author

@sambsnyd Ended up squashing most of the previous commits in some form and rebasing the branch again. The changes that I made following your last comment are all part of: a5abf62

@steve-aom-elliott steve-aom-elliott merged commit 24938cf into main Apr 10, 2026
1 check passed
@steve-aom-elliott steve-aom-elliott deleted the fix/changepackage-ambiguous-star-imports branch April 10, 2026 20:08
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Apr 10, 2026
timtebeek added a commit to openrewrite/rewrite-hibernate that referenced this pull request Apr 11, 2026
openrewrite/rewrite#7202 added JavaSourceSet marker updates to
ChangeDependencyGroupIdAndArtifactId, which now modifies Java files'
markers without changing text content. This causes the test framework
to flag "empty diff" errors for Java source specs without explicit
`after` text.

Provide explicit `after` text (identical to `before`) for the Java
source file in the affected test to accept the marker-only change.

See openrewrite/rewrite#7349 for the upstream issue.
timtebeek added a commit to openrewrite/rewrite-migrate-java that referenced this pull request Apr 11, 2026
openrewrite/rewrite#7202 added JavaSourceSet marker updates to
ChangeDependencyGroupIdAndArtifactId, which now modifies Java files'
markers without changing text content. This causes the test framework
to flag "empty diff" errors for Java source specs without explicit
`after` text.

Provide explicit `after` text (identical to `before`) for the Java
source file in the affected test to accept the marker-only change.

See openrewrite/rewrite#7349 for the upstream issue.
timtebeek added a commit that referenced this pull request Apr 11, 2026
…ty diff

After #7202, `ChangeDependencyGroupIdAndArtifactId` updates `JavaSourceSet`
markers on Java files when dependency coordinates change. These marker-only
changes produce Results with identical before/after text, which
`RewriteTest` previously treated as errors ("An empty diff was generated").

Instead of failing, silently accept Results where the printed text is
unchanged. This allows recipes to update markers (e.g. classpath metadata)
without requiring downstream tests to add explicit `after` text for every
Java source spec.

The `afterRecipe` callback still fires with the updated tree, so tests
that need to verify marker changes (like `updatesJavaSourceSetMarkerOnJavaFiles`)
continue to work.

Fixes #7349
steve-aom-elliott added a commit that referenced this pull request Apr 13, 2026
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 a commit that referenced this pull request Apr 13, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request gradle java maven test provided Already replicated with a unit test, using JUnit pioneer's ExpectedToFail

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants