Fix dup modmaps taking into account macro test targets. (#8524)

In my previous attempt to fix the duplicate modulemaps from C library
dependencies of macros/plugins and destination targets, I didn't take
into account test targets depending on macros, i.e. macro tests. In
these cases the modulemap flags do need to pass through to the tests.
This was discovered in swift-foundation which couldn't find the
_SwiftSyntaxCShims module map.

Fixed this my moving the traversing link dependencies a layer lower
since we need access to the parent node during traversal and this is
available in the successor methods. Cleaned up my previous attempt.

Author: dschaefer2

Sources/Basics/Graph/GraphAlgorithms.swift

@@ -131,50 +131,3 @@ public func depthFirstSearch<T: Hashable>(
         }
     }
 }
-
-/// Implements a pre-order depth-first search that traverses the whole graph and
-/// doesn't distinguish between unique and duplicate nodes. The visitor can abort
-/// a path as needed to prune the tree.
-/// The method expects the graph to be acyclic but doesn't check that.
-///
-/// - Parameters:
-///   - nodes: The list of input nodes to sort.
-///   - successors: A closure for fetching the successors of a particular node.
-///   - onNext: A callback to indicate the node currently being processed
-///             including its parent (if any) and its depth. Returns whether to
-///             continue down the current path.
-///
-/// - Complexity: O(v + e) where (v, e) are the number of vertices and edges
-/// reachable from the input nodes via the relation.
-public enum DepthFirstContinue {
-    case `continue`
-    case abort
-}
-
-public func depthFirstSearch<T: Hashable>(
-    _ nodes: [T],
-    successors: (T) throws -> [T],
-    visitNext: (T, _ parent: T?) throws -> DepthFirstContinue
-) rethrows {
-    var stack = OrderedSet<TraversalNode<T>>()
-
-    for node in nodes {
-        precondition(stack.isEmpty)
-        stack.append(TraversalNode(parent: nil, curr: node))
-
-        while !stack.isEmpty {
-            let node = stack.removeLast()
-
-            if try visitNext(node.curr, node.parent) == .continue {
-                for succ in try successors(node.curr) {
-                    stack.append(
-                        TraversalNode(
-                            parent: node.curr,
-                            curr: succ
-                        )
-                    )
-                }
-            }
-        }
-    }
-}

Sources/Build/BuildDescription/ModuleBuildDescription.swift

@@ -187,30 +187,18 @@ extension ModuleBuildDescription {
         var dependencies: [Dependency] = []
         plan.traverseDependencies(of: self) { product, _, description in
             dependencies.append(.product(product, description))
-            return .continue
         } onModule: { module, _, description in
             dependencies.append(.module(module, description))
-            return .continue
         }
         return dependencies
     }
 
     package func recursiveLinkDependencies(using plan: BuildPlan) -> [Dependency] {
         var dependencies: [Dependency] = []
-        plan.traverseDependencies(of: self) { product, _, description in
-            guard product.type != .macro && product.type != .plugin else {
-                return .abort
-            }
-
+        plan.traverseLinkDependencies(of: self) { product, _, description in
             dependencies.append(.product(product, description))
-            return .continue
         } onModule: { module, _, description in
-            guard module.type != .macro && module.type != .plugin else {
-                return .abort
-            }
-
             dependencies.append(.module(module, description))
-            return .continue
         }
         return dependencies
     }

Sources/Build/BuildPlan/BuildPlan+Swift.swift

@@ -24,7 +24,7 @@ extension BuildPlan {
     func plan(swiftTarget: SwiftModuleBuildDescription) throws {
         // We need to iterate recursive dependencies because Swift compiler needs to see all the targets a target
         // builds against
-        for case .module(let dependency, let description) in swiftTarget.recursiveDependencies(using: self) {
+        for case .module(let dependency, let description) in swiftTarget.recursiveLinkDependencies(using: self) {
             switch dependency.underlying {
             case let underlyingTarget as ClangModule where underlyingTarget.type == .library:
                 guard case let .clang(target)? = description else {

Sources/Build/BuildPlan/BuildPlan.swift

@@ -1175,8 +1175,8 @@ extension BuildPlan {
 
     package func traverseDependencies(
         of description: ModuleBuildDescription,
-        onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> DepthFirstContinue,
-        onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> DepthFirstContinue
+        onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> Void,
+        onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> Void
     ) {
         var visited = Set<TraversalNode>()
         func successors(
@@ -1217,16 +1217,92 @@ extension BuildPlan {
             case .package:
                 []
             }
-        } visitNext: { module, _ in
+        } onNext: { module, _ in
             switch module {
             case .package:
-                return .continue
+                break
+
+            case .product(let product, let destination):
+                onProduct(product, destination, self.description(for: product, context: destination))
+
+            case .module(let module, let destination):
+                onModule(module, destination, self.description(for: module, context: destination))
+            }
+        }
+    }
+
+    // Only follow link time dependencies, i.e. skip dependencies on macros and plugins
+    // except for testTargets that depend on macros.
+    package func traverseLinkDependencies(
+        of description: ModuleBuildDescription,
+        onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> Void,
+        onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> Void
+    ) {
+        var visited = Set<TraversalNode>()
+        func successors(
+            for product: ResolvedProduct,
+            destination: Destination
+        ) -> [TraversalNode] {
+            product.modules.map { module in
+                TraversalNode(module: module, context: destination)
+            }.filter {
+                visited.insert($0).inserted
+            }
+        }
+
+        func successors(
+            for parentModule: ResolvedModule,
+            destination: Destination
+        ) -> [TraversalNode] {
+            parentModule
+                .dependencies(satisfying: description.buildParameters.buildEnvironment)
+                .reduce(into: [TraversalNode]()) { partial, dependency in
+                    switch dependency {
+                    case .product(let product, _):
+                        guard product.type != .plugin else {
+                            return
+                        }
+                        
+                        guard product.type != .macro || parentModule.type == .test else {
+                            return
+                        }
+
+                        partial.append(.init(product: product, context: destination))
+                    case .module(let childModule, _):
+                        guard childModule.type != .plugin else {
+                            return
+                        }
+                        
+                        guard childModule.type != .macro || parentModule.type == .test else {
+                            return
+                        }
+
+                        partial.append(.init(module: childModule, context: destination))
+                    }
+                }.filter {
+                    visited.insert($0).inserted
+                }
+        }
+
+        depthFirstSearch(successors(for: description.module, destination: description.destination)) {
+            switch $0 {
+            case .module(let module, let destination):
+                successors(for: module, destination: destination)
+            case .product(let product, let destination):
+                successors(for: product, destination: destination)
+            case .package:
+                []
+            }
+        } onNext: { module, _ in
+            switch module {
+            case .package:
+                break
 
             case .product(let product, let destination):
-                return onProduct(product, destination, self.description(for: product, context: destination))
+                onProduct(product, destination, self.description(for: product, context: destination))
 
             case .module(let module, let destination):
-                return onModule(module, destination, self.description(for: module, context: destination))
+                onModule(module, destination, self.description(for: module, context: destination))
             }
         }
     }

Tests/BuildTests/BuildPlanTests.swift

@@ -6928,14 +6928,16 @@ class BuildPlanTestCase: BuildSystemProviderTestCase {
             "/LeakTest/Sources/CLib/Clib.c",
             "/LeakTest/Sources/MyMacro/MyMacro.swift",
             "/LeakTest/Sources/MyPluginTool/MyPluginTool.swift",
-            "/LeakTest/Plugins/MyPlugin/MyPlugin.swift",
             "/LeakTest/Sources/MyLib/MyLib.swift",
+            "/LeakTest/Plugins/MyPlugin/MyPlugin.swift",
+            "/LeakTest/Tests/MyMacroTests/MyMacroTests.swift",
+            "/LeakTest/Tests/MyMacro2Tests/MyMacro2Tests.swift",
             "/LeakLib/Sources/CLib2/include/Clib.h",
             "/LeakLib/Sources/CLib2/Clib.c",
             "/LeakLib/Sources/MyMacro2/MyMacro.swift",
             "/LeakLib/Sources/MyPluginTool2/MyPluginTool.swift",
+            "/LeakLib/Sources/MyLib2/MyLib.swift",
             "/LeakLib/Plugins/MyPlugin2/MyPlugin.swift",
-            "/LeakLib/Sources/MyLib2/MyLib.swift"
         ])
 
         let graph = try loadModulesGraph(fileSystem: fs, manifests: [
@@ -6944,6 +6946,7 @@ class BuildPlanTestCase: BuildSystemProviderTestCase {
                 path: "/LeakLib",
                 products: [
                     ProductDescription(name: "MyLib2", type: .library(.automatic), targets: ["MyLib2"]),
+                    ProductDescription(name: "MyMacros2", type: .macro, targets: ["MyMacro2"])
                 ],
                 targets: [
                     TargetDescription(name: "CLib2"),
@@ -6969,6 +6972,11 @@ class BuildPlanTestCase: BuildSystemProviderTestCase {
                         dependencies: ["CLib", "MyMacro", .product(name: "MyLib2", package: "LeakLib")],
                         pluginUsages: [.plugin(name: "MyPlugin", package: nil)]
                     ),
+                    TargetDescription(name: "MyMacroTests", dependencies: ["MyMacro"], type: .test),
+                    TargetDescription(
+                        name: "MyMacro2Tests",
+                        dependencies: [.product(name: "MyMacros2", package: "LeakLib")],
+                        type: .test),
                 ]
             )
         ], observabilityScope: observability.topScope)
@@ -6982,8 +6990,17 @@ class BuildPlanTestCase: BuildSystemProviderTestCase {
         XCTAssertNoDiagnostics(observability.diagnostics)
 
         let myLib = try XCTUnwrap(plan.targets.first(where: { $0.module.name == "MyLib" })).swift()
-        print(myLib.additionalFlags)
-        XCTAssertFalse(myLib.additionalFlags.contains(where: { $0.contains("-tool/include")}), "flags shouldn't contain tools items")
+        XCTAssertFalse(myLib.additionalFlags.contains(where: { $0.contains("-tool")}), "flags shouldn't contain tools items")
+        
+        // Make sure the tests do have the include path and the module map from the lib
+        let myMacroTests = try XCTUnwrap(plan.targets.first(where: { $0.module.name == "MyMacroTests" })).swift()
+        let flags = myMacroTests.additionalFlags.joined(separator: " ")
+        XCTAssertMatch(flags, .regex("CLib[/\\\\]include"))
+        XCTAssertMatch(flags, .regex("CLib-tool.build[/\\\\]module.modulemap"))
+        let myMacro2Tests = try XCTUnwrap(plan.targets.first(where: { $0.module.name == "MyMacro2Tests" })).swift()
+        let flags2 = myMacro2Tests.additionalFlags.joined(separator: " ")
+        XCTAssertMatch(flags2, .regex("CLib2[/\\\\]include"))
+        XCTAssertMatch(flags2, .regex("CLib2-tool.build[/\\\\]module.modulemap"))
     }
 
     func testDiagnosticsAreMentionedInOutputsFileMap() async throws {

Tests/BuildTests/BuildPlanTraversalTests.swift

@@ -146,10 +146,8 @@ final class BuildPlanTraversalTests: XCTestCase {
             XCTAssertEqual(product.name, "SwiftSyntax")
             XCTAssertEqual(destination, .host)
             XCTAssertNil(description)
-            return .continue
         } onModule: { module, destination, description in
             moduleDependencies.append((module, destination, description))
-            return .continue
         }
 
         XCTAssertEqual(moduleDependencies.count, 2)