[Traits] Change TraitConfiguration to be an enum instead of struct (#8370)

Fixes #8355.

Since the way a `TraitConfiguration` is set up can affect how traits are
calculated and whether default traits are overridden or not, it's more
ergonomic to change this to an `enum` that lists the various possible
cases of configuration:

- No trait configuration, which defaults to using default traits if
applicable
- A configuration with an empty set of traits that overrides the default
traits
 - A configuration that enables all traits
- A configuration that specifies an exact set of traits that are enabled

This way we can avoid doing multiple checks on the properties of the
`struct` and instead rely on the exact case that describes how to
continue calculating enabled traits.

The refactoring done as a part of this change also fixes #8356.

Author: bripeticca

Sources/Build/LLBuildDescription.swift

@@ -17,6 +17,8 @@ import SPMBuildCore
 import PackageGraph
 import PackageModel
 
+import enum PackageModel.TraitConfiguration
+
 import struct TSCBasic.ByteString
 
 /// Contains the description of the build that is needed during the execution.

Sources/Commands/PackageCommands/EditCommands.swift

@@ -37,7 +37,7 @@ extension SwiftPackageCommand {
         var packageIdentity: String
 
         func run(_ swiftCommandState: SwiftCommandState) async throws {
-            try await swiftCommandState.resolve(nil)
+            try await swiftCommandState.resolve()
             let workspace = try swiftCommandState.getActiveWorkspace()
 
             // Put the dependency in edit mode.
@@ -66,7 +66,7 @@ extension SwiftPackageCommand {
         var packageIdentity: String
 
         func run(_ swiftCommandState: SwiftCommandState) async throws {
-            try await swiftCommandState.resolve(nil)
+            try await swiftCommandState.resolve()
             let workspace = try swiftCommandState.getActiveWorkspace()
 
             try await workspace.unedit(

Sources/Commands/PackageCommands/Resolve.swift

@@ -16,7 +16,7 @@ import CoreCommands
 import TSCUtility
 import Workspace
 
-import struct PackageGraph.TraitConfiguration
+import enum PackageModel.TraitConfiguration
 
 extension SwiftPackageCommand {
     struct ResolveOptions: ParsableArguments {

Sources/CoreCommands/BuildSystemSupport.swift

@@ -22,6 +22,7 @@ import class Basics.ObservabilityScope
 import struct PackageGraph.ModulesGraph
 import struct PackageLoading.FileRuleDescription
 import protocol TSCBasic.OutputByteStream
+import enum PackageModel.TraitConfiguration
 
 private struct NativeBuildSystemFactory: BuildSystemFactory {
     let swiftCommandState: SwiftCommandState

Sources/CoreCommands/Options.swift

@@ -27,7 +27,7 @@ import struct PackageModel.PackageIdentity
 import enum PackageModel.Sanitizer
 @_spi(SwiftPMInternal) import struct PackageModel.SwiftSDK
 
-import struct PackageGraph.TraitConfiguration
+import enum PackageModel.TraitConfiguration
 
 import struct SPMBuildCore.BuildParameters
 import struct SPMBuildCore.BuildSystemProvider

Sources/CoreCommands/SwiftCommandState.swift

@@ -214,14 +214,14 @@ public final class SwiftCommandState {
     }
 
     /// Get the current workspace root object.
-    public func getWorkspaceRoot(traitConfiguration: TraitConfiguration? = nil) throws -> PackageGraphRootInput {
+    public func getWorkspaceRoot(traitConfiguration: TraitConfiguration = .default) throws -> PackageGraphRootInput {
         let packages: [AbsolutePath]
 
         if let workspace = options.locations.multirootPackageDataFile {
             packages = try self.workspaceLoaderProvider(self.fileSystem, self.observabilityScope)
                 .load(workspace: workspace)
         } else {
-            packages = [try getPackageRoot()]
+            packages = try [self.getPackageRoot()]
         }
 
         return PackageGraphRootInput(packages: packages, traitConfiguration: traitConfiguration)
@@ -457,10 +457,7 @@ public final class SwiftCommandState {
     }
 
     /// Returns the currently active workspace.
-    public func getActiveWorkspace(
-        emitDeprecatedConfigurationWarning: Bool = false,
-        traitConfiguration: TraitConfiguration? = nil
-    ) throws -> Workspace {
+    public func getActiveWorkspace(emitDeprecatedConfigurationWarning: Bool = false, traitConfiguration: TraitConfiguration = .default) throws -> Workspace {
         if let workspace = _workspace {
             return workspace
         }
@@ -525,9 +522,7 @@ public final class SwiftCommandState {
         return workspace
     }
 
-    public func getRootPackageInformation(traitConfiguration: TraitConfiguration? = nil) async throws
-        -> (dependencies: [PackageIdentity: [PackageIdentity]], targets: [PackageIdentity: [String]])
-    {
+    public func getRootPackageInformation(traitConfiguration: TraitConfiguration = .default) async throws -> (dependencies: [PackageIdentity: [PackageIdentity]], targets: [PackageIdentity: [String]]) {
         let workspace = try self.getActiveWorkspace(traitConfiguration: traitConfiguration)
         let root = try self.getWorkspaceRoot(traitConfiguration: traitConfiguration)
         let rootManifests = try await workspace.loadRootManifests(
@@ -652,7 +647,7 @@ public final class SwiftCommandState {
     }
 
     /// Resolve the dependencies.
-    public func resolve(_ traitConfiguration: TraitConfiguration?) async throws {
+    public func resolve(_ traitConfiguration: TraitConfiguration = .default) async throws {
         let workspace = try getActiveWorkspace(traitConfiguration: traitConfiguration)
         let root = try getWorkspaceRoot(traitConfiguration: traitConfiguration)
 
@@ -682,7 +677,7 @@ public final class SwiftCommandState {
     ) async throws -> ModulesGraph {
         try await self.loadPackageGraph(
             explicitProduct: explicitProduct,
-            traitConfiguration: nil,
+            traitConfiguration: .default,
             testEntryPointPath: testEntryPointPath
         )
     }
@@ -695,7 +690,7 @@ public final class SwiftCommandState {
     @discardableResult
     package func loadPackageGraph(
         explicitProduct: String? = nil,
-        traitConfiguration: TraitConfiguration? = nil,
+        traitConfiguration: TraitConfiguration = .default,
         testEntryPointPath: AbsolutePath? = nil
     ) async throws -> ModulesGraph {
         do {
@@ -750,7 +745,7 @@ public final class SwiftCommandState {
         try self._manifestLoader.get()
     }
 
-    public func canUseCachedBuildManifest(_ traitConfiguration: TraitConfiguration? = nil) async throws -> Bool {
+    public func canUseCachedBuildManifest(_ traitConfiguration: TraitConfiguration = .default) async throws -> Bool {
         if !self.options.caching.cacheBuildManifest {
             return false
         }

Sources/PackageGraph/CMakeLists.txt

@@ -19,7 +19,6 @@ add_library(PackageGraph
   PackageModel+Extensions.swift
   PackageRequirement.swift
   ResolvedPackagesStore.swift
-  TraitConfiguration.swift
   Resolution/PubGrub/Assignment.swift
   Resolution/PubGrub/ContainerProvider.swift
   Resolution/PubGrub/DiagnosticReportBuilder.swift

Sources/PackageGraph/ModulesGraph+Loading.swift

@@ -1035,7 +1035,7 @@ private func calculateEnabledTraits(
         }
     }
 
-    if let parentPackage, !(explictlyEnabledTraits == nil || areDefaultsEnabled) && manifest.traits.isEmpty {
+    if let parentPackage, !(explictlyEnabledTraits == nil || areDefaultsEnabled) && !manifest.supportsTraits {
         // We throw an error when default traits are disabled for a package without any traits
         // This allows packages to initially move new API behind traits once.
         throw ModuleError.disablingDefaultTraitsOnEmptyTraits(

Sources/PackageGraph/ModulesGraph.swift

@@ -458,7 +458,7 @@ public func loadModulesGraph(
     useXCBuildFileRules: Bool = false,
     customXCTestMinimumDeploymentTargets: [PackageModel.Platform: PlatformVersion]? = .none,
     observabilityScope: ObservabilityScope,
-    traitConfiguration: TraitConfiguration? = nil
+    traitConfiguration: TraitConfiguration = .default
 ) throws -> ModulesGraph {
     let rootManifests = manifests.filter(\.packageKind.isRoot).spm_createDictionary { ($0.path, $0) }
     let externalManifests = try manifests.filter { !$0.packageKind.isRoot }
@@ -471,7 +471,7 @@ public func loadModulesGraph(
 
     let packages = Array(rootManifests.keys)
     let input = PackageGraphRootInput(packages: packages, traitConfiguration: traitConfiguration)
-    let graphRoot = PackageGraphRoot(
+    let graphRoot = try PackageGraphRoot(
         input: input,
         manifests: rootManifests,
         explicitProduct: explicitProduct,

Sources/PackageGraph/PackageContainer.swift

@@ -102,7 +102,7 @@ public protocol PackageContainer {
     /// Fetch the enabled traits of a package container.
     ///
     /// NOTE: This method should only be called on root packages.
-    func getEnabledTraits(traitConfiguration: TraitConfiguration?, version: Version?) async throws -> Set<String>
+    func getEnabledTraits(traitConfiguration: TraitConfiguration, version: Version?) async throws -> Set<String>
 }
 
 extension PackageContainer {
@@ -118,7 +118,7 @@ extension PackageContainer {
         return true
     }
 
-    public func getEnabledTraits(traitConfiguration: TraitConfiguration?, version: Version? = nil) async throws -> Set<String> {
+    public func getEnabledTraits(traitConfiguration: TraitConfiguration, version: Version? = nil) async throws -> Set<String> {
         return []
     }
 }

Sources/PackageGraph/PackageGraphRoot.swift

@@ -24,13 +24,13 @@ public struct PackageGraphRootInput {
     public let dependencies: [PackageDependency]
 
     /// The trait configuration for the root packages.
-    public let traitConfiguration: TraitConfiguration?
+    public let traitConfiguration: TraitConfiguration
 
     /// Create a package graph root.
     public init(
         packages: [AbsolutePath],
         dependencies: [PackageDependency] = [],
-        traitConfiguration: TraitConfiguration? = nil
+        traitConfiguration: TraitConfiguration = .default
     ) {
         self.packages = packages
         self.dependencies = dependencies
@@ -49,6 +49,7 @@ public struct PackageGraphRoot {
         return self.packages.compactMapValues { $0.manifest }
     }
 
+    /// The root manifest(s)'s enabled traits (and their transitively enabled traits).
     public var enabledTraits: [PackageIdentity: Set<String>]
 
     /// The root package references.
@@ -94,7 +95,7 @@ public struct PackageGraphRoot {
         explicitProduct: String? = nil,
         dependencyMapper: DependencyMapper? = nil,
         observabilityScope: ObservabilityScope
-    ) {
+    ) throws {
         self.packages = input.packages.reduce(into: .init(), { partial, inputPath in
             if let manifest = manifests[inputPath]  {
                 let packagePath = manifest.path.parentDirectory
@@ -103,20 +104,27 @@ public struct PackageGraphRoot {
             }
         })
 
-        do {
-            // Calculate the enabled traits for root.
-            self.enabledTraits = try packages.reduce(into: [PackageIdentity: Set<String>]()) { traitsMap, package in
-                let manifest = package.value.manifest
-                let traitConfiguration = input.traitConfiguration
-
-                let enabledTraits = try manifest.enabledTraits(using: traitConfiguration?.enabledTraits, enableAllTraits: traitConfiguration?.enableAllTraits ?? false)
-
-                traitsMap[package.key] = enabledTraits
+        // Calculate the enabled traits for root.
+        var enableTraitsMap: [PackageIdentity: Set<String>] = [:]
+        enableTraitsMap = try packages.reduce(into: [PackageIdentity: Set<String>]()) { traitsMap, package in
+            let manifest = package.value.manifest
+            let traitConfiguration = input.traitConfiguration
+
+            // Should only ever have to use trait configuration here for roots.
+            let enabledTraits = try manifest.enabledTraits(using: traitConfiguration)
+            traitsMap[package.key] = enabledTraits
+
+            // Calculate the enabled traits for each dependency of this root:
+            manifest.dependencies.forEach { dependency in
+                if let traits = dependency.traits {
+                    let traitNames = traits.map(\.name)
+                    traitsMap[dependency.identity, default: []].formUnion(Set(traitNames))
+                }
             }
-        } catch {
-            self.enabledTraits = [:]
         }
 
+        self.enabledTraits = enableTraitsMap
+
         // FIXME: Deprecate special casing once the manifest supports declaring used executable products.
         // Special casing explicit products like this is necessary to pass the test suite and satisfy backwards compatibility.
         // However, changing the dependencies based on the command line arguments may force `Package.resolved` to temporarily change,
@@ -129,18 +137,26 @@ public struct PackageGraphRoot {
             // Check that the dependency is used in at least one of the manifests.
             // If not, then we can omit this dependency if pruning unused dependencies
             // is enabled.
-            return manifests.values.reduce(false) {
-                guard $1.pruneDependencies else { return $0 || true }
-                if let isUsed = try? $1.isPackageDependencyUsed(dep, enabledTraits: input.traitConfiguration?.enabledTraits, enableAllTraits: input.traitConfiguration?.enableAllTraits ?? false) {
-                    return $0 || isUsed
+            return manifests.values.reduce(false) { result, manifest in
+                guard manifest.pruneDependencies else { return true }
+                let enabledTraits: Set<String>? = enableTraitsMap[manifest.packageIdentity]
+                if let isUsed = try? manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits) {
+                    return result || isUsed
                 }
+
                 return true
             }
         })
 
         if let explicitProduct {
             // FIXME: `dependenciesRequired` modifies manifests and prevents conversion of `Manifest` to a value type
-            let deps = try? manifests.values.lazy.map({ try $0.dependenciesRequired(for: .everything, input.traitConfiguration?.enabledTraits, enableAllTraits: input.traitConfiguration?.enableAllTraits ?? false) }).flatMap({ $0 })
+            let deps = try? manifests.values.lazy
+                .map({ manifest -> [PackageDependency] in
+                    let enabledTraits: Set<String>? = enableTraitsMap[manifest.packageIdentity]
+                    return try manifest.dependenciesRequired(for: .everything, enabledTraits)
+                })
+                .flatMap({ $0 })
+
             for dependency in deps ?? [] {
                 adjustedDependencies.append(dependency.filtered(by: .specific([explicitProduct])))
             }
@@ -224,3 +240,4 @@ extension PackageDependency.Registry.Requirement {
         }
     }
 }
+

Sources/PackageGraph/Resolution/DependencyResolutionNode.swift

@@ -58,7 +58,7 @@ public enum DependencyResolutionNode {
     ///   It is a warning condition, and builds do not actually need these dependencies.
     ///   However, forcing the graph to resolve and fetch them anyway allows the diagnostics passes access
     ///   to the information needed in order to provide actionable suggestions to help the user stitch up the dependency declarations properly.
-    case root(package: PackageReference, traitConfiguration: TraitConfiguration? = nil)
+    case root(package: PackageReference, traitConfiguration: TraitConfiguration = .default)
 
     /// The package.
     public var package: PackageReference {
@@ -94,14 +94,25 @@ public enum DependencyResolutionNode {
     public var traits: Set<String>? {
         switch self {
         case .root(_, let config):
-            return config?.enabledTraits
+            return config.enabledTraits
         case .product(_, _, let enabledTraits):
             return enabledTraits
         default:
             return nil
         }
     }
 
+    public var traitConfiguration: TraitConfiguration {
+        switch self {
+        case .root(_, let config):
+            return config
+        case .product(_, _, let enabledTraits):
+            return .init(enabledTraits: enabledTraits)
+        case .empty:
+            return .default
+        }
+    }
+
     /// Returns the dependency that a product has on its own package, if relevant.
     ///
     /// This is the constraint that requires all products from a package resolve to the same version.

Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift

@@ -165,7 +165,7 @@ public struct PubGrubDependencyResolver {
     }
 
     /// Execute the resolution algorithm to find a valid assignment of versions.
-    public func solve(constraints: [Constraint], traitConfiguration: TraitConfiguration? = nil) async -> Result<[DependencyResolverBinding], Error> {
+    public func solve(constraints: [Constraint], traitConfiguration: TraitConfiguration = .default) async -> Result<[DependencyResolverBinding], Error> {
         // the graph resolution root
         let root: DependencyResolutionNode
         if constraints.count == 1, let constraint = constraints.first, constraint.package.kind.isRoot {

Sources/PackageGraph/Resolution/PubGrub/PubGrubPackageContainer.swift

@@ -154,8 +154,7 @@ final class PubGrubPackageContainer {
         at version: Version,
         node: DependencyResolutionNode,
         overriddenPackages: [PackageReference: (version: BoundVersion, products: ProductFilter)],
-        root: DependencyResolutionNode,
-        traitConfiguration: TraitConfiguration? = nil
+        root: DependencyResolutionNode
     ) async throws -> [Incompatibility] {
         // FIXME: It would be nice to compute bounds for this as well.
         if await !self.underlying.isToolsVersionCompatible(at: version) {
@@ -168,10 +167,11 @@ final class PubGrubPackageContainer {
             )]
         }
 
+        let enabledTraits = node.package.kind.isRoot ? try await self.underlying.getEnabledTraits(traitConfiguration: node.traitConfiguration) : node.traits
         var unprocessedDependencies = try await self.underlying.getDependencies(
             at: version,
             productFilter: node.productFilter,
-            node.traits
+            enabledTraits
         )
         if let sharedVersion = node.versionLock(version: version) {
             unprocessedDependencies.append(sharedVersion)

Sources/PackageGraph/TraitConfiguration.swift

@@ -440,6 +440,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
 
                 let manifest = Manifest(
                     displayName: parsedManifest.name,
+                    packageIdentity: packageIdentity,
                     path: manifestPath,
                     packageKind: packageKind,
                     packageLocation: packageLocation,