Fix duplicate modulemap errors with macro and plugin deps (swiftlang#8472)

We were including flags to hook up modulemaps and include files to C
library dependencies in macros and plugin tools through to the modules
that depend on those. This adds the capability to prune the depth first
searches through the dependencies to ensure these are skipped when
crossing macro and plugin boundaries.

Author: dschaefer2

Sources/Basics/Graph/GraphAlgorithms.swift

@@ -131,3 +131,50 @@ 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,8 +187,30 @@ 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
+            }
+
+            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/BuildDescription/SwiftModuleBuildDescription.swift

@@ -1042,6 +1042,12 @@ extension SwiftModuleBuildDescription {
         ModuleBuildDescription.swift(self).dependencies(using: plan)
     }
 
+    package func recursiveLinkDependencies(
+        using plan: BuildPlan
+    ) -> [ModuleBuildDescription.Dependency] {
+        ModuleBuildDescription.swift(self).recursiveLinkDependencies(using: plan)
+    }
+
     package func recursiveDependencies(
         using plan: BuildPlan
     ) -> [ModuleBuildDescription.Dependency] {

Sources/Build/BuildPlan/BuildPlan+Swift.swift

@@ -19,7 +19,7 @@ import class PackageModel.SystemLibraryModule
 extension BuildPlan {
     func plan(swiftTarget: SwiftModuleBuildDescription) throws {
         // We need to iterate recursive dependencies because Swift compiler needs to see all the targets a target
-        // depends on.
+        // builds against
         for case .module(let dependency, let description) in swiftTarget.recursiveDependencies(using: self) {
             switch dependency.underlying {
             case let underlyingTarget as ClangModule where underlyingTarget.type == .library:
@@ -53,5 +53,4 @@ extension BuildPlan {
             }
         }
     }
-
 }

Sources/Build/BuildPlan/BuildPlan.swift

@@ -1153,8 +1153,8 @@ extension BuildPlan {
 
     package func traverseDependencies(
         of description: ModuleBuildDescription,
-        onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> Void,
-        onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> Void
+        onProduct: (ResolvedProduct, BuildParameters.Destination, ProductBuildDescription?) -> DepthFirstContinue,
+        onModule: (ResolvedModule, BuildParameters.Destination, ModuleBuildDescription?) -> DepthFirstContinue
     ) {
         var visited = Set<TraversalNode>()
         func successors(
@@ -1195,16 +1195,16 @@ extension BuildPlan {
             case .package:
                 []
             }
-        } onNext: { module, _ in
+        } visitNext: { module, _ in
             switch module {
             case .package:
-                break
+                return .continue
 
             case .product(let product, let destination):
-                onProduct(product, destination, self.description(for: product, context: destination))
+                return onProduct(product, destination, self.description(for: product, context: destination))
 
             case .module(let module, let destination):
-                onModule(module, destination, self.description(for: module, context: destination))
+                return onModule(module, destination, self.description(for: module, context: destination))
             }
         }
     }

Tests/BuildTests/BuildPlanTests.swift

@@ -6842,4 +6842,70 @@ final class BuildPlanTests: XCTestCase {
         XCTAssertMatch(contents, .regex(#"args: \[.*"-I","/testpackagedep/SomeArtifact.xcframework/macos/Headers".*,"/testpackage/Sources/CLib/lib.c".*]"#))
         XCTAssertMatch(contents, .regex(#"args: \[.*"-module-name","SwiftLib",.*"-I","/testpackagedep/SomeArtifact.xcframework/macos/Headers".*]"#))
     }
+
+    func testMacroPluginDependencyLeakage() async throws {
+        // Make sure the include paths from macro and plugin executables don't leak into dependents
+        let observability = ObservabilitySystem.makeForTesting()
+        let fs = InMemoryFileSystem(emptyFiles: [
+            "/LeakTest/Sources/CLib/include/Clib.h",
+            "/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",
+            "/LeakLib/Sources/CLib2/include/Clib.h",
+            "/LeakLib/Sources/CLib2/Clib.c",
+            "/LeakLib/Sources/MyMacro2/MyMacro.swift",
+            "/LeakLib/Sources/MyPluginTool2/MyPluginTool.swift",
+            "/LeakLib/Plugins/MyPlugin2/MyPlugin.swift",
+            "/LeakLib/Sources/MyLib2/MyLib.swift"
+        ])
+
+        let graph = try loadModulesGraph(fileSystem: fs, manifests: [
+            Manifest.createFileSystemManifest(
+                displayName: "LeakLib",
+                path: "/LeakLib",
+                products: [
+                    ProductDescription(name: "MyLib2", type: .library(.automatic), targets: ["MyLib2"]),
+                ],
+                targets: [
+                    TargetDescription(name: "CLib2"),
+                    TargetDescription(name: "MyMacro2", dependencies: ["CLib2"], type: .macro),
+                    TargetDescription(name: "MyPluginTool2", dependencies: ["CLib2"], type: .executable),
+                    TargetDescription(name: "MyPlugin2", dependencies: ["MyPluginTool2"], type: .plugin, pluginCapability: .buildTool),
+                    TargetDescription(name: "MyLib2", dependencies: ["CLib2", "MyMacro2"], pluginUsages: [.plugin(name: "MyPlugin2", package: nil)]),
+                ]
+            ),
+            Manifest.createRootManifest(
+                displayName: "LeakTest",
+                path: "/LeakTest",
+                dependencies: [
+                    .fileSystem(path: "/LeakLib")
+                ],
+                targets: [
+                    TargetDescription(name: "CLib"),
+                    TargetDescription(name: "MyMacro", dependencies: ["CLib"], type: .macro),
+                    TargetDescription(name: "MyPluginTool", dependencies: ["CLib"], type: .executable),
+                    TargetDescription(name: "MyPlugin", dependencies: ["MyPluginTool"], type: .plugin, pluginCapability: .buildTool),
+                    TargetDescription(
+                        name: "MyLib",
+                        dependencies: ["CLib", "MyMacro", .product(name: "MyLib2", package: "LeakLib")],
+                        pluginUsages: [.plugin(name: "MyPlugin", package: nil)]
+                    ),
+                ]
+            )
+        ], observabilityScope: observability.topScope)
+        XCTAssertNoDiagnostics(observability.diagnostics)
+
+        let plan = try await mockBuildPlan(
+            graph: graph,
+            fileSystem: fs,
+            observabilityScope: observability.topScope
+        )
+        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")
+    }
 }

Tests/BuildTests/BuildPlanTraversalTests.swift

@@ -146,8 +146,10 @@ 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)