Add LSP completion for field numbers#4260
Conversation
Suggests the next available field number, taking into account existing
field numbers in the message as well as reserved field numbers. We want
this so that people get suggested the next lowest field number, from
[protobuf.com][1]:
> When creating messages, authors should prefer lower numbers:
> unmarshaling a message is typically optimized for smaller field
> numbers. Also, smaller values are more efficiently encoded and decoded:
> field numbers 1 to 15 can be encoded in a single byte. So it is
> recommended to always start numbering fields at 1 and to use smaller
> values for more commonly used fields when possible.
Should work with or without the semicolon after, e.g.:
```proto
message ABC {
string xyz = |
}
```
```proto
message ABC {
string xyz = |;
}
```
[1]: https://protobuf.com/docs/language-spec#field-numbers
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/buf/buflsp/completion.go
Outdated
| } | ||
|
|
||
| // Check if the first non-whitespace token is an equals sign | ||
| return !before.IsZero() && before.Kind() == token.Keyword && before.Text() == "=" |
There was a problem hiding this comment.
Can check this against keyword.Assign.
There was a problem hiding this comment.
36b9ae8. Probably don't even need to check if the token is a keyword?
| } | ||
|
|
||
| func completionItemsForFieldNumber( | ||
| parentDef ast.DeclDef, |
There was a problem hiding this comment.
So, I know that completions are based off the AST, and this has to do with resolving arbitrary and potentially invalid locations in the token stream, but given that we have the parentDef here, an alternative approach we can take is resolving the parentDef's symbol, which would give us access to the ir.Symbol. And since the parentDef is asserted to be a valid message, the ir.Symbol should always be a ir.Type, which has a nice API for accessing the reserved ranges, without us re-doing the parsing here.
There might be a catch with this approach, but maybe it's worth looking into?
There was a problem hiding this comment.
7d7ca07 - there might be a simpler way to resolve the parentDef's symbol, though?
There was a problem hiding this comment.
looks like maybe not, so just consolidated the callsites into the same helper: c383ffe
There was a problem hiding this comment.
^ honestly, this is pretty much exactly what I had in mind, lol. it's a little ugly because you need to pass the file around, but i feel like it's not the worst? given that it's always going to be scoped to the current file, it's pretty safe?
doriable
left a comment
doriable
left a comment
There was a problem hiding this comment.
Looks good, I pulled in main and tested locally. Going to pre-approve, but do you mind putting the feature into the changelog?
Added in e3776f5; merging, but feel free to wordsmith separately! |
stefanvanburen
changed the title
Similar to how this works for messages, but with an enum-specific flair; enum values must be an int32 (including negative values). Also simplifies the CompletionItem to only include a Label; we don't need to specify `InsertText` if it's the same value. (We mentioned this feature specifically in [the LSP blog post][1] and I realized I had totally forgotten about doing it alongside message field completion in #4260.) [1]: https://buf.build/blog/protobuf-lsp
Similar to how this works for messages, but with an enum-specific flair; enum values must be an int32 (including negative values). Also simplifies the CompletionItem to only include a Label; we don't need to specify `InsertText` if it's the same value. (We mentioned this feature specifically in [the LSP blog post][1] and I realized I had totally forgotten about doing it alongside message field completion in #4260.) [1]: https://buf.build/blog/protobuf-lsp
3 participants
Suggests the next available field number, taking into account existing field numbers in the message as well as reserved field numbers. We want this so that people get suggested the next lowest field number, from protobuf.com:
Should work with or without the semicolon after, e.g.: