Skip to content

[WIP] Fix for trait-guarded deps being included in resolution #8852

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 3 commits into
base: main
Choose a base branch
from
Draft

[WIP] Fix for trait-guarded deps being included in resolution #8852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5469b84
Fix for trait-guarded deps being included in resolution
bripeticca Jun 19, 2025
f67f3df
Comment cleanup
bripeticca Jun 19, 2025
0d46a4c
De-duplicate calculation of transitively enabled traits
bripeticca Jun 19, 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
4 changes: 2 additions & 2 deletions Sources/PackageGraph/GraphLoadingNode.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ public struct GraphLoadingNode: Equatable, Hashable {
public let productFilter: ProductFilter

/// The enabled traits for this package.
package var enabledTraits: Set<String>
package var enabledTraits: Set<String>?

public init(
identity: PackageIdentity,
manifest: Manifest,
productFilter: ProductFilter,
enabledTraits: Set<String>
enabledTraits: Set<String>?
) throws {
self.identity = identity
self.manifest = manifest
Expand Down
22 changes: 10 additions & 12 deletions Sources/PackageGraph/ModulesGraph+Loading.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ extension ModulesGraph {
identity: dependency.identity,
manifest: $0.manifest,
productFilter: dependency.productFilter,
enabledTraits: []
enabledTraits: root.enabledTraits[dependency.identity]
)
}
}
Expand All @@ -104,15 +104,10 @@ extension ModulesGraph {
guard let conditionTraits = $0.condition?.traits else {
return true
}
return !conditionTraits.intersection(node.item.enabledTraits).isEmpty
return !conditionTraits.intersection(node.item.enabledTraits ?? []).isEmpty
}.map(\.name)

let calculatedTraits = try calculateEnabledTraits(
parentPackage: node.item.identity,
identity: dependency.identity,
manifest: manifest,
explictlyEnabledTraits: explictlyEnabledTraits.flatMap { Set($0) }
)
let calculatedTraits = try manifest.enabledTraits(using: explictlyEnabledTraits.flatMap { Set($0) }, node.item.identity.description)

return try KeyedPair(
GraphLoadingNode(
Expand Down Expand Up @@ -153,7 +148,10 @@ extension ModulesGraph {
allNodes[$0.key] = $0.item
} onDuplicate: { first, second in
// We are unifying the enabled traits on duplicate
allNodes[first.key]?.enabledTraits.formUnion(second.item.enabledTraits)
// TODO bp: this may not be necessary anymore since we pre-compute all enabled and transitively enabled traits...?
if let enabledTraits = second.item.enabledTraits {
allNodes[first.key]?.enabledTraits?.formUnion(enabledTraits)
}
}

// Create the packages.
Expand Down Expand Up @@ -184,7 +182,7 @@ extension ModulesGraph {
createREPLProduct: manifest.packageKind.isRoot ? createREPLProduct : false,
fileSystem: fileSystem,
observabilityScope: nodeObservabilityScope,
enabledTraits: node.enabledTraits
enabledTraits: node.enabledTraits ?? ["default"]
)
let package = try builder.construct()
manifestToPackage[manifest] = package
Expand Down Expand Up @@ -410,7 +408,7 @@ private func createResolvedPackages(
return ResolvedPackageBuilder(
package,
productFilter: node.productFilter,
enabledTraits: node.enabledTraits,
enabledTraits: node.enabledTraits ?? ["default"],
isAllowedToVendUnsafeProducts: isAllowedToVendUnsafeProducts,
allowedToOverride: allowedToOverride,
platformVersionProvider: platformVersionProvider
Expand Down Expand Up @@ -1449,7 +1447,7 @@ private final class ResolvedPackageBuilder: ResolvedBuilder<ResolvedPackage> {
var products: [ResolvedProductBuilder] = []

/// The enabled traits of this package.
var enabledTraits: Set<String> = []
var enabledTraits: Set<String> = ["default"]

/// The dependencies of this package.
var dependencies: [ResolvedPackageBuilder] = []
Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageGraph/PackageGraphRoot.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ 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.
// TODO bp: assure that trait-guarded deps are pruned regardless
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
Expand Down
40 changes: 32 additions & 8 deletions Sources/PackageModel/Manifest/Manifest+Traits.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,10 @@ extension Manifest {
let traitGuardedDeps = try target.dependencies.filter { dep in
let traits = dep.condition?.traits ?? []

// If traits is empty, then we must manually validate the explicitly enabled traits.
if traits.isEmpty {
try validateEnabledTraits(enabledTraits)
}
// For each trait that is a condition on this target dependency, assure that
// each one is enabled in the manifest.
return try traits.allSatisfy({ try isTraitEnabled(.init(stringLiteral: $0), enabledTraits) })
Expand Down Expand Up @@ -433,14 +437,34 @@ extension Manifest {
}
/// 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>?) throws -> Bool {
let usedDependencies = try self.usedDependencies(withTraits: enabledTraits)
let foundKnownPackage = usedDependencies.knownPackage.contains(where: {
$0.caseInsensitiveCompare(dependency.identity.description) == .orderedSame
})

// if there is a target dependency referenced by name and the package it originates from is unknown, default to
// tentatively marking the package dependency as used. to be resolved later on.
return foundKnownPackage || (!foundKnownPackage && !usedDependencies.unknownPackage.isEmpty)
if self.pruneDependencies {
let usedDependencies = try self.usedDependencies(withTraits: enabledTraits)
let foundKnownPackage = usedDependencies.knownPackage.contains(where: {
$0.caseInsensitiveCompare(dependency.identity.description) == .orderedSame
})

// if there is a target dependency referenced by name and the package it originates from is unknown, default to
// tentatively marking the package dependency as used. to be resolved later on.
return foundKnownPackage || (!foundKnownPackage && !usedDependencies.unknownPackage.isEmpty)
} else {
// alternate path to compute trait-guarded package dependencies if the prune deps feature is not enabled
try validateEnabledTraits(enabledTraits)

let targetDependenciesForPackageDependency = self.targets.flatMap({ $0.dependencies })
.filter({
$0.package?.caseInsensitiveCompare(dependency.identity.description) == .orderedSame
})

// 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 isUsedWithoutTraitGuarding = !targetDependenciesForPackageDependency.filter({ $0.condition?.traits == nil }).isEmpty

return isUsedWithoutTraitGuarding || !isTraitGuarded
}
}
}

Expand Down
7 changes: 2 additions & 5 deletions Sources/PackageModel/Manifest/Manifest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,21 +287,18 @@ public final class Manifest: Sendable {

guard self.toolsVersion >= .v5_2 && !self.packageKind.isRoot else {
var dependencies = self.dependencies
if pruneDependencies {
dependencies = try dependencies.filter({
return try self.isPackageDependencyUsed($0, enabledTraits: enabledTraits)
let isUsed = try self.isPackageDependencyUsed($0, enabledTraits: enabledTraits)
return isUsed
})
}
return dependencies
}

// using .nothing as cache key while ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION is false
if var dependencies = self._requiredDependencies[.nothing] {
if self.pruneDependencies {
dependencies = try dependencies.filter({
return try self.isPackageDependencyUsed($0, enabledTraits: enabledTraits)
})
}
return dependencies
} else {
var requiredDependencies: Set<PackageIdentity> = []
Expand Down
58 changes: 36 additions & 22 deletions Sources/Workspace/Workspace+Manifests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,36 +216,38 @@ extension Workspace {
return true
}
return !conditionTraits.intersection(allRootEnabledTraits).isEmpty
}.map(\.name) ?? []
}.map(\.name)

traitMap[dependency.identity] = Set(explicitlyEnabledTraits)
if let explicitlyEnabledTraits {
traitMap[dependency.identity] = Set(explicitlyEnabledTraits)
}
}

var unusedIdentities: OrderedCollections.OrderedSet<PackageReference> = []
var inputIdentities: OrderedCollections.OrderedSet<PackageReference> = []

let inputNodes: [GraphLoadingNode] = try root.packages.map { identity, package in
inputIdentities.append(package.reference)
let traits: Set<String>? = rootEnabledTraitsMap[package.reference.identity] ?? []
let traits: Set<String>? = rootEnabledTraitsMap[package.reference.identity]

let node = try GraphLoadingNode(
identity: identity,
manifest: package.manifest,
productFilter: .everything,
enabledTraits: traits ?? []
enabledTraits: traits //?? []
)
return node
} + root.dependencies.compactMap { dependency in
let package = dependency.packageRef
inputIdentities.append(package)
return try manifestsMap[dependency.identity].map { manifest in
let traits: Set<String>? = rootDependenciesEnabledTraitsMap[dependency.identity] ?? []
let traits: Set<String>? = rootDependenciesEnabledTraitsMap[dependency.identity]

return try GraphLoadingNode(
identity: dependency.identity,
manifest: manifest,
productFilter: dependency.productFilter,
enabledTraits: traits ?? []
enabledTraits: traits //?? []
)
}
}
Expand All @@ -265,11 +267,12 @@ extension Workspace {

var requiredIdentities: OrderedCollections.OrderedSet<PackageReference> = []
_ = try transitiveClosure(inputNodes) { node in

try node.manifest.dependenciesRequired(for: node.productFilter, node.enabledTraits)
// print("checking \(node.manifest.displayName) with traits \(node.enabledTraits)")
return try node.manifest.dependenciesRequired(for: node.productFilter, node.enabledTraits)
.compactMap { dependency in
let package = dependency.packageRef

// print("checking if dep \(dependency.identity) is used in \(node.manifest.displayName) with traits \(node.enabledTraits)")
// Check if traits are guarding the dependency from being enabled.
// Also check whether we've enabled pruning unused dependencies.
let isDepUsed = try node.manifest.isPackageDependencyUsed(
Expand All @@ -280,7 +283,7 @@ extension Workspace {
observabilityScope.emit(debug: """
'\(package.identity)' from '\(package.locationString)' was omitted \
from required dependencies because it is being guarded by the following traits:' \
\(node.enabledTraits.joined(separator: ", "))
\(node.enabledTraits?.joined(separator: ", "))
""")
} else {
unusedDepsPerPackage[node.identity, default: []] = unusedDepsPerPackage[
Expand Down Expand Up @@ -333,13 +336,13 @@ extension Workspace {
guard let conditionTraits = $0.condition?.traits else {
return true
}
return !conditionTraits.intersection(node.enabledTraits).isEmpty
return !conditionTraits.intersection(node.enabledTraits ?? []).isEmpty
}.map(\.name)

return try manifestsMap[dependency.identity].map { manifest in
// Calculate all transitively enabled traits for this manifest.

var allEnabledTraits: Set<String> = []
var allEnabledTraits: Set<String>?
if let explicitlyEnabledTraits
{
allEnabledTraits = Set(explicitlyEnabledTraits)
Expand Down Expand Up @@ -425,9 +428,11 @@ extension Workspace {
}
return !conditionTraits
.intersection(workspace.configuration.traitConfiguration.enabledTraits ?? []).isEmpty
}.map(\.name) ?? []
}.map(\.name)

traitMap[dependency.identity] = Set(explicitlyEnabledTraits)
if let explicitlyEnabledTraits {
traitMap[dependency.identity] = Set(explicitlyEnabledTraits)
}
}

for (externalManifest, managedDependency, productFilter, _) in dependencies {
Expand Down Expand Up @@ -591,7 +596,7 @@ extension Workspace {

let rootManifests = try root.manifests.mapValues { manifest in
let deps = try manifest.dependencies.filter { dep in
guard configuration.pruneDependencies else { return true }
// guard configuration.pruneDependencies else { return true }
let enabledTraits = root.enabledTraits[manifest.packageIdentity]
let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits)
return isDepUsed
Expand Down Expand Up @@ -628,7 +633,7 @@ extension Workspace {
// optimization: preload first level dependencies manifest (in parallel)
let firstLevelDependencies = try topLevelManifests.values.map { manifest in
try manifest.dependencies.filter { dep in
guard configuration.pruneDependencies else { return true }
// guard configuration.pruneDependencies else { return true }
let enabledTraits: Set<String>? = root.enabledTraits[manifest.packageIdentity]
let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: enabledTraits)
return isDepUsed
Expand All @@ -647,6 +652,7 @@ extension Workspace {
GraphLoadingNode,
PackageIdentity
>] = { node in
// observabilityScope.emit(warning: "entering with \(node.item.manifest.displayName) and enabled traits: \(node.item.enabledTraits)")
// optimization: preload manifest we know about in parallel
let dependenciesRequired = try node.item.manifest.dependenciesRequired(
for: node.item.productFilter,
Expand All @@ -662,8 +668,10 @@ extension Workspace {
observabilityScope: observabilityScope
)
dependenciesManifests.forEach { loadedManifests[$0.key] = $0.value }
// observabilityScope.emit(warning: "deps: \(dependenciesGuarded.count + dependenciesRequired.count)")
return try (dependenciesRequired + dependenciesGuarded).compactMap { dependency in
try loadedManifests[dependency.identity].flatMap { manifest in
return try loadedManifests[dependency.identity].flatMap { manifest in

// we also compare the location as this function may attempt to load
// dependencies that have the same identity but from a different location
// which is an error case we diagnose an report about in the GraphLoading part which
Expand All @@ -672,9 +680,11 @@ extension Workspace {
guard let conditionTraits = $0.condition?.traits else {
return true
}
return !conditionTraits.intersection(node.item.enabledTraits).isEmpty
return !conditionTraits.intersection(node.item.enabledTraits ?? []).isEmpty
}.map(\.name)

let usingTraits = explicitlyEnabledTraits.flatMap { Set($0) }

let calculatedTraits = try manifest.enabledTraits(
using: explicitlyEnabledTraits.flatMap { Set($0) },
node.item.identity.description
Expand All @@ -686,7 +696,7 @@ extension Workspace {
identity: dependency.identity,
manifest: manifest,
productFilter: dependency.productFilter,
enabledTraits: calculatedTraits ?? []
enabledTraits: calculatedTraits
),
key: dependency.identity
) :
Expand All @@ -699,14 +709,15 @@ extension Workspace {

do {
let manifestGraphRoots = try topLevelManifests.map { identity, manifest in
let isRoot = manifest.packageKind.isRoot
let enabledTraits = isRoot ? root.enabledTraits[identity] : []
// TODO bp: not sure if this is correct
// let isRoot = manifest.packageKind.isRoot
// let enabledTraits = isRoot ? root.enabledTraits[identity] : []
return try KeyedPair(
GraphLoadingNode(
identity: identity,
manifest: manifest,
productFilter: .everything,
enabledTraits: enabledTraits ?? []
enabledTraits: root.enabledTraits[identity]
),
key: identity
)
Expand All @@ -718,7 +729,10 @@ extension Workspace {
) {
allNodes[$0.key] = $0.item
} onDuplicate: { old, new in
allNodes[old.key]?.enabledTraits.formUnion(new.item.enabledTraits)
if let enabledTraits = new.item.enabledTraits {
// TODO bp: this may not be necessary anymore since we pre-compute all enabled and transitively enabled traits...?
// allNodes[old.key]?.enabledTraits?.formUnion(enabledTraits)
}
}
}

Expand Down
Loading