Skip to content

Commit 9fa9e2b

Browse files
jirfagjirfag
committed
Fix golangci#106: fix transitive expanding of nolint: we could nolint more lines than needed
1 parent 7495c4d commit 9fa9e2b

File tree

14 files changed

+104
-53
lines changed

14 files changed

+104
-53
lines changed

‎pkg/commands/linters.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import (
88
"github.com/fatih/color"
99
"github.com/golangci/golangci-lint/pkg/lint/linter"
1010
"github.com/golangci/golangci-lint/pkg/lint/lintersdb"
11-
"github.com/golangci/golangci-lint/pkg/printers"
11+
"github.com/golangci/golangci-lint/pkg/logutils"
1212
"github.com/spf13/cobra"
1313
)
1414

@@ -23,7 +23,7 @@ func (e *Executor) initLinters() {
2323

2424
func printLinterConfigs(lcs []linter.Config) {
2525
for _, lc := range lcs {
26-
fmt.Fprintf(printers.StdOut, "%s: %s [fast: %t]\n", color.YellowString(lc.Linter.Name()),
26+
fmt.Fprintf(logutils.StdOut, "%s: %s [fast: %t]\n", color.YellowString(lc.Linter.Name()),
2727
lc.Linter.Desc(), !lc.DoesFullImport)
2828
}
2929
}
@@ -50,7 +50,7 @@ func (e Executor) executeLinters(cmd *cobra.Command, args []string) {
5050
for _, lc := range linters {
5151
linterNames = append(linterNames, lc.Linter.Name())
5252
}
53-
fmt.Fprintf(printers.StdOut, "%s: %s\n", color.YellowString(p), strings.Join(linterNames, ", "))
53+
fmt.Fprintf(logutils.StdOut, "%s: %s\n", color.YellowString(p), strings.Join(linterNames, ", "))
5454
}
5555

5656
os.Exit(0)

‎pkg/commands/root.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,13 @@ import (
88

99
"github.com/golangci/golangci-lint/pkg/config"
1010
"github.com/golangci/golangci-lint/pkg/logutils"
11-
"github.com/golangci/golangci-lint/pkg/printers"
1211
"github.com/spf13/cobra"
1312
"github.com/spf13/pflag"
1413
)
1514

1615
func (e *Executor) persistentPreRun(cmd *cobra.Command, args []string) {
1716
if e.cfg.Run.PrintVersion {
18-
fmt.Fprintf(printers.StdOut, "golangci-lint has version %s built from %s on %s\n", e.version, e.commit, e.date)
17+
fmt.Fprintf(logutils.StdOut, "golangci-lint has version %s built from %s on %s\n", e.version, e.commit, e.date)
1918
os.Exit(0)
2019
}
2120

‎pkg/commands/run.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ func (e *Executor) initRun() {
150150
}
151151
e.rootCmd.AddCommand(runCmd)
152152

153-
runCmd.SetOutput(printers.StdOut) // use custom output to properly color it in Windows terminals
153+
runCmd.SetOutput(logutils.StdOut) // use custom output to properly color it in Windows terminals
154154

155155
fs := runCmd.Flags()
156156
fs.SortFlags = false // sort them as they are defined here

‎pkg/config/reader.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88

99
"github.com/golangci/golangci-lint/pkg/fsutils"
1010
"github.com/golangci/golangci-lint/pkg/logutils"
11-
"github.com/golangci/golangci-lint/pkg/printers"
1211
"github.com/spf13/pflag"
1312
"github.com/spf13/viper"
1413
)
@@ -81,7 +80,7 @@ func (r *FileReader) parseConfig() error {
8180
}
8281

8382
if r.cfg.InternalTest { // just for testing purposes: to detect config file usage
84-
fmt.Fprintln(printers.StdOut, "test")
83+
fmt.Fprintln(logutils.StdOut, "test")
8584
os.Exit(0)
8685
}
8786

‎pkg/logutils/out.go

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package logutils
2+
3+
import (
4+
"github.com/fatih/color"
5+
colorable "github.com/mattn/go-colorable"
6+
)
7+
8+
var StdOut = color.Output // https://github.com/golangci/golangci-lint/issues/14
9+
var StdErr = colorable.NewColorableStderr()

‎pkg/logutils/stderr_log.go

+1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ func NewStderrLog(name string) *StderrLog {
2727

2828
// control log level in logutils, not in logrus
2929
sl.logger.SetLevel(logrus.DebugLevel)
30+
sl.logger.Out = StdErr
3031
sl.logger.Formatter = &logrus.TextFormatter{
3132
DisableTimestamp: true, // `INFO[0007] msg` -> `INFO msg`
3233
}

‎pkg/printers/checkstyle.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/xml"
66
"fmt"
77

8+
"github.com/golangci/golangci-lint/pkg/logutils"
89
"github.com/golangci/golangci-lint/pkg/result"
910
)
1011

@@ -73,6 +74,6 @@ func (Checkstyle) Print(ctx context.Context, issues <-chan result.Issue) (bool,
7374
return false, err
7475
}
7576

76-
fmt.Fprintf(StdOut, "%s%s\n", xml.Header, data)
77+
fmt.Fprintf(logutils.StdOut, "%s%s\n", xml.Header, data)
7778
return len(files) > 0, nil
7879
}

‎pkg/printers/json.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/json"
66
"fmt"
77

8+
"github.com/golangci/golangci-lint/pkg/logutils"
89
"github.com/golangci/golangci-lint/pkg/result"
910
)
1011

@@ -33,6 +34,6 @@ func (JSON) Print(ctx context.Context, issues <-chan result.Issue) (bool, error)
3334
return false, err
3435
}
3536

36-
fmt.Fprint(StdOut, string(outputJSON))
37+
fmt.Fprint(logutils.StdOut, string(outputJSON))
3738
return len(allIssues) != 0, nil
3839
}

‎pkg/printers/tab.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func (p Tab) SprintfColored(ca color.Attribute, format string, args ...interface
2929
}
3030

3131
func (p *Tab) Print(ctx context.Context, issues <-chan result.Issue) (bool, error) {
32-
w := tabwriter.NewWriter(StdOut, 0, 0, 2, ' ', 0)
32+
w := tabwriter.NewWriter(logutils.StdOut, 0, 0, 2, ' ', 0)
3333

3434
issuesN := 0
3535
for i := range issues {
@@ -41,7 +41,7 @@ func (p *Tab) Print(ctx context.Context, issues <-chan result.Issue) (bool, erro
4141
p.log.Infof("Found %d issues", issuesN)
4242
} else if ctx.Err() == nil { // don't print "congrats" if timeouted
4343
outStr := p.SprintfColored(color.FgGreen, "Congrats! No issues were found.")
44-
fmt.Fprintln(StdOut, outStr)
44+
fmt.Fprintln(logutils.StdOut, outStr)
4545
}
4646

4747
if err := w.Flush(); err != nil {

‎pkg/printers/text.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ func (p *Text) Print(ctx context.Context, issues <-chan result.Issue) (bool, err
9393
p.log.Infof("Found %d issues", issuesN)
9494
} else if ctx.Err() == nil { // don't print "congrats" if timeouted
9595
outStr := p.SprintfColored(color.FgGreen, "Congrats! No issues were found.")
96-
fmt.Fprintln(StdOut, outStr)
96+
fmt.Fprintln(logutils.StdOut, outStr)
9797
}
9898

9999
return issuesN != 0, nil
@@ -108,7 +108,7 @@ func (p Text) printIssue(i *result.Issue) {
108108
if i.Pos.Column != 0 {
109109
pos += fmt.Sprintf(":%d", i.Pos.Column)
110110
}
111-
fmt.Fprintf(StdOut, "%s: %s\n", pos, text)
111+
fmt.Fprintf(logutils.StdOut, "%s: %s\n", pos, text)
112112
}
113113

114114
func (p Text) printIssuedLines(i *result.Issue, lines linesCache) {
@@ -126,7 +126,7 @@ func (p Text) printIssuedLines(i *result.Issue, lines linesCache) {
126126
}
127127

128128
lineStr = string(bytes.Trim(lines[zeroIndexedLine], "\r"))
129-
fmt.Fprintln(StdOut, lineStr)
129+
fmt.Fprintln(logutils.StdOut, lineStr)
130130
}
131131
}
132132

@@ -149,5 +149,5 @@ func (p Text) printUnderLinePointer(i *result.Issue, line string) {
149149
prefix += strings.Repeat(" ", spacesCount)
150150
}
151151

152-
fmt.Fprintf(StdOut, "%s%s\n", prefix, p.SprintfColored(color.FgYellow, "^"))
152+
fmt.Fprintf(logutils.StdOut, "%s%s\n", prefix, p.SprintfColored(color.FgYellow, "^"))
153153
}

‎pkg/printers/utils.go

-5
This file was deleted.

‎pkg/result/processors/nolint.go

+37-32
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,21 @@ import (
44
"fmt"
55
"go/ast"
66
"go/token"
7-
"sort"
87
"strings"
98

109
"github.com/golangci/golangci-lint/pkg/lint/astcache"
10+
"github.com/golangci/golangci-lint/pkg/logutils"
1111
"github.com/golangci/golangci-lint/pkg/result"
1212
)
1313

14+
var nolintDebugf = logutils.Debug("nolint")
15+
1416
type ignoredRange struct {
1517
linters []string
1618
result.Range
1719
col int
1820
}
1921

20-
func (i *ignoredRange) isAdjacent(col, start int) bool {
21-
return col == i.col && i.To == start-1
22-
}
23-
2422
func (i *ignoredRange) doesMatch(issue *result.Issue) bool {
2523
if issue.Line() < i.From || issue.Line() > i.To {
2624
return false
@@ -81,25 +79,31 @@ func (p *Nolint) getOrCreateFileData(i *result.Issue) (*fileData, error) {
8179
return nil, fmt.Errorf("can't parse file %s: %s", i.FilePath(), file.Err)
8280
}
8381

84-
fd.ignoredRanges = buildIgnoredRangesForFile(file.F, file.Fset)
82+
fd.ignoredRanges = buildIgnoredRangesForFile(file.F, file.Fset, i.FilePath())
83+
nolintDebugf("file %s: built nolint ranges are %+v", i.FilePath(), fd.ignoredRanges)
8584
return fd, nil
8685
}
8786

88-
func buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet) []ignoredRange {
87+
func buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, filePath string) []ignoredRange {
8988
inlineRanges := extractFileCommentsInlineRanges(fset, f.Comments...)
89+
nolintDebugf("file %s: inline nolint ranges are %+v", filePath, inlineRanges)
9090

9191
if len(inlineRanges) == 0 {
9292
return nil
9393
}
9494

9595
e := rangeExpander{
96-
fset: fset,
97-
ranges: ignoredRanges(inlineRanges),
96+
fset: fset,
97+
inlineRanges: inlineRanges,
9898
}
9999

100100
ast.Walk(&e, f)
101101

102-
return e.ranges
102+
// TODO: merge all ranges: there are repeated ranges
103+
allRanges := append([]ignoredRange{}, inlineRanges...)
104+
allRanges = append(allRanges, e.expandedRanges...)
105+
106+
return allRanges
103107
}
104108

105109
func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
@@ -117,38 +121,39 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
117121
return true, nil
118122
}
119123

120-
type ignoredRanges []ignoredRange
121-
122-
func (ir ignoredRanges) Len() int { return len(ir) }
123-
func (ir ignoredRanges) Swap(i, j int) { ir[i], ir[j] = ir[j], ir[i] }
124-
func (ir ignoredRanges) Less(i, j int) bool { return ir[i].To < ir[j].To }
125-
126124
type rangeExpander struct {
127-
fset *token.FileSet
128-
ranges ignoredRanges
125+
fset *token.FileSet
126+
inlineRanges []ignoredRange
127+
expandedRanges []ignoredRange
129128
}
130129

131130
func (e *rangeExpander) Visit(node ast.Node) ast.Visitor {
132131
if node == nil {
133132
return e
134133
}
135134

136-
startPos := e.fset.Position(node.Pos())
137-
start := startPos.Line
138-
end := e.fset.Position(node.End()).Line
139-
found := sort.Search(len(e.ranges), func(i int) bool {
140-
return e.ranges[i].To+1 >= start
141-
})
142-
143-
if found < len(e.ranges) && e.ranges[found].isAdjacent(startPos.Column, start) {
144-
r := &e.ranges[found]
145-
if r.From > start {
146-
r.From = start
147-
}
148-
if r.To < end {
149-
r.To = end
135+
nodeStartPos := e.fset.Position(node.Pos())
136+
nodeStartLine := nodeStartPos.Line
137+
nodeEndLine := e.fset.Position(node.End()).Line
138+
139+
var foundRange *ignoredRange
140+
for _, r := range e.inlineRanges {
141+
if r.To == nodeStartLine-1 && nodeStartPos.Column == r.col {
142+
foundRange = &r
143+
break
150144
}
151145
}
146+
if foundRange == nil {
147+
return e
148+
}
149+
150+
expandedRange := *foundRange
151+
if expandedRange.To < nodeEndLine {
152+
expandedRange.To = nodeEndLine
153+
}
154+
nolintDebugf("found range is %v for node %#v [%d;%d], expanded range is %v",
155+
*foundRange, node, nodeStartLine, nodeEndLine, expandedRange)
156+
e.expandedRanges = append(e.expandedRanges, expandedRange)
152157

153158
return e
154159
}

‎pkg/result/processors/nolint_test.go

+24
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ func newNolintFileIssue(line int, fromLinter string) result.Issue {
2121
}
2222
}
2323

24+
func newNolint2FileIssue(line int, fromLinter string) result.Issue {
25+
i := newNolintFileIssue(line, fromLinter)
26+
i.Pos.Filename = filepath.Join("testdata", "nolint2.go")
27+
return i
28+
}
29+
2430
func TestNolint(t *testing.T) {
2531
p := NewNolint(astcache.NewCache(logutils.NewStderrLog("")))
2632

@@ -76,6 +82,24 @@ func TestNolint(t *testing.T) {
7682
for i := 39; i <= 45; i++ {
7783
processAssertEmpty(t, p, newNolintFileIssue(i, "any"))
7884
}
85+
86+
// check bug with transitive expanding for next and next line
87+
for i := 1; i <= 8; i++ {
88+
processAssertSame(t, p, newNolint2FileIssue(i, "errcheck"))
89+
}
90+
for i := 9; i <= 10; i++ {
91+
processAssertEmpty(t, p, newNolint2FileIssue(i, "errcheck"))
92+
}
93+
94+
// check inline comment for function
95+
for i := 11; i <= 13; i++ {
96+
processAssertSame(t, p, newNolint2FileIssue(i, "errcheck"))
97+
}
98+
processAssertEmpty(t, p, newNolint2FileIssue(14, "errcheck"))
99+
for i := 15; i <= 18; i++ {
100+
processAssertSame(t, p, newNolint2FileIssue(i, "errcheck"))
101+
}
102+
79103
}
80104

81105
func TestIgnoredRangeMatches(t *testing.T) {

‎pkg/result/processors/testdata/nolint2.go

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package testdata
2+
3+
import (
4+
"bytes"
5+
"io"
6+
)
7+
8+
func noTransitiveExpanding() {
9+
//nolint:errcheck
10+
var buf io.Writer = &bytes.Buffer{}
11+
buf.Write([]byte("123"))
12+
}
13+
14+
func nolintFuncByInlineCommentDoesNotWork() { //nolint:errcheck
15+
var buf io.Writer = &bytes.Buffer{}
16+
buf.Write([]byte("123"))
17+
}

0 commit comments

Comments
 (0)