Proposal: Addressing gaps in Swift Macro Testing

[gohanlon]

There are a couple cases where swift-macro-testing (SMT) lacks important coverage of macro behavior. Here I present a solution for addressing those gaps. While I believe the solution I’m offering is cohesive and compelling, I’m also keenly aware that there could be other, possibly even more compelling, solutions out there. I’m also suggesting breaking changes, and whether they are acceptable is an important question for discussion.

I’ve locally implemented my proposed solution and applied it to my MemberwiseInit macro:

1. I snapshot the original expanded source when actual expansion occurs, even if diagnostics are present. To recognize "actual expansion", I use the AttributeRemover from Swift Syntax to remove the macro attributes from the original source code, and string-compare that to the expanded source: If the strings match, then no "actual expansion" took place and we can safely omit the expanded source from the snapshot. (SMT currently doesn't cover the expanded source when macros simultaneously emit both diagnostics and code.)

2. I removed the automatic re-expansion of fixed source due to completeness concerns and current limitations in SMT. Complete re-expansion requires an additional layer of expansion, diagnostics, and fixes, and SMT currently fails silently when re-expansion encounters diagnostics, hindering troubleshooting. Instead, I now require a separate assertMacro instance for each expansion to be tested. This approach, while more explicit, often aligns with existing practices, as I've found that fixed source expansion typically duplicates "happy path" tests already in place.

3. To address Exclusively applicable fix-its aren't testable #14, I changed the fixes string format to apply each fix-it individually instead of assuming that fix-its can always be applied simultaneously. This new fixes format is more verbose (and I think necessary for correctness). At least for MemberwiseInit, it’s easy to reason about and visually verify.

4. To address Fix-it coverage lost when macros include diagnostics with and without fix-its #19, I always snapshot the fixes, even when not all diagnostics have a fix-it. (I also allow fixes to be suppressed using applyFixIts: false.)

Piecemeal vs. wholesale changes

Given how interconnected the issues are, I think this proposal and its alternatives need to be considered as a whole. That said, here’s a way in which each of the above could potentially be pursued more independently:

1. To solely fix expansion coverage when macros emit both diagnostics and fix-its, the macro would need to have two “expansion” closures: one for the initial expansion and another for the final fixesExpansion. Then if/when automatic re-expansion of fixed source is removed, the fixesExpansion closure would go away.

2. To address SMT failing silently when re-expansion encounters diagnostics, we have two main options:

   a) We can "overload" the final expansion closure to present diagnostics when they're encountered rather than omitting the expansion closure entirely. (Awkward, but effective.)

   b) Alternatively, we could modify the signature of assertMacro to include additional closures specifically for handling re-expansion diagnostics. While this would complicate the function signature, it would clearly separate initial expansion and re-expansion results.

   If we later decide to remove automatic re-expansion of fixed source, the implications would depend on which solution we choose:

   • If we'd implemented the overloaded expansion closure (option a), the expansion closure would revert to a single purpose (initial expansion only) and could be moved to be the first output closure.

   • If we'd implemented the modified assertMacro signature (option b), we could simplify the signature by removing the additional closures for re-expansion, and move the remaining expansion closure to be the first output closure.

3. To solely address Exclusively applicable fix-its aren't testable #14, the fixes snapshot format would need to be changed to reflect each fix-it applied individually. The expansion that follows would still be driven by the assumption, often incorrectly, that it’s valid to apply all the fix-its simultaneously. (The final expansion closure would either be called expansion or fixedExpansion, depending on whether #1 above is done.)

4. To solely address Fix-it coverage lost when macros include diagnostics with and without fix-its #19, I think it’s as simple as changing the logic for including fixes from requiring that “all diagnostics have at least one fix-it” to “any of the diagnostics has a fix-it”. (Re-expansion would still fail silently.)

But also note that, while addressing all the above in one fell swoop isn’t trivial, it’s also not that big of a patch and can be conveyed reasonably simply to SMT users. Here are all the changes I made to AssertMacro.swift:

--- /tmp/pointfreeco-AssertMacro.swift	2024-07-18 19:08:24
+++ /tmp/gohanlon-AssertMacro.swift	2024-07-18 19:08:24
@@ -9,6 +9,70 @@
 import SwiftSyntaxMacros
 import XCTest
 
+//let compilableUsages = {
+//  assertMacro { "" }
+//  assertMacro { "" } expansion: { "" }
+//  assertMacro { "" } expansion: { "" } diagnostics: { "" }
+//  assertMacro { "" } expansion: { "" } diagnostics: { "" } fixes: { "" }
+//  assertMacro { "" } diagnostics: { "" }
+//  assertMacro { "" } diagnostics: { "" } fixes: { "" }
+//
+//  // technically allowed by signature
+//  assertMacro { "" } fixes: { "" }
+//
+//  // old usage
+//  assertMacro { "" } diagnostics: { "" } expansion: { "" }
+//  assertMacro { "" } fixes: { "" } expansion: { "" }
+//  assertMacro { "" } diagnostics: { "" } fixes: { "" } expansion: { "" }
+//
+//  return false
+//}()
+
+// Allow previous usage to compile during migration.
+// Fail any test where `expansion` is given after `diagnostics`/`fixes`.
+@_disfavoredOverload
+public func assertMacro(
+  _ macros: [String: Macro.Type]? = nil,
+  indentationWidth: Trivia? = nil,
+  record: SnapshotTestingConfiguration.Record? = nil,
+  of originalSource: () throws -> String,
+  diagnostics diagnosedSource: (() -> String)? = nil,
+  fixes fixedSource: (() -> String)? = nil,
+  expansion expandedSource: (() -> String)? = nil,
+  fileID: StaticString = #fileID,
+  file filePath: StaticString = #filePath,
+  function: StaticString = #function,
+  line: UInt = #line,
+  column: UInt = #column
+) {
+  if expandedSource != nil,
+    diagnosedSource != nil || fixedSource != nil
+  {
+    recordIssue(
+      "`expansion` must come before both `diagnostics` and `fixes`",
+      fileID: fileID,
+      filePath: filePath,
+      line: line,
+      column: column
+    )
+    return
+  }
+  assertMacro(
+    macros,
+    indentationWidth: indentationWidth,
+    record: record,
+    of: originalSource,
+    expansion: expandedSource,
+    diagnostics: diagnosedSource,
+    fixes: fixedSource,
+    fileID: fileID,
+    file: filePath,
+    function: function,
+    line: line,
+    column: column
+  )
+}
+
 /// Asserts that a given Swift source string matches an expected string with all macros expanded.
 ///
 /// To write a macro assertion, you simply pass the mapping of macros to expand along with the
@@ -94,6 +158,7 @@
 ///   - macros: The macros to expand in the original source string. Required, either implicitly via
 ///     ``withMacroTesting(indentationWidth:isRecording:macros:operation:)-5id9j``, or explicitly
 ///     via this parameter.
+///   - applyFixIts: Whether to assert the application of fix-its. Defaults to `true`.
 ///   - indentationWidth: The `Trivia` for setting indentation during macro expansion
 ///     (e.g., `.spaces(2)`). Defaults to the original source's indentation if unspecified. If the
 ///     original source lacks indentation, it defaults to `.spaces(4)`.
@@ -112,12 +177,13 @@
 ///     function.
 public func assertMacro(
   _ macros: [String: Macro.Type]? = nil,
+  applyFixIts: Bool = true,
   indentationWidth: Trivia? = nil,
   record: SnapshotTestingConfiguration.Record? = nil,
   of originalSource: () throws -> String,
+  expansion expandedSource: (() -> String)? = nil,
   diagnostics diagnosedSource: (() -> String)? = nil,
   fixes fixedSource: (() -> String)? = nil,
-  expansion expandedSource: (() -> String)? = nil,
   fileID: StaticString = #fileID,
   file filePath: StaticString = #filePath,
   function: StaticString = #function,
@@ -172,19 +241,19 @@
           )
         )
 
-      var context = BasicMacroExpansionContext(
+      let context = BasicMacroExpansionContext(
         sourceFiles: [
           origSourceFile: .init(moduleName: "TestModule", fullFilePath: "Test.swift")
         ]
       )
       #if canImport(SwiftSyntax600)
-        var expandedSourceFile = origSourceFile.expand(
+        let expandedSourceFile = origSourceFile.expand(
           macros: macros,
           contextGenerator: { _ in context },
           indentationWidth: indentationWidth
         )
       #else
-        var expandedSourceFile = origSourceFile.expand(
+        let expandedSourceFile = origSourceFile.expand(
           macros: macros,
           in: context,
           indentationWidth: indentationWidth
@@ -205,58 +275,62 @@
         )
       }
 
-      var allDiagnostics: [Diagnostic] { origDiagnostics + context.diagnostics }
-      if !allDiagnostics.isEmpty || diagnosedSource != nil {
-        offset += 1
+      // TODO: write a test where didExpand returns false
+      // For now, covered in MemberwiseInitTests.testAppliedToEnum_FailsWithDiagnostic
+      var didExpand: Bool {
+        let removingWhere: (AttributeSyntax) -> Bool = {
+          guard let name = $0.attributeName.as(IdentifierTypeSyntax.self)?.name.text
+          else { return false }
+          return macros.keys.contains(name)
+        }
 
-        let converter = SourceLocationConverter(fileName: "-", tree: origSourceFile)
-        let lineCount = converter.location(for: origSourceFile.endPosition).line
-        let diagnostics =
-          DiagnosticsFormatter
-          .annotatedSource(
-            tree: origSourceFile,
-            diags: allDiagnostics.map(anchor),
-            context: context,
-            contextSize: lineCount
-          )
-          .description
-          .replacingOccurrences(of: #"(^|\n) *\d* +│ "#, with: "$1", options: .regularExpression)
-          .trimmingCharacters(in: .newlines)
+        #if canImport(SwiftSyntax600)
+          let remover = MacroTesting.AttributeRemover600(removingWhere: removingWhere)
+        #else
+          let remover = MacroTesting.AttributeRemover509(removingWhere: removingWhere)
+        #endif
 
+        let removedSource = remover.rewrite(origSourceFile)
+
+        return expandedSourceFile.description.trimmingCharacters(in: .newlines)
+          != removedSource.description.trimmingCharacters(in: .newlines)
+      }
+
+      if didExpand {
+        offset += 1
         assertInlineSnapshot(
-          of: diagnostics,
+          of: expandedSourceFile.description.trimmingCharacters(in: .newlines),
           as: ._lines,
           message: """
-            Diagnostic output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
+            Expanded output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
             Difference: …
             """,
           syntaxDescriptor: InlineSnapshotSyntaxDescriptor(
             deprecatedTrailingClosureLabels: ["matches"],
-            trailingClosureLabel: "diagnostics",
+            trailingClosureLabel: "expansion",
             trailingClosureOffset: offset
           ),
-          matches: diagnosedSource,
-          fileID: fileID,
+          matches: expandedSource,
           file: filePath,
           function: function,
           line: line,
           column: column
         )
-      } else if diagnosedSource != nil {
+      } else if expandedSource != nil {
         offset += 1
         assertInlineSnapshot(
           of: nil,
           as: ._lines,
           message: """
-            Diagnostic output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
+            Expanded output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
             Difference: …
             """,
           syntaxDescriptor: InlineSnapshotSyntaxDescriptor(
             deprecatedTrailingClosureLabels: ["matches"],
-            trailingClosureLabel: "diagnostics",
+            trailingClosureLabel: "expansion",
             trailingClosureOffset: offset
           ),
-          matches: diagnosedSource,
+          matches: expandedSource,
           fileID: fileID,
           file: filePath,
           function: function,
@@ -265,79 +339,59 @@
         )
       }
 
-      if !allDiagnostics.isEmpty && allDiagnostics.allSatisfy({ !$0.fixIts.isEmpty }) {
+      let allDiagnostics: [Diagnostic] = origDiagnostics + context.diagnostics
+      if !allDiagnostics.isEmpty || diagnosedSource != nil {
         offset += 1
 
-        let edits =
-          context.diagnostics
-          .flatMap(\.fixIts)
-          .flatMap { $0.changes }
-          .map { $0.edit(in: context) }
-
-        var fixedSourceFile = origSourceFile
-        fixedSourceFile = Parser.parse(
-          source: FixItApplier.apply(
-            edits: edits, to: origSourceFile
+        let converter = SourceLocationConverter(fileName: "-", tree: origSourceFile)
+        let lineCount = converter.location(for: origSourceFile.endPosition).line
+        let diagnostics =
+          DiagnosticsFormatter
+          .annotatedSource(
+            tree: origSourceFile,
+            diags: allDiagnostics.map(anchor),
+            context: context,
+            contextSize: lineCount
           )
           .description
-        )
-        if let foldedSourceFile = try OperatorTable.standardOperators.foldAll(fixedSourceFile).as(
-          SourceFileSyntax.self
-        ) {
-          fixedSourceFile = foldedSourceFile
-        }
+          .replacingOccurrences(
+            of: #"(^|\n) *\d* +│ "#, with: "$1", options: .regularExpression
+          )
+          .trimmingCharacters(in: .newlines)
 
         assertInlineSnapshot(
-          of: fixedSourceFile.description.trimmingCharacters(in: .newlines),
+          of: diagnostics,
           as: ._lines,
           message: """
-            Fixed output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
+            Diagnostic output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
             Difference: …
             """,
           syntaxDescriptor: InlineSnapshotSyntaxDescriptor(
-            trailingClosureLabel: "fixes",
+            deprecatedTrailingClosureLabels: ["matches"],
+            trailingClosureLabel: "diagnostics",
             trailingClosureOffset: offset
           ),
-          matches: fixedSource,
-          fileID: fileID,
+          matches: diagnosedSource,
           file: filePath,
           function: function,
           line: line,
           column: column
         )
-
-        context = BasicMacroExpansionContext(
-          sourceFiles: [
-            fixedSourceFile: .init(moduleName: "TestModule", fullFilePath: "Test.swift")
-          ]
-        )
-        #if canImport(SwiftSyntax600)
-          expandedSourceFile = fixedSourceFile.expand(
-            macros: macros,
-            contextGenerator: { _ in context },
-            indentationWidth: indentationWidth
-          )
-        #else
-          expandedSourceFile = fixedSourceFile.expand(
-            macros: macros,
-            in: context,
-            indentationWidth: indentationWidth
-          )
-        #endif
-      } else if fixedSource != nil {
+      } else if diagnosedSource != nil {
         offset += 1
         assertInlineSnapshot(
           of: nil,
           as: ._lines,
           message: """
-            Fixed output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
+            Diagnostic output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
             Difference: …
             """,
           syntaxDescriptor: InlineSnapshotSyntaxDescriptor(
-            trailingClosureLabel: "fixes",
+            deprecatedTrailingClosureLabels: ["matches"],
+            trailingClosureLabel: "diagnostics",
             trailingClosureOffset: offset
           ),
-          matches: fixedSource,
+          matches: diagnosedSource,
           fileID: fileID,
           file: filePath,
           function: function,
@@ -346,42 +400,104 @@
         )
       }
 
-      if allDiagnostics.filter({ $0.diagMessage.severity == .error }).isEmpty {
+      if applyFixIts && !allDiagnostics.isEmpty
+        && allDiagnostics.contains(where: { !$0.fixIts.isEmpty })
+      {
         offset += 1
+
+        let diagnostics = allDiagnostics.filter { !$0.fixIts.isEmpty }
+
+        // NB: Only one of the fix-its can be applied at a time--they can be exclustive, e.g. two
+        // options for how to resolve an issue.
+        var fixedSourceLines = [String]()
+        for (diagnosticIndex, diagnostic) in diagnostics.enumerated() {
+          let diagnosticsString =
+            DiagnosticsFormatter
+            .annotatedSource(
+              tree: origSourceFile,
+              diags: [diagnostic].map(anchor),
+              context: context,
+              contextSize: 0
+            )
+            .description
+            .replacingOccurrences(
+              of: #"(^|\n) *\d* +│ "#, with: "$1", options: .regularExpression
+            )
+            .trimmingCharacters(in: .newlines)
+
+          let lines = diagnosticsString.split(separator: "\n")
+          let extraLeadingWhitespace =
+            lines.first?.prefix(while: \.isWhitespace).count ?? 0
+          var fixIts = diagnostic.fixIts
+          for (lineNumber, line) in lines.enumerated() {
+            if line.first(where: { !$0.isWhitespace }) != "✏️" {
+              let line = String(line.dropFirst(extraLeadingWhitespace))
+
+              if diagnosticIndex > 0 && lineNumber == 0 {
+                fixedSourceLines.append("")
+              }
+              fixedSourceLines.append(line)
+              continue
+            }
+            let line = String(line.drop(while: \.isWhitespace))
+
+            let fixIt = fixIts.removeFirst()
+
+            let edits = fixIt.changes
+              .map { $0.edit(in: context) }
+
+            var fixedSourceFile = Parser.parse(
+              source: FixItApplier.apply(
+                edits: edits,
+                to: origSourceFile
+              )
+              .description
+            )
+
+            if let foldedSourceFile = try OperatorTable.standardOperators.foldAll(
+              fixedSourceFile
+            )
+            .as(SourceFileSyntax.self) {
+              fixedSourceFile = foldedSourceFile
+            }
+
+            fixedSourceLines.append("")
+            fixedSourceLines.append(line)
+            fixedSourceLines.append(fixedSourceFile.description)
+          }
+        }
+
         assertInlineSnapshot(
-          of: expandedSourceFile.description.trimmingCharacters(in: .newlines),
+          of: fixedSourceLines.joined(separator: "\n").trimmingCharacters(in: .newlines),
           as: ._lines,
           message: """
-            Expanded output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
+            Fixed output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
             Difference: …
             """,
           syntaxDescriptor: InlineSnapshotSyntaxDescriptor(
-            deprecatedTrailingClosureLabels: ["matches"],
-            trailingClosureLabel: "expansion",
+            trailingClosureLabel: "fixes",
             trailingClosureOffset: offset
           ),
-          matches: expandedSource,
-          fileID: fileID,
+          matches: fixedSource,
           file: filePath,
           function: function,
           line: line,
           column: column
         )
-      } else if expandedSource != nil {
+      } else if fixedSource != nil {
         offset += 1
         assertInlineSnapshot(
           of: nil,
           as: ._lines,
           message: """
-            Expanded output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
+            Fixed output (\(newPrefix)) differed from expected output (\(oldPrefix)). \
             Difference: …
             """,
           syntaxDescriptor: InlineSnapshotSyntaxDescriptor(
-            deprecatedTrailingClosureLabels: ["matches"],
-            trailingClosureLabel: "expansion",
+            trailingClosureLabel: "fixes",
             trailingClosureOffset: offset
           ),
-          matches: expandedSource,
+          matches: fixedSource,
           fileID: fileID,
           file: filePath,
           function: function,
@@ -482,9 +598,9 @@
   indentationWidth: Trivia? = nil,
   record: SnapshotTestingConfiguration.Record? = nil,
   of originalSource: () throws -> String,
+  expansion expandedSource: (() -> String)? = nil,
   diagnostics diagnosedSource: (() -> String)? = nil,
   fixes fixedSource: (() -> String)? = nil,
-  expansion expandedSource: (() -> String)? = nil,
   fileID: StaticString = #fileID,
   file filePath: StaticString = #filePath,
   function: StaticString = #function,
@@ -496,9 +612,9 @@
     indentationWidth: indentationWidth,
     record: record,
     of: originalSource,
+    expansion: expandedSource,
     diagnostics: diagnosedSource,
     fixes: fixedSource,
-    expansion: expandedSource,
     fileID: fileID,
     file: filePath,
     function: function,

Addendum

Click to expand/collapse

Removing the automatic re-expansion of fixed source seems like a major regression: Why remove such a powerful feature? A surprising amount of reasoning lead me here.

Macros can emit both diagnostics and code simultaneously

Oftentimes, simultaneous emission of both diagnostics and code is a bug in the macro. Other times, this is the intended behavior. In both cases, the snapshot should capture the behavior. Currently, SMT omits the initial expansion when diagnostics are present, and overrides the initial expansion when fix-its are present.

Make expansion the first closure

Because expansion should be included regardless of whether it produced diagnostics and/or fix-its, it should not be assertMacro’s last closure, after diagnostics and fixes. Instead, expansion should be assertMacro’s first closure after the original source. Now, expansion will always reflect the initial expanded original source, and will only be omitted when the source code didn’t change as a result of expanding macros (see: “Determining whether macros expanded the source”):

public func assertMacro(
  // […]
  of originalSource: () throws -> String,
  expansion expandedOriginalSource: (() -> String)? = nil,  // 👈
  diagnostics diagnosedOriginalSource: (() -> String)? = nil,
  fixes fixedOriginalSource: (() -> String)? = nil,
)

Add fixesExpansion closure

To not lose the original expansion while still supporting the fixed expansion, assertMacro needs a new fixesExpansion closure in addition to the expansion closure:

public func assertMacro(
  // […]
  of originalSource: () throws -> String,
  expansion expandedOriginalSource: (() -> String)? = nil,
  diagnostics diagnosedOriginalSource: (() -> String)? = nil,
  fixes fixedOriginalSource: (() -> String)? = nil,
  fixesExpansion expandedFixedSource: (() -> String)? = nil  // 👈
)

Fix-its and diagnostics coverage

Because SMT only applies fix-its when all emitted diagnostics have one or more fix-its, fix-it coverage is lost when macros include diagnostics with and without fix-its. (See #19.) Instead, fixes should be included if any diagnostic has fix-its, even though it’s probable that the fixed source won’t resolve all the diagnostics. (I say “probable” because it is possible, yet unusual, for one fix-it to resolve multiple diagnostics.)

The expansion of fix-its benefits from the same expressiveness as the expansion of the original source. Expanding fixes can also emit both diagnostics and code simultaneously. Again, this can be a bug or intended behavior, and should be covered in the snapshot. Along with fixesExpansion, let’s also add fixesDiagnostics and fixesFixes:

public func assertMacro(
  // […]
  of originalSource: () throws -> String,
  expansion expandedOriginalSource: (() -> String)? = nil,
  diagnostics diagnosedOriginalSource: (() -> String)? = nil,
  fixes fixedOriginalSource: (() -> String)? = nil,
  fixesExpansion expandedFixedSource: (() -> String)? = nil,
  fixesDiagnostics diagnosedFixedSource: (() -> String)? = nil,  // 👈
  fixesFixes fixedSourceFixes (() -> String)? = nil  // 👈
)

Already we’ve made some significant improvements. SMT currently fails “silently” when expanding the fixed source results in diagnostics. Now, the expansion of the fixed source is always represented in the snapshot, including when diagnostics and fix-its. Even the application of fix-its as a result of expanding fixedOriginalSource is represented (in the awkwardly named fixesFixes).

However, we can also already see the increasing complexity introduced by automatically re-expanding fixed source, as we grapple with multiple layers of expansion, diagnostics, and fix-its.

Challenges with simultaneously applying fix-its

Another shortcoming concerning fix-its: Not all emitted fix-its can be validly applied simultaneously. (See #14.) The only safe assumption is that any single fix-it should apply-able. The assumption that all emitted fix-its can be applied simultaneously is the basis of SMT’s current behavior.

Multiple fix-its can often be applied simultaneously, but not always. In my MemberwiseInit macro, the emitted fix-its often can’t all be applied simultaneously. SMT needs additional configuration in order to know how to behave:

enum AssertFixItsConfig {
  case individually
  case combined
}

public func assertMacro(
  // […]
  assertFixIts: AssertFixItsConfig = .individually,  // 👈
  of originalSource: () throws -> String,
  expansion expandedOriginalSource: (() -> String)? = nil,
  diagnostics diagnosedOriginalSource: (() -> String)? = nil,
  fixes fixedOriginalSource: (() -> String)? = nil,
  fixesExpansion expandedFixedSource: (() -> String)? = nil
  fixesDiagnostics diagnosedFixedSource: (() -> String)? = nil,
  fixesFixes fixedSourceFixes (() -> String)? = nil
)

Note
When the original expansion emits a lone fix-it, it’s clearly okay to assume “all” the fix-its can be applied simultaneously and expanded. In this case, SMT could safely default to the combined behavior instead of individually.

Differentiating individually and combined configurations

Importantly, the fixes string format will differ substantially between individually and combined configurations. combined can use the current SMT fixes format of being a single block of source code. But individually needs to be a new format that reflects the application of each fix-it individually and the source that results from each fix. (See: “New fixes string format”.)

Disabling re-expansion in individually configuration

When applying fix-its individually, assertMacro can’t reasonably expand each fixed source. The volume of code resulting from several expansions is already a likely non-starter. But, like the other instances of macro expansion, each expansion would in turn need to include expansion, diagnostics and fixes. Even if this could somehow be represented in the snapshot, applying some but not all the fix-its typically will result in a redundant subset of the original diagnostics and fix-its.

So, individually disables the re-expansion of fixed source; fixesExpansion, fixesDiagnostics, and fixesFixes will be omitted.

Addressing redundant fix-its and re-expansions

That’s a lot of ground covered. Since we now have configuration over how fix-its are asserted on, let’s build on that configuration to address some more concerns.

Certain tests may redundantly apply fix-its already covered in other tests. (See #17.) Therefore, a feature enabling developers to opt out of fix-it applications in specific tests where they are unnecessary could be beneficial:

enum AssertFixItsConfig {
  case none  // 👈
  case individually
  case combined
}

Similarly, the re-expansion of fixed source can be redundant with other tests. Let’s make that configurable, too:

enum AssertFixItsConfig {
  case none
  case individually
  case combined(expand: Bool)  // 👈
}

Convention over configuration for fixesFixes

Technically, when re-expanding the combined fixes, the fixesFixes can be redundant, too. Should the option to re-expand the combined fixes also be configurable? Additionally, should we make it configurable whether fixesFixes be the fix-its applied "individually" or "combined"?

The configuration complexity is getting a bit out of control. Instead, we can be (conveniently) opinionated and decide that assertMacro(assertFixIts: .combined) should always include fixesFixes when re-expanding the combined fixed source emits fix-its.

Similarly, in order to avoid configuration of the fixesFixes format, we can conveniently decide that it be that of individually, the only safe choice. And since SMT will never go three levels deep by re-expanding fixesFixes, this doesn’t trade off any expressiveness. (It does trade off verboseness by using the longer fixes format that results from applying fixes separately, tho.)

Reconsidering the value of automatically expanding fixed source

This raises a final, overarching question: Would it be better if assertMacro simply never re-expanded fixed source?

A lot of SMT usage complexity and internal complexity is saved by instead requiring separate invocations of assertMacro for each desired expansion:

enum AssertFixItsConfig {
  case none
  case individually
  case combined
}

public func assertMacro(
  assertFixIts: AssertFixItsConfig = .individually,
  of originalSource: () throws -> String,
  expansion expandedOriginalSource: (() -> String)? = nil,
  diagnostics diagnosedOriginalSource: (() -> String)? = nil,
  fixes fixedOriginalSource: (() -> String)? = nil,
)

Requiring two separate assertMacro invocations creates the opportunity for inconsistency between the fixes of the first assert and the orginalSource of the next assert. I don’t find this burdensome in my own macros because it’s straightforward enough to see whether the fixed source is correct. Additionally, the expansion of the fixed source is already tested by a pre-existing “happy path” instance of assertMacro—a test that covers expanding of the fixed source has already been written.

I also have several tests with only slight differences. For example, when a declaration has no access modifier my macro emits a fix-it titled “Add ‘public’ access”, but when it has an access modifier already, the fix-it is titled “Replace ‘private’ access with ‘public’ access”. Both those fix-its result in the same fixed source, so re-expanding it is largely redundant.

We could go even further in simplifying configuration, and since we’re not needing to support re-expanding fixed source, we could always apply fix-its individually and replace AssertFixItsConfig with a simple Bool to allow opting-out of fixes:

public func assertMacro(
  assertFixIts: Bool = true,
  of originalSource: () throws -> String,
  expansion expandedOriginalSource: (() -> String)? = nil,
  diagnostics diagnosedOriginalSource: (() -> String)? = nil,
  fixes fixedOriginalSource: (() -> String)? = nil,
)

Note
For future consideration, I do propose that the fixes string format could be configurable. See “Potential feature: Configurable fixes string formats”.

Addendum to the addendum

New fixes string format

Here’s the proposed format, showing a single fix-it:

func testLetWithMultipleBindings() {
  assertMacro {
    """
    @MemberwiseInit
    struct S {
      @Init(default: 42) let x, y: Int
    }
    """
  } expansion: {
    """
    struct S {
      let x, y: Int

      internal init() {
      }
    }
    """
  } diagnostics: {
    """
    @MemberwiseInit
    struct S {
      @Init(default: 42) let x, y: Int
            ┬──────────
            ╰─ 🛑 Custom 'default' can't be applied to multiple bindings
               ✏️ Remove '@Init(default: 42)'
    }
    """
  } fixes: {
    """
    @Init(default: 42) let x, y: Int
          ┬──────────
          ╰─ 🛑 Custom 'default' can't be applied to multiple bindings

    ✏️ Remove '@Init(default: 42)'
    @MemberwiseInit
    struct S {
      let x, y: Int
    }
    """
  }
}

Repeating the diagnostic may seem unnecessary, but it makes the fixes easier to interpret when the original source is much longer and I appreciate the consistency of it always being included. It’s also more useful when a diagnostic has multiple fix-its, and essential when multiple diagnostics have fix-its.

Here’s an example with multiple fix-its:

func testInitializedLet() {
  assertMacro {
    """
    @MemberwiseInit
    struct S {
      @Init(default: 42) let number = 0
    }
    """
  } expansion: {
    """
    struct S {
      let number = 0

      internal init() {
      }
    }
    """
  } diagnostics: {
    """
    @MemberwiseInit
    struct S {
      @Init(default: 42) let number = 0
            ┬──────────
            ╰─ 🛑 @Init can't be applied to already initialized constant
               ✏️ Remove '@Init(default: 42)'
               ✏️ Remove '= 0'
    }
    """
  } fixes: {
    """
    @Init(default: 42) let number = 0
          ┬──────────
          ╰─ 🛑 @Init can't be applied to already initialized constant

    ✏️ Remove '@Init(default: 42)'
    @MemberwiseInit
    struct S {
      let number = 0
    }

    ✏️ Remove '= 0'
    @MemberwiseInit
    struct S {
      @Init(default: 42) let number: Int
    }
    """
  }
}

Here’s an example with 3 diagnostics, where two of them have fix-its:

func testAccessLeakCustomDefaultAndInvalidLabelOnMultipleBindings() {
  assertMacro {
    """
    @MemberwiseInit(.public)
    struct S {
      @Init(default: 0, label: "$foo") private let x, y: T
    }
    """
  } expansion: {
    """
    struct S {
      private let x, y: T

      public init() {
      }
    }
    """
  } diagnostics: {
    """
    @MemberwiseInit(.public)
    struct S {
      @Init(default: 0, label: "$foo") private let x, y: T
                                       ┬──────
            │           │              ╰─ 🛑 @MemberwiseInit(.public) would leak access to 'private' property
            │           │                 ✏️ Add '@Init(.public)'
            │           │                 ✏️ Replace 'private' access with 'public'
            │           │                 ✏️ Add '@Init(.ignore)' and an initializer
                        ┬────────────
            │           ╰─ 🛑 Custom 'label' can't be applied to multiple bindings
            ┬──────────
            ╰─ 🛑 Custom 'default' can't be applied to multiple bindings
               ✏️ Remove 'default: 0'
    }
    """
  } fixes: {
    """
    @Init(default: 0, label: "$foo") private let x, y: T
          ┬──────────
          ╰─ 🛑 Custom 'default' can't be applied to multiple bindings

    ✏️ Remove 'default: 0'
    @MemberwiseInit(.public)
    struct S {
      @Init(label: "$foo") private let x, y: T
    }

    @Init(default: 0, label: "$foo") private let x, y: T
                                     ┬──────
                                     ╰─ 🛑 @MemberwiseInit(.public) would leak access to 'private' property

    ✏️ Add '@Init(.public)'
    @MemberwiseInit(.public)
    struct S {
      @Init(.public, default: 0, label: "$foo") private let x, y: T
    }

    ✏️ Replace 'private' access with 'public'
    @MemberwiseInit(.public)
    struct S {
      @Init(default: 0, label: "$foo") public let x, y: T
    }

    ✏️ Add '@Init(.ignore)' and an initializer
    @MemberwiseInit(.public)
    struct S {
      @Init(.ignore) private let x = <#value#>, y: T = <#value#>
    }
    """
  }
}

Potential future feature: Configurable fixes string formats

Including the full fixed source for multiple fix-its can be quite verbose, particularly when the original source or expanded original source is long. SMT can help by offering additional string formats:

enum FixItStringFormat {
  case changedRange
  case fullSource
  case diff
}
enum AssertFixItsConfig {
  case none
  case individually(format: FixItStringFormat)
  case combined
}

Note
Technically, the combined case could also contain the same configurable formats. Aside from consistency, I don’t (yet?) have a strong motivating rationale for it, tho. This needs proper consideration.

fullSource

Show the full fixed source from applying each fix.

changedRange

Fix-its could be shortened to only include the beginning and end of the changed source range. Many fix-its become a single line, which in my macros has been by far the most common case:

} fixes: {
  """
  @Init(default: 42) let number = 0
        ┬──────────
        ╰─ 🛑 @Init can't be applied to already initialized constant
  
  ✏️ Remove '@Init(default: 42)'
  let number = 0
  
  ✏️ Remove '= 0'
  @Init(default: 42) let number: Int
  """
}

I haven’t fully considered how functional this changedRange format would be in more complex fix-it cases, e.g. a fix-it that makes changes at multiple, separate locations. But should developers face unideal output when using changedRange, they can fall back to fullSource.

diff

Fix-its can target far away nodes, not just their current line, and they can make changes at multiple locations. That can be represented with a diff.

(Developers would then need to contend with diffs of diffs, which is not fun, but this being an opt-in behavior implies that they’re contending with an alternative that’s even worse.)

I don’t have a real example of this, but to illustrate a potential diff format, here’s a contrived example where the diagnostic on a child could apply a fix on a parent:

assertMacro {
  """
  @MemberwiseInit(.public)
  struct S {
    private let x: T
  }
  """
} expansion: {
  """
  struct S {
    private let x: T

    public init() {
    }
  }
  """
} diagnostics: {
  """
  @MemberwiseInit(.public)
  struct S {
    private let x: T
    ┬──────
    ╰─ 🛑 @MemberwiseInit(.public) would leak access to 'private' property
       ✏️ Change to '@MemberwiseInit(.private)'
  }
  """
} fixes: {
  """
  private let x: T
  ┬──────
  ╰─ 🛑 @MemberwiseInit(.public) would leak access to 'private' property
  
  ✏️ Change to '@MemberwiseInit(.private)'
  -@MemberwiseInit(.public)
  +@MemberwiseInit(.private)
  """
}

Determining whether macros expanded the source

Ideally, the SwiftSyntaxMacroExpansion/MacroSystem API would expose whether macros expanded the source. To my knowledge, it does not, and I don’t know of a straightforward solution.

One solution is to pull in the upstream AttributeRemover used by MacroSystem, and use it to remove the macro attributes from the original source. Then, use string comparison with the expanded source to see if the macro expansion did more than remove the macro attributes.

The upstream AttributeRemover hasn’t changed in the 509.0.x series of swift-syntax releases, but has been significantly improved on main (see PR #2215).

Because swift-macro-testing supports swift-syntax from: "509.0.0", all released versions of AttributeRemover in that series would need to be accounted for. So, we’d try multiple AttributeRemovers, e.g. AttributeRemover50900 and AttributeRemover50903 (hypothetically), and any if of those attribute-remover outputs match the macro expansion, consider that source to be unchanged.

One oddity when using this strategy is that when original source doesn't expand at all and there are no diagnostics/fixes, the assertMacro passes and remains as it was before running the test:

// After recording, `assertMacro` is unchanged. (It passes, tho.)
func testExpansionIgnoresVariables() {
  assertMacro {
    """
    @PeerValueWithSuffixName
    var someVariable: Int
    """
  }
}

I don't think this is necessarily bad, in fact, I appreciate that when the expansion is omitted, you know that the macro didn't do anything, saving you from visually "diffing" the original source and the expansion to make sure nothing unexpectedly changed. Still, it may be preferable to always include expansion when neither diagnostics/fixes are not present.

Note
The AttributeRemover in the 509.0.x series of swift-syntax would benefit from some minor back-porting to shift from being node identity-based to predicate-based. Before PR #2215, you had to first collect the attribute nodes to remove from the syntax tree, and then provide those nodes to AttributeRemover for removal. [Update: I implemented this in MemberwiseInit, see AttributeRemover509.swift.]

Note
swift-syntax PR #2215 was not included in the 509.0.2 release, despite being merged into main a few weeks prior. This implies that changes to AttributeRemover between patch versions will be infrequent/manageable. [Update: PR #2215 is included in 600.0.0-prerelease, after missing both the 509.x.x and 510.x.x series of releases. MemberwiseInit's SMT also has the new AttributeRemover600.swift.]

The assumption that all fix-its can be applied simultaneously is pervasive

Assumptions about how fix-its can be applied are present in at least both swift-syntax and in Xcode. Xcode has Editor → Fix All Issues , and FixItApplier has an API that assumes that all fix-its can be applied together. FixItAppiler has the added heuristic that fix-its that apply to overlapping ranges can’t be applied together. In that case, FixItApplier only applies the first of the overlapping fix-its. When that occurs in an assertMacro snapshot, it can be quite hard to work out what happened. (Is there a bug in a fix-it? Which fix-it was applied and which was skipped?) This heuristic, already lacking, also doesn’t address that even non-overlapping fix-its can be invalid to apply together.