Resolve dependency closures lazily when comparing exclusions

Closure resolution was eager: every visited node carrying an inherited
exclusion triggered a POM download, resolve and compile-scope graph
resolution. Since ancestor exclusions are propagated into each child's
requested dependency, the "no exclusions" early-out rarely fired, so a
spring-boot-starter tree performed ~38 extra resolutions of which one was
ever consulted.

TransitiveDependency now carries declared exclusions, and the closure
intersection is deferred into isRedundant, where it runs only for a
coordinate that actually matched and only when the two declared sets
differ. Both sides share a single closure for that coordinate.

Also collapse the (repositories, downloader, ctx, cache) tuple into a
ClosureResolver, dedupe the two recursive dependency walks, key the cache
on GroupArtifactVersion so it is not repository-sensitive, and correct
the description and comments that described the abandoned mechanism.

Author: timtebeek

src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java

@@ -29,6 +29,7 @@
 import org.openrewrite.maven.tree.*;
 
 import java.util.*;
+import java.util.function.Consumer;
 
 import static java.util.Collections.*;
 
@@ -52,14 +53,14 @@ public class RemoveRedundantDependencies extends ScanningRecipe<RemoveRedundantD
                 "This recipe downloads and resolves the parent dependency's POM to determine its true transitive " +
                 "dependencies, allowing it to detect redundancies even when both dependencies are explicitly declared. " +
                 "A direct dependency is only removed when the transitive one provides it at the exact same scope and " +
-                "with the same effective exclusions, so that removing it does not change the effective classpath.";
+                "with equivalent exclusions, so that removing it does not change the effective classpath.";
 
     @Value
     public static class Accumulator {
         // Map from project identifier -> scope/configuration -> Set of transitive dependencies
         Map<String, Map<String, Set<TransitiveDependency>>> transitivesByProjectAndScope;
-        // Cache of each coordinate's own clean dependency closure, shared across all source files in the run
-        Map<ResolvedGroupArtifactVersion, Set<GroupArtifact>> closureCache;
+        // Run-scoped: every closure miss costs a POM download and resolve
+        Map<GroupArtifactVersion, Set<GroupArtifact>> closureCache;
     }
 
     @Value
@@ -147,18 +148,17 @@ private void resolveTransitivesFromPom(
                     MavenPomDownloader downloader,
                     ExecutionContext ctx,
                     Set<TransitiveDependency> transitives) {
-                List<MavenRepository> effectiveRepos = withMavenCentral(repositories);
                 try {
                     // Get the resolved dependencies for compile scope (which includes most transitives)
-                    Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, effectiveRepos);
-                    ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, effectiveRepos, ctx);
+                    ResolvedPom resolvedPom = resolvePom(
+                            gav.asGroupArtifactVersion(), withMavenCentral(repositories), downloader, ctx);
                     ResolvedPom patchedPom = applyExclusions(resolvedPom, effectiveExclusions);
-                    List<ResolvedDependency> resolved = patchedPom.resolveDependencies(Scope.Compile, downloader, ctx);
 
                     // Collect all dependencies (both direct and transitive of the parent)
                     Set<ResolvedGroupArtifactVersion> visited = new HashSet<>();
-                    for (ResolvedDependency dep : resolved) {
-                        collectAllDependencies(dep, transitives, visited, effectiveRepos, downloader, ctx);
+                    for (ResolvedDependency dep : patchedPom.resolveDependencies(Scope.Compile, downloader, ctx)) {
+                        walkDependencies(dep, visited, d ->
+                                transitives.add(new TransitiveDependency(d.getGav(), declaredExclusions(d))));
                     }
                 } catch (MavenDownloadingException | MavenDownloadingExceptions e) {
                     // If we can't download/resolve the POM, fall back to not detecting redundancies
@@ -174,62 +174,32 @@ private ResolvedPom applyExclusions(ResolvedPom resolvedPom, List<GroupArtifact>
                         .anyMatch(e -> e.getGroupId().equals(d.getGroupId()) && e.getArtifactId().equals(d.getArtifactId())));
                 return patchedPom;
             }
-
-            private void collectAllDependencies(ResolvedDependency dep, Set<TransitiveDependency> transitives,
-                                                Set<ResolvedGroupArtifactVersion> visited, List<MavenRepository> repositories,
-                                                MavenPomDownloader downloader, ExecutionContext ctx) {
-                if (visited.add(dep.getGav())) {
-                    transitives.add(new TransitiveDependency(dep.getGav(),
-                            relevantExclusions(dep, repositories, downloader, ctx, acc.closureCache)));
-                    for (ResolvedDependency transitive : dep.getDependencies()) {
-                        collectAllDependencies(transitive, transitives, visited, repositories, downloader, ctx);
-                    }
-                }
-            }
         };
     }
 
-    // Compares a coordinate's declared (requested) exclusions rather than its effective ones: within a large
-    // tree the effective set is corrupted by dependency mediation (an artifact excluded here may already have
-    // been pruned by a shared node elsewhere, so it silently drops out of getEffectiveExclusions). The declared
-    // exclusions are then intersected with the coordinate's own clean closure, dropping no-op exclusions that
-    // target artifacts the coordinate could never bring on its own. Comparing this set on the direct and the
-    // transitively-provided declaration tells us whether removing the direct one changes the effective classpath.
-    private static Set<GroupArtifact> relevantExclusions(ResolvedDependency dep, List<MavenRepository> repositories,
-                                                         MavenPomDownloader downloader, ExecutionContext ctx,
-                                                         Map<ResolvedGroupArtifactVersion, Set<GroupArtifact>> closureCache) {
-        List<GroupArtifact> requested = dep.getRequested() == null ? null : dep.getRequested().getExclusions();
-        if (requested == null || requested.isEmpty()) {
-            return emptySet();
-        }
-        Set<GroupArtifact> exclusions = new HashSet<>(requested);
-        exclusions.retainAll(dependencyClosure(dep.getGav(), repositories, downloader, ctx, closureCache));
-        return exclusions;
+    // Declared (requested) exclusions rather than effective ones: within a large tree the effective set is
+    // corrupted by dependency mediation, as an artifact excluded here may already have been pruned by a shared
+    // node elsewhere, in which case the exclusion never fires and drops out of getEffectiveExclusions().
+    private static Set<GroupArtifact> declaredExclusions(ResolvedDependency dep) {
+        Dependency requested = dep.getRequested();
+        List<GroupArtifact> exclusions = requested == null ? null : requested.getExclusions();
+        return exclusions == null || exclusions.isEmpty() ? emptySet() : new HashSet<>(exclusions);
     }
 
-    private static Set<GroupArtifact> dependencyClosure(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
-                                                        MavenPomDownloader downloader, ExecutionContext ctx,
-                                                        Map<ResolvedGroupArtifactVersion, Set<GroupArtifact>> closureCache) {
-        return closureCache.computeIfAbsent(gav, g -> {
-            Set<GroupArtifact> closure = new HashSet<>();
-            try {
-                for (ResolvedDependency d : resolvePom(g, repositories, downloader, ctx)
-                        .resolveDependencies(Scope.Compile, downloader, ctx)) {
-                    collectClosure(d, closure);
-                }
-            } catch (MavenDownloadingException | MavenDownloadingExceptions e) {
-                // Best-effort: an unresolvable closure leaves the exclusions unfiltered
+    private static void walkDependencies(ResolvedDependency dep, Set<ResolvedGroupArtifactVersion> visited,
+                                         Consumer<ResolvedDependency> action) {
+        if (visited.add(dep.getGav())) {
+            action.accept(dep);
+            for (ResolvedDependency transitive : dep.getDependencies()) {
+                walkDependencies(transitive, visited, action);
             }
-            return closure;
-        });
+        }
     }
 
-    private static ResolvedPom resolvePom(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
+    private static ResolvedPom resolvePom(GroupArtifactVersion gav, List<MavenRepository> repos,
                                           MavenPomDownloader downloader, ExecutionContext ctx)
             throws MavenDownloadingException, MavenDownloadingExceptions {
-        List<MavenRepository> repos = withMavenCentral(repositories);
-        Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, repos);
-        return pom.resolve(emptyList(), downloader, repos, ctx);
+        return downloader.download(gav, null, null, repos).resolve(emptyList(), downloader, repos, ctx);
     }
 
     private static List<MavenRepository> withMavenCentral(List<MavenRepository> repositories) {
@@ -241,11 +211,36 @@ private static List<MavenRepository> withMavenCentral(List<MavenRepository> repo
         return effectiveRepos;
     }
 
-    private static void collectClosure(ResolvedDependency dep, Set<GroupArtifact> closure) {
-        if (closure.add(dep.getGav().asGroupArtifact())) {
-            for (ResolvedDependency transitive : dep.getDependencies()) {
-                collectClosure(transitive, closure);
-            }
+    // Resolves what a coordinate can bring in on its own, so that exclusions targeting artifacts it could never
+    // have brought can be discarded before comparing two declarations.
+    private static class ClosureResolver {
+        private final List<MavenRepository> repositories;
+        private final MavenPomDownloader downloader;
+        private final ExecutionContext ctx;
+        private final Map<GroupArtifactVersion, Set<GroupArtifact>> cache;
+
+        private ClosureResolver(List<MavenRepository> repositories, ExecutionContext ctx,
+                                Map<GroupArtifactVersion, Set<GroupArtifact>> cache) {
+            this.repositories = withMavenCentral(repositories);
+            this.downloader = new MavenPomDownloader(ctx);
+            this.ctx = ctx;
+            this.cache = cache;
+        }
+
+        private Set<GroupArtifact> closureOf(GroupArtifactVersion gav) {
+            return cache.computeIfAbsent(gav, g -> {
+                Set<GroupArtifact> closure = new HashSet<>();
+                Set<ResolvedGroupArtifactVersion> visited = new HashSet<>();
+                try {
+                    for (ResolvedDependency d : resolvePom(g, repositories, downloader, ctx)
+                            .resolveDependencies(Scope.Compile, downloader, ctx)) {
+                        walkDependencies(d, visited, r -> closure.add(r.getGav().asGroupArtifact()));
+                    }
+                } catch (MavenDownloadingException | MavenDownloadingExceptions e) {
+                    // Best-effort: an empty closure makes every exclusion look like a no-op
+                }
+                return closure;
+            });
         }
     }
 
@@ -277,7 +272,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
                 String projectId = gradle.getGroup() + ":" + gradle.getName();
                 Map<String, Set<TransitiveDependency>> scopeToTransitives =
                         acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap());
-                MavenPomDownloader downloader = new MavenPomDownloader(ctx);
+                ClosureResolver resolver = new ClosureResolver(gradle.getMavenRepositories(), ctx, acc.closureCache);
 
                 for (GradleDependencyConfiguration conf : gradle.getConfigurations()) {
                     Set<TransitiveDependency> transitives = getCompatibleGradleTransitives(
@@ -289,7 +284,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
                     for (ResolvedDependency dep : conf.getResolved()) {
                         if (dep.isDirect() &&
                                 doesNotMatchArguments(dep) &&
-                                isRedundant(dep, transitives, gradle.getMavenRepositories(), downloader, ctx)) {
+                                isRedundant(dep, transitives, resolver)) {
                             // This direct dependency is transitively provided, remove it
                             // Don't specify configuration - Gradle's resolved config names differ from declaration names
                             result = new RemoveDependency(
@@ -305,7 +300,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
                 String projectId = maven.getPom().getGroupId() + ":" + maven.getPom().getArtifactId();
                 Map<String, Set<TransitiveDependency>> scopeToTransitives =
                         acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap());
-                MavenPomDownloader downloader = new MavenPomDownloader(ctx);
+                ClosureResolver resolver = new ClosureResolver(maven.getPom().getRepositories(), ctx, acc.closureCache);
 
                 // A direct dependency appears under every scope bucket it is visible in; evaluate each
                 // one once using its own effective scope so a wider transitive scope does not falsely
@@ -319,7 +314,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
                             Scope depScope = Scope.fromName(dep.getRequested().getScope());
                             Set<TransitiveDependency> transitives = scopeToTransitives.getOrDefault(
                                     depScope.name().toLowerCase(), emptySet());
-                            if (isRedundant(dep, transitives, maven.getPom().getRepositories(), downloader, ctx)) {
+                            if (isRedundant(dep, transitives, resolver)) {
                                 // This direct dependency is transitively provided at the same scope and
                                 // with the same exclusions, remove it.
                                 result = new RemoveDependency(
@@ -338,20 +333,38 @@ private boolean doesNotMatchArguments(ResolvedDependency dep) {
             }
 
             private boolean isRedundant(ResolvedDependency dep, Set<TransitiveDependency> transitives,
-                                        List<MavenRepository> repositories, MavenPomDownloader downloader, ExecutionContext ctx) {
-                Set<GroupArtifact> depExclusions = relevantExclusions(dep, repositories, downloader, ctx, acc.closureCache);
+                                        ClosureResolver resolver) {
+                Set<GroupArtifact> depExclusions = declaredExclusions(dep);
+                Set<GroupArtifact> closure = null;
                 for (TransitiveDependency transitive : transitives) {
                     ResolvedGroupArtifactVersion gav = transitive.getGav();
                     if (dep.getGroupId().equals(gav.getGroupId()) &&
                             dep.getArtifactId().equals(gav.getArtifactId()) &&
-                            dep.getVersion().equals(gav.getVersion()) &&
-                            depExclusions.equals(transitive.getExclusions())) {
-                        return true;
+                            dep.getVersion().equals(gav.getVersion())) {
+                        if (depExclusions.equals(transitive.getExclusions())) {
+                            return true;
+                        }
+                        // Differing declarations can still be equivalent once exclusions that the coordinate
+                        // could never have honoured anyway are discarded. Both sides share one closure, and
+                        // every candidate here has the same coordinate, so this resolves at most once.
+                        if (closure == null) {
+                            closure = resolver.closureOf(gav.asGroupArtifactVersion());
+                        }
+                        if (retainClosure(depExclusions, closure).equals(
+                                retainClosure(transitive.getExclusions(), closure))) {
+                            return true;
+                        }
                     }
                 }
                 return false;
             }
 
+            private Set<GroupArtifact> retainClosure(Set<GroupArtifact> exclusions, Set<GroupArtifact> closure) {
+                Set<GroupArtifact> relevant = new HashSet<>(exclusions);
+                relevant.retainAll(closure);
+                return relevant;
+            }
+
             private Set<TransitiveDependency> getCompatibleGradleTransitives(
                     Map<String, Set<TransitiveDependency>> scopeToTransitives,
                     String targetScope) {

src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java

@@ -17,6 +17,7 @@
 
 import org.junit.jupiter.api.Test;
 import org.openrewrite.DocumentExample;
+import org.openrewrite.Issue;
 import org.openrewrite.test.RewriteTest;
 
 import static org.openrewrite.gradle.Assertions.buildGradle;
@@ -657,9 +658,9 @@ void removesTomcatEmbedCoreWhenExclusionsMatchTransitive() {
         );
     }
 
+    @Issue("https://github.com/openrewrite/rewrite/issues/8336")
     @Test
     void keepsWebsocketWhenDirectHasNoExclusionButTransitiveDoes() {
-        // https://github.com/openrewrite/rewrite/issues/8336
         // spring-boot-starter-tomcat excludes tomcat-annotations-api from tomcat-embed-websocket, which still
         // brings its own tomcat-embed-core. A direct websocket with no exclusion is therefore NOT equivalent to
         // the transitively-provided one and must be kept, even though websocket's effective-exclusion set is
@@ -697,9 +698,9 @@ void keepsWebsocketWhenDirectHasNoExclusionButTransitiveDoes() {
         );
     }
 
+    @Issue("https://github.com/openrewrite/rewrite/issues/8336")
     @Test
     void removesWebsocketWhenDirectRepeatsTheTransitiveExclusion() {
-        // https://github.com/openrewrite/rewrite/issues/8336
         // The mirror image: a direct websocket that repeats the same tomcat-annotations-api exclusion the
         // starter applies is truly equivalent to the transitively-provided one and should be removed.
         rewriteRun(