From adb6be78bb7758f8993e7a87499bef99551ae00a Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Mon, 11 Jun 2018 00:50:31 +0300 Subject: [PATCH] Fix #72: match more autogenerated files patterns. We skip all issues from autogenerated files. Also reuse AST parsing for nolint and autogenerated exclude processors: decrease processing time on golang source code from 3s to 800ms. --- pkg/commands/run.go | 10 +-- pkg/lint/astcache/astcache.go | 40 +++++++-- .../processors/autogenerated_exclude.go | 88 +++++++++++++++++++ .../processors/autogenerated_exclude_test.go | 88 +++++++++++++++++++ pkg/result/processors/nolint.go | 61 +++---------- pkg/result/processors/nolint_test.go | 23 +---- ...lint_autogenerated.go => autogenerated.go} | 0 ...ed_alt_hdr.go => autogenerated_alt_hdr.go} | 0 ..._alt_hdr2.go => autogenerated_alt_hdr2.go} | 0 test/run_test.go | 5 ++ test/testdata/autogenerated/gen.go | 4 + 11 files changed, 232 insertions(+), 87 deletions(-) create mode 100644 pkg/result/processors/autogenerated_exclude.go create mode 100644 pkg/result/processors/autogenerated_exclude_test.go rename pkg/result/processors/testdata/{nolint_autogenerated.go => autogenerated.go} (100%) rename pkg/result/processors/testdata/{nolint_autogenerated_alt_hdr.go => autogenerated_alt_hdr.go} (100%) rename pkg/result/processors/testdata/{nolint_autogenerated_alt_hdr2.go => autogenerated_alt_hdr2.go} (100%) create mode 100644 test/testdata/autogenerated/gen.go diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 328be40ffadd..92d417e6d32d 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -3,7 +3,6 @@ package commands import ( "context" "fmt" - "go/token" "io/ioutil" "log" "os" @@ -192,10 +191,6 @@ func (e *Executor) runAnalysis(ctx context.Context, args []string) (<-chan resul if len(excludePatterns) != 0 { excludeTotalPattern = fmt.Sprintf("(%s)", strings.Join(excludePatterns, "|")) } - fset := token.NewFileSet() - if lintCtx.Program != nil { - fset = lintCtx.Program.Fset - } skipFilesProcessor, err := processors.NewSkipFiles(e.cfg.Run.SkipFiles) if err != nil { @@ -204,12 +199,13 @@ func (e *Executor) runAnalysis(ctx context.Context, args []string) (<-chan resul runner := lint.SimpleRunner{ Processors: []processors.Processor{ - processors.NewPathPrettifier(), // must be before diff processor at least + processors.NewPathPrettifier(), // must be before diff, nolint and exclude autogenerated processor at least processors.NewCgo(), skipFilesProcessor, + processors.NewAutogeneratedExclude(lintCtx.ASTCache), processors.NewExclude(excludeTotalPattern), - processors.NewNolint(fset), + processors.NewNolint(lintCtx.ASTCache), processors.NewUniqByLine(), processors.NewDiff(e.cfg.Issues.Diff, e.cfg.Issues.DiffFromRevision, e.cfg.Issues.DiffPatchFilePath), diff --git a/pkg/lint/astcache/astcache.go b/pkg/lint/astcache/astcache.go index 9415652474bc..5415529f3698 100644 --- a/pkg/lint/astcache/astcache.go +++ b/pkg/lint/astcache/astcache.go @@ -24,10 +24,27 @@ type Cache struct { s []*File } +func NewCache() *Cache { + return &Cache{ + m: map[string]*File{}, + } +} + func (c Cache) Get(filename string) *File { return c.m[filepath.Clean(filename)] } +func (c Cache) GetOrParse(filename string) *File { + f := c.m[filename] + if f != nil { + return f + } + + logrus.Infof("Parse AST for file %s on demand", filename) + c.parseFile(filename, nil) + return c.m[filename] +} + func (c Cache) GetAllValidFiles() []*File { return c.s } @@ -79,6 +96,20 @@ func LoadFromProgram(prog *loader.Program) (*Cache, error) { return c, nil } +func (c *Cache) parseFile(filePath string, fset *token.FileSet) { + if fset == nil { + fset = token.NewFileSet() + } + + f, err := parser.ParseFile(fset, filePath, nil, parser.ParseComments) // comments needed by e.g. golint + c.m[filePath] = &File{ + F: f, + Fset: fset, + Err: err, + Name: filePath, + } +} + func LoadFromFiles(files []string) (*Cache, error) { c := &Cache{ m: map[string]*File{}, @@ -87,14 +118,7 @@ func LoadFromFiles(files []string) (*Cache, error) { fset := token.NewFileSet() for _, filePath := range files { filePath = filepath.Clean(filePath) - - f, err := parser.ParseFile(fset, filePath, nil, parser.ParseComments) // comments needed by e.g. golint - c.m[filePath] = &File{ - F: f, - Fset: fset, - Err: err, - Name: filePath, - } + c.parseFile(filePath, fset) } c.prepareValidFiles() diff --git a/pkg/result/processors/autogenerated_exclude.go b/pkg/result/processors/autogenerated_exclude.go new file mode 100644 index 000000000000..b8f48f5bc3cd --- /dev/null +++ b/pkg/result/processors/autogenerated_exclude.go @@ -0,0 +1,88 @@ +package processors + +import ( + "fmt" + "strings" + + "github.com/golangci/golangci-lint/pkg/lint/astcache" + "github.com/golangci/golangci-lint/pkg/result" +) + +type ageFileSummary struct { + isGenerated bool +} + +type ageFileSummaryCache map[string]*ageFileSummary + +type AutogeneratedExclude struct { + fileSummaryCache ageFileSummaryCache + astCache *astcache.Cache +} + +func NewAutogeneratedExclude(astCache *astcache.Cache) *AutogeneratedExclude { + return &AutogeneratedExclude{ + fileSummaryCache: ageFileSummaryCache{}, + astCache: astCache, + } +} + +var _ Processor = &AutogeneratedExclude{} + +func (p AutogeneratedExclude) Name() string { + return "autogenerated_exclude" +} + +func (p *AutogeneratedExclude) Process(issues []result.Issue) ([]result.Issue, error) { + return filterIssuesErr(issues, p.shouldPassIssue) +} + +func (p *AutogeneratedExclude) shouldPassIssue(i *result.Issue) (bool, error) { + fs, err := p.getOrCreateFileSummary(i) + if err != nil { + return false, err + } + + // don't report issues for autogenerated files + return !fs.isGenerated, nil +} + +// isGenerated reports whether the source file is generated code. +// Using a bit laxer rules than https://golang.org/s/generatedcode to +// match more generated code. See #48 and #72. +func isGeneratedFileByComment(doc string) bool { + const ( + genCodeGenerated = "code generated" + genDoNotEdit = "do not edit" + genAutoFile = "autogenerated file" // easyjson + ) + + markers := []string{genCodeGenerated, genDoNotEdit, genAutoFile} + doc = strings.ToLower(doc) + for _, marker := range markers { + if strings.Contains(doc, marker) { + return true + } + } + + return false +} + +func (p *AutogeneratedExclude) getOrCreateFileSummary(i *result.Issue) (*ageFileSummary, error) { + fs := p.fileSummaryCache[i.FilePath()] + if fs != nil { + return fs, nil + } + + fs = &ageFileSummary{} + p.fileSummaryCache[i.FilePath()] = fs + + f := p.astCache.GetOrParse(i.FilePath()) + if f.Err != nil { + return nil, fmt.Errorf("can't parse file %s: %s", i.FilePath(), f.Err) + } + + fs.isGenerated = isGeneratedFileByComment(f.F.Doc.Text()) + return fs, nil +} + +func (p AutogeneratedExclude) Finish() {} diff --git a/pkg/result/processors/autogenerated_exclude_test.go b/pkg/result/processors/autogenerated_exclude_test.go new file mode 100644 index 000000000000..a8820dcb8a82 --- /dev/null +++ b/pkg/result/processors/autogenerated_exclude_test.go @@ -0,0 +1,88 @@ +package processors + +import ( + "go/token" + "path/filepath" + "strings" + "testing" + + "github.com/golangci/golangci-lint/pkg/lint/astcache" + "github.com/golangci/golangci-lint/pkg/result" + "github.com/stretchr/testify/assert" +) + +func TestIsAutogeneratedDetection(t *testing.T) { + all := ` + // generated by stringer -type Pill pill.go; DO NOT EDIT + +// Code generated by "stringer -type Pill pill.go"; DO NOT EDIT + +// Code generated by vfsgen; DO NOT EDIT + +// Created by cgo -godefs - DO NOT EDIT + +/* Created by cgo - DO NOT EDIT. */ + +// Generated by stringer -i a.out.go -o anames.go -p ppc64 +// Do not edit. + +// DO NOT EDIT +// generated by: x86map -fmt=decoder ../x86.csv + +// DO NOT EDIT. +// Generate with: go run gen.go -full -output md5block.go + +// generated by "go run gen.go". DO NOT EDIT. + +// DO NOT EDIT. This file is generated by mksyntaxgo from the RE2 distribution. + +// GENERATED BY make_perl_groups.pl; DO NOT EDIT. + +// generated by mknacl.sh - do not edit + +// DO NOT EDIT ** This file was generated with the bake tool ** DO NOT EDIT // + +// Generated by running +// maketables --tables=all --data=http://www.unicode.org/Public/8.0.0/ucd/UnicodeData.txt --casefolding=http://www.unicode.org/Public/8.0.0/ucd/CaseFolding.txt +// DO NOT EDIT + +/* +* CODE GENERATED AUTOMATICALLY WITH github.com/ernesto-jimenez/gogen/unmarshalmap +* THIS FILE SHOULD NOT BE EDITED BY HAND +*/ + +// AUTOGENERATED FILE: easyjson file.go +` + + generatedCases := strings.Split(all, "\n\n") + for _, gc := range generatedCases { + isGenerated := isGeneratedFileByComment(gc) + assert.True(t, isGenerated) + } + + notGeneratedCases := []string{"code not generated by", "test"} + for _, ngc := range notGeneratedCases { + isGenerated := isGeneratedFileByComment(ngc) + assert.False(t, isGenerated) + } +} + +func TestNoIssuesInAutogeneratedFiles(t *testing.T) { + files := []string{ + "autogenerated.go", + "autogenerated_alt_hdr.go", + "autogenerated_alt_hdr2.go", + } + for _, file := range files { + t.Run(file, func(t *testing.T) { + i := result.Issue{ + Pos: token.Position{ + Filename: filepath.Join("testdata", file), + Line: 4, + }, + } + p := NewAutogeneratedExclude(astcache.NewCache()) + processAssertEmpty(t, p, i) + }) + } +} diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index f6788daa2d18..3d195ad0d343 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -1,16 +1,13 @@ package processors import ( - "bufio" - "bytes" "fmt" "go/ast" - "go/parser" "go/token" - "io/ioutil" "sort" "strings" + "github.com/golangci/golangci-lint/pkg/lint/astcache" "github.com/golangci/golangci-lint/pkg/result" ) @@ -44,20 +41,19 @@ func (i *ignoredRange) doesMatch(issue *result.Issue) bool { type fileData struct { ignoredRanges []ignoredRange - isGenerated bool } type filesCache map[string]*fileData type Nolint struct { - fset *token.FileSet - cache filesCache + cache filesCache + astCache *astcache.Cache } -func NewNolint(fset *token.FileSet) *Nolint { +func NewNolint(astCache *astcache.Cache) *Nolint { return &Nolint{ - fset: fset, - cache: filesCache{}, + cache: filesCache{}, + astCache: astCache, } } @@ -71,29 +67,6 @@ func (p *Nolint) Process(issues []result.Issue) ([]result.Issue, error) { return filterIssuesErr(issues, p.shouldPassIssue) } -var ( - genHdr = []byte("// Code generated") - genFtr = []byte("DO NOT EDIT") -) - -// isGenerated reports whether the source file is generated code. -// Using a bit laxer rules than https://golang.org/s/generatedcode to -// match more generated code. -func isGenerated(src []byte) bool { - sc := bufio.NewScanner(bytes.NewReader(src)) - var hdr, ftr bool - for sc.Scan() { - b := sc.Bytes() - if bytes.HasPrefix(b, genHdr) { - hdr = true - } - if bytes.Contains(b, genFtr) { - ftr = true - } - } - return hdr && ftr -} - func (p *Nolint) getOrCreateFileData(i *result.Issue) (*fileData, error) { fd := p.cache[i.FilePath()] if fd != nil { @@ -103,22 +76,12 @@ func (p *Nolint) getOrCreateFileData(i *result.Issue) (*fileData, error) { fd = &fileData{} p.cache[i.FilePath()] = fd - src, err := ioutil.ReadFile(i.FilePath()) - if err != nil { - return nil, fmt.Errorf("can't read file %s: %s", i.FilePath(), err) - } - - fd.isGenerated = isGenerated(src) - if fd.isGenerated { // don't report issues for autogenerated files - return fd, nil - } - - file, err := parser.ParseFile(p.fset, i.FilePath(), src, parser.ParseComments) - if err != nil { - return nil, fmt.Errorf("can't parse file %s", i.FilePath()) + file := p.astCache.GetOrParse(i.FilePath()) + if file.Err != nil { + return nil, fmt.Errorf("can't parse file %s: %s", i.FilePath(), file.Err) } - fd.ignoredRanges = buildIgnoredRangesForFile(file, p.fset) + fd.ignoredRanges = buildIgnoredRangesForFile(file.F, file.Fset) return fd, nil } @@ -145,10 +108,6 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) { return false, err } - if fd.isGenerated { // don't report issues for autogenerated files - return false, nil - } - for _, ir := range fd.ignoredRanges { if ir.doesMatch(i) { return false, nil diff --git a/pkg/result/processors/nolint_test.go b/pkg/result/processors/nolint_test.go index 203c7c81bdfd..de255fa2ba37 100644 --- a/pkg/result/processors/nolint_test.go +++ b/pkg/result/processors/nolint_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/golangci/golangci-lint/pkg/lint/astcache" "github.com/golangci/golangci-lint/pkg/result" "github.com/stretchr/testify/assert" ) @@ -20,7 +21,7 @@ func newNolintFileIssue(line int, fromLinter string) result.Issue { } func TestNolint(t *testing.T) { - p := NewNolint(token.NewFileSet()) + p := NewNolint(astcache.NewCache()) // test inline comments processAssertEmpty(t, p, newNolintFileIssue(3, "gofmt")) @@ -76,26 +77,6 @@ func TestNolint(t *testing.T) { } } -func TestNoIssuesInAutogeneratedFiles(t *testing.T) { - files := []string{ - "nolint_autogenerated.go", - "nolint_autogenerated_alt_hdr.go", - "nolint_autogenerated_alt_hdr2.go", - } - for _, file := range files { - t.Run(file, func(t *testing.T) { - i := result.Issue{ - Pos: token.Position{ - Filename: filepath.Join("testdata", file), - Line: 4, - }, - } - p := NewNolint(token.NewFileSet()) - processAssertEmpty(t, p, i) - }) - } -} - func TestIgnoredRangeMatches(t *testing.T) { var testcases = []struct { doc string diff --git a/pkg/result/processors/testdata/nolint_autogenerated.go b/pkg/result/processors/testdata/autogenerated.go similarity index 100% rename from pkg/result/processors/testdata/nolint_autogenerated.go rename to pkg/result/processors/testdata/autogenerated.go diff --git a/pkg/result/processors/testdata/nolint_autogenerated_alt_hdr.go b/pkg/result/processors/testdata/autogenerated_alt_hdr.go similarity index 100% rename from pkg/result/processors/testdata/nolint_autogenerated_alt_hdr.go rename to pkg/result/processors/testdata/autogenerated_alt_hdr.go diff --git a/pkg/result/processors/testdata/nolint_autogenerated_alt_hdr2.go b/pkg/result/processors/testdata/autogenerated_alt_hdr2.go similarity index 100% rename from pkg/result/processors/testdata/nolint_autogenerated_alt_hdr2.go rename to pkg/result/processors/testdata/autogenerated_alt_hdr2.go diff --git a/test/run_test.go b/test/run_test.go index 113b8f7af272..27cf4f38f0ea 100644 --- a/test/run_test.go +++ b/test/run_test.go @@ -37,6 +37,11 @@ func TestCongratsMessageIfNoIssues(t *testing.T) { checkNoIssuesRun(t, out, exitCode) } +func TestAutogeneratedNoIssues(t *testing.T) { + out, exitCode := runGolangciLint(t, filepath.Join(testdataDir, "autogenerated")) + checkNoIssuesRun(t, out, exitCode) +} + func TestSymlinkLoop(t *testing.T) { out, exitCode := runGolangciLint(t, filepath.Join(testdataDir, "symlink_loop", "...")) checkNoIssuesRun(t, out, exitCode) diff --git a/test/testdata/autogenerated/gen.go b/test/testdata/autogenerated/gen.go new file mode 100644 index 000000000000..749ed62587f5 --- /dev/null +++ b/test/testdata/autogenerated/gen.go @@ -0,0 +1,4 @@ +// DO NOT EDIT Code generated by something +package p + +func unusedFunc() {}