Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution #8852

bripeticca · 2025-06-19T18:34:35Z

Trait-guarded dependencies were still being considered during dependency resolution, when they should be excluded if they aren't being used in any other scenario. Additionally, enabled traits should be pre-computed before entering the resolution stage in order to avoid possible race conditions when navigating through the dependency graph nodes.

Modifications:

Since we have the --experimental-prune-unused-dependencies feature behind an experimental flag, we'll now consider an alternate path that will prune trait-guarded package dependencies from the dependency graph if and only if said dependency is not used in any other unguarded context.

A dictionary wrapper titled EnabledTraitsMap is used to store the enabled traits per package for the package graph and is stored as a property within the Workspace, with some additional behaviour to return a ["default"] set of traits if the package has not yet been stored in the dictionary, rather than returning nil.

Following this behaviour, when passing a set of traits to methods that require them (e.g. for dependency computation, enabled trait computation, etc.) we now require that it is not Optional, since the checks done on a nil set of traits were redundant.

SwiftCommandState now also stores a TraitConfiguration, since we'll want access to this across multiple SwiftCommands, and it is essentially a part of the state anyhow. TraitOptions is now included in the GlobalOptions for SwiftCommands to supplement this, so that when a SwiftCommandState is created we will have access to the user-passed enabled traits. These options, as entitled, are available globally across all the swift package commands, so previous properties that declared TraitOptions in these commands has been removed in favour of using it straight from the GlobalOptions.

Result:

Trait-guarded dependencies are excluded from dependency resolution, and since traits are pre-computed there should no longer be an issue with race conditions for traits in resolution as well.

bripeticca · 2025-06-19T20:40:27Z

@swift-ci please test

- Create a 'precomputeTraits' method that cycles through the graph to unify all the traits - Modify the traits error message to include both the package identity and display name for better readability TODO - change instances of optional enabledTraits to non-optional, defaulting instead to ["default"] - assure that loaded manifests arent fetching guarded dependencies - this is contingent on how we are calculating the unified traits afterwards

Since there is a bit of redundancy when checking the conditions of an optional set of enabled traits, it makes more sense to simply have the set of enabled traits be non-optional, and to derive the checks for defaults by asserting whether the set contains the "default" keyword.

To help with the computation of traits, a wrapper struct for a dictionary of traits has been created wherein the default value when a key isn't found is no longer nil but instead ["default"], since if we haven't defined a set of explicitly enabled traits for a given package, then it should default to the defaults.

* Whenever this method is called, it will store a previously- created Workspace object in the SwiftCommandState. Some calls to this method were missing the traitConfiguration, thus instantiating the Workspace without the necessary trait config.

bripeticca · 2025-07-09T15:49:09Z

@swift-ci please test

- Previous calls to getActiveWorkspace could have defaulted to using the default trait configuration (re: PluginCommand -> PluginDelegate.createSymbolGraphForPlugin case), therefore we should update the existing workspace's trait configuration if a new one is passed.

Since the trait configuration is used for a variety of swift package subcommands, it makes it a lot easier to add the trait options as a part of the GlobalOptions which is consumed by SwiftCommandState and persisted across all SwiftCommands. This also allows us to omit passing a trait configuration to the build system factory methods, and instead replacing it with a flag that can override the trait configuration to enable all traits for edge-case scenarios (e.g. fetching symbol graphs)

bripeticca · 2025-07-10T19:14:47Z

@swift-ci please test

bripeticca · 2025-07-10T20:24:48Z

@swift-ci please test

bripeticca · 2025-07-11T17:41:35Z

@swift-ci please test

bripeticca · 2025-07-15T19:10:03Z

@swift-ci please test

FranzBusch

Amazing! So great to see unused dependencies getting pruned

FranzBusch · 2025-07-16T12:06:23Z

Sources/PackageModel/EnabledTraitsMap.swift

+//===----------------------------------------------------------------------===//
+
+/// A wrapper for a dictionary that stores the transitively enabled traits for each package.
+public struct EnabledTraitsMap: ExpressibleByDictionaryLiteral {


Does this need to be public?

I'll double check, it may not need to be

Ah yeah this does need to be public since it's used in public initializers in other packages.

Since we'll have others consuming the ModulesGraph load method, create a version of the method without the enabledTraitsMap parameter.

bripeticca · 2025-07-16T16:15:17Z

@swift-ci please test

bripeticca · 2025-07-16T17:13:21Z

@swift-ci please test windows

bkhouri · 2025-07-16T19:32:19Z

Sources/Commands/PackageCommands/DumpCommands.swift

@@ -53,7 +53,7 @@ struct DumpSymbolGraph: AsyncSwiftCommand {
        // We turn build manifest caching off because we need the build plan.
        let buildSystem = try await swiftCommandState.createBuildSystem(
            // We are enabling all traits for dumping the symbol graph.
-            traitConfiguration: .init(enableAllTraits: true),
+            enableAllTraits: true,


question: what benefit does passing this argument have on the creation of a build system? Should the enableAllTraits option be provided when building instead?

This will affect how the SwiftCommandState fetches and stores the currently active workspace, which is determined by its method SwiftCommandState.getActiveWorkspace. When creating a build system, there are many calls to various SwiftCommandState methods that call getActiveWorkspace, and the workspace that is created in this method is stored in its _workspace property.

The workspace is only ever created once in this method, and every subsequent call to this method checks whether the _workspace property has a value. If so, it will return the workspace stored there.

The reason why we have enableAllTraits propagated to this call is because we have special edge case scenarios where we would initially create a SwiftCommandState with some trait configuration, but if we then make a call that wishes to dump the symbol graph (see PluginDelegate.createSymbolGraphForPlugin), it will return a workspace that has whichever trait configuration we've initialized with the SwiftCommandState, which isn't always what we'd want when requesting a dump of the symbol graph.

bkhouri · 2025-07-16T19:38:28Z

Sources/CoreCommands/SwiftCommandState.swift

@@ -292,6 +292,8 @@ public final class SwiftCommandState {

    package var preferredBuildConfiguration = BuildConfiguration.debug

+    public var traitConfiguration: TraitConfiguration


question: can this be a let instead of var?

Indeed it can!

bkhouri · 2025-07-16T20:24:50Z

Sources/PackageDescription/PackageDependency.swift

@@ -56,7 +56,7 @@ extension Package {
        public let kind: Kind

        /// The dependencies traits configuration.
-        @available(_PackageDescription, introduced: 999.0)
+        @available(_PackageDescription, introduced: 6.1)


praise: nice!

bkhouri · 2025-07-16T20:29:52Z

Tests/FunctionalTests/TraitTests.swift

                ],
                buildSystem: buildSystem,
            )
            // We expect no warnings to be produced. Specifically no unused dependency warnings.
            let unusedDependencyRegex = try Regex("warning: '.*': dependency '.*' is not used by any target")
            #expect(!stderr.contains(unusedDependencyRegex))
+            if buildSystem == .swiftbuild {
+                print(stderr)


question: out of curiosity, why are we printing stderr here? if it's meant to troubleshoot a failed expectation, we can add a comment to the #expect(...) or #require(...)

See the expect(::sourcelocation:) API docs for details.

Ah, this must've slipped by me. I was printing out stderr for debugging purposes. Thanks for catching this!

bripeticca · 2025-07-16T21:02:02Z

@swift-ci please test

dschaefer2 · 2025-07-17T03:21:21Z

@swift-ci please test windows

…compute enabled traits before resolution (swiftlang#8852) Trait-guarded dependencies were still being considered during dependency resolution, when they should be excluded if they aren't being used in any other scenario. Additionally, enabled traits should be pre-computed before entering the resolution stage in order to avoid possible race conditions when navigating through the dependency graph nodes. Since we have the `--experimental-prune-unused-dependencies` feature behind an experimental flag, we'll now consider an alternate path that will prune trait-guarded package dependencies from the dependency graph **_if and only if_** said dependency is not used in any other unguarded context. A dictionary wrapper titled `EnabledTraitsMap` is used to store the enabled traits per package for the package graph and is stored as a property within the `Workspace`, with some additional behaviour to return a `["default"]` set of traits if the package has not yet been stored in the dictionary, rather than returning `nil`. Following this behaviour, when passing a set of traits to methods that require them (e.g. for dependency computation, enabled trait computation, etc.) we now require that it is not Optional, since the checks done on a `nil` set of traits were redundant. SwiftCommandState now also stores a `TraitConfiguration`, since we'll want access to this across multiple `SwiftCommand`s, and it is essentially a part of the state anyhow. `TraitOptions` is now included in the `GlobalOptions` for `SwiftCommand`s to supplement this, so that when a `SwiftCommandState` is created we will have access to the user-passed enabled traits. These options, as entitled, are available globally across all the swift package commands, so previous properties that declared `TraitOptions` in these commands has been removed in favour of using it straight from the `GlobalOptions`. Trait-guarded dependencies are excluded from dependency resolution, and since traits are pre-computed there should no longer be an issue with race conditions for traits in resolution as well.

bripeticca added 3 commits June 19, 2025 14:30

Fix for trait-guarded deps being included in resolution

5469b84

Comment cleanup

f67f3df

De-duplicate calculation of transitively enabled traits

0d46a4c

bripeticca added 10 commits June 25, 2025 14:57

Precompute traits and persist to module graph load

e6bc7e6

Fix trait tests to account for removal of guarded deps

6a3d2bb

Move enabled traits map into Workspace

2398e7d

Merge branch 'main' into traitresolutionbug

1b1ba10

Cleanup; logging messages to help debug failing traits tests

adf28f7

Amend plugin command configuration of package graph

cb6723e

bripeticca added 2 commits July 9, 2025 13:24

bripeticca added 4 commits July 10, 2025 15:58

Ensure PackageGraphRoot.init has latest enabledTraitsMap

c0ecd4e

Cleanup

e989978

Move EnabledTraitsMap to its own file in PackageModel

ad0b958

Cleanup remaining comments/whitespace

7e71383

Add EnabledTraitsMap.swift to CMakeList

da91ac0

bripeticca marked this pull request as ready for review July 11, 2025 17:41

bripeticca requested review from AnthonyLatsis, bnbarham, xedin and jakepetroules as code owners July 11, 2025 17:41

bripeticca requested review from dschaefer2, shawnhyam, plemarquand, owenv, bkhouri, cmcgee1024 and daveyc123 as code owners July 11, 2025 17:41

bripeticca requested a review from FranzBusch July 11, 2025 17:41

bripeticca changed the title ~~[WIP] Fix for trait-guarded deps being included in resolution~~ Fix for trait-guarded deps being included in resolution Jul 11, 2025

bripeticca changed the title ~~Fix for trait-guarded deps being included in resolution~~ Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution Jul 11, 2025

Remnant cleanup

886d272

FranzBusch approved these changes Jul 16, 2025

View reviewed changes

bripeticca added 3 commits July 16, 2025 11:15

Fix ModulesGraph load method

9113b3e

Since we'll have others consuming the ModulesGraph load method, create a version of the method without the enabledTraitsMap parameter.

Merge branch 'main' into traitresolutionbug

0b63db4

Fix remaining logs left in for debugging

fcc904e

bkhouri reviewed Jul 16, 2025

View reviewed changes

Address PR comments

7fad68a

dschaefer2 approved these changes Jul 17, 2025

View reviewed changes

bripeticca merged commit 6e3f087 into swiftlang:main Jul 17, 2025
6 checks passed

bripeticca mentioned this pull request Jul 17, 2025

Cherry-pick Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution (#8852) #8948

Open

		@@ -292,6 +292,8 @@ public final class SwiftCommandState {

		package var preferredBuildConfiguration = BuildConfiguration.debug

		public var traitConfiguration: TraitConfiguration

Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution #8852

Assure trait-guarded dependencies are not included in resolution; precompute enabled traits before resolution #8852

Uh oh!

Conversation

bripeticca commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Modifications:

Result:

Uh oh!

bripeticca commented Jun 19, 2025

Uh oh!

bripeticca commented Jul 9, 2025

Uh oh!

bripeticca commented Jul 10, 2025

Uh oh!

bripeticca commented Jul 10, 2025

Uh oh!

bripeticca commented Jul 11, 2025

Uh oh!

bripeticca commented Jul 15, 2025

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bripeticca commented Jul 16, 2025

Uh oh!

bripeticca commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bripeticca commented Jul 16, 2025

Uh oh!

dschaefer2 commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

bripeticca commented Jun 19, 2025 •

edited

Loading