[Traits] Fix traits omitting used package dependencies

Sources/CoreCommands/SwiftCommandState.swift

@@ -204,11 +204,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)
@@ -715,7 +717,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
         }
@@ -732,7 +734,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
         }

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.

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"]))
         }
     }