[PackageGraph] ModuleGraph: Product and target lookups should involve a destination

The products and targets are currently indentified by their
package + name + build triple and they should be looked up
in the same fashion to avoid situations when results of
`product(for:)` and `target(for:)` return target that has
an unexpected destination.

Author: xedin

Sources/Build/BuildOperation.swift

@@ -522,19 +522,48 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
             return LLBuildManifestBuilder.TargetKind.main.targetName
         case .allIncludingTests:
             return LLBuildManifestBuilder.TargetKind.test.targetName
-        case .product(_, let destination), .target(_, let destination):
+        case .product(let productName, let destination):
             // FIXME: This is super unfortunate that we might need to load the package graph.
             let graph = try getPackageGraph()
-            if let result = subset.llbuildTargetName(
-                for: graph,
-                using: destination == .host
-                            ? self.toolsBuildParameters
-                            : self.productsBuildParameters,
-                observabilityScope: self.observabilityScope
-            ) {
-                return result
+
+            guard let product = graph.product(
+                for: productName,
+                destination: destination == .host ? .tools : .destination
+            ) else {
+                observabilityScope.emit(error: "no product named '\(productName)'")
+                throw Diagnostics.fatalError
             }
-            throw Diagnostics.fatalError
+            // If the product is automatic, we build the main target because automatic products
+            // do not produce a binary right now.
+            if product.type == .library(.automatic) {
+                observabilityScope.emit(
+                    warning:
+                        "'--product' cannot be used with the automatic product '\(productName)'; building the default target instead"
+                )
+                return LLBuildManifestBuilder.TargetKind.main.targetName
+            }
+            return try product.getLLBuildTargetName(
+                    buildParameters: destination == .host
+                                        ? self.toolsBuildParameters
+                                        : self.productsBuildParameters
+            )
+        case .target(let targetName, let destination):
+            // FIXME: This is super unfortunate that we might need to load the package graph.
+            let graph = try getPackageGraph()
+
+            guard let target = graph.target(
+                for: targetName,
+                destination: destination == .host ? .tools : .destination
+            ) else {
+                observabilityScope.emit(error: "no target named '\(targetName)'")
+                throw Diagnostics.fatalError
+            }
+
+            return target.getLLBuildTargetName(
+                buildParameters: destination == .host
+                                            ? self.toolsBuildParameters
+                                            : self.productsBuildParameters
+            )
         }
     }
 
@@ -898,59 +927,24 @@ extension BuildSubset {
             return Array(graph.reachableTargets)
         case .allExcludingTests:
             return graph.reachableTargets.filter { $0.type != .test }
-        case .product(let productName, _):
-            guard let product = graph.product(for: productName) else {
-                observabilityScope.emit(error: "no product named '\(productName)'")
-                return nil
-            }
-            return try product.recursiveTargetDependencies()
-        case .target(let targetName, _):
-            guard let target = graph.target(for: targetName) else {
-                observabilityScope.emit(error: "no target named '\(targetName)'")
-                return nil
-            }
-            return try target.recursiveTargetDependencies()
-        }
-    }
-
-    /// Returns the name of the llbuild target that corresponds to the build subset.
-    func llbuildTargetName(
-        for graph: ModulesGraph,
-        using buildParameters: BuildParameters,
-        observabilityScope: ObservabilityScope
-    ) -> String? {
-        switch self {
-        case .allExcludingTests:
-            return LLBuildManifestBuilder.TargetKind.main.targetName
-        case .allIncludingTests:
-            return LLBuildManifestBuilder.TargetKind.test.targetName
         case .product(let productName, let destination):
-            precondition(buildParameters.destination == destination)
-
-            guard let product = graph.product(for: productName) else {
+            guard let product = graph.product(
+                for: productName,
+                destination: destination == .host ? .tools : .destination
+            ) else {
                 observabilityScope.emit(error: "no product named '\(productName)'")
                 return nil
             }
-            // If the product is automatic, we build the main target because automatic products
-            // do not produce a binary right now.
-            if product.type == .library(.automatic) {
-                observabilityScope.emit(
-                    warning:
-                        "'--product' cannot be used with the automatic product '\(productName)'; building the default target instead"
-                )
-                return LLBuildManifestBuilder.TargetKind.main.targetName
-            }
-            return observabilityScope.trap {
-                try product.getLLBuildTargetName(buildParameters: buildParameters)
-            }
+            return try product.recursiveTargetDependencies()
         case .target(let targetName, let destination):
-            precondition(buildParameters.destination == destination)
-
-            guard let target = graph.target(for: targetName) else {
+            guard let target = graph.target(
+                for: targetName,
+                destination: destination == .host ? .tools : .destination
+            ) else {
                 observabilityScope.emit(error: "no target named '\(targetName)'")
                 return nil
             }
-            return target.getLLBuildTargetName(buildParameters: buildParameters)
+            return try target.recursiveTargetDependencies()
         }
     }
 }

Sources/Build/BuildPlan/BuildPlan.swift

@@ -580,7 +580,7 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
         arguments.append("-l" + replProductName)
 
         // The graph should have the REPL product.
-        assert(self.graph.product(for: replProductName) != nil)
+        assert(self.graph.product(for: replProductName, destination: .destination) != nil)
 
         // Add the search path to the directory containing the modulemap file.
         for target in self.targets {

Sources/Commands/Snippets/Cards/SnippetCard.swift

@@ -96,7 +96,7 @@ struct SnippetCard: Card {
         let buildSystem = try swiftCommandState.createBuildSystem(explicitProduct: snippet.name)
         try buildSystem.build(subset: .product(snippet.name))
         let executablePath = try swiftCommandState.productsBuildParameters.buildPath.appending(component: snippet.name)
-        if let exampleTarget = try buildSystem.getPackageGraph().target(for: snippet.name) {
+        if let exampleTarget = try buildSystem.getPackageGraph().target(for: snippet.name, destination: .destination) {
             try ProcessEnv.chdir(exampleTarget.sources.paths[0].parentDirectory)
         }
         try exec(path: executablePath.pathString, args: [])

Sources/Commands/SwiftRunCommand.swift

@@ -220,7 +220,7 @@ public struct SwiftRunCommand: AsyncSwiftCommand {
         if let executable = options.executable {
             // There should be only one product with the given name in the graph
             // and it should be executable or snippet.
-            guard let product = graph.product(for: executable),
+            guard let product = graph.product(for: executable, destination: .destination),
                   product.type == .executable || product.type == .snippet
             else {
                 throw RunError.executableNotFound(executable)

Sources/Commands/Utilities/PluginDelegate.swift

@@ -385,7 +385,7 @@ final class PluginDelegate: PluginInvocationDelegate {
 
         // Find the target in the build operation's package graph; it's an error if we don't find it.
         let packageGraph = try buildSystem.getPackageGraph()
-        guard let target = packageGraph.target(for: targetName) else {
+        guard let target = packageGraph.target(for: targetName, destination: .destination) else {
             throw StringError("could not find a target named “\(targetName)”")
         }
 
@@ -430,6 +430,7 @@ final class PluginDelegate: PluginInvocationDelegate {
         let result = try symbolGraphExtractor.extractSymbolGraph(
             module: target,
             buildPlan: try buildSystem.buildPlan,
+            buildParameters: buildSystem.buildPlan.destinationBuildParameters,
             outputRedirection: .collect,
             outputDirectory: outputDir,
             verboseOutput: self.swiftCommandState.logLevel <= .info

Sources/PackageGraph/ModulesGraph.swift

@@ -166,14 +166,12 @@ public struct ModulesGraph {
         package.dependencies.compactMap { self.package(for: $0) }
     }
 
-    public func product(for name: String) -> ResolvedProduct? {
-        // FIXME: This is wrong since graph can contain products with the same name but different triples.
-        self.allProducts.first { $0.name == name }
+    public func product(for name: String, destination: BuildTriple) -> ResolvedProduct? {
+        self.allProducts.first { $0.name == name && $0.buildTriple == destination }
     }
 
-    public func target(for name: String) -> ResolvedModule? {
-        // FIXME: This is wrong since graph can contain products with the same name but different triples.
-        self.allTargets.first { $0.name == name }
+    public func target(for name: String, destination: BuildTriple) -> ResolvedModule? {
+        self.allTargets.first { $0.name == name && $0.buildTriple == destination }
     }
 
     /// All root and root dependency packages provided as input to the graph.

Sources/SPMBuildCore/Plugins/PluginInvocation.swift

@@ -675,7 +675,7 @@ public extension PluginTarget {
                 executableOrBinaryTarget = target
             case .product(let productRef, _):
                 guard
-                    let product = packageGraph.product(for: productRef.name),
+                    let product = packageGraph.product(for: productRef.name, destination: .tools),
                     let executableTarget = product.targets.map({ $0.underlying }).executables.spm_only
                 else {
                     throw StringError("no product named \(productRef.name)")

Sources/SPMTestSupport/PackageGraphTester.swift

@@ -88,19 +88,18 @@ public final class PackageGraphResult {
 
     public func checkTarget(
         _ name: String,
+        destination: BuildTriple = .destination,
         file: StaticString = #file,
         line: UInt = #line,
         body: (ResolvedTargetResult) -> Void
     ) {
-        let targets = find(target: name)
+        let target = graph.target(for: name, destination: destination)
 
-        guard targets.count > 0 else {
+        guard let target else {
             return XCTFail("Target \(name) not found", file: file, line: line)
         }
-        guard targets.count == 1 else {
-            return XCTFail("More than a single target with name \(name) found", file: file, line: line)
-        }
-        body(ResolvedTargetResult(targets[0]))
+
+        body(ResolvedTargetResult(target))
     }
 
     package func checkTargets(
@@ -114,20 +113,18 @@ public final class PackageGraphResult {
 
     public func checkProduct(
         _ name: String,
+        destination: BuildTriple = .destination,
         file: StaticString = #file,
         line: UInt = #line,
         body: (ResolvedProductResult) -> Void
     ) {
-        let products = find(product: name)
+        let product = graph.product(for: name, destination: destination)
 
-        guard products.count > 0 else {
+        guard let product else {
             return XCTFail("Product \(name) not found", file: file, line: line)
         }
 
-        guard products.count == 1 else {
-            return XCTFail("More than a single product with name \(name) found", file: file, line: line)
-        }
-        body(ResolvedProductResult(products[0]))
+        body(ResolvedProductResult(product))
     }
 
     public func check(testModules: String..., file: StaticString = #file, line: UInt = #line) {
@@ -138,14 +135,6 @@ public final class PackageGraphResult {
                 .sorted(), testModules.sorted(), file: file, line: line)
     }
 
-    public func find(target: String) -> [ResolvedModule] {
-        return graph.allTargets.filter { $0.name == target }
-    }
-
-    public func find(product: String) -> [ResolvedProduct] {
-        return graph.allProducts.filter { $0.name == product }
-    }
-
     public func find(package: PackageIdentity) -> ResolvedPackage? {
         return graph.package(for: package)
     }

Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift

@@ -119,7 +119,7 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
             result.checkTargets("MMIOMacros") { results in
                 XCTAssertEqual(results.count, 1)
             }
-            result.checkTarget("MMIOMacrosTests") { result in
+            result.checkTarget("MMIOMacrosTests", destination: .tools) { result in
                 result.check(buildTriple: .tools)
                 result.checkDependency("MMIOMacros") { result in
                     result.checkTarget { result in

Tests/PackageGraphTests/ModulesGraphTests.swift

@@ -84,12 +84,12 @@ final class ModulesGraphTests: XCTestCase {
         }
 
         let fooPackage = try XCTUnwrap(g.package(for: .plain("Foo")))
-        let fooTarget = try XCTUnwrap(g.target(for: "Foo"))
-        let fooDepTarget = try XCTUnwrap(g.target(for: "FooDep"))
+        let fooTarget = try XCTUnwrap(g.target(for: "Foo", destination: .destination))
+        let fooDepTarget = try XCTUnwrap(g.target(for: "FooDep", destination: .destination))
         XCTAssertEqual(g.package(for: fooTarget)?.id, fooPackage.id)
         XCTAssertEqual(g.package(for: fooDepTarget)?.id, fooPackage.id)
         let barPackage = try XCTUnwrap(g.package(for: .plain("Bar")))
-        let barTarget = try XCTUnwrap(g.target(for: "Bar"))
+        let barTarget = try XCTUnwrap(g.target(for: "Bar", destination: .destination))
         XCTAssertEqual(g.package(for: barTarget)?.id, barPackage.id)
     }
 

Tests/SPMBuildCoreTests/PluginInvocationTests.swift

@@ -36,6 +36,7 @@ final class PluginInvocationTests: XCTestCase {
         let fileSystem = InMemoryFileSystem(emptyFiles:
             "/Foo/Plugins/FooPlugin/source.swift",
             "/Foo/Sources/FooTool/source.swift",
+            "/Foo/Sources/FooToolLib/source.swift",
             "/Foo/Sources/Foo/source.swift",
             "/Foo/Sources/Foo/SomeFile.abc"
         )
@@ -67,9 +68,14 @@ final class PluginInvocationTests: XCTestCase {
                         ),
                         TargetDescription(
                             name: "FooTool",
-                            dependencies: [],
+                            dependencies: ["FooToolLib"],
                             type: .executable
                         ),
+                        TargetDescription(
+                            name: "FooToolLib",
+                            dependencies: [],
+                            type: .regular
+                        ),
                     ]
                 )
             ],
@@ -80,18 +86,24 @@ final class PluginInvocationTests: XCTestCase {
         XCTAssertNoDiagnostics(observability.diagnostics)
         PackageGraphTester(graph) { graph in
             graph.check(packages: "Foo")
-            // "FooTool" duplicated as it's present for both build tools and end products triples.
-            graph.check(targets: "Foo", "FooPlugin", "FooTool", "FooTool")
+            // "FooTool{Lib}" duplicated as it's present for both build tools and end products triples.
+            graph.check(targets: "Foo", "FooPlugin", "FooTool", "FooTool", "FooToolLib", "FooToolLib")
             graph.checkTarget("Foo") { target in
                 target.check(dependencies: "FooPlugin")
             }
-            graph.checkTarget("FooPlugin") { target in
+            graph.checkTarget("FooPlugin", destination: .tools) { target in
                 target.check(type: .plugin)
                 target.check(dependencies: "FooTool")
             }
-            graph.checkTargets("FooTool") { targets in
-                for target in targets {
+            for destination: BuildTriple in [.tools, .destination] {
+                graph.checkTarget("FooTool", destination: destination) { target in
                     target.check(type: .executable)
+                    target.check(buildTriple: destination)
+                    target.checkDependency("FooToolLib") { dependency in
+                        dependency.checkTarget {
+                            $0.check(buildTriple: destination)
+                        }
+                    }
                 }
             }
         }

Tests/SourceKitLSPAPITests/SourceKitLSPAPITests.swift

@@ -92,7 +92,7 @@ extension SourceKitLSPAPI.BuildDescription {
         partialArguments: [String],
         isPartOfRootPackage: Bool
     ) throws -> Bool {
-        let target = try XCTUnwrap(graph.target(for: targetName))
+        let target = try XCTUnwrap(graph.target(for: targetName, destination: .destination))
         let buildTarget = try XCTUnwrap(self.getBuildTarget(for: target, in: graph))
 
         guard let file = buildTarget.sources.first else {