-
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 2 commits
4058ac6
dd7de19
62334c0
d0e544e
f509987
e147601
67e5f17
82cc691
d143d8d
8dfcbfa
c9a943e
b372182
05d8d02
6f4ee70
36a5f83
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 |
---|---|---|
|
@@ -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 | ||
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. Similar to my other comment about the 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 but please check the tests about |
||
case "md": self = .markdown | ||
case "tutorial": self = .tutorial | ||
default: return nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -553,7 +553,7 @@ package actor SourceKitLSPServer { | |
} | ||
|
||
let toolchain = await workspace.buildServerManager.toolchain( | ||
for: uri, | ||
for: uri.buildSettingsFile, | ||
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. Is this change necessary? I just checked and it looks like |
||
in: workspace.buildServerManager.canonicalTarget(for: uri), | ||
language: language | ||
) | ||
|
@@ -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,36 @@ 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 let referenceDoc = try? ReferenceDocumentURL(from: originatorUri), | ||
case .generatedInterface(let interfaceData) = referenceDoc, | ||
aelam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.primaryFile ?? originatorUri | ||
|
||
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 |
---|---|---|
|
@@ -92,7 +92,13 @@ actor GeneratedInterfaceManager { | |
incrementingRefCount: Bool | ||
) async throws -> OpenGeneratedInterfaceDocumentDetails { | ||
func loadFromCache() -> OpenGeneratedInterfaceDocumentDetails? { | ||
guard let cachedIndex = openInterfaces.firstIndex(where: { $0.url == document }) else { | ||
// Cache by module name and group name, not the full document data | ||
// This allows reuse across different buildSettingsFrom URIs | ||
guard let cachedIndex = openInterfaces.firstIndex(where: { | ||
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 don’t think this is correct. Depending on the build settings used to retrieve the generated interface, the interface might look differently. I can’t come up with a great real-world example right now where the contents differ but a canonical example would be that we can’t load the generated interface if we are missing an include path to one of the module’s dependencies but we can generate the interface if all build settings are present. But if we continue passing along the same 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. you are right but about same |
||
$0.url.moduleName == document.moduleName && | ||
$0.url.groupName == document.groupName && | ||
$0.url.sourcekitdDocumentName == document.sourcekitdDocumentName | ||
}) else { | ||
return nil | ||
} | ||
if incrementingRefCount { | ||
|
@@ -160,7 +166,11 @@ actor GeneratedInterfaceManager { | |
} | ||
|
||
private func decrementRefCount(for document: GeneratedInterfaceDocumentURLData) { | ||
guard let cachedIndex = openInterfaces.firstIndex(where: { $0.url == document }) else { | ||
guard let cachedIndex = openInterfaces.firstIndex(where: { | ||
$0.url.moduleName == document.moduleName && | ||
$0.url.groupName == document.groupName && | ||
$0.url.sourcekitdDocumentName == document.sourcekitdDocumentName | ||
}) else { | ||
logger.fault( | ||
"Generated interface document for \(document.moduleName) is not open anymore. Unbalanced retain and releases?" | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,18 @@ extension SwiftLanguageService { | |
groupName: String?, | ||
symbolUSR symbol: String? | ||
) async throws -> GeneratedInterfaceDetails? { | ||
// Generate a deterministic document name based on module name and group name | ||
// This ensures we reuse the same interface for the same module/group combination | ||
let documentName = if let 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. Similar to my comment above, generated interfaces might look different depending on the build settings that were used to generate them, so module name and group name don’t uniquely identify its contents. That’s why we used the UUID-based filename below. |
||
"\(moduleName)-\(groupName.replacingOccurrences(of: "/", with: "-"))" | ||
} else { | ||
moduleName | ||
} | ||
|
||
let urlData = GeneratedInterfaceDocumentURLData( | ||
moduleName: moduleName, | ||
groupName: groupName, | ||
sourcekitdDocumentName: "\(moduleName)-\(UUID())", | ||
sourcekitdDocumentName: documentName, | ||
primaryFile: document | ||
) | ||
let position: Position? = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -165,13 +165,14 @@ extension DocumentURI { | |
return referenceDocument.buildSettingsFile | ||
} | ||
return self | ||
} /// Convert sourcekit-lsp:// URIs to actual file system paths when possible. | ||
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. Missing newlines before |
||
/// For generated Swift interfaces, this returns the path where the interface file | ||
/// would be stored in the GeneratedInterfaces directory. | ||
var actualFileSystemPath: DocumentURI { | ||
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 don’t think this is used anywhere, or am I missing something? |
||
return self | ||
} | ||
} | ||
|
||
package struct ReferenceDocumentURLError: Error, CustomStringConvertible { | ||
package var description: String | ||
|
||
init(description: String) { | ||
self.description = description | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.