From cf56bf2ce6e44d07bdc2eaa1348ddcd8322a6201 Mon Sep 17 00:00:00 2001 From: loveucifer <134506987+loveucifer@users.noreply.github.com> Date: Mon, 19 Jan 2026 17:12:33 +0530 Subject: [PATCH] Address PR review feedback - Add DefinitionLocations.swift to CMakeLists.txt - Add proper documentation comments to functions in DefinitionLocations.swift - Make indexToLSPLocation private since it's only used within the file - Revert unrelated comment changes in InlayHintResolve.swift - Use .only instead of .first in InlayHintResolve to avoid ambiguous types - Refactor TypeDefinition.swift to use cleaner control flow with guard/else - Add test for jumping to generated interface (String) --- Sources/SourceKitLSP/CMakeLists.txt | 1 + .../SourceKitLSP/DefinitionLocations.swift | 19 ++++++++-- .../InlayHintResolve.swift | 8 +++- .../SwiftLanguageService/TypeDefinition.swift | 37 +++++++++++-------- .../TypeDefinitionTests.swift | 32 ++++++++++++++++ 5 files changed, 76 insertions(+), 21 deletions(-) diff --git a/Sources/SourceKitLSP/CMakeLists.txt b/Sources/SourceKitLSP/CMakeLists.txt index f4c25ecc..ecc446eb 100644 --- a/Sources/SourceKitLSP/CMakeLists.txt +++ b/Sources/SourceKitLSP/CMakeLists.txt @@ -4,6 +4,7 @@ add_library(SourceKitLSP STATIC DocumentManager.swift DocumentSnapshot+FromFileContents.swift DocumentSnapshot+PositionConversions.swift + DefinitionLocations.swift GeneratedInterfaceDocumentURLData.swift Hooks.swift IndexProgressManager.swift diff --git a/Sources/SourceKitLSP/DefinitionLocations.swift b/Sources/SourceKitLSP/DefinitionLocations.swift index 7fbc1f45..098dab14 100644 --- a/Sources/SourceKitLSP/DefinitionLocations.swift +++ b/Sources/SourceKitLSP/DefinitionLocations.swift @@ -15,21 +15,28 @@ import IndexStoreDB @_spi(SourceKitLSP) import SKLogging package import SemanticIndex -/// converts a symbol index location to an LSP location -func indexToLSPLocation(_ location: SymbolLocation) -> Location? { +/// Converts a location from the symbol index to an LSP location. +/// +/// - Parameter location: The symbol index location +/// - Returns: The LSP location +private func indexToLSPLocation(_ location: SymbolLocation) -> Location? { guard !location.path.isEmpty else { return nil } return Location( uri: location.documentUri, range: Range( Position( + // 1-based -> 0-based + // Note that we still use max(0, ...) as a fallback if the location is zero. line: max(0, location.line - 1), + // Technically we would need to convert the UTF-8 column to a UTF-16 column. This would require reading the + // file. In practice they almost always coincide, so we accept the incorrectness here to avoid the file read. utf16index: max(0, location.utf8Column - 1) ) ) ) } -/// returns the definition location for a symbol, handling generated interfaces for SDK types +/// Return the locations for jump to definition from the given `SymbolDetails`. package func definitionLocations( for symbol: SymbolDetails, originatorUri: DocumentURI, @@ -112,7 +119,11 @@ package func definitionLocations( return occurrences.compactMap { indexToLSPLocation($0.location) }.sorted() } -/// generates a swift interface and returns location for the given symbol +/// Generate the generated interface for the given module, write it to disk and return the location to which to jump +/// to get to the definition of `symbolUSR`. +/// +/// `originatorUri` is the URI of the file from which the definition request is performed. It is used to determine the +/// compiler arguments to generate the generated interface. package func definitionInInterface( moduleName: String, groupName: String?, diff --git a/Sources/SwiftLanguageService/InlayHintResolve.swift b/Sources/SwiftLanguageService/InlayHintResolve.swift index ca38630b..b567a9dd 100644 --- a/Sources/SwiftLanguageService/InlayHintResolve.swift +++ b/Sources/SwiftLanguageService/InlayHintResolve.swift @@ -16,6 +16,7 @@ import IndexStoreDB import SemanticIndex import SourceKitD import SourceKitLSP +import SwiftExtensions extension SwiftLanguageService { /// Resolves an inlay hint by looking up the type definition location. @@ -59,7 +60,10 @@ extension SwiftLanguageService { return hint } - /// looks up the definition location for the type at the given position + /// Looks up the definition location for the type at the given position. + /// + /// This is used by inlay hint resolution to enable go-to-definition on type hints. + /// For SDK types, this returns a location in the generated interface. func lookupTypeDefinitionLocation( snapshot: DocumentSnapshot, position: Position @@ -98,6 +102,6 @@ extension SwiftLanguageService { } ) - return locations.first + return locations.only } } diff --git a/Sources/SwiftLanguageService/TypeDefinition.swift b/Sources/SwiftLanguageService/TypeDefinition.swift index aa636f15..8bfc217f 100644 --- a/Sources/SwiftLanguageService/TypeDefinition.swift +++ b/Sources/SwiftLanguageService/TypeDefinition.swift @@ -17,7 +17,7 @@ import SourceKitD import SourceKitLSP extension SwiftLanguageService { - /// handles the textDocument/typeDefinition request + /// Handles the textDocument/typeDefinition request. package func typeDefinition(_ request: TypeDefinitionRequest) async throws -> LocationsOrLocationLinksResponse? { let uri = request.textDocument.uri let position = request.position @@ -36,28 +36,35 @@ extension SwiftLanguageService { let dict = try await send(sourcekitdRequest: \.cursorInfo, skreq, snapshot: snapshot) let documentManager = try self.documentManager - // if cursor is on a type symbol itself, use its USR directly - var symbol: SymbolDetails? + // Helper to get symbol from typeUsr + func symbolFromTypeUsr() async throws -> SymbolDetails? { + guard let typeUsr: String = dict[keys.typeUsr] else { + return nil + } + guard let typeInfo = try await cursorInfoFromTypeUSR(typeUsr, in: snapshot) else { + return nil + } + return typeInfo.symbolInfo + } + + // If the cursor is on a type symbol itself, use its USR directly. + // Otherwise get the type of the symbol at this position. + let symbol: SymbolDetails if let cursorInfo = CursorInfo(dict, snapshot: snapshot, documentManager: documentManager, sourcekitd: sourcekitd) { switch cursorInfo.symbolInfo.kind { case .class, .struct, .enum, .interface, .typeParameter: symbol = cursorInfo.symbolInfo default: - break + guard let typeSymbol = try await symbolFromTypeUsr() else { + return nil + } + symbol = typeSymbol } - } - - // otherwise get the type of the symbol at this position - if symbol == nil { - guard let typeUsr: String = dict[keys.typeUsr] else { + } else { + guard let typeSymbol = try await symbolFromTypeUsr() else { return nil } - let typeInfo = try await cursorInfoFromTypeUSR(typeUsr, in: snapshot) - symbol = typeInfo?.symbolInfo - } - - guard let symbol else { - return nil + symbol = typeSymbol } let locations = try await SourceKitLSP.definitionLocations( diff --git a/Tests/SourceKitLSPTests/TypeDefinitionTests.swift b/Tests/SourceKitLSPTests/TypeDefinitionTests.swift index ae3fd2ee..c248b7da 100644 --- a/Tests/SourceKitLSPTests/TypeDefinitionTests.swift +++ b/Tests/SourceKitLSPTests/TypeDefinitionTests.swift @@ -12,6 +12,7 @@ @_spi(SourceKitLSP) import LanguageServerProtocol import SKTestSupport +import SwiftExtensions import XCTest final class TypeDefinitionTests: SourceKitLSPTestCase { @@ -171,4 +172,35 @@ final class TypeDefinitionTests: SourceKitLSPTestCase { XCTAssertEqual(location.uri, uri) XCTAssertEqual(location.range, Range(positions["1️⃣"])) } + + func testTypeDefinitionGeneratedInterface() async throws { + let testClient = try await TestSourceKitLSPClient() + let uri = DocumentURI(for: .swift) + + let positions = testClient.openDocument( + """ + let 1️⃣x = "hello" + """, + uri: uri + ) + + let response = try await testClient.send( + TypeDefinitionRequest( + textDocument: TextDocumentIdentifier(uri), + position: positions["1️⃣"] + ) + ) + + guard case .locations(let locations) = response, let location = locations.only else { + XCTFail("Expected single location response") + return + } + + // Should jump to String in the generated Swift interface + XCTAssertTrue( + location.uri.pseudoPath.hasSuffix(".swiftinterface"), + "Expected swiftinterface file, got: \(location.uri.pseudoPath)" + ) + assertContains(location.uri.pseudoPath, "String") + } }