From 9fa9e2b3f88a0901308e52945ef031cb6869adcb Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Sat, 16 Jun 2018 10:40:03 +0300 Subject: [PATCH] Fix #106: fix transitive expanding of nolint: we could nolint more lines than needed --- pkg/commands/linters.go | 6 +- pkg/commands/root.go | 3 +- pkg/commands/run.go | 2 +- pkg/config/reader.go | 3 +- pkg/logutils/out.go | 9 +++ pkg/logutils/stderr_log.go | 1 + pkg/printers/checkstyle.go | 3 +- pkg/printers/json.go | 3 +- pkg/printers/tab.go | 4 +- pkg/printers/text.go | 8 +-- pkg/printers/utils.go | 5 -- pkg/result/processors/nolint.go | 67 ++++++++++++----------- pkg/result/processors/nolint_test.go | 24 ++++++++ pkg/result/processors/testdata/nolint2.go | 17 ++++++ 14 files changed, 103 insertions(+), 52 deletions(-) create mode 100644 pkg/logutils/out.go delete mode 100644 pkg/printers/utils.go create mode 100644 pkg/result/processors/testdata/nolint2.go diff --git a/pkg/commands/linters.go b/pkg/commands/linters.go index 032db3c4..f63b2553 100644 --- a/pkg/commands/linters.go +++ b/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) diff --git a/pkg/commands/root.go b/pkg/commands/root.go index 0716331d..61d97c4b 100644 --- a/pkg/commands/root.go +++ b/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) } diff --git a/pkg/commands/run.go b/pkg/commands/run.go index ef00dc7a..99c15ec5 100644 --- a/pkg/commands/run.go +++ b/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 diff --git a/pkg/config/reader.go b/pkg/config/reader.go index a5538c6f..a837ff48 100644 --- a/pkg/config/reader.go +++ b/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) } diff --git a/pkg/logutils/out.go b/pkg/logutils/out.go new file mode 100644 index 00000000..67c70dc8 --- /dev/null +++ b/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() diff --git a/pkg/logutils/stderr_log.go b/pkg/logutils/stderr_log.go index 08b8d77a..67b217bc 100644 --- a/pkg/logutils/stderr_log.go +++ b/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` } diff --git a/pkg/printers/checkstyle.go b/pkg/printers/checkstyle.go index 587b6f9f..8ba01a53 100644 --- a/pkg/printers/checkstyle.go +++ b/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 } diff --git a/pkg/printers/json.go b/pkg/printers/json.go index c3ec923d..49c36155 100644 --- a/pkg/printers/json.go +++ b/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 } diff --git a/pkg/printers/tab.go b/pkg/printers/tab.go index 7b0e4c04..976013d3 100644 --- a/pkg/printers/tab.go +++ b/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 { diff --git a/pkg/printers/text.go b/pkg/printers/text.go index c291265a..0f041c35 100644 --- a/pkg/printers/text.go +++ b/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, "^")) } diff --git a/pkg/printers/utils.go b/pkg/printers/utils.go deleted file mode 100644 index ace9f57a..00000000 --- a/pkg/printers/utils.go +++ /dev/null @@ -1,5 +0,0 @@ -package printers - -import "github.com/fatih/color" - -var StdOut = color.Output // https://github.com/golangci/golangci-lint/issues/14 diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index 3d195ad0..198ea272 100644 --- a/pkg/result/processors/nolint.go +++ b/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, - ranges: ignoredRanges(inlineRanges), + fset: fset, + 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 - ranges ignoredRanges + fset *token.FileSet + 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()) - 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 - }) + nodeStartPos := e.fset.Position(node.Pos()) + nodeStartLine := nodeStartPos.Line + nodeEndLine := e.fset.Position(node.End()).Line - 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 + var foundRange *ignoredRange + for _, r := range e.inlineRanges { + if r.To == nodeStartLine-1 && nodeStartPos.Column == r.col { + foundRange = &r + break } } + 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 } diff --git a/pkg/result/processors/nolint_test.go b/pkg/result/processors/nolint_test.go index 36299d89..27006b84 100644 --- a/pkg/result/processors/nolint_test.go +++ b/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) { diff --git a/pkg/result/processors/testdata/nolint2.go b/pkg/result/processors/testdata/nolint2.go new file mode 100644 index 00000000..b2f8a1f3 --- /dev/null +++ b/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")) +}