Warn about effectful expectations. (#302)

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

```swift
 #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:

```swift
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`:

```swift
__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()`?

```swift
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](https://github.com/apple/swift-testing/blob/932cb01aa2da55fb410b8fede294efa8d5545f62/Sources/Testing/Test%2BMacro.swift#L464)
and
[here](https://github.com/apple/swift-testing/blob/932cb01aa2da55fb410b8fede294efa8d5545f62/Sources/TestingMacros/TestDeclarationMacro.swift#L306).)

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:

```swift
 #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](https://github.com/apple/swift/blob/main/docs/Diagnostics.md). 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:

- [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.

Author: grynspan

Sources/TestingMacros/Support/ConditionArgumentParsing.swift

@@ -128,6 +128,64 @@ func removeParentheses(from expr: ExprSyntax) -> ExprSyntax? {
   return nil
 }
 
+/// A class that walks a syntax tree looking for `try` and `await` expressions.
+///
+/// - Bug: This class does not use `lexicalContext` to check for the presence of
+///   `try` or `await` _outside_ the current macro expansion.
+private final class _EffectFinder: SyntaxVisitor {
+  /// The effectful expressions discovered so far.
+  var effectfulExprs = [ExprSyntax]()
+
+  /// Common implementation for `visit(_: TryExprSyntax)` and
+  /// `visit(_: AwaitExprSyntax)`.
+  ///
+  /// - Parameters:
+  ///   - node: The `try` or `await` expression.
+  ///   - expression: The `.expression` property of `node`.
+  ///
+  /// - Returns: Whether or not to recurse into `node`.
+  private func _visitEffectful(_ node: some ExprSyntaxProtocol, expression: ExprSyntax) -> SyntaxVisitorContinueKind {
+    if let parentNode = node.parent, parentNode.is(TryExprSyntax.self) {
+      // Suppress this expression as its immediate parent is also an effectful
+      // expression (e.g. it's a `try await` expression overall.) The diagnostic
+      // reported for the parent expression should include both as needed.
+      return .visitChildren
+    } else if expression.is(AsExprSyntax.self) {
+      // Do not walk into explicit `as T` casts. This provides an escape hatch
+      // for expressions that should not diagnose.
+      return .skipChildren
+    } else if let awaitExpr = expression.as(AwaitExprSyntax.self), awaitExpr.expression.is(AsExprSyntax.self) {
+      // As above but for `try await _ as T`.
+      return .skipChildren
+    }
+    effectfulExprs.append(ExprSyntax(node))
+    return .visitChildren
+  }
+
+  override func visit(_ node: TryExprSyntax) -> SyntaxVisitorContinueKind {
+    _visitEffectful(node, expression: node.expression)
+  }
+
+  override func visit(_ node: AwaitExprSyntax) -> SyntaxVisitorContinueKind {
+    _visitEffectful(node, expression: node.expression)
+  }
+
+  override func visit(_ node: AsExprSyntax) -> SyntaxVisitorContinueKind {
+    // Do not walk into explicit `as T` casts. This provides an escape hatch for
+    // expressions that should not diagnose.
+    return .skipChildren
+  }
+
+  override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind {
+    // Do not walk into closures. Although they are not meant to be an escape
+    // hatch like `as` casts, it is very difficult (often impossible) to reason
+    // about effectful expressions inside the scope of a closure. If the closure
+    // is invoked locally, its caller will also need to say `try`/`await` and we
+    // can still diagnose those outer expressions.
+    return .skipChildren
+  }
+}
+
 // MARK: -
 
 /// Parse a condition argument from a binary operation expression.
@@ -496,6 +554,17 @@ private func _parseCondition(from expr: ExprSyntax, for macro: some Freestanding
 ///
 /// - Returns: An instance of ``Condition`` describing `expr`.
 func parseCondition(from expr: ExprSyntax, for macro: some FreestandingMacroExpansionSyntax, in context: some MacroExpansionContext) -> Condition {
+  // Handle `await` first. If present in the expression or a subexpression,
+  // diagnose and don't expand further.
+  let effectFinder = _EffectFinder(viewMode: .sourceAccurate)
+  effectFinder.walk(expr)
+  guard effectFinder.effectfulExprs.isEmpty else {
+    for effectfulExpr in effectFinder.effectfulExprs {
+      context.diagnose(.effectfulExpressionNotParsed(effectfulExpr, in: macro))
+    }
+    return Condition(expression: expr)
+  }
+
   let result = _parseCondition(from: expr, for: macro, in: context)
   if result.arguments.count == 1, let onlyArgument = result.arguments.first {
     _diagnoseTrivialBooleanValue(from: onlyArgument.expression, for: macro, in: context)

Sources/TestingMacros/Support/DiagnosticMessage.swift

@@ -32,9 +32,10 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   ///
   /// - Returns: A diagnostic message.
   static func condition(_ condition: ExprSyntax, isAlways value: Bool, in macro: some FreestandingMacroExpansionSyntax) -> Self {
-    Self(
+    let action = value ? "pass" : "fail"
+    return Self(
       syntax: Syntax(condition),
-      message: "#\(macro.macroName.textWithoutBackticks)(_:_:) will always \(value ? "pass" : "fail") here; use Bool(\(condition)) to silence this warning",
+      message: "\(_macroName(macro)) will always \(action) here; use 'Bool(\(condition))' to silence this warning",
       severity: value ? .note : .warning
     )
   }
@@ -50,11 +51,59 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func asExclamationMarkIsEvaluatedEarly(_ expr: AsExprSyntax, in macro: some FreestandingMacroExpansionSyntax) -> Self {
     return Self(
       syntax: Syntax(expr.asKeyword),
-      message: "The expression \(expr.trimmed) will be evaluated before #\(macro.macroName.textWithoutBackticks)(_:_:) is invoked; use as? instead of as! to silence this warning",
+      message: "Expression '\(expr.trimmed)' will be evaluated before \(_macroName(macro)) is invoked; use 'as?' instead of 'as!' to silence this warning",
+      severity: .warning
+    )
+  }
+
+  /// Create a diagnostic message stating that an effectful (`try` or `await`)
+  /// expression cannot be parsed and should be broken apart.
+  ///
+  /// - Parameters:
+  ///   - expr: The expression being diagnosed.
+  ///   - macro: The macro expression.
+  ///
+  /// - Returns: A diagnostic message.
+  static func effectfulExpressionNotParsed(_ expr: ExprSyntax, in macro: some FreestandingMacroExpansionSyntax) -> Self {
+    let effectful = if let tryExpr = expr.as(TryExprSyntax.self) {
+      if tryExpr.expression.is(AwaitExprSyntax.self) {
+        "throwing/asynchronous"
+      } else {
+        "throwing"
+      }
+    } else {
+      "asynchronous"
+    }
+    return Self(
+      syntax: Syntax(expr),
+      message: "Expression '\(expr.trimmed)' will not be expanded on failure; move the \(effectful) part out of the call to \(_macroName(macro))",
       severity: .warning
     )
   }
 
+  /// Get the human-readable name of the given freestanding macro.
+  ///
+  /// - Parameters:
+  ///   - macro: The freestanding macro node to name.
+  ///
+  /// - Returns: The name of the macro as understood by a developer, such as
+  ///   `"'#expect(_:_:)'"`. Include single quotes.
+  private static func _macroName(_ macro: some FreestandingMacroExpansionSyntax) -> String {
+    "'#\(macro.macroName.textWithoutBackticks)(_:_:)'"
+  }
+
+  /// Get the human-readable name of the given attached macro.
+  ///
+  /// - Parameters:
+  ///   - attribute: The attached macro node to name.
+  ///
+  /// - Returns: The name of the macro as understood by a developer, such as
+  ///   `"'@Test'"`. Include single quotes.
+  private static func _macroName(_ attribute: AttributeSyntax) -> String {
+    // SEE: https://github.com/apple/swift/blob/main/docs/Diagnostics.md?plain=1#L44
+    "'\(attribute.attributeNameText)'"
+  }
+
   /// Get a string corresponding to the specified syntax node (for instance,
   /// `"function"` for a function declaration.)
   ///
@@ -117,7 +166,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
     precondition(!attributes.isEmpty)
     return Self(
       syntax: Syntax(attributes.last!),
-      message: "The @\(attributes.last!.attributeNameText) attribute cannot be applied to \(_kindString(for: decl, includeA: true)) more than once.",
+      message: "Attribute \(_macroName(attributes.last!)) cannot be applied to \(_kindString(for: decl, includeA: true)) more than once",
       severity: .error
     )
   }
@@ -134,7 +183,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func genericDeclarationNotSupported(_ decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax, becauseOf genericClause: some SyntaxProtocol) -> Self {
     Self(
       syntax: Syntax(genericClause),
-      message: "The @\(attribute.attributeNameText) attribute cannot be applied to a generic \(_kindString(for: decl)).",
+      message: "Attribute \(_macroName(attribute)) cannot be applied to a generic \(_kindString(for: decl))",
       severity: .error
     )
   }
@@ -155,7 +204,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func availabilityAttributeNotSupported(_ availabilityAttribute: AttributeSyntax, on decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax) -> Self {
     Self(
       syntax: Syntax(availabilityAttribute),
-      message: "The @\(attribute.attributeNameText) attribute cannot be applied to this \(_kindString(for: decl)) because it has been marked \(availabilityAttribute.trimmed).",
+      message: "Attribute \(_macroName(attribute)) cannot be applied to this \(_kindString(for: decl)) because it has been marked '\(availabilityAttribute.trimmed)'",
       severity: .error
     )
   }
@@ -171,7 +220,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func attributeNotSupported(_ attribute: AttributeSyntax, on decl: some SyntaxProtocol) -> Self {
     Self(
       syntax: Syntax(decl),
-      message: "The @\(attribute.attributeNameText) attribute cannot be applied to \(_kindString(for: decl, includeA: true)).",
+      message: "Attribute \(_macroName(attribute)) cannot be applied to \(_kindString(for: decl, includeA: true))",
       severity: .error
     )
   }
@@ -187,8 +236,14 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func attributeHasNoEffect(_ attribute: AttributeSyntax, on decl: ExtensionDeclSyntax) -> Self {
     Self(
       syntax: Syntax(decl),
-      message: "The @\(attribute.attributeNameText) attribute has no effect when applied to an extension and should be removed.",
-      severity: .error
+      message: "Attribute \(_macroName(attribute)) has no effect when applied to an extension",
+      severity: .error,
+      fixIts: [
+        FixIt(
+          message: MacroExpansionFixItMessage("Remove attribute \(_macroName(attribute))"),
+          changes: [.replace(oldNode: Syntax(attribute), newNode: Syntax("" as ExprSyntax))]
+        ),
+      ]
     )
   }
 
@@ -206,19 +261,19 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
     case 0:
       return Self(
         syntax: Syntax(functionDecl),
-        message: "The @\(attribute.attributeNameText) attribute cannot specify arguments when used with \(functionDecl.completeName) because it does not take any.",
+        message: "Attribute \(_macroName(attribute)) cannot specify arguments when used with '\(functionDecl.completeName)' because it does not take any",
         severity: .error
       )
     case 1:
       return Self(
         syntax: Syntax(functionDecl),
-        message: "The @\(attribute.attributeNameText) attribute must specify an argument when used with \(functionDecl.completeName).",
+        message: "Attribute \(_macroName(attribute)) must specify an argument when used with '\(functionDecl.completeName)'",
         severity: .error
       )
     default:
       return Self(
         syntax: Syntax(functionDecl),
-        message: "The @\(attribute.attributeNameText) attribute must specify \(expectedArgumentCount) arguments when used with \(functionDecl.completeName).",
+        message: "Attribute \(_macroName(attribute)) must specify \(expectedArgumentCount) arguments when used with '\(functionDecl.completeName)'",
         severity: .error
       )
     }
@@ -236,7 +291,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func xcTestCaseNotSupported(_ decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax) -> Self {
     Self(
       syntax: Syntax(decl),
-      message: "The @\(attribute.attributeNameText) attribute cannot be applied to a subclass of XCTestCase.",
+      message: "Attribute \(_macroName(attribute)) cannot be applied to a subclass of 'XCTestCase'",
       severity: .error
     )
   }
@@ -252,7 +307,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func nonFinalClassNotSupported(_ decl: ClassDeclSyntax, whenUsing attribute: AttributeSyntax) -> Self {
     Self(
       syntax: Syntax(decl),
-      message: "The @\(attribute.attributeNameText) attribute cannot be applied to non-final class \(decl.name.textWithoutBackticks).",
+      message: "Attribute \(_macroName(attribute)) cannot be applied to non-final class '\(decl.name.textWithoutBackticks)'",
       severity: .error
     )
   }
@@ -269,7 +324,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func specifierNotSupported(_ specifier: TokenSyntax, on parameter: FunctionParameterSyntax, whenUsing attribute: AttributeSyntax) -> Self {
     Self(
       syntax: Syntax(parameter),
-      message: "The @\(attribute.attributeNameText) attribute cannot be applied to a function with a parameter marked '\(specifier.textWithoutBackticks)'.",
+      message: "Attribute \(_macroName(attribute)) cannot be applied to a function with a parameter marked '\(specifier.textWithoutBackticks)'",
       severity: .error
     )
   }
@@ -286,7 +341,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func returnTypeNotSupported(_ returnType: TypeSyntax, on decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax) -> Self {
     Self(
       syntax: Syntax(returnType),
-      message: "The result of this \(_kindString(for: decl)) will be discarded during testing.",
+      message: "The result of this \(_kindString(for: decl)) will be discarded during testing",
       severity: .warning
     )
   }
@@ -302,7 +357,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func tagExprNotSupported(_ tagExpr: some SyntaxProtocol, in attribute: AttributeSyntax) -> Self {
     Self(
       syntax: Syntax(tagExpr),
-      message: "The tag \(tagExpr.trimmed) cannot be used with the @\(attribute.attributeNameText) attribute. Pass a member of Tag or a string literal instead.",
+      message: "Tag '\(tagExpr.trimmed)' cannot be used with attribute \(_macroName(attribute)); pass a member of 'Tag' or a string literal instead",
       severity: .error
     )
   }
@@ -317,15 +372,15 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   static func optionalBoolExprIsAmbiguous(_ boolExpr: ExprSyntax) -> Self {
     Self(
       syntax: Syntax(boolExpr),
-      message: "Requirement '\(boolExpr.trimmed)' is ambiguous.",
+      message: "Requirement '\(boolExpr.trimmed)' is ambiguous",
       severity: .warning,
       fixIts: [
         FixIt(
-          message: MacroExpansionFixItMessage("To unwrap an optional value, add 'as Bool?'."),
+          message: MacroExpansionFixItMessage("To unwrap an optional value, add 'as Bool?'"),
           changes: [.replace(oldNode: Syntax(boolExpr), newNode: Syntax("\(boolExpr) as Bool?" as ExprSyntax))]
         ),
         FixIt(
-          message: MacroExpansionFixItMessage("To check if a value is true, add '?? false'."),
+          message: MacroExpansionFixItMessage("To check if a value is true, add '?? false'"),
           changes: [.replace(oldNode: Syntax(boolExpr), newNode: Syntax("\(boolExpr) ?? false" as ExprSyntax))]
         ),
       ]

Tests/TestingMacrosTests/ConditionMacroTests.swift

@@ -331,6 +331,49 @@ struct ConditionMacroTests {
     #expect(diagnostics.isEmpty)
   }
 
+  @Test("#expect(try/await) produces a diagnostic",
+    arguments: [
+      "#expect(try foo())": ["Expression 'try foo()' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'"],
+      "#expect(await foo())": ["Expression 'await foo()' will not be expanded on failure; move the asynchronous part out of the call to '#expect(_:_:)'"],
+      "#expect(try await foo())": ["Expression 'try await foo()' will not be expanded on failure; move the throwing/asynchronous part out of the call to '#expect(_:_:)'"],
+      "#expect(try await foo(try bar(await quux())))": [
+        "Expression 'try await foo(try bar(await quux()))' will not be expanded on failure; move the throwing/asynchronous part out of the call to '#expect(_:_:)'",
+        "Expression 'try bar(await quux())' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'",
+        "Expression 'await quux()' will not be expanded on failure; move the asynchronous part out of the call to '#expect(_:_:)'",
+      ],
+
+      // Diagnoses because the diagnostic for `await` is suppressed due to the
+      // `as T` cast, but the parentheses limit the effect of the suppression.
+      "#expect(try (await foo() as T))": ["Expression 'try (await foo() as T)' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'"],
+    ]
+  )
+  func effectfulExpectationDiagnoses(input: String, diagnosticMessages: [String]) throws {
+    let (_, diagnostics) = try parse(input)
+    #expect(diagnostics.count == diagnosticMessages.count)
+    for message in diagnosticMessages {
+      #expect(diagnostics.contains { $0.diagMessage.message == message }, "Missing \(message): \(diagnostics.map(\.diagMessage.message))")
+    }
+  }
+
+  @Test("#expect(try/await as Bool) suppresses its diagnostic",
+    arguments: [
+      "#expect(try foo() as Bool)",
+      "#expect(await foo() as Bool)",
+      "#expect(try await foo(try await bar()) as Bool)",
+      "#expect(try foo() as T?)",
+      "#expect(await foo() as? T)",
+      "#expect(try await foo(try await bar()) as! T)",
+      "#expect((try foo()) as T)",
+      "#expect((await foo()) as T)",
+      "#expect((try await foo(try await bar())) as T)",
+      "#expect(try (await foo()) as T)",
+    ]
+  )
+  func effectfulExpectationDiagnosticSuppressWithExplicitBool(input: String) throws {
+    let (_, diagnostics) = try parse(input)
+    #expect(diagnostics.isEmpty)
+  }
+
   @Test("Macro expansion is performed within a test function")
   func macroExpansionInTestFunction() throws {
     let input = ##"""