Run lints on open/save/change; remove breaking checks (#4109)

Co-authored-by: Edward McFarlane <[email protected]>

Author: stefanvanburen

private/buf/buflsp/file.go

@@ -17,7 +17,6 @@
 package buflsp
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -28,7 +27,6 @@ import (
 	"os"
 	"slices"
 	"strings"
-	"time"
 
 	"buf.build/go/standard/xio"
 	"buf.build/go/standard/xlog/xslog"
@@ -50,17 +48,12 @@ import (
 	"go.lsp.dev/uri"
 )
 
-const (
-	checkRefreshPeriod = 3 * time.Second
-)
-
 // file is a file that has been opened by the client.
 //
 // Mutating a file is thread-safe.
 type file struct {
-	lsp       *lsp
-	uri       protocol.URI
-	checkWork chan<- struct{}
+	lsp *lsp
+	uri protocol.URI
 
 	file *report.File
 	// Version is an opaque version identifier given to us by the LSP client. This
@@ -82,7 +75,7 @@ type file struct {
 	referenceSymbols     []*symbol
 	symbols              []*symbol
 	diagnostics          []protocol.Diagnostic
-	image, againstImage  bufimage.Image
+	image                bufimage.Image
 }
 
 // IsLocal returns whether this is a local file, i.e. a file that the editor
@@ -126,10 +119,6 @@ func (f *file) Reset(ctx context.Context) {
 // for this file.
 func (f *file) Close(ctx context.Context) {
 	f.Manager().Close(ctx, f.uri)
-	if f.checkWork != nil {
-		close(f.checkWork)
-		f.checkWork = nil
-	}
 	if f.workspace != nil {
 		f.workspace.Release()
 		f.workspace = nil
@@ -298,7 +287,10 @@ func (f *file) Refresh(ctx context.Context) {
 	f.IndexSymbols(ctx)
 
 	progress.Report(ctx, "Running Checks", 5.0/5)
-	f.RunChecks(ctx)
+	if f.IsOpenInEditor() {
+		f.BuildImage(ctx)
+		f.RunLints(ctx)
+	}
 
 	progress.Done(ctx)
 
@@ -864,60 +856,6 @@ func (f *file) SymbolAt(ctx context.Context, cursor protocol.Position) *symbol {
 	return symbol
 }
 
-// RunChecks initiates background checks (lint and breaking) on this file and
-// returns immediately.
-//
-// Checks are executed in a background goroutine to avoid blocking the LSP
-// call. Each call to RunChecks invalidates any ongoing checks, triggering a
-// fresh run. However, previous checks are not interrupted. The checks acquire
-// the LSP mutex. Subsequent LSP calls will wait for the current check to
-// complete before proceeding.
-//
-// Checks are debounce (with the delay defined by checkRefreshPeriod) to avoid
-// overwhelming the client with expensive checks. If the file is not open in the
-// editor, checks are skipped. Diagnostics are published after checks are run.
-//
-// This operation requires IndexImports..
-func (f *file) RunChecks(ctx context.Context) {
-	// If we have not yet started a goroutine to run checks, start one.
-	// This goroutine will run checks in the background and publish diagnostics.
-	// We debounce checks to avoid spamming the client.
-	if f.checkWork == nil {
-		// We use a buffered channel of length one as the check invalidation mechanism.
-		work := make(chan struct{}, 1)
-		f.checkWork = work
-		runChecks := func(ctx context.Context) {
-			f.lsp.lock.Lock()
-			defer f.lsp.lock.Unlock()
-			if !f.IsOpenInEditor() {
-				// Skip checks if the file is not open in the editor.
-				return
-			}
-			f.lsp.logger.Info(fmt.Sprintf("running checks for %v, %v", f.uri, f.version))
-			f.BuildImages(ctx)
-			f.RunLints(ctx)
-			f.RunBreaking(ctx)
-			f.PublishDiagnostics(ctx) // Publish the latest diagnostics.
-		}
-		// Start a goroutine to process checks.
-		go func() {
-			// Detach from the parent RPC context.
-			ctx := context.WithoutCancel(ctx)
-			for range work {
-				runChecks(ctx)
-				// Debounce checks to prevent thrashing expensive checks.
-				time.Sleep(checkRefreshPeriod)
-			}
-		}()
-	}
-	// Signal the goroutine to invalidate and rerun checks.
-	select {
-	case f.checkWork <- struct{}{}:
-	default:
-		// Channel is full, checks are already invalidated and will be rerun.
-	}
-}
-
 // newFileOpener returns a fileOpener for the context of this file.
 //
 // May return nil, if insufficient information is present to open the file.
@@ -936,57 +874,11 @@ func (f *file) newFileOpener() fileOpener {
 	}
 }
 
-// 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() {
-		return nil
-	}
-
-	if f.againstStrategy == againstGit && f.againstGitRef == "" {
-		return nil
-	}
-
-	return func(path string) (io.ReadCloser, error) {
-		var file *file
-		if f.objectInfo.Path() == path {
-			file = f
-		} else {
-			file = f.importToFile[path]
-		}
-		if file == nil {
-			return nil, fmt.Errorf("%s: %w", path, fs.ErrNotExist)
-		}
-		var (
-			data []byte
-			err  error
-		)
-		if f.againstGitRef != "" {
-			data, err = git.ReadFileAtRef(
-				ctx,
-				f.lsp.container,
-				file.objectInfo.LocalPath(),
-				f.againstGitRef,
-			)
-		}
-
-		if data == nil || errors.Is(err, git.ErrInvalidGitCheckout) {
-			return os.Open(file.objectInfo.LocalPath())
-		}
-
-		return xio.CompositeReadCloser(bytes.NewReader(data), xio.NopCloser), err
-	}
-}
-
-// BuildImages builds Buf Images for this file, to be used with linting
+// BuildImage builds an image for linting.
 // routines.
 //
 // This operation requires IndexImports.
-func (f *file) BuildImages(ctx context.Context) {
+func (f *file) BuildImage(ctx context.Context) {
 	if f.objectInfo == nil {
 		return
 	}
@@ -1000,18 +892,6 @@ func (f *file) BuildImages(ctx context.Context) {
 	} else {
 		f.lsp.logger.Warn("not building image", slog.String("uri", string(f.uri)))
 	}
-
-	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.
@@ -1049,42 +929,6 @@ func (f *file) RunLints(ctx context.Context) bool {
 	))
 }
 
-// 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
-	}
-
-	workspace := f.workspace.Workspace()
-	module := f.workspace.GetModule(f.uri)
-	checkClient := f.workspace.CheckClient()
-	if workspace == nil {
-		f.lsp.logger.Warn(fmt.Sprintf("could not find workspace for %q", f.uri))
-		return false
-	}
-	if 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
-	}
-	if checkClient == nil {
-		f.lsp.logger.Warn(fmt.Sprintf("could not find check client for %q", f.uri))
-		return false
-	}
-
-	f.lsp.logger.Debug(fmt.Sprintf("running breaking for %q in %v", f.uri, module.FullName()))
-	return f.appendLintErrors("buf breaking", checkClient.Breaking(
-		ctx,
-		workspace.GetBreakingConfigForOpaqueID(module.OpaqueID()),
-		f.image,
-		f.againstImage,
-		bufcheck.WithPluginConfigs(workspace.PluginConfigs()...),
-		bufcheck.WithPolicyConfigs(workspace.PolicyConfigs()...),
-	))
-}
-
 func (f *file) appendLintErrors(source string, err error) bool {
 	if err == nil {
 		f.lsp.logger.Debug(fmt.Sprintf("%s generated no errors for %s", source, f.uri))