From ce26db4201145d6f18df8e520311478e3de38a52 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 22 Aug 2022 15:21:17 -0400 Subject: [PATCH] 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 Reviewed-by: Robert Findley Auto-Submit: Alan Donovan Run-TryBot: Alan Donovan Reviewed-by: Tim King --- go/analysis/analysis.go | 15 +--- go/analysis/internal/checker/checker.go | 86 +++---------------- go/analysis/unitchecker/unitchecker.go | 1 + go/packages/packages.go | 4 + .../lsp/analysis/fillreturns/fillreturns.go | 3 +- .../lsp/analysis/nonewvars/nonewvars.go | 5 +- .../analysis/noresultvalues/noresultvalues.go | 5 +- .../lsp/analysis/stubmethods/stubmethods.go | 2 +- .../lsp/analysis/undeclaredname/undeclared.go | 4 +- .../analysis/unusedvariable/unusedvariable.go | 3 +- gopls/internal/lsp/cache/analysis.go | 3 +- internal/analysisinternal/analysis.go | 16 +--- 12 files changed, 29 insertions(+), 118 deletions(-) diff --git a/go/analysis/analysis.go b/go/analysis/analysis.go index d11505a165..44ada22a03 100644 --- a/go/analysis/analysis.go +++ b/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. diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index 2972e44155..cf76bdebe7 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -26,7 +26,6 @@ import ( "runtime/pprof" "runtime/trace" "sort" - "strconv" "strings" "sync" "time" @@ -34,7 +33,6 @@ import ( "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") diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go index d9c8f11cdd..6e6907d261 100644 --- a/go/analysis/unitchecker/unitchecker.go +++ b/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, diff --git a/go/packages/packages.go b/go/packages/packages.go index 54d880d206..046212347c 100644 --- a/go/packages/packages.go +++ b/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, diff --git a/gopls/internal/lsp/analysis/fillreturns/fillreturns.go b/gopls/internal/lsp/analysis/fillreturns/fillreturns.go index 4415ddd66e..c8146df2dd 100644 --- a/gopls/internal/lsp/analysis/fillreturns/fillreturns.go +++ b/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 diff --git a/gopls/internal/lsp/analysis/nonewvars/nonewvars.go b/gopls/internal/lsp/analysis/nonewvars/nonewvars.go index e7fa430cc5..2d2044eb39 100644 --- a/gopls/internal/lsp/analysis/nonewvars/nonewvars.go +++ b/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 } diff --git a/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go b/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go index b9f21f3135..b3fd84bcc9 100644 --- a/gopls/internal/lsp/analysis/noresultvalues/noresultvalues.go +++ b/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 } diff --git a/gopls/internal/lsp/analysis/stubmethods/stubmethods.go b/gopls/internal/lsp/analysis/stubmethods/stubmethods.go index f9dc69a965..effeb8a405 100644 --- a/gopls/internal/lsp/analysis/stubmethods/stubmethods.go +++ b/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 diff --git a/gopls/internal/lsp/analysis/undeclaredname/undeclared.go b/gopls/internal/lsp/analysis/undeclaredname/undeclared.go index fd8d35eb1b..742a4d265d 100644 --- a/gopls/internal/lsp/analysis/undeclaredname/undeclared.go +++ b/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 diff --git a/gopls/internal/lsp/analysis/unusedvariable/unusedvariable.go b/gopls/internal/lsp/analysis/unusedvariable/unusedvariable.go index 134cbb2c43..904016be71 100644 --- a/gopls/internal/lsp/analysis/unusedvariable/unusedvariable.go +++ b/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) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 43f1afbe89..e2c56a53c0 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/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()) diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go index 6fceef5e72..d15f0eb7ab 100644 --- a/internal/analysisinternal/analysis.go +++ b/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: //