Skip to content

Adjust test cases for fixed enum case rename #1471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Sources/SKTestSupport/SkipUnless.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
36 changes: 1 addition & 35 deletions Sources/SourceKitLSP/Rename.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
109 changes: 99 additions & 10 deletions Tests/SourceKitLSPTests/RenameTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Comment on lines 1220 to +1224
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be dropping the label completely here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s valuable to keep it as an internal label, similar to what we do for function completion (and yes, also easier to implement 🙈)

}

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")
}
"""
)
}
Expand Down