From dba3907ff3bfd71638f51a63997b7b685478d1b3 Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Fri, 23 Nov 2018 18:23:24 +0300 Subject: [PATCH] improve typecheck errors parsing --- pkg/golinters/megacheck.go | 2 +- pkg/golinters/typecheck.go | 38 +++++-------------- pkg/lint/load.go | 2 +- pkg/packages/errors.go | 38 +++++++++++++++++++ pkg/packages/util.go | 26 +++++++------ .../processors/autogenerated_exclude.go | 5 +++ test/run_test.go | 3 +- 7 files changed, 72 insertions(+), 42 deletions(-) create mode 100644 pkg/packages/errors.go diff --git a/pkg/golinters/megacheck.go b/pkg/golinters/megacheck.go index 7b9d3ab4..f5bc4d54 100644 --- a/pkg/golinters/megacheck.go +++ b/pkg/golinters/megacheck.go @@ -88,7 +88,7 @@ func (m Megacheck) Run(ctx context.Context, lintCtx *linter.Context) ([]result.I var errors []packages.Error for _, p := range lintCtx.NotCompilingPackages { errPkgs = append(errPkgs, p.String()) - errors = append(errors, libpackages.ExtractErrors(p)...) + errors = append(errors, libpackages.ExtractErrors(p, lintCtx.ASTCache)...) } warnText := fmt.Sprintf("Can't run megacheck because of compilation errors in packages %s", diff --git a/pkg/golinters/typecheck.go b/pkg/golinters/typecheck.go index 400c8e11..fde1756b 100644 --- a/pkg/golinters/typecheck.go +++ b/pkg/golinters/typecheck.go @@ -2,12 +2,7 @@ package golinters import ( "context" - "fmt" - "go/token" - "strconv" - "strings" - "github.com/pkg/errors" "golang.org/x/tools/go/packages" "github.com/golangci/golangci-lint/pkg/lint/linter" @@ -26,44 +21,31 @@ func (TypeCheck) Desc() string { } func (lint TypeCheck) parseError(srcErr packages.Error) (*result.Issue, error) { - // file:line(:colon) - parts := strings.Split(srcErr.Pos, ":") - if len(parts) == 1 { - return nil, errors.New("no colons") - } - - file := parts[0] - line, err := strconv.Atoi(parts[1]) + pos, err := libpackages.ParseErrorPosition(srcErr.Pos) if err != nil { - return nil, fmt.Errorf("can't parse line number %q: %s", parts[1], err) - } - - var column int - if len(parts) == 3 { // no column - column, err = strconv.Atoi(parts[2]) - if err != nil { - return nil, errors.Wrapf(err, "failed to parse column from %q", parts[2]) - } + return nil, err } return &result.Issue{ - Pos: token.Position{ - Filename: file, - Line: line, - Column: column, - }, + Pos: *pos, Text: srcErr.Msg, FromLinter: lint.Name(), }, nil } func (lint TypeCheck) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { + uniqReportedIssues := map[string]bool{} + var res []result.Issue for _, pkg := range lintCtx.NotCompilingPackages { - errors := libpackages.ExtractErrors(pkg) + errors := libpackages.ExtractErrors(pkg, lintCtx.ASTCache) for _, err := range errors { i, perr := lint.parseError(err) if perr != nil { // failed to parse + if uniqReportedIssues[err.Msg] { + continue + } + uniqReportedIssues[err.Msg] = true lintCtx.Log.Errorf("typechecking error: %s", err.Msg) } else { res = append(res, *i) diff --git a/pkg/lint/load.go b/pkg/lint/load.go index d098d0ef..e7f25d70 100644 --- a/pkg/lint/load.go +++ b/pkg/lint/load.go @@ -334,7 +334,7 @@ func (cl ContextLoader) Load(ctx context.Context, linters []linter.Config) (*lin } else { for _, pkg := range pkgs { if pkg.IllTyped { - cl.log.Infof("Pkg %s errors: %v", pkg.ID, libpackages.ExtractErrors(pkg)) + cl.log.Infof("Pkg %s errors: %v", pkg.ID, libpackages.ExtractErrors(pkg, astCache)) } } } diff --git a/pkg/packages/errors.go b/pkg/packages/errors.go new file mode 100644 index 00000000..72fb8601 --- /dev/null +++ b/pkg/packages/errors.go @@ -0,0 +1,38 @@ +package packages + +import ( + "fmt" + "go/token" + "strconv" + "strings" + + "github.com/pkg/errors" +) + +func ParseErrorPosition(pos string) (*token.Position, error) { + // file:line(:colon) + parts := strings.Split(pos, ":") + if len(parts) == 1 { + return nil, errors.New("no colons") + } + + file := parts[0] + line, err := strconv.Atoi(parts[1]) + if err != nil { + return nil, fmt.Errorf("can't parse line number %q: %s", parts[1], err) + } + + var column int + if len(parts) == 3 { // no column + column, err = strconv.Atoi(parts[2]) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse column from %q", parts[2]) + } + } + + return &token.Position{ + Filename: file, + Line: line, + Column: column, + }, nil +} diff --git a/pkg/packages/util.go b/pkg/packages/util.go index 43968b51..804c2a19 100644 --- a/pkg/packages/util.go +++ b/pkg/packages/util.go @@ -3,10 +3,13 @@ package packages import ( "fmt" + "github.com/golangci/golangci-lint/pkg/lint/astcache" + "golang.org/x/tools/go/packages" ) -func ExtractErrors(pkg *packages.Package) []packages.Error { +//nolint:gocyclo +func ExtractErrors(pkg *packages.Package, astCache *astcache.Cache) []packages.Error { errors := extractErrorsImpl(pkg) if len(errors) == 0 { return errors @@ -22,17 +25,18 @@ func ExtractErrors(pkg *packages.Package) []packages.Error { uniqErrors = append(uniqErrors, err) } - if len(pkg.Errors) == 0 && len(pkg.GoFiles) != 0 { - // erorrs were extracted from deps and have at leat one file in package - for i := range uniqErrors { - // change pos to local file to properly process it by processors (properly read line etc) - uniqErrors[i].Msg = fmt.Sprintf("%s: %s", uniqErrors[i].Pos, uniqErrors[i].Msg) - uniqErrors[i].Pos = fmt.Sprintf("%s:1", pkg.GoFiles[0]) - } - } - - // some errors like "code in directory expects import" don't have Pos, set it here if len(pkg.GoFiles) != 0 { + // errors were extracted from deps and have at leat one file in package + for i := range uniqErrors { + errPos, parseErr := ParseErrorPosition(uniqErrors[i].Pos) + if parseErr != nil || astCache.Get(errPos.Filename) == nil { + // change pos to local file to properly process it by processors (properly read line etc) + uniqErrors[i].Msg = fmt.Sprintf("%s: %s", uniqErrors[i].Pos, uniqErrors[i].Msg) + uniqErrors[i].Pos = fmt.Sprintf("%s:1", pkg.GoFiles[0]) + } + } + + // some errors like "code in directory expects import" don't have Pos, set it here for i := range uniqErrors { err := &uniqErrors[i] if err.Pos == "" { diff --git a/pkg/result/processors/autogenerated_exclude.go b/pkg/result/processors/autogenerated_exclude.go index 7412f1c5..5b455fc1 100644 --- a/pkg/result/processors/autogenerated_exclude.go +++ b/pkg/result/processors/autogenerated_exclude.go @@ -42,6 +42,11 @@ func (p *AutogeneratedExclude) Process(issues []result.Issue) ([]result.Issue, e } func (p *AutogeneratedExclude) shouldPassIssue(i *result.Issue) (bool, error) { + if i.FromLinter == "typecheck" { + // don't hide typechecking errors in generated files: users expect to see why the project isn't compiling + return true, nil + } + fs, err := p.getOrCreateFileSummary(i) if err != nil { return false, err diff --git a/test/run_test.go b/test/run_test.go index 688336f4..837ec2d0 100644 --- a/test/run_test.go +++ b/test/run_test.go @@ -24,7 +24,8 @@ func TestEmptyDirRun(t *testing.T) { func TestNotExistingDirRun(t *testing.T) { testshared.NewLintRunner(t).Run(getTestDataDir("no_such_dir")). - ExpectHasIssue(`cannot find package \"./testdata/no_such_dir\"`) + ExpectExitCode(exitcodes.WarningInTest). + ExpectOutputContains(`cannot find package \"./testdata/no_such_dir\"`) } func TestSymlinkLoop(t *testing.T) {