From 2e76be7c962e5a14755afebd145c0e530690c49d Mon Sep 17 00:00:00 2001
From: Alex Hoppen <ahoppen@apple.com>
Date: Thu, 28 Apr 2022 10:59:33 +0200
Subject: [PATCH] Remove placeholders from code actions instead of replacinng
 them by snippets

#481 replaced SourceKit placeholders (like `<#code#>`) by LSP snippets for code actions, which are used to represent SourceKit refactoring and diagnostic Fix-Its. But LSP snippets are only supported for code completion, not for code actions. This results in LSP snippets like ${1:code} being inserted into the editor.

Instead of replacing SourceKit placeholders by LSP placeholders, we should just remove them altogether from the code action response.

rdar://92447079 [#488]
---
 Sources/LSPTestSupport/CheckCoding.swift      |  7 -----
 .../String+TrimTrailingWhitespace.swift       | 19 ++++++++++++
 Sources/SourceKitLSP/Swift/Diagnostic.swift   | 29 +++++++++----------
 .../Swift/SemanticRefactoring.swift           |  9 +++---
 .../Swift/SwiftLanguageServer.swift           |  4 +--
 Tests/SourceKitLSPTests/CodeActionTests.swift | 10 +++----
 6 files changed, 44 insertions(+), 34 deletions(-)
 create mode 100644 Sources/LSPTestSupport/String+TrimTrailingWhitespace.swift

diff --git a/Sources/LSPTestSupport/CheckCoding.swift b/Sources/LSPTestSupport/CheckCoding.swift
index 173f2fa8..e9d7dc3a 100644
--- a/Sources/LSPTestSupport/CheckCoding.swift
+++ b/Sources/LSPTestSupport/CheckCoding.swift
@@ -78,10 +78,3 @@ public func checkCoding<T>(_ value: T, json: String, userInfo: [CodingUserInfoKe
 
   body(decodedValue)
 }
-
-extension String {
-  // This is fileprivate because the implementation is really slow; to use it outside a test it should be optimized.
-  fileprivate func trimmingTrailingWhitespace() -> String {
-    return self.replacingOccurrences(of: "[ ]+\\n", with: "\n", options: .regularExpression)
-  }
-}
diff --git a/Sources/LSPTestSupport/String+TrimTrailingWhitespace.swift b/Sources/LSPTestSupport/String+TrimTrailingWhitespace.swift
new file mode 100644
index 00000000..6a162835
--- /dev/null
+++ b/Sources/LSPTestSupport/String+TrimTrailingWhitespace.swift
@@ -0,0 +1,19 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2022 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
+//
+//===----------------------------------------------------------------------===//
+
+
+public extension String {
+  // This implementation is really slow; to use it outside a test it should be optimized.
+  func trimmingTrailingWhitespace() -> String {
+    return self.replacingOccurrences(of: "[ ]+\\n", with: "\n", options: .regularExpression)
+  }
+}
diff --git a/Sources/SourceKitLSP/Swift/Diagnostic.swift b/Sources/SourceKitLSP/Swift/Diagnostic.swift
index a095bdf4..99b8f8d5 100644
--- a/Sources/SourceKitLSP/Swift/Diagnostic.swift
+++ b/Sources/SourceKitLSP/Swift/Diagnostic.swift
@@ -20,10 +20,10 @@ extension CodeAction {
   /// Creates a CodeAction from a list for sourcekit fixits.
   ///
   /// If this is from a note, the note's description should be passed as `fromNote`.
-  init?(fixits: SKDResponseArray, in snapshot: DocumentSnapshot, fromNote: String?, clientSupportsSnippets: Bool) {
+  init?(fixits: SKDResponseArray, in snapshot: DocumentSnapshot, fromNote: String?) {
     var edits: [TextEdit] = []
     let editsMapped = fixits.forEach { (_, skfixit) -> Bool in
-      if let edit = TextEdit(fixit: skfixit, in: snapshot, clientSupportsSnippets: clientSupportsSnippets) {
+      if let edit = TextEdit(fixit: skfixit, in: snapshot) {
         edits.append(edit)
         return true
       }
@@ -88,7 +88,7 @@ extension CodeAction {
 extension TextEdit {
 
   /// Creates a TextEdit from a sourcekitd fixit response dictionary.
-  init?(fixit: SKDResponseDictionary, in snapshot: DocumentSnapshot, clientSupportsSnippets: Bool) {
+  init?(fixit: SKDResponseDictionary, in snapshot: DocumentSnapshot) {
     let keys = fixit.sourcekitd.keys
     if let utf8Offset: Int = fixit[keys.offset],
        let length: Int = fixit[keys.length],
@@ -97,8 +97,10 @@ extension TextEdit {
        let endPosition = snapshot.positionOf(utf8Offset: utf8Offset + length),
        length > 0 || !replacement.isEmpty
     {
-      let replacementWithSnippets = rewriteSourceKitPlaceholders(inString: replacement, clientSupportsSnippets: clientSupportsSnippets)
-      self.init(range: position..<endPosition, newText: replacementWithSnippets)
+      // Snippets are only suppored in code completion.
+      // Remove SourceKit placeholders from Fix-Its because they can't be represented in the editor properly.
+      let replacementWithoutPlaceholders = rewriteSourceKitPlaceholders(inString: replacement, clientSupportsSnippets: false)
+      self.init(range: position..<endPosition, newText: replacementWithoutPlaceholders)
     } else {
       return nil
     }
@@ -110,8 +112,7 @@ extension Diagnostic {
   /// Creates a diagnostic from a sourcekitd response dictionary.
   init?(_ diag: SKDResponseDictionary,
         in snapshot: DocumentSnapshot,
-        useEducationalNoteAsCode: Bool,
-        clientSupportsSnippets: Bool) {
+        useEducationalNoteAsCode: Bool) {
     // FIXME: this assumes that the diagnostics are all in the same file.
 
     let keys = diag.sourcekitd.keys
@@ -179,7 +180,7 @@ extension Diagnostic {
 
     var actions: [CodeAction]? = nil
     if let skfixits: SKDResponseArray = diag[keys.fixits],
-       let action = CodeAction(fixits: skfixits, in: snapshot, fromNote: nil, clientSupportsSnippets: clientSupportsSnippets) {
+       let action = CodeAction(fixits: skfixits, in: snapshot, fromNote: nil) {
       actions = [action]
     }
 
@@ -187,7 +188,7 @@ extension Diagnostic {
     if let sknotes: SKDResponseArray = diag[keys.diagnostics] {
       notes = []
       sknotes.forEach { (_, sknote) -> Bool in
-        guard let note = DiagnosticRelatedInformation(sknote, in: snapshot, clientSupportsSnippets: clientSupportsSnippets) else { return true }
+        guard let note = DiagnosticRelatedInformation(sknote, in: snapshot) else { return true }
         notes?.append(note)
         return true
       }
@@ -224,7 +225,7 @@ extension Diagnostic {
 extension DiagnosticRelatedInformation {
 
   /// Creates related information from a sourcekitd note response dictionary.
-  init?(_ diag: SKDResponseDictionary, in snapshot: DocumentSnapshot, clientSupportsSnippets: Bool) {
+  init?(_ diag: SKDResponseDictionary, in snapshot: DocumentSnapshot) {
     let keys = diag.sourcekitd.keys
 
     var position: Position? = nil
@@ -245,7 +246,7 @@ extension DiagnosticRelatedInformation {
 
     var actions: [CodeAction]? = nil
     if let skfixits: SKDResponseArray = diag[keys.fixits],
-       let action = CodeAction(fixits: skfixits, in: snapshot, fromNote: message, clientSupportsSnippets: clientSupportsSnippets) {
+       let action = CodeAction(fixits: skfixits, in: snapshot, fromNote: message) {
       actions = [action]
     }
 
@@ -279,13 +280,11 @@ struct CachedDiagnostic {
 extension CachedDiagnostic {
   init?(_ diag: SKDResponseDictionary,
         in snapshot: DocumentSnapshot,
-        useEducationalNoteAsCode: Bool,
-        clientSupportsSnippets: Bool) {
+        useEducationalNoteAsCode: Bool) {
     let sk = diag.sourcekitd
     guard let diagnostic = Diagnostic(diag,
                                       in: snapshot,
-                                      useEducationalNoteAsCode: useEducationalNoteAsCode,
-                                      clientSupportsSnippets: clientSupportsSnippets) else {
+                                      useEducationalNoteAsCode: useEducationalNoteAsCode) else {
       return nil
     }
     self.diagnostic = diagnostic
diff --git a/Sources/SourceKitLSP/Swift/SemanticRefactoring.swift b/Sources/SourceKitLSP/Swift/SemanticRefactoring.swift
index 02e10dfe..c3c1fcad 100644
--- a/Sources/SourceKitLSP/Swift/SemanticRefactoring.swift
+++ b/Sources/SourceKitLSP/Swift/SemanticRefactoring.swift
@@ -36,7 +36,7 @@ struct SemanticRefactoring {
   ///   - dict: Response dictionary to extract information from.
   ///   - url: The client URL that triggered the `semantic_refactoring` request.
   ///   - keys: The sourcekitd key set to use for looking up into `dict`.
-  init?(_ title: String, _ dict: SKDResponseDictionary, _ snapshot: DocumentSnapshot, _ keys: sourcekitd_keys, clientSupportsSnippets: Bool) {
+  init?(_ title: String, _ dict: SKDResponseDictionary, _ snapshot: DocumentSnapshot, _ keys: sourcekitd_keys) {
     guard let categorizedEdits: SKDResponseArray = dict[keys.categorizededits] else {
       return nil
     }
@@ -59,7 +59,9 @@ struct SemanticRefactoring {
                                                  utf8Column: endColumn - 1),
            let text: String = value[keys.text]
         {
-          let textWithSnippets = rewriteSourceKitPlaceholders(inString: text, clientSupportsSnippets: clientSupportsSnippets)
+          // Snippets are only suppored in code completion.
+          // Remove SourceKit placeholders in refactoring actions because they can't be represented in the editor properly.
+          let textWithSnippets = rewriteSourceKitPlaceholders(inString: text, clientSupportsSnippets: false)
           let edit = TextEdit(range: startPosition..<endPosition, newText: textWithSnippets)
           textEdits.append(edit)
         }
@@ -165,8 +167,7 @@ extension SwiftLanguageServer {
         guard let dict = result.success else {
           return completion(.failure(.responseError(ResponseError(result.failure!))))
         }
-        let supportsSnippets = (self.clientCapabilities.textDocument?.completion?.completionItem?.snippetSupport == true)
-        guard let refactor = SemanticRefactoring(refactorCommand.title, dict, snapshot, self.keys, clientSupportsSnippets: supportsSnippets) else {
+        guard let refactor = SemanticRefactoring(refactorCommand.title, dict, snapshot, self.keys) else {
           return completion(.failure(.noEditsNeeded(uri)))
         }
         completion(.success(refactor))
diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
index ab77c7d2..32a3a7bf 100644
--- a/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
+++ b/Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift
@@ -293,15 +293,13 @@ public final class SwiftLanguageServer: ToolchainLanguageServer {
 
     let supportsCodeDescription =
            (clientCapabilities.textDocument?.publishDiagnostics?.codeDescriptionSupport == true)
-    let supportsSnippets = (self.clientCapabilities.textDocument?.completion?.completionItem?.snippetSupport == true)
 
     // Note: we make the notification even if there are no diagnostics to clear the current state.
     var newDiags: [CachedDiagnostic] = []
     response[keys.diagnostics]?.forEach { _, diag in
       if let diag = CachedDiagnostic(diag,
                                      in: snapshot,
-                                     useEducationalNoteAsCode: supportsCodeDescription,
-                                     clientSupportsSnippets: supportsSnippets) {
+                                     useEducationalNoteAsCode: supportsCodeDescription) {
         newDiags.append(diag)
       }
       return true
diff --git a/Tests/SourceKitLSPTests/CodeActionTests.swift b/Tests/SourceKitLSPTests/CodeActionTests.swift
index 90659d43..5bba65c9 100644
--- a/Tests/SourceKitLSPTests/CodeActionTests.swift
+++ b/Tests/SourceKitLSPTests/CodeActionTests.swift
@@ -248,7 +248,7 @@ final class CodeActionTests: XCTestCase {
     XCTAssertEqual(result, .codeActions([expectedCodeAction]))
   }
 
-  func testCodeActionsUseLSPSnippets() throws {
+  func testCodeActionsRemovePlaceholders() throws {
     let capabilities = clientCapabilitiesWithCodeActionSupport()
     let ws = try staticSourceKitTibsWorkspace(name: "Fixit", clientCapabilities: capabilities)!
 
@@ -293,10 +293,10 @@ final class CodeActionTests: XCTestCase {
     guard let change = quickFixAction.edit?.changes?[def.docUri]?.spm_only else {
       return XCTFail("Expected exactly one change")
     }
-    XCTAssertEqual(change.newText, """
+    XCTAssertEqual(change.newText.trimmingTrailingWhitespace(), """
 
         func foo() {
-            ${1:code}
+
         }
 
     """)
@@ -318,10 +318,10 @@ final class CodeActionTests: XCTestCase {
       guard let change = request.params.edit.changes?[def.docUri]?.spm_only else {
         return XCTFail("Expected exactly one edit")
       }
-      XCTAssertEqual(change.newText, """
+      XCTAssertEqual(change.newText.trimmingTrailingWhitespace(), """
 
           func foo() {
-              ${1:code}
+
           }
 
       """)