Repository navigation
Map CEL offsets through the escapes protocompile records - #4739
Merged
anuraaga merged 3 commits intoOct 23, 2026
Merged
Conversation
stefanvanburen
force-pushed
the
svanburen/cel-escapes-from-tokens
branch
3 times, most recently
from
October 15, 2026 19:41
b5efccf to
20b64ac
Compare
stefanvanburen
marked this pull request as ready for review
October 15, 2026 19:41
The LSP mapped offsets between a CEL expression and the proto string literals it was written as by lexing the literals again, by hand, in `createCELSpan` and `fileByteOffsetToCELOffset`. Each had its own copy of the walk, and both got escapes wrong: - a token's end was taken as its start plus its length in the expression, so a CEL string literal written with escapes, such as `\"a\"` or `'\\n'`, was highlighted and hovered short of its end; - each escape counted as one byte of the expression, while `\u` and `\U` decode to up to four, so every token after one was misplaced: hovering on the second `this` in `this != '\u00e9' && this != ''` showed the `!=` before it; - an offset inside an escape mapped to nothing, so hovering on the `"` of `\"` found no token. `celSourceMap` is built once per expression from the string token protocompile lexed, using the escapes it records with their spans and decoded values, and maps in both directions. It replaces both walks and the escape lexer they shared. `token.StringToken.Escapes` also yields each rune that follows a string's first escape, with an empty span, which the map skips. An escape of a value that is not a valid rune, such as `\uD800`, is reported by the lexer and decoded as U+FFFD, three bytes long, and the map counts it that way. The semantic tokens case and the hover cases after a `\u` or `\U` escape are from bufbuild#4738.
stefanvanburen
force-pushed
the
svanburen/cel-escapes-from-tokens
branch
from
October 15, 2026 19:45
20b64ac to
0d1175f
Compare
anuraaga
reviewed
Oct 16, 2026
| end := i + 2 | ||
| for end < len(s) && end < i+6 && isProtoHexDigit(s[end]) { | ||
| end++ | ||
| // Escapes also yields each rune that follows a string's first escape, with an |
Contributor
There was a problem hiding this comment.
TBH can't understand what this comment is trying to say to even give a suggestion to make it simpler
Contributor
Author
There was a problem hiding this comment.
I think it's confusing because protocompile's Escapes() has an entry for every rune that follows it, escaped or not. Could send protocompile change that makes Escapes record only escape sequences, which seems more correct to me?
`celSourceMap` kept parallel slices of start and end offsets beside the file they were in. `celSourceSpans` is a `[]source.Span` instead, one span per byte of the expression, and is built by walking the escape sequences in order, adding the plain bytes between them, rather than by walking the contents and checking each byte against the next escape.
emcfarlane
approved these changes
Oct 22, 2026
anuraaga
approved these changes
Oct 23, 2026
anuraaga
left a comment
anuraaga
left a comment
Contributor
There was a problem hiding this comment.
Sorry for the delay, thought I had approved this already. Thanks!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The LSP maps offsets between a CEL expression and the proto string literals it is written as, in both directions: to place hover ranges and semantic tokens, and to find the CEL token under the cursor. It did that by lexing the literals again by hand, which got escape sequences wrong in the three ways the commit message lists. protocompile already records each escape's span and decoded value on the string token, so this builds the mapping from that instead and deletes buf's copy of the escape lexer.
flowchart LR cel["cel-go offset range"] -->|"celOffsetRangeToByteRange (#4737)"| expr["CEL expression byte offset"] tok["StringToken and its Escapes"] -->|newCELSourceMap| map["celSourceMap"] expr -->|"celSourceMap.span"| span[".proto file span"] pos[".proto file offset"] -->|"celSourceMap.offset"| exprThe bugs are cosmetic and escapes in CEL expressions are uncommon (2 of the 326 rules in protovalidate's
validate.protouse one), so the main gain is the deleted lexer rather than the fixes.The new cases go in
TestCELHover's table, which gains optionalstartandendfields so that they can check the hover range as well as its content.This supersedes #4738 and carries over its test cases. It also counts a
\uescape of a value that is not a valid rune at the three bytes of the U+FFFD the lexer decodes it to; without that, one such escape left the whole expression without hovers.