Merge pull request swiftlang#2226 from kimdv/kimdv/rewrite-fit-it-applier

Rewrite FixItApplier to be string based

Author: kimdv

Sources/SwiftParserDiagnostics/DiagnosticExtensions.swift

@@ -48,28 +48,21 @@ extension FixIt {
 
 extension FixIt.MultiNodeChange {
   /// Replaced a present token with a missing node.
+  ///
   /// If `transferTrivia` is `true`, the leading and trailing trivia of the
   /// removed node will be transferred to the trailing trivia of the previous token.
   static func makeMissing(_ token: TokenSyntax, transferTrivia: Bool = true) -> Self {
     return makeMissing([token], transferTrivia: transferTrivia)
   }
 
   /// Replace present tokens with missing tokens.
-  /// If `transferTrivia` is `true`, the leading and trailing trivia of the
-  /// removed node will be transferred to the trailing trivia of the previous token.
+  ///
+  /// If `transferTrivia` is `true`, the leading trivia of the first token and
+  /// the trailing trivia of the last token will be transferred to their adjecent
+  /// tokens.
   static func makeMissing(_ tokens: [TokenSyntax], transferTrivia: Bool = true) -> Self {
-    precondition(!tokens.isEmpty)
-    precondition(tokens.allSatisfy({ $0.isPresent }))
-    var changes = tokens.map {
-      FixIt.Change.replace(
-        oldNode: Syntax($0),
-        newNode: Syntax($0.with(\.presence, .missing))
-      )
-    }
-    if transferTrivia {
-      changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: tokens).primitiveChanges
-    }
-    return FixIt.MultiNodeChange(primitiveChanges: changes)
+    precondition(tokens.allSatisfy(\.isPresent))
+    return .makeMissing(tokens.map(Syntax.init), transferTrivia: transferTrivia)
   }
 
   /// If `transferTrivia` is `true`, the leading and trailing trivia of the
@@ -104,6 +97,25 @@ extension FixIt.MultiNodeChange {
       return FixIt.MultiNodeChange()
     }
   }
+
+  /// Replace present nodes with their missing equivalents.
+  ///
+  /// If `transferTrivia` is `true`, the leading trivia of the first node and
+  /// the trailing trivia of the last node will be transferred to their adjecent
+  /// tokens.
+  static func makeMissing(_ nodes: [Syntax], transferTrivia: Bool = true) -> Self {
+    precondition(!nodes.isEmpty)
+    var changes = nodes.map {
+      FixIt.Change.replace(
+        oldNode: $0,
+        newNode: MissingMaker().rewrite($0, detach: true)
+      )
+    }
+    if transferTrivia {
+      changes += FixIt.MultiNodeChange.transferTriviaAtSides(from: nodes).primitiveChanges
+    }
+    return FixIt.MultiNodeChange(primitiveChanges: changes)
+  }
 }
 
 // MARK: - Make present

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

@@ -182,7 +182,6 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
       correctToken.isMissing
     {
       // We are exchanging two adjacent tokens, transfer the trivia from the incorrect token to the corrected token.
-      changes += misplacedTokens.map { FixIt.MultiNodeChange.makeMissing($0, transferTrivia: false) }
       changes.append(
         FixIt.MultiNodeChange.makePresent(
           correctToken,
@@ -191,6 +190,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
           trailingTrivia: misplacedToken.trailingTrivia.isEmpty ? nil : misplacedToken.trailingTrivia
         )
       )
+      changes.append(FixIt.MultiNodeChange.makeMissing(misplacedTokens, transferTrivia: false))
     } else {
       changes += misplacedTokens.map { FixIt.MultiNodeChange.makeMissing($0) }
       changes += correctAndMissingTokens.map { FixIt.MultiNodeChange.makePresent($0) }
@@ -236,7 +236,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     exchangeTokens(
       unexpected: misplacedSpecifiers,
       unexpectedTokenCondition: { EffectSpecifier(token: $0) != nil },
-      correctTokens: [effectSpecifiers?.throwsSpecifier, effectSpecifiers?.asyncSpecifier],
+      correctTokens: [effectSpecifiers?.asyncSpecifier, effectSpecifiers?.throwsSpecifier],
       message: { EffectsSpecifierAfterArrow(effectsSpecifiersAfterArrow: $0) },
       moveFixIt: { MoveTokensInFrontOfFixIt(movedTokens: $0, inFrontOf: .arrow) },
       removeRedundantFixIt: { RemoveRedundantFixIt(removeTokens: $0) }
@@ -764,20 +764,17 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
     if let unexpected = node.unexpectedBetweenRequirementAndTrailingComma,
       let token = unexpected.presentTokens(satisfying: { $0.tokenKind == .binaryOperator("&&") }).first,
       let trailingComma = node.trailingComma,
-      trailingComma.isMissing,
-      let previous = node.unexpectedBetweenRequirementAndTrailingComma?.previousToken(viewMode: .sourceAccurate)
+      trailingComma.isMissing
     {
-
       addDiagnostic(
         unexpected,
         .expectedCommaInWhereClause,
         fixIts: [
           FixIt(
             message: ReplaceTokensFixIt(replaceTokens: [token], replacements: [.commaToken()]),
             changes: [
-              .makeMissing(token),
-              .makePresent(trailingComma),
-              FixIt.MultiNodeChange(.replaceTrailingTrivia(token: previous, newTrivia: [])),
+              .makeMissing(token, transferTrivia: false),
+              .makePresent(trailingComma, leadingTrivia: token.leadingTrivia, trailingTrivia: token.trailingTrivia),
             ]
           )
         ],
@@ -818,7 +815,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
         fixIts: [
           FixIt(
             message: RemoveNodesFixIt(nodes),
-            changes: nodes.map { .makeMissing($0) }
+            changes: .makeMissing(nodes)
           )
         ],
         handledNodes: nodes.map { $0.id }
@@ -1542,7 +1539,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
         fixIts: [
           FixIt(
             message: RemoveNodesFixIt(rawDelimiters),
-            changes: rawDelimiters.map { .makeMissing($0) }
+            changes: .makeMissing(rawDelimiters)
           )
         ],
         handledNodes: rawDelimiters.map { $0.id }
@@ -1862,8 +1859,8 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
               replacements: [node.colon]
             ),
             changes: [
-              FixIt.MultiNodeChange.makeMissing(equalToken),
-              FixIt.MultiNodeChange.makePresent(node.colon),
+              .makeMissing(equalToken, transferTrivia: false),
+              .makePresent(node.colon, leadingTrivia: equalToken.leadingTrivia, trailingTrivia: equalToken.trailingTrivia),
             ]
           )
         ],
@@ -1971,8 +1968,9 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
             FixIt(
               message: fixItMessage,
               changes: [
-                FixIt.MultiNodeChange.makePresent(detail.detail)
-              ] + unexpectedTokens.map { FixIt.MultiNodeChange.makeMissing($0) }
+                .makePresent(detail.detail),
+                .makeMissing(unexpectedTokens),
+              ]
             )
           ],
           handledNodes: [detail.id] + unexpectedTokens.map(\.id)

Sources/_SwiftSyntaxTestSupport/FixItApplier.swift

@@ -0,0 +1,114 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2023 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 the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import SwiftDiagnostics
+import SwiftSyntax
+
+public enum FixItApplier {
+  struct Edit: Equatable {
+    var startUtf8Offset: Int
+    var endUtf8Offset: Int
+    let replacement: String
+
+    var replacementLength: Int {
+      return replacement.utf8.count
+    }
+
+    var replacementRange: Range<Int> {
+      return startUtf8Offset..<endUtf8Offset
+    }
+  }
+
+  /// Applies selected or all Fix-Its from the provided diagnostics to a given syntax tree.
+  ///
+  /// - Parameters:
+  ///   - diagnostics: An array of `Diagnostic` objects, each containing one or more Fix-Its.
+  ///   - filterByMessages: An optional array of message strings to filter which Fix-Its to apply.
+  ///     If `nil`, the first Fix-It from each diagnostic is applied.
+  ///   - tree: The syntax tree to which the Fix-Its will be applied.
+  ///
+  /// - Returns: A ``String`` representation of the modified syntax tree after applying the Fix-Its.
+  public static func applyFixes(
+    from diagnostics: [Diagnostic],
+    filterByMessages messages: [String]?,
+    to tree: any SyntaxProtocol
+  ) -> String {
+    let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message }
+
+    let changes =
+      diagnostics
+      .flatMap(\.fixIts)
+      .filter { messages.contains($0.message.message) }
+      .flatMap(\.changes)
+
+    var edits: [Edit] = changes.map(\.edit)
+    var source = tree.description
+
+    while let edit = edits.first {
+      edits = Array(edits.dropFirst())
+
+      let startIndex = source.utf8.index(source.utf8.startIndex, offsetBy: edit.startUtf8Offset)
+      let endIndex = source.utf8.index(source.utf8.startIndex, offsetBy: edit.endUtf8Offset)
+
+      source.replaceSubrange(startIndex..<endIndex, with: edit.replacement)
+
+      edits = edits.compactMap { remainingEdit -> FixItApplier.Edit? in
+        var remainingEdit = remainingEdit
+
+        if remainingEdit.replacementRange.overlaps(edit.replacementRange) {
+          // The edit overlaps with the previous edit. We can't apply both
+          // without conflicts. Apply the one that's listed first and drop the
+          // later edit.
+          return nil
+        }
+
+        // If the remaining edit starts after or at the end of the edit that we just applied,
+        // shift it by the current edit's difference in length.
+        if edit.endUtf8Offset <= remainingEdit.startUtf8Offset {
+          remainingEdit.startUtf8Offset = remainingEdit.startUtf8Offset - edit.replacementRange.count + edit.replacementLength
+          remainingEdit.endUtf8Offset = remainingEdit.endUtf8Offset - edit.replacementRange.count + edit.replacementLength
+        }
+
+        return remainingEdit
+      }
+    }
+
+    return source
+  }
+}
+
+fileprivate extension FixIt.Change {
+  var edit: FixItApplier.Edit {
+    switch self {
+    case .replace(let oldNode, let newNode):
+      return FixItApplier.Edit(
+        startUtf8Offset: oldNode.position.utf8Offset,
+        endUtf8Offset: oldNode.endPosition.utf8Offset,
+        replacement: newNode.description
+      )
+
+    case .replaceLeadingTrivia(let token, let newTrivia):
+      return FixItApplier.Edit(
+        startUtf8Offset: token.position.utf8Offset,
+        endUtf8Offset: token.positionAfterSkippingLeadingTrivia.utf8Offset,
+        replacement: newTrivia.description
+      )
+
+    case .replaceTrailingTrivia(let token, let newTrivia):
+      return FixItApplier.Edit(
+        startUtf8Offset: token.endPositionBeforeTrailingTrivia.utf8Offset,
+        endUtf8Offset: token.endPosition.utf8Offset,
+        replacement: newTrivia.description
+      )
+    }
+  }
+}

Tests/SwiftParserTest/Assertions.swift

@@ -276,58 +276,6 @@ struct DiagnosticSpec {
   }
 }
 
-class FixItApplier: SyntaxRewriter {
-  var changes: [FixIt.Change]
-
-  init(diagnostics: [Diagnostic], withMessages messages: [String]?) {
-    let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message }
-
-    self.changes =
-      diagnostics
-      .flatMap { $0.fixIts }
-      .filter {
-        return messages.contains($0.message.message)
-      }
-      .flatMap { $0.changes }
-
-    super.init(viewMode: .all)
-  }
-
-  public override func visitAny(_ node: Syntax) -> Syntax? {
-    for change in changes {
-      switch change {
-      case .replace(oldNode: let oldNode, newNode: let newNode) where oldNode.id == node.id:
-        return newNode
-      default:
-        break
-      }
-    }
-    return nil
-  }
-
-  override func visit(_ node: TokenSyntax) -> TokenSyntax {
-    var modifiedNode = node
-    for change in changes {
-      switch change {
-      case .replaceLeadingTrivia(token: let changedNode, newTrivia: let newTrivia) where changedNode.id == node.id:
-        modifiedNode = node.with(\.leadingTrivia, newTrivia)
-      case .replaceTrailingTrivia(token: let changedNode, newTrivia: let newTrivia) where changedNode.id == node.id:
-        modifiedNode = node.with(\.trailingTrivia, newTrivia)
-      default:
-        break
-      }
-    }
-    return modifiedNode
-  }
-
-  /// If `messages` is `nil`, applies all Fix-Its in `diagnostics` to `tree` and returns the fixed syntax tree.
-  /// If `messages` is not `nil`, applies only Fix-Its whose message is in `messages`.
-  public static func applyFixes<T: SyntaxProtocol>(in diagnostics: [Diagnostic], withMessages messages: [String]?, to tree: T) -> Syntax {
-    let applier = FixItApplier(diagnostics: diagnostics, withMessages: messages)
-    return applier.rewrite(tree)
-  }
-}
-
 /// Assert that `location` is the same as that of `locationMarker` in `tree`.
 func assertLocation<T: SyntaxProtocol>(
   _ location: SourceLocation,
@@ -679,7 +627,7 @@ extension ParserTestCase {
     if expectedDiagnostics.contains(where: { !$0.fixIts.isEmpty }) && expectedFixedSource == nil {
       XCTFail("Expected a fixed source if the test case produces diagnostics with Fix-Its", file: file, line: line)
     } else if let expectedFixedSource = expectedFixedSource {
-      let fixedTree = FixItApplier.applyFixes(in: diags, withMessages: applyFixIts, to: tree)
+      let fixedTree = FixItApplier.applyFixes(from: diags, filterByMessages: applyFixIts, to: tree)
       var fixedTreeDescription = fixedTree.description
       if options.contains(.normalizeNewlinesInFixedSource) {
         fixedTreeDescription =