Merge pull request #71 from golangci/feature/full-support-of-nolint-directives

#65, #68: make //nolint processing like in gometalinter

Author: jirfag

Diff for: 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 for: 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,53 +149,79 @@ 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, "/ ")
 			if !strings.HasPrefix(text, "nolint") {
 				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 for: 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 for: 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
+}