diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java index 1178ea1744a..da6828e9cf4 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/builders/DependencyLoopFinder.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2012 IBM Corporation and others. + * Copyright (c) 2000, 2026 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -13,7 +13,15 @@ *******************************************************************************/ package org.eclipse.pde.internal.core.builders; -import java.util.Vector; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; import org.eclipse.osgi.util.NLS; import org.eclipse.pde.core.plugin.IPlugin; @@ -25,6 +33,13 @@ public class DependencyLoopFinder { + /** + * Upper bound on the number of reported loops. The search runs + * synchronously on the UI thread and the number of distinct cycles through + * a plug-in can grow exponentially with the size of the tangle it sits in. + */ + private static final int MAX_LOOPS = 100; + public static DependencyLoop[] findLoops(IPlugin root) { return findLoops(root, null); } @@ -34,94 +49,128 @@ public static DependencyLoop[] findLoops(IPlugin root, IPlugin[] candidates) { } public static DependencyLoop[] findLoops(IPlugin root, IPlugin[] candidates, boolean onlyCandidates) { - Vector loops = new Vector<>(); - - Vector path = new Vector<>(); - findLoops(loops, path, root, candidates, onlyCandidates, new Vector<>()); - return loops.toArray(new DependencyLoop[loops.size()]); + LoopSearch search = new LoopSearch(root.getId()); + List rootDependencies = new ArrayList<>(); + if (!onlyCandidates) { + rootDependencies.addAll(search.dependenciesOf(root)); + } + if (candidates != null) { + rootDependencies.addAll(Arrays.asList(candidates)); + } + search.restrictToLoopMembers(rootDependencies); + search.collectLoops(root, rootDependencies); + return search.loops(); } - private static void findLoops(Vector loops, Vector path, IPlugin subroot, IPlugin[] candidates, boolean onlyCandidates, Vector exploredPlugins) { - if (!path.isEmpty()) { - // test the path so far - // is the subroot the same as root - if yes, that's it - - IPlugin root = path.elementAt(0); - if (isEquivalent(root, subroot)) { - // our loop!! - DependencyLoop loop = new DependencyLoop(); - loop.setMembers(path.toArray(new IPlugin[path.size()])); - int no = loops.size() + 1; - loop.setName(NLS.bind(PDECoreMessages.Builders_DependencyLoopFinder_loopName, ("" + no))); //$NON-NLS-1$ - loops.add(loop); - return; + /** + * Enumerates the cycles that pass through one root plug-in. + */ + private static final class LoopSearch { + + private final String rootId; + private final List loops = new ArrayList<>(); + private final List path = new ArrayList<>(); + private final Map> dependencies = new HashMap<>(); + private Set loopMembers = Set.of(); + + LoopSearch(String rootId) { + this.rootId = rootId; + } + + /** + * Narrows the search to the plug-ins that can actually sit on a cycle + * through the root, that is those reachable from the root that also + * lead back to it. Unlike a "this plug-in yielded no loop" blacklist, + * this property does not depend on the path taken to reach a plug-in, + * so pruning by it cannot hide a cycle. + */ + void restrictToLoopMembers(List rootDependencies) { + Map> dependents = new HashMap<>(); + Set reached = new HashSet<>(); + Deque pending = new ArrayDeque<>(); + for (IPlugin dependency : rootDependencies) { + dependents.computeIfAbsent(dependency.getId(), id -> new ArrayList<>()).add(rootId); + if (reached.add(dependency.getId())) { + pending.add(dependency); + } } - // is the subroot the same as any other node? - // if yes, abort - local loop that is not ours - for (int i = 1; i < path.size(); i++) { - IPlugin node = path.elementAt(i); - if (isEquivalent(subroot, node)) { - // local loop - return; + while (!pending.isEmpty()) { + IPlugin plugin = pending.remove(); + for (IPlugin dependency : dependenciesOf(plugin)) { + dependents.computeIfAbsent(dependency.getId(), id -> new ArrayList<>()).add(plugin.getId()); + if (reached.add(dependency.getId())) { + pending.add(dependency); + } + } + } + // walking the collected edges backwards from the root yields the + // plug-ins that lead back to it + loopMembers = new HashSet<>(); + Deque backwards = new ArrayDeque<>(); + backwards.add(rootId); + while (!backwards.isEmpty()) { + for (String dependent : dependents.getOrDefault(backwards.remove(), List.of())) { + if (!dependent.equals(rootId) && loopMembers.add(dependent)) { + backwards.add(dependent); + } } } } - @SuppressWarnings("unchecked") - Vector newPath = !path.isEmpty() ? ((Vector) path.clone()) : path; - newPath.add(subroot); - if (!onlyCandidates) { - IPluginImport[] iimports = subroot.getImports(); - for (IPluginImport iimport : iimports) { - String id = iimport.getId(); - //Be paranoid - if (id == null) { - continue; + void collectLoops(IPlugin plugin, List pluginDependencies) { + path.add(plugin); + for (IPlugin dependency : pluginDependencies) { + if (loops.size() >= MAX_LOOPS) { + break; } - if (!exploredPlugins.contains(id)) { - // is plugin in list of non loop yielding plugins - //Commenting linear lookup - was very slow - //when called from here. We will use - //model manager instead because it - //has a hash table lookup that is much faster. - //IPlugin child = PDECore.getDefault().findPlugin(id); - IPlugin child = findPlugin(id); - if (child != null) { - // number of loops before traversing plugin - int oldLoopSize = loops.size(); - - findLoops(loops, newPath, child, null, false, exploredPlugins); - - // number of loops after traversing plugin - int newLoopsSize = loops.size(); - - if (oldLoopSize == newLoopsSize) {// no change in number of loops - // no loops from going to this node, skip next time - exploredPlugins.add(id); - } - } + String id = dependency.getId(); + if (rootId.equals(id)) { + addLoop(); + } else if (loopMembers.contains(id) && !isOnPath(id)) { + collectLoops(dependency, dependenciesOf(dependency)); } - } - + path.remove(path.size() - 1); } - if (candidates != null) { - for (IPlugin candidate : candidates) { - // number of loops before traversing plugin - int oldLoopSize = loops.size(); - findLoops(loops, newPath, candidate, null, false, exploredPlugins); + /** + * Returns the plug-ins required by the given one, resolved through the + * registry. Cached, as the search visits a plug-in once per path + * leading to it. + */ + List dependenciesOf(IPlugin plugin) { + return dependencies.computeIfAbsent(plugin.getId(), id -> { + List resolved = new ArrayList<>(); + for (IPluginImport iimport : plugin.getImports()) { + String importedId = iimport.getId(); + //Be paranoid + if (importedId == null) { + continue; + } + IPlugin imported = findPlugin(importedId); + if (imported != null) { + resolved.add(imported); + } + } + return resolved; + }); + } - // number of loops after traversing plugin - int newLoopsSize = loops.size(); + private boolean isOnPath(String id) { + return path.stream().anyMatch(plugin -> id.equals(plugin.getId())); + } - if (oldLoopSize == newLoopsSize) { // no change in number of loops - // no loops from going to this node, skip next time - exploredPlugins.add(candidate.getId()); - } - } + private void addLoop() { + DependencyLoop loop = new DependencyLoop(); + loop.setMembers(path.toArray(new IPlugin[path.size()])); + int no = loops.size() + 1; + loop.setName(NLS.bind(PDECoreMessages.Builders_DependencyLoopFinder_loopName, ("" + no))); //$NON-NLS-1$ + loops.add(loop); } + DependencyLoop[] loops() { + return loops.toArray(new DependencyLoop[loops.size()]); + } } private static IPlugin findPlugin(String id) { @@ -131,8 +180,4 @@ private static IPlugin findPlugin(String id) { } return (IPlugin) childModel.getPluginBase(); } - - private static boolean isEquivalent(IPlugin left, IPlugin right) { - return left.getId().equals(right.getId()); - } } diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java index 496d54a3935..810878cf8df 100644 --- a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/core/tests/internal/core/builders/DependencyLoopFinderTest.java @@ -115,6 +115,34 @@ public void testTwoSeparateCyclesThroughRoot() throws Exception { assertEquals(List.of("loop.r -> loop.a", "loop.r -> loop.b"), loopSignatures("loop.r")); } + /** + * A cycle that is only reachable through a second dependency of the root + * must be reported too. + * + *
+	 *   r -> a, r -> d
+	 *   a -> b, a -> r
+	 *   b -> a
+	 *   d -> b
+	 * 
+ * + * Two cycles pass through {@code r}: {@code r -> a -> r} and + * {@code r -> d -> b -> a -> r}. Reaching {@code b} from {@code a} ends in + * a cycle that does not touch {@code r}, which must not stop the search + * from reaching {@code b} again through {@code d}. + */ + @Test + public void testCycleReachableOnlyViaSecondImportPathIsFound() throws Exception { + setTargetPlatform( // + bundle("loop.r", "1.0.0", entry(REQUIRE_BUNDLE, "loop.a,loop.d")), // + bundle("loop.a", "1.0.0", entry(REQUIRE_BUNDLE, "loop.b,loop.r")), // + bundle("loop.b", "1.0.0", entry(REQUIRE_BUNDLE, "loop.a")), // + bundle("loop.d", "1.0.0", entry(REQUIRE_BUNDLE, "loop.b"))); + + assertEquals(List.of("loop.r -> loop.a", "loop.r -> loop.d -> loop.b -> loop.a"), + loopSignatures("loop.r")); + } + /** * The reported cycles must not depend on the order in which the root * declares its dependencies.