Skip to content

Support cross-file rename for clang languages #1031

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
Jan 24, 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
1 change: 1 addition & 0 deletions Sources/LanguageServerProtocol/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ add_library(LanguageServerProtocol STATIC
Requests/FormattingRequests.swift
Requests/HoverRequest.swift
Requests/ImplementationRequest.swift
Requests/IndexedRenameRequest.swift
Requests/InitializeRequest.swift
Requests/InlayHintRefreshRequest.swift
Requests/InlayHintRequest.swift
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
//===----------------------------------------------------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//

/// Rename all occurrences of a symbol named `oldName` to `newName` at the
/// given `positions`.
///
/// The use case of this method is for when the positions to rename are already
/// known, eg. from an index lookup outside of clangd's built-in index. In
/// particular, it determines the edits necessary to rename multi-piece
/// Objective-C selector names.
///
/// `textDocument` is used to determine the language options for the symbol to
/// rename, eg. to decide whether `oldName` and `newName` are Objective-C
/// selectors or normal identifiers.
///
/// This is a clangd extension.
public struct IndexedRenameRequest: TextDocumentRequest, Hashable {
public static let method: String = "workspace/indexedRename"
public typealias Response = WorkspaceEdit?

/// The document in which the declaration to rename is declared. Its compiler
/// arguments are used to infer language settings for the rename.
public var textDocument: TextDocumentIdentifier

/// The old name of the symbol.
public var oldName: String

/// The new name of the symbol.
public var newName: String

/// The positions at which the symbol is known to appear and that should be
/// renamed. The key is a document URI
public var positions: [DocumentURI: [Position]]

public init(
textDocument: TextDocumentIdentifier,
oldName: String,
newName: String,
positions: [DocumentURI: [Position]]
) {
self.textDocument = textDocument
self.oldName = oldName
self.newName = newName
self.positions = positions
}
}

// Workaround for Codable not correctly encoding dictionaries whose keys aren't strings.
extension IndexedRenameRequest: Codable {
private enum CodingKeys: CodingKey {
case textDocument
case oldName
case newName
case positions
}

public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)

self.textDocument = try container.decode(
TextDocumentIdentifier.self,
forKey: IndexedRenameRequest.CodingKeys.textDocument
)
self.oldName = try container.decode(String.self, forKey: IndexedRenameRequest.CodingKeys.oldName)
self.newName = try container.decode(String.self, forKey: IndexedRenameRequest.CodingKeys.newName)
self.positions = try container.decode([String: [Position]].self, forKey: .positions).mapKeys(DocumentURI.init)
}

public func encode(to encoder: Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)

try container.encode(self.textDocument, forKey: IndexedRenameRequest.CodingKeys.textDocument)
try container.encode(self.oldName, forKey: IndexedRenameRequest.CodingKeys.oldName)
try container.encode(self.newName, forKey: IndexedRenameRequest.CodingKeys.newName)
try container.encode(self.positions.mapKeys(\.stringValue), forKey: IndexedRenameRequest.CodingKeys.positions)

}
}

fileprivate extension Dictionary {
func mapKeys<NewKeyType: Hashable>(_ transform: (Key) -> NewKeyType) -> [NewKeyType: Value] {
return [NewKeyType: Value](uniqueKeysWithValues: self.map { (transform($0.key), $0.value) })
}
}
32 changes: 27 additions & 5 deletions Sources/SourceKitLSP/Rename.swift
Original file line number Diff line number Diff line change
Expand Up @@ -608,18 +608,40 @@ extension SwiftLanguageServer {
// MARK: - Clang

extension ClangLanguageServerShim {
func rename(_ request: RenameRequest) async throws -> (edits: WorkspaceEdit, usr: String?, oldName: String?) {
let edits = try await forwardRequestToClangd(request)
return (edits ?? WorkspaceEdit(), nil, nil)
func rename(_ renameRequest: RenameRequest) async throws -> (edits: WorkspaceEdit, usr: String?, oldName: String?) {
async let edits = forwardRequestToClangd(renameRequest)
let symbolInfoRequest = SymbolInfoRequest(
textDocument: renameRequest.textDocument,
position: renameRequest.position
)
let symbolDetail = try await forwardRequestToClangd(symbolInfoRequest).only
return (try await edits ?? WorkspaceEdit(), symbolDetail?.usr, symbolDetail?.name)
}

func editsToRename(
locations renameLocations: [RenameLocation],
in snapshot: DocumentSnapshot,
oldName oldNameString: String,
oldName: String,
newName: String
) async throws -> [TextEdit] {
throw ResponseError.internalError("Global rename not implemented for clangd")
let positions = [
snapshot.uri: renameLocations.compactMap {
snapshot.positionOf(zeroBasedLine: $0.line - 1, utf8Column: $0.utf8Column - 1)
}
]
let request = IndexedRenameRequest(
textDocument: TextDocumentIdentifier(snapshot.uri),
oldName: oldName,
newName: newName,
positions: positions
)
do {
let edits = try await forwardRequestToClangd(request)
return edits?.changes?[snapshot.uri] ?? []
} catch {
logger.error("Failed to get indexed rename edits: \(error.forLogging)")
return []
}
}

public func prepareRename(_ request: PrepareRenameRequest) async throws -> PrepareRenameResponse? {
Expand Down
76 changes: 74 additions & 2 deletions Tests/SourceKitLSPTests/RenameTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ private func apply(edits: [TextEdit], to source: String) -> String {
/// Test that applying the edits returned from the requests always result in `expected`.
private func assertSingleFileRename(
_ markedSource: String,
language: Language? = nil,
newName: String,
expected: String,
testName: String = #function,
Expand All @@ -44,7 +45,7 @@ private func assertSingleFileRename(
) async throws {
let testClient = try await TestSourceKitLSPClient()
let uri = DocumentURI.for(.swift, testName: testName)
let positions = testClient.openDocument(markedSource, uri: uri)
let positions = testClient.openDocument(markedSource, uri: uri, language: language)
for marker in positions.allMarkers {
let response: WorkspaceEdit?
do {
Expand Down Expand Up @@ -111,6 +112,7 @@ private func assertRenamedSourceMatches(
/// to be placed in a state where there are in-memory changes that haven't been written to disk yet.
private func assertMultiFileRename(
files: [RelativeFileLocation: String],
language: Language? = nil,
newName: String,
expected: [RelativeFileLocation: String],
manifest: String = SwiftPMTestWorkspace.defaultPackageManifest,
Expand All @@ -131,7 +133,7 @@ private func assertMultiFileRename(
if markers.isEmpty {
continue
}
let (uri, positions) = try ws.openDocument(fileLocation.fileName)
let (uri, positions) = try ws.openDocument(fileLocation.fileName, language: language)
defer {
ws.testClient.send(DidCloseTextDocumentNotification(textDocument: TextDocumentIdentifier(uri)))
}
Expand Down Expand Up @@ -759,4 +761,74 @@ final class RenameTests: XCTestCase {
XCTAssertEqual(range, positions["1️⃣"]..<positions["2️⃣"])
XCTAssertEqual(placeholder, "foo(a:b:)")
}

func testGlobalRenameC() async throws {
try await assertMultiFileRename(
files: [
"Sources/MyLibrary/include/lib.h": """
void 1️⃣do2️⃣Stuff();
""",
"lib.c": """
#include "lib.h"

void 3️⃣doStuff() {
4️⃣doStuff();
}
""",
],
language: .c,
newName: "doRecursiveStuff",
expected: [
"Sources/MyLibrary/include/lib.h": """
void doRecursiveStuff();
""",
"lib.c": """
#include "lib.h"

void doRecursiveStuff() {
doRecursiveStuff();
}
""",
]
)
}

func testGlobalRenameObjC() async throws {
try await assertMultiFileRename(
files: [
"Sources/MyLibrary/include/lib.h": """
@interface Foo
- (int)1️⃣perform2️⃣Action:(int)action 3️⃣wi4️⃣th:(int)value;
@end
""",
"lib.m": """
#include "lib.h"

@implementation Foo
- (int)5️⃣performAction:(int)action 6️⃣with:(int)value {
return [self 7️⃣performAction:action 8️⃣with:value];
}
@end
""",
],
language: .objective_c,
newName: "performNewAction:by:",
expected: [
"Sources/MyLibrary/include/lib.h": """
@interface Foo
- (int)performNewAction:(int)action by:(int)value;
@end
""",
"lib.m": """
#include "lib.h"

@implementation Foo
- (int)performNewAction:(int)action by:(int)value {
return [self performNewAction:action by:value];
}
@end
""",
]
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a cross-language test? Or multiple even, eg. swift <-> objc, swift <-> c, and swift <-> c++?

Copy link
Member Author

@ahoppen ahoppen Jan 24, 2024

Choose a reason for hiding this comment

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

Ha, no. This PR is about adding global rename for clang languages, not doing cross-language rename yet. For that we need name translation, which I’ll add in a follow-up PR (and which is currently blocked by rdar://120066209).

rdar://118996461 is tracking the addition of cross-language rename.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC only ObjC is blocked by that right 😅? But cool, didn't realize we didn't have name translation yet, thanks 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but ObjC is really the interesting candidate for name translation. And it’s easiest to design it with all name translations in mind. There will be a follow-up PR soon-ish.

}