[Traits] Fix traits omitting used package dependencies (#8399)

bripeticca · web-flow · commit 5b30b32a14a0 · 2025-03-26T09:45:20.000-04:00
Fixes #8398 Used package dependencies of a manifest were considered trait-guarded if a target dependency in the manifest: 1. used a product from said package and 2. was trait-guarded regardless as to whether the same package dependency was used elsewhere (whether in the same target or another in the same manifest) and was not guarded by traits. This change ensures that this is considered when doing the calculations for package dependencies that should be omitted if *every* target dependency that uses said package dependency is guarded.
diff --git a/Sources/CoreCommands/SwiftCommandState.swift b/Sources/CoreCommands/SwiftCommandState.swift
@@ -215,11 +215,13 @@ public final class SwiftCommandState {
 
     /// Get the current workspace root object.
     public func getWorkspaceRoot(traitConfiguration: TraitConfiguration? = nil) throws -> PackageGraphRootInput {
-        let packages: [AbsolutePath] = if let workspace = options.locations.multirootPackageDataFile {
-            try self.workspaceLoaderProvider(self.fileSystem, self.observabilityScope)
+        let packages: [AbsolutePath]
+
+        if let workspace = options.locations.multirootPackageDataFile {
+            packages = try self.workspaceLoaderProvider(self.fileSystem, self.observabilityScope)
                 .load(workspace: workspace)
         } else {
-            try [self.getPackageRoot()]
+            packages = [try getPackageRoot()]
         }
 
         return PackageGraphRootInput(packages: packages, traitConfiguration: traitConfiguration)
@@ -748,7 +750,7 @@ public final class SwiftCommandState {
         try self._manifestLoader.get()
     }
 
-    public func canUseCachedBuildManifest() async throws -> Bool {
+    public func canUseCachedBuildManifest(_ traitConfiguration: TraitConfiguration? = nil) async throws -> Bool {
         if !self.options.caching.cacheBuildManifest {
             return false
         }
@@ -765,7 +767,7 @@ public final class SwiftCommandState {
         // Perform steps for build manifest caching if we can enabled it.
         //
         // FIXME: We don't add edited packages in the package structure command yet (SR-11254).
-        let hasEditedPackages = try await self.getActiveWorkspace().state.dependencies.contains(where: \.isEdited)
+        let hasEditedPackages = try await self.getActiveWorkspace(traitConfiguration: traitConfiguration).state.dependencies.contains(where: \.isEdited)
         if hasEditedPackages {
             return false
         }
diff --git a/Sources/PackageModel/Manifest/Manifest.swift b/Sources/PackageModel/Manifest/Manifest.swift
@@ -184,8 +184,7 @@ public final class Manifest: Sendable {
     /// Returns a list of dependencies that are being guarded by unenabled traits, given a set of enabled traits.
     ///
     /// If a trait that is guarding a dependency is enabled (and is reflected in the `enabledTraits` parameter) and
-    /// results in
-    /// that dependency being used, then that dependency is not considered trait-guarded.
+    /// results in that dependency being used, then that dependency is not considered trait-guarded.
     ///
     /// For example:
     ///
@@ -203,38 +202,46 @@ public final class Manifest: Sendable {
             return []
         }
 
-        let traitGuardedDeps = self.traitGuardedDependencies(lowercasedKeys: true)
+        let traitGuardedDeps = self.traitGuardedTargetDependencies(lowercasedKeys: true)
         let explicitlyEnabledTraits = try? self.enabledTraits(using: enabledTraits, enableAllTraits: enableAllTraits)
         guard self.toolsVersion >= .v5_2 && !self.packageKind.isRoot else {
             let deps = self.dependencies.filter {
-                if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }),
-                   !guardTraits.isEmpty, let explicitlyEnabledTraits
-                {
-                    return !guardTraits.allSatisfy { explicitlyEnabledTraits.contains($0) }
+                var result = false
+                for guardedTargetDeps in traitGuardedDeps[$0.identity.description] ?? [] {
+                    if let guardTraits = guardedTargetDeps.condition?.traits, !guardTraits.isEmpty,
+                       let explicitlyEnabledTraits
+                    {
+                        result = result || !guardTraits.allSatisfy { explicitlyEnabledTraits.contains($0) }
+                    }
                 }
 
-                return false
+                return result
             }
             return deps
         }
 
         if let dependencies = self._requiredDependencies[.nothing] {
             let deps = dependencies.filter {
-                if let guardTraits = traitGuardedDeps[$0.identity.description]?.values.flatMap({ $0 }),
-                   let explicitlyEnabledTraits
-                {
-                    return !guardTraits.allSatisfy { explicitlyEnabledTraits.contains($0) }
+                var result = false
+                for guardedTargetDeps in traitGuardedDeps[$0.identity.description] ?? [] {
+                    if let guardTraits = guardedTargetDeps.condition?.traits, !guardTraits.isEmpty,
+                       let explicitlyEnabledTraits
+                    {
+                        result = result || !guardTraits.allSatisfy { explicitlyEnabledTraits.contains($0) }
+                    }
                 }
 
-                return false
+                return result
             }
             return deps
         } else {
             var guardedDependencies: Set<PackageIdentity> = []
             for target in self.targetsRequired(for: self.products) {
+                let traitGuardedTargetDeps = traitGuardedTargetDependencies(for: target)
+
                 for targetDependency in target.dependencies {
                     guard let dependency = self.packageDependency(referencedBy: targetDependency),
-                          let guardingTraits = traitGuardedDeps[dependency.identity.description]?[target.name]
+                          let guardingTraits = traitGuardedTargetDeps[targetDependency]
                     else {
                         continue
                     }
@@ -245,19 +252,6 @@ public final class Manifest: Sendable {
                         guardedDependencies.insert(dependency.identity)
                     }
                 }
-
-                target.pluginUsages?.forEach {
-                    guard let dependency = self.packageDependency(referencedBy: $0),
-                          let guardingTraits = traitGuardedDeps[dependency.identity.description]?[target.name]
-                    else {
-                        return
-                    }
-                    if let explicitlyEnabledTraits,
-                       guardingTraits.intersection(explicitlyEnabledTraits) != guardingTraits
-                    {
-                        guardedDependencies.insert(dependency.identity)
-                    }
-                }
             }
 
             let dependencies = self.dependencies.filter { guardedDependencies.contains($0.identity) }
@@ -301,8 +295,6 @@ public final class Manifest: Sendable {
             return dependencies
         }
 
-        // calculate explicitly enabled traits through config:
-
         // using .nothing as cache key while ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION is false
         if var dependencies = self._requiredDependencies[.nothing] {
             if self.pruneDependencies {
@@ -877,20 +869,45 @@ extension Manifest {
         return (knownPackage: known, unknownPackage: unknown)
     }
 
-    /// Returns the set of package dependencies that are potentially guarded by traits, per target. This does not
-    /// calculate enabled and disabled dependencies enforced by traits.
-    public func traitGuardedDependencies(lowercasedKeys: Bool = false) -> [String: [String: Set<String>]] {
-        self.targets.reduce(into: [String: [String: Set<String>]]()) { depMap, target in
-            let traitGuardedTargetDependencies = target.dependencies.filter {
-                !($0.condition?.traits?.isEmpty ?? true)
-            }
+    /// Computes the list of target dependencies per target that are guarded by traits.
+    /// A target dependency is considered potentially trait-guarded if it defines a condition wherein there exists a
+    /// list of traits.
+    /// - Parameters:
+    ///    - lowercasedKeys: A flag that determines whether the keys in the resulting dictionary are lowercased.
+    /// - Returns: A dictionary that maps the name of a `TargetDescription` to a list of its dependencies that are
+    /// guarded by traits.
+    public func traitGuardedTargetDependencies(
+        lowercasedKeys: Bool = false
+    ) -> [String: [TargetDescription.Dependency]] {
+        self.targets.reduce(into: [String: [TargetDescription.Dependency]]()) { depMap, target in
+            let traitGuardedTargetDependencies = traitGuardedTargetDependencies(
+                for: target
+            )
+
             traitGuardedTargetDependencies.forEach {
-                guard let package = lowercasedKeys ? $0.package?.lowercased() : $0.package else { return }
-                depMap[package, default: [:]][target.name, default: []].formUnion($0.condition?.traits ?? [])
+                guard let package = lowercasedKeys ? $0.key.package?.lowercased() : $0.key.package else { return }
+                depMap[package, default: []].append($0.key)
             }
         }
     }
 
+    /// Computes the list of target dependencies that are guarded by traits for given target.
+    /// A target dependency is considered potentially trait-guarded if it defines a condition wherein there exists a
+    /// list of traits.
+    /// - Parameters:
+    ///    - target: A `TargetDescription` for which the trait-guarded target dependencies are calculated.
+    /// - Returns: A dictionary that maps each trait-guarded `TargetDescription.Dependency` of the given
+    /// `TargetDescription` to the list of traits that guard it.
+    public func traitGuardedTargetDependencies(for target: TargetDescription)
+        -> [TargetDescription.Dependency: Set<String>]
+    {
+        target.dependencies.filter {
+            !($0.condition?.traits?.isEmpty ?? true)
+        }.reduce(into: [TargetDescription.Dependency: Set<String>]()) { depMap, dep in
+            depMap[dep, default: []].formUnion(dep.condition?.traits ?? [])
+        }
+    }
+
     /// Computes the enabled traits for a given target dependency
     public func enabledTraits(forDependency dependency: TargetDescription.Dependency) -> Set<String>? {
         guard let package = self.packageDependency(referencedBy: dependency),
@@ -910,21 +927,22 @@ extension Manifest {
         enableAllTraits: Bool = false
     ) throws -> Bool {
         guard self.supportsTraits, !enableAllTraits else { return true }
-        guard let package = dependency.package, let target = self.targetMap[target] else { return false }
+        guard let target = self.targetMap[target] else { return false }
         guard target.dependencies.contains(where: { $0 == dependency }) else {
             throw InternalError(
                 "target dependency \(dependency.name) not found for target \(target.name) in package \(self.displayName)"
             )
         }
-        let traitsThatEnableDependency = self.traitGuardedDependencies()[package]?[target.name] ?? []
 
-        let isEnabled = try traitsThatEnableDependency.allSatisfy { try self.isTraitEnabled(
+        let traitsToEnable = self.traitGuardedTargetDependencies(for: target)[dependency] ?? []
+
+        let isEnabled = try traitsToEnable.allSatisfy { try self.isTraitEnabled(
             .init(stringLiteral: $0),
             enabledTraits,
             enableAllTraits
         ) }
 
-        return traitsThatEnableDependency.isEmpty || isEnabled
+        return traitsToEnable.isEmpty || isEnabled
     }
 
     /// Determines whether a given package dependency is used by this manifest given a set of enabled traits.
diff --git a/Tests/PackageModelTests/ManifestTests.swift b/Tests/PackageModelTests/ManifestTests.swift
@@ -459,6 +459,10 @@ class ManifestTests: XCTestCase {
                         name: "Baz",
                         package: "Baz"
                     ),
+                    .product(
+                        name: "Bar2",
+                        package: "Bar"
+                    )
                 ]
             ),
         ]
@@ -481,11 +485,14 @@ class ManifestTests: XCTestCase {
                 traits: traits
             )
 
-            let traitGuardedDependencies = manifest.traitGuardedDependencies()
+            let traitGuardedDependencies = manifest.traitGuardedTargetDependencies()
+
             XCTAssertEqual(
                 traitGuardedDependencies,
                 [
-                    "Bar": ["Foo": ["Trait2"]],
+                    "Bar": [
+                        .product(name: "Bar", package: "Bar", condition: .init(traits: ["Trait2"]))
+                    ]
                 ]
             )
         }
@@ -501,24 +508,35 @@ class ManifestTests: XCTestCase {
             ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]),
         ]
 
-        let unguardedTargetDependency: TargetDescription.Dependency = .product(name: "Bar", package: "Blah")
+        let unguardedTargetDependency: TargetDescription.Dependency = .product(
+            name: "Bar",
+            package: "Blah"
+        )
+
         let trait3GuardedTargetDependency: TargetDescription.Dependency = .product(
             name: "Baz",
             package: "Buzz",
             condition: .init(traits: ["Trait3"])
         )
+
         let defaultTraitGuardedTargetDependency: TargetDescription.Dependency = .product(
             name: "Bam",
             package: "Boom",
             condition: .init(traits: ["Trait2"])
         )
 
+        let enabledTargetDependencyWithSamePackage: TargetDescription.Dependency = .product(
+            name: "Kaboom",
+            package: "Boom"
+        )
+
         let target = try TargetDescription(
             name: "Foo",
             dependencies: [
                 unguardedTargetDependency,
                 trait3GuardedTargetDependency,
                 defaultTraitGuardedTargetDependency,
+                enabledTargetDependencyWithSamePackage,
             ]
         )
 
@@ -586,6 +604,14 @@ class ManifestTests: XCTestCase {
                 defaultTraitGuardedTargetDependency,
                 enabledTraits: []
             ))
+
+            // Test if a target dependency that isn't guarded by traits wherein it uses a product
+            // from the same package as another target dependency that is guarded by traits; should be true.
+            XCTAssertTrue(try manifest.isTargetDependencyEnabled(
+                target: "Foo",
+                enabledTargetDependencyWithSamePackage,
+                enabledTraits: []
+            ))
         }
     }
 
@@ -604,18 +630,28 @@ class ManifestTests: XCTestCase {
             ProductDescription(name: "Foo", type: .library(.automatic), targets: ["Foo"]),
         ]
 
-        let unguardedTargetDependency: TargetDescription.Dependency = .product(name: "Bar", package: "Bar")
+        let unguardedTargetDependency: TargetDescription.Dependency = .product(
+            name: "Bar",
+            package: "Bar"
+        )
+
         let trait3GuardedTargetDependency: TargetDescription.Dependency = .product(
             name: "Baz",
             package: "Baz",
             condition: .init(traits: ["Trait3"])
         )
+
         let defaultTraitGuardedTargetDependency: TargetDescription.Dependency = .product(
             name: "Bam",
             package: "Bam",
             condition: .init(traits: ["Trait2"])
         )
 
+        let unguardedTargetDependencyWithBamPackage: TargetDescription.Dependency = .product(
+            name: "Qux",
+            package: "Bam"
+        )
+
         let target = try TargetDescription(
             name: "Foo",
             dependencies: [
@@ -625,6 +661,16 @@ class ManifestTests: XCTestCase {
             ]
         )
 
+        let targetWithUnguardedBamPackageDep = try TargetDescription(
+            name: "Foo",
+            dependencies: [
+                unguardedTargetDependency,
+                trait3GuardedTargetDependency,
+                defaultTraitGuardedTargetDependency,
+                unguardedTargetDependencyWithBamPackage,
+            ]
+        )
+
         let traits: Set<TraitDescription> = [
             TraitDescription(name: "default", enabledTraits: ["Trait1"]),
             TraitDescription(name: "Trait1", enabledTraits: ["Trait2"]),
@@ -643,13 +689,31 @@ class ManifestTests: XCTestCase {
                 traits: traits
             )
 
+            let manifestWithBamDependencyAlwaysUsed = Manifest.createRootManifest(
+                displayName: "Foo",
+                path: "/Foo",
+                toolsVersion: .v5_2,
+                dependencies: dependencies,
+                products: products,
+                targets: [targetWithUnguardedBamPackageDep],
+                traits: traits
+            )
+
             XCTAssertTrue(try manifest.isPackageDependencyUsed(bar, enabledTraits: nil))
             XCTAssertTrue(try manifest.isPackageDependencyUsed(bar, enabledTraits: []))
             XCTAssertFalse(try manifest.isPackageDependencyUsed(baz, enabledTraits: nil))
             XCTAssertTrue(try manifest.isPackageDependencyUsed(baz, enabledTraits: ["Trait3"]))
             XCTAssertTrue(try manifest.isPackageDependencyUsed(bam, enabledTraits: nil))
             XCTAssertFalse(try manifest.isPackageDependencyUsed(bam, enabledTraits: []))
             XCTAssertFalse(try manifest.isPackageDependencyUsed(bam, enabledTraits: ["Trait3"]))
+
+            // Configuration of the manifest that includes a case where there exists a target
+            // dependency that depends on the same package as another target dependency, but
+            // is unguarded by traits; therefore, this package dependency should be considered used
+            // in every scenario, regardless of the passed trait configuration.
+            XCTAssertTrue(try manifestWithBamDependencyAlwaysUsed.isPackageDependencyUsed(bam, enabledTraits: nil))
+            XCTAssertTrue(try manifestWithBamDependencyAlwaysUsed.isPackageDependencyUsed(bam, enabledTraits: []))
+            XCTAssertTrue(try manifestWithBamDependencyAlwaysUsed.isPackageDependencyUsed(bam, enabledTraits: ["Trait3"]))
         }
     }
 
