Skip to content

SourceKitLSP: generate Swift textual interfaces for module references #668

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

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

bwhiteley
Copy link
Contributor

When doing a jump to definition on a Swift import statement, generate the textual interface for the module so the IDE can load the generated interface file.

@adam-fowler
Copy link
Contributor

@ahoppen do you think this will work with my suggested path to displaying symbols in VSCode. swiftlang/vscode-swift#408

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR. I think the idea of generating the .swiftinterfaces into a temporary directory is a good solution for now. @adam-fowler I have a vague memory that we discussed that LSP/VSCode has/will have a notion of generated/transient/non-disk files but I can’t find it anymore. Do you remember anything about it?

The implementation looks good to me overall, I have some comments inline. Also: Could you add a test case for jump to definition to symbols in other modules and a test case for a standalone open interface request?

@adam-fowler
Copy link
Contributor

@adam-fowler I have a vague memory that we discussed that LSP/VSCode has/will have a notion of generated/transient/non-disk files but I can’t find it anymore. Do you remember anything about it?

That'll be in the issue I linked to swiftlang/vscode-swift#408 (comment)

@ahoppen
Copy link
Member

ahoppen commented Nov 24, 2022

Is there any precedence from language servers that use these kinds of transient documents or an LSP proposal? I think you are the best person to judge how the contents of the interface should be transferred to the editor. The nice thing about the temporary file implementation is that it will work for all editors.

@adam-fowler
Copy link
Contributor

Is there any precedence from language servers that use these kinds of transient documents or an LSP proposal? I think you are the best person to judge how the contents of the interface should be transferred to the editor. The nice thing about the temporary file implementation is that it will work for all editors.

We can go with the temporary file solution. You'll need to be careful to ensure you generate separate versions of these for different swift versions.

@bwhiteley
Copy link
Contributor Author

Thanks for the feedback. I've addressed the issues with the exception wanting to add a couple of more tests.

add a test case for jump to definition to symbols in other modules

Is there an existing test that is similar to this that you could point me to? I've found a couple of candidates, but one of the challenges I am facing is that some of the tests fail for me even in upstream/main.
For example:
image

@ahoppen
Copy link
Member

ahoppen commented Dec 2, 2022

That’s odd that the tests are failing for you. They are passing fine for me locally. Which Xcode version are you using and are you using a Swift Open Source Toolchain? I’m using Xcode 14.0 with the default toolchain.

I think you can use Tests/SourceKitLSPTests/ImplementationTests.swift as a blueprint for your tests. I don’t think you need to check that many locations but the impls(at:) function should give you a good idea of how to invoke an LSP request in the tests.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test cases. I’ve added a few questions inline.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good to me. Let's get this merged. I've got one final comment inline. Could you also squash your commits after you've addressed the comment to create a nicer git history?

@ahoppen
Copy link
Member

ahoppen commented Dec 12, 2022

@swift-ci Please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks a lot for implementing this and addressing all my comments 🙏

This is a great features and I’m sure may people will love it ❤️

@ahoppen
Copy link
Member

ahoppen commented Dec 13, 2022

@swift-ci Please test

@ahoppen ahoppen merged commit c31dff2 into swiftlang:main Dec 13, 2022
@artemcm
Copy link
Contributor

artemcm commented Dec 14, 2022

Not sure how yet, but I think this is related to a failure in swift-driver Windows CI:
swiftlang/swift-driver#1240
https://ci-external.swift.org/job/swift-driver-PR-windows/227/console

C:\Users\swift-ci\jenkins\workspace\swift-driver-PR-windows\sourcekit-lsp\Sources\LanguageServerProtocol\Messages.swift:59:3: error: cannot find 'OpenInterfaceRequest' in scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants