From 2164ba0ee7118bdbb098f78edb9b40f06fc18994 Mon Sep 17 00:00:00 2001 From: Miguel Young de la Sota Date: Wed, 16 Oct 2024 15:46:07 -0400 Subject: [PATCH] breaking --- .golangci.yml | 5 + private/buf/buflsp/buflsp.go | 65 +++-- private/buf/buflsp/file.go | 413 +++++++++++++++++++++---------- private/buf/buflsp/image.go | 141 +++++++++++ private/buf/buflsp/server.go | 51 +++- private/buf/buflsp/symbol.go | 5 - private/pkg/git/git.go | 67 +++++ private/pkg/refcount/refcount.go | 14 ++ 8 files changed, 601 insertions(+), 160 deletions(-) create mode 100644 private/buf/buflsp/image.go diff --git a/.golangci.yml b/.golangci.yml index de17f1bf56..a732345c04 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -115,6 +115,11 @@ issues: # G115 checks for use of truncating conversions. path: private/buf/buflsp/file.go text: "G115:" + - linters: + - gosec + # G115 checks for use of truncating conversions. + path: private/buf/buflsp/image.go + text: "G115:" - linters: - gosec # G115 checks for use of truncating conversions. diff --git a/private/buf/buflsp/buflsp.go b/private/buf/buflsp/buflsp.go index 9d04d840e3..abdc0cc236 100644 --- a/private/buf/buflsp/buflsp.go +++ b/private/buf/buflsp/buflsp.go @@ -27,6 +27,7 @@ import ( "github.com/bufbuild/buf/private/buf/bufctl" "github.com/bufbuild/buf/private/bufpkg/bufcheck" "github.com/bufbuild/buf/private/pkg/app/appext" + "github.com/bufbuild/buf/private/pkg/command" "github.com/bufbuild/buf/private/pkg/slogext" "github.com/bufbuild/buf/private/pkg/storage" "github.com/bufbuild/buf/private/pkg/storage/storageos" @@ -65,11 +66,13 @@ func Serve( &connWrapper{Conn: conn, logger: container.Logger()}, zap.NewNop(), // The logging from protocol itself isn't very good, we've replaced it with connAdapter here. ), + container: container, logger: container.Logger(), controller: controller, checkClient: checkClient, rootBucket: bucket, wktBucket: wktBucket, + runner: command.NewRunner(), } lsp.fileManager = newFileManager(lsp) off := protocol.TraceOff @@ -89,14 +92,16 @@ func Serve( // Its handler methods are not defined in buflsp.go; they are defined in other files, grouped // according to the groupings in type lsp struct { - conn jsonrpc2.Conn - client protocol.Client + conn jsonrpc2.Conn + client protocol.Client + container appext.Container logger *slog.Logger controller bufctl.Controller checkClient bufcheck.Client rootBucket storage.ReadBucket fileManager *fileManager + runner command.Runner wktBucket storage.ReadBucket @@ -130,19 +135,47 @@ func (l *lsp) init(_ context.Context, params *protocol.InitializeParams) error { // to inject debug logging, tracing, and timeouts to requests. func (l *lsp) newHandler() jsonrpc2.Handler { actual := protocol.ServerHandler(newServer(l), nil) - return func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) (retErr error) { - defer slogext.DebugProfile(l.logger, slog.String("method", req.Method()), slog.Any("params", req.Params()))() - - replier := l.wrapReplier(reply, req) - - // Verify that the server has been initialized if this isn't the initialization - // request. - if req.Method() != protocol.MethodInitialize && l.initParams.Load() == nil { - return replier(ctx, nil, fmt.Errorf("the first call to the server must be the %q method", protocol.MethodInitialize)) - } - - l.lock.Lock() - defer l.lock.Unlock() - return actual(ctx, replier, req) + return func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) error { + go func() { + // Although everything is behind one big lock, we need to run requests + // in their own goroutines; otherwise, if a request performs a call to the + // client, the single goroutine that actually handles sending to and + // from the client will deadlock. + // + // jsonrpc2 does not do a good job of documenting this limitation. + + l.logger.Debug( + "handling request", + slog.String("method", req.Method()), + slog.Any("params", req.Params()), + ) + defer slogext.DebugProfile( + l.logger, + slog.String("method", req.Method()), + slog.Any("params", req.Params()), + )() + + replier := l.wrapReplier(reply, req) + + var err error + if req.Method() != protocol.MethodInitialize && l.initParams.Load() == nil { + // Verify that the server has been initialized if this isn't the initialization + // request. + err = replier(ctx, nil, fmt.Errorf("the first call to the server must be the %q method", protocol.MethodInitialize)) + } else { + l.lock.Lock() + err = actual(ctx, replier, req) + l.lock.Unlock() + } + + if err != nil { + l.logger.Error( + "error while replying to request", + slog.String("method", req.Method()), + slogext.ErrorAttr(err), + ) + } + }() + return nil } } diff --git a/private/buf/buflsp/file.go b/private/buf/buflsp/file.go index 2103dff795..c1128386e6 100644 --- a/private/buf/buflsp/file.go +++ b/private/buf/buflsp/file.go @@ -17,6 +17,7 @@ package buflsp import ( + "bytes" "context" "errors" "fmt" @@ -31,22 +32,46 @@ import ( "github.com/bufbuild/buf/private/bufpkg/bufcheck" "github.com/bufbuild/buf/private/bufpkg/bufimage" "github.com/bufbuild/buf/private/bufpkg/bufmodule" + "github.com/bufbuild/buf/private/pkg/git" "github.com/bufbuild/buf/private/pkg/ioext" + "github.com/bufbuild/buf/private/pkg/normalpath" "github.com/bufbuild/buf/private/pkg/slogext" "github.com/bufbuild/buf/private/pkg/storage" - "github.com/bufbuild/protocompile" "github.com/bufbuild/protocompile/ast" - "github.com/bufbuild/protocompile/linker" "github.com/bufbuild/protocompile/parser" - "github.com/bufbuild/protocompile/protoutil" "github.com/bufbuild/protocompile/reporter" - "github.com/google/uuid" "go.lsp.dev/protocol" - "google.golang.org/protobuf/reflect/protoreflect" +) + +const ( + againstTrunk againstKind = iota + 1 + againstHead + againstSaved ) const descriptorPath = "google/protobuf/descriptor.proto" +type againstKind int + +// againstKindFromString parses an againstKind from a config setting sent by +// the client. +// +// Returns againstTrunk, false if the value is not recognized. +func againstKindFromString(s string) (againstKind, bool) { + switch s { + // These values are the same as those present in the package.json for the + // VSCode client. + case "trunk": + return againstTrunk, true + case "head": + return againstHead, true + case "lastSaved": + return againstSaved, true + default: + return againstTrunk, false + } +} + // file is a file that has been opened by the client. // // Mutating a file is thread-safe. @@ -60,20 +85,23 @@ type file struct { // diagnostics or symbols an operating refers to. version int32 hasText bool // Whether this file has ever had text read into it. - // Always set false->true. Once true, never becomes false again. workspace bufworkspace.Workspace module bufmodule.Module + againstKind againstKind + gitRemote string + gitRemoteBranch string + objectInfo storage.ObjectInfo importablePathToObject map[string]storage.ObjectInfo - fileNode *ast.FileNode - packageNode *ast.PackageNode - diagnostics []protocol.Diagnostic - importToFile map[string]*file - symbols []*symbol - image bufimage.Image + fileNode *ast.FileNode + packageNode *ast.PackageNode + diagnostics []protocol.Diagnostic + importToFile map[string]*file + symbols []*symbol + image, againstImage bufimage.Image } // IsWKT returns whether this file corresponds to a well-known type. @@ -131,6 +159,15 @@ func (f *file) Close(ctx context.Context) { f.lsp.fileManager.Close(ctx, f.uri) } +// IsOpenInEditor returns whether this file was opened in the LSP client's +// editor. +// +// Some files may be opened as dependencies, so we want to avoid doing extra +// work like sending progress notifications. +func (f *file) IsOpenInEditor() bool { + return f.version != -1 // See [file.ReadFromDisk]. +} + // ReadFromDisk reads this file from disk if it has never had data loaded into it before. // // If it has been read from disk before, or has received updates from the LSP client, this @@ -161,6 +198,107 @@ func (f *file) Update(ctx context.Context, version int32, text string) { f.hasText = true } +// FetchSettings refreshes configuration settings for this file. +// +// This only needs to happen when the file is open or when the client signals +// that configuration settings have changed. +func (f *file) RefreshSettings(ctx context.Context) { + settings, err := f.lsp.client.Configuration(ctx, &protocol.ConfigurationParams{ + Items: []protocol.ConfigurationItem{ + {ScopeURI: f.uri, Section: "buf.against"}, + {ScopeURI: f.uri, Section: "buf.gitRemote"}, + }, + }) + if err != nil { + // We can throw the error away, since the handler logs it for us. + return + } + + f.againstKind = getSetting(f, settings, "buf.against", 0, againstKindFromString) + f.gitRemote = getSetting(f, settings, "buf.gitRemote", 1, func(s string) (string, bool) { return s, true }) + + // Given the remote, we need to find the main branch. First, look up the + // remote. + remote, err := git.GetRemote( + ctx, + f.lsp.runner, + f.lsp.container, + normalpath.Dir(f.uri.Filename()), + f.gitRemote, + ) + 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 { + f.lsp.logger.Warn( + "failed to validate buf.gitRemote", + slog.String("uri", string(f.uri)), + slogext.ErrorAttr(err), + ) + f.gitRemote = "" + } else { + f.gitRemoteBranch = remote.HEADBranch() + + f.lsp.logger.Debug( + "found remote branch", + slog.String("uri", string(f.uri)), + slog.String("ref", f.gitRemote+"/"+f.gitRemoteBranch), + ) + } +} + +// getSetting is a helper that extracts a configuration setting from the return +// value of [protocol.Client.Configuration]. +// +// The parse function should convert the JSON value we get from the protocol +// (such as a string), potentially performing validation, and returning a default +// value on validation failure. +func getSetting[T, U any](f *file, settings []any, name string, index int, parse func(T) (U, bool)) (value U) { + if len(settings) <= index { + f.lsp.logger.Warn( + "missing config setting", + slog.String("setting", name), + slog.String("uri", string(f.uri)), + ) + } + + if raw, ok := settings[index].(T); ok { + // For invalid settings, this will default to againstTrunk for us! + value, ok = parse(raw) + if !ok { + f.lsp.logger.Warn( + "invalid config setting", + slog.String("setting", name), + slog.String("uri", string(f.uri)), + slog.Any("raw", raw), + ) + } + } else { + f.lsp.logger.Warn( + "invalid setting for buf.against", + slog.String("setting", name), + slog.String("uri", string(f.uri)), + slog.Any("raw", raw), + ) + } + + f.lsp.logger.Debug( + "parsed configuration setting", + slog.String("setting", name), + slog.String("uri", string(f.uri)), + slog.Any("value", value), + ) + + return value +} + // Refresh rebuilds all of a file's internal book-keeping. // // If deep is set, this will also load imports and refresh those, too. @@ -174,12 +312,13 @@ func (f *file) Refresh(ctx context.Context) { progress.Report(ctx, "Indexing Imports", 2.0/6) f.IndexImports(ctx) - progress.Report(ctx, "Detecting Module", 3.0/6) + progress.Report(ctx, "Detecting Buf Module", 3.0/6) f.FindModule(ctx) - progress.Report(ctx, "Linking Descriptors", 4.0/6) - f.BuildImage(ctx) + progress.Report(ctx, "Running Lints", 4.0/6) + f.BuildImages(ctx) f.RunLints(ctx) + f.RunBreaking(ctx) progress.Report(ctx, "Indexing Symbols", 5.0/6) f.IndexSymbols(ctx) @@ -196,10 +335,6 @@ func (f *file) Refresh(ctx context.Context) { // // Returns whether a reparse was necessary. func (f *file) RefreshAST(ctx context.Context) bool { - if f.fileNode != nil { - return false - } - // NOTE: We intentionally do not use var report report here, because we need // report to be non-nil when empty; this is because if it is nil, when calling // PublishDiagnostics() below it will be serialized as JSON null. @@ -407,132 +542,115 @@ func (f *file) IndexImports(ctx context.Context) { // Parse the imported file and find all symbols in it, but do not // index symbols in the import's imports, otherwise we will recursively // index the universe and that would be quite slow. - file.RefreshAST(ctx) + + if f.fileNode != nil { + file.RefreshAST(ctx) + } file.IndexSymbols(ctx) } } -// BuildImage builds a Buf Image for this file. This does not use the controller to build -// the image, because we need delicate control over the input files: namely, for the case -// when we depend on a file that has been opened and modified in the editor. +// newFileOpener returns a fileOpener for the context of this file. // -// This operation requires IndexImports(). -func (f *file) BuildImage(ctx context.Context) { - importable := f.importablePathToObject - fileInfo := f.objectInfo - - if importable == nil || fileInfo == nil { - return - } - - var report report - var symbols linker.Symbols - compiler := protocompile.Compiler{ - SourceInfoMode: protocompile.SourceInfoExtraOptionLocations, - Resolver: &protocompile.SourceResolver{ - Accessor: func(path string) (io.ReadCloser, error) { - var uri protocol.URI - fileInfo, ok := importable[path] - if ok { - uri = protocol.URI("file://" + fileInfo.LocalPath()) - } else { - uri = protocol.URI("file://" + path) - } - - if file := f.Manager().Get(uri); file != nil { - return ioext.CompositeReadCloser(strings.NewReader(file.text), ioext.NopCloser), nil - } else if !ok { - return nil, os.ErrNotExist - } - - return os.Open(fileInfo.LocalPath()) - }, - }, - Symbols: &symbols, - Reporter: &report, - } - - compiled, err := compiler.Compile(ctx, fileInfo.Path()) - if err != nil { - f.diagnostics = report.diagnostics - } - if compiled[0] == nil { - return +// May return nil, if insufficient information is present to open the file. +func (f *file) newFileOpener() fileOpener { + if f.importablePathToObject == nil { + return nil } - var imageFiles []bufimage.ImageFile - seen := map[string]bool{} - - queue := []protoreflect.FileDescriptor{compiled[0]} - for len(queue) > 0 { - descriptor := queue[len(queue)-1] - queue = queue[:len(queue)-1] - - if seen[descriptor.Path()] { - continue + return func(path string) (io.ReadCloser, error) { + var uri protocol.URI + fileInfo, ok := f.importablePathToObject[path] + if ok { + uri = protocol.URI("file://" + fileInfo.LocalPath()) + } else { + uri = protocol.URI("file://" + path) } - seen[descriptor.Path()] = true - unused, ok := report.pathToUnusedImports[descriptor.Path()] - var unusedIndices []int32 - if ok { - unusedIndices = make([]int32, 0, len(unused)) + if file := f.Manager().Get(uri); file != nil { + return ioext.CompositeReadCloser(strings.NewReader(file.text), ioext.NopCloser), nil + } else if !ok { + return nil, os.ErrNotExist } - imports := descriptor.Imports() - for i := 0; i < imports.Len(); i++ { - dep := imports.Get(i).FileDescriptor - if dep == nil { - f.lsp.logger.Warn(fmt.Sprintf("found nil FileDescriptor for import %s", imports.Get(i).Path())) - continue - } + return os.Open(fileInfo.LocalPath()) + } +} - queue = append(queue, dep) +// newAgainstFileOpener returns a fileOpener for building the --against file +// for this file. In other words, this pulls files out of the git index, if +// necessary. +// +// May return nil, if there is insufficient information to build an --against +// file. +func (f *file) newAgainstFileOpener(ctx context.Context) fileOpener { + if !f.IsLocal() || f.importablePathToObject == nil || f.gitRemoteBranch == "" { + return nil + } - if unused != nil { - if _, ok := unused[dep.Path()]; ok { - unusedIndices = append(unusedIndices, int32(i)) - } - } + return func(path string) (io.ReadCloser, error) { + fileInfo, ok := f.importablePathToObject[path] + if !ok { + return nil, fmt.Errorf("failed to resolve import: %s", path) } - descriptorProto := protoutil.ProtoFromFileDescriptor(descriptor) - if descriptorProto == nil { - err = fmt.Errorf("protoutil.ProtoFromFileDescriptor() returned nil for %q", descriptor.Path()) - break + readAtRef := func(ref string) ([]byte, error) { + return git.ReadFileAtRef(ctx, f.lsp.runner, f.lsp.container, fileInfo.LocalPath(), ref) } - var imageFile bufimage.ImageFile - imageFile, err = bufimage.NewImageFile( - descriptorProto, - nil, - uuid.UUID{}, - "", - descriptor.Path(), - descriptor.Path() != fileInfo.Path(), - report.syntaxMissing[descriptor.Path()], - unusedIndices, + var ( + data []byte + err error ) - if err != nil { - break + switch f.againstKind { + case againstTrunk: + if f.gitRemote == "" { + data, err = readAtRef(f.gitRemoteBranch) + } else { + data, err = readAtRef(f.gitRemote + "/" + f.gitRemoteBranch) + } + case againstHead: + data, err = readAtRef("HEAD") } - imageFiles = append(imageFiles, imageFile) - f.lsp.logger.Debug(fmt.Sprintf("added image file for %s", descriptor.Path())) + if data == nil || errors.Is(err, git.ErrNotARepo) { + return os.Open(fileInfo.LocalPath()) + } + + return ioext.CompositeReadCloser(bytes.NewReader(data), ioext.NopCloser), err } +} - if err != nil { - f.lsp.logger.Warn("could not build image", slog.String("uri", string(f.uri)), slogext.ErrorAttr(err)) +// BuildImage build Buf Images for this file, to be used with linting +// routines. +// +// This operation requires IndexImports(). +func (f *file) BuildImages(ctx context.Context) { + if f.objectInfo == nil { return } - image, err := bufimage.NewImage(imageFiles) - if err != nil { - f.lsp.logger.Warn("could not build image", slog.String("uri", string(f.uri)), slogext.ErrorAttr(err)) - return + if opener := f.newFileOpener(); opener != nil { + image, diagnostics := buildImage(ctx, f.objectInfo.Path(), f.lsp.logger, opener) + if len(diagnostics) > 0 { + f.diagnostics = diagnostics + } + f.image = image + } else { + f.lsp.logger.Warn("not building image", slog.String("uri", string(f.uri))) } - f.image = image + if opener := f.newAgainstFileOpener(ctx); opener != nil { + // We explicitly throw the diagnostics away. + image, diagnostics := buildImage(ctx, f.objectInfo.Path(), f.lsp.logger, opener) + + f.againstImage = image + if image == nil { + f.lsp.logger.Warn("failed to build --against image", slog.Any("diagnostics", diagnostics)) + } + } else { + f.lsp.logger.Warn("not building --against image", slog.String("uri", string(f.uri))) + } } // RunLints runs linting on this file. Returns whether any lints failed. @@ -544,41 +662,61 @@ func (f *file) RunLints(ctx context.Context) bool { return false } - workspace := f.workspace - module := f.module - image := f.image - - if module == nil || image == nil { + if f.module == nil || f.image == nil { f.lsp.logger.Warn(fmt.Sprintf("could not find image for %q", f.uri)) return false } - f.lsp.logger.Debug(fmt.Sprintf("running lint for %q in %v", f.uri, module.ModuleFullName())) + f.lsp.logger.Debug(fmt.Sprintf("running lint for %q in %v", f.uri, f.module.ModuleFullName())) + return f.appendLintErrors("buf breaking", f.lsp.checkClient.Lint( + ctx, + f.workspace.GetLintConfigForOpaqueID(f.module.OpaqueID()), + f.image, + bufcheck.WithPluginConfigs(f.workspace.PluginConfigs()...), + )) +} + +// RunBreaking runs breaking lints on this file. Returns whether any lints failed. +// +// This operation requires BuildImage(). +func (f *file) RunBreaking(ctx context.Context) bool { + if f.IsWKT() { + // Well-known types are not linted. + return false + } + + if f.module == nil || f.image == nil || f.againstImage == nil { + f.lsp.logger.Warn(fmt.Sprintf("could not find --against image for %q", f.uri)) + return false + } - lintConfig := workspace.GetLintConfigForOpaqueID(module.OpaqueID()) - err := f.lsp.checkClient.Lint( + f.lsp.logger.Debug(fmt.Sprintf("running breaking for %q in %v", f.uri, f.module.ModuleFullName())) + return f.appendLintErrors("buf breaking", f.lsp.checkClient.Breaking( ctx, - lintConfig, - image, - bufcheck.WithPluginConfigs(workspace.PluginConfigs()...), - ) + f.workspace.GetBreakingConfigForOpaqueID(f.module.OpaqueID()), + f.image, + f.againstImage, + bufcheck.WithPluginConfigs(f.workspace.PluginConfigs()...), + )) +} +func (f *file) appendLintErrors(source string, err error) bool { if err == nil { - f.lsp.logger.Warn(fmt.Sprintf("lint generated no errors for %s", f.uri)) + f.lsp.logger.Debug(fmt.Sprintf("%s generated no errors for %s", source, f.uri)) return false } var annotations bufanalysis.FileAnnotationSet if !errors.As(err, &annotations) { - f.lsp.logger.Warn("error while linting", slog.String("uri", string(f.uri)), slogext.ErrorAttr(err)) + f.lsp.logger.Warn( + "error while linting", + slog.String("uri", string(f.uri)), + slogext.ErrorAttr(err), + ) return false } - f.lsp.logger.Warn(fmt.Sprintf("lint generated %d error(s) for %s", len(annotations.FileAnnotations()), f.uri)) - for _, annotation := range annotations.FileAnnotations() { - f.lsp.logger.Info(annotation.FileInfo().Path(), " ", annotation.FileInfo().ExternalPath()) - f.diagnostics = append(f.diagnostics, protocol.Diagnostic{ Range: protocol.Range{ Start: protocol.Position{ @@ -592,10 +730,11 @@ func (f *file) RunLints(ctx context.Context) bool { }, Code: annotation.Type(), Severity: protocol.DiagnosticSeverityError, - Source: "buf lint", + Source: source, Message: annotation.Message(), }) } + return true } diff --git a/private/buf/buflsp/image.go b/private/buf/buflsp/image.go new file mode 100644 index 0000000000..0e4bc7df5b --- /dev/null +++ b/private/buf/buflsp/image.go @@ -0,0 +1,141 @@ +// Copyright 2020-2024 Buf Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package buflsp + +import ( + "context" + "fmt" + "io" + "log/slog" + + "github.com/bufbuild/buf/private/bufpkg/bufimage" + "github.com/bufbuild/buf/private/pkg/slogext" + "github.com/bufbuild/protocompile" + "github.com/bufbuild/protocompile/linker" + "github.com/bufbuild/protocompile/protoutil" + "github.com/google/uuid" + "go.lsp.dev/protocol" + "google.golang.org/protobuf/reflect/protoreflect" +) + +// fileOpener is a function that opens files as they are named in the import +// statements of .proto files. +// +// This is the context given to [buildImage] to control what text to look up for +// specific files, so that we can e.g. use file contents that are still unsaved +// in the editor, or use files from a different commit for building an --against +// image. +type fileOpener func(string) (io.ReadCloser, error) + +// buildImage builds a Buf Image for the given path. This does not use the controller to build +// the image, because we need delicate control over the input files: namely, for the case +// when we depend on a file that has been opened and modified in the editor. +func buildImage( + ctx context.Context, + path string, + logger *slog.Logger, + opener fileOpener, +) (bufimage.Image, []protocol.Diagnostic) { + var report report + var symbols linker.Symbols + compiler := protocompile.Compiler{ + SourceInfoMode: protocompile.SourceInfoExtraOptionLocations, + Resolver: &protocompile.SourceResolver{Accessor: opener}, + Symbols: &symbols, + Reporter: &report, + } + + compiled, err := compiler.Compile(ctx, path) + if err != nil { + logger.Warn("error building image", slog.String("path", path), slogext.ErrorAttr(err)) + } + if compiled[0] == nil { + return nil, report.diagnostics + } + + var imageFiles []bufimage.ImageFile + seen := map[string]bool{} + + queue := []protoreflect.FileDescriptor{compiled[0]} + for len(queue) > 0 { + descriptor := queue[len(queue)-1] + queue = queue[:len(queue)-1] + + if seen[descriptor.Path()] { + continue + } + seen[descriptor.Path()] = true + + unused, ok := report.pathToUnusedImports[descriptor.Path()] + var unusedIndices []int32 + if ok { + unusedIndices = make([]int32, 0, len(unused)) + } + + imports := descriptor.Imports() + for i := 0; i < imports.Len(); i++ { + dep := imports.Get(i).FileDescriptor + if dep == nil { + logger.Warn(fmt.Sprintf("found nil FileDescriptor for import %s", imports.Get(i).Path())) + continue + } + + queue = append(queue, dep) + + if unused != nil { + if _, ok := unused[dep.Path()]; ok { + unusedIndices = append(unusedIndices, int32(i)) + } + } + } + + descriptorProto := protoutil.ProtoFromFileDescriptor(descriptor) + if descriptorProto == nil { + err = fmt.Errorf("protoutil.ProtoFromFileDescriptor() returned nil for %q", descriptor.Path()) + break + } + + var imageFile bufimage.ImageFile + imageFile, err = bufimage.NewImageFile( + descriptorProto, + nil, + uuid.UUID{}, + "", + descriptor.Path(), + descriptor.Path() != path, + report.syntaxMissing[descriptor.Path()], + unusedIndices, + ) + if err != nil { + break + } + + imageFiles = append(imageFiles, imageFile) + logger.Debug(fmt.Sprintf("added image file for %s", descriptor.Path())) + } + + if err != nil { + logger.Warn("could not build image", slog.String("path", path), slogext.ErrorAttr(err)) + return nil, report.diagnostics + } + + image, err := bufimage.NewImage(imageFiles) + if err != nil { + logger.Warn("could not build image", slog.String("path", path), slogext.ErrorAttr(err)) + return nil, report.diagnostics + } + + return image, report.diagnostics +} diff --git a/private/buf/buflsp/server.go b/private/buf/buflsp/server.go index eaf1f3c724..4b6f56dc74 100644 --- a/private/buf/buflsp/server.go +++ b/private/buf/buflsp/server.go @@ -110,6 +110,9 @@ func (s *server) Initialize( // usually get especially huge, so this simplifies our logic without // necessarily making the LSP slow. Change: protocol.TextDocumentSyncKindFull, + Save: &protocol.SaveOptions{ + IncludeText: false, + }, }, DefinitionProvider: &protocol.DefinitionOptions{ WorkDoneProgressOptions: protocol.WorkDoneProgressOptions{WorkDoneProgress: true}, @@ -135,6 +138,15 @@ func (s *server) Initialized( ctx context.Context, params *protocol.InitializedParams, ) error { + if s.initParams.Load().Capabilities.Workspace.DidChangeConfiguration.DynamicRegistration { + // The error is logged for us by the client wrapper. + _ = s.client.RegisterCapability(ctx, &protocol.RegistrationParams{ + Registrations: []protocol.Registration{ + {Method: protocol.MethodWorkspaceDidChangeConfiguration}, + }, + }) + } + return nil } @@ -162,6 +174,24 @@ func (s *server) Exit(ctx context.Context) error { return s.lsp.conn.Close() } +// DidChangeConfiguration is sent whenever the +func (s *server) DidChangeConfiguration( + ctx context.Context, + params *protocol.DidChangeConfigurationParams, +) error { + // We need to refresh every open file's settings, and refresh the file + // itself. + s.fileManager.uriToFile.Range(func(_ protocol.URI, file *file) bool { + if file.IsOpenInEditor() { + file.RefreshSettings(ctx) + file.Refresh(ctx) + } + return true + }) + + return nil +} + // -- File synchronization methods. // DidOpen is called whenever the client opens a document. This is our signal to parse @@ -171,8 +201,9 @@ func (s *server) DidOpen( params *protocol.DidOpenTextDocumentParams, ) error { file := s.fileManager.Open(ctx, params.TextDocument.URI) + file.RefreshSettings(ctx) file.Update(ctx, params.TextDocument.Version, params.TextDocument.Text) - file.Refresh(context.WithoutCancel(ctx)) + file.Refresh(ctx) return nil } @@ -189,7 +220,23 @@ func (s *server) DidChange( } file.Update(ctx, params.TextDocument.Version, params.ContentChanges[0].Text) - file.Refresh(context.WithoutCancel(ctx)) + file.Refresh(ctx) + return nil +} + +// DidSave is called whenever the client saves a document. +func (s *server) DidSave( + ctx context.Context, + params *protocol.DidSaveTextDocumentParams, +) error { + // We use this as an opportunity to do a refresh; some lints, such as + // breaking-against-last-saved, rely on this. + file := s.fileManager.Get(params.TextDocument.URI) + if file == nil { + // Update for a file we don't know about? Seems bad! + return fmt.Errorf("received update for file that was not open: %q", params.TextDocument.URI) + } + file.Refresh(ctx) return nil } diff --git a/private/buf/buflsp/symbol.go b/private/buf/buflsp/symbol.go index 8ea0da97f1..1d73107849 100644 --- a/private/buf/buflsp/symbol.go +++ b/private/buf/buflsp/symbol.go @@ -169,11 +169,6 @@ func (s *symbol) ResolveCrossFile(ctx context.Context) { return } - // Fully index the file this reference is in, if different from the current. - if s.file != ref.file { - ref.file.Refresh(ctx) - } - // Find the definition that contains the type we want. def, node := kind.seeTypeOf.Definition(ctx) if def == nil { diff --git a/private/pkg/git/git.go b/private/pkg/git/git.go index 04c950d6af..6c0f3004bd 100644 --- a/private/pkg/git/git.go +++ b/private/pkg/git/git.go @@ -21,7 +21,9 @@ import ( "errors" "fmt" "log/slog" + "os" "os/exec" + "path/filepath" "regexp" "strings" @@ -46,6 +48,10 @@ var ( // ErrInvalidGitCheckout is returned from CheckDirectoryIsValidGitCheckout when the // specified directory is not a valid git checkout. ErrInvalidGitCheckout = errors.New("invalid git checkout") + + // ErrNotARepo is returned from ReadFileAtRef when the given path does not + // point into a local repository. + ErrNotARepo = errors.New("not a git repository") ) // Name is a name identifiable by git. @@ -352,6 +358,67 @@ func GetRefsForGitCommitAndRemote( return refs, nil } +// ReadFileAtRef will read the file at path rolled back to the given ref, if +// it exists at that ref. +// +// Specifically, this will take path, and find the associated .git directory +// (and thus, repository root) associated with that path, if any. It will then +// look up the commit corresponding to ref in that git directory, and within +// that commit, the blob corresponding to that path (relative to the repository +// root). +func ReadFileAtRef( + ctx context.Context, + runner command.Runner, + envContainer app.EnvContainer, + path, ref string, +) ([]byte, error) { + orig := path + path, err := filepath.Abs(path) + if err != nil { + return nil, err + } + + // Find a directory with a .git directory. + dir := filepath.Dir(path) + 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 + } + } + + // This gives us the name of the blob we're looking for. + rel, err := filepath.Rel(dir, path) + if err != nil { + return nil, err + } + + // Call git show to show us the file we want. + stdout := bytes.NewBuffer(nil) + stderr := bytes.NewBuffer(nil) + if err := runner.Run( + ctx, + gitCommand, + command.RunWithArgs("--no-pager", "show", ref+":"+rel), + command.RunWithStdout(stdout), + command.RunWithStderr(stderr), + command.RunWithDir(dir), + command.RunWithEnv(app.EnvironMap(envContainer)), + ); err != nil { + return nil, fmt.Errorf("failed to get text of file %s at ref %s: %w: %s", orig, ref, err, stderr.String()) + } + + return stdout.Bytes(), nil +} + func getAllTrimmedLinesFromBuffer(buffer *bytes.Buffer) []string { scanner := bufio.NewScanner(buffer) var lines []string diff --git a/private/pkg/refcount/refcount.go b/private/pkg/refcount/refcount.go index cdbdf3b63b..efed9b1971 100644 --- a/private/pkg/refcount/refcount.go +++ b/private/pkg/refcount/refcount.go @@ -83,6 +83,20 @@ func (m *Map[K, V]) Get(key K) *V { return &value.value } +// Range ranges over this map. Beware! While ranging, writes to the map will block. +// +// This implements [iter.Seq2] for Map[K, V]. +func (m *Map[K, V]) Range(yield func(k K, v *V) bool) { + m.lock.RLock() + defer m.lock.RUnlock() + + for k, v := range m.table { + if !yield(k, &v.value) { + break + } + } +} + // Delete deletes a key from the map. // // The key will only be evicted once [Map.Delete] has been called an equal number of times