Fix LSP leading comment formatting #3424
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/buf/buflsp/server.go
Outdated
| Range: protocol.Range{ | ||
| // The formatter always returns the entire file; make sure we | ||
| // represent the edit as the entire range (which may not match up to | ||
| // the file's NodeInfo.Start()). | ||
| Start: protocol.Position{ | ||
| Line: 0, | ||
| Character: 0, | ||
| }, | ||
| End: protocol.Position{ | ||
| Line: uint32(nodeInfo.End().Line) - 1, | ||
| Character: uint32(nodeInfo.End().Col) - 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This is still potentially incorrect, due to trailing comments.
Instead, given that parsing is a relatively intense operation, I would prefer that we simply count the number of
lines in file.fileNode and the number of characters on the last line, and construct a Range using that. Don't bother with the spans in the AST, since they clearly don't reflect what we actually mean.
Also, include the following comment:
// XXX: The current compiler does not expose a span for the full file. Instead of
// potentially undershooting the correct span (which can cause comments at the
// start and end of the file to be duplicated), we instead manually count up the
// number of lines in the file. This is comparatively cheap, compared to sending the
// entire file over a domain socket.Also, please file a bug against protocompile to include a way to retrieve a span of the whole file.
There was a problem hiding this comment.
FWIW, it doesn't seem like the issue extends to trailing comments, maybe because NodeInfo.End() has special handling for EOF?
I would prefer that we simply count the number of lines in file.fileNode and the number of characters on the last line, and construct a Range using that.
I was looking around at the FileNode APIs and didn't find much exposed that would help with that; mind pointing me to how that would be done / make that change yourself? (Happy for you to take this over; my protocompile knowledge is weak 😅.)
There was a problem hiding this comment.
I'm juggling a bunch of PRs already and am about to go on vacation, so I prefer you do it if only to make sure it gets done. Also, good excuse to grow your knowledge!
Anyways, the right way to do this:
var lastLine, lastLineStart int
for i := 0; i < len(file.text); i++ {
// NOTE: we are iterating over bytes, not runes.
if file.text[i] == '\n' {
lastLine++
lastLineStart = i + 1 // Skip the \n.
}
}
lastChar := len(file.text[lastLineStart:]) - 1 // Bytes, not runes!There was a problem hiding this comment.
Also, please file a bug against protocompile to include a way to retrieve a span of the whole file.
Fixes #3423.