Skip to content

Commit 4bb01f6

Browse files
authored
Introduce richer EnabledTrait model to handle thread-safe enabled traits computations (#9269)
This PR introduces the `EnabledTrait` model and associated infrastructure to properly track and manage trait enablement throughout the dependency resolution process. The new system replaces simple string-based trait tracking with a rich model that maintains provenance information about how and why each trait was enabled. Fixes #9286 ## Core Changes ### New `EnabledTrait` Model (EnabledTrait.swift) Introduces three main components for tracking enabled traits for packages: **EnabledTrait** - Represents an individual enabled trait with: - Trait name (used as the identifier) - Set of `Setter` values tracking how the trait was enabled (via command-line config, parent package, or another trait) - Support for unifying traits with the same name by merging their setter lists - it's important to note that this is usually done with an `EnabledTrait` for a specific package, and that many packages can define their own (unique) trait that shares a name with a trait from another package but are treated as fundamentally different instances. **EnabledTraits** - Collection wrapper around `IdentifiableSet<EnabledTrait>` that: - Automatically unifies traits with the same name when inserted - Provides convenient set operations (union, intersection) - Maintains enablement metadata while treating traits with the same name as equal (merges `Setters` for traits of the same name) **EnabledTraitsMap** - Thread-safe map storing enabled traits per package that: - Maps `PackageIdentity` to `EnabledTraits`, while maintaining a thread-safe storage box of its properties to accommodate for loading manifests in parallel - Implicitly returns `["default"]` for packages with no explicitly enabled traits - Uses union behaviour: setting traits for the same package multiple times accumulates all traits - Tracks whether default traits have been explicitly disabled by a parent package or trait configuration (set with `[]`). - Tracks parent packages that request `default` trait usage for a package dependency (whether it's been requested through explicitly listing `default` as a trait, or if a trait specification is omitted) The introduction of these models have also provided some API conveniences: **String Interoperability** - `EnabledTrait` and `EnabledTraits` provide seamless string integration: - `ExpressibleByStringLiteral` and `ExpressibleByArrayLiteral` for natural initialization of enabled traits and sets of enabled traits: `let enabledTrait: EnabledTrait = "foo"` or `let enabledTraits: EnabledTraits = ["foo", "bar"]` - Bidirectional string equality: `enabledTrait == "foo"` and `"foo" == enabledTrait` both work - `EnabledTraitConvertible` protocol allows APIs to accept string collections and auto-convert them to `EnabledTrait` **Collection Protocol Support** - `EnabledTraits` conforms to `Collection` with set-like operations (union, intersection, contains) and overloaded methods that accept either `Collection<String>` or `Collection<EnabledTrait>`, enabling flexible APIs that work with both types ## Refactored Trait Handling (`Manifest+Traits.swift`) - Replaced all `Set<String>` trait representations with richer `EnabledTraits` model - Updated validation methods to work with `EnabledTrait` instead of raw strings - Removed parentPackage parameters from validation (now tracked via `EnabledTrait.Setter`) - Improved type safety and metadata tracking throughout trait calculation ### `Workspace+Traits.swift` (new file) - Extracted trait-specific workspace operations - `updateEnabledTraits(for:)` method determines enabled traits for loaded manifests - `updateEnabledTraits(forDependency:_:)` handles dependency-specific trait propagation ## Bug Fixes ### Required Dependencies Calculation **Problem**: During PubGrub dependency resolution, enabled traits were being reset to defaults instead of propagating previously calculated values when creating `DependencyResolutionNode` instances, which relied on an accurate set of `enabledTraits`. **Impact**: Wrong trait sets were considered when evaluating required dependencies, leading to incorrect dependency graphs. **Solution**: Properly propagate enabled traits through the `nodes()` method for `PackageContainerConstraint`, which creates instances of `DependencyResolutionNode` from these constraints during PubGrub dependency resolution. ### IdentifiableSet Intersection **Problem**: The intersection method created an empty set instead of starting with self, causing loss of element data. **Solution**: Changed `var result = Self()` to `var result = self` in `IdentifiableSet.swift:86`, ensuring proper preservation of element data during intersections. ### Test Improvements - Separated traits tests into dedicated files: - `ModulesGraphTests+Traits.swift` - `WorkspaceTests+Traits.swift` - `EnabledTraitTests.swift` ## Additional Improvements - Enhanced `IdentifiableSet` with `remove(id:)` and `remove(_:)` methods - Added `Workspace+Traits.swift` for trait-specific workspace operations - Updated all package container implementations for proper trait handling
1 parent 4e878fa commit 4bb01f6

37 files changed

+4265
-2109
lines changed

Sources/Basics/Collections/IdentifiableSet.swift

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public struct IdentifiableSet<Element: Identifiable>: Collection {
8383

8484
public func intersection(_ otherSequence: some Sequence<Element>) -> Self {
8585
let keysToRemove = Set(self.storage.keys).subtracting(otherSequence.map(\.id))
86-
var result = Self()
86+
var result = self
8787
for key in keysToRemove {
8888
result.storage.removeValue(forKey: key)
8989
}
@@ -101,6 +101,14 @@ public struct IdentifiableSet<Element: Identifiable>: Collection {
101101
public func contains(id: Element.ID) -> Bool {
102102
self.storage.keys.contains(id)
103103
}
104+
105+
public mutating func remove(id: Element.ID) -> Element? {
106+
self.storage.removeValue(forKey: id)
107+
}
108+
109+
public mutating func remove(_ member: Element) -> Element? {
110+
self.storage.removeValue(forKey: member.id)
111+
}
104112
}
105113

106114
extension OrderedDictionary where Value: Identifiable, Key == Value.ID {

Sources/PackageGraph/GraphLoadingNode.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,13 @@ public struct GraphLoadingNode: Equatable, Hashable {
3030
public let productFilter: ProductFilter
3131

3232
/// The enabled traits for this package.
33-
package var enabledTraits: Set<String>
33+
package var enabledTraits: EnabledTraits
3434

3535
public init(
3636
identity: PackageIdentity,
3737
manifest: Manifest,
3838
productFilter: ProductFilter,
39-
enabledTraits: Set<String>
39+
enabledTraits: EnabledTraits
4040
) throws {
4141
self.identity = identity
4242
self.manifest = manifest

Sources/PackageGraph/ModulesGraph+Loading.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,7 @@ private func createResolvedPackages(
399399
return ResolvedPackageBuilder(
400400
package,
401401
productFilter: node.productFilter,
402-
enabledTraits: node.enabledTraits /*?? []*/,
402+
enabledTraits: node.enabledTraits,
403403
isAllowedToVendUnsafeProducts: isAllowedToVendUnsafeProducts,
404404
allowedToOverride: allowedToOverride,
405405
platformVersionProvider: platformVersionProvider
@@ -1438,7 +1438,7 @@ private final class ResolvedPackageBuilder: ResolvedBuilder<ResolvedPackage> {
14381438
var products: [ResolvedProductBuilder] = []
14391439

14401440
/// The enabled traits of this package.
1441-
var enabledTraits: Set<String>
1441+
var enabledTraits: EnabledTraits
14421442

14431443
/// The dependencies of this package.
14441444
var dependencies: [ResolvedPackageBuilder] = []
@@ -1462,7 +1462,7 @@ private final class ResolvedPackageBuilder: ResolvedBuilder<ResolvedPackage> {
14621462
init(
14631463
_ package: Package,
14641464
productFilter: ProductFilter,
1465-
enabledTraits: Set<String>,
1465+
enabledTraits: EnabledTraits,
14661466
isAllowedToVendUnsafeProducts: Bool,
14671467
allowedToOverride: Bool,
14681468
platformVersionProvider: PlatformVersionProvider
@@ -1485,7 +1485,7 @@ private final class ResolvedPackageBuilder: ResolvedBuilder<ResolvedPackage> {
14851485
defaultLocalization: self.defaultLocalization,
14861486
supportedPlatforms: self.supportedPlatforms,
14871487
dependencies: self.dependencies.map(\.package.identity),
1488-
enabledTraits: self.enabledTraits,
1488+
enabledTraits: self.enabledTraits.names,
14891489
modules: modules,
14901490
products: products,
14911491
registryMetadata: self.registryMetadata,

Sources/PackageGraph/ModulesGraph.swift

Lines changed: 2 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,8 @@ public func loadModulesGraph(
466466
useXCBuildFileRules: Bool = false,
467467
customXCTestMinimumDeploymentTargets: [PackageModel.Platform: PlatformVersion]? = .none,
468468
observabilityScope: ObservabilityScope,
469-
traitConfiguration: TraitConfiguration = .default
469+
traitConfiguration: TraitConfiguration = .default,
470+
enabledTraitsMap: EnabledTraitsMap = .init()
470471
) throws -> ModulesGraph {
471472
let rootManifests = manifests.filter(\.packageKind.isRoot).spm_createDictionary { ($0.path, $0) }
472473
let externalManifests = try manifests.filter { !$0.packageKind.isRoot }
@@ -479,70 +480,6 @@ public func loadModulesGraph(
479480

480481
let packages = Array(rootManifests.keys)
481482

482-
let manifestMap = manifests.reduce(into: [PackageIdentity: Manifest]()) { manifestMap, manifest in
483-
manifestMap[manifest.packageIdentity] = manifest
484-
}
485-
486-
// Note: The following is a copy of the existing `Workspace.precomputeTraits` method
487-
func precomputeTraits(
488-
_ enabledTraitsMap: EnabledTraitsMap,
489-
_ topLevelManifests: [Manifest],
490-
_ manifestMap: [PackageIdentity: Manifest]
491-
) throws -> [PackageIdentity: Set<String>] {
492-
var visited: Set<PackageIdentity> = []
493-
var enabledTraitsMap = enabledTraitsMap
494-
495-
func dependencies(of parent: Manifest, _ productFilter: ProductFilter = .everything) throws {
496-
let parentTraits = enabledTraitsMap[parent.packageIdentity]
497-
let requiredDependencies = try parent.dependenciesRequired(for: productFilter, parentTraits)
498-
let guardedDependencies = parent.dependenciesTraitGuarded(withEnabledTraits: parentTraits)
499-
500-
_ = try (requiredDependencies + guardedDependencies).compactMap({ dependency in
501-
return try manifestMap[dependency.identity].flatMap({ manifest in
502-
503-
let explicitlyEnabledTraits = dependency.traits?.filter {
504-
guard let condition = $0.condition else { return true }
505-
return condition.isSatisfied(by: parentTraits)
506-
}.map(\.name)
507-
508-
if let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) {
509-
let calculatedTraits = try manifest.enabledTraits(
510-
using: enabledTraitsSet,
511-
.init(parent)
512-
)
513-
enabledTraitsMap[dependency.identity] = calculatedTraits
514-
}
515-
516-
let result = visited.insert(dependency.identity)
517-
if result.inserted {
518-
try dependencies(of: manifest, dependency.productFilter)
519-
}
520-
521-
return manifest
522-
})
523-
})
524-
}
525-
526-
for manifest in topLevelManifests {
527-
// Track already-visited manifests to avoid cycles
528-
let result = visited.insert(manifest.packageIdentity)
529-
if result.inserted {
530-
try dependencies(of: manifest)
531-
}
532-
}
533-
534-
return enabledTraitsMap.dictionaryLiteral
535-
}
536-
537-
538-
// Precompute enabled traits for roots.
539-
var enabledTraitsMap: EnabledTraitsMap = [:]
540-
for root in rootManifests.values {
541-
let enabledTraits = try root.enabledTraits(using: traitConfiguration)
542-
enabledTraitsMap[root.packageIdentity] = enabledTraits
543-
}
544-
enabledTraitsMap = .init(try precomputeTraits(enabledTraitsMap, manifests, manifestMap))
545-
546483
let input = PackageGraphRootInput(packages: packages, traitConfiguration: traitConfiguration)
547484
let graphRoot = try PackageGraphRoot(
548485
input: input,

Sources/PackageGraph/PackageContainer.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public protocol PackageContainer {
7575
/// - Precondition: `versions.contains(version)`
7676
/// - Throws: If the version could not be resolved; this will abort
7777
/// dependency resolution completely.
78-
func getDependencies(at version: Version, productFilter: ProductFilter, _ enabledTraits: Set<String>) async throws -> [PackageContainerConstraint]
78+
func getDependencies(at version: Version, productFilter: ProductFilter, _ enabledTraits: EnabledTraits) async throws -> [PackageContainerConstraint]
7979

8080
/// Fetch the declared dependencies for a particular revision.
8181
///
@@ -84,12 +84,12 @@ public protocol PackageContainer {
8484
///
8585
/// - Throws: If the revision could not be resolved; this will abort
8686
/// dependency resolution completely.
87-
func getDependencies(at revision: String, productFilter: ProductFilter, _ enabledTraits: Set<String>) async throws -> [PackageContainerConstraint]
87+
func getDependencies(at revision: String, productFilter: ProductFilter, _ enabledTraits: EnabledTraits) async throws -> [PackageContainerConstraint]
8888

8989
/// Fetch the dependencies of an unversioned package container.
9090
///
9191
/// NOTE: This method should not be called on a versioned container.
92-
func getUnversionedDependencies(productFilter: ProductFilter, _ enabledTraits: Set<String>) async throws -> [PackageContainerConstraint]
92+
func getUnversionedDependencies(productFilter: ProductFilter, _ enabledTraits: EnabledTraits) async throws -> [PackageContainerConstraint]
9393

9494
/// Get the updated identifier at a bound version.
9595
///
@@ -150,11 +150,11 @@ public struct PackageContainerConstraint: Equatable, Hashable {
150150
public let products: ProductFilter
151151

152152
/// The traits that have been enabled for the package.
153-
public let enabledTraits: Set<String>
153+
public let enabledTraits: EnabledTraits
154154

155155
/// Create a constraint requiring the given `container` satisfying the
156156
/// `requirement`.
157-
public init(package: PackageReference, requirement: PackageRequirement, products: ProductFilter, enabledTraits: Set<String> = ["default"]) {
157+
public init(package: PackageReference, requirement: PackageRequirement, products: ProductFilter, enabledTraits: EnabledTraits = ["default"]) {
158158
self.package = package
159159
self.requirement = requirement
160160
self.products = products
@@ -163,7 +163,7 @@ public struct PackageContainerConstraint: Equatable, Hashable {
163163

164164
/// Create a constraint requiring the given `container` satisfying the
165165
/// `versionRequirement`.
166-
public init(package: PackageReference, versionRequirement: VersionSetSpecifier, products: ProductFilter, enabledTraits: Set<String> = ["default"]) {
166+
public init(package: PackageReference, versionRequirement: VersionSetSpecifier, products: ProductFilter, enabledTraits: EnabledTraits = ["default"]) {
167167
self.init(package: package, requirement: .versionSet(versionRequirement), products: products, enabledTraits: enabledTraits)
168168
}
169169

Sources/PackageGraph/PackageGraphRoot.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ public struct PackageGraphRoot {
115115
// If not, then we can omit this dependency if pruning unused dependencies
116116
// is enabled.
117117
return manifests.values.reduce(false) { result, manifest in
118-
let enabledTraits: Set<String> = enabledTraitsMap[manifest.packageIdentity]
118+
let enabledTraits = enabledTraitsMap[manifest.packageIdentity]
119119
if let isUsed = try? manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) {
120120
return result || isUsed
121121
}
@@ -128,7 +128,7 @@ public struct PackageGraphRoot {
128128
// FIXME: `dependenciesRequired` modifies manifests and prevents conversion of `Manifest` to a value type
129129
let deps = try? manifests.values.lazy
130130
.map({ manifest -> [PackageDependency] in
131-
let enabledTraits: Set<String> = enabledTraitsMap[manifest.packageIdentity]
131+
let enabledTraits = enabledTraitsMap[manifest.packageIdentity]
132132
return try manifest.dependenciesRequired(for: .everything, enabledTraits)
133133
})
134134
.flatMap({ $0 })
@@ -145,7 +145,7 @@ public struct PackageGraphRoot {
145145

146146
/// Returns the constraints imposed by root manifests + dependencies.
147147
public func constraints(_ enabledTraitsMap: EnabledTraitsMap) throws -> [PackageContainerConstraint] {
148-
var rootEnabledTraits: Set<String> = []
148+
var rootEnabledTraits: EnabledTraits = []
149149
let constraints = self.packages.map { (identity, package) in
150150
let enabledTraits = enabledTraitsMap[identity]
151151
rootEnabledTraits.formUnion(enabledTraits)
@@ -161,11 +161,11 @@ public struct PackageGraphRoot {
161161
.map { dep in
162162
let enabledTraits = dep.traits?.filter {
163163
guard let condition = $0.condition else { return true }
164-
return condition.isSatisfied(by: rootEnabledTraits)
165-
}.map(\.name)
164+
return condition.isSatisfied(by: rootEnabledTraits.names)
165+
}.map({ EnabledTrait(name: $0.name, setBy: .package(.init(identity: "root"))) })
166166

167167
var enabledTraitsSet = enabledTraitsMap[dep.identity]
168-
enabledTraitsSet.formUnion(enabledTraits.flatMap({ Set($0) }) ?? [])
168+
enabledTraitsSet.formUnion(enabledTraits ?? [])
169169

170170
return PackageContainerConstraint(
171171
package: dep.packageRef,

Sources/PackageGraph/PackageModel+Extensions.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,14 +35,14 @@ extension PackageDependency {
3535

3636
extension Manifest {
3737
/// Constructs constraints of the dependencies in the raw package.
38-
public func dependencyConstraints(productFilter: ProductFilter, _ enabledTraits: Set<String> = ["default"]) throws -> [PackageContainerConstraint] {
38+
public func dependencyConstraints(productFilter: ProductFilter, _ enabledTraits: EnabledTraits = ["default"]) throws -> [PackageContainerConstraint] {
3939
return try self.dependenciesRequired(for: productFilter, enabledTraits).map({
4040
let explicitlyEnabledTraits = $0.traits?.filter {
4141
guard let condition = $0.condition else { return true }
42-
return condition.isSatisfied(by: enabledTraits)
42+
return condition.isSatisfied(by: enabledTraits.names)
4343
}.map(\.name)
4444

45-
let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) ?? ["default"]
45+
let enabledTraitsSet = EnabledTraits(explicitlyEnabledTraits ?? [], setBy: .package(.init(identity: self.packageIdentity, name: self.displayName)))
4646

4747
return PackageContainerConstraint(
4848
package: $0.packageRef,
@@ -60,7 +60,7 @@ extension PackageContainerConstraint {
6060
internal func nodes() -> [DependencyResolutionNode] {
6161
switch products {
6262
case .everything:
63-
return [.root(package: self.package)]
63+
return [.root(package: self.package, enabledTraits: self.enabledTraits)]
6464
case .specific:
6565
switch products {
6666
case .everything:
@@ -70,7 +70,7 @@ extension PackageContainerConstraint {
7070
if set.isEmpty { // Pointing at the package without a particular product.
7171
return [.empty(package: self.package)]
7272
} else {
73-
return set.sorted().map { .product($0, package: self.package) }
73+
return set.sorted().map { .product($0, package: self.package, enabledTraits: self.enabledTraits) }
7474
}
7575
}
7676
}

Sources/PackageGraph/Resolution/DependencyResolutionNode.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public enum DependencyResolutionNode {
4444
/// Since a non‐existent product ends up with only its implicit dependency on its own package,
4545
/// only whichever package contains the product will end up adding additional constraints.
4646
/// See `ProductFilter` and `Manifest.register(...)`.
47-
case product(String, package: PackageReference, enabledTraits: Set<String> = ["default"])
47+
case product(String, package: PackageReference, enabledTraits: EnabledTraits = ["default"])
4848

4949
/// A root node.
5050
///
@@ -58,7 +58,7 @@ public enum DependencyResolutionNode {
5858
/// It is a warning condition, and builds do not actually need these dependencies.
5959
/// However, forcing the graph to resolve and fetch them anyway allows the diagnostics passes access
6060
/// to the information needed in order to provide actionable suggestions to help the user stitch up the dependency declarations properly.
61-
case root(package: PackageReference, enabledTraits: Set<String> = ["default"])
61+
case root(package: PackageReference, enabledTraits: EnabledTraits = ["default"])
6262

6363
/// The package.
6464
public var package: PackageReference {
@@ -91,7 +91,7 @@ public enum DependencyResolutionNode {
9191
}
9292

9393
/// Returns the enabled traits for this node's manifest.
94-
public var enabledTraits: Set<String> {
94+
public var enabledTraits: EnabledTraits {
9595
switch self {
9696
case .root(_, let enabledTraits), .product(_, _, let enabledTraits):
9797
return enabledTraits

Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,7 @@ public struct PubGrubDependencyResolver {
357357
}
358358

359359
for dependency in try await container.underlying
360-
.getUnversionedDependencies(productFilter: node.productFilter, constraint.enabledTraits)
360+
.getUnversionedDependencies(productFilter: node.productFilter, node.enabledTraits)
361361
{
362362
if let versionedBasedConstraints = VersionBasedConstraint.constraints(dependency) {
363363
for constraint in versionedBasedConstraints {
@@ -431,7 +431,7 @@ public struct PubGrubDependencyResolver {
431431
var unprocessedDependencies = try await container.underlying.getDependencies(
432432
at: revisionForDependencies,
433433
productFilter: constraint.products,
434-
constraint.enabledTraits
434+
node.enabledTraits
435435
)
436436
if let sharedRevision = node.revisionLock(revision: revision) {
437437
unprocessedDependencies.append(sharedRevision)

Sources/PackageLoading/PackageBuilder.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ public final class PackageBuilder {
388388
private var swiftVersionCache: SwiftLanguageVersion? = nil
389389

390390
/// The enabled traits of this package.
391-
private let enabledTraits: Set<String>
391+
private let enabledTraits: EnabledTraits
392392

393393
/// Create a builder for the given manifest and package `path`.
394394
///
@@ -414,7 +414,7 @@ public final class PackageBuilder {
414414
createREPLProduct: Bool = false,
415415
fileSystem: FileSystem,
416416
observabilityScope: ObservabilityScope,
417-
enabledTraits: Set<String>
417+
enabledTraits: EnabledTraits
418418
) {
419419
self.identity = identity
420420
self.manifest = manifest
@@ -1151,7 +1151,7 @@ public final class PackageBuilder {
11511151

11521152
// Process each setting.
11531153
for setting in target.settings {
1154-
if let traits = setting.condition?.traits, traits.intersection(self.enabledTraits).isEmpty {
1154+
if let traits = setting.condition?.traits, traits.intersection(self.enabledTraits.names).isEmpty {
11551155
// The setting is currently not enabled so we should skip it
11561156
continue
11571157
}

0 commit comments

Comments
 (0)