[6.0][Build/PackageGraph] Switch BuildSubset.{product, target} and …

…`Modu… (#7668)

…lesGraph.{product, target}(...)` to use optional destination

- Explanation:

  Follow-up to #7646
  
This is mostly NFC change. It makes more sense to default `destination`
to "undefined" when applicable because it's not always possible to know
intended destination based on user input.

This kind of design makes more sense for internal APIs, instead of
having users to pass `.destination` even though it might not be always
correct.

- Main Branch PRs:
#7650

- Risk: Very Low

- Reviewed By: @MaxDesiatov 

- Testing: Existing test-cases were modified and new tests were added.

Sources/Build/BuildOperation.swift

@@ -516,7 +516,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
     }
 
     /// Compute the llbuild target name using the given subset.
-    private func computeLLBuildTargetName(for subset: BuildSubset) throws -> String {
+    func computeLLBuildTargetName(for subset: BuildSubset) throws -> String {
         switch subset {
         case .allExcludingTests:
             return LLBuildManifestBuilder.TargetKind.main.targetName
@@ -526,9 +526,15 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
             // FIXME: This is super unfortunate that we might need to load the package graph.
             let graph = try getPackageGraph()
 
+            let buildTriple: BuildTriple? = if let destination {
+                destination == .host ? .tools : .destination
+            } else {
+                nil
+            }
+
             let product = graph.product(
                 for: productName,
-                destination: destination == .host ? .tools : .destination
+                destination: buildTriple
             )
 
             guard let product else {
@@ -556,9 +562,15 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
             // FIXME: This is super unfortunate that we might need to load the package graph.
             let graph = try getPackageGraph()
 
+            let buildTriple: BuildTriple? = if let destination {
+                destination == .host ? .tools : .destination
+            } else {
+                nil
+            }
+
             let target = graph.target(
                 for: targetName,
-                destination: destination == .host ? .tools : .destination
+                destination: buildTriple
             )
 
             guard let target else {
@@ -669,7 +681,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
         // Emit warnings about any unhandled files in authored packages. We do this after applying build tool plugins, once we know what files they handled.
         // rdar://113256834 This fix works for the plugins that do not have PreBuildCommands.
         let targetsToConsider: [ResolvedModule]
-        if let subset = subset, let recursiveDependencies = try 
+        if let subset = subset, let recursiveDependencies = try
             subset.recursiveDependencies(for: graph, observabilityScope: observabilityScope) {
             targetsToConsider = recursiveDependencies
         } else {
@@ -939,18 +951,30 @@ extension BuildSubset {
         case .allExcludingTests:
             return graph.reachableTargets.filter { $0.type != .test }
         case .product(let productName, let destination):
+            let buildTriple: BuildTriple? = if let destination {
+                destination == .host ? .tools : .destination
+            } else {
+                nil
+            }
+
             guard let product = graph.product(
                 for: productName,
-                destination: destination == .host ? .tools : .destination
+                destination: buildTriple
             ) else {
                 observabilityScope.emit(error: "no product named '\(productName)'")
                 return nil
             }
             return try product.recursiveTargetDependencies()
         case .target(let targetName, let destination):
+            let buildTriple: BuildTriple? = if let destination {
+                destination == .host ? .tools : .destination
+            } else {
+                nil
+            }
+
             guard let target = graph.target(
                 for: targetName,
-                destination: destination == .host ? .tools : .destination
+                destination: buildTriple
             ) else {
                 observabilityScope.emit(error: "no target named '\(targetName)'")
                 return nil

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, destination: .destination) else {
+        guard let target = packageGraph.target(for: targetName) else {
             throw StringError("could not find a target named “\(targetName)”")
         }
 

Sources/PackageGraph/ModulesGraph.swift

@@ -138,55 +138,59 @@ public struct ModulesGraph {
         package.dependencies.compactMap { self.package(for: $0) }
     }
 
-    public func product(for name: String, destination: BuildTriple) -> ResolvedProduct? {
+    /// Find a product given a name and an optional destination. If a destination is not specified
+    /// this method uses `.destination` and falls back to `.tools` for macros, plugins, and tests.
+    public func product(for name: String, destination: BuildTriple? = .none) -> ResolvedProduct? {
         func findProduct(name: String, destination: BuildTriple) -> ResolvedProduct? {
             self.allProducts.first { $0.name == name && $0.buildTriple == destination }
         }
 
-        if let product = findProduct(name: name, destination: destination) {
-            return product
+        if let destination {
+            return findProduct(name: name, destination: destination)
         }
 
-        // FIXME: This is a temporary workaround and needs to be handled by the callers.
+        if let product = findProduct(name: name, destination: .destination) {
+            return product
+        }
 
         // It's possible to request a build of a macro, a plugin, or a test via `swift build`
         // which won't have the right destination set because it's impossible to indicate it.
         //
         // Same happens with `--test-product` - if one of the test targets directly references
         // a macro then all if its targets and the product itself become `host`.
-        if destination == .destination {
-            if let toolsProduct = findProduct(name: name, destination: .tools),
-                toolsProduct.type == .macro || toolsProduct.type == .plugin || toolsProduct.type == .test
-            {
-                return toolsProduct
-            }
+        if let toolsProduct = findProduct(name: name, destination: .tools),
+            toolsProduct.type == .macro || toolsProduct.type == .plugin || toolsProduct.type == .test
+        {
+            return toolsProduct
         }
 
         return nil
     }
 
-    public func target(for name: String, destination: BuildTriple) -> ResolvedModule? {
+    /// Find a target given a name and an optional destination. If a destination is not specified
+    /// this method uses `.destination` and falls back to `.tools` for macros, plugins, and tests.
+    public func target(for name: String, destination: BuildTriple? = .none) -> ResolvedModule? {
         func findModule(name: String, destination: BuildTriple) -> ResolvedModule? {
             self.allTargets.first { $0.name == name && $0.buildTriple == destination }
         }
 
-        if let module = findModule(name: name, destination: destination) {
-            return module
+        if let destination {
+            return findModule(name: name, destination: destination)
         }
 
-        // FIXME: This is a temporary workaround and needs to be handled by the callers.
+        if let module = findModule(name: name, destination: .destination) {
+            return module
+        }
 
         // It's possible to request a build of a macro, a plugin or a test via `swift build`
         // which won't have the right destination set because it's impossible to indicate it.
         //
         // Same happens with `--test-product` - if one of the test targets directly references
         // a macro then all if its targets and the product itself become `host`.
-        if destination == .destination {
-            if let toolsModule = findModule(name: name, destination: .tools),
-                toolsModule.type == .macro || toolsModule.type == .plugin || toolsModule.type == .test
-            {
-                return toolsModule
-            }
+        if let toolsModule = findModule(name: name, destination: .tools),
+            toolsModule.type == .macro || toolsModule.type == .plugin || toolsModule.type == .test
+        {
+            return toolsModule
         }
 
         return nil

Sources/SPMBuildCore/BuildSystem/BuildSystem.swift

@@ -24,11 +24,13 @@ public enum BuildSubset {
     /// Represents the subset of all products and targets.
     case allIncludingTests
 
-    /// Represents a specific product.
-    case product(String, for: BuildParameters.Destination = .target)
+    /// Represents a specific product. Allows to set a specific
+    /// destination if it's known.
+    case product(String, for: BuildParameters.Destination? = .none)
 
-    /// Represents a specific target.
-    case target(String, for: BuildParameters.Destination = .target)
+    /// Represents a specific target. Allows to set a specific
+    /// destination if it's known.
+    case target(String, for: BuildParameters.Destination? = .none)
 }
 
 /// A protocol that represents a build system used by SwiftPM for all build operations. This allows factoring out the

Sources/SPMTestSupport/MockPackageGraphs.swift

@@ -127,9 +127,11 @@ package func macrosPackageGraph() throws -> MockPackageGraph {
 
 package func macrosTestsPackageGraph() throws -> MockPackageGraph {
     let fs = InMemoryFileSystem(emptyFiles:
+        "/swift-mmio/Plugins/MMIOPlugin/source.swift",
         "/swift-mmio/Sources/MMIO/source.swift",
         "/swift-mmio/Sources/MMIOMacros/source.swift",
         "/swift-mmio/Sources/MMIOMacrosTests/source.swift",
+        "/swift-mmio/Sources/MMIOMacro+PluginTests/source.swift",
         "/swift-syntax/Sources/SwiftSyntax/source.swift",
         "/swift-syntax/Sources/SwiftSyntaxMacrosTestSupport/source.swift",
         "/swift-syntax/Sources/SwiftSyntaxMacros/source.swift",
@@ -156,6 +158,11 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph {
                         name: "MMIO",
                         type: .library(.automatic),
                         targets: ["MMIO"]
+                    ),
+                    ProductDescription(
+                        name: "MMIOPlugin",
+                        type: .plugin,
+                        targets: ["MMIOPlugin"]
                     )
                 ],
                 targets: [
@@ -171,13 +178,26 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph {
                         ],
                         type: .macro
                     ),
+                    TargetDescription(
+                        name: "MMIOPlugin",
+                        type: .plugin,
+                        pluginCapability: .buildTool
+                    ),
                     TargetDescription(
                         name: "MMIOMacrosTests",
                         dependencies: [
                             .target(name: "MMIOMacros"),
                             .product(name: "SwiftSyntaxMacrosTestSupport", package: "swift-syntax")
                         ],
                         type: .test
+                    ),
+                    TargetDescription(
+                        name: "MMIOMacro+PluginTests",
+                        dependencies: [
+                            .target(name: "MMIOPlugin"),
+                            .target(name: "MMIOMacros")
+                        ],
+                        type: .test
                     )
                 ]
             ),

Sources/SPMTestSupport/PackageGraphTester.swift

@@ -88,7 +88,7 @@ public final class PackageGraphResult {
 
     package func checkTarget(
         _ name: String,
-        destination: BuildTriple = .destination,
+        destination: BuildTriple? = .none,
         file: StaticString = #file,
         line: UInt = #line,
         body: (ResolvedTargetResult) -> Void
@@ -113,7 +113,7 @@ public final class PackageGraphResult {
 
     public func checkProduct(
         _ name: String,
-        destination: BuildTriple = .destination,
+        destination: BuildTriple? = .none,
         file: StaticString = #file,
         line: UInt = #line,
         body: (ResolvedProductResult) -> Void

Tests/BuildTests/BuildOperationTests.swift

@@ -18,6 +18,7 @@ import LLBuildManifest
 @_spi(DontAdoptOutsideOfSwiftPMExposedForBenchmarksAndTestsOnly)
 import PackageGraph
 import SPMBuildCore
+@_spi(SwiftPMInternal)
 import SPMTestSupport
 import XCTest
 
@@ -176,4 +177,41 @@ final class BuildOperationTests: XCTestCase {
             }
         }
     }
+
+    func testHostProductsAndTargetsWithoutExplicitDestination() throws {
+        let mock  = try macrosTestsPackageGraph()
+
+        let op = mockBuildOperation(
+            productsBuildParameters: mockBuildParameters(destination: .target),
+            toolsBuildParameters: mockBuildParameters(destination: .host),
+            packageGraphLoader: { mock.graph },
+            scratchDirectory: AbsolutePath("/.build/\(hostTriple)"),
+            fs: mock.fileSystem,
+            observabilityScope: mock.observabilityScope
+        )
+
+        XCTAssertEqual(
+            "MMIOMacros-\(hostTriple)-debug-tool.exe",
+            try op.computeLLBuildTargetName(for: .product("MMIOMacros"))
+        )
+
+        for target in ["MMIOMacros", "MMIOPlugin", "MMIOMacrosTests", "MMIOMacro+PluginTests"] {
+            XCTAssertEqual(
+                "\(target)-\(hostTriple)-debug-tool.module",
+                try op.computeLLBuildTargetName(for: .target(target))
+            )
+        }
+
+        let dependencies = try BuildSubset.target("MMIOMacro+PluginTests").recursiveDependencies(
+            for: mock.graph,
+            observabilityScope: mock.observabilityScope
+        )
+
+        XCTAssertNotNil(dependencies)
+        XCTAssertTrue(dependencies!.count > 0)
+
+        for dependency in dependencies! {
+            XCTAssertEqual(dependency.buildTriple, .tools)
+        }
+    }
 }

Tests/BuildTests/CrossCompilationBuildPlanTests.swift

@@ -300,7 +300,7 @@ final class CrossCompilationBuildPlanTests: XCTestCase {
         )
         let result = try BuildPlanResult(plan: plan)
         result.checkProductsCount(2)
-        result.checkTargetsCount(15)
+        result.checkTargetsCount(16)
 
         XCTAssertTrue(try result.allTargets(named: "SwiftSyntax")
             .map { try $0.swiftTarget() }