Include fix-its in the diagnostic on a parameterized test when @Test is missing arguments: (#450)

stmontgomery · web-flow · commit bd9249e59e5c · 2024-06-16T23:06:16.000-05:00
This adds fix-its to the diagnostic emitted by the `@Test` macro when it has been applied to a function which includes parameters but does not specify `arguments: ...`. ### Motivation: When transforming a non-parameterized `@Test` function into a parameterized one, it's common to begin by adding one or more parameters to the function's signature. Immediately after doing that, the `@Test` macro emits an error diagnostic if `arguments:` is not passed to `@Test`. Since this is a common workflow and it may not always be obvious to new users where to add `arguments:`, it would be beneficial for this diagnostic to include a fix-it. ### Modifications: - Add a couple of styles of fix-it to this diagnostic, with variations for single- and multiple-parameter functions. - Enhance a utility on `EditorPlaceholderExprSyntax` to prefer typed placeholders (see code comment). ### 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. Resolves rdar://128396593
diff --git a/Sources/TestingMacros/Support/Additions/EditorPlaceholderExprSyntaxAdditions.swift b/Sources/TestingMacros/Support/Additions/EditorPlaceholderExprSyntaxAdditions.swift
@@ -11,12 +11,44 @@
 import SwiftSyntax
 
 extension EditorPlaceholderExprSyntax {
-  /// Initialize an instance of this type with the given placeholder string.
+  /// Initialize an instance of this type with the given placeholder string and
+  /// optional type.
   ///
   /// - Parameters:
   ///   - placeholder: The placeholder string, not including surrounding angle
   ///     brackets or pound characters.
-  init(_ placeholder: String) {
-    self.init(placeholder: .identifier("<# \(placeholder) #" + ">"))
+  ///   - type: The type which this placeholder have, if any. When non-`nil`,
+  ///     the expression will use typed placeholder syntax.
+  init(_ placeholder: String, type: String? = nil) {
+    let placeholderContent = if let type {
+      // These use typed placeholder syntax, which allows the compiler to
+      // type-check the expression successfully. The resulting code still does
+      // not compile due to the placeholder, but it makes the diagnostic more
+      // clear. See
+      // https://developer.apple.com/documentation/swift-playgrounds/specifying-editable-regions-in-a-playground-page#Mark-Editable-Areas-with-Placeholder-Tokens
+      if placeholder == type {
+        // When the placeholder string is exactly the same as the type string,
+        // use the shorter typed placeholder format.
+        "T##\(placeholder)"
+      } else {
+        "T##\(placeholder)##\(type)"
+      }
+    } else {
+      placeholder
+    }
+
+    // Manually concatenate the string to avoid it being interpreted as a
+    // placeholder when editing this file.
+    self.init(placeholder: .identifier("<#\(placeholderContent)#" + ">"))
+  }
+
+  /// Initialize an instance of this type with the given type, using that as the
+  /// placeholder string.
+  ///
+  /// - Parameters:
+  ///   - type: The type to use both as the placeholder text and as the
+  ///     expression's type.
+  init(type: String) {
+    self.init(type, type: type)
   }
 }
diff --git a/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift b/Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift
@@ -136,12 +136,8 @@ extension FunctionParameterSyntax {
   /// For example, for the parameter `y` of `func x(y: Int)`, the value of this
   /// property is an expression equivalent to `Int.self`.
   private var typeMetatypeExpression: some ExprSyntaxProtocol {
-    // Discard any specifiers such as `inout` or `borrowing`, since we're only
-    // trying to obtain the base type to reference it in an expression.
-    let baseType = type.as(AttributedTypeSyntax.self)?.baseType ?? type
-
-    // Construct a member access expression, referencing the base type above.
-    let baseTypeDeclReferenceExpr = DeclReferenceExprSyntax(baseName: .identifier(baseType.trimmedDescription))
+    // Construct a member access expression, referencing the base type name.
+    let baseTypeDeclReferenceExpr = DeclReferenceExprSyntax(baseName: .identifier(baseTypeName))
 
     // Enclose the base type declaration reference in a 1-element tuple, e.g.
     // `(<baseType>)`. It will be used in a member access expression below, and
@@ -155,3 +151,13 @@ extension FunctionParameterSyntax {
     return MemberAccessExprSyntax(base: metatypeMemberAccessBase, name: .identifier("self"))
   }
 }
+
+extension FunctionParameterSyntax {
+  /// The base type name of this parameter.
+  var baseTypeName: String {
+    // Discard any specifiers such as `inout` or `borrowing`, since we're only
+    // trying to obtain the base type to reference it in an expression.
+    let baseType = type.as(AttributedTypeSyntax.self)?.baseType ?? type
+    return baseType.trimmedDescription
+  }
+}
diff --git a/Sources/TestingMacros/Support/DiagnosticMessage.swift b/Sources/TestingMacros/Support/DiagnosticMessage.swift
@@ -432,32 +432,109 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
   /// number of arguments when applied to the given function declaration.
   ///
   /// - Parameters:
-  ///   - attribute: The `@Test` or `@Suite` attribute.
+  ///   - attribute: The `@Test` attribute.
   ///   - functionDecl: The declaration in question.
   ///
   /// - Returns: A diagnostic message.
   static func attributeArgumentCountIncorrect(_ attribute: AttributeSyntax, on functionDecl: FunctionDeclSyntax) -> Self {
     let expectedArgumentCount = functionDecl.signature.parameterClause.parameters.count
-    switch expectedArgumentCount {
-    case 0:
+    if expectedArgumentCount == 0 {
       return Self(
         syntax: Syntax(functionDecl),
-        message: "Attribute \(_macroName(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 function '\(functionDecl.completeName)' because it does not take any",
         severity: .error
       )
-    case 1:
+    } else {
       return Self(
         syntax: Syntax(functionDecl),
-        message: "Attribute \(_macroName(attribute)) must specify an argument when used with '\(functionDecl.completeName)'",
-        severity: .error
+        message: "Attribute \(_macroName(attribute)) must specify arguments when used with function '\(functionDecl.completeName)'",
+        severity: .error,
+        fixIts: _addArgumentsFixIts(for: attribute, given: functionDecl.signature.parameterClause.parameters)
       )
-    default:
-      return Self(
-        syntax: Syntax(functionDecl),
-        message: "Attribute \(_macroName(attribute)) must specify \(expectedArgumentCount) arguments when used with '\(functionDecl.completeName)'",
-        severity: .error
+    }
+  }
+
+  /// Create fix-its for a diagnostic stating that the given attribute must
+  /// specify arguments since it is applied to a function which has parameters.
+  ///
+  /// - Parameters:
+  ///   - attribute: The `@Test` attribute.
+  ///   - parameters: The parameter list of the function `attribute` is applied
+  ///     to.
+  ///
+  /// - Returns: An array of fix-its to include in a diagnostic.
+  private static func _addArgumentsFixIts(for attribute: AttributeSyntax, given parameters: FunctionParameterListSyntax) -> [FixIt] {
+    let baseArguments: LabeledExprListSyntax
+    if let existingArguments = attribute.arguments {
+      guard case var .argumentList(existingLabeledArguments) = existingArguments else {
+        // If there are existing arguments but they are of an unexpected type,
+        // don't attempt to provide any fix-its.
+        return []
+      }
+
+      // If the existing argument list is non-empty, ensure the last argument
+      // has a trailing comma and space.
+      if !existingLabeledArguments.isEmpty {
+        let lastIndex = existingLabeledArguments.index(before: existingLabeledArguments.endIndex)
+        existingLabeledArguments[lastIndex].trailingComma = .commaToken(trailingTrivia: .space)
+      }
+
+      baseArguments = existingLabeledArguments
+    } else {
+      baseArguments = .init()
+    }
+
+    var fixIts: [FixIt] = []
+    func addFixIt(_ message: String, appendingArguments arguments: some Collection<LabeledExprSyntax>) {
+      var newAttribute = attribute
+      newAttribute.leftParen = .leftParenToken()
+      newAttribute.arguments = .argumentList(baseArguments + arguments)
+      let trailingTrivia = newAttribute.rightParen?.trailingTrivia
+        ?? newAttribute.attributeName.as(IdentifierTypeSyntax.self)?.name.trailingTrivia
+        ?? .space
+      newAttribute.rightParen = .rightParenToken(trailingTrivia: trailingTrivia)
+      newAttribute.attributeName = newAttribute.attributeName.trimmed
+
+      fixIts.append(FixIt(
+        message: MacroExpansionFixItMessage(message),
+        changes: [.replace(oldNode: Syntax(attribute), newNode: Syntax(newAttribute))]
+      ))
+    }
+
+    // Fix-It to add 'arguments:' with one collection. If the function has 2 or
+    // more parameters, the elements of the placeholder collection are of tuple
+    // type.
+    do {
+      let argumentsCollectionType = if parameters.count == 1, let parameter = parameters.first {
+        "[\(parameter.baseTypeName)]"
+      } else {
+        "[(\(parameters.map(\.baseTypeName).joined(separator: ", ")))]"
+      }
+
+      addFixIt(
+        "Add 'arguments:' with one collection",
+        appendingArguments: [LabeledExprSyntax(label: "arguments", expression: EditorPlaceholderExprSyntax(type: argumentsCollectionType))]
       )
     }
+
+    // Fix-It to add 'arguments:' with all combinations of <N> collections,
+    // where <N> is the count of the function's parameters. Only offered for
+    // functions with 2 parameters.
+    if parameters.count == 2 {
+      let additionalArguments = parameters.indices.map { index in
+        let label = index == parameters.startIndex ? "arguments" : nil
+        let argumentsCollectionType = "[\(parameters[index].baseTypeName)]"
+        return LabeledExprSyntax(
+          label: label.map { .identifier($0) },
+          colon: label == nil ? nil : .colonToken(trailingTrivia: .space),
+          expression: EditorPlaceholderExprSyntax(type: argumentsCollectionType),
+          trailingComma: parameters.index(after: index) < parameters.endIndex ? .commaToken(trailingTrivia: .space) : nil
+        )
+      }
+      addFixIt("Add 'arguments:' with all combinations of \(parameters.count) collections", appendingArguments: additionalArguments)
+    }
+
+    return fixIts
   }
 
   /// Create a diagnostic message stating that `@Test` or `@Suite` is
@@ -565,7 +642,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
       fixIts: [
         FixIt(
           message: MacroExpansionFixItMessage(#"Replace "\#(urlString)" with URL"#),
-          changes: [.replace(oldNode: Syntax(urlExpr), newNode: Syntax(EditorPlaceholderExprSyntax("url")))]
+          changes: [.replace(oldNode: Syntax(urlExpr), newNode: Syntax(EditorPlaceholderExprSyntax("url", type: "String")))]
         ),
         FixIt(
           message: MacroExpansionFixItMessage("Remove trait '\(traitName.trimmed)'"),
diff --git a/Tests/TestingMacrosTests/TestDeclarationMacroTests.swift b/Tests/TestingMacrosTests/TestDeclarationMacroTests.swift
@@ -98,12 +98,8 @@ struct TestDeclarationMacroTests {
         "Attribute 'Test' cannot be applied to a function with a parameter marked '_const'",
 
       // Argument count mismatches.
-      "@Test func f(i: Int) {}":
-        "Attribute 'Test' must specify an argument when used with 'f(i:)'",
-      "@Test func f(i: Int, j: Int) {}":
-        "Attribute 'Test' must specify 2 arguments when used with 'f(i:j:)'",
       "@Test(arguments: []) func f() {}":
-        "Attribute 'Test' cannot specify arguments when used with 'f()' because it does not take any",
+        "Attribute 'Test' cannot specify arguments when used with function 'f()' because it does not take any",
 
       // Invalid lexical contexts
       "struct S { func f() { @Test func g() {} } }":
@@ -158,6 +154,95 @@ struct TestDeclarationMacroTests {
     }
   }
 
+  static var errorsWithFixIts: [String: (message: String, fixIts: [ExpectedFixIt])] {
+    [
+      // 'Test' attribute must specify arguments to parameterized test functions.
+      "@Test func f(i: Int) {}":
+        (
+          message: "Attribute 'Test' must specify arguments when used with function 'f(i:)'",
+          fixIts: [
+            ExpectedFixIt(
+              message: "Add 'arguments:' with one collection",
+              changes: [.replace(oldSourceCode: "@Test ", newSourceCode: "@Test(arguments: \(EditorPlaceholderExprSyntax(type: "[Int]"))) ")]
+            ),
+          ]
+        ),
+      "@Test func f(i: Int, j: String) {}":
+        (
+          message: "Attribute 'Test' must specify arguments when used with function 'f(i:j:)'",
+          fixIts: [
+            ExpectedFixIt(
+              message: "Add 'arguments:' with one collection",
+              changes: [.replace(oldSourceCode: "@Test ", newSourceCode: "@Test(arguments: \(EditorPlaceholderExprSyntax(type: "[(Int, String)]"))) ")]
+            ),
+            ExpectedFixIt(
+              message: "Add 'arguments:' with all combinations of 2 collections",
+              changes: [.replace(oldSourceCode: "@Test ", newSourceCode: "@Test(arguments: \(EditorPlaceholderExprSyntax(type: "[Int]")), \(EditorPlaceholderExprSyntax(type: "[String]"))) ")]
+            ),
+          ]
+        ),
+      "@Test func f(i: Int, j: String, k: Double) {}":
+        (
+          message: "Attribute 'Test' must specify arguments when used with function 'f(i:j:k:)'",
+          fixIts: [
+            ExpectedFixIt(
+              message: "Add 'arguments:' with one collection",
+              changes: [.replace(oldSourceCode: "@Test ", newSourceCode: "@Test(arguments: \(EditorPlaceholderExprSyntax(type: "[(Int, String, Double)]"))) ")]
+            ),
+          ]
+        ),
+      #"@Test("Some display name") func f(i: Int) {}"#:
+        (
+          message: "Attribute 'Test' must specify arguments when used with function 'f(i:)'",
+          fixIts: [
+            ExpectedFixIt(
+              message: "Add 'arguments:' with one collection",
+              changes: [.replace(oldSourceCode: #"@Test("Some display name") "#, newSourceCode: #"@Test("Some display name", arguments: \#(EditorPlaceholderExprSyntax(type: "[Int]"))) "#)]
+            ),
+          ]
+        ),
+      #"@Test /*comment*/ func f(i: Int) {}"#:
+        (
+          message: "Attribute 'Test' must specify arguments when used with function 'f(i:)'",
+          fixIts: [
+            ExpectedFixIt(
+              message: "Add 'arguments:' with one collection",
+              changes: [.replace(oldSourceCode: #"@Test /*comment*/ "#, newSourceCode: #"@Test(arguments: \#(EditorPlaceholderExprSyntax(type: "[Int]"))) /*comment*/ "#)]
+            ),
+          ]
+        ),
+    ]
+  }
+
+  @Test("Error diagnostics which include fix-its emitted on API misuse", arguments: errorsWithFixIts)
+  func apiMisuseErrorsIncludingFixIts(input: String, expectedDiagnostic: (message: String, fixIts: [ExpectedFixIt])) throws {
+    let (_, diagnostics) = try parse(input)
+
+    #expect(diagnostics.count == 1)
+    let diagnostic = try #require(diagnostics.first)
+    #expect(diagnostic.diagMessage.severity == .error)
+    #expect(diagnostic.message == expectedDiagnostic.message)
+
+    try #require(diagnostic.fixIts.count == expectedDiagnostic.fixIts.count)
+    for (fixIt, expectedFixIt) in zip(diagnostic.fixIts, expectedDiagnostic.fixIts) {
+      #expect(fixIt.message.message == expectedFixIt.message)
+
+      try #require(fixIt.changes.count == expectedFixIt.changes.count)
+      for (change, expectedChange) in zip(fixIt.changes, expectedFixIt.changes) {
+        switch (change, expectedChange) {
+        case let (.replace(oldNode, newNode), .replace(expectedOldSourceCode, expectedNewSourceCode)):
+          let oldSourceCode = String(describing: oldNode.formatted())
+          #expect(oldSourceCode == expectedOldSourceCode)
+
+          let newSourceCode = String(describing: newNode.formatted())
+          #expect(newSourceCode == expectedNewSourceCode)
+        default:
+          Issue.record("Change \(change) differs from expected change \(expectedChange)")
+        }
+      }
+    }
+  }
+
   @Test("Warning diagnostics emitted on API misuse",
     arguments: [
       // return types
diff --git a/Tests/TestingMacrosTests/TestSupport/FixIts.swift b/Tests/TestingMacrosTests/TestSupport/FixIts.swift
@@ -0,0 +1,28 @@
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for Swift project authors
+//
+
+/// A type representing a fix-it which is expected to be included in a
+/// diagnostic from a macro.
+struct ExpectedFixIt {
+  /// A description of what this expected fix-it performs.
+  var message: String
+
+  /// An enumeration describing a change to be performed by a fix-it.
+  ///
+  /// - Note: Not all changes in the real `FixIt` type are currently supported
+  ///   and included in this list.
+  enum Change {
+    /// Replace `oldSourceCode` by `newSourceCode`.
+    case replace(oldSourceCode: String, newSourceCode: String)
+  }
+
+  /// The changes that would be performed when this expected fix-it is applied.
+  var changes: [Change] = []
+}
