lsp: don't null-pointer-deref panic when TextDocumentCompletion.Context missing #39
Conversation
…ssing It's not compulsory. Clients don't have to send it if they don't support it yet. This solution just defaults the trigger character to [
cormacrelf
force-pushed
the
optional-context
branch
from
05efb28
to
9df2cb2
Compare
cormacrelf
changed the title
| triggerChar := params.Context.TriggerCharacter | ||
| if params.Context.TriggerKind != protocol.CompletionTriggerKindTriggerCharacter || triggerChar == nil { | ||
| return nil, nil | ||
| triggerChar := "[" |
There was a problem hiding this comment.
The auto-completion is also used for tags, so I'm afraid that defaulting to [ would make it unreachable. Maybe we could just take a look at the previously entered characters (like here) and ignore the trigger character in this case?
I didn't encounter an editor which doesn't support the context and trigger characters yet. How is the auto-completion triggered in this case, manually with C-Space? Which editor / LSP plugin are you using?
There was a problem hiding this comment.
Neovim's built in LSP doesn't do it yet. Moreover the spec says it's optional, so you do have to check the nil. I think it is perhaps a low priority feature, because generally LSPs are accepting file content as well as completion requests, and have access to a stored copy of the file. So nobody actually needs to know the trigger character. Not even you! I'll fix with a look behind instead.
There was a problem hiding this comment.
It's also optional because completion gets triggered in many other ways than typing a character.
There was a problem hiding this comment.
Yeah in the end this context is not super useful then. Thanks for addressing this!
|
Superseded by https://github.com/mickael-menu/zk/pull/41 @cormacrelf I took over this PR in another branch (as I didn't have write access on yours) but credited you in the changelog. Thanks again for spotting this 🙏 |
No description provided.