[6.2] Cherry-pick: Introduce richer EnabledTrait model (#9269)
#9349
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…read-safe enabled traits computations (swiftlang#9269) This PR introduces the `EnabledTrait` model and associated infrastructure to properly track and manage trait enablement throughout the dependency resolution process. The new system replaces simple string-based trait tracking with a rich model that maintains provenance information about how and why each trait was enabled. Fixes swiftlang#9286 ## Core Changes ### New `EnabledTrait` Model (EnabledTrait.swift) Introduces three main components for tracking enabled traits for packages: **EnabledTrait** - Represents an individual enabled trait with: - Trait name (used as the identifier) - Set of `Setter` values tracking how the trait was enabled (via command-line config, parent package, or another trait) - Support for unifying traits with the same name by merging their setter lists - it's important to note that this is usually done with an `EnabledTrait` for a specific package, and that many packages can define their own (unique) trait that shares a name with a trait from another package but are treated as fundamentally different instances. **EnabledTraits** - Collection wrapper around `IdentifiableSet<EnabledTrait>` that: - Automatically unifies traits with the same name when inserted - Provides convenient set operations (union, intersection) - Maintains enablement metadata while treating traits with the same name as equal (merges `Setters` for traits of the same name) **EnabledTraitsMap** - Thread-safe map storing enabled traits per package that: - Maps `PackageIdentity` to `EnabledTraits`, while maintaining a thread-safe storage box of its properties to accommodate for loading manifests in parallel - Implicitly returns `["default"]` for packages with no explicitly enabled traits - Uses union behaviour: setting traits for the same package multiple times accumulates all traits - Tracks whether default traits have been explicitly disabled by a parent package or trait configuration (set with `[]`). - Tracks parent packages that request `default` trait usage for a package dependency (whether it's been requested through explicitly listing `default` as a trait, or if a trait specification is omitted) The introduction of these models have also provided some API conveniences: **String Interoperability** - `EnabledTrait` and `EnabledTraits` provide seamless string integration: - `ExpressibleByStringLiteral` and `ExpressibleByArrayLiteral` for natural initialization of enabled traits and sets of enabled traits: `let enabledTrait: EnabledTrait = "foo"` or `let enabledTraits: EnabledTraits = ["foo", "bar"]` - Bidirectional string equality: `enabledTrait == "foo"` and `"foo" == enabledTrait` both work - `EnabledTraitConvertible` protocol allows APIs to accept string collections and auto-convert them to `EnabledTrait` **Collection Protocol Support** - `EnabledTraits` conforms to `Collection` with set-like operations (union, intersection, contains) and overloaded methods that accept either `Collection<String>` or `Collection<EnabledTrait>`, enabling flexible APIs that work with both types ## Refactored Trait Handling (`Manifest+Traits.swift`) - Replaced all `Set<String>` trait representations with richer `EnabledTraits` model - Updated validation methods to work with `EnabledTrait` instead of raw strings - Removed parentPackage parameters from validation (now tracked via `EnabledTrait.Setter`) - Improved type safety and metadata tracking throughout trait calculation ### `Workspace+Traits.swift` (new file) - Extracted trait-specific workspace operations - `updateEnabledTraits(for:)` method determines enabled traits for loaded manifests - `updateEnabledTraits(forDependency:_:)` handles dependency-specific trait propagation ## Bug Fixes ### Required Dependencies Calculation **Problem**: During PubGrub dependency resolution, enabled traits were being reset to defaults instead of propagating previously calculated values when creating `DependencyResolutionNode` instances, which relied on an accurate set of `enabledTraits`. **Impact**: Wrong trait sets were considered when evaluating required dependencies, leading to incorrect dependency graphs. **Solution**: Properly propagate enabled traits through the `nodes()` method for `PackageContainerConstraint`, which creates instances of `DependencyResolutionNode` from these constraints during PubGrub dependency resolution. ### IdentifiableSet Intersection **Problem**: The intersection method created an empty set instead of starting with self, causing loss of element data. **Solution**: Changed `var result = Self()` to `var result = self` in `IdentifiableSet.swift:86`, ensuring proper preservation of element data during intersections. - Separated traits tests into dedicated files: - `ModulesGraphTests+Traits.swift` - `WorkspaceTests+Traits.swift` - `EnabledTraitTests.swift` ## Additional Improvements - Enhanced `IdentifiableSet` with `remove(id:)` and `remove(_:)` methods - Added `Workspace+Traits.swift` for trait-specific workspace operations - Updated all package container implementations for proper trait handling
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please test windows |
1 similar comment
Contributor
Author
|
@swift-ci please test windows |
Contributor
Author
|
@swift-ci please test |
Contributor
Author
|
@swift-ci please test windows |
Contributor
Author
|
Seems like an unrelated issue with the temporary file system, failure recorded with test Triggering windows builds again. @swift-ci please test windows |
Contributor
Author
|
@swift-ci please test self hosted windows |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation:
A new model to handle the computation of enabled traits during resolution has been implemented to remove redundant calculations done throughout the parallel loading of manifests and to handle trait-specific calculations (handling of the
defaulttrait, flattening traits and their transitively enabled traits, tracking parent packages that have disabled default traits for a dependency, etc.). This new model is thread-safe and addresses the flakiness of re-calculating the additive list of enabled traits for a given package when loading multiple manifests in parallel.Scope:
Addresses high-priority bugs related to traits that would omit transitive dependencies despite having their default traits enabled; removes redundant transitive trait computation, allowing for simpler debugging for traits in the future. Also addresses some other bugs found in
IdentifiableSet.Original PRs:
Introduce richer
EnabledTraitmodel to handle thread-safe enabled traits computations #9269Risk:
Moderate risk as there have been significant changes to the traits computation logic as well as an introduction of a new model to handle this. Multiple tests have been added to assert that the new model handles traits in the same way the system did beforehand to accommodate for the large change and some new tests have been added to assert that the traits bugs this was initially addressing are also working as expected.
This new system will allow for simpler debugging where traits are concerned moving forward, as the computations are more centralized to a singular method (
updateEnabledTraitsFor) and theEnabledTraitmodels handle trait-specific cases when tracking enabled traits per package.Testing:
A new test suite with ~50 tests to assert that the
EnabledTraitmodel works as expected, handling edge cases. Integration of this model is also tested in theWorkspaceTests+Traitsas well asModulesGraphTests+Traits.Reviewers:
@dschaefer2