-
Notifications
You must be signed in to change notification settings - Fork 320
Fix module definition jumping in Swift interface files #2233
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
4058ac6
dd7de19
62334c0
d0e544e
f509987
e147601
67e5f17
82cc691
d143d8d
8dfcbfa
c9a943e
b372182
05d8d02
6f4ee70
36a5f83
00cfa79
6f214ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1850,10 +1850,24 @@ extension SourceKitLSPServer { | |
languageService: LanguageService | ||
) async throws -> [Location] { | ||
// If this symbol is a module then generate a textual interface | ||
if symbol.kind == .module, let name = symbol.name { | ||
if symbol.kind == .module { | ||
// For module symbols, prefer using systemModule information if available | ||
let moduleName: String | ||
let groupName: String? | ||
|
||
if let systemModule = symbol.systemModule { | ||
moduleName = systemModule.moduleName | ||
groupName = systemModule.groupName | ||
} else if let name = symbol.name { | ||
moduleName = name | ||
groupName = nil | ||
} else { | ||
return [] | ||
} | ||
|
||
let interfaceLocation = try await self.definitionInInterface( | ||
moduleName: name, | ||
groupName: nil, | ||
moduleName: moduleName, | ||
groupName: groupName, | ||
symbolUSR: nil, | ||
originatorUri: uri, | ||
languageService: languageService | ||
|
@@ -2051,9 +2065,34 @@ extension SourceKitLSPServer { | |
originatorUri: DocumentURI, | ||
languageService: LanguageService | ||
) async throws -> Location { | ||
// Check if we're already in the target interface with the same module/group/symbol | ||
if case .generatedInterface(let interfaceData) = try? ReferenceDocumentURL(from: originatorUri), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment still applies here: #2233 |
||
interfaceData.moduleName == moduleName && interfaceData.groupName == groupName { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to my other comments about the build settings, we need to check here that we are using build settings from the same original file. But I think as long as we pass along the same build settings file, this shouldn’t cause any issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ahoppen Thanks for the comment without this change, here would happen if I open defination of Cause:
this makes the logic quite complicated
I think we could assume the interface is the same if the interface was originally opened from a same then we could simplify the originalUri as
the benefits are
Untitled.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, IMO when doing a jump to definition from within an interface, the
|
||
// If we have a specific symbol USR, try to find its position in the current interface | ||
if let symbolUSR = symbolUSR, | ||
let swiftLanguageService = languageService as? SwiftLanguageService { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m currently trying to unravel the dependency of the What are the challenges of always going through
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding a group might be a good idea i imagine there could be different versions of swiftinterface which are used by different modules/files i think the challenge is how to determine it in the beginning instead of a uuid |
||
do { | ||
let position = try await swiftLanguageService.generatedInterfaceManager.position( | ||
ofUsr: symbolUSR, | ||
in: interfaceData | ||
) | ||
return Location(uri: originatorUri, range: Range(position)) | ||
} catch { | ||
// If we can't find the symbol, just return the top of the current interface | ||
return Location(uri: originatorUri, range: Range(Position(line: 0, utf16index: 0))) | ||
} | ||
} else { | ||
// No specific symbol, just return the current interface location | ||
return Location(uri: originatorUri, range: Range(Position(line: 0, utf16index: 0))) | ||
} | ||
} | ||
|
||
// If the originator URI is already a generated interface, use its primary file for build settings | ||
let documentForBuildSettings = originatorUri.buildSettingsFile | ||
|
||
guard | ||
let interfaceDetails = try await languageService.openGeneratedInterface( | ||
document: originatorUri, | ||
document: documentForBuildSettings, | ||
moduleName: moduleName, | ||
groupName: groupName, | ||
symbolUSR: symbolUSR | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -67,7 +67,14 @@ package struct GeneratedInterfaceDocumentURLData: Hashable, ReferenceURLData { | |||||||||||||||||||||
self.moduleName = moduleName | ||||||||||||||||||||||
self.groupName = groupName | ||||||||||||||||||||||
self.sourcekitdDocumentName = sourcekitdDocumentName | ||||||||||||||||||||||
self.buildSettingsFrom = primaryFile | ||||||||||||||||||||||
if let referenceDocumentURL = try? ReferenceDocumentURL(from: primaryFile), | ||||||||||||||||||||||
let fileURL = referenceDocumentURL.buildSettingsFile.fileURL { | ||||||||||||||||||||||
self.buildSettingsFrom = DocumentURI(fileURL) | ||||||||||||||||||||||
} else if let fileURL = primaryFile.fileURL { | ||||||||||||||||||||||
self.buildSettingsFrom = DocumentURI(fileURL) | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
self.buildSettingsFrom = primaryFile | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sure buildSettingsFrom can be only an actual file otherwise it could be NOTE: this happens if we enable navigation inside swiftinterface in this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have an accessor on
Suggested change
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
init(queryItems: [URLQueryItem]) throws { | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,10 @@ import SourceKitLSP | |
import SwiftExtensions | ||
import XCTest | ||
|
||
private extension Language { | ||
static let swiftinterface = Language(rawValue: "swiftinterface") | ||
} | ||
|
||
final class SwiftInterfaceTests: XCTestCase { | ||
func testSystemModuleInterface() async throws { | ||
let testClient = try await TestSourceKitLSPClient() | ||
|
@@ -62,7 +66,7 @@ final class SwiftInterfaceTests: XCTestCase { | |
let referenceDocument = try await testClient.send(GetReferenceDocumentRequest(uri: location.uri)) | ||
XCTAssert( | ||
referenceDocument.content.hasPrefix("import "), | ||
"Expected that the foundation swift interface starts with 'import ' but got '\(referenceDocument.content.prefix(100))'" | ||
"Expected foundation swift interface to start with 'import ' but got '\(referenceDocument.content.prefix(100))'" | ||
) | ||
} | ||
|
||
|
@@ -149,7 +153,7 @@ final class SwiftInterfaceTests: XCTestCase { | |
public init() {} | ||
} | ||
""", | ||
"Exec/main.swift": "import 1️⃣MyLibrary", | ||
"Exec/main.swift": "import 1️⃣MyLibrary" | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
], | ||
manifest: """ | ||
let package = Package( | ||
|
@@ -192,21 +196,13 @@ final class SwiftInterfaceTests: XCTestCase { | |
func testSemanticFunctionalityInGeneratedInterface() async throws { | ||
let project = try await SwiftPMTestProject( | ||
files: [ | ||
"MyLibrary/MyLibrary.swift": """ | ||
public struct Lib { | ||
public func foo() -> String {} | ||
public init() {} | ||
} | ||
""", | ||
"Exec/main.swift": "import 1️⃣MyLibrary", | ||
"MyLibrary/MyLibrary.swift": "public struct Lib { public func foo() -> String {} }", | ||
"Exec/main.swift": "import 1️⃣MyLibrary" | ||
], | ||
manifest: """ | ||
let package = Package( | ||
name: "MyLibrary", | ||
targets: [ | ||
.target(name: "MyLibrary"), | ||
.executableTarget(name: "Exec", dependencies: ["MyLibrary"]) | ||
] | ||
targets: [.target(name: "MyLibrary"), .executableTarget(name: "Exec", dependencies: ["MyLibrary"])] | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
""", | ||
capabilities: ClientCapabilities(experimental: [ | ||
|
@@ -216,36 +212,26 @@ final class SwiftInterfaceTests: XCTestCase { | |
) | ||
|
||
let (mainUri, mainPositions) = try project.openDocument("main.swift") | ||
let response = | ||
try await project.testClient.send( | ||
DefinitionRequest( | ||
textDocument: TextDocumentIdentifier(mainUri), | ||
position: mainPositions["1️⃣"] | ||
) | ||
) | ||
let response = try await project.testClient.send( | ||
DefinitionRequest(textDocument: TextDocumentIdentifier(mainUri), position: mainPositions["1️⃣"]) | ||
) | ||
let referenceDocumentUri = try XCTUnwrap(response?.locations?.only).uri | ||
let referenceDocument = try await project.testClient.send(GetReferenceDocumentRequest(uri: referenceDocumentUri)) | ||
let stringIndex = try XCTUnwrap(referenceDocument.content.firstRange(of: "-> String")) | ||
let (stringLine, stringColumn) = LineTable(referenceDocument.content) | ||
.lineAndUTF16ColumnOf(referenceDocument.content.index(stringIndex.lowerBound, offsetBy: 3)) | ||
|
||
project.testClient.send( | ||
DidOpenTextDocumentNotification( | ||
textDocument: TextDocumentItem( | ||
uri: referenceDocumentUri, | ||
language: .swift, | ||
version: 0, | ||
text: referenceDocument.content | ||
|
||
// Test hover functionality in the interface | ||
if let stringIndex = referenceDocument.content.firstRange(of: "-> String") { | ||
let (line, column) = LineTable(referenceDocument.content) | ||
.lineAndUTF16ColumnOf(referenceDocument.content.index(stringIndex.lowerBound, offsetBy: 3)) | ||
project.testClient.send( | ||
DidOpenTextDocumentNotification( | ||
textDocument: TextDocumentItem(uri: referenceDocumentUri, language: .swift, version: 0, text: referenceDocument.content) | ||
) | ||
) | ||
) | ||
let hover = try await project.testClient.send( | ||
HoverRequest( | ||
textDocument: TextDocumentIdentifier(referenceDocumentUri), | ||
position: Position(line: stringLine, utf16index: stringColumn) | ||
let hover = try await project.testClient.send( | ||
HoverRequest(textDocument: TextDocumentIdentifier(referenceDocumentUri), position: Position(line: line, utf16index: column)) | ||
) | ||
) | ||
XCTAssertNotNil(hover) | ||
XCTAssertNotNil(hover) | ||
} | ||
} | ||
|
||
func testJumpToSynthesizedExtensionMethodInSystemModuleWithoutIndex() async throws { | ||
|
@@ -295,6 +281,7 @@ final class SwiftInterfaceTests: XCTestCase { | |
GetReferenceDocumentRequest.method: .dictionary(["supported": .bool(true)]) | ||
]) | ||
) | ||
|
||
let uri = DocumentURI(for: .swift) | ||
|
||
let positions = testClient.openDocument( | ||
|
@@ -319,6 +306,137 @@ final class SwiftInterfaceTests: XCTestCase { | |
) | ||
XCTAssertEqual(diagnostics.fullReport?.items, []) | ||
} | ||
|
||
func testInternalNavigationInSwiftInterface() async throws { | ||
let testClient = try await TestSourceKitLSPClient( | ||
capabilities: ClientCapabilities(experimental: [ | ||
GetReferenceDocumentRequest.method: .dictionary(["supported": .bool(true)]) | ||
]) | ||
) | ||
let uri = DocumentURI(for: .swiftinterface) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original source file is a plain There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
XCTAssertTrue(
location.uri.pseudoPath.hasSuffix(".swiftinterface") || I added test case for file suffix, is it necessary? |
||
|
||
let positions = testClient.openDocument( | ||
""" | ||
func test(x: 1️⃣String) { | ||
let a: 2️⃣Int = 5 | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
""", | ||
uri: uri | ||
) | ||
|
||
// First, get definition for String to open the Swift interface | ||
let stringDefinition = try await testClient.send( | ||
DefinitionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) | ||
) | ||
let interfaceUri = try XCTUnwrap(stringDefinition?.locations?.only?.uri) | ||
let interfaceContents = try await testClient.send(GetReferenceDocumentRequest(uri: interfaceUri)) | ||
// Open the interface document | ||
testClient.send( | ||
DidOpenTextDocumentNotification( | ||
textDocument: TextDocumentItem( | ||
uri: interfaceUri, | ||
language: .swift, | ||
version: 0, | ||
text: interfaceContents.content | ||
) | ||
) | ||
) | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Find a symbol within the interface (e.g., "init" method in String) | ||
guard let initRange = interfaceContents.content.range(of: "public init()") else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we pick a function that’s unique within the Swift interface (eg. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I deleted this tests, maybe it's duplicated |
||
XCTFail("Could not find 'public init()' in String interface") | ||
return | ||
} | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let lineTable = LineTable(interfaceContents.content) | ||
let (line, column) = lineTable.lineAndUTF16ColumnOf(initRange.lowerBound) | ||
let initPosition = Position(line: line, utf16index: column + 7) // Position on "init" | ||
|
||
// Test definition request within the interface | ||
let internalDefinition = try await testClient.send( | ||
DefinitionRequest( | ||
textDocument: TextDocumentIdentifier(interfaceUri), | ||
position: initPosition | ||
) | ||
) | ||
|
||
// The definition should either: | ||
// 1. Return a position within the same interface (internal navigation) | ||
// 2. Return the same position if it's already at the definition | ||
// 3. Return nil if no definition is available | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test should deterministically return the same results, in which case only one of these checks should be true. Could we remove the other options? |
||
if let location = internalDefinition?.locations?.first { | ||
// If we get a location, it should be in the same interface or a related one | ||
XCTAssertTrue( | ||
location.uri.pseudoPath.hasSuffix(".swiftinterface") || | ||
location.uri == interfaceUri, | ||
"Internal navigation should stay within interface files, got: \(location.uri.pseudoPath)" | ||
) | ||
} | ||
} | ||
|
||
func testFoundationImportNavigation() async throws { | ||
ahoppen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let testClient = try await TestSourceKitLSPClient( | ||
capabilities: ClientCapabilities(experimental: [ | ||
GetReferenceDocumentRequest.method: .dictionary(["supported": .bool(true)]) | ||
]) | ||
) | ||
let uri = DocumentURI(for: .swiftinterface) | ||
|
||
let positions = testClient.openDocument( | ||
""" | ||
import 1️⃣Foundation | ||
""", | ||
uri: uri | ||
) | ||
|
||
// Test navigation to Foundation module | ||
let foundationDefinition = try await testClient.send( | ||
DefinitionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) | ||
) | ||
let foundationLocation = try XCTUnwrap(foundationDefinition?.locations?.only) | ||
// Verify it's a swiftinterface file (can be either file:// or sourcekit-lsp:// scheme) | ||
XCTAssertTrue( | ||
foundationLocation.uri.pseudoPath.hasSuffix(".swiftinterface") || | ||
(foundationLocation.uri.scheme == "sourcekit-lsp" && | ||
foundationLocation.uri.pseudoPath.contains("Foundation.swiftinterface")) | ||
) | ||
} | ||
|
||
func testFoundationSubmoduleNavigation() async throws { | ||
let testClient = try await TestSourceKitLSPClient( | ||
capabilities: ClientCapabilities(experimental: [ | ||
GetReferenceDocumentRequest.method: .dictionary(["supported": .bool(true)]) | ||
]) | ||
) | ||
let uri = DocumentURI(for: .swift) | ||
|
||
let positions = testClient.openDocument( | ||
""" | ||
import 1️⃣Foundation.2️⃣NSAffineTransform | ||
""", | ||
uri: uri | ||
) | ||
|
||
let foundationDefinition = try await testClient.send( | ||
DefinitionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) | ||
) | ||
if let foundationLocation = foundationDefinition?.locations?.only { | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
XCTAssertTrue( | ||
foundationLocation.uri.pseudoPath.contains("Foundation.swiftinterface") || | ||
foundationLocation.uri.scheme == "sourcekit-lsp" | ||
) | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
// Test navigation to NSAffineTransform | ||
let transformDefinition = try await testClient.send( | ||
DefinitionRequest(textDocument: TextDocumentIdentifier(uri), position: positions["2️⃣"]) | ||
) | ||
if let transformLocation = transformDefinition?.locations?.only { | ||
// Verify we can identify this as a swiftinterface file | ||
XCTAssertTrue( | ||
transformLocation.uri.pseudoPath.contains("Foundation.NSAffineTransform.swiftinterface") || | ||
transformLocation.uri.scheme == "sourcekit-lsp" | ||
) | ||
} | ||
} | ||
} | ||
|
||
private func assertSystemSwiftInterface( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my other comment about the
.swiftinterface
extension, I don’t think we need this here, do we? This only applies to tests and I don’t think we open any.swiftinterface
files in tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted but please check the tests about
swiftinterface