From bda1387a46ee88739d92af0bf35725886770b9ce Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 19 Dec 2023 10:54:06 -0800 Subject: [PATCH 1/2] Refactor rename to avoid a force unwrap --- Sources/SourceKitLSP/Rename.swift | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Sources/SourceKitLSP/Rename.swift b/Sources/SourceKitLSP/Rename.swift index 0b5bdc408..6a04bed2f 100644 --- a/Sources/SourceKitLSP/Rename.swift +++ b/Sources/SourceKitLSP/Rename.swift @@ -298,11 +298,7 @@ extension SourceKitServer { // 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 = [:] - } + var changes = renameResult.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. @@ -324,7 +320,7 @@ extension SourceKitServer { await withTaskGroup(of: (DocumentURI, [TextEdit])?.self) { taskGroup in for (url, renameLocations) in locationsByFile { let uri = DocumentURI(url) - if edits.changes![uri] != nil { + if 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 @@ -358,11 +354,13 @@ extension SourceKitServer { } } 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 + precondition(changes[uri] == nil, "We should not create tasks for URIs that already have edits") + changes[uri] = textEdits } } } + var edits = renameResult.edits + edits.changes = changes return edits } } From f88c241714120c6818f25d2b636fd0805a03f795 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 19 Dec 2023 10:54:35 -0800 Subject: [PATCH 2/2] Refactor multi-file rename tests to specify expected renamed source instead of asserting on edits --- .../MultiFileTestWorkspace.swift | 4 +- Tests/SourceKitLSPTests/RenameTests.swift | 278 ++++++++++-------- 2 files changed, 164 insertions(+), 118 deletions(-) diff --git a/Sources/SKTestSupport/MultiFileTestWorkspace.swift b/Sources/SKTestSupport/MultiFileTestWorkspace.swift index 78e26a307..50efded06 100644 --- a/Sources/SKTestSupport/MultiFileTestWorkspace.swift +++ b/Sources/SKTestSupport/MultiFileTestWorkspace.swift @@ -17,10 +17,10 @@ import SKCore /// The location of a test file within test workspace. public struct RelativeFileLocation: Hashable, ExpressibleByStringLiteral { /// The subdirectories in which the file is located. - let directories: [String] + public let directories: [String] /// The file's name. - let fileName: String + public let fileName: String public init(directories: [String] = [], _ fileName: String) { self.directories = directories diff --git a/Tests/SourceKitLSPTests/RenameTests.swift b/Tests/SourceKitLSPTests/RenameTests.swift index 040d10cb1..61622de74 100644 --- a/Tests/SourceKitLSPTests/RenameTests.swift +++ b/Tests/SourceKitLSPTests/RenameTests.swift @@ -38,11 +38,12 @@ private func assertSingleFileRename( _ markedSource: String, newName: String, expected: String, + testName: String = #function, file: StaticString = #file, line: UInt = #line ) async throws { let testClient = try await TestSourceKitLSPClient() - let uri = DocumentURI.for(.swift) + let uri = DocumentURI.for(.swift, testName: testName) let positions = testClient.openDocument(markedSource, uri: uri) for marker in positions.allMarkers { let response = try await testClient.send( @@ -59,6 +60,90 @@ private func assertSingleFileRename( } } +/// Assert that applying changes to `originalFiles` results in `expected`. +/// +/// Upon failure, `message` is added to the XCTest failure messages to provide context which rename failed. +private func assertRenamedSourceMatches( + originalFiles: [RelativeFileLocation: String], + changes: [DocumentURI: [TextEdit]], + expected: [RelativeFileLocation: String], + in ws: MultiFileTestWorkspace, + message: String, + testName: String = #function, + file: StaticString = #file, + line: UInt = #line +) throws { + for (expectedFileLocation, expectedRenamed) in expected { + let originalMarkedSource = try XCTUnwrap( + originalFiles[expectedFileLocation], + "No original source for \(expectedFileLocation.fileName) specified; \(message)", + file: file, + line: line + ) + let originalSource = extractMarkers(originalMarkedSource).textWithoutMarkers + let edits = changes[try ws.uri(for: expectedFileLocation.fileName)] ?? [] + let renamed = apply(edits: edits, to: originalSource) + XCTAssertEqual( + renamed, + expectedRenamed, + "applying edits did not match expected renamed source for \(expectedFileLocation.fileName); \(message)", + file: file, + line: line + ) + } +} + +/// Perform a rename request at every location marker except 0️⃣ in `files`, renaming it to `newName`. The location +/// marker 0️⃣ is intended to be used as an anchor for `preRenameActions`. +/// +/// Test that applying the edits returned from the requests always result in `expected`. +/// +/// `preRenameActions` is executed after opening the workspace but before performing the rename. This allows a workspace +/// 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], + newName: String, + expected: [RelativeFileLocation: String], + manifest: String = SwiftPMTestWorkspace.defaultPackageManifest, + preRenameActions: (SwiftPMTestWorkspace) throws -> Void = { _ in }, + testName: String = #function, + file: StaticString = #file, + line: UInt = #line +) async throws { + let ws = try await SwiftPMTestWorkspace( + files: files, + manifest: manifest, + build: true, + testName: testName + ) + try preRenameActions(ws) + for (fileLocation, markedSource) in files.sorted(by: { $0.key.fileName < $1.key.fileName }) { + let markers = extractMarkers(markedSource).markers.keys.sorted().filter { $0 != "0️⃣" } + if markers.isEmpty { + continue + } + let (uri, positions) = try ws.openDocument(fileLocation.fileName) + defer { + ws.testClient.send(DidCloseTextDocumentNotification(textDocument: TextDocumentIdentifier(uri))) + } + for marker in markers { + let response = try await ws.testClient.send( + RenameRequest(textDocument: TextDocumentIdentifier(uri), position: positions[marker], newName: newName) + ) + let changes = try XCTUnwrap(response?.changes) + try assertRenamedSourceMatches( + originalFiles: files, + changes: changes, + expected: expected, + in: ws, + message: "while performing rename at \(marker)", + file: file, + line: line + ) + } + } +} + final class RenameTests: XCTestCase { func testRenameVariableBaseName() async throws { try await assertSingleFileRename( @@ -464,46 +549,53 @@ final class RenameTests: XCTestCase { } func testCrossFileSwiftRename() async throws { - let ws = try await SwiftPMTestWorkspace( + try await assertMultiFileRename( files: [ "a.swift": """ - func 1️⃣foo2️⃣() {} + func 1️⃣foo() {} """, "b.swift": """ func test() { - 3️⃣foo4️⃣() + 2️⃣foo() } """, ], - 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️⃣"]..