Skip to content
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

Migrate to upstream LSP inlay hints on the client side #198

Merged
merged 3 commits into from
Jul 8, 2022
Merged

Migrate to upstream LSP inlay hints on the client side #198

merged 3 commits into from
Jul 8, 2022

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Mar 18, 2022

Official support for inlay hints in LSP will soon make the custom implementation superfluous. For more information, see the accompanying server-side PR (which this PR should be considered blocked on):

This PR removes the client-side implementation of inlay hints as custom text decorations, which will no longer be needed once VSCode's and vscode-languageclient's support for native inlay hints ships in the next release.

Additionally, vscode-languageclient is upgraded to a beta release, so native inlay hints can already be tested in a recent VSCode Insiders build.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@adam-fowler
Copy link
Contributor

adam-fowler commented Mar 18, 2022

Hi @fwcd thanks for this.

I do actually have a separate branch which implements this in a slightly different manner. See PR #199

I must say I don't understand Microsoft's versioning for the language client. It appears they don't have a proper release of 8.0.0 and the next releases are kinda beta releases. But nothing says this explicitly. Given this I would rather wait on a full 8.0.0 release, before moving up.

I see this also relies on changes to sourcekit-lsp so it won't work until there is a new release of Swift.

Eventually your solution is correct and we should just rely on the LanguageClient to provide these. In the meantime it might be better to go for mine.

What are your thoughts?

@fwcd
Copy link
Member Author

fwcd commented Mar 18, 2022

Yeah, that sounds good. I've created this branch this early mostly to have a client for playing around with the new sourcekit-lsp that supports the proposed LSP request. Holding off until Swift and VSCode ship new releases sounds totally reasonable. Once ready, we should definitely also move to a stable vscode-languageclient again.

@adam-fowler
Copy link
Contributor

I see vscode-languageclient v8.0.0 is out and that your pull request for the sourcekit-lsp changes is proceeding. I would like to split this PR into two the upgrade to 8.0.0 and then using the new InlayHint requests.

If it is ok with you I would like to do the v8 upgrade as I would like to see what new stuff it has. I'll leave the InlayHint code to you. Remember we need to still support the old InlayHints method as Swift 5.6 requires it.

@fwcd
Copy link
Member Author

fwcd commented May 24, 2022

That sounds good. The v8 update unfortunately includes a breaking change, the LanguageClient.onReady method is gone, so we have to keep track of this promise manually after starting the client.

Regarding the old inlay hints, we might want to deprecate (and, by default, disable) the sourcekit-lsp.inlayHints.enabled option, since requesting the non-standard hints on a newer sourcekit-lsp (that uses the standard inlay hints) would cause errors I think. Though that might also be better placed in another PR.

I will see if I can slim down this PR to only include the v8 update (+ necessary changes due to API breaks).

@adam-fowler
Copy link
Contributor

adam-fowler commented May 24, 2022

That sounds good. The v8 update unfortunately includes a breaking change, the LanguageClient.onReady method is gone, so we have to keep track of this promise manually after starting the client.

Yeah saw that

Regarding the old inlay hints, we might want to deprecate (and, by default, disable) the sourcekit-lsp.inlayHints.enabled option, since requesting the non-standard hints on a newer sourcekit-lsp (that uses the standard inlay hints) would cause errors I think. Though that might also be better placed in another PR.

We can deprecate it and point users to the standard hint display setting

I will see if I can slim down this PR to only include the v8 update (+ necessary changes due to API breaks).

I'm looking at the v8 update just now. It has broken a couple of things related to restarting the server so I need to resolve these first

@adam-fowler
Copy link
Contributor

The v8 update is done #314, if you want to do the InlayHints changes

@fwcd
Copy link
Member Author

fwcd commented Jun 6, 2022

I have rebased the PR on main, so it should now only contain the inlay hint-related changes.

@adam-fowler
Copy link
Contributor

You still need to support the old inlay hints for people running swift 5.6. You can check if someone is running Swift 5.6 or earlier using

if(workspaceContext.swiftVersion.isLessThan(new Version(5, 7, 0)) {
    ...
}

package.json Outdated Show resolved Hide resolved
@fwcd
Copy link
Member Author

fwcd commented Jun 6, 2022

Good catch, the branch should now only disable inlay hints for Swift >= 5.7. For clarity and to avoid ambiguity with the new LSP types, I have also prefixed the non-standard inlay hint structures with 'legacy'.

@adam-fowler
Copy link
Contributor

@fwcd I currently away on holiday. I'll have a look at this when I get back next Wednesday

@fwcd fwcd marked this pull request as ready for review June 14, 2022 13:35
@adam-fowler
Copy link
Contributor

From what I can see this looks good. I am going to hold off merging it though until there is a 5.7 development build released that includes the new InlayHInt support. Does that sound ok?

@fwcd
Copy link
Member Author

fwcd commented Jun 17, 2022

Sure!

@adam-fowler
Copy link
Contributor

Can you deprecate the sourcekit-lsp.inlayHints.enabled setting and link to the editor.inlayHints.enabled in the deprecation message.

@fwcd
Copy link
Member Author

fwcd commented Jun 19, 2022

Done!

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

Is see the new InlayHint support is now in the swift 5.7 nightlies, so lets merge this

@adam-fowler adam-fowler merged commit 3c5836a into swiftlang:main Jul 8, 2022
@fwcd fwcd deleted the upstream-inlay-hints branch July 8, 2022 11:36
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.

3 participants