From 34a36b44e61e8c4bf1c95a8b87bd6945a3a9da3e Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 8 Dec 2023 11:02:31 -0800 Subject: [PATCH 1/6] Refactor rename to support index-based discovery of rename locations --- Sources/SourceKitD/SKDRequestArray.swift | 6 ++ Sources/SourceKitLSP/Swift/Rename.swift | 91 +++++++++++++++-------- Tests/SourceKitLSPTests/RenameTests.swift | 54 +++++++------- 3 files changed, 92 insertions(+), 59 deletions(-) diff --git a/Sources/SourceKitD/SKDRequestArray.swift b/Sources/SourceKitD/SKDRequestArray.swift index 61d3a7e8b..4c66d80a1 100644 --- a/Sources/SourceKitD/SKDRequestArray.swift +++ b/Sources/SourceKitD/SKDRequestArray.swift @@ -40,6 +40,12 @@ public final class SKDRequestArray { public func append(_ value: SKDRequestDictionary) { sourcekitd.api.request_array_set_value(array, -1, value.dict) } + + public static func += (array: SKDRequestArray, other: some Sequence) { + for item in other { + array.append(item) + } + } } extension SKDRequestArray: CustomStringConvertible { diff --git a/Sources/SourceKitLSP/Swift/Rename.swift b/Sources/SourceKitLSP/Swift/Rename.swift index 20e64ef7e..a65f22604 100644 --- a/Sources/SourceKitLSP/Swift/Rename.swift +++ b/Sources/SourceKitLSP/Swift/Rename.swift @@ -268,33 +268,47 @@ fileprivate struct SyntacticRenameName { } } +struct RenameLocation { + /// The line of the identifier to be renamed (1-based). + let line: Int + /// The column of the identifier to be renamed in UTF-8 bytes (1-based). + let utf8Column: Int + let usage: RelatedIdentifier.Usage +} + +private extension DocumentSnapshot { + init(_ url: URL, language: Language) throws { + let contents = try String(contentsOf: url) + self.init(uri: DocumentURI(url), language: language, version: 0, lineTable: LineTable(contents)) + } +} + extension SwiftLanguageServer { - /// From a list of rename locations, provided by a related identifiers request, compute the list of - /// `SyntacticRenameName`s that define which ranges need to be edited to rename a compound decl name. + /// From a list of rename locations compute the list of `SyntacticRenameName`s that define which ranges need to be + /// edited to rename a compound decl name. + /// + /// - Parameters: + /// - renameLocations: The locations to rename + /// - oldName: The compound decl name that the declaration had before the rename. Used to verify that the rename + /// locations match that name. Eg. `myFunc(argLabel:otherLabel:)` or `myVar` + /// - snapshot: If the document has been modified from the on-disk version, the current snapshot. `nil` to read the + /// file contents from disk. private func getSyntacticRenameRanges( - relatedIdentifiers: RelatedIdentifiersResponse, + renameLocations: [RenameLocation], + oldName: String, in snapshot: DocumentSnapshot ) async throws -> [SyntacticRenameName] { let locations = SKDRequestArray(sourcekitd: sourcekitd) - for relatedIdentifier in relatedIdentifiers.relatedIdentifiers { - let position = relatedIdentifier.range.lowerBound - guard let utf8Column = snapshot.lineTable.utf8ColumnAt(line: position.line, utf16Column: position.utf16index) - else { - logger.fault("Unable to find UTF-8 column for \(position.line):\(position.utf16index)") - continue - } - let renameLocation = SKDRequestDictionary(sourcekitd: sourcekitd) - renameLocation[keys.line] = position.line + 1 - renameLocation[keys.column] = utf8Column + 1 - renameLocation[keys.nameType] = relatedIdentifier.usage.uid(keys: keys) - locations.append(renameLocation) - } - guard let name = relatedIdentifiers.name else { - throw ResponseError.unknown("Running sourcekit-lsp with a version of sourcekitd that does not support rename") + locations += renameLocations.map { renameLocation in + let skRenameLocation = SKDRequestDictionary(sourcekitd: sourcekitd) + skRenameLocation[keys.line] = renameLocation.line + skRenameLocation[keys.column] = renameLocation.utf8Column + skRenameLocation[keys.nameType] = renameLocation.usage.uid(keys: keys) + return skRenameLocation } let renameLocation = SKDRequestDictionary(sourcekitd: sourcekitd) renameLocation[keys.locations] = locations - renameLocation[keys.name] = name + renameLocation[keys.name] = oldName let renameLocations = SKDRequestArray(sourcekitd: sourcekitd) renameLocations.append(renameLocation) @@ -323,22 +337,38 @@ extension SwiftLanguageServer { in: snapshot, includeNonEditableBaseNames: true ) + guard let oldName = relatedIdentifiers.name else { + throw ResponseError.unknown("Running sourcekit-lsp with a version of sourcekitd that does not support rename") + } + + try Task.checkCancellation() + let renameLocations = relatedIdentifiers.relatedIdentifiers.compactMap { (relatedIdentifier) -> RenameLocation? in + let position = relatedIdentifier.range.lowerBound + guard let utf8Column = snapshot.lineTable.utf8ColumnAt(line: position.line, utf16Column: position.utf16index) + else { + logger.fault("Unable to find UTF-8 column for \(position.line):\(position.utf16index)") + return nil + } + return RenameLocation(line: position.line + 1, utf8Column: utf8Column + 1, usage: relatedIdentifier.usage) + } + try Task.checkCancellation() - let compoundRenameRanges = try await getSyntacticRenameRanges(relatedIdentifiers: relatedIdentifiers, in: snapshot) + let edits = try await renameRanges(from: renameLocations, in: snapshot, oldName: oldName, newName: try CompoundDeclName(request.newName)) - try Task.checkCancellation() + return WorkspaceEdit(changes: [ + snapshot.uri: edits + ]) + } - let oldName = - if let name = relatedIdentifiers.name { - try CompoundDeclName(name) - } else { - throw ResponseError.unknown("Running sourcekit-lsp with a version of sourcekitd that does not support rename") - } - let newName = try CompoundDeclName(request.newName) + private func renameRanges(from renameLocations: [RenameLocation], in snapshot: DocumentSnapshot, oldName oldNameString: String, newName: CompoundDeclName) async throws -> [TextEdit] { + let compoundRenameRanges = try await getSyntacticRenameRanges(renameLocations: renameLocations, oldName: oldNameString, in: snapshot) + let oldName = try CompoundDeclName(oldNameString) + + try Task.checkCancellation() - let edits = compoundRenameRanges.flatMap { (compoundRenameRange) -> [TextEdit] in + return compoundRenameRanges.flatMap { (compoundRenameRange) -> [TextEdit] in switch compoundRenameRange.category { case .unmatched, .mismatch: // The location didn't match. Don't rename it @@ -428,9 +458,6 @@ extension SwiftLanguageServer { } } } - return WorkspaceEdit(changes: [ - snapshot.uri: edits - ]) } } diff --git a/Tests/SourceKitLSPTests/RenameTests.swift b/Tests/SourceKitLSPTests/RenameTests.swift index b146e9ebc..bd55b93d1 100644 --- a/Tests/SourceKitLSPTests/RenameTests.swift +++ b/Tests/SourceKitLSPTests/RenameTests.swift @@ -32,7 +32,7 @@ private func apply(edits: [TextEdit], to source: String) -> String { return lineTable.content } -private func assertRename( +private func assertSingleFileRename( _ markedSource: String, newName: String, expected: String, @@ -57,7 +57,7 @@ private func assertRename( final class RenameTests: XCTestCase { func testRenameVariableBaseName() async throws { - try await assertRename( + try await assertSingleFileRename( """ let 1️⃣foo = 1 print(foo) @@ -71,7 +71,7 @@ final class RenameTests: XCTestCase { } func testRenameFunctionBaseName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo() {} foo() @@ -85,7 +85,7 @@ final class RenameTests: XCTestCase { } func testRenameFunctionParameter() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(x: Int) {} foo(x: 1) @@ -99,7 +99,7 @@ final class RenameTests: XCTestCase { } func testSecondParameterNameIfMatches() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(x y: Int) {} foo(x: 1) @@ -113,7 +113,7 @@ final class RenameTests: XCTestCase { } func testIntroduceLabel() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(_ y: Int) {} foo(1) @@ -127,7 +127,7 @@ final class RenameTests: XCTestCase { } func testRemoveLabel() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(x: Int) {} foo(x: 1) @@ -141,7 +141,7 @@ final class RenameTests: XCTestCase { } func testRemoveLabelWithExistingInternalName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(x a: Int) {} foo(x: 1) @@ -155,7 +155,7 @@ final class RenameTests: XCTestCase { } func testRenameSubscript() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo { 1️⃣subscript(x x: Int) -> Int { x } @@ -173,7 +173,7 @@ final class RenameTests: XCTestCase { } func testRemoveExternalLabelFromSubscript() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo { 1️⃣subscript(x x: Int) -> Int { x } @@ -191,7 +191,7 @@ final class RenameTests: XCTestCase { } func testIntroduceExternalLabelFromSubscript() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo { 1️⃣subscript(x: Int) -> Int { x } @@ -209,7 +209,7 @@ final class RenameTests: XCTestCase { } func testIgnoreRenameSubscriptBaseName() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo { 1️⃣subscript(x: Int) -> Int { x } @@ -227,7 +227,7 @@ final class RenameTests: XCTestCase { } func testRenameInitializerLabels() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo { 1️⃣init(x: Int) {} @@ -245,7 +245,7 @@ final class RenameTests: XCTestCase { } func testIgnoreRenameOfInitBaseName() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo { 1️⃣init(x: Int) {} @@ -263,7 +263,7 @@ final class RenameTests: XCTestCase { } func testRenameCompoundFunctionName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} _ = foo(a:) @@ -277,7 +277,7 @@ final class RenameTests: XCTestCase { } func testRemoveLabelFromCompoundFunctionName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} _ = foo(a:) @@ -291,7 +291,7 @@ final class RenameTests: XCTestCase { } func testIntroduceLabelToCompoundFunctionName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(_ a: Int) {} _ = foo(_:) @@ -305,7 +305,7 @@ final class RenameTests: XCTestCase { } func testRenameFromReference() async throws { - try await assertRename( + try await assertSingleFileRename( """ func foo(_ a: Int) {} _ = 1️⃣foo(_:) @@ -319,7 +319,7 @@ final class RenameTests: XCTestCase { } func testRenameMultipleParameters() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int, b: Int) {} foo(a: 1, b: 1) @@ -333,7 +333,7 @@ final class RenameTests: XCTestCase { } func testDontRenameParametersOmittedFromNewName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int, b: Int) {} foo(a: 1, b: 1) @@ -347,7 +347,7 @@ final class RenameTests: XCTestCase { } func testIgnoreAdditionalParametersInNewName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} foo(a: 1) @@ -361,7 +361,7 @@ final class RenameTests: XCTestCase { } func testOnlySpecifyBaseNameWhenRenamingFunction() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} foo(a: 1) @@ -375,7 +375,7 @@ final class RenameTests: XCTestCase { } func testIgnoreParametersInNewNameWhenRenamingVariable() async throws { - try await assertRename( + try await assertSingleFileRename( """ let 1️⃣foo = 1 _ = foo @@ -421,7 +421,7 @@ final class RenameTests: XCTestCase { } func testSpacesInNewParameterNames() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} foo(a: 1) @@ -435,7 +435,7 @@ final class RenameTests: XCTestCase { } func testRenameOperator() async throws { - try await assertRename( + try await assertSingleFileRename( """ struct Foo {} func 1️⃣+(x: Foo, y: Foo) {} @@ -451,7 +451,7 @@ final class RenameTests: XCTestCase { } func testRenameParameterToEmptyName() async throws { - try await assertRename( + try await assertSingleFileRename( """ func 1️⃣foo(x: Int) {} foo(x: 1) @@ -468,7 +468,7 @@ final class RenameTests: XCTestCase { #if !canImport(Darwin) throw XCTSkip("#selector in test case doesn't compile without Objective-C runtime.") #endif - try await assertRename( + try await assertSingleFileRename( """ import Foundation class Foo: NSObject { From 997ef38916af0867621a16c52ae7abc50b303f2d Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 9 Dec 2023 08:36:39 -0800 Subject: [PATCH 2/6] Support rename across Swift files rdar://118995700 --- Sources/SKSupport/CMakeLists.txt | 1 + Sources/SKSupport/Collection+Only.swift | 22 +++ Sources/SourceKitLSP/CMakeLists.txt | 2 +- .../Clang/ClangLanguageServer.swift | 4 - Sources/SourceKitLSP/{Swift => }/Rename.swift | 184 +++++++++++++++--- Sources/SourceKitLSP/SourceKitServer.swift | 11 -- .../Swift/RelatedIdentifiers.swift | 27 +-- .../Swift/SwiftLanguageServer.swift | 2 +- .../ToolchainLanguageServer.swift | 58 +++++- Tests/SourceKitLSPTests/RenameTests.swift | 178 +++++++++++++++++ 10 files changed, 418 insertions(+), 71 deletions(-) create mode 100644 Sources/SKSupport/Collection+Only.swift rename Sources/SourceKitLSP/{Swift => }/Rename.swift (75%) diff --git a/Sources/SKSupport/CMakeLists.txt b/Sources/SKSupport/CMakeLists.txt index 7314e200e..e44aa2a5c 100644 --- a/Sources/SKSupport/CMakeLists.txt +++ b/Sources/SKSupport/CMakeLists.txt @@ -4,6 +4,7 @@ add_library(SKSupport STATIC AsyncUtils.swift BuildConfiguration.swift ByteString.swift + Collection+Only.swift Connection+Send.swift dlopen.swift FileSystem.swift diff --git a/Sources/SKSupport/Collection+Only.swift b/Sources/SKSupport/Collection+Only.swift new file mode 100644 index 000000000..7a0e1a593 --- /dev/null +++ b/Sources/SKSupport/Collection+Only.swift @@ -0,0 +1,22 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2023 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 Collection { + /// If the collection contains a single element, return it, otherwise `nil`. + var only: Element? { + if !isEmpty && index(after: startIndex) == endIndex { + return self.first! + } else { + return nil + } + } +} diff --git a/Sources/SourceKitLSP/CMakeLists.txt b/Sources/SourceKitLSP/CMakeLists.txt index cfe507eb9..0141840ba 100644 --- a/Sources/SourceKitLSP/CMakeLists.txt +++ b/Sources/SourceKitLSP/CMakeLists.txt @@ -4,6 +4,7 @@ add_library(SourceKitLSP STATIC DocumentManager.swift IndexStoreDB+MainFilesProvider.swift ResponseError+Init.swift + Rename.swift Sequence+AsyncMap.swift SourceKitIndexDelegate.swift SourceKitLSPCommandMetadata.swift @@ -25,7 +26,6 @@ target_sources(SourceKitLSP PRIVATE Swift/EditorPlaceholder.swift Swift/OpenInterface.swift Swift/RelatedIdentifiers.swift - Swift/Rename.swift Swift/SemanticRefactorCommand.swift Swift/SemanticRefactoring.swift Swift/SemanticTokens.swift diff --git a/Sources/SourceKitLSP/Clang/ClangLanguageServer.swift b/Sources/SourceKitLSP/Clang/ClangLanguageServer.swift index c00092e02..12143d5a9 100644 --- a/Sources/SourceKitLSP/Clang/ClangLanguageServer.swift +++ b/Sources/SourceKitLSP/Clang/ClangLanguageServer.swift @@ -597,10 +597,6 @@ extension ClangLanguageServerShim { func executeCommand(_ req: ExecuteCommandRequest) async throws -> LSPAny? { return try await forwardRequestToClangd(req) } - - func rename(_ request: RenameRequest) async throws -> WorkspaceEdit? { - return try await forwardRequestToClangd(request) - } } /// Clang build settings derived from a `FileBuildSettingsChange`. diff --git a/Sources/SourceKitLSP/Swift/Rename.swift b/Sources/SourceKitLSP/Rename.swift similarity index 75% rename from Sources/SourceKitLSP/Swift/Rename.swift rename to Sources/SourceKitLSP/Rename.swift index a65f22604..6d70be421 100644 --- a/Sources/SourceKitLSP/Swift/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -10,11 +10,14 @@ // //===----------------------------------------------------------------------===// +import IndexStoreDB import LSPLogging import LanguageServerProtocol import SKSupport import SourceKitD +// MARK: - Helper types + /// A parsed representation of a name that may be disambiguated by its argument labels. /// /// ### Examples @@ -268,12 +271,15 @@ fileprivate struct SyntacticRenameName { } } -struct RenameLocation { - /// The line of the identifier to be renamed (1-based). - let line: Int - /// The column of the identifier to be renamed in UTF-8 bytes (1-based). - let utf8Column: Int - let usage: RelatedIdentifier.Usage +private extension LineTable { + subscript(range: Range) -> Substring? { + guard let start = self.stringIndexOf(line: range.lowerBound.line, utf16Column: range.lowerBound.utf16index), + let end = self.stringIndexOf(line: range.upperBound.line, utf16Column: range.upperBound.utf16index) + else { + return nil + } + return self.content[start.. WorkspaceEdit? { + let uri = request.textDocument.uri + guard let workspace = await workspaceForDocument(uri: uri) else { + throw ResponseError.workspaceNotOpen(uri) + } + guard let languageService = workspace.documentService[uri] else { + return nil + } + + // Determine the local edits and the USR to rename + let renameResult = try await languageService.rename(request) + var edits = renameResult.edits + if edits.changes == nil { + // Make sure `edits.changes` is non-nil so we can force-unwrap it below. + edits.changes = [:] + } + + if let usr = renameResult.usr, let oldName = renameResult.oldName, let index = workspace.index { + // If we have a USR + old name, perform an index lookup to find workspace-wide symbols to rename. + // First, group all occurrences of that USR by the files they occur in. + var locationsByFile: [URL: [RenameLocation]] = [:] + let occurrences = index.occurrences(ofUSR: usr, roles: [.declaration, .definition, .reference]) + for occurrence in occurrences { + let url = URL(fileURLWithPath: occurrence.location.path) + let renameLocation = RenameLocation( + line: occurrence.location.line, + utf8Column: occurrence.location.utf8Column, + usage: RenameLocation.Usage(roles: occurrence.roles) + ) + locationsByFile[url, default: []].append(renameLocation) + } + + // Now, call `editsToRename(locations:in:oldName:newName:)` on the language service to convert these ranges into + // edits. + await withTaskGroup(of: (DocumentURI, [TextEdit])?.self) { taskGroup in + for (url, renameLocations) in locationsByFile { + let uri = DocumentURI(url) + if edits.changes![uri] != nil { + // We already have edits for this document provided by the language service, so we don't need to compute + // rename ranges for it. + continue + } + taskGroup.addTask { + // Create a document snapshot to operate on. If the document is open, load it from the document manager, + // otherwise conjure one from the file on disk. We need the file in memory to perform UTF-8 to UTF-16 column + // conversions. + // We should technically infer the language for the from-disk snapshot. But `editsToRename` doesn't care + // about it, so defaulting to Swift is good enough for now + // If we fail to get edits for one file, log an error and continue but don't fail rename completely. + guard + let snapshot = (try? self.documentManager.latestSnapshot(uri)) + ?? (try? DocumentSnapshot(url, language: .swift)) + else { + logger.error("Failed to get document snapshot for \(uri.forLogging)") + return nil + } + do { + let edits = try await languageService.editsToRename( + locations: renameLocations, + in: snapshot, + oldName: oldName, + newName: request.newName + ) + return (uri, edits) + } catch { + logger.error("Failed to get edits for \(uri.forLogging): \(error.forLogging)") + return nil + } + } + } + for await case let (uri, textEdits)? in taskGroup where !textEdits.isEmpty { + precondition(edits.changes![uri] == nil, "We should create tasks for URIs that already have edits") + edits.changes![uri] = textEdits + } + } + } + return edits + } +} + +// MARK: - Swift + extension SwiftLanguageServer { - /// From a list of rename locations compute the list of `SyntacticRenameName`s that define which ranges need to be + /// From a list of rename locations compute the list of `SyntacticRenameName`s that define which ranges need to be /// edited to rename a compound decl name. - /// + /// /// - Parameters: /// - renameLocations: The locations to rename - /// - oldName: The compound decl name that the declaration had before the rename. Used to verify that the rename + /// - oldName: The compound decl name that the declaration had before the rename. Used to verify that the rename /// locations match that name. Eg. `myFunc(argLabel:otherLabel:)` or `myVar` - /// - snapshot: If the document has been modified from the on-disk version, the current snapshot. `nil` to read the - /// file contents from disk. + /// - snapshot: A `DocumentSnapshot` containing the contents of the file for which to compute the rename ranges. private func getSyntacticRenameRanges( renameLocations: [RenameLocation], oldName: String, @@ -329,7 +431,7 @@ extension SwiftLanguageServer { return categorizedRanges.compactMap { SyntacticRenameName($0, in: snapshot, keys: keys) } } - public func rename(_ request: RenameRequest) async throws -> WorkspaceEdit? { + public func rename(_ request: RenameRequest) async throws -> (edits: WorkspaceEdit, usr: String?, oldName: String?) { let snapshot = try self.documentManager.latestSnapshot(request.textDocument.uri) let relatedIdentifiers = try await self.relatedIdentifiers( @@ -340,7 +442,7 @@ extension SwiftLanguageServer { guard let oldName = relatedIdentifiers.name else { throw ResponseError.unknown("Running sourcekit-lsp with a version of sourcekitd that does not support rename") } - + try Task.checkCancellation() let renameLocations = relatedIdentifiers.relatedIdentifiers.compactMap { (relatedIdentifier) -> RenameLocation? in @@ -352,19 +454,38 @@ extension SwiftLanguageServer { } return RenameLocation(line: position.line + 1, utf8Column: utf8Column + 1, usage: relatedIdentifier.usage) } - + try Task.checkCancellation() - let edits = try await renameRanges(from: renameLocations, in: snapshot, oldName: oldName, newName: try CompoundDeclName(request.newName)) + let edits = try await editsToRename( + locations: renameLocations, + in: snapshot, + oldName: oldName, + newName: request.newName + ) + + try Task.checkCancellation() - return WorkspaceEdit(changes: [ - snapshot.uri: edits - ]) + let usr = + (try? await self.symbolInfo(SymbolInfoRequest(textDocument: request.textDocument, position: request.position)))? + .only?.usr + + return (edits: WorkspaceEdit(changes: [snapshot.uri: edits]), usr: usr, oldName: oldName) } - private func renameRanges(from renameLocations: [RenameLocation], in snapshot: DocumentSnapshot, oldName oldNameString: String, newName: CompoundDeclName) async throws -> [TextEdit] { - let compoundRenameRanges = try await getSyntacticRenameRanges(renameLocations: renameLocations, oldName: oldNameString, in: snapshot) + public func editsToRename( + locations renameLocations: [RenameLocation], + in snapshot: DocumentSnapshot, + oldName oldNameString: String, + newName newNameString: String + ) async throws -> [TextEdit] { + let compoundRenameRanges = try await getSyntacticRenameRanges( + renameLocations: renameLocations, + oldName: oldNameString, + in: snapshot + ) let oldName = try CompoundDeclName(oldNameString) + let newName = try CompoundDeclName(newNameString) try Task.checkCancellation() @@ -461,13 +582,20 @@ extension SwiftLanguageServer { } } -extension LineTable { - subscript(range: Range) -> Substring? { - guard let start = self.stringIndexOf(line: range.lowerBound.line, utf16Column: range.lowerBound.utf16index), - let end = self.stringIndexOf(line: range.upperBound.line, utf16Column: range.upperBound.utf16index) - else { - return nil - } - return self.content[start.. (edits: WorkspaceEdit, usr: String?, oldName: String?) { + let edits = try await forwardRequestToClangd(request) + return (edits ?? WorkspaceEdit(), nil, nil) + } + + func editsToRename( + locations renameLocations: [RenameLocation], + in snapshot: DocumentSnapshot, + oldName oldNameString: String, + newName: String + ) async throws -> [TextEdit] { + throw ResponseError.internalError("Global rename not implemented for clangd") } } diff --git a/Sources/SourceKitLSP/SourceKitServer.swift b/Sources/SourceKitLSP/SourceKitServer.swift index cefbe9705..b4c2200e0 100644 --- a/Sources/SourceKitLSP/SourceKitServer.swift +++ b/Sources/SourceKitLSP/SourceKitServer.swift @@ -1646,17 +1646,6 @@ extension SourceKitServer { return try await languageService.executeCommand(executeCommand) } - func rename(_ request: RenameRequest) async throws -> WorkspaceEdit? { - let uri = request.textDocument.uri - guard let workspace = await workspaceForDocument(uri: uri) else { - throw ResponseError.workspaceNotOpen(uri) - } - guard let languageService = workspace.documentService[uri] else { - return nil - } - return try await languageService.rename(request) - } - func codeAction( _ req: CodeActionRequest, workspace: Workspace, diff --git a/Sources/SourceKitLSP/Swift/RelatedIdentifiers.swift b/Sources/SourceKitLSP/Swift/RelatedIdentifiers.swift index 16c9beefe..6ffc0af13 100644 --- a/Sources/SourceKitLSP/Swift/RelatedIdentifiers.swift +++ b/Sources/SourceKitLSP/Swift/RelatedIdentifiers.swift @@ -15,32 +15,11 @@ import LanguageServerProtocol import SourceKitD struct RelatedIdentifier { - enum Usage { - /// The definition of a function/subscript/variable/... - case definition - - /// The symbol is being referenced. - /// - /// This includes - /// - References to variables - /// - Unapplied references to functions (`myStruct.memberFunc`) - /// - Calls to subscripts (`myArray[1]`, location is `[` here, length 1) - case reference - - /// A function that is being called. - case call - - /// Unknown name usage occurs if we don't have an entry in the index that - /// tells us whether the location is a call, reference or a definition. The - /// most common reasons why this happens is if the editor is adding syntactic - /// results (eg. from comments or string literals). - case unknown - } let range: Range - let usage: Usage + let usage: RenameLocation.Usage } -extension RelatedIdentifier.Usage { +extension RenameLocation.Usage { fileprivate init?(_ uid: sourcekitd_uid_t?, _ keys: sourcekitd_keys) { switch uid { case keys.syntacticRenameDefinition: @@ -116,7 +95,7 @@ extension SwiftLanguageServer { let length: Int = value[keys.length], let end: Position = snapshot.positionOf(utf8Offset: offset + length) { - let usage = RelatedIdentifier.Usage(value[keys.nameType], keys) ?? .unknown + let usage = RenameLocation.Usage(value[keys.nameType], keys) ?? .unknown relatedIdentifiers.append( RelatedIdentifier(range: start.. [InlayHint] func documentDiagnostic(_ req: DocumentDiagnosticsRequest) async throws -> DocumentDiagnosticReport + // MARK: - Rename + + /// Entry point to perform rename. + /// + /// Rename is implemented as a two-step process: This function returns all the edits it knows need to be performed. + /// For Swift these edits are those within the current file. In addition, it can return a USR + the old name of the + /// symbol to be renamed so that `SourceKitServer` can perform an index lookup to discover more locations to rename + /// within the entire workspace. `SourceKitServer` will transform those into edits by calling + /// `editsToRename(locations:in:oldName:newName:)` on the toolchain server to perform the actual rename. + func rename(_ request: RenameRequest) async throws -> (edits: WorkspaceEdit, usr: String?, oldName: String?) + + /// Given a list of `locations``, return the list of edits that need to be performed to rename these occurrences from + /// `oldName` to `newName`. + func editsToRename( + locations renameLocations: [RenameLocation], + in snapshot: DocumentSnapshot, + oldName: String, + newName: String + ) async throws -> [TextEdit] + // MARK: - Other func executeCommand(_ req: ExecuteCommandRequest) async throws -> LSPAny? - func rename(_ request: RenameRequest) async throws -> WorkspaceEdit? - /// Crash the language server. Should be used for crash recovery testing only. func _crash() async } diff --git a/Tests/SourceKitLSPTests/RenameTests.swift b/Tests/SourceKitLSPTests/RenameTests.swift index bd55b93d1..f351c8b91 100644 --- a/Tests/SourceKitLSPTests/RenameTests.swift +++ b/Tests/SourceKitLSPTests/RenameTests.swift @@ -486,4 +486,182 @@ final class RenameTests: XCTestCase { """ ) } + + func testCrossFileSwiftRename() async throws { + let ws = try await SwiftPMTestWorkspace( + files: [ + "a.swift": """ + func 1️⃣foo2️⃣() {} + """, + "b.swift": """ + func test() { + 3️⃣foo4️⃣() + } + """, + ], + build: true + ) + + let (aUri, aPositions) = try ws.openDocument("a.swift") + let response = try await ws.testClient.send( + RenameRequest(textDocument: TextDocumentIdentifier(aUri), position: aPositions["1️⃣"], newName: "bar") + ) + let changes = try XCTUnwrap(response?.changes) + XCTAssertEqual( + changes, + [ + aUri: [TextEdit(range: aPositions["1️⃣"].. Date: Fri, 8 Dec 2023 16:17:33 -0800 Subject: [PATCH 3/6] Address stylistic review comments from #990 --- Sources/SourceKitLSP/Rename.swift | 142 ++++++++++++++++-------------- 1 file changed, 78 insertions(+), 64 deletions(-) diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index 6d70be421..a6053c8b5 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -40,19 +40,19 @@ fileprivate struct CompoundDeclName { /// The parameter of a compound decl name, which can either be the parameter's name or `_` to indicate that the /// parameter is unnamed. enum Parameter: Equatable { - case label(String) + case named(String) case wildcard var stringOrWildcard: String { switch self { - case .label(let str): return str + case .named(let str): return str case .wildcard: return "_" } } var stringOrEmpty: String { switch self { - case .label(let str): return str + case .named(let str): return str case .wildcard: return "" } } @@ -83,7 +83,7 @@ fileprivate struct CompoundDeclName { parameters = parameterStrings.map { switch $0 { case "", "_": return .wildcard - default: return .label(String($0)) + default: return .named(String($0)) } } } @@ -434,18 +434,19 @@ extension SwiftLanguageServer { public func rename(_ request: RenameRequest) async throws -> (edits: WorkspaceEdit, usr: String?, oldName: String?) { let snapshot = try self.documentManager.latestSnapshot(request.textDocument.uri) - let relatedIdentifiers = try await self.relatedIdentifiers( + let relatedIdentifiersResponse = try await self.relatedIdentifiers( at: request.position, in: snapshot, includeNonEditableBaseNames: true ) - guard let oldName = relatedIdentifiers.name else { + guard let oldName = relatedIdentifiersResponse.name else { throw ResponseError.unknown("Running sourcekit-lsp with a version of sourcekitd that does not support rename") } try Task.checkCancellation() - let renameLocations = relatedIdentifiers.relatedIdentifiers.compactMap { (relatedIdentifier) -> RenameLocation? in + let renameLocations = relatedIdentifiersResponse.relatedIdentifiers.compactMap { + (relatedIdentifier) -> RenameLocation? in let position = relatedIdentifier.range.lowerBound guard let utf8Column = snapshot.lineTable.utf8ColumnAt(line: position.line, utf16Column: position.utf16index) else { @@ -473,6 +474,69 @@ extension SwiftLanguageServer { return (edits: WorkspaceEdit(changes: [snapshot.uri: edits]), usr: usr, oldName: oldName) } + /// Return the edit that needs to be performed for the given syntactic rename piece to rename it from + /// `oldParameter` to `newParameter`. + /// Returns `nil` if no edit needs to be performed. + private func textEdit( + for piece: SyntacticRenamePiece, + in snapshot: DocumentSnapshot, + oldParameter: CompoundDeclName.Parameter, + newParameter: CompoundDeclName.Parameter + ) -> TextEdit? { + switch piece.kind { + case .parameterName: + if newParameter == .wildcard, piece.range.isEmpty, case .named(let oldParameterName) = oldParameter { + // We are changing a named parameter to an unnamed one. If the parameter didn't have an internal parameter + // name, we need to transfer the previously external parameter name to be the internal one. + // E.g. `func foo(a: Int)` becomes `func foo(_ a: Int)`. + return TextEdit(range: piece.range, newText: " " + oldParameterName) + } + if let original = snapshot.lineTable[piece.range], + case .named(let newParameterLabel) = newParameter, + newParameterLabel.trimmingCharacters(in: .whitespaces) == original.trimmingCharacters(in: .whitespaces) + { + // We are changing the external parameter name to be the same one as the internal parameter name. The + // internal name is thus no longer needed. Drop it. + // Eg. an old declaration `func foo(_ a: Int)` becomes `func foo(a: Int)` when renaming the parameter to `a` + return TextEdit(range: piece.range, newText: "") + } + // In all other cases, don't touch the internal parameter name. It's not part of the public API. + return nil + case .noncollapsibleParameterName: + // Noncollapsible parameter names should never be renamed because they are the same as `parameterName` but + // never fall into one of the two categories above. + return nil + case .declArgumentLabel: + if piece.range.isEmpty { + // If we are inserting a new external argument label where there wasn't one before, add a space after it to + // separate it from the internal name. + // E.g. `subscript(a: Int)` becomes `subscript(a a: Int)`. + return TextEdit(range: piece.range, newText: newParameter.stringOrWildcard + " ") + } + // Otherwise, just update the name. + return TextEdit(range: piece.range, newText: newParameter.stringOrWildcard) + case .callArgumentLabel: + // Argument labels of calls are just updated. + return TextEdit(range: piece.range, newText: newParameter.stringOrEmpty) + case .callArgumentColon: + if case .wildcard = newParameter { + // If the parameter becomes unnamed, remove the colon after the argument name. + return TextEdit(range: piece.range, newText: "") + } + return nil + case .callArgumentCombined: + if case .named(let newParameterName) = newParameter { + // If an unnamed parameter becomes named, insert the new name and a colon. + return TextEdit(range: piece.range, newText: newParameterName + ": ") + } + return nil + case .selectorArgumentLabel: + return TextEdit(range: piece.range, newText: newParameter.stringOrWildcard) + case .baseName, .keywordBaseName: + preconditionFailure("Handled above") + } + } + public func editsToRename( locations renameLocations: [RenameLocation], in snapshot: DocumentSnapshot, @@ -520,63 +584,13 @@ extension SwiftLanguageServer { // renaming `func foo(a: Int, b: Int)` and the user specified `bar(x:)` as the new name. return nil } - let newParameterName = newName.parameters[parameterIndex] - let oldParameterName = oldName.parameters[parameterIndex] - switch piece.kind { - case .parameterName: - if newParameterName == .wildcard, piece.range.isEmpty, case .label(let oldParameterLabel) = oldParameterName { - // We are changing a named parameter to an unnamed one. If the parameter didn't have an internal parameter - // name, we need to transfer the previously external parameter name to be the internal one. - // E.g. `func foo(a: Int)` becomes `func foo(_ a: Int)`. - return TextEdit(range: piece.range, newText: " " + oldParameterLabel) - } else if let original = snapshot.lineTable[piece.range], - case .label(let newParameterLabel) = newParameterName, - newParameterLabel.trimmingCharacters(in: .whitespaces) == original.trimmingCharacters(in: .whitespaces) - { - // We are changing the external parameter name to be the same one as the internal parameter name. The - // internal name is thus no longer needed. Drop it. - // Eg. an old declaration `func foo(_ a: Int)` becomes `func foo(a: Int)` when renaming the parameter to `a` - return TextEdit(range: piece.range, newText: "") - } else { - // In all other cases, don't touch the internal parameter name. It's not part of the public API. - return nil - } - case .noncollapsibleParameterName: - // Noncollapsible parameter names should never be renamed because they are the same as `parameterName` but - // never fall into one of the two categories above. - return nil - case .declArgumentLabel: - if piece.range.isEmpty { - // If we are inserting a new external argument label where there wasn't one before, add a space after it to - // separate it from the internal name. - // E.g. `subscript(a: Int)` becomes `subscript(a a: Int)`. - return TextEdit(range: piece.range, newText: newParameterName.stringOrWildcard + " ") - } else { - // Otherwise, just update the name. - return TextEdit(range: piece.range, newText: newParameterName.stringOrWildcard) - } - case .callArgumentLabel: - // Argument labels of calls are just updated. - return TextEdit(range: piece.range, newText: newParameterName.stringOrEmpty) - case .callArgumentColon: - if case .wildcard = newParameterName { - // If the parameter becomes unnamed, remove the colon after the argument name. - return TextEdit(range: piece.range, newText: "") - } else { - return nil - } - case .callArgumentCombined: - if case .label(let newParameterLabel) = newParameterName { - // If an unnamed parameter becomes named, insert the new name and a colon. - return TextEdit(range: piece.range, newText: newParameterLabel + ": ") - } else { - return nil - } - case .selectorArgumentLabel: - return TextEdit(range: piece.range, newText: newParameterName.stringOrWildcard) - case .baseName, .keywordBaseName: - preconditionFailure("Handled above") - } + + return self.textEdit( + for: piece, + in: snapshot, + oldParameter: oldName.parameters[parameterIndex], + newParameter: newName.parameters[parameterIndex] + ) } } } From 2b9a99e3cb7a5a7cbea8774bf0062b6903910d15 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 8 Dec 2023 16:36:24 -0800 Subject: [PATCH 4/6] Increase test coverage of rename - Invoke the rename request from multiple marker locations within a test file - Add more test cases to test compound decl name references --- .../TestSourceKitLSPClient.swift | 5 + Tests/SourceKitLSPTests/RenameTests.swift | 184 ++++++++---------- 2 files changed, 89 insertions(+), 100 deletions(-) diff --git a/Sources/SKTestSupport/TestSourceKitLSPClient.swift b/Sources/SKTestSupport/TestSourceKitLSPClient.swift index 6ee5c49f1..b9407e4a3 100644 --- a/Sources/SKTestSupport/TestSourceKitLSPClient.swift +++ b/Sources/SKTestSupport/TestSourceKitLSPClient.swift @@ -336,6 +336,11 @@ public struct DocumentPositions { } return position } + + /// Returns all position makers within these `DocumentPositions`. + public var allMarkers: [String] { + return positions.keys.sorted() + } } // MARK: - WeakMessageHelper diff --git a/Tests/SourceKitLSPTests/RenameTests.swift b/Tests/SourceKitLSPTests/RenameTests.swift index f351c8b91..5a6553ddf 100644 --- a/Tests/SourceKitLSPTests/RenameTests.swift +++ b/Tests/SourceKitLSPTests/RenameTests.swift @@ -32,6 +32,8 @@ private func apply(edits: [TextEdit], to source: String) -> String { return lineTable.content } +/// Perform a rename request at every location marker in `markedSource`, renaming it to `newName`. +/// Test that applying the edits returned from the requests always result in `expected`. private func assertSingleFileRename( _ markedSource: String, newName: String, @@ -42,17 +44,19 @@ private func assertSingleFileRename( let testClient = try await TestSourceKitLSPClient() let uri = DocumentURI.for(.swift) let positions = testClient.openDocument(markedSource, uri: uri) - let response = try await testClient.send( - RenameRequest( - textDocument: TextDocumentIdentifier(uri), - position: positions["1️⃣"], - newName: newName + for marker in positions.allMarkers { + let response = try await testClient.send( + RenameRequest( + textDocument: TextDocumentIdentifier(uri), + position: positions[marker], + newName: newName + ) ) - ) - let edits = try XCTUnwrap(response?.changes?[uri], file: file, line: line) - let source = extractMarkers(markedSource).textWithoutMarkers - let renamed = apply(edits: edits, to: source) - XCTAssertEqual(renamed, expected, file: file, line: line) + let edits = try XCTUnwrap(response?.changes?[uri], "while performing rename at \(marker)", file: file, line: line) + let source = extractMarkers(markedSource).textWithoutMarkers + let renamed = apply(edits: edits, to: source) + XCTAssertEqual(renamed, expected, "while performing rename at \(marker)", file: file, line: line) + } } final class RenameTests: XCTestCase { @@ -60,7 +64,7 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ let 1️⃣foo = 1 - print(foo) + print(2️⃣foo) """, newName: "bar", expected: """ @@ -74,12 +78,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo() {} - foo() + 2️⃣foo() + _ = 3️⃣foo """, newName: "bar()", expected: """ func bar() {} bar() + _ = bar """ ) } @@ -88,12 +94,16 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(x: Int) {} - foo(x: 1) + 2️⃣foo(x: 1) + _ = 3️⃣foo(x:) + _ = 4️⃣foo """, newName: "bar(y:)", expected: """ func bar(y: Int) {} bar(y: 1) + _ = bar(y:) + _ = bar """ ) } @@ -102,12 +112,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(x y: Int) {} - foo(x: 1) + 2️⃣foo(x: 1) + _ = 3️⃣foo(x:) """, newName: "foo(y:)", expected: """ func foo(y: Int) {} foo(y: 1) + _ = foo(y:) """ ) } @@ -116,12 +128,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(_ y: Int) {} - foo(1) + 2️⃣foo(1) + _ = 3️⃣foo(_:) """, newName: "foo(y:)", expected: """ func foo(y: Int) {} foo(y: 1) + _ = foo(y:) """ ) } @@ -130,12 +144,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(x: Int) {} - foo(x: 1) + 2️⃣foo(x: 1) + _ = 3️⃣foo(x:) """, newName: "foo(_:)", expected: """ func foo(_ x: Int) {} foo(1) + _ = foo(_:) """ ) } @@ -144,12 +160,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(x a: Int) {} - foo(x: 1) + 2️⃣foo(x: 1) + _ = 3️⃣foo(x:) """, newName: "foo(_:)", expected: """ func foo(_ a: Int) {} foo(1) + _ = foo(_:) """ ) } @@ -160,7 +178,7 @@ final class RenameTests: XCTestCase { struct Foo { 1️⃣subscript(x x: Int) -> Int { x } } - Foo()[x: 1] + Foo()2️⃣[x: 1] """, newName: "subscript(y:)", expected: """ @@ -178,7 +196,7 @@ final class RenameTests: XCTestCase { struct Foo { 1️⃣subscript(x x: Int) -> Int { x } } - Foo()[x: 1] + Foo()2️⃣[x: 1] """, newName: "subscript(_:)", expected: """ @@ -196,7 +214,7 @@ final class RenameTests: XCTestCase { struct Foo { 1️⃣subscript(x: Int) -> Int { x } } - Foo()[1] + Foo()2️⃣[1] """, newName: "subscript(x:)", expected: """ @@ -214,7 +232,7 @@ final class RenameTests: XCTestCase { struct Foo { 1️⃣subscript(x: Int) -> Int { x } } - Foo()[1] + Foo()2️⃣[1] """, newName: "arrayAccess(x:)", expected: """ @@ -233,6 +251,8 @@ final class RenameTests: XCTestCase { 1️⃣init(x: Int) {} } Foo(x: 1) + Foo.2️⃣init(x: 1) + _ = Foo.3️⃣init(x:) """, newName: "init(y:)", expected: """ @@ -240,6 +260,8 @@ final class RenameTests: XCTestCase { init(y: Int) {} } Foo(y: 1) + Foo.init(y: 1) + _ = Foo.init(y:) """ ) } @@ -251,6 +273,8 @@ final class RenameTests: XCTestCase { 1️⃣init(x: Int) {} } Foo(x: 1) + Foo.2️⃣init(x: 1) + _ = Foo.3️⃣init(x:) """, newName: "create(y:)", expected: """ @@ -258,62 +282,8 @@ final class RenameTests: XCTestCase { init(y: Int) {} } Foo(y: 1) - """ - ) - } - - func testRenameCompoundFunctionName() async throws { - try await assertSingleFileRename( - """ - func 1️⃣foo(a: Int) {} - _ = foo(a:) - """, - newName: "foo(b:)", - expected: """ - func foo(b: Int) {} - _ = foo(b:) - """ - ) - } - - func testRemoveLabelFromCompoundFunctionName() async throws { - try await assertSingleFileRename( - """ - func 1️⃣foo(a: Int) {} - _ = foo(a:) - """, - newName: "foo(_:)", - expected: """ - func foo(_ a: Int) {} - _ = foo(_:) - """ - ) - } - - func testIntroduceLabelToCompoundFunctionName() async throws { - try await assertSingleFileRename( - """ - func 1️⃣foo(_ a: Int) {} - _ = foo(_:) - """, - newName: "foo(a:)", - expected: """ - func foo(a: Int) {} - _ = foo(a:) - """ - ) - } - - func testRenameFromReference() async throws { - try await assertSingleFileRename( - """ - func foo(_ a: Int) {} - _ = 1️⃣foo(_:) - """, - newName: "foo(a:)", - expected: """ - func foo(a: Int) {} - _ = foo(a:) + Foo.init(y: 1) + _ = Foo.init(y:) """ ) } @@ -322,12 +292,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(a: Int, b: Int) {} - foo(a: 1, b: 1) + 2️⃣foo(a: 1, b: 1) + _ = 3️⃣foo(a:b:) """, newName: "foo(x:y:)", expected: """ func foo(x: Int, y: Int) {} foo(x: 1, y: 1) + _ = foo(x:y:) """ ) } @@ -336,12 +308,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(a: Int, b: Int) {} - foo(a: 1, b: 1) + 2️⃣foo(a: 1, b: 1) + _ = 3️⃣foo(a:b:) """, newName: "foo(x:)", expected: """ func foo(x: Int, b: Int) {} foo(x: 1, b: 1) + _ = foo(x:b:) """ ) } @@ -350,12 +324,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} - foo(a: 1) + 2️⃣foo(a: 1) + _ = 3️⃣foo(a:) """, newName: "foo(x:y:)", expected: """ func foo(x: Int) {} foo(x: 1) + _ = foo(x:) """ ) } @@ -364,12 +340,14 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} - foo(a: 1) + 2️⃣foo(a: 1) + _ = 3️⃣foo(a:) """, newName: "bar", expected: """ func bar(a: Int) {} bar(a: 1) + _ = bar(a:) """ ) } @@ -378,7 +356,7 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ let 1️⃣foo = 1 - _ = foo + _ = 2️⃣foo """, newName: "bar(x:y:)", expected: """ @@ -389,47 +367,53 @@ final class RenameTests: XCTestCase { } func testErrorIfNewNameDoesntContainClosingParenthesis() async throws { - // FIXME: syntactic rename does not support in-memory files... It should - let ws = try await IndexedSingleSwiftFileWorkspace( + let testClient = try await TestSourceKitLSPClient() + let uri = DocumentURI.for(.swift) + let positions = testClient.openDocument( """ func 1️⃣foo(a: Int) {} - foo(a: 1) - """ + 2️⃣foo(a: 1) + """, + uri: uri ) let request = RenameRequest( - textDocument: TextDocumentIdentifier(ws.fileURI), - position: ws.positions["1️⃣"], + textDocument: TextDocumentIdentifier(uri), + position: positions["1️⃣"], newName: "bar(x:" ) - await assertThrowsError(try await ws.testClient.send(request)) + await assertThrowsError(try await testClient.send(request)) } func testErrorIfNewNameContainsTextAfterParenthesis() async throws { - // FIXME: syntactic rename does not support in-memory files... It should - let ws = try await IndexedSingleSwiftFileWorkspace( + let testClient = try await TestSourceKitLSPClient() + let uri = DocumentURI.for(.swift) + let positions = testClient.openDocument( """ func 1️⃣foo(a: Int) {} - foo(a: 1) - """ + 2️⃣foo(a: 1) + """, + uri: uri ) let request = RenameRequest( - textDocument: TextDocumentIdentifier(ws.fileURI), - position: ws.positions["1️⃣"], + textDocument: TextDocumentIdentifier(uri), + position: positions["1️⃣"], newName: "bar(x:)other:" ) - await assertThrowsError(try await ws.testClient.send(request)) + await assertThrowsError(try await testClient.send(request)) } func testSpacesInNewParameterNames() async throws { try await assertSingleFileRename( """ func 1️⃣foo(a: Int) {} - foo(a: 1) + 2️⃣foo(a: 1) + _ = foo(a:) """, newName: "bar ( x : )", expected: """ func bar ( x : Int) {} bar ( x : 1) + _ = bar ( x :) """ ) } @@ -439,7 +423,7 @@ final class RenameTests: XCTestCase { """ struct Foo {} func 1️⃣+(x: Foo, y: Foo) {} - Foo() + Foo() + Foo() 2️⃣+ Foo() """, newName: "-", expected: """ @@ -454,7 +438,7 @@ final class RenameTests: XCTestCase { try await assertSingleFileRename( """ func 1️⃣foo(x: Int) {} - foo(x: 1) + 2️⃣foo(x: 1) """, newName: "bar(:)", expected: """ @@ -474,7 +458,7 @@ final class RenameTests: XCTestCase { class Foo: NSObject { @objc public func 1️⃣bar(x: Int) {} } - _ = #selector(Foo.bar(x:)) + _ = #selector(Foo.2️⃣bar(x:)) """, newName: "foo(y:)", expected: """ From 86553d98ab251f14f2b96935ed055c3881dc47a7 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 8 Dec 2023 16:41:40 -0800 Subject: [PATCH 5/6] Allow rename when the new name is missing the `)` or if it has text after `)` --- Sources/SourceKitLSP/Rename.swift | 25 +++------------- Tests/SourceKitLSPTests/RenameTests.swift | 36 +++++++++-------------- 2 files changed, 18 insertions(+), 43 deletions(-) diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index a6053c8b5..a01790a61 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -25,18 +25,6 @@ import SourceKitD /// - `foo(_:b:)` /// - `foo` if no argument labels are specified, eg. for a variable. fileprivate struct CompoundDeclName { - enum CompoundDeclNameParsingError: Error, CustomStringConvertible { - case missingClosingParenthesis - case closingParenthesisNotAtEnd - - var description: String { - switch self { - case .missingClosingParenthesis: "Name contains '(' but no matching ')'" - case .closingParenthesisNotAtEnd: "Additional text after ')'" - } - } - } - /// The parameter of a compound decl name, which can either be the parameter's name or `_` to indicate that the /// parameter is unnamed. enum Parameter: Equatable { @@ -62,7 +50,7 @@ fileprivate struct CompoundDeclName { let parameters: [Parameter] /// Parse a compound decl name into its base names and parameters. - init(_ compoundDeclName: String) throws { + init(_ compoundDeclName: String) { guard let openParen = compoundDeclName.firstIndex(of: "(") else { // We don't have a compound name. Everything is the base name self.baseName = compoundDeclName @@ -70,12 +58,7 @@ fileprivate struct CompoundDeclName { return } self.baseName = String(compoundDeclName[.. Date: Mon, 11 Dec 2023 14:41:19 -0800 Subject: [PATCH 6/6] Set the `renameProvider` capability --- Sources/SourceKitLSP/Rename.swift | 2 +- Sources/SourceKitLSP/SourceKitServer.swift | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index a01790a61..0b5bdc408 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -325,7 +325,7 @@ extension SourceKitServer { for (url, renameLocations) in locationsByFile { let uri = DocumentURI(url) if edits.changes![uri] != nil { - // We already have edits for this document provided by the language service, so we don't need to compute + // We already have edits for this document provided by the language service, so we don't need to compute // rename ranges for it. continue } diff --git a/Sources/SourceKitLSP/SourceKitServer.swift b/Sources/SourceKitLSP/SourceKitServer.swift index b4c2200e0..2bcd7128c 100644 --- a/Sources/SourceKitLSP/SourceKitServer.swift +++ b/Sources/SourceKitLSP/SourceKitServer.swift @@ -1169,6 +1169,7 @@ extension SourceKitServer { supportsCodeActions: true ) ), + renameProvider: .value(RenameOptions()), colorProvider: .bool(true), foldingRangeProvider: .bool(!registry.clientHasDynamicFoldingRangeRegistration), declarationProvider: .bool(true),