[NFC] Replace PackageConditionProtocol with PackageCondition
#7117
Conversation
This file contains 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
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@swift-ci test windows |
|
@swift-ci test |
|
@swift-ci test windows |
|
@swift-ci test windows |
1 similar comment
|
@swift-ci test windows |
|
@swift-ci test linux |
|
@swift-ci test linux |
|
Marking as draft until "Linux (smoke test)" job is fixed. |
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
MaxDesiatov
added
no functional change
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
Multiple places in our codebase pass around and store `[any PackageConditionProtocol]` arrays of existentials with `FIXME` notes that those should be sets and not arrays. Adding `Hashable` requirement to `PackageConditionProtocol` doesn't fix the issue, since existentials of this protocol still wouldn't conform to `Hashable`. A simple alternative is to create `enum PackageCondition` with cases for each condition type. We only have two of those types and they're always declared in the same package. There's no use case for externally defined types for this protocol. That allows us to convert uses of `[any PackageConditionProtocol]` to `Set<PackageCondition>`. Existing protocol kept around until clients of SwiftPM can migrate to the new `PackageCondition` enum.
MaxDesiatov
force-pushed
the
maxd/package-condition
branch
from
37c0f41 to
a6760d9
Compare
|
@swift-ci test |
|
@swift-ci test windows |
1 similar comment
|
@swift-ci test windows |
MaxDesiatov
added
the
no functional change
MaxDesiatov
changed the title
PackageCondition to become value types
MaxDesiatov
added a commit
that referenced
this pull request
Dec 3, 2023
Depends on #7117, unblocks #7160. The `PackageModel` module has no business of defining supported platforms computation for resolved targets, products, and packages. This belongs to `PackageGraph`, but was previously leaking out into `PackageModel` by passing the `derivedXCTestPlatformProvider` closure around. That prevented us from converting marking `SupportedPlatforms` as `Hashable` and marking any of `Resolved*` types as value types and also `Hashable`. In the future, when we'll also need to make them `Sendable`, this could become problematic, passing so much state captured in a closure, mostly by accident.
MaxDesiatov
changed the title
PackageCondition to become value typesPackageConditionProtocol with PackageCondition
tomerd
approved these changes
Dec 5, 2023
| } | ||
| } | ||
|
|
||
| public var platformsCondition: PlatformsCondition? { |
There was a problem hiding this comment.
These are used below in .compactMap(\.configurationCondition) replacing previous .compactMap({ $0 as? ConfigurationCondition }).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Unblocks
PackageGraphvalue types refactoring in #7160 and macros cross-compilation fix in #7118.Multiple places in our codebase pass around and store
[any PackageConditionProtocol]arrays of existentials withFIXMEnotes that those should be sets and not arrays.We also can't convert types containing these arrays to value types with auto-generated
Hashableconformance, since neither elements of these arrays nor arrays themselves areHashable. AddingHashablerequirement toPackageConditionProtocoldoesn't fix the issue, since existentials of this protocol still wouldn't conform toHashable.A simple alternative is to create
enum PackageConditionwith cases for each condition type. We only have two of those types and they're always declared in the same module. There's no use case for externally defined types for this protocol. That allows us to convert uses of[any PackageConditionProtocol]to[PackageCondition]. Existing protocol is kept around until clients of SwiftPM can migrate to the newPackageConditionenum.This also allows us to remove a custom conformance to
HashableonResolvedTarget, unblocking a conversion ofResolvedTargetto a value type and making itSendablein the future.