Skip to content

Commit 14cc304

Browse files
emcfarlaneemcfarlane
authored
LSP checks run async (#4120)
1 parent 391cb0c commit 14cc304

File tree

2 files changed

+113
-111
lines changed

2 files changed

+113
-111
lines changed

‎private/buf/buflsp/file.go‎

Lines changed: 93 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,16 @@ import (
2121
"errors"
2222
"fmt"
2323
"io"
24-
"io/fs"
2524
"iter"
2625
"log/slog"
2726
"slices"
2827
"strings"
28+
"time"
2929

3030
"buf.build/go/standard/xlog/xslog"
3131
"buf.build/go/standard/xslices"
3232
"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
3333
"github.com/bufbuild/buf/private/bufpkg/bufcheck"
34-
"github.com/bufbuild/buf/private/bufpkg/bufimage"
3534
"github.com/bufbuild/buf/private/bufpkg/bufmodule"
3635
"github.com/bufbuild/buf/private/pkg/storage"
3736
"github.com/bufbuild/protocompile/experimental/ast/predeclared"
@@ -66,7 +65,7 @@ type file struct {
6665
referenceSymbols []*symbol
6766
symbols []*symbol
6867
diagnostics []protocol.Diagnostic
69-
image bufimage.Image
68+
cancelChecks func()
7069
}
7170

7271
// IsLocal returns whether this is a local file, i.e. a file that the editor
@@ -92,12 +91,11 @@ func (f *file) Manager() *fileManager {
9291

9392
// Reset clears all bookkeeping information on this file.
9493
func (f *file) Reset(ctx context.Context) {
95-
f.lsp.logger.Debug(fmt.Sprintf("resetting file %v", f.uri))
94+
f.lsp.logger.DebugContext(ctx, "resetting file", slog.String("uri", f.uri.Filename()))
9695

9796
f.ir = ir.File{}
9897
f.diagnostics = nil
9998
f.symbols = nil
100-
f.image = nil
10199
}
102100

103101
// Close marks a file as closed.
@@ -162,8 +160,9 @@ func (f *file) ReadFromWorkspace(ctx context.Context) (err error) {
162160
// the LSP client.
163161
func (f *file) Update(ctx context.Context, version int32, text string) {
164162
f.Reset(ctx)
163+
f.CancelChecks(ctx)
165164

166-
f.lsp.logger.Info(fmt.Sprintf("new file version: %v, %v -> %v", f.uri, f.version, version))
165+
f.lsp.logger.InfoContext(ctx, "file updated", slog.String("uri", f.uri.Filename()), slog.Int("old_version", int(f.version)), slog.Int("new_version", int(version)))
167166
f.version = version
168167
f.file = report.NewFile(f.uri.Filename(), text)
169168
f.hasText = true
@@ -189,10 +188,7 @@ func (f *file) Refresh(ctx context.Context) {
189188
f.IndexSymbols(ctx)
190189

191190
progress.Report(ctx, "Running Checks", 4.0/4)
192-
if f.IsOpenInEditor() {
193-
f.BuildImage(ctx)
194-
f.RunLints(ctx)
195-
}
191+
f.RunChecks(ctx)
196192

197193
progress.Done(ctx)
198194

@@ -298,8 +294,10 @@ func (f *file) RefreshIR(ctx context.Context) {
298294
)
299295
}
300296
f.diagnostics = diagnostics
301-
f.lsp.logger.Debug(
302-
fmt.Sprintf("got %v diagnostic(s) for %s", len(f.diagnostics), f.uri.Filename()),
297+
f.lsp.logger.DebugContext(
298+
ctx, "ir diagnostic(s)",
299+
slog.String("uri", f.uri.Filename()),
300+
slog.Int("count", len(f.diagnostics)),
303301
slog.Any("diagnostics", f.diagnostics),
304302
)
305303
}
@@ -435,7 +433,7 @@ func (f *file) IndexSymbols(ctx context.Context) {
435433
return diff
436434
})
437435

438-
f.lsp.logger.DebugContext(ctx, fmt.Sprintf("symbol indexing complete %s", f.uri))
436+
f.lsp.logger.DebugContext(ctx, "symbol indexing complete", slog.String("uri", f.uri.Filename()))
439437
}
440438

441439
// indexSymbols takes the IR [ir.File] for each [file] and returns all the file symbols in
@@ -693,96 +691,102 @@ func (f *file) SymbolAt(ctx context.Context, cursor protocol.Position) *symbol {
693691
return symbol
694692
}
695693

696-
// newFileOpener returns a fileOpener for the context of this file.
697-
//
698-
// May return nil, if insufficient information is present to open the file.
699-
func (f *file) newFileOpener() fileOpener {
700-
return func(path string) (io.ReadCloser, error) {
701-
var file *file
702-
if f.objectInfo.Path() == path {
703-
file = f
704-
} else {
705-
file = f.workspace.PathToFile()[path]
706-
}
707-
if file == nil {
708-
return nil, fmt.Errorf("%s: %w", path, fs.ErrNotExist)
709-
}
710-
return io.NopCloser(strings.NewReader(file.file.Text())), nil
694+
// CancelChecks cancels any currently running checks for this file.
695+
func (f *file) CancelChecks(ctx context.Context) {
696+
if f.cancelChecks != nil {
697+
f.cancelChecks()
698+
f.cancelChecks = nil
711699
}
712700
}
713701

714-
// BuildImage builds an image for linting.
715-
// routines.
716-
//
717-
// This operation requires IndexImports.
718-
func (f *file) BuildImage(ctx context.Context) {
719-
if f.objectInfo == nil {
720-
return
721-
}
722-
723-
if opener := f.newFileOpener(); opener != nil {
724-
image, diagnostics := buildImage(ctx, f.objectInfo.Path(), f.lsp.logger, opener)
725-
if len(diagnostics) > 0 {
726-
f.diagnostics = diagnostics
727-
}
728-
f.image = image
729-
} else {
730-
f.lsp.logger.Warn("not building image", slog.String("uri", string(f.uri)))
731-
}
732-
}
733-
734-
// RunLints runs linting on this file. Returns whether any lints failed.
735-
//
736-
// This operation requires BuildImage.
737-
func (f *file) RunLints(ctx context.Context) bool {
738-
if f.IsWKT() {
739-
// Well-known types are not linted.
740-
return false
702+
// RunChecks triggers the run of checks for this file. Diagnostics are published asynchronously.
703+
func (f *file) RunChecks(ctx context.Context) {
704+
if f.IsWKT() || !f.IsOpenInEditor() {
705+
return // Must be open and not a WKT.
741706
}
707+
f.CancelChecks(ctx)
742708

709+
path := f.objectInfo.Path()
743710
workspace := f.workspace.Workspace()
744711
module := f.workspace.GetModule(f.uri)
745712
checkClient := f.workspace.CheckClient()
746-
if workspace == nil {
747-
f.lsp.logger.Warn(fmt.Sprintf("could not find workspace for %q", f.uri))
748-
return false
749-
}
750-
if module == nil || f.image == nil {
751-
f.lsp.logger.Warn(fmt.Sprintf("could not find image for %q", f.uri))
752-
return false
713+
if workspace == nil || module == nil || checkClient == nil {
714+
f.lsp.logger.Debug("checks skipped", slog.String("uri", f.uri.Filename()))
715+
return
753716
}
754-
if checkClient == nil {
755-
f.lsp.logger.Warn(fmt.Sprintf("could not find check client for %q", f.uri))
756-
return false
717+
718+
opener := make(fileOpener)
719+
for path, file := range f.workspace.PathToFile() {
720+
opener[path] = file.file.Text()
757721
}
758722

759-
f.lsp.logger.Debug(fmt.Sprintf("running lint for %q in %v", f.uri, module.FullName()))
760-
return f.appendLintErrors("buf lint", checkClient.Lint(
761-
ctx,
762-
workspace.GetLintConfigForOpaqueID(module.OpaqueID()),
763-
f.image,
764-
bufcheck.WithPluginConfigs(workspace.PluginConfigs()...),
765-
bufcheck.WithPolicyConfigs(workspace.PolicyConfigs()...),
766-
))
767-
}
723+
const checkTimeout = 30 * time.Second
724+
ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), checkTimeout)
725+
f.cancelChecks = cancel
768726

769-
func (f *file) appendLintErrors(source string, err error) bool {
770-
if err == nil {
771-
f.lsp.logger.Debug(fmt.Sprintf("%s generated no errors for %s", source, f.uri))
772-
return false
773-
}
727+
go func() {
728+
image, diagnostics := buildImage(ctx, path, f.lsp.logger, opener)
729+
if image == nil {
730+
f.lsp.logger.DebugContext(ctx, "checks cancelled on image build", slog.String("uri", f.uri.Filename()))
731+
return
732+
}
774733

775-
var annotations bufanalysis.FileAnnotationSet
776-
if !errors.As(err, &annotations) {
777-
f.lsp.logger.Warn(
778-
"error while linting",
779-
slog.String("uri", string(f.uri)),
780-
xslog.ErrorAttr(err),
781-
)
782-
return false
783-
}
734+
f.lsp.logger.DebugContext(ctx, "checks running lint", slog.String("uri", f.uri.Filename()), slog.String("module", module.OpaqueID()))
735+
var annotations []bufanalysis.FileAnnotation
736+
if err := checkClient.Lint(
737+
ctx,
738+
workspace.GetLintConfigForOpaqueID(module.OpaqueID()),
739+
image,
740+
bufcheck.WithPluginConfigs(workspace.PluginConfigs()...),
741+
bufcheck.WithPolicyConfigs(workspace.PolicyConfigs()...),
742+
); err != nil {
743+
var fileAnnotationSet bufanalysis.FileAnnotationSet
744+
if !errors.As(err, &fileAnnotationSet) {
745+
if errors.Is(err, context.Canceled) {
746+
f.lsp.logger.DebugContext(ctx, "checks cancelled", slog.String("uri", f.uri.Filename()), xslog.ErrorAttr(err))
747+
} else if errors.Is(err, context.DeadlineExceeded) {
748+
f.lsp.logger.WarnContext(ctx, "checks deadline exceeded", slog.String("uri", f.uri.Filename()), xslog.ErrorAttr(err))
749+
} else {
750+
f.lsp.logger.WarnContext(ctx, "checks failed", slog.String("uri", f.uri.Filename()), xslog.ErrorAttr(err))
751+
}
752+
return
753+
}
754+
if len(fileAnnotationSet.FileAnnotations()) == 0 {
755+
f.lsp.logger.DebugContext(ctx, "checks lint passed", slog.String("uri", f.uri.Filename()))
756+
} else {
757+
annotations = append(annotations, fileAnnotationSet.FileAnnotations()...)
758+
}
759+
}
760+
761+
select {
762+
case <-ctx.Done():
763+
f.lsp.logger.DebugContext(ctx, "checks cancelled", slog.String("uri", f.uri.Filename()), xslog.ErrorAttr(ctx.Err()))
764+
return
765+
default:
766+
}
767+
768+
f.lsp.lock.Lock()
769+
defer f.lsp.lock.Unlock()
770+
771+
select {
772+
case <-ctx.Done():
773+
f.lsp.logger.DebugContext(ctx, "checks: cancelled after waiting for file lock", slog.String("uri", f.uri.Filename()), xslog.ErrorAttr(ctx.Err()))
774+
return // Context cancelled whilst waiting to publishing diagnostics.
775+
default:
776+
}
777+
778+
// Update diagnostics and publish.
779+
if len(diagnostics) != 0 {
780+
// TODO: prefer diagnostics from the old compiler to the new compiler to remove duplicates from both.
781+
f.diagnostics = diagnostics
782+
}
783+
f.appendAnnotations("buf lint", annotations)
784+
f.PublishDiagnostics(ctx)
785+
}()
786+
}
784787

785-
for _, annotation := range annotations.FileAnnotations() {
788+
func (f *file) appendAnnotations(source string, annotations []bufanalysis.FileAnnotation) {
789+
for _, annotation := range annotations {
786790
// Convert 1-indexed byte-based line/column to byte offset.
787791
startLocation := f.file.InverseLocation(annotation.StartLine(), annotation.StartColumn(), positionalEncoding)
788792
endLocation := f.file.InverseLocation(annotation.EndLine(), annotation.EndColumn(), positionalEncoding)
@@ -795,7 +799,6 @@ func (f *file) appendLintErrors(source string, err error) bool {
795799
Message: annotation.Message(),
796800
})
797801
}
798-
return true
799802
}
800803

801804
// PublishDiagnostics publishes all of this file's diagnostics to the LSP client.

‎private/buf/buflsp/image.go‎

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"context"
1919
"fmt"
2020
"io"
21+
"io/fs"
2122
"log/slog"
2223
"strings"
2324

@@ -35,14 +36,22 @@ import (
3536
"google.golang.org/protobuf/reflect/protoreflect"
3637
)
3738

38-
// fileOpener is a function that opens files as they are named in the import
39+
// fileOpener is a type that opens files as they are named in the import
3940
// statements of .proto files.
4041
//
4142
// This is the context given to [buildImage] to control what text to look up for
4243
// specific files, so that we can e.g. use file contents that are still unsaved
4344
// in the editor, or use files from a different commit for building an --against
4445
// image.
45-
type fileOpener func(string) (io.ReadCloser, error)
46+
type fileOpener map[string]string
47+
48+
func (p fileOpener) Open(path string) (io.ReadCloser, error) {
49+
text, ok := p[path]
50+
if !ok {
51+
return nil, fmt.Errorf("%s: %w", path, fs.ErrNotExist)
52+
}
53+
return io.NopCloser(strings.NewReader(text)), nil
54+
}
4655

4756
// buildImage builds a Buf Image for the given path. This does not use the controller to build
4857
// the image, because we need delicate control over the input files: namely, for the case
@@ -58,7 +67,7 @@ func buildImage(
5867
var symbols linker.Symbols
5968
compiler := protocompile.Compiler{
6069
SourceInfoMode: protocompile.SourceInfoExtraOptionLocations,
61-
Resolver: &protocompile.SourceResolver{Accessor: opener},
70+
Resolver: &protocompile.SourceResolver{Accessor: opener.Open},
6271
Symbols: &symbols,
6372
Reporter: reporter.NewReporter(
6473
func(errorWithPos reporter.ErrorWithPos) error {
@@ -81,9 +90,10 @@ func buildImage(
8190
})
8291
}
8392
}
84-
if compiled[0] == nil {
85-
return nil, nil
93+
if len(compiled) == 0 || compiled[0] == nil {
94+
return nil, nil // Image failed to build.
8695
}
96+
compiledFile := compiled[0]
8797

8898
syntaxMissing := make(map[string]bool)
8999
pathToUnusedImports := make(map[string]map[string]bool)
@@ -104,7 +114,7 @@ func buildImage(
104114
var imageFiles []bufimage.ImageFile
105115
seen := map[string]bool{}
106116

107-
queue := []protoreflect.FileDescriptor{compiled[0]}
117+
queue := []protoreflect.FileDescriptor{compiledFile}
108118
for len(queue) > 0 {
109119
descriptor := queue[len(queue)-1]
110120
queue = queue[:len(queue)-1]
@@ -189,25 +199,14 @@ func newDiagnostic(err reporter.ErrorWithPos, isWarning bool, opener fileOpener,
189199

190200
// TODO: this is a temporary workaround for old diagnostic errors.
191201
// When using the new compiler these conversions will be already handled.
192-
if rc, openErr := opener(filename); openErr == nil {
193-
defer rc.Close()
194-
var builder strings.Builder
195-
if _, readErr := io.Copy(&builder, rc); readErr == nil {
196-
file := report.NewFile(filename, builder.String())
197-
loc := file.Location(position.Offset, positionalEncoding)
198-
utf16Col = loc.Column - 1
199-
} else {
200-
logger.Warn(
201-
"failed to read file for diagnostic position encoding",
202-
slog.String("filename", filename),
203-
xslog.ErrorAttr(readErr),
204-
)
205-
}
202+
if text, ok := opener[filename]; ok {
203+
file := report.NewFile(filename, text)
204+
loc := file.Location(position.Offset, positionalEncoding)
205+
utf16Col = loc.Column - 1
206206
} else {
207207
logger.Warn(
208208
"failed to open file for diagnostic position encoding",
209209
slog.String("filename", filename),
210-
xslog.ErrorAttr(openErr),
211210
)
212211
}
213212

0 commit comments

Comments
 (0)