Implement breaking lints in the LSP #3404
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/buf/buflsp/file.go
Outdated
| const ( | ||
| againstTrunk againstKind = iota + 1 | ||
| againstHead | ||
| againstSaved | ||
| ) | ||
|
|
||
| const descriptorPath = "google/protobuf/descriptor.proto" |
There was a problem hiding this comment.
Style nit: I would group the enum value declarations alongside the type, something like:
const descriptorPath = "google/protobuf/descriptor.proto"
const (
againstTrunk againstKind = iota + 1
againstHead
againstSaved
)
type againstKind int
According to our style guide, we should group related declarations, so swapping the order of the two const blocks makes sense to me.
Also we should have some docs around what each one is.
private/buf/buflsp/buflsp.go
Outdated
| l.lock.Lock() | ||
| defer l.lock.Unlock() | ||
| return actual(ctx, replier, req) | ||
| return func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) error { |
There was a problem hiding this comment.
Discussed in DMs, but we may be able to use jsonrpc2.AsyncHandler for this https://pkg.go.dev/go.lsp.dev/jsonrpc2#AsyncHandler
There was a problem hiding this comment.
The semantics are not precisely the same, but it's close enough!
private/buf/buflsp/file.go
Outdated
| if errors.Is(err, git.ErrRemoteNotFound) { | ||
| // This is an invalid remote, so it means gitRemote should be | ||
| // interpreted as a local branch name instead. | ||
| f.gitRemoteBranch = f.gitRemote | ||
| f.gitRemote = "" | ||
| f.lsp.logger.Debug( | ||
| "found local branch", | ||
| slog.String("uri", string(f.uri)), | ||
| slog.String("ref", f.gitRemoteBranch), | ||
| ) | ||
| } else if err != nil { |
There was a problem hiding this comment.
This behaviour is somewhat reasonable, but I wonder if branch + remote should just be different config fields with sane defaults for clarity. Even if this is documented behaviour, the config field being named gitRemote but pointing to a local branch might cause some confusion. This would have cascading changes through the fileOpener, etc.
There was a problem hiding this comment.
I think I'm going to change the semantics as follows:
- againstKind will be
againstGitoragainstSaved. - There will be a single config knob for the ref to use for
againstGit, with a default oforigin/HEAD.
private/buf/buflsp/file.go
Outdated
| if data == nil || errors.Is(err, git.ErrNotARepo) { | ||
| return os.Open(fileInfo.LocalPath()) |
There was a problem hiding this comment.
For this case, should we check f.IsLocal()?
There was a problem hiding this comment.
No: the local path is the path on disk, which is the file we need to open for e.g. things that are in cache.
private/buf/buflsp/file.go
Outdated
| // BuildImage build Buf Images for this file, to be used with linting | ||
| // routines. | ||
| // | ||
| // This operation requires IndexImports(). |
There was a problem hiding this comment.
Nit: %s/BuildImage/BuildImages
private/pkg/git/git.go
Outdated
| for { | ||
| _, err := os.Stat(filepath.Join(dir, ".git")) | ||
| if err == nil { | ||
| break | ||
| } else if errors.Is(err, os.ErrNotExist) { | ||
| parent := filepath.Dir(dir) | ||
| if parent == dir { | ||
| return nil, fmt.Errorf("could not find .git directory for %s: %w", orig, ErrNotARepo) | ||
| } | ||
| dir = parent | ||
| } else { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
We could use CheckDirectoryIsValidGitCheckout to check if this is a git repository:
Line 215 in b886118
The call-site will need to handle ErrInvalidGitCheckout instead.
There was a problem hiding this comment.
That's not quite what we want. We want to get the repository root, which requires traversing upwards (vs IsValidCheckout, which just kicks rev-parse).
This change adds breaking lints to the LSP as part of the general lint pass.
Breaking lints are interesting because they need an "against". I've chosen to expose two configuration knobs (which are set in the settings for an LSP in the standard way):
buf.againstsets the against strategy. For the LSP, there are three that reasonably make sense: against some remote's HEAD, against your local HEAD, and against whatever is on disk. This means that lints go away when pushing to main, committing working changes, or saving in the editor, respectively.buf.gitRemote, the remote to use for the above. "origin" is the default for almost everyone, but it need not be that, so it's a config knob.Also, in the process of standing up loading config settings from the client, I found a deadlock in the
jsonrpc2library. To work around it, every request runs on its own goroutine (although they are still serialized wrt to the big LSP lock). This ensures that the single goroutinejsonrpc2spawns to actually poll the IPC socket doesn't deadlock. Why do they only spawn one goroutine? Presumably because they expect users to spawn a goroutine to handle each request, but this does not appear to be visibly documented. (Much debugging of deadlocked goroutines happened today.)