From 8a9b3a514374b51ecc7e45a8f4d326a975289044 Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Wed, 6 Jun 2018 23:51:15 +0300 Subject: [PATCH] #65, #68: make //nolint processing like in gometalinter --- pkg/result/processors/cgo.go | 2 +- pkg/result/processors/nolint.go | 142 +++++++++++++----- pkg/result/processors/nolint_test.go | 98 ++++++++++++ pkg/result/processors/testdata/nolint.go | 38 +++++ .../testdata/nolint_autogenerated.go | 2 +- .../testdata/nolint_autogenerated_alt_hdr2.go | 2 +- third_party/gometalinter/LICENSE | 19 +++ 7 files changed, 261 insertions(+), 42 deletions(-) create mode 100644 third_party/gometalinter/LICENSE diff --git a/pkg/result/processors/cgo.go b/pkg/result/processors/cgo.go index 14189691e786..97499ea31991 100644 --- a/pkg/result/processors/cgo.go +++ b/pkg/result/processors/cgo.go @@ -23,7 +23,7 @@ func (p Cgo) Process(issues []result.Issue) ([]result.Issue, error) { return filterIssues(issues, func(i *result.Issue) bool { // some linters (.e.g gas, deadcode) return incorrect filepaths for cgo issues, // it breaks next processing, so skip them - return !strings.HasSuffix(i.FilePath(), "/C") + return i.FilePath() != "C" && !strings.HasSuffix(i.FilePath(), "/C") }), nil } diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index ec3e4e2e9506..f6788daa2d18 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -8,20 +8,45 @@ import ( "go/parser" "go/token" "io/ioutil" + "sort" "strings" "github.com/golangci/golangci-lint/pkg/result" ) -type comment struct { +type ignoredRange struct { linters []string - line int + result.Range + col int } -type fileComments []comment + +func (i *ignoredRange) isAdjacent(col, start int) bool { + return col == i.col && i.To == start-1 +} + +func (i *ignoredRange) doesMatch(issue *result.Issue) bool { + if issue.Line() < i.From || issue.Line() > i.To { + return false + } + + if len(i.linters) == 0 { + return true + } + + for _, l := range i.linters { + if l == issue.FromLinter { + return true + } + } + + return false +} + type fileData struct { - comments fileComments - isGenerated bool + ignoredRanges []ignoredRange + isGenerated bool } + type filesCache map[string]*fileData type Nolint struct { @@ -93,15 +118,28 @@ func (p *Nolint) getOrCreateFileData(i *result.Issue) (*fileData, error) { return nil, fmt.Errorf("can't parse file %s", i.FilePath()) } - fd.comments = extractFileComments(p.fset, file.Comments...) + fd.ignoredRanges = buildIgnoredRangesForFile(file, p.fset) return fd, nil } -func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) { - if i.FilePath() == "C" { - return false, nil +func buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet) []ignoredRange { + inlineRanges := extractFileCommentsInlineRanges(fset, f.Comments...) + + if len(inlineRanges) == 0 { + return nil } + e := rangeExpander{ + fset: fset, + ranges: ignoredRanges(inlineRanges), + } + + ast.Walk(&e, f) + + return e.ranges +} + +func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) { fd, err := p.getOrCreateFileData(i) if err != nil { return false, err @@ -111,27 +149,53 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) { return false, nil } - for _, comment := range fd.comments { - if comment.line != i.Line() { - continue + for _, ir := range fd.ignoredRanges { + if ir.doesMatch(i) { + return false, nil } + } - if len(comment.linters) == 0 { - return false, nil // skip all linters - } + return true, nil +} - for _, linter := range comment.linters { - if i.FromLinter == linter { - return false, nil - } +type ignoredRanges []ignoredRange + +func (ir ignoredRanges) Len() int { return len(ir) } +func (ir ignoredRanges) Swap(i, j int) { ir[i], ir[j] = ir[j], ir[i] } +func (ir ignoredRanges) Less(i, j int) bool { return ir[i].To < ir[j].To } + +type rangeExpander struct { + fset *token.FileSet + ranges ignoredRanges +} + +func (e *rangeExpander) Visit(node ast.Node) ast.Visitor { + if node == nil { + return e + } + + startPos := e.fset.Position(node.Pos()) + start := startPos.Line + end := e.fset.Position(node.End()).Line + found := sort.Search(len(e.ranges), func(i int) bool { + return e.ranges[i].To+1 >= start + }) + + if found < len(e.ranges) && e.ranges[found].isAdjacent(startPos.Column, start) { + r := &e.ranges[found] + if r.From > start { + r.From = start + } + if r.To < end { + r.To = end } } - return true, nil + return e } -func extractFileComments(fset *token.FileSet, comments ...*ast.CommentGroup) fileComments { - ret := fileComments{} +func extractFileCommentsInlineRanges(fset *token.FileSet, comments ...*ast.CommentGroup) []ignoredRange { + var ret []ignoredRange for _, g := range comments { for _, c := range g.List { text := strings.TrimLeft(c.Text, "/ ") @@ -139,25 +203,25 @@ func extractFileComments(fset *token.FileSet, comments ...*ast.CommentGroup) fil continue } - pos := fset.Position(g.Pos()) - if !strings.HasPrefix(text, "nolint:") { // ignore all linters - ret = append(ret, comment{ - line: pos.Line, - }) - continue - } - - // ignore specific linters var linters []string - text = strings.Split(text, "//")[0] // allow another comment after this comment - linterItems := strings.Split(strings.TrimPrefix(text, "nolint:"), ",") - for _, linter := range linterItems { - linterName := strings.TrimSpace(linter) // TODO: validate it here - linters = append(linters, linterName) - } - ret = append(ret, comment{ + if strings.HasPrefix(text, "nolint:") { + // ignore specific linters + text = strings.Split(text, "//")[0] // allow another comment after this comment + linterItems := strings.Split(strings.TrimPrefix(text, "nolint:"), ",") + for _, linter := range linterItems { + linterName := strings.TrimSpace(linter) // TODO: validate it here + linters = append(linters, linterName) + } + } // else ignore all linters + + pos := fset.Position(g.Pos()) + ret = append(ret, ignoredRange{ + Range: result.Range{ + From: pos.Line, + To: fset.Position(g.End()).Line, + }, + col: pos.Column, linters: linters, - line: pos.Line, }) } } diff --git a/pkg/result/processors/nolint_test.go b/pkg/result/processors/nolint_test.go index 22091db918d5..203c7c81bdfd 100644 --- a/pkg/result/processors/nolint_test.go +++ b/pkg/result/processors/nolint_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/golangci/golangci-lint/pkg/result" + "github.com/stretchr/testify/assert" ) func newNolintFileIssue(line int, fromLinter string) result.Issue { @@ -20,6 +21,8 @@ func newNolintFileIssue(line int, fromLinter string) result.Issue { func TestNolint(t *testing.T) { p := NewNolint(token.NewFileSet()) + + // test inline comments processAssertEmpty(t, p, newNolintFileIssue(3, "gofmt")) processAssertEmpty(t, p, newNolintFileIssue(3, "gofmt")) // check cached is ok processAssertSame(t, p, newNolintFileIssue(3, "gofmtA")) // check different name @@ -35,6 +38,42 @@ func TestNolint(t *testing.T) { processAssertEmpty(t, p, newNolintFileIssue(7, "any")) processAssertSame(t, p, newNolintFileIssue(1, "golint")) // no directive + + // test preceding comments + processAssertEmpty(t, p, newNolintFileIssue(10, "any")) // preceding comment for var + processAssertEmpty(t, p, newNolintFileIssue(9, "any")) // preceding comment for var itself + + processAssertSame(t, p, newNolintFileIssue(14, "any")) // preceding comment with extra \n + processAssertEmpty(t, p, newNolintFileIssue(12, "any")) // preceding comment with extra \n itself + + processAssertSame(t, p, newNolintFileIssue(17, "any")) // preceding comment on different column + processAssertEmpty(t, p, newNolintFileIssue(16, "any")) // preceding comment on different column itself + + // preceding comment for func name and comment itself + for i := 19; i <= 23; i++ { + processAssertEmpty(t, p, newNolintFileIssue(i, "any")) + } + + processAssertSame(t, p, newNolintFileIssue(24, "any")) // right after func + + // preceding multiline comment: last line + for i := 25; i <= 30; i++ { + processAssertEmpty(t, p, newNolintFileIssue(i, "any")) + } + + processAssertSame(t, p, newNolintFileIssue(31, "any")) // between funcs + + // preceding multiline comment: first line + for i := 32; i <= 37; i++ { + processAssertEmpty(t, p, newNolintFileIssue(i, "any")) + } + + processAssertSame(t, p, newNolintFileIssue(38, "any")) // between funcs + + // preceding multiline comment: medium line + for i := 39; i <= 45; i++ { + processAssertEmpty(t, p, newNolintFileIssue(i, "any")) + } } func TestNoIssuesInAutogeneratedFiles(t *testing.T) { @@ -56,3 +95,62 @@ func TestNoIssuesInAutogeneratedFiles(t *testing.T) { }) } } + +func TestIgnoredRangeMatches(t *testing.T) { + var testcases = []struct { + doc string + issue result.Issue + linters []string + expected bool + }{ + { + doc: "unmatched line", + issue: result.Issue{ + Pos: token.Position{ + Line: 100, + }, + }, + }, + { + doc: "matched line, all linters", + issue: result.Issue{ + Pos: token.Position{ + Line: 5, + }, + }, + expected: true, + }, + { + doc: "matched line, unmatched linter", + issue: result.Issue{ + Pos: token.Position{ + Line: 5, + }, + }, + linters: []string{"vet"}, + }, + { + doc: "matched line and linters", + issue: result.Issue{ + Pos: token.Position{ + Line: 20, + }, + FromLinter: "vet", + }, + linters: []string{"vet"}, + expected: true, + }, + } + + for _, testcase := range testcases { + ir := ignoredRange{ + col: 20, + Range: result.Range{ + From: 5, + To: 20, + }, + linters: testcase.linters, + } + assert.Equal(t, testcase.expected, ir.doesMatch(&testcase.issue), testcase.doc) + } +} diff --git a/pkg/result/processors/testdata/nolint.go b/pkg/result/processors/testdata/nolint.go index 9adf9c77f5f8..ae67f8b8e61d 100644 --- a/pkg/result/processors/testdata/nolint.go +++ b/pkg/result/processors/testdata/nolint.go @@ -5,3 +5,41 @@ var nolintSpace int // nolint: gofmt var nolintSpaces int //nolint: gofmt, govet var nolintAll int // nolint var nolintAndAppendix int // nolint // another comment + +//nolint +var nolintVarByPrecedingComment int + +//nolint + +var dontNolintVarByPrecedingCommentBecauseOfNewLine int + +var nolintPrecedingVar string //nolint +var dontNolintVarByPrecedingCommentBecauseOfDifferentColumn int + +//nolint +func nolintFuncByPrecedingComment() *string { + xv := "v" + return &xv +} + +//nolint +// second line +func nolintFuncByPrecedingMultilineComment1() *string { + xv := "v" + return &xv +} + +// first line +//nolint +func nolintFuncByPrecedingMultilineComment2() *string { + xv := "v" + return &xv +} + +// first line +//nolint +// third line +func nolintFuncByPrecedingMultilineComment3() *string { + xv := "v" + return &xv +} diff --git a/pkg/result/processors/testdata/nolint_autogenerated.go b/pkg/result/processors/testdata/nolint_autogenerated.go index 110b77c4694e..d4878924040b 100644 --- a/pkg/result/processors/testdata/nolint_autogenerated.go +++ b/pkg/result/processors/testdata/nolint_autogenerated.go @@ -1,4 +1,4 @@ // Code generated by ... DO NOT EDIT. package testdata -var v int +var vvv int diff --git a/pkg/result/processors/testdata/nolint_autogenerated_alt_hdr2.go b/pkg/result/processors/testdata/nolint_autogenerated_alt_hdr2.go index 7b973d119116..8739d8daae75 100644 --- a/pkg/result/processors/testdata/nolint_autogenerated_alt_hdr2.go +++ b/pkg/result/processors/testdata/nolint_autogenerated_alt_hdr2.go @@ -5,4 +5,4 @@ // DO NOT EDIT! package testdata -var v int +var vv int diff --git a/third_party/gometalinter/LICENSE b/third_party/gometalinter/LICENSE new file mode 100644 index 000000000000..9b2886e6c312 --- /dev/null +++ b/third_party/gometalinter/LICENSE @@ -0,0 +1,19 @@ +Copyright (C) 2012 Alec Thomas + +Permission is hereby granted, free of charge, to any person obtaining a copy of +this software and associated documentation files (the "Software"), to deal in +the Software without restriction, including without limitation the rights to +use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies +of the Software, and to permit persons to whom the Software is furnished to do +so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. \ No newline at end of file