Warn about effectful expectations.

[grynspan]

This PR adds diagnostics (emitted at macro expansion time) when an effectful expression is passed to #expect() or #require(). For example:

 #expect(try foo()) // ⚠️ Expression 'try foo()' will not be expanded on failure;
                   // move the throwing part out of the call to '#expect(_:_:)'

(A fix-it here is not possible for the same reasons we need to diagnose in the first place, explained momentarily.)

Expressions containing try or await are affected; the diagnostic can be suppressed by adding an explicit cast to the expression's type (as Bool or as T?.)

Why can't we break down these expressions?

The try and await keywords in Swift are allowed to be used anywhere in an expression or a containing expression and cover all throwing/asynchronous subexpressions. For example, the following is valid even though only foo() strictly needs the await keyword:

func foo() async -> Int { ... }
 #expect(await quux(1, 2, 3, foo() + bar()) > 10)

Because swift-testing can only see the syntax tree (that is, the characters the developer typed into a Swift source file) and not the types or effects of expressions, when presented with the #expect() expression above, it has no way to know that the only part of the expression that needs to be awaited is foo().

Expression expansion works by breaking down an expression into known subexpression patterns. For example, x.y(z: 123) represents a member function call and useful subexpressions include x, and the argument z: 123:

__checkFunctionCall(
  x, // the base expression
  calling: { $0.y(z: $1) }, // a closure that invokes the .y(z:) member function
  123 // the argument, labelled 'z', to the member function
)

These subexpressions can then be presented as their source code and runtime values if an expectation fails, allowing developers to quickly see that e.g. x was misspecified or 123 should have been 456.

But if some subexpression is effectful, there's no way for swift-testing to break down the whole expression into syntactically and contextually correct subexpressions because there's no way to know where the effects need to be reapplied. Given the similar expression await x.y(z: 123), where does await need to go when calling __checkFunctionCall()?

await __checkFunctionCall(
  x, // should this be `await x`?
  calling: { $0.y(z: $1) }, // `{ await $0.y(z: $1) }` perhaps?
  123 // well, at least this is an integer literal...
)

If the await is placed in the wrong location, an error occurs after macro expansion. If swift-testing is paranoid and adds await to every subexpression (literals aside), warnings occur. Diagnostics occur no matter what we do unless every subexpression just so happened to be effectful.

What about that __requiringAwait trick used during expansion of @Test?

(See here and here.)

This is a tempting approach, but it comes with a serious caveat: it would introduce additional suspension points to code that might only need a single one. The result would be code that behaves differently in a call to #expect() than when invoked directly, which would be a serious defect in swift-testing.

What about just doing that for try?

I had almost gotten this working, but ran into the problem that macros behave differently from functions in a way that would make an expansion syntactically incorrect:

 #expect(try foo()) // 🛑 Call can throw but is not marked with 'try'

In effect, adding the expansion here would require that the developer always write try #expect(try foo()) (i.e. try twice) which is inconsistent with how the developer would write a function call that takes the result of foo() in a non-obvious way.

Okay, so where does that leave us?

This PR adds the diagnostics I mentioned above, remember?

I also took the time to adjust the other diagnostics we emit to more closely match Swift/LLVM house style. So the diff is more extensive than was necessary just for the new diagnostics, but the result is a more consistent developer experience.

Resolves rdar://124976452.

Checklist:

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

[grynspan]

@swift-ci please test

[grynspan]

See also #162.

[grynspan]

@swift-ci please test

[grynspan]

@swift-ci please test Linux

[grynspan]

@swift-ci please test

[suzannaratcliff]

This will be super helpful! Thanks!

[tevelee]

Should/can we convert this warning into a fix-it diagnostic? move the throwing/asynchronous part out of the call to '#expect(_:_:)'

[grynspan]

It is not possible to provide a valid fix-it because we don't know what part of the expression requires the effects. We could provide a fix-it that moves the entire expression out, but that would just behave identically to how #expect behaves in this context, so it wouldn't really fix anything.

[tevelee]

I think these conversions should work. (took a few examples from your unit tests)

"#expect(try foo() as Bool)" -> "try #expect(foo() as Bool)
"#expect(await foo() as Bool)" -> "await #expect(foo() as Bool)
"#expect(try await foo(try await bar()) as Bool)" -> "try await #expect(foo(bar()) as Bool)
"#expect(try foo() as T?)" -> "try #expect(foo() as T?)
"#expect(await foo() as? T)" -> "await #expect(foo() as? T)
"#expect(try await foo(try await bar()) as! T)" -> "try await #expect(foo(bar()) as! T)
"#expect((try foo()) as T)" -> "try #expect((foo()) as T)
"#expect((await foo()) as T)" -> "await #expect((foo()) as T)
"#expect((try await foo(try await bar())) as T)" -> "try await #expect((foo(bar())) as T)
"#expect(try (await foo()) as T)" -> "try await #expect((foo()) as T)

The body of an #expect call is a single expression, right? As long as there are no embedded closures or try?s or try!s in there I think it should be fine.

TSPL documents this behavior under the 'Expressions' section and it works for both throwing (try) and async (await) effects:

// try applies to both function calls
sum = try someThrowingFunction() + anotherThrowingFunction()

[grynspan]

Most if not all of those examples will fail to compile because the try and await keywords are not visible during macro expansion, so they cannot be reasoned about during said expansion. If we could see them, we'd still need to diagnose them the same way as other uses are diagnosed per this PR.

[tevelee]

I think I get it now. The complexity is around the macro expansion and not the compilation of the effects. Thanks!