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.

Sign up for GitHub

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

Jump to bottom

Add server-side support for inlay hints using CollectExpressionType #406

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Add server-side support for inlay hints using CollectExpressionType #406

merged 2 commits into from
Jun 15, 2021

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented May 28, 2021 •
edited
Loading

This is an early-stage draft of the proposed inlay type hints for SourceKit-LSP.

Inlay hints let LSP clients/editors dynamically display inline type annotations, making it easier to read and understand Swift code, without requiring the programmer to spell out every type.

For this purpose, a new, non-standard request is introduced (sourcekit-lsp/inlayHints) that currently requires custom support on the client side. Note that this does not affect compatibility with 'vanilla' LSP clients as inlay hints follow a pull model, i.e. the client has to initiate the request.

Since inlay hints are a proposed LSP feature, however, this implementation strives to conform to the proposed request structures as faithfully as possible to make the migration easy once the feature becomes official. More details can be found here:

Current progress:

  • Add InlayHint and support structures on the SourceKit-LSP side
  • Implement request and handler on the SourceKit-LSP side
  • Add unit tests for the request on the SourceKit-LSP side

Next steps:

  • Implement a new request on the SourceKit-side specifically for inlay hints and use it (instead of the current DocumentSymbols + CollectExpressionType combination)

Once possible:

  • Migrate to the official LSP API
  • Add the corresponding server/client capabilities

See also:

@ahoppen ahoppen self-assigned this Jun 1, 2021
ahoppen
ahoppen reviewed Jun 2, 2021
View reviewed changes
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.

Looking good to me overall so far, I’ve put a few comments inline.

I think you’re also still missing the client and server capabilities for the request and I didn’t see it on your to-do list. Are you planning to do that in the future?

Sources/SourceKitLSP/Clang/ClangLanguageServer.swift Show resolved Hide resolved
Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Show resolved Hide resolved
@fwcd
Copy link
Member Author

fwcd commented Jun 2, 2021

Adding the server/client capabilities is a good point, though I think this would first become relevant as soon as the inlay hints get upstreamed: Clients that aren't currently interested in inlay hints would simply not send the request and those that are interested already have some special logic on the client-side for that.

Once they are added to the official spec though, capabilities would become useful to the client for negotiating with the (from its POV arbitrary) language server whether it supports the request.

I have added it to the todo list.

@fwcd
Copy link
Member Author

fwcd commented Jun 2, 2021

image

I've got an early-stage demo of inlay hints to work, although the results are still very noisy with every expression type being highlighted.

ahoppen
ahoppen reviewed Jun 3, 2021
View reviewed changes
Editors/vscode/src/inlayHints.ts Outdated Show resolved Hide resolved
Editors/vscode/src/inlayHints.ts Outdated Show resolved Hide resolved
@fwcd
Copy link
Member Author

fwcd commented Jun 6, 2021

image

The handler now uses document symbols to figure out the positions of variable bindings and only renders the inlay hints there. While this is already a lot less noisy than previously, there are still some limitations:

  • Inlay hints are rendered after the statement, rather than between the variable name and the = (i.e. in same spot as a regular type annotation)
  • Hints are always rendered, regardless of whether an explicit type annotation is already provided or not

As far as I can tell, the sourcekitd-provided document symbols do not provide any information about whether a type annotation is present, making this a bit challenging. Perhaps it would make sense to introduce a new request on the sourcekitd-side for inlay hints?

@fwcd fwcd changed the title Add support for inlay type hints Add support for inlay type hints using CollectExpressionType Jun 8, 2021
@fwcd fwcd changed the title Add support for inlay type hints using CollectExpressionType Add server-side support for inlay type hints using CollectExpressionType Jun 8, 2021
@fwcd fwcd mentioned this pull request Jun 8, 2021
Merged
@fwcd fwcd changed the title Add server-side support for inlay type hints using CollectExpressionType Add server-side support for inlay hints using CollectExpressionType Jun 8, 2021
ahoppen
ahoppen reviewed Jun 10, 2021
View reviewed changes
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’ve just reviewed the PR as a whole and left some comments inline, mostly small improvement suggestions.

Sources/LanguageServerProtocol/Requests/InlayHintsRequest.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Clang/ClangLanguageServer.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Show resolved Hide resolved
Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift Show resolved Hide resolved
Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift Show resolved Hide resolved
Tests/SourceKitLSPTests/InlayHintsTests.swift Show resolved Hide resolved
Tests/SourceKitLSPTests/InlayHintsTests.swift Show resolved Hide resolved
@fwcd fwcd marked this pull request as ready for review June 10, 2021 14:32
ahoppen
ahoppen approved these changes Jun 10, 2021
View reviewed changes
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.

Looks good to me now. Could you squash the commits? Then I’ll run CI and we can merge this.

@fwcd
Add support for inlay hints using CollectExpressionType
3fef514 
- Add UID for CollectExpressionType request
- Add ExpressionTypeInfo structure
- Add keys to support sourcekitd's CollectExpressionType
- Implement CollectExpressionType request
- Add SwiftLanguageServer.expressionTypeInfos
- Add InlayHint and supporting types
- Add InlayHintsRequest
- Add inlayHints handler stub
- Implement inlay hints request
- Update InlayHint to follow the current proposal
- # This is the commit message swiftlang#11:
- ...as described in the LSP proposal
- Update doc comment on InlayHintsRequest
- Map inlay hints lazily
- Fix minor style issue
- Add new files to CMakeLists.txt
- Specify commit of the current inlay hints proposal state
- Add public, memberwise initializer for InlayHintsRequest
- assert(false) if deserializing ExpressionTypeInfos fails
- Add dispatch precondition to _expressionTypeInfos
- Add InlayHintsRequest to the builtinRequests
- Factor out function for querying document symbols for URI
- Only render inlay hints after variable bindings
- Test inlay hints on empty document
- Test inlay hints for some simple bindings
- Test ranged inlay hint requests
- Make sure that inlay hints are unique per position
- Test inlay hints for fields
- Apply various PR suggestions regarding inlay hints
- Update inlay hint tests and add case with explicit type annotations
- Continue iterating if an ExpressionTypeInfo fails to deserialize
@fwcd
Copy link
Member Author

fwcd commented Jun 10, 2021

Sure, here you go.

@ahoppen
Copy link
Member

ahoppen commented Jun 10, 2021

@swift-ci Please test

benlangmuir
benlangmuir reviewed Jun 10, 2021
View reviewed changes
Copy link
Contributor

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

A few minor things, but otherwise LGTM

Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Swift/ExpressionTypeInfo.swift Outdated Show resolved Hide resolved
Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift Outdated Show resolved Hide resolved
@benlangmuir
Copy link
Contributor

@swift-ci please test

@fwcd fwcd mentioned this pull request Jun 10, 2021
Merged
4 tasks
@ahoppen ahoppen merged commit 7a2d760 into swiftlang:main Jun 15, 2021
@fwcd fwcd deleted the inlay-hints branch June 15, 2021 12:17
@fwcd fwcd mentioned this pull request Mar 8, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benlangmuir benlangmuir benlangmuir approved these changes

@ahoppen ahoppen ahoppen approved these changes

Assignees

@ahoppen ahoppen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fwcd @ahoppen @benlangmuir