gopls/internal/lsp: don't recompute diagnostics during code actions

As noted in an outstanding TODO, there's no reason to re-compute
diagnostics during code actions. This may have been cheap in the
previous gopls architecture, but still complicated the code. With the
new architecture it has significant cost, especially when running
analysis with a different set of analyzers.

Fix this by instead unbundling serialized code actions, or failing that
by matching code action context with previously published diagnostics
stored on the server.

After doing so, what remained was the additional "diagnostics" produced
by convenience analyzers. These are (rather pragmatically) refactored to
be computed from typed syntax alone, outside of the analysis framework.
Incidentally, this fixes the long-standing golang/go#51478.

The analyzers are left as thin shells, so that existing analyzer
configuration will continue to work for the short term. Longer term, we
should design a new mechanism for parameterizing refactorings and
deprecate the old analyzer settings.

Make some additional cleanup along the way, and some other relatively
superficial changes to get tests to pass.

Fixes golang/go#51478
Fixes golang/go#61508

Change-Id: I0218d4fc15a6fdb5f2fd5c0560b9d9afa079b84d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/511995
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Run-TryBot: Robert Findley <[email protected]>

Author: findleyr

gopls/internal/lsp/analysis/fillstruct/fillstruct.go

@@ -48,26 +48,34 @@ var Analyzer = &analysis.Analyzer{
 	RunDespiteErrors: true,
 }
 
+// TODO(rfindley): remove this thin wrapper around the fillstruct refactoring,
+// and eliminate the fillstruct analyzer.
+//
+// Previous iterations used the analysis framework for computing refactorings,
+// which proved inefficient.
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	for _, d := range DiagnoseFillableStructs(inspect, token.NoPos, token.NoPos, pass.Pkg, pass.TypesInfo) {
+		pass.Report(d)
+	}
+	return nil, nil
+}
+
+// DiagnoseFillableStructs computes diagnostics for fillable struct composite
+// literals overlapping with the provided start and end position.
+//
+// If either start or end is invalid, it is considered an unbounded condition.
+func DiagnoseFillableStructs(inspect *inspector.Inspector, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
+	var diags []analysis.Diagnostic
 	nodeFilter := []ast.Node{(*ast.CompositeLit)(nil)}
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
 		expr := n.(*ast.CompositeLit)
 
-		// Find enclosing file.
-		// TODO(adonovan): use inspect.WithStack?
-		var file *ast.File
-		for _, f := range pass.Files {
-			if f.Pos() <= expr.Pos() && expr.Pos() <= f.End() {
-				file = f
-				break
-			}
-		}
-		if file == nil {
-			return
+		if (start.IsValid() && expr.End() < start) || (end.IsValid() && expr.Pos() > end) {
+			return // non-overlapping
 		}
 
-		typ := pass.TypesInfo.TypeOf(expr)
+		typ := info.TypeOf(expr)
 		if typ == nil {
 			return
 		}
@@ -92,7 +100,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		for i := 0; i < fieldCount; i++ {
 			field := tStruct.Field(i)
 			// Ignore fields that are not accessible in the current package.
-			if field.Pkg() != nil && field.Pkg() != pass.Pkg && !field.Exported() {
+			if field.Pkg() != nil && field.Pkg() != pkg && !field.Exported() {
 				continue
 			}
 			fillableFields = append(fillableFields, fmt.Sprintf("%s: %s", field.Name(), field.Type().String()))
@@ -105,7 +113,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		var name string
 		if typ != tStruct {
 			// named struct type (e.g. pkg.S[T])
-			name = types.TypeString(typ, types.RelativeTo(pass.Pkg))
+			name = types.TypeString(typ, types.RelativeTo(pkg))
 		} else {
 			// anonymous struct type
 			totalFields := len(fillableFields)
@@ -124,13 +132,14 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			}
 			name = fmt.Sprintf("anonymous struct { %s }", strings.Join(fillableFields, ", "))
 		}
-		pass.Report(analysis.Diagnostic{
+		diags = append(diags, analysis.Diagnostic{
 			Message: fmt.Sprintf("Fill %s", name),
 			Pos:     expr.Pos(),
 			End:     expr.End(),
 		})
 	})
-	return nil, nil
+
+	return diags
 }
 
 // SuggestedFix computes the suggested fix for the kinds of

gopls/internal/lsp/analysis/infertypeargs/infertypeargs.go

@@ -7,8 +7,11 @@
 package infertypeargs
 
 import (
+	"go/token"
+
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
 )
 
 const Doc = `check for unnecessary type arguments in call expressions
@@ -29,3 +32,16 @@ var Analyzer = &analysis.Analyzer{
 	Requires: []*analysis.Analyzer{inspect.Analyzer},
 	Run:      run,
 }
+
+// TODO(rfindley): remove this thin wrapper around the infertypeargs refactoring,
+// and eliminate the infertypeargs analyzer.
+//
+// Previous iterations used the analysis framework for computing refactorings,
+// which proved inefficient.
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	for _, diag := range DiagnoseInferableTypeArgs(pass.Fset, inspect, token.NoPos, token.NoPos, pass.Pkg, pass.TypesInfo) {
+		pass.Report(diag)
+	}
+	return nil, nil
+}

gopls/internal/lsp/analysis/infertypeargs/run_go117.go

@@ -7,10 +7,16 @@
 
 package infertypeargs
 
-import "golang.org/x/tools/go/analysis"
+import (
+	"go/token"
+	"go/types"
 
-// This analyzer only relates to go1.18+, and uses the types.CheckExpr API that
-// was added in Go 1.13.
-func run(pass *analysis.Pass) (interface{}, error) {
-	return nil, nil
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+// DiagnoseInferableTypeArgs returns an empty slice, as generics are not supported at
+// this go version.
+func DiagnoseInferableTypeArgs(fset *token.FileSet, inspect *inspector.Inspector, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
+	return nil
 }

gopls/internal/lsp/analysis/infertypeargs/run_go118.go

@@ -13,18 +13,19 @@ import (
 	"go/types"
 
 	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/typeparams"
 )
 
-func run(pass *analysis.Pass) (interface{}, error) {
-	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-
-	nodeFilter := []ast.Node{
-		(*ast.CallExpr)(nil),
-	}
+// DiagnoseInferableTypeArgs reports diagnostics describing simplifications to type
+// arguments overlapping with the provided start and end position.
+//
+// If start or end is token.NoPos, the corresponding bound is not checked
+// (i.e. if both start and end are NoPos, all call expressions are considered).
+func DiagnoseInferableTypeArgs(fset *token.FileSet, inspect *inspector.Inspector, start, end token.Pos, pkg *types.Package, info *types.Info) []analysis.Diagnostic {
+	var diags []analysis.Diagnostic
 
+	nodeFilter := []ast.Node{(*ast.CallExpr)(nil)}
 	inspect.Preorder(nodeFilter, func(node ast.Node) {
 		call := node.(*ast.CallExpr)
 		x, lbrack, indices, rbrack := typeparams.UnpackIndexExpr(call.Fun)
@@ -33,8 +34,12 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			return // no explicit args, nothing to do
 		}
 
+		if (start.IsValid() && call.End() < start) || (end.IsValid() && call.Pos() > end) {
+			return // non-overlapping
+		}
+
 		// Confirm that instantiation actually occurred at this ident.
-		idata, ok := typeparams.GetInstances(pass.TypesInfo)[ident]
+		idata, ok := typeparams.GetInstances(info)[ident]
 		if !ok {
 			return // something went wrong, but fail open
 		}
@@ -60,7 +65,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			}
 			info := new(types.Info)
 			typeparams.InitInstanceInfo(info)
-			if err := types.CheckExpr(pass.Fset, pass.Pkg, call.Pos(), newCall, info); err != nil {
+			if err := types.CheckExpr(fset, pkg, call.Pos(), newCall, info); err != nil {
 				// Most likely inference failed.
 				break
 			}
@@ -74,20 +79,24 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			required = i
 		}
 		if required < len(indices) {
-			var start, end token.Pos
+			var s, e token.Pos
 			var edit analysis.TextEdit
 			if required == 0 {
-				start, end = lbrack, rbrack+1 // erase the entire index
-				edit = analysis.TextEdit{Pos: start, End: end}
+				s, e = lbrack, rbrack+1 // erase the entire index
+				edit = analysis.TextEdit{Pos: s, End: e}
 			} else {
-				start = indices[required].Pos()
-				end = rbrack
+				s = indices[required].Pos()
+				e = rbrack
 				//  erase from end of last arg to include last comma & white-spaces
-				edit = analysis.TextEdit{Pos: indices[required-1].End(), End: end}
+				edit = analysis.TextEdit{Pos: indices[required-1].End(), End: e}
+			}
+			// Recheck that our (narrower) fixes overlap with the requested range.
+			if (start.IsValid() && e < start) || (end.IsValid() && s > end) {
+				return // non-overlapping
 			}
-			pass.Report(analysis.Diagnostic{
-				Pos:     start,
-				End:     end,
+			diags = append(diags, analysis.Diagnostic{
+				Pos:     s,
+				End:     e,
 				Message: "unnecessary type arguments",
 				SuggestedFixes: []analysis.SuggestedFix{{
 					Message:   "simplify type arguments",
@@ -97,7 +106,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		}
 	})
 
-	return nil, nil
+	return diags
 }
 
 func calledIdent(x ast.Expr) *ast.Ident {

gopls/internal/lsp/analysis/stubmethods/stubmethods.go

@@ -15,7 +15,6 @@ import (
 	"strings"
 
 	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/typesinternal"
@@ -29,51 +28,72 @@ in order to implement a target interface`
 var Analyzer = &analysis.Analyzer{
 	Name:             "stubmethods",
 	Doc:              Doc,
-	Requires:         []*analysis.Analyzer{inspect.Analyzer},
 	Run:              run,
 	RunDespiteErrors: true,
 }
 
+// TODO(rfindley): remove this thin wrapper around the stubmethods refactoring,
+// and eliminate the stubmethods analyzer.
+//
+// Previous iterations used the analysis framework for computing refactorings,
+// which proved inefficient.
 func run(pass *analysis.Pass) (interface{}, error) {
 	for _, err := range pass.TypeErrors {
-		ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")
-		if !ifaceErr {
-			continue
-		}
 		var file *ast.File
 		for _, f := range pass.Files {
 			if f.Pos() <= err.Pos && err.Pos < f.End() {
 				file = f
 				break
 			}
 		}
-		if file == nil {
-			continue
-		}
 		// Get the end position of the error.
-		_, _, endPos, ok := typesinternal.ReadGo116ErrorData(err)
+		_, _, end, ok := typesinternal.ReadGo116ErrorData(err)
 		if !ok {
 			var buf bytes.Buffer
 			if err := format.Node(&buf, pass.Fset, file); err != nil {
 				continue
 			}
-			endPos = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
+			end = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
 		}
-		path, _ := astutil.PathEnclosingInterval(file, err.Pos, endPos)
-		si := GetStubInfo(pass.Fset, pass.TypesInfo, path, err.Pos)
-		if si == nil {
-			continue
+		if diag, ok := DiagnosticForError(pass.Fset, file, err.Pos, end, err.Msg, pass.TypesInfo); ok {
+			pass.Report(diag)
 		}
-		qf := RelativeToFiles(si.Concrete.Obj().Pkg(), file, nil, nil)
-		pass.Report(analysis.Diagnostic{
-			Pos:     err.Pos,
-			End:     endPos,
-			Message: fmt.Sprintf("Implement %s", types.TypeString(si.Interface.Type(), qf)),
-		})
 	}
+
 	return nil, nil
 }
 
+// MatchesMessage reports whether msg matches the error message sought after by
+// the stubmethods fix.
+func MatchesMessage(msg string) bool {
+	return strings.Contains(msg, "missing method") || strings.HasPrefix(msg, "cannot convert")
+}
+
+// DiagnosticForError computes a diagnostic suggesting to implement an
+// interface to fix the type checking error defined by (start, end, msg).
+//
+// If no such fix is possible, the second result is false.
+//
+// TODO(rfindley): simplify this signature once the stubmethods refactoring is
+// no longer wedged into the analysis framework.
+func DiagnosticForError(fset *token.FileSet, file *ast.File, start, end token.Pos, msg string, info *types.Info) (analysis.Diagnostic, bool) {
+	if !MatchesMessage(msg) {
+		return analysis.Diagnostic{}, false
+	}
+
+	path, _ := astutil.PathEnclosingInterval(file, start, end)
+	si := GetStubInfo(fset, info, path, start)
+	if si == nil {
+		return analysis.Diagnostic{}, false
+	}
+	qf := RelativeToFiles(si.Concrete.Obj().Pkg(), file, nil, nil)
+	return analysis.Diagnostic{
+		Pos:     start,
+		End:     end,
+		Message: fmt.Sprintf("Implement %s", types.TypeString(si.Interface.Type(), qf)),
+	}, true
+}
+
 // StubInfo represents a concrete type
 // that wants to stub out an interface type
 type StubInfo struct {

gopls/internal/lsp/cache/pkg.go

@@ -153,12 +153,12 @@ func (p *Package) DependencyTypes(path source.PackagePath) *types.Package {
 	return p.pkg.importMap[path]
 }
 
-func (p *Package) HasParseErrors() bool {
-	return len(p.pkg.parseErrors) != 0
+func (p *Package) GetParseErrors() []scanner.ErrorList {
+	return p.pkg.parseErrors
 }
 
-func (p *Package) HasTypeErrors() bool {
-	return len(p.pkg.typeErrors) != 0
+func (p *Package) GetTypeErrors() []types.Error {
+	return p.pkg.typeErrors
 }
 
 func (p *Package) DiagnosticsForFile(ctx context.Context, s source.Snapshot, uri span.URI) ([]*source.Diagnostic, error) {