Refactor and improve generated LSP code#3299
Conversation
| RegisterOptions: &lsproto.RegisterOptions{ | ||
| DidChangeWatchedFiles: &lsproto.DidChangeWatchedFilesRegistrationOptions{ | ||
| WorkspaceDidChangeWatchedFiles: &lsproto.DidChangeWatchedFilesRegistrationOptions{ |
There was a problem hiding this comment.
Note the improvement here; we no longer need to make sure that the method and the options match. The lib determines the method from the options type itself.
There was a problem hiding this comment.
Pull request overview
Refactors the generated LSP protocol code to reduce or eliminate “double decode” during JSON unmarshalling, while also simplifying/typing registration options and removing unused notebook-related protocol surface.
Changes:
- Reworks generated union unmarshalling to dispatch via
PeekKindand minimal token scanning (discriminant field / required-prop presence) where possible. - Replaces
Registration.method+registerOptionshandling with a typedRegisterOptionsstruct keyed by method; method is derived during marshal/unmarshal. - Filters notebook-related types/requests/notifications/enums from the generated protocol model.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/lsp/server.go | Updates capability registrations to use typed RegisterOptions (method derived automatically). |
| internal/lsp/lsproto/lsp.go | Adds JSON object key scanning helpers used by generated union dispatch; tweaks assertion helpers. |
| internal/lsp/lsproto/_generate/generate.mts | Implements new generation logic for typed registration options + optimized union dispatch + notebook filtering. |
| internal/json/json.go | Exposes NewDecoder wrapper used by lsproto’s lightweight JSON scanners. |
| type requiredProp bool | ||
| // jsonKeyCheck compares a raw JSON key token (including quotes) against a Go string. | ||
| func jsonKeyCheck(name []byte, key string) bool { | ||
| return len(name) == len(key)+2 && name[0] == '"' && string(name[1:len(name)-1]) == key |
There was a problem hiding this comment.
jsonKeyCheck converts the raw key token into a Go string (string(name[1:len-1])), which allocates on every key comparison. Since this function is used in the fast-path union dispatch scanners, it undermines the goal of minimizing overhead; consider doing a zero-allocation byte-by-byte compare against the ASCII key (or reading the key via a token API that returns the unescaped string).
| return len(name) == len(key)+2 && name[0] == '"' && string(name[1:len(name)-1]) == key | |
| // Expect: "key" with no escapes; this matches the previous implementation's assumptions. | |
| if len(name) != len(key)+2 { | |
| return false | |
| } | |
| if name[0] != '"' || name[len(name)-1] != '"' { | |
| return false | |
| } | |
| for i := 0; i < len(key); i++ { | |
| if name[i+1] != key[i] { | |
| return false | |
| } | |
| } | |
| return true |
There was a problem hiding this comment.
Not entirely opposed to the suggestion.
There was a problem hiding this comment.
It's untrue; Go knows to not copy bytes for this kind of expression: https://godbolt.org/z/7jq5nG84q
Note that it's just memequal.
| if err := dec.SkipValue(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This was a bug; we were continuing this loop, so this could technically have broken if the ignored key were the same as a prop!
| switch dec.PeekKind() { | ||
| case '0': | ||
| var v int32 | ||
| if err := json.UnmarshalDecode(dec, &v); err != nil { | ||
| return err | ||
| } | ||
| o.Integer = &v | ||
| return nil | ||
| } | ||
| var vString string | ||
| if err := json.Unmarshal(data, &vString); err == nil { | ||
| o.String = &vString | ||
| case '"': | ||
| var v string | ||
| if err := json.UnmarshalDecode(dec, &v); err != nil { | ||
| return err | ||
| } | ||
| o.String = &v | ||
| return nil | ||
| default: | ||
| return fmt.Errorf("invalid IntegerOrString: expected number, string, got %v", dec.PeekKind()) | ||
| } |
There was a problem hiding this comment.
Here's an example of peeking the kind. This is IntegerOrString, we don't need to try integer first, just ask the decoder for the token kind.
| switch string(jsonObjectRawField(data, "kind")) { | ||
| case `"rename"`: | ||
| var v RenameFile | ||
| if err := json.Unmarshal(data, &v); err != nil { | ||
| return err | ||
| } | ||
| o.RenameFile = &v | ||
| return nil | ||
| } | ||
| var vCreateFile CreateFile | ||
| if err := json.Unmarshal(data, &vCreateFile); err == nil { | ||
| o.CreateFile = &vCreateFile | ||
| case `"create"`: | ||
| var v CreateFile | ||
| if err := json.Unmarshal(data, &v); err != nil { | ||
| return err | ||
| } | ||
| o.CreateFile = &v | ||
| return nil | ||
| } | ||
| var vRenameFile RenameFile | ||
| if err := json.Unmarshal(data, &vRenameFile); err == nil { | ||
| o.RenameFile = &vRenameFile | ||
| case `"delete"`: | ||
| var v DeleteFile | ||
| if err := json.Unmarshal(data, &v); err != nil { | ||
| return err | ||
| } | ||
| o.DeleteFile = &v | ||
| return nil | ||
| } | ||
| var vDeleteFile DeleteFile | ||
| if err := json.Unmarshal(data, &vDeleteFile); err == nil { | ||
| o.DeleteFile = &vDeleteFile | ||
| default: | ||
| var vTextDocumentEdit TextDocumentEdit | ||
| if err := json.Unmarshal(data, &vTextDocumentEdit); err != nil { | ||
| return err | ||
| } | ||
| o.TextDocumentEdit = &vTextDocumentEdit | ||
| return nil | ||
| } |
There was a problem hiding this comment.
For this file event, we can peek ahead looking for kind.
| switch jsonObjectHasKey(data, "insert", "range") { | ||
| case 1: // insert | ||
| var v InsertReplaceEdit | ||
| if err := json.Unmarshal(data, &v); err != nil { | ||
| return err | ||
| } | ||
| o.InsertReplaceEdit = &v | ||
| return nil | ||
| } | ||
| var vInsertReplaceEdit InsertReplaceEdit | ||
| if err := json.Unmarshal(data, &vInsertReplaceEdit); err == nil { | ||
| o.InsertReplaceEdit = &vInsertReplaceEdit | ||
| case 2: // range | ||
| var v TextEdit | ||
| if err := json.Unmarshal(data, &v); err != nil { | ||
| return err | ||
| } | ||
| o.TextEdit = &v | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Here's an example of discriminating by property existence.
…or better asm and perf
| o.Integer = new(int32) | ||
| return json.UnmarshalDecode(dec, o.Integer) | ||
| case '"': | ||
| o.String = new(string) |
There was a problem hiding this comment.
Why do you need the new(string) if you're immediately calling json.UnmarshalDecode onto o.String?
There was a problem hiding this comment.
Is this just in case an error condition takes place?
There was a problem hiding this comment.
It's o.String, not &o.String; we are passing in the pointer we just allocated. Notably the point here is that for say string, we do not want to allow null to be parsed. If we passed in &o.String, we would be passing in **string, which the decoder would happily stick nil into for a null input.
Another omnibus of changes I've wanted to do for a while. Mainly this is focused on eliminating as much "double decode" as possible.
RegisterOptionsis now a typed struct keyed by registration method, withmethodderived automatically during marshal/unmarshal. Before you had to know to write the correct method. (seeserver.go).PeekKindand decode directly without buffering when unambiguous.kindscan just enough to extract the discriminant,The effect of this PR is that we never double decode anything coming from the client! A lot are distinguishable by kind, and otherwise, we can simply tokenize the input to look for a discriminant, bailing early and not actually decoding anything.
After this, 90% of all types don't need to do any extra decoding (maybe 20% need to do a bit of tokenizing), and the only ones that aren't are client -> server so are not critical.
It also turns out that one type in the spec,
TextDocumentContentOptionsOrRegistrationOptions, is a union that cannot be distinguished at all! I'm not attempting to do anything about that in this PR.