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

[ASTGen/MacroEvaluation] Fix and adjust FixIt application

Author: rintaro

lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift

@@ -23,7 +23,7 @@ fileprivate func emitDiagnosticParts(
   position: AbsolutePosition,
   offset: Int,
   highlights: [Syntax] = [],
-  fixItChanges: [FixIt.Change] = []
+  edits: [SourceEdit] = []
 ) {
   // Map severity
   let bridgedSeverity = severity.bridged
@@ -52,40 +52,12 @@ fileprivate func emitDiagnosticParts(
   }
 
   // Emit changes for a Fix-It.
-  for change in fixItChanges {
-    let replaceStartLoc: BridgedSourceLoc
-    let replaceEndLoc: BridgedSourceLoc
-    var newText: String
-
-    switch change {
-    case .replace(let oldNode, let newNode):
-      replaceStartLoc = bridgedSourceLoc(at: oldNode.position)
-      replaceEndLoc = bridgedSourceLoc(at: oldNode.endPosition)
-      newText = newNode.description
-
-    case .replaceLeadingTrivia(let oldToken, let newTrivia):
-      replaceStartLoc = bridgedSourceLoc(at: oldToken.position)
-      replaceEndLoc = bridgedSourceLoc(
-        at: oldToken.positionAfterSkippingLeadingTrivia
-      )
-      newText = newTrivia.description
-
-    case .replaceTrailingTrivia(let oldToken, let newTrivia):
-      replaceStartLoc = bridgedSourceLoc(at: oldToken.endPositionBeforeTrailingTrivia)
-      replaceEndLoc = bridgedSourceLoc(at: oldToken.endPosition)
-      newText = newTrivia.description
-
-    case .replaceChild(let replacingChildData):
-      let replacementRange = replacingChildData.replacementRange
-      replaceStartLoc = bridgedSourceLoc(at: replacementRange.lowerBound)
-      replaceEndLoc = bridgedSourceLoc(at: replacementRange.upperBound)
-      newText = replacingChildData.newChild.description
-    }
-
+  for edit in edits {
+    var newText: String = edit.replacement
     newText.withBridgedString { bridgedMessage in
       diag.fixItReplace(
-        start: replaceStartLoc,
-        end: replaceEndLoc,
+        start: bridgedSourceLoc(at: edit.range.lowerBound),
+        end: bridgedSourceLoc(at: edit.range.upperBound),
         replacement: bridgedMessage
       )
     }
@@ -115,6 +87,7 @@ public func emitDiagnostic(
   )
 
   // Emit Fix-Its.
+  // FIXME: Ths assumes the fixIt is on the same tree/buffer, which is not guaranteed.
   for fixIt in diagnostic.fixIts {
     emitDiagnosticParts(
       diagnosticEngine: diagnosticEngine,
@@ -123,11 +96,12 @@ public func emitDiagnostic(
       severity: .note,
       position: diagnostic.position,
       offset: sourceFileBufferOffset,
-      fixItChanges: fixIt.changes
+      edits: fixIt.edits
     )
   }
 
   // Emit any notes as follow-ons.
+  // FIXME: Ths assumes the node is on the same tree/buffer, which is not guaranteed.
   for note in diagnostic.notes {
     emitDiagnosticParts(
       diagnosticEngine: diagnosticEngine,

lib/ASTGen/Sources/MacroEvaluation/SourceManager.swift

@@ -159,17 +159,31 @@ extension SourceManager {
 
       switch change {
       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
         replaceStartLoc = bridgedSourceLoc(
           for: oldNode,
-          at: oldNode.positionAfterSkippingLeadingTrivia
+          at: leadingMatch ? oldNode.positionAfterSkippingLeadingTrivia : oldNode.position
         )
         replaceEndLoc = bridgedSourceLoc(
           for: oldNode,
-          at: oldNode.endPositionBeforeTrailingTrivia
+          at: trailingMatch ? oldNode.endPositionBeforeTrailingTrivia : oldNode.endPosition
         )
+        var newNode = newNode.detached
+        if leadingMatch {
+          newNode.leadingTrivia = []
+        }
+        if trailingMatch {
+          newNode.trailingTrivia = []
+        }
         newText = newNode.description
 
       case .replaceLeadingTrivia(let oldToken, let newTrivia):
+        guard oldToken.leadingTrivia != newTrivia else {
+          continue
+        }
         replaceStartLoc = bridgedSourceLoc(for: oldToken)
         replaceEndLoc = bridgedSourceLoc(
           for: oldToken,
@@ -178,6 +192,9 @@ extension SourceManager {
         newText = newTrivia.description
 
       case .replaceTrailingTrivia(let oldToken, let newTrivia):
+        guard oldToken.trailingTrivia != newTrivia else {
+          continue
+        }
         replaceStartLoc = bridgedSourceLoc(
           for: oldToken,
           at: oldToken.endPositionBeforeTrailingTrivia

test/ASTGen/diagnostics.swift

@@ -21,7 +21,7 @@ func testEditorPlaceholder() -> Int {
 
 _ = [(Int) -> async throws Int]()
 // expected-error@-1{{'async throws' must precede '->'}}
-// expected-note@-2{{move 'async throws' in front of '->'}}{{15-21=}} {{21-28=}} {{20-21= }} {{12-12=async }} {{12-12=throws }}
+// expected-note@-2{{move 'async throws' in front of '->'}}{{15-21=}} {{21-28=}} {{12-12=async }} {{12-12=throws }}
 
 @freestanding // expected-error {{expected arguments for 'freestanding' attribute}}
 func dummy() {}

test/Macros/Inputs/syntax_macro_definitions.swift

@@ -161,12 +161,7 @@ public enum AddBlocker: ExpressionMacro {
                   changes: [
                     FixIt.Change.replace(
                       oldNode: Syntax(binOp.operator),
-                      newNode: Syntax(
-                        TokenSyntax(
-                          .binaryOperator("-"),
-                          presence: .present
-                        )
-                      )
+                      newNode: Syntax(binOp.operator.detached.with(\.tokenKind, .binaryOperator("-"))),
                     )
                   ]
                 ),