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

Value not shown in error message when an expectation condition has effects (try/await) #162

Open
tevelee opened this issue Dec 15, 2023 · 10 comments · May be fixed by #840
Open

Value not shown in error message when an expectation condition has effects (try/await) #162

tevelee opened this issue Dec 15, 2023 · 10 comments · May be fixed by #840
Assignees
Labels
concurrency Swift concurrency/sendability issues enhancement New feature or request issue-handling Related to Issue handling within the testing library public-api Affects public API
Milestone

Comments

@tevelee
Copy link

tevelee commented Dec 15, 2023

Description

#expect works differently when await keyword is inside its body. await #expect(expr) shows a breakdown of the actual value, while #expect(await expr) does not. I would like to know why the expectation failed even if the result is computed from an async expression.

image

Expected behavior

#expect(await Array(stream) == [1,2]) throws the error testAll(): Expectation failed: (Array(stream) → [1, 2, 3]) == ([1,2] → [1, 2])

Actual behavior

#expect(await Array(stream) == [1,2]) throws the error testAll(): Expectation failed: await Array(stream) == [1,2]

Steps to reproduce

No response

swift-testing version/commit hash

3fa4ea0

Swift & OS version (output of swift --version && uname -a)

swift-driver version: 1.87.3 Apple Swift version 5.9.2 (swiftlang-5.9.2.2.56 clang-1500.1.0.2.5)
Target: arm64-apple-macosx14.0
Darwin MBP 23.1.0 Darwin Kernel Version 23.1.0: Mon Oct 9 21:27:24 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T6000 arm64

@tevelee tevelee added the enhancement New feature or request label Dec 15, 2023
@grynspan
Copy link
Contributor

This is expected and is a constraint of macros in Swift. The presence of the await keyword signals to the compiler that some subexpression of the expectation is asynchronous, but it is impossible to know exactly which subexpression from the AST alone. This means it is not possible to correctly decompose the expression as we can with synchronous, non-throwing ones.

@grynspan grynspan added the wontfix This will not be worked on label Dec 15, 2023
@grynspan grynspan closed this as not planned Won't fix, can't repro, duplicate, stale Dec 15, 2023
@tevelee
Copy link
Author

tevelee commented Dec 21, 2023

@grynspan Thanks! If I understand the issue correctly, for a test case like

@Test func testAsyncExpectation() {
  func value() async -> Int { 1 }
  #expect(await value() == 2)
}

the expect macro is going to resolve to a __checkValue (generic boolean expression) and not a __checkBinaryOperation (where we would be able to decompose actual and expected values on lhs and rhs respectively).

Can't we work around this in private func _parseCondition by not just looking for InfixOperatorExprSyntax but anything embedded in an AwaitExprSyntax? In this case:

AwaitExprSyntax
├─awaitKeyword: keyword(SwiftSyntax.Keyword.await)
╰─expression: InfixOperatorExprSyntax
  ├─leftOperand: FunctionCallExprSyntax
  │ ├─calledExpression: DeclReferenceExprSyntax
  │ │ ╰─baseName: identifier("value")
  │ ├─leftParen: leftParen
  │ ├─arguments: LabeledExprListSyntax
  │ ├─rightParen: rightParen
  │ ╰─additionalTrailingClosures: MultipleTrailingClosureElementListSyntax
  ├─operator: BinaryOperatorExprSyntax
  │ ╰─operator: binaryOperator("==")
  ╰─rightOperand: IntegerLiteralExprSyntax
    ╰─literal: integerLiteral("2")

Also, how is this issue different from matching a try expression?

@grynspan
Copy link
Contributor

grynspan commented Dec 21, 2023

The same issue exists for a try expression.

Imagine we add support for an AwaitExprSyntax path. What would the implementation look like? It would need to preserve await semantics on all parts of the expanded expression, but it is impossible to know which parts need to be awaited (and which are synchronous) just from the syntax tree alone.

This means we'd need additional overloads of every __check function that are async, throws, and async throws. This quadruples the number of overloads that need to be resolved (which has a worse-than-linear impact on compile times), and it is semantically incorrect because we'd have to introduce multiple fake suspension points that could affect the correctness of a test.

The expanded form of #expect(await x == y) would look something like:

__checkBinaryOperation(
  await (x, Testing.__requiringAwait).0,
  { await $0 == $1() },
  await (y, Testing.__requiringAwait).0,
  ...
).__expected()

And would have at least 4 and at most 6 suspension points when the original expression had as few as 1.

@grynspan
Copy link
Contributor

Now, it may be possible to simplify that expansion a bit given that the outermost call to #expect() would need an await keyword applied to it, but the effect keywords on a macro are not visible to the macro during expansion, so we can't know if the developer typed it or not. We could place our own await keyword on the call to __check(), and that could let us simplify part of (but not all of) the macro expansion, but we're still left with four times as many overloads as before. rethrows (and a hypothetical reasync) doesn't help us because we must express the infix operator (among other possible subexpressions) as a closure, and that requires us to explicitly write try and/or await within the closure body, and that defeats the purpose of rethrows/reasync.

@tevelee
Copy link
Author

tevelee commented Dec 21, 2023

Thank you for the detailed explanation. I understand the tradeoffs now.

We could place our own await keyword on the call to __check(), and that could let us simplify part of (but not all of) the macro expansion, but we're still left with four times as many overloads as before.

This seems to be the only viable solution to the issue.

@grynspan
Copy link
Contributor

The issue can be avoided by extracting the async subexpression out into a separate expression:

let y = await foo()
#expect(x == y)

So I'd steer developers toward that solution as preferable.

@tevelee
Copy link
Author

tevelee commented Mar 21, 2024

I appreciate the writeup and the warning in #302. Thank you @grynspan

@grynspan
Copy link
Contributor

grynspan commented Oct 9, 2024

Reopening. Now that we have #isolation, it may be possible to avoid the unnecessary hops.

@grynspan grynspan reopened this Oct 9, 2024
@grynspan grynspan added concurrency Swift concurrency/sendability issues public-api Affects public API issue-handling Related to Issue handling within the testing library and removed wontfix This will not be worked on labels Oct 9, 2024
@grynspan grynspan added this to the Swift 6.1 milestone Oct 9, 2024
@grynspan grynspan changed the title Value not shown in error message when calling async variant of #expect Value not shown in error message when an expectation condition has effects (try/await) Oct 9, 2024
@grynspan
Copy link
Contributor

grynspan commented Oct 9, 2024

Blocked by swiftlang/swift#76930.

grynspan added a commit that referenced this issue Oct 30, 2024
… present.

For reasons that have been documented at length (see #162), we aren't able to
correctly expand conditions on `#expect()` or `#require()` that have effects
(`try` or `await`.) We aren't currently detecting all possible patterns that
expand incorrectly. This PR causes us to back out of the full expansion if the
`try` or `await` keyword is present _anywhere_ inside an expectation condition
expression.

Resolves #783.
grynspan added a commit that referenced this issue Nov 5, 2024
… present. (#790)

For reasons that have been documented at length (see #162), we aren't
able to correctly expand conditions on `#expect()` or `#require()` that
have effects (`try` or `await`.) We aren't currently detecting all
possible patterns that expand incorrectly. This PR causes us to back out
of the full expansion if the `try` or `await` keyword is present
_anywhere_ inside an expectation condition expression.

Resolves #783.

### 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.
@grynspan grynspan modified the milestones: Swift 6.1, Swift 6.x Nov 19, 2024
@grynspan
Copy link
Contributor

grynspan commented Dec 2, 2024

Tracked internally as rdar://137583415.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Swift concurrency/sendability issues enhancement New feature or request issue-handling Related to Issue handling within the testing library public-api Affects public API
Projects
None yet
2 participants