Diagnose ambiguous uses of #require() with an argument of type Bool?. (#276)

This PR adds new diagnostics when `#require()` is passed an argument of
type `Bool?` because such an argument may be ambiguously interpreted. If
the test author writes:

```swift
try #require(love?.isFleeting)
```

Do they mean to unwrap the optional boolean value and return it, or do
they mean to test if `isFleeting` is true or false?

With this change, an expression such as that one that returns a value of
type `Bool?` will produce a diagnostic of the form:

![Screenshot 2024-03-08 at 4 41
30 PM](https://github.com/apple/swift-testing/assets/4145863/248105f8-1d48-4c9b-96b9-e6e3b066b4ca)

(The diagnostics are presented as fix-its in Xcode.)

Resolves rdar://105727852.

### 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/Testing/Expectations/Expectation+Macro.swift

@@ -57,18 +57,49 @@
 ///   - sourceLocation: The source location to which recorded expectations and
 ///     issues should be attributed.
 ///
-/// - Returns: The unwrapped value of `value`.
+/// - Returns: The unwrapped value of `optionalValue`.
 ///
-/// - Throws: An instance of ``ExpectationFailedError`` if `value` is `nil`.
+/// - Throws: An instance of ``ExpectationFailedError`` if `optionalValue` is
+///   `nil`.
 ///
-/// If `value` is `nil`, an ``Issue`` is recorded for the test that is running
-/// in the current task and an instance of ``ExpectationFailedError`` is thrown.
+/// If `optionalValue` is `nil`, an ``Issue`` is recorded for the test that is
+/// running in the current task and an instance of ``ExpectationFailedError`` is
+/// thrown.
 @freestanding(expression) public macro require<T>(
   _ optionalValue: T?,
   _ comment: @autoclosure () -> Comment? = nil,
   sourceLocation: SourceLocation = SourceLocation()
 ) -> T = #externalMacro(module: "TestingMacros", type: "RequireMacro")
 
+/// Unwrap an optional boolean value or, if it is `nil`, fail and throw an
+/// error.
+///
+/// - Parameters:
+///   - optionalValue: The optional value to be unwrapped.
+///   - comment: A comment describing the expectation.
+///   - sourceLocation: The source location to which recorded expectations and
+///     issues should be attributed.
+///
+/// - Returns: The unwrapped value of `optionalValue`.
+///
+/// - Throws: An instance of ``ExpectationFailedError`` if `optionalValue` is
+///   `nil`.
+///
+/// If `optionalValue` is `nil`, an ``Issue`` is recorded for the test that is
+/// running in the current task and an instance of ``ExpectationFailedError`` is
+/// thrown.
+///
+/// This overload of ``require(_:_:sourceLocation:)-6w9oo`` checks if
+/// `optionalValue` may be ambiguous (i.e. it is unclear if the developer
+/// intended to check for a boolean value or unwrap an optional boolean value)
+/// and provides additional compile-time diagnostics when it is.
+@_documentation(visibility: private)
+@freestanding(expression) public macro require(
+  _ optionalValue: Bool?,
+  _ comment: @autoclosure () -> Comment? = nil,
+  sourceLocation: SourceLocation = SourceLocation()
+) -> Bool = #externalMacro(module: "TestingMacros", type: "AmbiguousRequireMacro")
+
 // MARK: - Matching errors by type
 
 /// Check that an expression always throws an error of a given type.

Sources/TestingMacros/ConditionMacro.swift

@@ -202,3 +202,42 @@ public struct RequireMacro: ConditionMacro {
     true
   }
 }
+
+/// A type describing the expansion of the `#require()` macro when it is
+/// ambiguous whether it refers to a boolean check or optional unwrapping.
+///
+/// This type is otherwise exactly equivalent to ``RequireMacro``.
+public struct AmbiguousRequireMacro: ConditionMacro {
+  public static func expansion(
+    of macro: some FreestandingMacroExpansionSyntax,
+    in context: some MacroExpansionContext
+  ) throws -> ExprSyntax {
+    if let argument = macro.argumentList.first {
+      _checkAmbiguousArgument(argument.expression, in: context)
+    }
+
+    // Perform the normal macro expansion for #require().
+    return try RequireMacro.expansion(of: macro, in: context)
+  }
+
+  private static func _checkAmbiguousArgument(_ argument: ExprSyntax, in context: some MacroExpansionContext) {
+    // If the argument is wrapped in parentheses, strip them before continuing.
+    if let argumentWithoutParentheses = removeParentheses(from: argument) {
+      return _checkAmbiguousArgument(argumentWithoutParentheses, in: context)
+    }
+
+    // If the argument is explicitly an as? cast already, do not diagnose.
+    if argument.is(AsExprSyntax.self) {
+      return
+    }
+
+    // If we reach this point, then the argument appears to be an ambiguous
+    // expression and we aren't sure if the developer intended to unwrap a Bool?
+    // or check the value of the wrapped Bool.
+    context.diagnose(.optionalBoolExprIsAmbiguous(argument))
+  }
+
+  public static var isThrowing: Bool {
+    true
+  }
+}

Sources/TestingMacros/Support/ConditionArgumentParsing.swift

@@ -98,10 +98,10 @@ private func _diagnoseTrivialBooleanValue(from expr: ExprSyntax, for macro: some
 ///
 /// This function handles expressions such as `!foo` or `!(bar)`.
 private func _negatedExpression(_ expr: ExprSyntax, in context: some MacroExpansionContext) -> ExprSyntax? {
-  let expr = _removeParentheses(from: expr) ?? expr
+  let expr = removeParentheses(from: expr) ?? expr
   if let op = expr.as(PrefixOperatorExprSyntax.self),
      op.operator.tokenKind == .prefixOperator("!") {
-    return _removeParentheses(from: op.expression) ?? op.expression
+    return removeParentheses(from: op.expression) ?? op.expression
   }
 
   return nil
@@ -117,12 +117,12 @@ private func _negatedExpression(_ expr: ExprSyntax, in context: some MacroExpans
 ///
 /// This function handles expressions such as `(foo)` or `((foo, bar))`. It does
 /// not remove interior parentheses (e.g. `(foo, (bar))`.)
-private func _removeParentheses(from expr: ExprSyntax) -> ExprSyntax? {
+func removeParentheses(from expr: ExprSyntax) -> ExprSyntax? {
   if let tuple = expr.as(TupleExprSyntax.self),
       tuple.elements.count == 1,
      let elementExpr = tuple.elements.first,
      elementExpr.label == nil {
-    return _removeParentheses(from: elementExpr.expression) ?? elementExpr.expression
+    return removeParentheses(from: elementExpr.expression) ?? elementExpr.expression
   }
 
   return nil
@@ -478,7 +478,7 @@ private func _parseCondition(from expr: ExprSyntax, for macro: some Freestanding
 
   // Parentheses are parsed as if they were tuples, so (true && false) appears
   // to the parser as a tuple containing one expression, `true && false`.
-  if let expr = _removeParentheses(from: expr) {
+  if let expr = removeParentheses(from: expr) {
     return _parseCondition(from: expr, for: macro, in: context)
   }
 

Sources/TestingMacros/Support/DiagnosticMessage.swift

@@ -12,9 +12,11 @@ import SwiftDiagnostics
 #if swift(>=5.11)
 import SwiftSyntax
 import SwiftSyntaxMacros
+import SwiftSyntaxMacroExpansion
 #else
 public import SwiftSyntax
 public import SwiftSyntaxMacros
+public import SwiftSyntaxMacroExpansion
 #endif
 
 /// A type describing diagnostic messages emitted by this module's macro during
@@ -293,8 +295,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   /// tag on a test or suite is not supported.
   ///
   /// - Parameters:
-  ///   - returnType: The unsupported return type.
-  ///   - decl: The declaration with an unsupported return type.
+  ///   - tagExpr: The unsupported tag expression.
   ///   - attribute: The `@Test` or `@Suite` attribute.
   ///
   /// - Returns: A diagnostic message.
@@ -306,13 +307,39 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
     )
   }
 
+  /// Create a diagnostic messages stating that the expression passed to
+  /// `#require()` is ambiguous.
+  ///
+  /// - Parameters:
+  ///   - boolExpr: The ambiguous optional boolean expression.
+  ///
+  /// - Returns: A diagnostic message.
+  static func optionalBoolExprIsAmbiguous(_ boolExpr: ExprSyntax) -> Self {
+    Self(
+      syntax: Syntax(boolExpr),
+      message: "Requirement '\(boolExpr.trimmed)' is ambiguous.",
+      severity: .warning,
+      fixIts: [
+        FixIt(
+          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'."),
+          changes: [.replace(oldNode: Syntax(boolExpr), newNode: Syntax("\(boolExpr) ?? false" as ExprSyntax))]
+        ),
+      ]
+    )
+  }
+
   var syntax: Syntax
 
   // MARK: - DiagnosticMessage
 
   var message: String
   var diagnosticID = MessageID(domain: "org.swift.testing", id: "macros")
   var severity: DiagnosticSeverity
+  var fixIts: [FixIt] = []
 }
 
 // MARK: -
@@ -324,12 +351,14 @@ extension MacroExpansionContext {
   ///   - message: The diagnostic message to emit. The `node` and `position`
   ///     arguments to `Diagnostic.init()` are derived from the message's
   ///     `syntax` property.
+  ///   - fixIts: Any Fix-Its to apply.
   func diagnose(_ message: DiagnosticMessage) {
     diagnose(
       Diagnostic(
         node: message.syntax,
         position: message.syntax.positionAfterSkippingLeadingTrivia,
-        message: message
+        message: message,
+        fixIts: message.fixIts
       )
     )
   }

Sources/TestingMacros/TestingMacrosMain.swift

@@ -26,6 +26,7 @@ struct TestingMacrosMain: CompilerPlugin {
       TestDeclarationMacro.self,
       ExpectMacro.self,
       RequireMacro.self,
+      AmbiguousRequireMacro.self,
       TagMacro.self,
     ]
   }

Tests/TestingMacrosTests/ConditionMacroTests.swift

@@ -292,6 +292,45 @@ struct ConditionMacroTests {
     #expect(diagnostics.count == 0)
   }
 
+  @Test("#require(Bool?) produces a diagnostic",
+    arguments: [
+      "#requireAmbiguous(expression)",
+      "#requireAmbiguous((expression))",
+      "#requireAmbiguous(a + b)",
+      "#requireAmbiguous((a + b))",
+      "#requireAmbiguous((a) + (b))",
+    ]
+  )
+  func requireOptionalBoolProducesDiagnostic(input: String) throws {
+    let (_, diagnostics) = try parse(input)
+
+    let diagnostic = try #require(diagnostics.first)
+    #expect(diagnostic.diagMessage.severity == .warning)
+    #expect(diagnostic.message.contains("is ambiguous"))
+    #expect(diagnostic.fixIts.count == 2)
+    #expect(diagnostic.fixIts[0].message.message.contains("as Bool?"))
+    #expect(diagnostic.fixIts[1].message.message.contains("?? false"))
+  }
+
+  @Test("#require(as Bool?) suppresses its diagnostic",
+    arguments: [
+      "#requireAmbiguous(expression as Bool?)",
+      "#requireAmbiguous((expression as Bool?))",
+      "#requireAmbiguous((expression) as Bool?)",
+      "#requireAmbiguous(a + b as Bool?)",
+      "#requireAmbiguous((a + b) as Bool?)",
+      "#requireAmbiguous((a) + (b) as Bool?)",
+      "#requireAmbiguous(((a) + (b)) as Bool?)",
+    ]
+  )
+  func requireOptionalBoolSuppressedWithExplicitType(input: String) throws {
+    // Note we do not need to test "as Bool" (non-optional) because an
+    // expression of type Bool rather than Bool? won't trigger the additional
+    // diagnostics in the first place.
+    let (_, diagnostics) = try parse(input)
+    #expect(diagnostics.isEmpty)
+  }
+
   @Test("Macro expansion is performed within a test function")
   func macroExpansionInTestFunction() throws {
     let input = ##"""

Tests/TestingMacrosTests/TestSupport/Parse.swift

@@ -21,6 +21,7 @@ import SwiftSyntaxMacroExpansion
 fileprivate let allMacros: [String: any Macro.Type] = [
   "expect": ExpectMacro.self,
   "require": RequireMacro.self,
+  "requireAmbiguous": AmbiguousRequireMacro.self, // different name needed only for unit testing
   "Suite": SuiteDeclarationMacro.self,
   "Test": TestDeclarationMacro.self,
 ]