Use null instead of empty signature helps in LSP#52126
Conversation
- The LSP specification requires that `signatures` be provided. Currently Roslyn breaks this LSP contract by returning a new signature help item. It should be returning `null`
dibarbet
left a comment
There was a problem hiding this comment.
👍 thanks for the change, just needs nullable annotations update
| var document = context.Document; | ||
| if (document == null) | ||
| { | ||
| return new LSP.SignatureHelp(); |
There was a problem hiding this comment.
the nullable annotations need to be updated for the method signature + implementing signature, also in the entry point here - https://github.com/dotnet/roslyn/blob/main/src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs#L426
You mean I can't just open up the GitHUb editor and submit PRs like this? 😆 |
|
|
||
| [ExportLspRequestHandler(LiveShareConstants.TypeScriptContractName, Methods.TextDocumentSignatureHelpName)] | ||
| internal class TypeScriptSignatureHelpHandlerShim : SignatureHelpHandler, ILspRequestHandler<TextDocumentPositionParams, SignatureHelp, Solution> | ||
| internal class TypeScriptSignatureHelpHandlerShim : SignatureHelpHandler, ILspRequestHandler<TextDocumentPositionParams, SignatureHelp?, Solution> |
There was a problem hiding this comment.
Oh yeah forgot about these. Are these dead yet @uniqueiniquity ? Should we just remove them
There was a problem hiding this comment.
Sorry, was out of the office for a few days.
Signature help definitely is, though I think others aren't (e.g. navbar stuff).
signaturesbe provided. Currently Roslyn breaks this LSP contract by returning a new signature help item. It should be returningnull