Fix #106: fix transitive expanding of nolint: we could nolint more lines than needed

pkg/commands/linters.go

@@ -8,7 +8,7 @@ import (
 	"github.com/fatih/color"
 	"github.com/golangci/golangci-lint/pkg/lint/linter"
 	"github.com/golangci/golangci-lint/pkg/lint/lintersdb"
-	"github.com/golangci/golangci-lint/pkg/printers"
+	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/spf13/cobra"
 )
 
@@ -23,7 +23,7 @@ func (e *Executor) initLinters() {
 
 func printLinterConfigs(lcs []linter.Config) {
 	for _, lc := range lcs {
-		fmt.Fprintf(printers.StdOut, "%s: %s [fast: %t]\n", color.YellowString(lc.Linter.Name()),
+		fmt.Fprintf(logutils.StdOut, "%s: %s [fast: %t]\n", color.YellowString(lc.Linter.Name()),
 			lc.Linter.Desc(), !lc.DoesFullImport)
 	}
 }
@@ -50,7 +50,7 @@ func (e Executor) executeLinters(cmd *cobra.Command, args []string) {
 		for _, lc := range linters {
 			linterNames = append(linterNames, lc.Linter.Name())
 		}
-		fmt.Fprintf(printers.StdOut, "%s: %s\n", color.YellowString(p), strings.Join(linterNames, ", "))
+		fmt.Fprintf(logutils.StdOut, "%s: %s\n", color.YellowString(p), strings.Join(linterNames, ", "))
 	}
 
 	os.Exit(0)

pkg/commands/root.go

@@ -8,14 +8,13 @@ import (
 
 	"github.com/golangci/golangci-lint/pkg/config"
 	"github.com/golangci/golangci-lint/pkg/logutils"
-	"github.com/golangci/golangci-lint/pkg/printers"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
 
 func (e *Executor) persistentPreRun(cmd *cobra.Command, args []string) {
 	if e.cfg.Run.PrintVersion {
-		fmt.Fprintf(printers.StdOut, "golangci-lint has version %s built from %s on %s\n", e.version, e.commit, e.date)
+		fmt.Fprintf(logutils.StdOut, "golangci-lint has version %s built from %s on %s\n", e.version, e.commit, e.date)
 		os.Exit(0)
 	}
 

pkg/commands/run.go

@@ -150,7 +150,7 @@ func (e *Executor) initRun() {
 	}
 	e.rootCmd.AddCommand(runCmd)
 
-	runCmd.SetOutput(printers.StdOut) // use custom output to properly color it in Windows terminals
+	runCmd.SetOutput(logutils.StdOut) // use custom output to properly color it in Windows terminals
 
 	fs := runCmd.Flags()
 	fs.SortFlags = false // sort them as they are defined here

pkg/config/reader.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/golangci/golangci-lint/pkg/fsutils"
 	"github.com/golangci/golangci-lint/pkg/logutils"
-	"github.com/golangci/golangci-lint/pkg/printers"
 	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
 )
@@ -81,7 +80,7 @@ func (r *FileReader) parseConfig() error {
 	}
 
 	if r.cfg.InternalTest { // just for testing purposes: to detect config file usage
-		fmt.Fprintln(printers.StdOut, "test")
+		fmt.Fprintln(logutils.StdOut, "test")
 		os.Exit(0)
 	}
 

pkg/logutils/out.go

@@ -0,0 +1,9 @@
+package logutils
+
+import (
+	"github.com/fatih/color"
+	colorable "github.com/mattn/go-colorable"
+)
+
+var StdOut = color.Output // https://github.com/golangci/golangci-lint/issues/14
+var StdErr = colorable.NewColorableStderr()

pkg/logutils/stderr_log.go

@@ -27,6 +27,7 @@ func NewStderrLog(name string) *StderrLog {
 
 	// control log level in logutils, not in logrus
 	sl.logger.SetLevel(logrus.DebugLevel)
+	sl.logger.Out = StdErr
 	sl.logger.Formatter = &logrus.TextFormatter{
 		DisableTimestamp: true, // `INFO[0007] msg` -> `INFO msg`
 	}

pkg/printers/checkstyle.go

@@ -5,6 +5,7 @@ import (
 	"encoding/xml"
 	"fmt"
 
+	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
@@ -73,6 +74,6 @@ func (Checkstyle) Print(ctx context.Context, issues <-chan result.Issue) (bool,
 		return false, err
 	}
 
-	fmt.Fprintf(StdOut, "%s%s\n", xml.Header, data)
+	fmt.Fprintf(logutils.StdOut, "%s%s\n", xml.Header, data)
 	return len(files) > 0, nil
 }

pkg/printers/json.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 
+	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
@@ -33,6 +34,6 @@ func (JSON) Print(ctx context.Context, issues <-chan result.Issue) (bool, error)
 		return false, err
 	}
 
-	fmt.Fprint(StdOut, string(outputJSON))
+	fmt.Fprint(logutils.StdOut, string(outputJSON))
 	return len(allIssues) != 0, nil
 }

pkg/printers/tab.go

@@ -29,7 +29,7 @@ func (p Tab) SprintfColored(ca color.Attribute, format string, args ...interface
 }
 
 func (p *Tab) Print(ctx context.Context, issues <-chan result.Issue) (bool, error) {
-	w := tabwriter.NewWriter(StdOut, 0, 0, 2, ' ', 0)
+	w := tabwriter.NewWriter(logutils.StdOut, 0, 0, 2, ' ', 0)
 
 	issuesN := 0
 	for i := range issues {
@@ -41,7 +41,7 @@ func (p *Tab) Print(ctx context.Context, issues <-chan result.Issue) (bool, erro
 		p.log.Infof("Found %d issues", issuesN)
 	} else if ctx.Err() == nil { // don't print "congrats" if timeouted
 		outStr := p.SprintfColored(color.FgGreen, "Congrats! No issues were found.")
-		fmt.Fprintln(StdOut, outStr)
+		fmt.Fprintln(logutils.StdOut, outStr)
 	}
 
 	if err := w.Flush(); err != nil {

pkg/printers/text.go

@@ -93,7 +93,7 @@ func (p *Text) Print(ctx context.Context, issues <-chan result.Issue) (bool, err
 		p.log.Infof("Found %d issues", issuesN)
 	} else if ctx.Err() == nil { // don't print "congrats" if timeouted
 		outStr := p.SprintfColored(color.FgGreen, "Congrats! No issues were found.")
-		fmt.Fprintln(StdOut, outStr)
+		fmt.Fprintln(logutils.StdOut, outStr)
 	}
 
 	return issuesN != 0, nil
@@ -108,7 +108,7 @@ func (p Text) printIssue(i *result.Issue) {
 	if i.Pos.Column != 0 {
 		pos += fmt.Sprintf(":%d", i.Pos.Column)
 	}
-	fmt.Fprintf(StdOut, "%s: %s\n", pos, text)
+	fmt.Fprintf(logutils.StdOut, "%s: %s\n", pos, text)
 }
 
 func (p Text) printIssuedLines(i *result.Issue, lines linesCache) {
@@ -126,7 +126,7 @@ func (p Text) printIssuedLines(i *result.Issue, lines linesCache) {
 		}
 
 		lineStr = string(bytes.Trim(lines[zeroIndexedLine], "\r"))
-		fmt.Fprintln(StdOut, lineStr)
+		fmt.Fprintln(logutils.StdOut, lineStr)
 	}
 }
 
@@ -149,5 +149,5 @@ func (p Text) printUnderLinePointer(i *result.Issue, line string) {
 		prefix += strings.Repeat(" ", spacesCount)
 	}
 
-	fmt.Fprintf(StdOut, "%s%s\n", prefix, p.SprintfColored(color.FgYellow, "^"))
+	fmt.Fprintf(logutils.StdOut, "%s%s\n", prefix, p.SprintfColored(color.FgYellow, "^"))
 }

pkg/printers/utils.go

@@ -1,5 +0,0 @@
-package printers
-
-import "github.com/fatih/color"
-
-var StdOut = color.Output // https://github.com/golangci/golangci-lint/issues/14

pkg/result/processors/nolint.go

@@ -4,23 +4,21 @@ import (
 	"fmt"
 	"go/ast"
 	"go/token"
-	"sort"
 	"strings"
 
 	"github.com/golangci/golangci-lint/pkg/lint/astcache"
+	"github.com/golangci/golangci-lint/pkg/logutils"
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
+var nolintDebugf = logutils.Debug("nolint")
+
 type ignoredRange struct {
 	linters []string
 	result.Range
 	col int
 }
 
-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
@@ -81,25 +79,31 @@ func (p *Nolint) getOrCreateFileData(i *result.Issue) (*fileData, error) {
 		return nil, fmt.Errorf("can't parse file %s: %s", i.FilePath(), file.Err)
 	}
 
-	fd.ignoredRanges = buildIgnoredRangesForFile(file.F, file.Fset)
+	fd.ignoredRanges = buildIgnoredRangesForFile(file.F, file.Fset, i.FilePath())
+	nolintDebugf("file %s: built nolint ranges are %+v", i.FilePath(), fd.ignoredRanges)
 	return fd, nil
 }
 
-func buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet) []ignoredRange {
+func buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, filePath string) []ignoredRange {
 	inlineRanges := extractFileCommentsInlineRanges(fset, f.Comments...)
+	nolintDebugf("file %s: inline nolint ranges are %+v", filePath, inlineRanges)
 
 	if len(inlineRanges) == 0 {
 		return nil
 	}
 
 	e := rangeExpander{
-		fset:   fset,
+		fset:         fset,
-		ranges: ignoredRanges(inlineRanges),
+		inlineRanges: inlineRanges,
 	}
 
 	ast.Walk(&e, f)
 
-	return e.ranges
+	// TODO: merge all ranges: there are repeated ranges
+	allRanges := append([]ignoredRange{}, inlineRanges...)
+	allRanges = append(allRanges, e.expandedRanges...)
+
+	return allRanges
 }
 
 func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
@@ -117,15 +121,10 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
 	return true, 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
+	fset           *token.FileSet
-	ranges ignoredRanges
+	inlineRanges   []ignoredRange
+	expandedRanges []ignoredRange
 }
 
 func (e *rangeExpander) Visit(node ast.Node) ast.Visitor {
@@ -133,22 +132,28 @@ func (e *rangeExpander) Visit(node ast.Node) ast.Visitor {
 		return e
 	}
 
-	startPos := e.fset.Position(node.Pos())
+	nodeStartPos := e.fset.Position(node.Pos())
-	start := startPos.Line
+	nodeStartLine := nodeStartPos.Line
-	end := e.fset.Position(node.End()).Line
+	nodeEndLine := 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) {
+	var foundRange *ignoredRange
-		r := &e.ranges[found]
+	for _, r := range e.inlineRanges {
-		if r.From > start {
+		if r.To == nodeStartLine-1 && nodeStartPos.Column == r.col {
-			r.From = start
+			foundRange = &r
-		}
+			break
-		if r.To < end {
-			r.To = end
 		}
 	}
+	if foundRange == nil {
+		return e
+	}
+
+	expandedRange := *foundRange
+	if expandedRange.To < nodeEndLine {
+		expandedRange.To = nodeEndLine
+	}
+	nolintDebugf("found range is %v for node %#v [%d;%d], expanded range is %v",
+		*foundRange, node, nodeStartLine, nodeEndLine, expandedRange)
+	e.expandedRanges = append(e.expandedRanges, expandedRange)
 
 	return e
 }

pkg/result/processors/nolint_test.go

@@ -21,6 +21,12 @@ func newNolintFileIssue(line int, fromLinter string) result.Issue {
 	}
 }
 
+func newNolint2FileIssue(line int, fromLinter string) result.Issue {
+	i := newNolintFileIssue(line, fromLinter)
+	i.Pos.Filename = filepath.Join("testdata", "nolint2.go")
+	return i
+}
+
 func TestNolint(t *testing.T) {
 	p := NewNolint(astcache.NewCache(logutils.NewStderrLog("")))
 
@@ -76,6 +82,24 @@ func TestNolint(t *testing.T) {
 	for i := 39; i <= 45; i++ {
 		processAssertEmpty(t, p, newNolintFileIssue(i, "any"))
 	}
+
+	// check bug with transitive expanding for next and next line
+	for i := 1; i <= 8; i++ {
+		processAssertSame(t, p, newNolint2FileIssue(i, "errcheck"))
+	}
+	for i := 9; i <= 10; i++ {
+		processAssertEmpty(t, p, newNolint2FileIssue(i, "errcheck"))
+	}
+
+	// check inline comment for function
+	for i := 11; i <= 13; i++ {
+		processAssertSame(t, p, newNolint2FileIssue(i, "errcheck"))
+	}
+	processAssertEmpty(t, p, newNolint2FileIssue(14, "errcheck"))
+	for i := 15; i <= 18; i++ {
+		processAssertSame(t, p, newNolint2FileIssue(i, "errcheck"))
+	}
+
 }
 
 func TestIgnoredRangeMatches(t *testing.T) {

pkg/result/processors/testdata/nolint2.go

@@ -0,0 +1,17 @@
+package testdata
+
+import (
+	"bytes"
+	"io"
+)
+
+func noTransitiveExpanding() {
+	//nolint:errcheck
+	var buf io.Writer = &bytes.Buffer{}
+	buf.Write([]byte("123"))
+}
+
+func nolintFuncByInlineCommentDoesNotWork() { //nolint:errcheck
+	var buf io.Writer = &bytes.Buffer{}
+	buf.Write([]byte("123"))
+}