Lower some symbols to SPI. #548
Conversation
This PR lowers some API symbols to SPI, mostly as experimental tools-only. These symbols are, we believe, important but deserve more thought and design before we promote them to API. As well, this PR merges the `comment` and `comments` properties of `ConditionTrait` as redundant. Resolves rdar://131937376.
grynspan
added
tools integration
grynspan
requested review from
SeanROlszewski,
briancroom,
dennisweissmann,
stmontgomery and
suzannaratcliff
as code owners
|
@swift-ci please test |
|
@swift-ci please test |
…, which is nonsensical--expose the ID type opaquely instead
|
@swift-ci please test |
| // See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
| // | ||
|
|
||
| @_spi(ForToolsIntegrationOnly) |
There was a problem hiding this comment.
I'm worried about this change -- making the Identifiable conformance public and also making its ID type public. Test.Case.ID is not yet fully designed and, in some scenarios will not actually uniquely identify a test case. When its argumentIDs property is nil (which is the case if any argument is non-Codable or equivalent), it will be considered equal to all other instances representing non-Codable arguments. This could be confusing to users or lead to bugs.
This is a known limitation of this ID type currently which we need to come back and fix, but the current tool adopters already use it properly with that consideration in mind. But if we make this change it will publish this ID type as API and I don't think it is mature or robust enough yet.
The existing solution was to wrap this entire extension in @_spi. Can you elaborate on why that is problematic? A runtime as? Identifiable cast may succeed, but since the concrete ID type is not public, I did not think this would be a problem.
There was a problem hiding this comment.
Protocol conformances are always as visible as the type/protocol in question. See here: https://developer.apple.com/documentation/testing/test/case#relationships
If we don't want the conformance to be public, we'll have to remove it outright.
There was a problem hiding this comment.
Behold:
func idForTestCase(_ testCase: Test.Case) -> some Hashable {
func thunk(_ x: some Identifiable) -> some Hashable {
x.id
}
return thunk(testCase)
}Contrived, but the point is the IDs are publicly accessible already.
There was a problem hiding this comment.
Actually, due to opaque types, this is valid even when everything's SPI and you haven't imported it:
let x = Test.Case.current!.id
let y = Test.Case.current!.id
#expect(x == y)So yeah, we need to pick a side of the fence to be on here.
There was a problem hiding this comment.
Yeah, let's just remove the Identifiable conformance
With further consideration after #548, we've decided to remove conformance to `Identifiable` from `Test.Case` rather than promoting it to API. The identity of a test case is fuzzy because it depends on the arguments to a test, not all of which may be uniquely identifiable. We need more time to consider the design of this part of the interface before making it a permanent part of our API.
With further consideration after #548, we've decided to remove conformance to `Identifiable` from `Test.Case` rather than promoting it to API. The identity of a test case is fuzzy because it depends on the arguments to a test, not all of which may be uniquely identifiable. We need more time to consider the design of this part of the interface before making it a permanent part of our API.
With further consideration after #548, we've decided to remove conformance to `Identifiable` from `Test.Case` rather than promoting it to API. The identity of a test case is fuzzy because it depends on the arguments to a test, not all of which may be uniquely identifiable. We need more time to consider the design of this part of the interface before making it a permanent part of our API. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
This PR lowers some API symbols to SPI, mostly as experimental tools-only. These symbols are, we believe, important but deserve more thought and design before we promote them to API.
As well, this PR merges the
commentandcommentsproperties ofConditionTraitas redundant.Resolves rdar://131937376.
Checklist: