Skip to content

Fix for .when(traits:) condition not working for multiple traits #9015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix for .when(traits:) condition not working for multiple traits #9015

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
121a048
Fix for .when(traits:) condition not working for multiple traits
bripeticca Aug 12, 2025
c5b7889
Update trait fixtures to include test case for this scenario
bripeticca Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions Sources/PackageModel/Manifest/Manifest+Traits.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ extension Manifest {
_ parentPackage: PackageIdentifier? = nil
) throws {
guard supportsTraits else {
if explicitlyEnabledTraits != ["default"] /*!explicitlyEnabledTraits.contains("default")*/ {
if explicitlyEnabledTraits != ["default"] {
throw TraitError.traitsNotSupported(
parent: parentPackage,
package: .init(self),
Expand All @@ -116,7 +116,7 @@ extension Manifest {
let areDefaultsEnabled = enabledTraits.contains("default")

// Ensure that disabling default traits is disallowed for packages that don't define any traits.
if !(explicitlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits {
if !areDefaultsEnabled && !self.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 TraitError.traitsNotSupported(
Expand Down Expand Up @@ -449,15 +449,18 @@ extension Manifest {

let traitsToEnable = self.traitGuardedTargetDependencies(for: target)[dependency] ?? []

let isEnabled = try traitsToEnable.allSatisfy { try self.isTraitEnabled(
// Check if any of the traits guarding this dependency is enabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: can we write some automated tests that would validate this function in isolation. This would allow us to validate various scenarios, include fault injection et al. If we are unable to control the test the was we want, we can rework the function to make it testable.

// if so, the condition is met and the target dependency is considered
// to be in an enabled state.
let isEnabled = try traitsToEnable.contains(where: { try self.isTraitEnabled(
.init(stringLiteral: $0),
enabledTraits,
) }
) })

return traitsToEnable.isEmpty || isEnabled
}
/// Determines whether a given package dependency is used by this manifest given a set of enabled traits.
public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set<String>/* = ["default"]*/) throws -> Bool {
public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set<String>) throws -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: can we write some automated tests that would validate this function in isolation. This would allow us to validate various scenarios, include fault injection et al. If we are unable to control the test the was we want, we can rework the function to make it testable.

if self.pruneDependencies {
let usedDependencies = try self.usedDependencies(withTraits: enabledTraits)
let foundKnownPackage = usedDependencies.knownPackage.contains(where: {
Expand All @@ -478,8 +481,8 @@ extension Manifest {

// if target deps is empty, default to returning true here.
let isTraitGuarded = targetDependenciesForPackageDependency.isEmpty ? false : targetDependenciesForPackageDependency.compactMap({ $0.condition?.traits }).allSatisfy({
let condition = $0.subtracting(enabledTraits)
return !condition.isEmpty
let isGuarded = $0.intersection(enabledTraits).isEmpty
return isGuarded
})

let isUsedWithoutTraitGuarding = !targetDependenciesForPackageDependency.filter({ $0.condition?.traits == nil }).isEmpty
Expand Down
107 changes: 107 additions & 0 deletions Tests/WorkspaceTests/WorkspaceTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16257,6 +16257,113 @@ final class WorkspaceTests: XCTestCase {
}
}

func testManyTraitsEnableTargetDependency() async throws {
let sandbox = AbsolutePath("/tmp/ws/")
let fs = InMemoryFileSystem()

func createMockWorkspace(_ traitConfiguration: TraitConfiguration) async throws -> MockWorkspace {
try await MockWorkspace(
sandbox: sandbox,
fileSystem: fs,
roots: [
MockPackage(
name: "Cereal",
targets: [
MockTarget(
name: "Wheat",
dependencies: [
.product(
name: "Icing",
package: "Sugar",
condition: .init(traits: ["BreakfastOfChampions", "DontTellMom"])
),
]
),
],
products: [
MockProduct(name: "YummyBreakfast", modules: ["Wheat"])
],
dependencies: [
.sourceControl(path: "./Sugar", requirement: .upToNextMajor(from: "1.0.0")),
],
traits: ["BreakfastOfChampions", "DontTellMom"]
),
],
packages: [
MockPackage(
name: "Sugar",
targets: [
MockTarget(name: "Icing"),
],
products: [
MockProduct(name: "Icing", modules: ["Icing"]),
],
versions: ["1.0.0", "1.5.0"]
),
],
traitConfiguration: traitConfiguration
)
}


let deps: [MockDependency] = [
.sourceControl(path: "./Sugar", requirement: .exact("1.0.0"), products: .specific(["Icing"])),
]

let workspaceOfChampions = try await createMockWorkspace(.enabledTraits(["BreakfastOfChampions"]))
try await workspaceOfChampions.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTesterXCTest(graph) { result in
result.check(roots: "Cereal")
result.check(packages: "cereal", "sugar")
result.check(modules: "Wheat", "Icing")
result.check(products: "YummyBreakfast", "Icing")
result.checkTarget("Wheat") { result in
result.check(dependencies: "Icing")
}
}
}

let dontTellMomAboutThisWorkspace = try await createMockWorkspace(.enabledTraits(["DontTellMom"]))
try await dontTellMomAboutThisWorkspace.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTesterXCTest(graph) { result in
result.check(roots: "Cereal")
result.check(packages: "cereal", "sugar")
result.check(modules: "Wheat", "Icing")
result.check(products: "YummyBreakfast", "Icing")
result.checkTarget("Wheat") { result in
result.check(dependencies: "Icing")
}
}
}

let allEnabledTraitsWorkspace = try await createMockWorkspace(.enableAllTraits)
try await allEnabledTraitsWorkspace.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTesterXCTest(graph) { result in
result.check(roots: "Cereal")
result.check(packages: "cereal", "sugar")
result.check(modules: "Wheat", "Icing")
result.check(products: "YummyBreakfast", "Icing")
result.checkTarget("Wheat") { result in
result.check(dependencies: "Icing")
}
}
}

let noSugarForBreakfastWorkspace = try await createMockWorkspace(.disableAllTraits)
try await noSugarForBreakfastWorkspace.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in
XCTAssertNoDiagnostics(diagnostics)
PackageGraphTesterXCTest(graph) { result in
result.check(roots: "Cereal")
result.check(packages: "cereal")
result.check(modules: "Wheat")
result.check(products: "YummyBreakfast")
}
}
}

func makeRegistryClient(
packageIdentity: PackageIdentity,
packageVersion: Version,
Expand Down