Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+108 −9
Open

Fix module definition jumping in Swift interface files #2233

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4058ac6
fixed group of module can't navi, no langserver for swiftinterface
Aug 11, 2025
dd7de19
fixed second swiftinterface page won't response
Aug 11, 2025
62334c0
revert cache strategy
Aug 12, 2025
d0e544e
clean old implementation
Aug 12, 2025
f509987
drop unnecessary change of sourcekitdDocumentName
Aug 12, 2025
e147601
drop unnecessary change of cachedIndex
Aug 12, 2025
67e5f17
revert changes
Aug 12, 2025
82cc691
find buildSettingsFile, primaryFile for RefenceDocumentURL
Aug 12, 2025
d143d8d
clean Language+Inference.swift
Aug 12, 2025
8dfcbfa
buildSettingFrom can be only file://
Aug 12, 2025
c9a943e
add unit test for supporting swiftinterface
Aug 12, 2025
b372182
revert unnecessary changes
Aug 13, 2025
05d8d02
fixed various test issues, format
Aug 13, 2025
6f4ee70
revert changes
Aug 13, 2025
36a5f83
fixed tests and format
Aug 13, 2025
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
2 changes: 1 addition & 1 deletion Sources/SKTestSupport/Utils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ extension Language {
case "cpp": self = .cpp
case "m": self = .objective_c
case "mm": self = .objective_cpp
case "swift": self = .swift
case "swift", "swiftinterface": self = .swift
Copy link
Member

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.

Copy link
Author

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

case "md": self = .markdown
case "tutorial": self = .tutorial
default: return nil
Expand Down
47 changes: 43 additions & 4 deletions Sources/SourceKitLSP/SourceKitLSPServer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

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

This comment still applies here: #2233

interfaceData.moduleName == moduleName && interfaceData.groupName == groupName {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Author

@aelam aelam Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The 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 Foundation in Foundation.swiftInterace, it will open another one, repeat, it will open another new one
even if it's acceptable but it's quite slow (please check the video)

Cause:

originalUri is sourcekit-lsp://xxx?buildSettingsFrom=A
A is sourcekit-lsp://xxx?buildSettingsFrom=B
B is sourcekit-lsp://xxx?buildSettingsFrom=C
C is file://

this makes the logic quite complicated

  1. we have to recursively find the buildSettingsFrom
  2. originalUri are different for same swiftinterface

I think we could assume the interface is the same if the interface was originally opened from a same buildSettingsFrom
because interface opened from A, B should always be consistent?

then we could simplify the originalUri as

originalUri is sourcekit-lsp://xxx?buildSettingsFrom=C

the benefits are

  1. we don't need to resursively search
  2. caching/creating of swiftinterface could be simplified and easy to hit the cache
  3. much faster
  4. the code change here is not needed any more.
Untitled.mov

Copy link
Contributor

Choose a reason for hiding this comment

The 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 buildSettingsFrom should be the original source file rather than the interface you're currently in. IIUC this is what you're saying in:

then we could simplify the originalUri as
originalUri is sourcekit-lsp://xxx?buildSettingsFrom=C
? If so, then 👍

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

I’m currently trying to unravel the dependency of the SourceKitLSP module on SwiftLanguageService in #2234, so getting the SwiftLanguageService won’t work in the future.

What are the challenges of always going through openGeneratedInterface? I can see the following but I might be missing some:

  • Every time openGeneratedInterface is called, we get a new sourcekitdDocumentName. We could fix that by instead of appending the group name and a hash of document (or something of that sort).
  • If the issue is that we always write a new file if the client doesn’t support reference documents, we could just not write out the file if the contents haven’t changed

Copy link
Author

Choose a reason for hiding this comment

The 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
then the cache logic could be simplified

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.primaryFile ?? originatorUri

guard
let interfaceDetails = try await languageService.openGeneratedInterface(
document: originatorUri,
document: documentForBuildSettings,
moduleName: moduleName,
groupName: groupName,
symbolUSR: symbolUSR
Expand Down
16 changes: 10 additions & 6 deletions Sources/SourceKitLSP/Swift/ReferenceDocumentURL.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,22 @@ package enum ReferenceDocumentURL {
var buildSettingsFile: DocumentURI {
switch self {
case .macroExpansion(let data): return data.primaryFile
case .generatedInterface(let data): return data.buildSettingsFrom
case .generatedInterface(let data):
if let referenceDocumentURL = try? ReferenceDocumentURL(from: data.buildSettingsFrom) {
return referenceDocumentURL.buildSettingsFile
}
return data.buildSettingsFrom
}
}

var primaryFile: DocumentURI? {
switch self {
case .macroExpansion(let data): return data.primaryFile
case .generatedInterface(let data): return data.buildSettingsFrom.primaryFile
case .generatedInterface(let data):
if let referenceDocumentURL = try? ReferenceDocumentURL(from: data.buildSettingsFrom) {
return referenceDocumentURL.primaryFile
}
return data.buildSettingsFrom.primaryFile
Copy link
Author

Choose a reason for hiding this comment

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

check https://github.com/swiftlang/sourcekit-lsp/pull/2233/files#r2269006407

}
}
}
Expand Down Expand Up @@ -170,8 +178,4 @@ extension DocumentURI {

package struct ReferenceDocumentURLError: Error, CustomStringConvertible {
package var description: String

init(description: String) {
self.description = description
}
}