Preserve contextual type for literal arguments: expressions in @Test#1627
Preserve contextual type for literal arguments: expressions in @Test#1627ojun9 wants to merge 19 commits into
arguments: expressions in @Test#1627Conversation
|
Type checking of arguments to the |
grynspan
left a comment
There was a problem hiding this comment.
Additional test coverage is needed too.
| return nil | ||
| } | ||
|
|
||
| if testFunctionArguments.count == 1, expression.is(ArrayExprSyntax.self) { |
There was a problem hiding this comment.
It should be safe to generalize this so that the argument count only needs to match the parameter count.
| } | ||
|
|
||
| let parameters = functionDecl.signature.parameterClause.parameters | ||
| guard !parameters.isEmpty else { |
There was a problem hiding this comment.
| guard !parameters.isEmpty else { | |
| if parameters.isEmpty { |
| private func _contextualTypeForLiteralArgument( | ||
| for expression: ExprSyntax, | ||
| among testFunctionArguments: [Argument] | ||
| ) -> String? { |
There was a problem hiding this comment.
| ) -> String? { | |
| ) -> TypeSyntax? { |
| // type itself, not tuple-shaped elements. | ||
| return "[\(parameter.baseTypeName)]" | ||
| } | ||
| let elementType = parameters.map(\.baseTypeName).joined(separator: ", ") |
There was a problem hiding this comment.
Can we construct an ArrayTypeSyntax here instead and leave the string interpolation to swift-syntax?
| if parameters.count == 1, let parameter = parameters.first { | ||
| // A single-parameter test expects collection elements of the parameter | ||
| // type itself, not tuple-shaped elements. | ||
| return "[\(parameter.baseTypeName)]" |
There was a problem hiding this comment.
Tokens from the original source need to be trimmed.
| arguments += testFunctionArguments.map { argument in | ||
| var copy = argument | ||
| copy.expression = .init(ClosureExprSyntax { argument.expression.trimmed }) | ||
| let argumentExpr = argument.expression.trimmed |
There was a problem hiding this comment.
We can simplify this part of the diff by modifying argumentExpr before creating the closure wrapper.
|
Thanks for the question. You're right that the existing test only verifies the macro expansion. @Test(arguments: [
(nil, 1),
("a", nil)
])
func f(s: String?, i: Int?) {}This case fails with The change addresses the situation where the collection type is already provided, for example via a generic parameter: @Test<[(String?, Int?)]>(arguments: [
(nil, 2),
("c", nil),
("d", nil)
])
func f(s: String?, i: Int?) {}In this situation the collection element type is known, but wrapping the literal inside the generated closure removes the contextual type required for literal inference. The added cast restores that contextual type and allows the code to compile. |
|
I haven't forgotten about this PR, but I have been tied up with other work. We should make sure to handle dictionary literals correctly as well, and can improve test efficiency by inferring them as instances of |
grynspan
left a comment
There was a problem hiding this comment.
We still need some fixture tests in the TestingTests target. See NonSendableTests for an example of how to write one (empty test that triggers this macro expansion code when we compile our own tests so that if the code isn't working, we fail to build). Those tests should go in the TestingTests target.
| // trying to obtain the base type to reference it in an expression. | ||
| let baseType = type.as(AttributedTypeSyntax.self)?.baseType ?? type | ||
| return baseType.trimmedDescription | ||
| return baseType.trimmed |
There was a problem hiding this comment.
Avoid trimming both here and in the call sites as it's somewhat expensive. I would leave it untrimmed here so that it can still be used for diagnostic attribution, but either choice is fine.
| ) | ||
| ) | ||
| } | ||
| let lazyExpression = expr.trimmed |
There was a problem hiding this comment.
Already trimmed by this point. (New nodes created programmatically are de facto trimmed if you don't explicitly specify trivia).
| @Test(.hidden, arguments: [("value", 123)]) | ||
| func one2TupleParameter(x: (String, Int)) {} | ||
|
|
||
| @Test<[(String?, Int?)]>(.hidden, arguments: [ |
There was a problem hiding this comment.
| @Test<[(String?, Int?)]>(.hidden, arguments: [ | |
| @Test(.hidden, arguments: [ |
Generic parameters on @Test are not supported and will diagnose if used.
There was a problem hiding this comment.
I removed the unsupported generic argument clause from @test.
The remaining type annotation is expressed as an explicit cast on the arguments: expression.
This preserves contextual type information (particularly for nil literals) and is consistent with the macro’s supported expansion semantics.
| @Test(.hidden, arguments: ["value": 123]) | ||
| func oneDictionaryElementTupleParameter(x: (key: String, value: Int)) {} | ||
|
|
||
| @Test<KeyValuePairs<String, Int?>>(.hidden, arguments: [ |
There was a problem hiding this comment.
| @Test<KeyValuePairs<String, Int?>>(.hidden, arguments: [ | |
| @Test(.hidden, arguments: [ |
Generic parameters on @Test are not supported and will diagnose if used.
|
Sorry for the delay! I was out sick. |
Co-authored-by: Jonathan Grynspan <grynspan@me.com>
Co-authored-by: Jonathan Grynspan <grynspan@me.com>
Co-authored-by: Jonathan Grynspan <grynspan@me.com>
|
Let me know when you're ready for another review. |
|
@grynspan |
|
If you see the "merge" button, please make sure to squash. Otherwise ping me and I'll click the button. |
…airs`. (#1699) This change ensures that test authors who explicitly pass collections of type `KeyValuePairs<K, V>` as test arguments will get the same sort of optimizations as they would if they passed a `Dictionary<K, V>` instead. This change complements, but is not dependent on, #1627. ### 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.
| return TypeSyntax( | ||
| MemberTypeSyntax( | ||
| baseType: IdentifierTypeSyntax(name: .identifier("Swift")), | ||
| name: .identifier("KeyValuePairs"), |
There was a problem hiding this comment.
I'm surprised to see DictionaryExprSyntax being interpreted as KeyValuePairs. I see this comment which I'm guessing is what led to that:
We should make sure to handle dictionary literals correctly as well, and can improve test efficiency by inferring them as instances of
KeyValuePairsrather thanDictionary.
That changes the semantics some, though: elements with equal keys will run the test multiple times, and the order we dispatch test cases in will be stable, whereas with a dictionary the order is nondeterministic due to hashing.
Is switching to KeyValuePairs necessary for this overall PR, or is it more of an opportunistic performance improvement? It feels like a potentially user-observable behavior change.
There was a problem hiding this comment.
This was my suggestion, yes.
That changes the semantics some, though: elements with equal keys will run the test multiple times
Equal keys crash outright with a dictionary when it is initialized.
and the order we dispatch test cases in will be stable, whereas with a dictionary the order is nondeterministic due to hashing.
This change is more consistent with how we treat arrays, yes.
Is switching to
KeyValuePairsnecessary for this overall PR, or is it more of an opportunistic performance improvement? It feels like a potentially user-observable behavior change.
It isn't strictly necessary, but the only user-observable change is that the random order of elements is no longer random (which we never really intended it to be anyway).
| (nil, 123), | ||
| ("value1", nil), | ||
| ("value2", nil), | ||
| ] as [(String?, Int?)]) |
There was a problem hiding this comment.
Why does this test, and the other new one below, include an explicit as cast on the passed-in collection? I thought this PR was intended to remove the need to specify that at the point where @Test is used?
I just pulled the branch locally, and when I try this example mentioned in the PR description without an as cast and without <...> generic type parameters on @Test, it's still emitting an error:
@Test(arguments: [
(nil, 2), // ❌ error: 'nil' cannot initialize specified type 'String'
("c", nil),
("d", nil)
])
func f(s: String?, i: Int?) {}If the outcome of this PR is that a user still needs to either (a) use an explicit as cast or (b) include generic type parameters on @Test, then I'm not sure what the benefit is. Users can already do (a), and we recently began emitting a warning diagnostic for (b), and it's not meaningfully simpler than option (a) IMO anyway.
stmontgomery
left a comment
There was a problem hiding this comment.
I appreciate your effort on this @ojun9 — I really do want to see this problem resolved so that test authors don't need to provide type information for test arguments explicitly. In fact, I took a stab at it myself a while back in this PR.
I'm concerned though that as written, it doesn't really help much in practice towards that goal. Unless I'm missing something, it seems like it still requires specifying the type of the argument collection explicitly.
If that's not accurate, or if there are scenarios where this is helpful without explicit type info that I'm overlooking, could you please give an example of that?
|
Sorry for the very delayed reply. Thanks, I see your point. The case this PR helps with is one where the type is already provided somehow before expansion, for example: @Test<[(String?, Int?)]>(arguments: [
(nil, 2),
("c", nil),
("d", nil),
])
func f(s: String?, i: Int?) {}In this case, the original arguments: expression has enough context to type-check, but the generated closure loses that context unless the expansion adds as That said, I agree this is still explicit type information at the use site. I don’t currently know of an example where this PR makes the literal work with neither an as cast nor a generic argument clause. |
Preserve contextual type for literal
arguments:expressions in@TestMotivation:
Parameterized tests using
@Test(arguments:)may fail to compile when the collection is written as an array literal containing values that rely on contextual type inference.For example:
During macro expansion, the
arguments:expression is wrapped in a closure so the collection can be evaluated lazily:When the array literal is evaluated inside this closure, the contextual type that would normally be provided by the test function parameters is no longer available. As a result, values such as
nilcannot be inferred and the code fails to type-check.The same code works if the array literal is explicitly cast:
In this case the generated code becomes:
The explicit cast preserves the contextual type inside the closure and allows the literals to type-check.
Modifications:
When
arguments:is supplied as a single array literal, the macro derives the expected array type from the test function parameter list and applies it as an explicit cast inside the generated closure.For example, the generated expression becomes:
This preserves the contextual type required for literal inference while keeping the existing lazy evaluation behavior.
Macro expansion only has access to source syntax and does not observe the inferred collection type directly. For this reason, the array type is derived from the test function parameter list. The change is intentionally limited to the case where
arguments:is a single array literal so that other overloads remain unaffected.Additional tests verify that contextual types are preserved for empty array literals and tuple literals containing optionals.
Checklist: