Skip to content
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.

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

Use @Test function parameters to explicitly type array literal argument expressions #808

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stmontgomery
Copy link
Contributor

This solves a problem that users who pass array literals to @Test(arguments: ...) may encounter if the array's elements are heterogeneous. For example:

@Test(arguments: [  // ❌ error: type 'Any' does not conform to the 'Sendable' protocol
  (Int.self, nominalName: "Int"),
  (String.self, nominalName: "String"),
])
func name1(type: Any.Type, nominalName: String) {
  #expect(API.name(type) == nominalName)
}

Due to the use of heterogeneous elements Int.self and String.self, the overall array's type is inferred as [Any]. This leads to two problems:

  1. The @Test(arguments:) macro requires a Collection which is Sendable, and [Any] is not Sendable. This prevents passing the array to the macro at all, since arguments to a macro must typecheck successfully before the macro is expanded.
  2. The test function's parameters are of type Any.Type and String, respectively. For the macro expansion to produce valid code, the array literal needs to have type [(Any.Type, String)].

This PR makes two changes to address those problems:

  • First, it introduces overloads of some of the @Test macro declarations without their Sendable requirements. This solves problem 1 above. (See the considerations below for discussion about why I believe this is safe and reasonable.)
  • Second, it modifies the macro expansion logic to add an as ... cast to array literal expressions passed to arguments: ... (unless they already have one) to provide an explicit type.

Continuing the example above, this results in the expanded code behaving as though the array expression had as [(Any.Type, String)] at the end, and the original code now compiles successfully.

Thank you to @ApolloZhu for suggesting the fix to use as ... in the macro expansion!

Concurrency safety

The expanded code from the new @Test overloads is no less concurrency safe than before, because it still calls APIs from the testing library which require Sendable. This means that passing a non-Sendable collection will still result in a compiler diagnostic, just with a different source location than before:

// With the changes in this PR:
struct NonSendable {}

@Test(arguments: [NonSendable()])  // ❌: @__swiftmacro_12TestingTests6unsafe4TestfMp_.swift:14:4: error: type 'NonSendable' does not conform to the 'Sendable' protocol
func unsafe(_ x: NonSendable) {}

Documentation

The new @Test overloads are necessarily public but are hidden from rendered DocC documentation using @_documentation(visibility: private). From an end user's perspective, @Test(arguments:) did, and still does, require Sendable; the only thing changing is where that enforcement occurs. So in terms of documentation, the only overloads we need to document are those that do require Sendable. In fact, for similar reasons, we already hide many of our @Test overloads from documentation so this has precedent.

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

Fixes swiftlang/swift#76637

@stmontgomery stmontgomery added the enhancement New feature or request label Nov 5, 2024
@stmontgomery stmontgomery added this to the Swift 6.1 milestone Nov 5, 2024
@stmontgomery stmontgomery self-assigned this Nov 5, 2024
@stmontgomery
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@grynspan grynspan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussing off-GitHub. It should be possible to implement this change without needing deep macro changes and, ideally, preserving the "good" diagnostics as much as possible.


return map { argument in
// Only add explicit types below if this is an Array literal expression.
guard argument.expression.is(ArrayExprSyntax.self) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to strip parentheses here.

.map { argument, parameter in
// Only add explicit types below if this is an Array literal
// expression.
guard argument.expression.is(ArrayExprSyntax.self) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to strip parentheses here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, does this branch even need to do anything interesting at all?

/// `as ...` cast.
fileprivate func testArguments(typedUsingParameters parameters: FunctionParameterListSyntax) -> [Argument] {
if count == 1 {
let tupleTypes = parameters.lazy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail to compile if any arguments have specifiers like borrowing. (I'm not sure if that's correct in this position.) You should check the exact type of type node.

fileprivate func testArguments(typedUsingParameters parameters: FunctionParameterListSyntax) -> [Argument] {
if count == 1 {
let tupleTypes = parameters.lazy
.map(\.type)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.trimmedDescription?

@stmontgomery
Copy link
Contributor Author

I've extracted some of the general macro refactoring into a separate PR (#834)

stmontgomery added a commit that referenced this pull request Dec 10, 2024
… macro (#834)

This is a minor refactor of the logic and data structures in the `@Test`
macro implementation responsible for parsing and handling test function
argument expressions.

### Motivation:

I discovered there was a way to simplify this logic while working on
#808, and since this isn't strictly pertinent to that PR I wanted to
extract it and land it separately here.

### Modifications:

- Remove the need to hardcode the parameter label `arguments:` in three
places.
- Remove the need to iterate a Collection using raw indices.
- Resolve one "TODO" comment.

### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any.Type issue with @Test
2 participants