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 request #465

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Mar 8, 2022

Inlay hints have been officially added to the VSCode API and are in the process of being officially added to the Language Server Protocol (LSP). As proposed in #406, this PR performs the final step of using the official LSP request for inlay hints (textDocument/inlayHint) instead of our current, non-standard request (sourcekit-lsp/inlayHints).

To do:

  • Rename to official LSP request name (textDocument/inlayHint)
  • Add client and server capabilities
  • Add support for dynamic registration of inlay hints
  • Adopt changes to the request structures
    • Rename InlayHintCategory to InlayHintKind and represent it using an integer
    • Add InlayHintPart and the other parameters to InlayHint (see here)
  • Figure out how to make the inlay hints pretty (i.e. whether we need padding, the colon indicating that it's a type annotation etc.)
  • Add a TextEdit that 'commits' the inlay hint as a type annotation to the the document (this will be possible once the new InlayHint structure is implemented fully)
  • Test sourcekit-lsp with a recent version of vscode-languageclient and VSCode (see the PR in vscode-swift)

See also:

cc @ahoppen

@ahoppen
Copy link
Member

ahoppen commented Mar 14, 2022

Thanks for continuing to work on this @fwcd. I would like to hold this PR back until LSP 3.17 has been finalized just so we don’t miss any last-minute changes to the official protocol.

@fwcd
Copy link
Member Author

fwcd commented Mar 18, 2022

Here is a small demo using the latest VSCode Insiders and vscode-languageclient showcasing LSP-native inlay hints (textDocument/inlayHint) in action:

image

This of course still needs some tweaking to get the styling right, but the basic machinery already works.

@ahoppen
Copy link
Member

ahoppen commented Mar 21, 2022

Nice, that looks really cool. Do you know if we can adjust the styling of the inlay type hints? In particular, use : before the type name?

@fwcd
Copy link
Member Author

fwcd commented Mar 24, 2022

Yes, we should do that server-side now (which requires updating the tests too, previously the client implementing our non-standard inlay hints automatically appended the : ).

An interesting discussion regarding the styling can be found here:

@fwcd fwcd force-pushed the upstream-inlay-hints branch from 57337da to 8945c2c Compare May 21, 2022 02:10
@fwcd fwcd marked this pull request as ready for review May 21, 2022 14:25
@fwcd fwcd requested review from benlangmuir and ahoppen as code owners May 21, 2022 14:25
@fwcd
Copy link
Member Author

fwcd commented May 21, 2022

The inlay hints have been officially added to LSP 3.17, VSCode and vscode-languageclient, therefore this PR should be ready for review. I have added the remaining parts of the InlayHint-related structures from the spec and attached a TextEdit to each hint. This lets users commit the annotation to the document e.g. through double-click in VSCode:

inlay-hints-demo.mov

(The video demo uses the new VSCode feature that keeps inlay hints only visible while ctrl+alt is pressed)

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.

I think you missed one data: LSPAny field from the LSP spec, otherwise LGTM 👍

label: info.printedType
.map { info -> InlayHint in
let position = info.range.upperBound
let label = ": \(info.printedType)"
Copy link
Member

Choose a reason for hiding this comment

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

It would be really nice if we could use InlayHintLabelParts here and make each label part clickable and to jump to that type’s definition. But that’s definitely out-of-scope for this PR and would probably also require changes to sourcekitd.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, that would be a cool feature for a future PR.

@ahoppen
Copy link
Member

ahoppen commented May 24, 2022

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented May 24, 2022

Would you mind squashing the commits?

- Use official textDocument/inlayHint request
- Rename InlayHintCategory to InlayHintKind
- Additionally, represent it using an Int, as in the proposed LSP API.
- Add inlay hint client capabilities
- Add inlay hint server capabilities
- Add dynamic registration of inlay hint request
- Rename InlayHintsRequest -> InlayHintRequest
  This is to be consistent with the request itself being named in singular
  in LSP and the other requests (e.g. DocumentSymbolRequest).
- Forward inlay hint requests to clangd
- Add colon before inlay hints
- Add other properties to InlayHint
- Add InlayHintLabel structures
- Conform InlayHintLabel to ExpressibleByStringX protocols
- Attach TextEdit to inlay hints for committing them
- Add InlayHint.data
- Fix InlayHintTests
  We need to include text edits in the expected inlay hints.
@fwcd fwcd force-pushed the upstream-inlay-hints branch from 670a696 to 818c44d Compare May 24, 2022 14:52
@fwcd
Copy link
Member Author

fwcd commented May 24, 2022

Done!

@ahoppen
Copy link
Member

ahoppen commented May 24, 2022

@swift-ci Please test

@ahoppen ahoppen self-assigned this May 24, 2022
@ahoppen ahoppen merged commit 44990b6 into swiftlang:main Jun 14, 2022
@fwcd fwcd deleted the upstream-inlay-hints branch June 14, 2022 13:14
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.

2 participants