Skip to content

Commit b368b96

Browse files
authored
Make Resolved* in PackageGraph value types (#7160)
Depends on #7161. Unblocks macros cross-compilation fix in #7118. ### Motivation: Value types are easier to reason about as they prevent "spooky action at a distance" bugs, and can be easily made `Equatable`, `Hashable`, and most important of all `Sendable`. ### Modifications: Converted `ResolvedTarget`, `ResolvedProduct`, and `ResolvedPackage` from `final class`es to `struct`s. Renamed `underlyingTarget`, `underlyingProduct`, and `underlyingPackage` to just `underlying` to avoid tautological patterns like `target.underlyingTarget`. ### Result: It's easier to apply changes in `PackageGraph`. Also, in my benchmarks done locally this makes package graph resolution consistently ~5% faster when resolving the package graph of SwiftPM's own `Package.swift`. #### `main` branch: ``` ╒══════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕ │ Metric │ p0 │ p25 │ p50 │ p75 │ p90 │ p99 │ p100 │ Samples │ ╞══════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡ │ Time (system CPU) (ms) │ 7 │ 8 │ 11 │ 14 │ 15 │ 18 │ 18 │ 25 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (total CPU) (ms) │ 12 │ 13 │ 16 │ 19 │ 20 │ 23 │ 23 │ 25 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (user CPU) (μs) │ 4688 │ 4857 │ 4890 │ 4956 │ 5107 │ 5347 │ 5347 │ 25 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (wall clock) (ms) │ 390 │ 397 │ 402 │ 409 │ 414 │ 420 │ 420 │ 25 │ ╘══════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛ ``` #### This PR's branch: ``` ╒══════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕ │ Metric │ p0 │ p25 │ p50 │ p75 │ p90 │ p99 │ p100 │ Samples │ ╞══════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡ │ Time (system CPU) (ms) │ 6 │ 8 │ 10 │ 13 │ 16 │ 17 │ 17 │ 26 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (total CPU) (ms) │ 11 │ 13 │ 15 │ 18 │ 20 │ 21 │ 21 │ 26 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (user CPU) (μs) │ 4476 │ 4567 │ 4648 │ 4796 │ 4931 │ 4998 │ 4998 │ 26 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (wall clock) (ms) │ 375 │ 379 │ 384 │ 391 │ 394 │ 398 │ 398 │ 26 │ ╘══════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛ ``` ### Future directions: I'm unable to fully remove `Resolved*Builder` classes yet, as they rely on `Target` being a reference type and module aliasing relying on side effects of modifying instances of `Target`. In turn, `Target` is a class hierarchy, which we should convert to multiple value types that use composition in the future. In the meantime this also prevents us from making it, and all types that contain it, `Sendable`.
1 parent b86d1d4 commit b368b96

File tree

51 files changed

+644
-426
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

51 files changed

+644
-426
lines changed

Sources/Build/BuildDescription/ClangTargetBuildDescription.swift

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import Basics
1414
import PackageLoading
1515
import PackageModel
16-
import class PackageGraph.ResolvedTarget
16+
import struct PackageGraph.ResolvedTarget
1717
import struct SPMBuildCore.BuildParameters
1818
import struct SPMBuildCore.BuildToolPluginInvocationResult
1919
import struct SPMBuildCore.PrebuildCommandResult
@@ -26,9 +26,7 @@ public final class ClangTargetBuildDescription {
2626
public let target: ResolvedTarget
2727

2828
/// The underlying clang target.
29-
public var clangTarget: ClangTarget {
30-
target.underlyingTarget as! ClangTarget
31-
}
29+
public let clangTarget: ClangTarget
3230

3331
/// The tools version of the package that declared the target. This can
3432
/// can be used to conditionalize semantically significant changes in how
@@ -45,7 +43,7 @@ public final class ClangTargetBuildDescription {
4543

4644
/// The list of all resource files in the target, including the derived ones.
4745
public var resources: [Resource] {
48-
self.target.underlyingTarget.resources + self.pluginDerivedResources
46+
self.target.underlying.resources + self.pluginDerivedResources
4947
}
5048

5149
/// Path to the bundle generated for this module (if any).
@@ -54,7 +52,7 @@ public final class ClangTargetBuildDescription {
5452
return .none
5553
}
5654

57-
if let bundleName = target.underlyingTarget.potentialBundleName {
55+
if let bundleName = target.underlying.potentialBundleName {
5856
return self.buildParameters.bundlePath(named: bundleName)
5957
} else {
6058
return .none
@@ -119,10 +117,11 @@ public final class ClangTargetBuildDescription {
119117
fileSystem: FileSystem,
120118
observabilityScope: ObservabilityScope
121119
) throws {
122-
guard target.underlyingTarget is ClangTarget else {
120+
guard let clangTarget = target.underlying as? ClangTarget else {
123121
throw InternalError("underlying target type mismatch \(target)")
124122
}
125123

124+
self.clangTarget = clangTarget
126125
self.fileSystem = fileSystem
127126
self.target = target
128127
self.toolsVersion = toolsVersion

Sources/Build/BuildDescription/PluginDescription.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public final class PluginDescription: Codable {
5050
testDiscoveryTarget: Bool = false,
5151
fileSystem: FileSystem
5252
) throws {
53-
guard target.underlyingTarget is PluginTarget else {
53+
guard target.underlying is PluginTarget else {
5454
throw InternalError("underlying target type mismatch \(target)")
5555
}
5656

Sources/Build/BuildDescription/ProductBuildDescription.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
236236
// Support for linking tests against executables is conditional on the tools
237237
// version of the package that defines the executable product.
238238
let executableTarget = try product.executableTarget
239-
if let target = executableTarget.underlyingTarget as? SwiftTarget,
239+
if let target = executableTarget.underlying as? SwiftTarget,
240240
self.toolsVersion >= .v5_5,
241241
self.buildParameters.driverParameters.canRenameEntrypointFunctionName,
242242
target.supportsTestableExecutablesFeature

Sources/Build/BuildDescription/SharedTargetBuildDescription.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ struct SharedTargetBuildDescription {
5353
additionalFileRules: additionalFileRules,
5454
defaultLocalization: target.defaultLocalization,
5555
targetName: target.name,
56-
targetPath: target.underlyingTarget.path,
56+
targetPath: target.underlying.path,
5757
observabilityScope: observabilityScope
5858
)
5959
let pluginDerivedResources = derivedResources

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ public final class SwiftTargetBuildDescription {
3333
/// The target described by this target.
3434
public let target: ResolvedTarget
3535

36+
private let swiftTarget: SwiftTarget
37+
3638
/// The tools version of the package that declared the target. This can
3739
/// can be used to conditionalize semantically significant changes in how
3840
/// a target is built.
@@ -57,7 +59,7 @@ public final class SwiftTargetBuildDescription {
5759

5860
/// Path to the bundle generated for this module (if any).
5961
var bundlePath: AbsolutePath? {
60-
if let bundleName = target.underlyingTarget.potentialBundleName, needsResourceBundle {
62+
if let bundleName = target.underlying.potentialBundleName, needsResourceBundle {
6163
return self.buildParameters.bundlePath(named: bundleName)
6264
} else {
6365
return .none
@@ -83,7 +85,7 @@ public final class SwiftTargetBuildDescription {
8385

8486
/// The list of all resource files in the target, including the derived ones.
8587
public var resources: [Resource] {
86-
self.target.underlyingTarget.resources + self.pluginDerivedResources
88+
self.target.underlying.resources + self.pluginDerivedResources
8789
}
8890

8991
/// The objects in this target, containing either machine code or bitcode
@@ -139,7 +141,7 @@ public final class SwiftTargetBuildDescription {
139141

140142
/// The swift version for this target.
141143
var swiftVersion: SwiftLanguageVersion {
142-
(self.target.underlyingTarget as! SwiftTarget).swiftVersion
144+
self.swiftTarget.swiftVersion
143145
}
144146

145147
/// Describes the purpose of a test target, including any special roles such as containing a list of discovered
@@ -257,10 +259,11 @@ public final class SwiftTargetBuildDescription {
257259
fileSystem: FileSystem,
258260
observabilityScope: ObservabilityScope
259261
) throws {
260-
guard target.underlyingTarget is SwiftTarget else {
262+
guard let swiftTarget = target.underlying as? SwiftTarget else {
261263
throw InternalError("underlying target type mismatch \(target)")
262264
}
263265

266+
self.swiftTarget = swiftTarget
264267
self.package = package
265268
self.target = target
266269
self.toolsVersion = toolsVersion
@@ -514,7 +517,7 @@ public final class SwiftTargetBuildDescription {
514517
// when we link the executable, we will ask the linker to rename the entry point
515518
// symbol to just `_main` again (or if the linker doesn't support it, we'll
516519
// generate a source containing a redirect).
517-
if (self.target.underlyingTarget as? SwiftTarget)?.supportsTestableExecutablesFeature == true
520+
if (self.target.underlying as? SwiftTarget)?.supportsTestableExecutablesFeature == true
518521
&& !self.isTestTarget && self.toolsVersion >= .v5_5
519522
{
520523
// We only do this if the linker supports it, as indicated by whether we

Sources/Build/BuildDescription/TargetBuildDescription.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@
1111
//===----------------------------------------------------------------------===//
1212

1313
import Basics
14-
import struct SPMBuildCore.BuildParameters
15-
import class PackageGraph.ResolvedTarget
14+
import struct PackageGraph.ResolvedTarget
1615
import struct PackageModel.Resource
1716
import struct SPMBuildCore.BuildToolPluginInvocationResult
17+
import struct SPMBuildCore.BuildParameters
1818

1919
public enum BuildDescriptionError: Swift.Error {
2020
case requestedFileNotPartOfTarget(targetName: String, requestedFilePath: AbsolutePath)

Sources/Build/BuildManifest/LLBuildManifestBuilder+Clang.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import struct LLBuildManifest.Node
1414
import struct Basics.AbsolutePath
1515
import struct Basics.InternalError
16-
import class PackageGraph.ResolvedTarget
16+
import struct PackageGraph.ResolvedTarget
1717
import PackageModel
1818

1919
extension LLBuildManifestBuilder {

Sources/Build/BuildManifest/LLBuildManifestBuilder+Swift.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import struct Basics.TSCAbsolutePath
1717
import struct LLBuildManifest.Node
1818
import struct LLBuildManifest.LLBuildManifest
1919
import struct SPMBuildCore.BuildParameters
20-
import class PackageGraph.ResolvedTarget
20+
import struct PackageGraph.ResolvedTarget
2121
import protocol TSCBasic.FileSystem
2222
import enum TSCBasic.ProcessEnv
2323
import func TSCBasic.topologicalSort
@@ -212,8 +212,8 @@ extension LLBuildManifestBuilder {
212212
// product into its constituent targets.
213213
continue
214214
}
215-
guard target.underlyingTarget.type != .systemModule,
216-
target.underlyingTarget.type != .binary
215+
guard target.underlying.type != .systemModule,
216+
target.underlying.type != .binary
217217
else {
218218
// Much like non-Swift targets, system modules will consist of a modulemap
219219
// somewhere in the filesystem, with the path to that module being either
@@ -416,11 +416,11 @@ extension LLBuildManifestBuilder {
416416

417417
func addStaticTargetInputs(_ target: ResolvedTarget) throws {
418418
// Ignore C Modules.
419-
if target.underlyingTarget is SystemLibraryTarget { return }
419+
if target.underlying is SystemLibraryTarget { return }
420420
// Ignore Binary Modules.
421-
if target.underlyingTarget is BinaryTarget { return }
421+
if target.underlying is BinaryTarget { return }
422422
// Ignore Plugin Targets.
423-
if target.underlyingTarget is PluginTarget { return }
423+
if target.underlying is PluginTarget { return }
424424

425425
// Depend on the binary for executable targets.
426426
if target.type == .executable {

Sources/Build/BuildOperation.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,7 +511,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
511511
for package in graph.rootPackages where package.manifest.toolsVersion >= .v5_3 {
512512
for target in package.targets {
513513
// Get the set of unhandled files in targets.
514-
var unhandledFiles = Set(target.underlyingTarget.others)
514+
var unhandledFiles = Set(target.underlying.others)
515515
if unhandledFiles.isEmpty { continue }
516516

517517
// Subtract out any that were inputs to any commands generated by plugins.

Sources/Build/BuildPlan/BuildPlan+Clang.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ extension BuildPlan {
2121
let dependencies = try clangTarget.target.recursiveDependencies(satisfying: clangTarget.buildEnvironment)
2222

2323
for case .target(let dependency, _) in dependencies {
24-
switch dependency.underlyingTarget {
24+
switch dependency.underlying {
2525
case is SwiftTarget:
2626
if case let .swift(dependencyTargetDescription)? = targetMap[dependency] {
2727
if let moduleMap = dependencyTargetDescription.moduleMap {

Sources/Build/BuildPlan/BuildPlan+Product.swift

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@
1313
import struct Basics.AbsolutePath
1414
import struct Basics.Triple
1515
import struct Basics.InternalError
16-
import class PackageGraph.ResolvedProduct
17-
import class PackageGraph.ResolvedTarget
16+
import struct PackageGraph.ResolvedProduct
17+
import struct PackageGraph.ResolvedTarget
1818
import class PackageModel.BinaryTarget
1919
import class PackageModel.ClangTarget
2020
import class PackageModel.Target
@@ -32,7 +32,7 @@ extension BuildPlan {
3232

3333
// Add flags for system targets.
3434
for systemModule in dependencies.systemModules {
35-
guard case let target as SystemLibraryTarget = systemModule.underlyingTarget else {
35+
guard case let target as SystemLibraryTarget = systemModule.underlying else {
3636
throw InternalError("This should not be possible.")
3737
}
3838
// Add pkgConfig libs arguments.
@@ -53,7 +53,7 @@ extension BuildPlan {
5353
// Link C++ if needed.
5454
// Note: This will come from build settings in future.
5555
for target in dependencies.staticTargets {
56-
if case let target as ClangTarget = target.underlyingTarget, target.isCXX {
56+
if case let target as ClangTarget = target.underlying, target.isCXX {
5757
let triple = buildProduct.buildParameters.triple
5858
if triple.isDarwin() {
5959
buildProduct.additionalFlags += ["-lc++"]
@@ -67,7 +67,7 @@ extension BuildPlan {
6767
}
6868

6969
for target in dependencies.staticTargets {
70-
switch target.underlyingTarget {
70+
switch target.underlying {
7171
case is SwiftTarget:
7272
// Swift targets are guaranteed to have a corresponding Swift description.
7373
guard case .swift(let description) = targetMap[target] else {
@@ -131,7 +131,7 @@ extension BuildPlan {
131131
// For test targets, we need to consider the first level of transitive dependencies since the first level is always test targets.
132132
let topLevelDependencies: [PackageModel.Target]
133133
if product.type == .test {
134-
topLevelDependencies = product.targets.flatMap { $0.underlyingTarget.dependencies }.compactMap {
134+
topLevelDependencies = product.targets.flatMap { $0.underlying.dependencies }.compactMap {
135135
switch $0 {
136136
case .product:
137137
return nil
@@ -149,7 +149,7 @@ extension BuildPlan {
149149
switch dependency {
150150
// Include all the dependencies of a target.
151151
case .target(let target, _):
152-
let isTopLevel = topLevelDependencies.contains(target.underlyingTarget) || product.targets.contains(target)
152+
let isTopLevel = topLevelDependencies.contains(target.underlying) || product.targets.contains(target)
153153
let topLevelIsMacro = isTopLevel && product.type == .macro
154154
let topLevelIsPlugin = isTopLevel && product.type == .plugin
155155
let topLevelIsTest = isTopLevel && product.type == .test
@@ -200,9 +200,9 @@ extension BuildPlan {
200200
case .executable, .snippet, .macro:
201201
if product.targets.contains(target) {
202202
staticTargets.append(target)
203-
} else if product.type == .test && (target.underlyingTarget as? SwiftTarget)?.supportsTestableExecutablesFeature == true {
203+
} else if product.type == .test && (target.underlying as? SwiftTarget)?.supportsTestableExecutablesFeature == true {
204204
// Only "top-level" targets should really be considered here, not transitive ones.
205-
let isTopLevel = topLevelDependencies.contains(target.underlyingTarget) || product.targets.contains(target)
205+
let isTopLevel = topLevelDependencies.contains(target.underlying) || product.targets.contains(target)
206206
if let toolsVersion = graph.package(for: product)?.manifest.toolsVersion, toolsVersion >= .v5_5, isTopLevel {
207207
staticTargets.append(target)
208208
}
@@ -220,7 +220,7 @@ extension BuildPlan {
220220
systemModules.append(target)
221221
// Add binary to binary paths set.
222222
case .binary:
223-
guard let binaryTarget = target.underlyingTarget as? BinaryTarget else {
223+
guard let binaryTarget = target.underlying as? BinaryTarget else {
224224
throw InternalError("invalid binary target '\(target.name)'")
225225
}
226226
switch binaryTarget.kind {

Sources/Build/BuildPlan/BuildPlan+Swift.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ extension BuildPlan {
2121
// depends on.
2222
let environment = swiftTarget.buildParameters.buildEnvironment
2323
for case .target(let dependency, _) in try swiftTarget.target.recursiveDependencies(satisfying: environment) {
24-
switch dependency.underlyingTarget {
24+
switch dependency.underlying {
2525
case let underlyingTarget as ClangTarget where underlyingTarget.type == .library:
2626
guard case let .clang(target)? = targetMap[dependency] else {
2727
throw InternalError("unexpected clang target \(underlyingTarget)")

0 commit comments

Comments
 (0)