Merge pull request #1471 from ahoppen/enum-case-rename

Adjust test cases for fixed enum case rename

Author: ahoppen

Sources/SKTestSupport/SkipUnless.swift

@@ -156,6 +156,31 @@ public actor SkipUnless {
     }
   }
 
+  /// Checks whether the sourcekitd contains a fix to rename labels of enum cases correctly
+  /// (https://github.com/apple/swift/pull/74241).
+  public static func sourcekitdCanRenameEnumCaseLabels(
+    file: StaticString = #filePath,
+    line: UInt = #line
+  ) async throws {
+    return try await shared.skipUnlessSupportedByToolchain(swiftVersion: SwiftVersion(6, 0), file: file, line: line) {
+      let testClient = try await TestSourceKitLSPClient()
+      let uri = DocumentURI(for: .swift)
+      let positions = testClient.openDocument(
+        """
+        enum MyEnum {
+          case 1️⃣myCase(2️⃣String)
+        }
+        """,
+        uri: uri
+      )
+
+      let renameResult = try await testClient.send(
+        RenameRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"], newName: "myCase(label:)")
+      )
+      return renameResult?.changes == [uri: [TextEdit(range: Range(positions["2️⃣"]), newText: "label: ")]]
+    }
+  }
+
   /// Whether clangd has support for the `workspace/indexedRename` request.
   public static func clangdSupportsIndexBasedRename(
     file: StaticString = #filePath,

Sources/SourceKitLSP/Rename.swift

@@ -1003,24 +1003,6 @@ extension SwiftLanguageService {
     return nil
   }
 
-  /// Returns `true` if the given position is inside an `EnumCaseDeclSyntax`.
-  fileprivate func isInsideEnumCaseDecl(position: Position, snapshot: DocumentSnapshot) async -> Bool {
-    let syntaxTree = await syntaxTreeManager.syntaxTree(for: snapshot)
-    var node = Syntax(syntaxTree.token(at: snapshot.absolutePosition(of: position)))
-
-    while let parent = node?.parent {
-      if parent.is(EnumCaseDeclSyntax.self) {
-        return true
-      }
-      if parent.is(MemberBlockItemSyntax.self) || parent.is(CodeBlockItemSyntax.self) {
-        // `MemberBlockItemSyntax` and `CodeBlockItemSyntax` can't be nested inside an EnumCaseDeclSyntax. Early exit.
-        return false
-      }
-      node = parent
-    }
-    return false
-  }
-
   /// When the user requested a rename at `position` in `snapshot`, determine the position at which the rename should be
   /// performed internally, the USR of the symbol to rename and the range to rename that should be returned to the
   /// editor.
@@ -1095,17 +1077,8 @@ extension SwiftLanguageService {
 
     try Task.checkCancellation()
 
-    var requestedNewName = request.newName
-    if let openParenIndex = requestedNewName.firstIndex(of: "("),
-      await isInsideEnumCaseDecl(position: renamePosition, snapshot: snapshot)
-    {
-      // We don't support renaming enum parameter labels at the moment
-      // (https://github.com/apple/sourcekit-lsp/issues/1228)
-      requestedNewName = String(requestedNewName[..<openParenIndex])
-    }
-
     let oldName = CrossLanguageName(clangName: nil, swiftName: oldNameString, definitionLanguage: .swift)
-    let newName = CrossLanguageName(clangName: nil, swiftName: requestedNewName, definitionLanguage: .swift)
+    let newName = CrossLanguageName(clangName: nil, swiftName: request.newName, definitionLanguage: .swift)
     var edits = try await editsToRename(
       locations: renameLocations,
       in: snapshot,
@@ -1398,13 +1371,6 @@ extension SwiftLanguageService {
     if name.hasSuffix("()") {
       name = String(name.dropLast(2))
     }
-    if let openParenIndex = name.firstIndex(of: "("),
-      await isInsideEnumCaseDecl(position: renamePosition, snapshot: snapshot)
-    {
-      // We don't support renaming enum parameter labels at the moment
-      // (https://github.com/apple/sourcekit-lsp/issues/1228)
-      name = String(name[..<openParenIndex])
-    }
     guard let relatedIdentRange = response.relatedIdentifiers.first(where: { $0.range.contains(renamePosition) })?.range
     else {
       return nil

Tests/SourceKitLSPTests/RenameTests.swift

@@ -1170,14 +1170,15 @@ final class RenameTests: XCTestCase {
   }
 
   func testRenameEnumCaseWithUnlabeledAssociatedValue() async throws {
+    try await SkipUnless.sourcekitdCanRenameEnumCaseLabels()
     try await assertSingleFileRename(
       """
       enum MyEnum {
         case 1️⃣myCase(String)
       }
       """,
       newName: "newName",
-      expectedPrepareRenamePlaceholder: "myCase",
+      expectedPrepareRenamePlaceholder: "myCase(_:)",
       expected: """
         enum MyEnum {
           case newName(String)
@@ -1187,39 +1188,127 @@ final class RenameTests: XCTestCase {
   }
 
   func testAddLabelToEnumCase() async throws {
-    // We don't support renaming enum parameter labels at the moment
-    // (https://github.com/apple/sourcekit-lsp/issues/1228)
+    try await SkipUnless.sourcekitdCanRenameEnumCaseLabels()
     try await assertSingleFileRename(
       """
       enum MyEnum {
         case 1️⃣myCase(String)
       }
       """,
       newName: "newName(newLabel:)",
-      expectedPrepareRenamePlaceholder: "myCase",
+      expectedPrepareRenamePlaceholder: "myCase(_:)",
       expected: """
         enum MyEnum {
-          case newName(String)
+          case newName(newLabel: String)
         }
         """
     )
   }
 
-  func testRemoveLabelToEnumCase() async throws {
-    // We don't support renaming enum parameter labels at the moment
-    // (https://github.com/apple/sourcekit-lsp/issues/1228)
+  func testRemoveLabelFromEnumCase() async throws {
+    try await SkipUnless.sourcekitdCanRenameEnumCaseLabels()
     try await assertSingleFileRename(
       """
       enum MyEnum {
-        case 1️⃣myCase(label: String)
+        case myCase(label: String)
+      }
+
+      func test() {
+        _ = MyEnum.2️⃣myCase(label: "abc")
       }
       """,
       newName: "newName(_:)",
-      expectedPrepareRenamePlaceholder: "myCase",
+      expectedPrepareRenamePlaceholder: "myCase(label:)",
+      expected: """
+        enum MyEnum {
+          case newName(_ label: String)
+        }
+
+        func test() {
+          _ = MyEnum.newName("abc")
+        }
+        """
+    )
+  }
+
+  func testRenameEnumCaseWithUnderscoreLabel() async throws {
+    try await SkipUnless.sourcekitdCanRenameEnumCaseLabels()
+    try await assertSingleFileRename(
+      """
+      enum MyEnum {
+        case myCase(_ label: String)
+      }
+
+      func test() {
+        _ = MyEnum.2️⃣myCase("abc")
+      }
+      """,
+      newName: "newName(_:)",
+      expectedPrepareRenamePlaceholder: "myCase(_:)",
+      expected: """
+        enum MyEnum {
+          case newName(_ label: String)
+        }
+
+        func test() {
+          _ = MyEnum.newName("abc")
+        }
+        """
+    )
+  }
+
+  func testRenameEnumCaseWithUnderscoreToLabelMatchingInternalName() async throws {
+    try await SkipUnless.sourcekitdCanRenameEnumCaseLabels()
+    try await assertSingleFileRename(
+      """
+      enum MyEnum {
+        case myCase(_ label: String)
+      }
+
+      func test() {
+        _ = MyEnum.2️⃣myCase("abc")
+      }
+      """,
+      newName: "newName(label:)",
+      expectedPrepareRenamePlaceholder: "myCase(_:)",
       expected: """
         enum MyEnum {
           case newName(label: String)
         }
+
+        func test() {
+          _ = MyEnum.newName(label: "abc")
+        }
+        """
+    )
+  }
+
+  func testRenameEnumCaseWithUnderscoreToLabelNotMatchingInternalName() async throws {
+    try await SkipUnless.sourcekitdCanRenameEnumCaseLabels()
+    // Note that the renamed code doesn't compile because enum cases can't have an external and internal parameter name.
+    // But it's probably the best thing we can do because we don't want to erase the user-specified internal name, which
+    // they didn't request to rename. Also, this is a pretty niche case and having a special case for it is probably not
+    // worth it.
+    try await assertSingleFileRename(
+      """
+      enum MyEnum {
+        case myCase(_ label: String)
+      }
+
+      func test() {
+        _ = MyEnum.2️⃣myCase("abc")
+      }
+      """,
+      newName: "newName(publicLabel:)",
+      expectedPrepareRenamePlaceholder: "myCase(_:)",
+      expected: """
+        enum MyEnum {
+          case newName(publicLabel label: String)
+        }
+
+        func test() {
+          _ = MyEnum.newName(publicLabel: "abc")
+        }
         """
     )
   }