Don’t include internal parameter names in LSP snippets

When translating editor placeholders to snippets, we should not include internal parameter names into the snippet. The problem is that once you leave snippet editing mode (essentially by moving the cursor to any place that’s not the snippet, eg. by typing a string literal for one of the parameters or moving the cursor), all snippets become verbatim text. And then it’s impossible to tell whether `paramName: ` was an annotation inside the snippet describing the internal parameter’s name or the external parameter label.

rdar://123772474

Author: ahoppen

Sources/SourceKitLSP/CMakeLists.txt

@@ -27,10 +27,10 @@ target_sources(SourceKitLSP PRIVATE
   Swift/DiagnosticReportManager.swift
   Swift/DocumentFormatting.swift
   Swift/DocumentSymbols.swift
-  Swift/EditorPlaceholder.swift
   Swift/FoldingRange.swift
   Swift/OpenInterface.swift
   Swift/RelatedIdentifiers.swift
+  Swift/RewriteSourceKitPlaceholders.swift
   Swift/SemanticRefactorCommand.swift
   Swift/SemanticRefactoring.swift
   Swift/SemanticTokens.swift

Sources/SourceKitLSP/Swift/CodeCompletionSession.swift

@@ -357,7 +357,7 @@ class CodeCompletionSession {
       }
 
       let text = insertText.map {
-        rewriteSourceKitPlaceholders(inString: $0, clientSupportsSnippets: clientSupportsSnippets)
+        rewriteSourceKitPlaceholders(in: $0, clientSupportsSnippets: clientSupportsSnippets)
       }
       let isInsertTextSnippet = clientSupportsSnippets && text != insertText
 

Sources/SourceKitLSP/Swift/Diagnostic.swift

@@ -129,10 +129,7 @@ extension TextEdit {
     {
       // 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
-      )
+      let replacementWithoutPlaceholders = rewriteSourceKitPlaceholders(in: replacement, clientSupportsSnippets: false)
 
       // If both the replacement without placeholders and the fixit are empty, no TextEdit should be created.
       if (replacementWithoutPlaceholders.isEmpty && length == 0) {

Sources/SourceKitLSP/Swift/EditorPlaceholder.swift

@@ -0,0 +1,43 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2019 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 LSPLogging
+@_spi(SourceKitLSP) import SwiftRefactor
+
+func rewriteSourceKitPlaceholders(in string: String, clientSupportsSnippets: Bool) -> String {
+  var result = string
+  var index = 1
+  while let start = result.range(of: "<#") {
+    guard let end = result[start.upperBound...].range(of: "#>") else {
+      logger.error("invalid placeholder in \(string)")
+      return string
+    }
+    let rawPlaceholder = String(result[start.lowerBound..<end.upperBound])
+    guard let displayName = EditorPlaceholderData(text: rawPlaceholder)?.nameForSnippet else {
+      logger.error("failed to decode placeholder \(rawPlaceholder) in \(string)")
+      return string
+    }
+    let snippet = clientSupportsSnippets ? "${\(index):\(displayName)}" : ""
+    result.replaceSubrange(start.lowerBound..<end.upperBound, with: snippet)
+    index += 1
+  }
+  return result
+}
+
+fileprivate extension EditorPlaceholderData {
+  var nameForSnippet: Substring {
+    switch self {
+    case .basic(text: let text): return text
+    case .typed(text: let text, type: _): return text
+    }
+  }
+}

Sources/SourceKitLSP/Swift/RewriteSourceKitPlaceholders.swift

@@ -64,7 +64,7 @@ struct SemanticRefactoring {
         {
           // 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 textWithSnippets = rewriteSourceKitPlaceholders(in: text, clientSupportsSnippets: false)
           let edit = TextEdit(range: startPosition..<endPosition, newText: textWithSnippets)
           textEdits.append(edit)
         }

Sources/SourceKitLSP/Swift/SemanticRefactoring.swift

@@ -1364,57 +1364,6 @@ final class LocalSwiftTests: XCTestCase {
     }
   }
 
-  func testEditorPlaceholderParsing() {
-    var text =
-      "<#basic placeholder"  // Need to end this in another line so Xcode doesn't treat it as a real placeholder
-      + "#>"
-    var data = EditorPlaceholder(text)
-    XCTAssertNotNil(data)
-    if let data = data {
-      XCTAssertEqual(data, .basic("basic placeholder"))
-      XCTAssertEqual(data.displayName, "basic placeholder")
-    }
-    text = "<#T##x: Int##Int" + "#>"
-    data = EditorPlaceholder(text)
-    XCTAssertNotNil(data)
-    if let data = data {
-      XCTAssertEqual(data, .typed(displayName: "x: Int", type: "Int", typeForExpansion: "Int"))
-      XCTAssertEqual(data.displayName, "x: Int")
-    }
-    text = "<#T##x: Int##Blah##()->Int" + "#>"
-    data = EditorPlaceholder(text)
-    XCTAssertNotNil(data)
-    if let data = data {
-      XCTAssertEqual(data, .typed(displayName: "x: Int", type: "Blah", typeForExpansion: "()->Int"))
-      XCTAssertEqual(data.displayName, "x: Int")
-    }
-    text = "<#T##Int" + "#>"
-    data = EditorPlaceholder(text)
-    XCTAssertNotNil(data)
-    if let data = data {
-      XCTAssertEqual(data, .typed(displayName: "Int", type: "Int", typeForExpansion: "Int"))
-      XCTAssertEqual(data.displayName, "Int")
-    }
-    text = "<#foo"
-    data = EditorPlaceholder(text)
-    XCTAssertNil(data)
-    text = " <#foo"
-    data = EditorPlaceholder(text)
-    XCTAssertNil(data)
-    text = "foo"
-    data = EditorPlaceholder(text)
-    XCTAssertNil(data)
-    text = "foo#>"
-    data = EditorPlaceholder(text)
-    XCTAssertNil(data)
-    text = "<#foo#"
-    data = EditorPlaceholder(text)
-    XCTAssertNil(data)
-    text = " <#foo" + "#>"
-    data = EditorPlaceholder(text)
-    XCTAssertNil(data)
-  }
-
   func testIncrementalParse() async throws {
     let testClient = try await TestSourceKitLSPClient()
     let url = URL(fileURLWithPath: "/\(UUID())/a.swift")

Tests/SourceKitLSPTests/LocalSwiftTests.swift

@@ -172,11 +172,11 @@ final class SwiftCompletionTests: XCTestCase {
         .textEdit(
           TextEdit(
             range: Position(line: 9, utf16index: 9)..<Position(line: 9, utf16index: 9),
-            newText: "test(${1:b: Int})"
+            newText: "test(${1:Int})"
           )
         )
       )
-      XCTAssertEqual(test.insertText, "test(${1:b: Int})")
+      XCTAssertEqual(test.insertText, "test(${1:Int})")
       XCTAssertEqual(test.insertTextFormat, .snippet)
     }
   }