Swiftinterface symbol lookup #747

adam-fowler · 2023-05-16T19:02:11Z

On calling definition if a symbol returns it is in a file with a .swiftinterface extension it will use OpenInterface to create the generated interface file. It will then call sourcekitd source.request.editor.find_usr to find the location of the symbol in the generated file.

OpenInterfaceRequest has been extended to include an optional symbol name.
Broke out openInterface code in SourceKitServer.definition into a separate function: respondWithInterface, as it is called from multiple sites now
If first occurrence of symbol definition occurs inside a swiftInterface file then use respondWithInterface to build generated interface and then find symbol location in generated interface.
Use DocumentManager to store instances of generated interface files, to avoid generating them on every call to openInterface and also cache the line table.
Added _findUSR function that calls source.request.editor.find_usr

When a symbol definition returns it is in a swiftinterface file, create textual version of swiftinterface and return that in response.

adam-fowler · 2023-05-16T19:04:23Z

Sources/SourceKitLSP/Swift/OpenInterface.swift

+    skreq[keys.usr] = symbol
+
+    if let compileCommand = self.commandsByFile[uri] {
+      skreq[keys.compilerargs] = compileCommand.compilerArgs


Wasn't sure if this was necessary

Compiler args are not used for the find_usr request, so there’s no need to specify them

/~https://github.com/apple/swift/blob/31032b9cae300878312dc21ddbd515c744553db6/tools/SourceKit/tools/sourcekitd/lib/Service/Requests.cpp#L3706-L3723

Sources/SourceKitLSP/SourceKitServer.swift

adam-fowler · 2023-05-16T19:11:17Z

Sources/SourceKitLSP/SourceKitServer.swift

+        // textual interface 
+        if let firstResolved = resolved.first, 
+           let moduleName = firstResolved.occurrence?.location.moduleName, 
+           firstResolved.location.uri.fileURL?.pathExtension == "swiftinterface" {


Is checking the extension of location uri enough here?

bnbarham · 2023-05-19T07:01:43Z

I thought I might give some extra background here.

The cursor info request returns the group name (which is where the module splitting is coming from) as well as whether it's "dynamic", ie. whether it could be overridden. In the dynamic case we will also provide the "receiver" USRs which are the statically known types of the member that's being called.

If the returned symbol isn't dynamic, then there's no need to do an index lookup - we can jump straight to the location provided by the result (*). In the case that location is a header we can make a choice:

Jump to the header
Do an index lookup for the definition and jump to that (if we have the definition)
Jump to the generated interface for that module
Provide the user a choice for one of these

If it is dynamic, we should always do an index lookup - looking up and down the hierarchy from the given receiver types. This is basically to avoid going off into a separate subtree, eg. if we had D: C, C: B, B: A, E: B and a call on C.foo() we wouldn't want to provide E.foo() as a result, but we would want to give D.foo() since it's possible that's the actual call.

(*) This result wouldn't be any more/less up to date than the index since we only index-while-building. Though if we instead had background indexing, it might be better to consider the index result all the time. We could also background build though so... 🤷The dynamic/receiver USR data is also in the index.

With that said...

Is checking the extension of location uri enough here?

Depends on what we actually want to do regarding the header case above. Another case to consider is what happens when library evolution isn't enabled, though I'm not sure we really want to to support that in general. I believe (but haven't checked) that we don't return a path at all in that case, but ideally we'd still jump to the generated interface. The index wouldn't contain any results from such modules anyway.

If this is just about providing the generated interface for swift modules, then this check works well enough.

Is there a better way to do this? Are there other modules that might be split up like this?

Technically any library could provide this via -group-info-path, though it really only matters for when there's no source. The index records group names as if they were submodules for the stdlib, but for all other modules that's not the case. So the only accurate place to get this from is the cursor info request.

But considering only the stdlib is likely good enough here. In that case passing the group name to the generated interface request would also be a good thing to do. Splitting works if we can't get it from the cursor info request.

Sources/SourceKitLSP/SourceKitServer.swift

ahoppen · 2023-05-19T17:04:49Z

Sources/SourceKitLSP/Swift/OpenInterface.swift

+        self._findUSR(request: request, uri: interfaceDocURI, snapshot: snapshot, symbol: symbol) { result in
+          switch result {
+            case .success(let info):
+              request.reply(.success(InterfaceDetails(uri: interfaceDocURI, position: info.position)))
+            case .failure:
+              request.reply(.success(InterfaceDetails(uri: interfaceDocURI, position: nil)))
+          }
+        }


I wonder whether we can share this code between the two if branches

ahoppen · 2023-05-19T17:11:31Z

Sources/SourceKitLSP/Swift/OpenInterface.swift

+    skreq[keys.usr] = symbol
+
+    if let compileCommand = self.commandsByFile[uri] {
+      skreq[keys.compilerargs] = compileCommand.compilerArgs


Compiler args are not used for the find_usr request, so there’s no need to specify them

/~https://github.com/apple/swift/blob/31032b9cae300878312dc21ddbd515c744553db6/tools/SourceKit/tools/sourcekitd/lib/Service/Requests.cpp#L3706-L3723

ahoppen · 2023-05-19T17:11:57Z

Sources/SourceKitLSP/Swift/OpenInterface.swift

+          if let offset: Int = dict[keys.offset],
+            let position = snapshot.positionOf(utf8Offset: offset) {
+              return completion(.success(FindUSRInfo(position: position)))
+          } else {
+            return completion(.success(FindUSRInfo(position: nil)))
+          } 


Should be indented by 2 spaces, I think.

Do you use one of the swift formats to format the code?

No, SourceKit-LSP is manual 2 space indentation

Use group names when running open interface request

Sources/LanguageServerProtocol/Requests/OpenInterfaceRequest.swift

Tests/SourceKitLSPTests/SwiftInterfaceTests.swift

rename symbol to symbolUSR Cleanup OpenInterfaceRequest.init

Use SwiftPMPackage test module Added version of buildAndIndex that includes system symbols

ahoppen

Thank you so much for this

ahoppen · 2023-05-22T23:21:42Z

@swift-ci Please test

Add multiple tests for various system modules

adam-fowler · 2023-05-23T13:23:14Z

@ahoppen Hopefully fixed the tests. Had to create a new test SwiftPM project for the swift interface tests as my edits of an existing one broke another test. Also added tests for symbols from multiple libs, with submodules and not.

ahoppen

Thanks

ahoppen · 2023-05-23T18:28:25Z

@swift-ci Please test

ahoppen · 2023-05-23T18:28:37Z

@swift-ci Please test Windows

ahoppen · 2023-05-23T22:40:45Z

@adam-fowler Could you also create a cherry-pick PR for release/5.9? I think it would be worth to include this improvement in the 5.9 release.

adam-fowler · 2023-05-24T12:45:43Z

ok will do

adam-fowler added 3 commits May 16, 2023 19:32

Generate swiftinterface for symbol lookup

cf702e2

When a symbol definition returns it is in a swiftinterface file, create textual version of swiftinterface and return that in response.

Extend OpenInterface to also seatch for a symbol

45adabb

Fix warning

1aa6fe4

adam-fowler requested review from benlangmuir and ahoppen as code owners May 16, 2023 19:02

adam-fowler commented May 16, 2023

View reviewed changes

adam-fowler mentioned this pull request May 16, 2023

SwiftInterface textual interface generation fails #744

Closed

adam-fowler commented May 16, 2023

View reviewed changes

Sources/SourceKitLSP/SourceKitServer.swift Outdated Show resolved Hide resolved

adam-fowler commented May 16, 2023

View reviewed changes

ahoppen reviewed May 19, 2023

View reviewed changes

adam-fowler added 2 commits May 20, 2023 16:32

Syntax changes after review

bbc4eee

Move module name split into OpenInterfaceRequest

0da1d40

Use group names when running open interface request

ahoppen reviewed May 22, 2023

View reviewed changes

adam-fowler added 4 commits May 22, 2023 22:37

Requested changes from PR

07b9cc2

rename symbol to symbolUSR Cleanup OpenInterfaceRequest.init

Fix tests

2a06084

Added testDefinitionInSystemModuleInterface

fcdf0d4

Use SwiftPMPackage test module Added version of buildAndIndex that includes system symbols

Merge buildAndIndexWithSystemSymbols with buildAndIndex

cc40831

ahoppen approved these changes May 22, 2023

View reviewed changes

Added specific test project for system swiftinterface tests

07de4c4

Add multiple tests for various system modules

ahoppen approved these changes May 23, 2023

View reviewed changes

ahoppen merged commit ff59c1a into swiftlang:main May 23, 2023

This was referenced May 24, 2023

Swiftinterface symbol lookup (release/5.9) #750

Merged

Investigate generating module interface from .swiftinterface and .swiftdoc files swiftlang/vscode-swift#408

Closed

jkl0619 mentioned this pull request Aug 20, 2024

Sourcekit-lsp go-to-def not working for modules built from diff toolchain version #1626

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swiftinterface symbol lookup #747

Swiftinterface symbol lookup #747

adam-fowler commented May 16, 2023

adam-fowler May 16, 2023

ahoppen May 19, 2023

adam-fowler May 16, 2023

bnbarham commented May 19, 2023 •

edited

Loading

ahoppen May 19, 2023

ahoppen May 19, 2023

ahoppen May 19, 2023

adam-fowler May 20, 2023

ahoppen May 20, 2023

ahoppen left a comment

ahoppen commented May 22, 2023

adam-fowler commented May 23, 2023

ahoppen left a comment

ahoppen commented May 23, 2023

ahoppen commented May 23, 2023

ahoppen commented May 23, 2023

adam-fowler commented May 24, 2023

Swiftinterface symbol lookup #747

Swiftinterface symbol lookup #747

Conversation

adam-fowler commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham commented May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen commented May 22, 2023

adam-fowler commented May 23, 2023

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen commented May 23, 2023

ahoppen commented May 23, 2023

ahoppen commented May 23, 2023

adam-fowler commented May 24, 2023

bnbarham commented May 19, 2023 •

edited

Loading