[PackageGraph] Use DFS to build a set of package graph nodes to load

Avoids duplicating the work and allows dependencies to have cyclic
entries because cyclic product and/or target references are the
only problematic cases for the build since they cannot be planned
properly.

Author: xedin

Sources/Basics/Graph/GraphAlgorithms.swift

@@ -49,9 +49,9 @@ public func DFS<T: Hashable>(
                 continue
             }
 
-            for succ in try successors(curr).reversed() {
+            for succ in try successors(curr) {
                 stack.append(succ)
             }
         }
     }
-}
+}

Sources/PackageGraph/ModulesGraph+Loading.swift

@@ -15,6 +15,7 @@ import OrderedCollections
 import PackageLoading
 import PackageModel
 
+import struct TSCBasic.KeyedPair
 import func TSCBasic.bestMatch
 
 extension ModulesGraph {
@@ -44,17 +45,6 @@ extension ModulesGraph {
         root.manifests.forEach {
             manifestMap[$0.key] = ($0.value, fileSystem)
         }
-        func nodeSuccessorsProvider(node: GraphLoadingNode) -> [GraphLoadingNode] {
-            node.requiredDependencies.compactMap { dependency in
-                manifestMap[dependency.identity].map { (manifest, fileSystem) in
-                    GraphLoadingNode(
-                        identity: dependency.identity,
-                        manifest: manifest,
-                        productFilter: dependency.productFilter
-                    )
-                }
-            }
-        }
 
         // Construct the root root dependencies set.
         let rootDependencies = Set(root.dependencies.compactMap{
@@ -75,30 +65,26 @@ extension ModulesGraph {
         let inputManifests = rootManifestNodes + rootDependencyNodes
 
         // Collect the manifests for which we are going to build packages.
-        var allNodes: [GraphLoadingNode]
-
-        // Detect cycles in manifest dependencies.
-        if let cycle = findCycle(inputManifests, successors: nodeSuccessorsProvider) {
-            observabilityScope.emit(PackageGraphError.cycleDetected(cycle))
-            // Break the cycle so we can build a partial package graph.
-            allNodes = inputManifests.filter({ $0.manifest != cycle.cycle[0] })
-        } else {
-            // Sort all manifests topologically.
-            allNodes = try topologicalSort(inputManifests, successors: nodeSuccessorsProvider)
-        }
+        var allNodes = [GraphLoadingNode]()
 
-        var flattenedManifests: [PackageIdentity: GraphLoadingNode] = [:]
-        for node in allNodes {
-            if let existing = flattenedManifests[node.identity] {
-                let merged = GraphLoadingNode(
-                    identity: node.identity,
-                    manifest: node.manifest,
-                    productFilter: existing.productFilter.union(node.productFilter)
-                )
-                flattenedManifests[node.identity] = merged
-            } else {
-                flattenedManifests[node.identity] = node
+        // Cycles in dependencies don't matter as long as there are no target cycles between packages.
+        DFS(inputManifests.map { KeyedPair($0, key: $0.id) }) {
+            $0.item.requiredDependencies.compactMap { dependency in
+                manifestMap[dependency.identity].map { (manifest, fileSystem) in
+                    KeyedPair(
+                        GraphLoadingNode(
+                            identity: dependency.identity,
+                            manifest: manifest,
+                            productFilter: dependency.productFilter
+                        ),
+                        key: dependency.identity
+                    )
+                }
             }
+        } onUnique: {
+            allNodes.append($0.item)
+        } onDuplicate: { _,_ in
+            // no de-duplication is required.
         }
 
         // Create the packages.
@@ -1065,55 +1051,3 @@ private final class ResolvedPackageBuilder: ResolvedBuilder<ResolvedPackage> {
         )
     }
 }
-
-/// Finds the first cycle encountered in a graph.
-///
-/// This is different from the one in tools support core, in that it handles equality separately from node traversal. Nodes traverse product filters, but only the manifests must be equal for there to be a cycle.
-fileprivate func findCycle(
-    _ nodes: [GraphLoadingNode],
-    successors: (GraphLoadingNode) throws -> [GraphLoadingNode]
-) rethrows -> (path: [Manifest], cycle: [Manifest])? {
-    // Ordered set to hold the current traversed path.
-    var path = OrderedCollections.OrderedSet<Manifest>()
-    
-    var fullyVisitedManifests = Set<Manifest>()
-
-    // Function to visit nodes recursively.
-    // FIXME: Convert to stack.
-    func visit(
-      _ node: GraphLoadingNode,
-      _ successors: (GraphLoadingNode) throws -> [GraphLoadingNode]
-    ) rethrows -> (path: [Manifest], cycle: [Manifest])? {
-        // Once all successors have been visited, this node cannot participate
-        // in a cycle.
-        if fullyVisitedManifests.contains(node.manifest) {
-            return nil
-        }
-        
-        // If this node is already in the current path then we have found a cycle.
-        if !path.append(node.manifest).inserted {
-            let index = path.firstIndex(of: node.manifest)! // forced unwrap safe
-            return (Array(path[path.startIndex..<index]), Array(path[index..<path.endIndex]))
-        }
-
-        for succ in try successors(node) {
-            if let cycle = try visit(succ, successors) {
-                return cycle
-            }
-        }
-        // No cycle found for this node, remove it from the path.
-        let item = path.removeLast()
-        assert(item == node.manifest)
-        // Track fully visited nodes
-        fullyVisitedManifests.insert(node.manifest)
-        return nil
-    }
-
-    for node in nodes {
-        if let cycle = try visit(node, successors) {
-            return cycle
-        }
-    }
-    // Couldn't find any cycle in the graph.
-    return nil
-}

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -1971,7 +1971,7 @@ final class WorkspaceTests: XCTestCase {
             // Ensure that the order of the manifests is stable.
             XCTAssertEqual(
                 manifests.allDependencyManifests.map(\.value.manifest.displayName),
-                ["Foo", "Bar", "Baz", "Bam"]
+                ["Bam", "Baz", "Bar", "Foo"]
             )
             XCTAssertNoDiagnostics(diagnostics)
         }
@@ -11123,7 +11123,7 @@ final class WorkspaceTests: XCTestCase {
                 // FIXME: rdar://72940946
                 // we need to improve this situation or diagnostics when working on identity
                 result.check(
-                    diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage",
+                    diagnostic: "'bar' dependency on '/tmp/ws/pkgs/other/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'",
                     severity: .error
                 )
             }
@@ -11207,7 +11207,11 @@ final class WorkspaceTests: XCTestCase {
                 // FIXME: rdar://72940946
                 // we need to improve this situation or diagnostics when working on identity
                 result.check(
-                    diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage",
+                    diagnostic: "'bar' dependency on '/tmp/ws/pkgs/other/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'. this will be escalated to an error in future versions of SwiftPM.",
+                    severity: .warning
+                )
+                result.check(
+                    diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'OtherUtilityPackage'.",
                     severity: .error
                 )
             }
@@ -11282,7 +11286,7 @@ final class WorkspaceTests: XCTestCase {
                 // FIXME: rdar://72940946
                 // we need to improve this situation or diagnostics when working on identity
                 result.check(
-                    diagnostic: "cyclic dependency declaration found: Root -> BarPackage -> Root",
+                    diagnostic: "product 'FooProduct' required by package 'bar' target 'BarTarget' not found in package 'FooPackage'.",
                     severity: .error
                 )
             }