Merge pull request #2941 from rintaro/swiftdiags-replace-trivia

[Diagnostics] Fix and adjust FixIt application

Author: rintaro

Sources/SwiftCompilerPluginMessageHandling/Diagnostics.swift

@@ -110,20 +110,37 @@ extension PluginMessage.Diagnostic {
           let text: String
           switch $0 {
           case .replace(let oldNode, let newNode):
+            // Replace the whole node including leading/trailing trivia, but if
+            // the trivia are the same, don't include them in the replacing range.
+            let leadingMatch = oldNode.leadingTrivia == newNode.leadingTrivia
+            let trailingMatch = oldNode.trailingTrivia == newNode.trailingTrivia
             range = sourceManager.range(
               of: oldNode,
-              from: .afterLeadingTrivia,
-              to: .beforeTrailingTrivia
+              from: leadingMatch ? .afterLeadingTrivia : .beforeLeadingTrivia,
+              to: trailingMatch ? .beforeTrailingTrivia : .afterTrailingTrivia
             )
-            text = newNode.trimmedDescription
+            var newNode = newNode.detached
+            if leadingMatch {
+              newNode.leadingTrivia = []
+            }
+            if trailingMatch {
+              newNode.trailingTrivia = []
+            }
+            text = newNode.description
           case .replaceLeadingTrivia(let token, let newTrivia):
+            guard token.leadingTrivia != newTrivia else {
+              return nil
+            }
             range = sourceManager.range(
               of: Syntax(token),
               from: .beforeLeadingTrivia,
               to: .afterLeadingTrivia
             )
             text = newTrivia.description
           case .replaceTrailingTrivia(let token, let newTrivia):
+            guard token.trailingTrivia != newTrivia else {
+              return nil
+            }
             range = sourceManager.range(
               of: Syntax(token),
               from: .beforeTrailingTrivia,

Sources/SwiftDiagnostics/FixIt.swift

@@ -97,7 +97,9 @@ extension FixIt {
   public var edits: [SourceEdit] {
     var existingEdits = [SourceEdit]()
     for change in changes {
-      let edit = change.edit
+      guard let edit = change.edit else {
+        continue
+      }
       let isOverlapping = existingEdits.contains { edit.range.overlaps($0.range) }
       if !isOverlapping {
         // The edit overlaps with the previous edit. We can't apply both
@@ -111,21 +113,40 @@ extension FixIt {
 }
 
 private extension FixIt.Change {
-  var edit: SourceEdit {
+  var edit: SourceEdit? {
     switch self {
     case .replace(let oldNode, let newNode):
+      // Replace the whole node including leading/trailing trivia, but if
+      // the trivia are the same, don't include them in the replacing range.
+      let leadingMatch = oldNode.leadingTrivia == newNode.leadingTrivia
+      let trailingMatch = oldNode.trailingTrivia == newNode.trailingTrivia
+      let start = leadingMatch ? oldNode.positionAfterSkippingLeadingTrivia : oldNode.position
+      let end = trailingMatch ? oldNode.endPositionBeforeTrailingTrivia : oldNode.endPosition
+      var newNode = newNode.detached
+      if leadingMatch {
+        newNode.leadingTrivia = []
+      }
+      if trailingMatch {
+        newNode.trailingTrivia = []
+      }
       return SourceEdit(
-        range: oldNode.position..<oldNode.endPosition,
+        range: start..<end,
         replacement: newNode.description
       )
 
     case .replaceLeadingTrivia(let token, let newTrivia):
+      guard token.leadingTrivia != newTrivia else {
+        return nil
+      }
       return SourceEdit(
         range: token.position..<token.positionAfterSkippingLeadingTrivia,
         replacement: newTrivia.description
       )
 
     case .replaceTrailingTrivia(let token, let newTrivia):
+      guard token.trailingTrivia != newTrivia else {
+        return nil
+      }
       return SourceEdit(
         range: token.endPositionBeforeTrailingTrivia..<token.endPosition,
         replacement: newTrivia.description

Sources/SwiftIDEUtils/FixItApplier.swift

@@ -36,6 +36,7 @@ public enum FixItApplier {
   ) -> String {
     let messages = messages ?? diagnostics.compactMap { $0.fixIts.first?.message.message }
 
+    // FIXME: This assumes every fix-it is applied to a node in the 'tree', which is not guaranteed.
     let edits =
       diagnostics
       .flatMap(\.fixIts)

Tests/SwiftDiagnosticsTest/FixItTests.swift

@@ -18,14 +18,14 @@ import _SwiftSyntaxTestSupport
 
 final class FixItTests: XCTestCase {
   func testEditsForFixIt() throws {
-    let markedSource = "protocol 1️⃣Multi 2️⃣ident 3️⃣{}"
+    let markedSource = "protocol 1️⃣Multi2️⃣ 3️⃣ident 4️⃣{}"
     let (markers, source) = extractMarkers(markedSource)
     let positions = markers.mapValues { AbsolutePosition(utf8Offset: $0) }
-    XCTAssertEqual(positions.count, 3)
+    XCTAssertEqual(positions.count, 4)
 
     let expectedEdits = [
-      SourceEdit(range: positions["1️⃣"]!..<positions["2️⃣"]!, replacement: "Multiident "),
-      SourceEdit(range: positions["2️⃣"]!..<positions["3️⃣"]!, replacement: ""),
+      SourceEdit(range: positions["1️⃣"]!..<positions["2️⃣"]!, replacement: "Multiident"),
+      SourceEdit(range: positions["3️⃣"]!..<positions["4️⃣"]!, replacement: ""),
     ]
     let tree = Parser.parse(source: source)
     let diags = ParseDiagnosticsGenerator.diagnostics(for: tree)