go/analysis: add Pass.TypeErrors field

The result of type checking is a package (types.Package), type
annotations on syntax trees (types.Info), and zero or more type
errors (types.Error).  Hitherto we had assumed that analyzers don't
need access to type errors, but in fact it turns out to be useful:
for example, analyzers can look at broken code and suggest quick
fixes to repair the mistakes.

This change adds a Pass.TypeErrors field that holds the errors
produced during type checking. It may be non-nil only when the
Analyzer.RunDespiteErrors flag was enabled. Similarly, it adds
a TypeErrors field to go/packages.Package.
(The existing Packages.Error field loses important details.)

Gopls was already using analyzers in this way, (ab)using its privileged
position in the x/tools repo. This change removes the need for such hacks.

We expect that all analysis drivers that support RunDespiteErrors will
in due course populate the Pass.TypesErrors field. This change updates
the go/packages-based driver to do so; no changes were needed to
unitchecker since it does not support RunDespiteErrors.
In the meantime, not populating this field is not expected to cause
any compatibility problems.

Fixes golang/go#54619

Change-Id: Ia7c72242e332782e8919a4c30b2107c37bcec9ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/425095
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Tim King <[email protected]>

Author: adonovan

go/analysis/analysis.go

@@ -11,8 +11,6 @@ import (
 	"go/token"
 	"go/types"
 	"reflect"
-
-	"golang.org/x/tools/internal/analysisinternal"
 )
 
 // An Analyzer describes an analysis function and its options.
@@ -48,6 +46,7 @@ type Analyzer struct {
 	// RunDespiteErrors allows the driver to invoke
 	// the Run method of this analyzer even on a
 	// package that contains parse or type errors.
+	// The Pass.TypeErrors field may consequently be non-empty.
 	RunDespiteErrors bool
 
 	// Requires is a set of analyzers that must run successfully
@@ -75,17 +74,6 @@ type Analyzer struct {
 
 func (a *Analyzer) String() string { return a.Name }
 
-func init() {
-	// Set the analysisinternal functions to be able to pass type errors
-	// to the Pass type without modifying the go/analysis API.
-	analysisinternal.SetTypeErrors = func(p interface{}, errors []types.Error) {
-		p.(*Pass).typeErrors = errors
-	}
-	analysisinternal.GetTypeErrors = func(p interface{}) []types.Error {
-		return p.(*Pass).typeErrors
-	}
-}
-
 // A Pass provides information to the Run function that
 // applies a specific analyzer to a single Go package.
 //
@@ -106,6 +94,7 @@ type Pass struct {
 	Pkg          *types.Package // type information about the package
 	TypesInfo    *types.Info    // type information about the syntax trees
 	TypesSizes   types.Sizes    // function for computing sizes of types
+	TypeErrors   []types.Error  // type errors (only if Analyzer.RunDespiteErrors)
 
 	// Report reports a Diagnostic, a finding about a specific location
 	// in the analyzed source code such as a potential mistake.

go/analysis/internal/checker/checker.go

@@ -26,15 +26,13 @@ import (
 	"runtime/pprof"
 	"runtime/trace"
 	"sort"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/internal/analysisflags"
 	"golang.org/x/tools/go/packages"
-	"golang.org/x/tools/internal/analysisinternal"
 )
 
 var (
@@ -699,14 +697,16 @@ func (act *action) execOnce() {
 
 	// Run the analysis.
 	pass := &analysis.Pass{
-		Analyzer:          act.a,
-		Fset:              act.pkg.Fset,
-		Files:             act.pkg.Syntax,
-		OtherFiles:        act.pkg.OtherFiles,
-		IgnoredFiles:      act.pkg.IgnoredFiles,
-		Pkg:               act.pkg.Types,
-		TypesInfo:         act.pkg.TypesInfo,
-		TypesSizes:        act.pkg.TypesSizes,
+		Analyzer:     act.a,
+		Fset:         act.pkg.Fset,
+		Files:        act.pkg.Syntax,
+		OtherFiles:   act.pkg.OtherFiles,
+		IgnoredFiles: act.pkg.IgnoredFiles,
+		Pkg:          act.pkg.Types,
+		TypesInfo:    act.pkg.TypesInfo,
+		TypesSizes:   act.pkg.TypesSizes,
+		TypeErrors:   act.pkg.TypeErrors,
+
 		ResultOf:          inputs,
 		Report:            func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) },
 		ImportObjectFact:  act.importObjectFact,
@@ -718,72 +718,6 @@ func (act *action) execOnce() {
 	}
 	act.pass = pass
 
-	var errors []types.Error
-	// Get any type errors that are attributed to the pkg.
-	// This is necessary to test analyzers that provide
-	// suggested fixes for compiler/type errors.
-	// TODO(adonovan): eliminate this hack;
-	// see https://github.com/golang/go/issues/54619.
-	for _, err := range act.pkg.Errors {
-		if err.Kind != packages.TypeError {
-			continue
-		}
-
-		// Parse err.Pos, a string of form: "file:line:col" or "file:line" or "" or "-"
-		// The filename may have a single ASCII letter Windows drive prefix such as "C:"
-		var file string
-		var line, col int
-		var convErr error
-		words := strings.Split(err.Pos, ":")
-		if runtime.GOOS == "windows" &&
-			len(words) > 2 &&
-			len(words[0]) == 1 &&
-			('A' <= words[0][0] && words[0][0] <= 'Z' ||
-				'a' <= words[0][0] && words[0][0] <= 'z') {
-			words[1] = words[0] + ":" + words[1]
-			words = words[1:]
-		}
-		switch len(words) {
-		case 2:
-			// file:line
-			file = words[0]
-			line, convErr = strconv.Atoi(words[1])
-		case 3:
-			// file:line:col
-			file = words[0]
-			line, convErr = strconv.Atoi(words[1])
-			if convErr == nil {
-				col, convErr = strconv.Atoi(words[2])
-			}
-		default:
-			continue
-		}
-		if convErr != nil {
-			continue
-		}
-
-		// Extract the token positions from the error string.
-		// (This is guesswork: Fset may contain all manner
-		// of stale files with the same name.)
-		offset := -1
-		act.pkg.Fset.Iterate(func(f *token.File) bool {
-			if f.Name() != file {
-				return true
-			}
-			offset = int(f.LineStart(line)) + col - 1
-			return false
-		})
-		if offset == -1 {
-			continue
-		}
-		errors = append(errors, types.Error{
-			Fset: act.pkg.Fset,
-			Msg:  err.Msg,
-			Pos:  token.Pos(offset),
-		})
-	}
-	analysisinternal.SetTypeErrors(pass, errors)
-
 	var err error
 	if act.pkg.IllTyped && !pass.Analyzer.RunDespiteErrors {
 		err = fmt.Errorf("analysis skipped due to errors in package")

go/analysis/unitchecker/unitchecker.go

@@ -340,6 +340,7 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re
 				Pkg:               pkg,
 				TypesInfo:         info,
 				TypesSizes:        tc.Sizes,
+				TypeErrors:        nil, // unitchecker doesn't RunDespiteErrors
 				ResultOf:          inputs,
 				Report:            func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) },
 				ImportObjectFact:  facts.ImportObjectFact,

go/packages/packages.go

@@ -303,6 +303,9 @@ type Package struct {
 	// of the package, or while parsing or type-checking its files.
 	Errors []Error
 
+	// TypeErrors contains the subset of errors produced during type checking.
+	TypeErrors []types.Error
+
 	// GoFiles lists the absolute file paths of the package's Go source files.
 	GoFiles []string
 
@@ -911,6 +914,7 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 
 		case types.Error:
 			// from type checker
+			lpkg.TypeErrors = append(lpkg.TypeErrors, err)
 			errs = append(errs, Error{
 				Pos:  err.Fset.Position(err.Pos).String(),
 				Msg:  err.Msg,

gopls/internal/lsp/analysis/fillreturns/fillreturns.go

@@ -52,9 +52,8 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		return nil, fmt.Errorf("nil TypeInfo")
 	}
 
-	errors := analysisinternal.GetTypeErrors(pass)
 outer:
-	for _, typeErr := range errors {
+	for _, typeErr := range pass.TypeErrors {
 		// Filter out the errors that are not relevant to this analyzer.
 		if !FixesError(typeErr) {
 			continue

gopls/internal/lsp/analysis/nonewvars/nonewvars.go

@@ -30,7 +30,7 @@ will turn into
 `
 
 var Analyzer = &analysis.Analyzer{
-	Name:             string(analysisinternal.NoNewVars),
+	Name:             "nonewvars",
 	Doc:              Doc,
 	Requires:         []*analysis.Analyzer{inspect.Analyzer},
 	Run:              run,
@@ -39,7 +39,6 @@ var Analyzer = &analysis.Analyzer{
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	errors := analysisinternal.GetTypeErrors(pass)
 
 	nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)}
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
@@ -60,7 +59,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			return
 		}
 
-		for _, err := range errors {
+		for _, err := range pass.TypeErrors {
 			if !FixesError(err.Msg) {
 				continue
 			}

gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go

@@ -29,7 +29,7 @@ will turn into
 `
 
 var Analyzer = &analysis.Analyzer{
-	Name:             string(analysisinternal.NoResultValues),
+	Name:             "noresultvalues",
 	Doc:              Doc,
 	Requires:         []*analysis.Analyzer{inspect.Analyzer},
 	Run:              run,
@@ -38,7 +38,6 @@ var Analyzer = &analysis.Analyzer{
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	errors := analysisinternal.GetTypeErrors(pass)
 
 	nodeFilter := []ast.Node{(*ast.ReturnStmt)(nil)}
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
@@ -55,7 +54,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			return
 		}
 
-		for _, err := range errors {
+		for _, err := range pass.TypeErrors {
 			if !FixesError(err.Msg) {
 				continue
 			}

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

@@ -35,7 +35,7 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
-	for _, err := range analysisinternal.GetTypeErrors(pass) {
+	for _, err := range pass.TypeErrors {
 		ifaceErr := strings.Contains(err.Msg, "missing method") || strings.HasPrefix(err.Msg, "cannot convert")
 		if !ifaceErr {
 			continue

gopls/internal/lsp/analysis/undeclaredname/undeclared.go

@@ -38,7 +38,7 @@ func <>(inferred parameters) {
 `
 
 var Analyzer = &analysis.Analyzer{
-	Name:             string(analysisinternal.UndeclaredName),
+	Name:             "undeclaredname",
 	Doc:              Doc,
 	Requires:         []*analysis.Analyzer{},
 	Run:              run,
@@ -49,7 +49,7 @@ var Analyzer = &analysis.Analyzer{
 var undeclaredNamePrefixes = []string{"undeclared name: ", "undefined: "}
 
 func run(pass *analysis.Pass) (interface{}, error) {
-	for _, err := range analysisinternal.GetTypeErrors(pass) {
+	for _, err := range pass.TypeErrors {
 		runForError(pass, err)
 	}
 	return nil, nil

gopls/internal/lsp/analysis/unusedvariable/unusedvariable.go

@@ -16,7 +16,6 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/ast/astutil"
-	"golang.org/x/tools/internal/analysisinternal"
 )
 
 const Doc = `check for unused variables
@@ -36,7 +35,7 @@ var Analyzer = &analysis.Analyzer{
 var unusedVariableSuffixes = []string{" declared and not used", " declared but not used"}
 
 func run(pass *analysis.Pass) (interface{}, error) {
-	for _, typeErr := range analysisinternal.GetTypeErrors(pass) {
+	for _, typeErr := range pass.TypeErrors {
 		for _, suffix := range unusedVariableSuffixes {
 			if strings.HasSuffix(typeErr.Msg, suffix) {
 				varName := strings.TrimSuffix(typeErr.Msg, suffix)

gopls/internal/lsp/cache/analysis.go

@@ -17,7 +17,6 @@ import (
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/span"
-	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/bug"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/event/tag"
@@ -291,6 +290,7 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a
 		Pkg:        pkg.GetTypes(),
 		TypesInfo:  pkg.GetTypesInfo(),
 		TypesSizes: pkg.GetTypesSizes(),
+		TypeErrors: pkg.typeErrors,
 		ResultOf:   inputs,
 		Report: func(d analysis.Diagnostic) {
 			// Prefix the diagnostic category with the analyzer's name.
@@ -351,7 +351,6 @@ func actionImpl(ctx context.Context, snapshot *snapshot, deps []*actionHandle, a
 			return facts
 		},
 	}
-	analysisinternal.SetTypeErrors(pass, pkg.typeErrors)
 
 	if (pkg.HasListOrParseErrors() || pkg.HasTypeErrors()) && !analyzer.RunDespiteErrors {
 		return nil, fmt.Errorf("skipping analysis %s because package %s contains errors", analyzer.Name, pkg.ID())

internal/analysisinternal/analysis.go

@@ -2,7 +2,8 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// Package analysisinternal exposes internal-only fields from go/analysis.
+// Package analysisinternal provides gopls' internal analyses with a
+// number of helper functions that operate on typed syntax trees.
 package analysisinternal
 
 import (
@@ -18,11 +19,6 @@ import (
 // in Go 1.18+.
 var DiagnoseFuzzTests bool = false
 
-var (
-	GetTypeErrors func(p interface{}) []types.Error
-	SetTypeErrors func(p interface{}, errors []types.Error)
-)
-
 func TypeErrorEndPos(fset *token.FileSet, src []byte, start token.Pos) token.Pos {
 	// Get the end position for the type error.
 	offset, end := fset.PositionFor(start, false).Offset, start
@@ -210,14 +206,6 @@ func TypeExpr(f *ast.File, pkg *types.Package, typ types.Type) ast.Expr {
 	}
 }
 
-type TypeErrorPass string
-
-const (
-	NoNewVars      TypeErrorPass = "nonewvars"
-	NoResultValues TypeErrorPass = "noresultvalues"
-	UndeclaredName TypeErrorPass = "undeclaredname"
-)
-
 // StmtToInsertVarBefore returns the ast.Stmt before which we can safely insert a new variable.
 // Some examples:
 //