-
Notifications
You must be signed in to change notification settings - Fork 322
Add signature help LSP request support #2250
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice 🌟. If you have a screen recording showing the signature help in real life, I would be very keen to see that.
PR looks great to me, I just have a few nitpicky comments.
Sources/DocumentationLanguageService/DocumentationLanguageService.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftLanguageService/AdjustPositionToStartOfArgument.swift
Outdated
Show resolved
Hide resolved
Here's a quick demo of signature help in VS Code. output.mp4There are some things to improve though:
|
b247a5b
to
70e2798
Compare
I've opened a PR on vscode-swift (swiftlang/vscode-swift#1802) to automatically trigger signature help on accepting a function-like completion item in VS Code. |
// LSP 3.18 defines a `noActiveParameterSupport` option which allows the | ||
// active parameter to be `null` causing editors not to show an active | ||
// parameter which would be the best solution here. | ||
parameters.count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that behavior is interesting. The LSP specification says the following. Does this mean that VS Code does not comply with the LSP spec in this regard?
If omitted or the value lies outside the range of
signatures[activeSignature].parameters
defaults to 0 if the active signature has parameters
I’m fine keeping this in but we should mention the intended behavior from the LSP spec in the comment and mention that we are deliberately relying on a VS Code LSP-non-compliance if that’s what we’re doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's VS Code-specific behavior which we use as a workaround here. For editors that are fully LSP-compliant this will have the same behavior as if we didn't return a value and we actually don't have a way of expressing "no active parameter" in that case so it's fine. I actually found rust-analyzer using this trick too.
There are also other VS Code-specific workarounds like setting the active parameter to -1
(microsoft/vscode#145851 (comment)).
I'll update the comment to explain this part too.
The screen recording looks great 🤩 One thought that just crossed my mind to extract the documentation for the parameters: SourceKit-LSP does link against docc, so we should be able to use docc to parse the doc comment into a symbol graph and then extract the parameters from there. In general, getting docc into the loop here might be open up new possibilities such as #2256. If you want to tackle this, I would make that a separate PR though. |
… in signature help
@ahoppen Thanks for the suggestion, I'll look into it more as I'm not familiar with DocC. I've updated the comments you mentioned as well, can you please recheck? 🙏🏼 |
There was a problem hiding this 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. Thanks for adding the explanation.
Depends on swiftlang/swift#83378
Adds support for the LSP signature help request.
Note
As of swiftlang/swift#83378, SourceKitD still doesn't separate parameter documentation from the signature documentation and thus parameters don't have their own separate documentation. This should just work once SourceKitD implements this functionality and we'll only need to modify the tests.