diff --git a/.golangci.yml b/.golangci.yml index e777c2e3..ea925e4a 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -43,15 +43,15 @@ run: skip-dirs: - test/testdata_etc -# golangci.com configuration -# https://github.com/golangci/golangci/wiki/Configuration -service: - golangci-lint-version: 1.13.x # use fixed version to not introduce new linters unexpectedly - prepare: - - echo "here I can run custom commands, but no preparation needed" - issues: exclude-rules: - text: "weak cryptographic primitive" linters: - - gosec \ No newline at end of file + - gosec + +# golangci.com configuration +# https://github.com/golangci/golangci/wiki/Configuration +service: + golangci-lint-version: 1.14.x # use the fixed version to not introduce new linters unexpectedly + prepare: + - echo "here I can run custom commands, but no preparation needed" diff --git a/README.md b/README.md index f19f63b2..7f791155 100644 --- a/README.md +++ b/README.md @@ -185,16 +185,16 @@ GolangCI-Lint can be used with zero configuration. By default the following lint ```bash $ golangci-lint help linters Enabled by default linters: -govet (vet, vetshadow): Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: true] -errcheck: Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: true] -staticcheck: Staticcheck is a go vet on steroids, applying a ton of static analysis checks [fast: false] -unused: Checks Go code for unused constants, variables, functions and types [fast: false] -gosimple: Linter for Go source code that specializes in simplifying a code [fast: false] -structcheck: Finds an unused struct fields [fast: true] -varcheck: Finds unused global variables and constants [fast: true] -ineffassign: Detects when assignments to existing variables are not used [fast: true] -deadcode: Finds unused code [fast: true] -typecheck: Like the front-end of a Go compiler, parses and type-checks Go code [fast: true] +govet (vet, vetshadow): Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: true, auto-fix: false] +errcheck: Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: true, auto-fix: false] +staticcheck: Staticcheck is a go vet on steroids, applying a ton of static analysis checks [fast: false, auto-fix: false] +unused: Checks Go code for unused constants, variables, functions and types [fast: false, auto-fix: false] +gosimple: Linter for Go source code that specializes in simplifying a code [fast: false, auto-fix: false] +structcheck: Finds an unused struct fields [fast: true, auto-fix: false] +varcheck: Finds unused global variables and constants [fast: true, auto-fix: false] +ineffassign: Detects when assignments to existing variables are not used [fast: true, auto-fix: false] +deadcode: Finds unused code [fast: true, auto-fix: false] +typecheck: Like the front-end of a Go compiler, parses and type-checks Go code [fast: true, auto-fix: false] ``` and the following linters are disabled by default: @@ -203,27 +203,27 @@ and the following linters are disabled by default: $ golangci-lint help linters ... Disabled by default linters: -golint: Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: true] -stylecheck: Stylecheck is a replacement for golint [fast: false] -gosec (gas): Inspects source code for security problems [fast: true] -interfacer: Linter that suggests narrower interface types [fast: false] -unconvert: Remove unnecessary type conversions [fast: true] -dupl: Tool for code clone detection [fast: true] -goconst: Finds repeated strings that could be replaced by a constant [fast: true] -gocyclo: Computes and checks the cyclomatic complexity of functions [fast: true] -gofmt: Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true] -goimports: Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true] -maligned: Tool to detect Go structs that would take less memory if their fields were sorted [fast: true] -depguard: Go linter that checks if package imports are in a list of acceptable packages [fast: true] -misspell: Finds commonly misspelled English words in comments [fast: true] -lll: Reports long lines [fast: true] -unparam: Reports unused function parameters [fast: false] -nakedret: Finds naked returns in functions greater than a specified function length [fast: true] -prealloc: Finds slice declarations that could potentially be preallocated [fast: true] -scopelint: Scopelint checks for unpinned variables in go programs [fast: true] -gocritic: The most opinionated Go source code linter [fast: true] -gochecknoinits: Checks that no init functions are present in Go code [fast: true] -gochecknoglobals: Checks that no globals are present in Go code [fast: true] +golint: Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: true, auto-fix: false] +stylecheck: Stylecheck is a replacement for golint [fast: false, auto-fix: false] +gosec (gas): Inspects source code for security problems [fast: true, auto-fix: false] +interfacer: Linter that suggests narrower interface types [fast: false, auto-fix: false] +unconvert: Remove unnecessary type conversions [fast: true, auto-fix: false] +dupl: Tool for code clone detection [fast: true, auto-fix: false] +goconst: Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false] +gocyclo: Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false] +gofmt: Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true] +goimports: Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true] +maligned: Tool to detect Go structs that would take less memory if their fields were sorted [fast: true, auto-fix: false] +depguard: Go linter that checks if package imports are in a list of acceptable packages [fast: true, auto-fix: false] +misspell: Finds commonly misspelled English words in comments [fast: true, auto-fix: true] +lll: Reports long lines [fast: true, auto-fix: false] +unparam: Reports unused function parameters [fast: false, auto-fix: false] +nakedret: Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false] +prealloc: Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false] +scopelint: Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false] +gocritic: The most opinionated Go source code linter [fast: true, auto-fix: false] +gochecknoinits: Checks that no init functions are present in Go code [fast: true, auto-fix: false] +gochecknoglobals: Checks that no globals are present in Go code [fast: true, auto-fix: false] ``` Pass `-E/--enable` to enable linter and `-D/--disable` to disable: @@ -499,6 +499,7 @@ Flags: For CI setups, prefer --new-from-rev=HEAD~, as --new can skip linting the current patch if any scripts generate unstaged files before golangci-lint runs. --new-from-rev REV Show only new issues created after git revision REV --new-from-patch PATH Show only new issues created in git patch with file path PATH + --fix Fix found issues (if it's supported by the linter) -h, --help help for run Global Flags: @@ -829,18 +830,18 @@ run: skip-dirs: - test/testdata_etc -# golangci.com configuration -# https://github.com/golangci/golangci/wiki/Configuration -service: - golangci-lint-version: 1.13.x # use fixed version to not introduce new linters unexpectedly - prepare: - - echo "here I can run custom commands, but no preparation needed" - issues: exclude-rules: - text: "weak cryptographic primitive" linters: - gosec + +# golangci.com configuration +# https://github.com/golangci/golangci/wiki/Configuration +service: + golangci-lint-version: 1.14.x # use the fixed version to not introduce new linters unexpectedly + prepare: + - echo "here I can run custom commands, but no preparation needed" ``` ## False Positives diff --git a/pkg/commands/help.go b/pkg/commands/help.go index d1e1bc11..39b2ef81 100644 --- a/pkg/commands/help.go +++ b/pkg/commands/help.go @@ -41,8 +41,8 @@ func printLinterConfigs(lcs []*linter.Config) { if len(lc.AlternativeNames) != 0 { altNamesStr = fmt.Sprintf(" (%s)", strings.Join(lc.AlternativeNames, ", ")) } - fmt.Fprintf(logutils.StdOut, "%s%s: %s [fast: %t]\n", color.YellowString(lc.Name()), - altNamesStr, lc.Linter.Desc(), !lc.NeedsSSARepr) + fmt.Fprintf(logutils.StdOut, "%s%s: %s [fast: %t, auto-fix: %t]\n", color.YellowString(lc.Name()), + altNamesStr, lc.Linter.Desc(), !lc.NeedsSSARepr, lc.CanAutoFix) } } diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 3bb569a5..e262711c 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -10,6 +10,8 @@ import ( "strings" "time" + "github.com/golangci/golangci-lint/pkg/result/processors" + "github.com/fatih/color" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -177,7 +179,7 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config, m *lintersdb.Manager, is wh("Show only new issues created after git revision `REV`")) fs.StringVar(&ic.DiffPatchFilePath, "new-from-patch", "", wh("Show only new issues created in git patch with file path `PATH`")) - + fs.BoolVar(&ic.NeedFix, "fix", false, "Fix found issues (if it's supported by the linter)") } func (e *Executor) initRunConfiguration(cmd *cobra.Command) { @@ -281,7 +283,9 @@ func (e *Executor) runAnalysis(ctx context.Context, args []string) (<-chan resul return nil, err } - return runner.Run(ctx, enabledLinters, lintCtx), nil + issuesCh := runner.Run(ctx, enabledLinters, lintCtx) + fixer := processors.NewFixer(e.cfg, e.log) + return fixer.Process(issuesCh), nil } func (e *Executor) setOutputToDevNull() (savedStdout, savedStderr *os.File) { diff --git a/pkg/config/config.go b/pkg/config/config.go index 83b7b581..66a9a996 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -278,6 +278,8 @@ type Issues struct { DiffFromRevision string `mapstructure:"new-from-rev"` DiffPatchFilePath string `mapstructure:"new-from-patch"` Diff bool `mapstructure:"new"` + + NeedFix bool `mapstructure:"fix"` } type Config struct { //nolint:maligned diff --git a/pkg/golinters/gofmt.go b/pkg/golinters/gofmt.go index 139c8a75..a2399cb9 100644 --- a/pkg/golinters/gofmt.go +++ b/pkg/golinters/gofmt.go @@ -5,9 +5,11 @@ import ( "context" "fmt" "go/token" + "strings" gofmtAPI "github.com/golangci/gofmt/gofmt" goimportsAPI "github.com/golangci/gofmt/goimports" + "github.com/pkg/errors" "golang.org/x/tools/imports" diffpkg "sourcegraph.com/sourcegraph/go-diff/diff" @@ -37,31 +39,204 @@ func (g Gofmt) Desc() string { "this tool runs with -s option to check for code simplification" } -func getFirstDeletedAndAddedLineNumberInHunk(h *diffpkg.Hunk) (firstDeleted, firstAdded int, err error) { - lines := bytes.Split(h.Body, []byte{'\n'}) - lineNumber := int(h.OrigStartLine - 1) - firstAddedLineNumber := -1 - for _, line := range lines { - lineNumber++ +type Change struct { + LineRange result.Range + Replacement result.Replacement +} - if len(line) == 0 { - continue +type diffLineType string + +const ( + diffLineAdded diffLineType = "added" + diffLineOriginal diffLineType = "original" + diffLineDeleted diffLineType = "deleted" +) + +type diffLine struct { + originalNumber int // 1-based original line number + typ diffLineType + data string // "+" or "-" stripped line +} + +type hunkChangesParser struct { + // needed because we merge currently added lines with the last original line + lastOriginalLine *diffLine + + // if the first line of diff is an adding we save all additions to replacementLinesToPrepend + replacementLinesToPrepend []string + + log logutils.Log + + lines []diffLine + + ret []Change +} + +func (p *hunkChangesParser) parseDiffLines(h *diffpkg.Hunk) { + lines := bytes.Split(h.Body, []byte{'\n'}) + currentOriginalLineNumer := int(h.OrigStartLine) + var ret []diffLine + + for i, line := range lines { + dl := diffLine{ + originalNumber: currentOriginalLineNumer, } - if line[0] == '+' && firstAddedLineNumber == -1 { - firstAddedLineNumber = lineNumber - } - if line[0] == '-' { - return lineNumber, firstAddedLineNumber, nil + + lineStr := string(line) + + //nolint:gocritic + if strings.HasPrefix(lineStr, "-") { + dl.typ = diffLineDeleted + dl.data = strings.TrimPrefix(lineStr, "-") + currentOriginalLineNumer++ + } else if strings.HasPrefix(lineStr, "+") { + dl.typ = diffLineAdded + dl.data = strings.TrimPrefix(lineStr, "+") + } else { + if i == len(lines)-1 && lineStr == "" { + // handle last \n: don't add an empty original line + break + } + + dl.typ = diffLineOriginal + dl.data = strings.TrimPrefix(lineStr, " ") + currentOriginalLineNumer++ } + + ret = append(ret, dl) } - return 0, firstAddedLineNumber, fmt.Errorf("didn't find deletion line in hunk %s", string(h.Body)) + p.lines = ret +} + +func (p *hunkChangesParser) handleOriginalLine(line diffLine, i *int) { + if len(p.replacementLinesToPrepend) == 0 { + p.lastOriginalLine = &line + *i++ + return + } + + // check following added lines for the case: + // + added line 1 + // original line + // + added line 2 + + *i++ + var followingAddedLines []string + for ; *i < len(p.lines) && p.lines[*i].typ == diffLineAdded; *i++ { + followingAddedLines = append(followingAddedLines, p.lines[*i].data) + } + + p.ret = append(p.ret, Change{ + LineRange: result.Range{ + From: line.originalNumber, + To: line.originalNumber, + }, + Replacement: result.Replacement{ + NewLines: append(p.replacementLinesToPrepend, append([]string{line.data}, followingAddedLines...)...), + }, + }) + p.replacementLinesToPrepend = nil + p.lastOriginalLine = &line +} + +func (p *hunkChangesParser) handleDeletedLines(deletedLines []diffLine, addedLines []string) { + change := Change{ + LineRange: result.Range{ + From: deletedLines[0].originalNumber, + To: deletedLines[len(deletedLines)-1].originalNumber, + }, + } + + if len(addedLines) != 0 { + //nolint:gocritic + change.Replacement.NewLines = append(p.replacementLinesToPrepend, addedLines...) + if len(p.replacementLinesToPrepend) != 0 { + p.replacementLinesToPrepend = nil + } + + p.ret = append(p.ret, change) + return + } + + // delete-only change with possible prepending + if len(p.replacementLinesToPrepend) != 0 { + change.Replacement.NewLines = p.replacementLinesToPrepend + p.replacementLinesToPrepend = nil + } else { + change.Replacement.NeedOnlyDelete = true + } + + p.ret = append(p.ret, change) +} + +func (p *hunkChangesParser) handleAddedOnlyLines(addedLines []string) { + if p.lastOriginalLine == nil { + // the first line is added; the diff looks like: + // 1. + ... + // 2. - ... + // or + // 1. + ... + // 2. ... + + p.replacementLinesToPrepend = addedLines + return + } + + // add-only change merged into the last original line with possible prepending + p.ret = append(p.ret, Change{ + LineRange: result.Range{ + From: p.lastOriginalLine.originalNumber, + To: p.lastOriginalLine.originalNumber, + }, + Replacement: result.Replacement{ + NewLines: append(p.replacementLinesToPrepend, append([]string{p.lastOriginalLine.data}, addedLines...)...), + }, + }) + p.replacementLinesToPrepend = nil +} + +func (p *hunkChangesParser) parse(h *diffpkg.Hunk) []Change { + p.parseDiffLines(h) + + for i := 0; i < len(p.lines); { + line := p.lines[i] + if line.typ == diffLineOriginal { + p.handleOriginalLine(line, &i) //nolint:scopelint + continue + } + + var deletedLines []diffLine + for ; i < len(p.lines) && p.lines[i].typ == diffLineDeleted; i++ { + deletedLines = append(deletedLines, p.lines[i]) + } + + var addedLines []string + for ; i < len(p.lines) && p.lines[i].typ == diffLineAdded; i++ { + addedLines = append(addedLines, p.lines[i].data) + } + + if len(deletedLines) != 0 { + p.handleDeletedLines(deletedLines, addedLines) + continue + } + + // no deletions, only addings + p.handleAddedOnlyLines(addedLines) + } + + if len(p.replacementLinesToPrepend) != 0 { + p.log.Infof("The diff contains only additions: no original or deleted lines: %#v", p.lines) + return nil + } + + return p.ret } func (g Gofmt) extractIssuesFromPatch(patch string, log logutils.Log) ([]result.Issue, error) { diffs, err := diffpkg.ParseMultiFileDiff([]byte(patch)) if err != nil { - return nil, fmt.Errorf("can't parse patch: %s", err) + return nil, errors.Wrap(err, "can't parse patch") } if len(diffs) == 0 { @@ -76,28 +251,31 @@ func (g Gofmt) extractIssuesFromPatch(patch string, log logutils.Log) ([]result. } for _, hunk := range d.Hunks { - deletedLine, addedLine, err := getFirstDeletedAndAddedLineNumberInHunk(hunk) - if err != nil { - if addedLine > 1 { - deletedLine = addedLine - 1 // use previous line, TODO: use both prev and next lines - } else { - deletedLine = 1 - } - } - text := "File is not `gofmt`-ed with `-s`" if g.UseGoimports { text = "File is not `goimports`-ed" } - i := result.Issue{ - FromLinter: g.Name(), - Pos: token.Position{ - Filename: d.NewName, - Line: deletedLine, - }, - Text: text, + p := hunkChangesParser{ + log: log, + } + changes := p.parse(hunk) + for _, change := range changes { + change := change // fix scope + i := result.Issue{ + FromLinter: g.Name(), + Pos: token.Position{ + Filename: d.NewName, + Line: change.LineRange.From, + }, + Text: text, + Replacement: &change.Replacement, + } + if change.LineRange.From != change.LineRange.To { + i.LineRange = &change.LineRange + } + + issues = append(issues, i) } - issues = append(issues, i) } } diff --git a/pkg/golinters/gofmt_test.go b/pkg/golinters/gofmt_test.go new file mode 100644 index 00000000..f50b5a94 --- /dev/null +++ b/pkg/golinters/gofmt_test.go @@ -0,0 +1,353 @@ +package golinters + +import ( + "testing" + + "github.com/golang/mock/gomock" + + "github.com/golangci/golangci-lint/pkg/result" + + "github.com/stretchr/testify/assert" + diffpkg "sourcegraph.com/sourcegraph/go-diff/diff" + + "github.com/golangci/golangci-lint/pkg/logutils" +) + +func testDiffProducesChanges(t *testing.T, log logutils.Log, diff string, expectedChanges ...Change) { + diffs, err := diffpkg.ParseMultiFileDiff([]byte(diff)) + if err != nil { + assert.NoError(t, err) + } + + assert.Len(t, diffs, 1) + hunks := diffs[0].Hunks + assert.NotEmpty(t, hunks) + + var changes []Change + for _, hunk := range hunks { + p := hunkChangesParser{ + log: log, + } + changes = append(changes, p.parse(hunk)...) + } + + assert.Equal(t, expectedChanges, changes) +} + +func TestExtractChangesFromHunkAddOnly(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..43d04bf 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -1,5 +1,6 @@ + package logutil + ++// added line + type Func func(format string, args ...interface{}) + + type Log interface { +` + + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 2, + To: 2, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "", + "// added line", + }, + }, + }) +} + +func TestExtractChangesFromHunkAddOnlyOnFirstLine(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..97e6660 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -1,3 +1,4 @@ ++// added line + package logutil + + type Func func(format string, args ...interface{}) +` + + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 1, + To: 1, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "// added line", + "package logutil", + }, + }, + }) +} + +func TestExtractChangesFromHunkAddOnlyOnFirstLineWithSharedOriginalLine(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..7ff80c9 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -1,4 +1,7 @@ ++// added line 1 + package logutil ++// added line 2 ++// added line 3 + + type Func func(format string, args ...interface{}) +` + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 1, + To: 1, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "// added line 1", + "package logutil", + "// added line 2", + "// added line 3", + }, + }, + }) +} + +func TestExtractChangesFromHunkAddOnlyInAllDiff(t *testing.T) { + const diff = `diff --git a/test.go b/test.go +new file mode 100644 +index 0000000..6399915 +--- /dev/null ++++ b/test.go +@@ -0,0 +1,3 @@ ++package test ++ ++// line +` + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + log := logutils.NewMockLog(ctrl) + log.EXPECT().Infof("The diff contains only additions: no original or deleted lines: %#v", gomock.Any()) + var noChanges []Change + testDiffProducesChanges(t, log, diff, noChanges...) +} + +func TestExtractChangesFromHunkAddOnlyMultipleLines(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..3b83a94 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -2,6 +2,9 @@ package logutil + + type Func func(format string, args ...interface{}) + ++// add line 1 ++// add line 2 ++ + type Log interface { + Fatalf(format string, args ...interface{}) + Errorf(format string, args ...interface{}) +` + + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 4, + To: 4, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "", + "// add line 1", + "// add line 2", + "", + }, + }, + }) +} + +func TestExtractChangesFromHunkAddOnlyDifferentLines(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..e5ed2ad 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -2,9 +2,12 @@ package logutil + + type Func func(format string, args ...interface{}) + ++// add line 1 ++ + type Log interface { + Fatalf(format string, args ...interface{}) + Errorf(format string, args ...interface{}) ++ // add line 2 + Warnf(format string, args ...interface{}) + Infof(format string, args ...interface{}) + Debugf(key string, format string, args ...interface{}) +` + + expectedChanges := []Change{ + { + LineRange: result.Range{ + From: 4, + To: 4, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "", + "// add line 1", + "", + }, + }, + }, + { + LineRange: result.Range{ + From: 7, + To: 7, + }, + Replacement: result.Replacement{ + NewLines: []string{ + " Errorf(format string, args ...interface{})", + " // add line 2", + }, + }, + }, + } + + testDiffProducesChanges(t, nil, diff, expectedChanges...) +} + +func TestExtractChangesDeleteOnlyFirstLines(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..0fb554e 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -1,5 +1,3 @@ +-package logutil +- + type Func func(format string, args ...interface{}) + + type Log interface { +` + + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 1, + To: 2, + }, + Replacement: result.Replacement{ + NeedOnlyDelete: true, + }, + }) +} + +func TestExtractChangesReplaceLine(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..c2a8516 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -1,4 +1,4 @@ +-package logutil ++package test2 + + type Func func(format string, args ...interface{}) +` + + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 1, + To: 1, + }, + Replacement: result.Replacement{ + NewLines: []string{"package test2"}, + }, + }) +} + +func TestExtractChangesReplaceLineAfterFirstLineAdding(t *testing.T) { + const diff = `diff --git a/internal/shared/logutil/log.go b/internal/shared/logutil/log.go +index 258b340..43fc0de 100644 +--- a/internal/shared/logutil/log.go ++++ b/internal/shared/logutil/log.go +@@ -1,6 +1,7 @@ ++// added line + package logutil + +-type Func func(format string, args ...interface{}) ++// changed line + + type Log interface { + Fatalf(format string, args ...interface{})` + + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 1, + To: 1, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "// added line", + "package logutil", + }, + }, + }, Change{ + LineRange: result.Range{ + From: 3, + To: 3, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "// changed line", + }, + }, + }) +} + +func TestGofmtDiff(t *testing.T) { + const diff = `diff --git a/gofmt.go b/gofmt.go +index 2c9f78d..c0d5791 100644 +--- a/gofmt.go ++++ b/gofmt.go +@@ -1,9 +1,9 @@ + //args: -Egofmt + package p + +- func gofmt(a, b int) int { +- if a != b { +- return 1 ++func gofmt(a, b int) int { ++ if a != b { ++ return 1 + } +- return 2 ++ return 2 + } +` + testDiffProducesChanges(t, nil, diff, Change{ + LineRange: result.Range{ + From: 4, + To: 6, + }, + Replacement: result.Replacement{ + NewLines: []string{ + "func gofmt(a, b int) int {", + " if a != b {", + " return 1", + }, + }, + }, Change{ + LineRange: result.Range{ + From: 8, + To: 8, + }, + Replacement: result.Replacement{ + NewLines: []string{ + " return 2", + }, + }, + }) +} diff --git a/pkg/golinters/golint.go b/pkg/golinters/golint.go index 4850e605..de58d0a7 100644 --- a/pkg/golinters/golint.go +++ b/pkg/golinters/golint.go @@ -61,7 +61,7 @@ func (g Golint) lintPkg(minConfidence float64, files []*ast.File, fset *token.Fi if ps[idx].Confidence >= minConfidence { issues = append(issues, result.Issue{ Pos: ps[idx].Position, - Text: markIdentifiers(ps[idx].Text), + Text: ps[idx].Text, FromLinter: g.Name(), }) // TODO: use p.Link and p.Category diff --git a/pkg/golinters/gosec.go b/pkg/golinters/gosec.go index c156bf33..24798887 100644 --- a/pkg/golinters/gosec.go +++ b/pkg/golinters/gosec.go @@ -40,7 +40,7 @@ func (lint Gosec) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Is res := make([]result.Issue, 0, len(issues)) for _, i := range issues { - text := fmt.Sprintf("%s: %s", i.RuleID, markIdentifiers(i.What)) // TODO: use severity and confidence + text := fmt.Sprintf("%s: %s", i.RuleID, i.What) // TODO: use severity and confidence var r *result.Range line, err := strconv.Atoi(i.Line) if err != nil { diff --git a/pkg/golinters/govet.go b/pkg/golinters/govet.go index 17af8ae6..b7b36266 100644 --- a/pkg/golinters/govet.go +++ b/pkg/golinters/govet.go @@ -39,7 +39,7 @@ func (g Govet) Run(_ context.Context, lintCtx *linter.Context) ([]result.Issue, for _, i := range govetIssues { res = append(res, result.Issue{ Pos: i.Pos, - Text: markIdentifiers(i.Message), + Text: i.Message, FromLinter: g.Name(), }) } diff --git a/pkg/golinters/interfacer.go b/pkg/golinters/interfacer.go index 7395706f..f3652e49 100644 --- a/pkg/golinters/interfacer.go +++ b/pkg/golinters/interfacer.go @@ -37,7 +37,7 @@ func (lint Interfacer) Run(ctx context.Context, lintCtx *linter.Context) ([]resu pos := lintCtx.SSAProgram.Fset.Position(i.Pos()) res = append(res, result.Issue{ Pos: pos, - Text: markIdentifiers(i.Message()), + Text: i.Message(), FromLinter: lint.Name(), }) } diff --git a/pkg/golinters/megacheck.go b/pkg/golinters/megacheck.go index 6f4efe7a..04d0bfd8 100644 --- a/pkg/golinters/megacheck.go +++ b/pkg/golinters/megacheck.go @@ -259,7 +259,7 @@ func (m megacheck) Run(ctx context.Context, lintCtx *linter.Context) ([]result.I res = append(res, result.Issue{ Pos: i.Position, - Text: markIdentifiers(i.Text), + Text: i.Text, FromLinter: i.Checker, }) } diff --git a/pkg/golinters/unparam.go b/pkg/golinters/unparam.go index 661efe36..c45a7933 100644 --- a/pkg/golinters/unparam.go +++ b/pkg/golinters/unparam.go @@ -40,7 +40,7 @@ func (lint Unparam) Run(ctx context.Context, lintCtx *linter.Context) ([]result. for _, i := range unparamIssues { res = append(res, result.Issue{ Pos: lintCtx.Program.Fset.Position(i.Pos()), - Text: markIdentifiers(i.Message()), + Text: i.Message(), FromLinter: lint.Name(), }) } diff --git a/pkg/golinters/util.go b/pkg/golinters/util.go index dc02c5ee..ee2919c0 100644 --- a/pkg/golinters/util.go +++ b/pkg/golinters/util.go @@ -4,9 +4,7 @@ import ( "fmt" "go/ast" "go/token" - "regexp" "strings" - "sync" gopackages "golang.org/x/tools/go/packages" @@ -30,68 +28,6 @@ func formatCodeBlock(code string, _ *config.Config) string { return fmt.Sprintf("```\n%s\n```", code) } -type replacePattern struct { - re string - repl string -} - -type replaceRegexp struct { - re *regexp.Regexp - repl string -} - -var replaceRegexps []replaceRegexp -var replaceRegexpsOnce sync.Once - -var replacePatterns = []replacePattern{ - // unparam - {`^(\S+) - (\S+) is unused$`, "`${1}` - `${2}` is unused"}, - {`^(\S+) - (\S+) always receives (\S+) \((.*)\)$`, "`${1}` - `${2}` always receives `${3}` (`${4}`)"}, - {`^(\S+) - (\S+) always receives (.*)$`, "`${1}` - `${2}` always receives `${3}`"}, - - // interfacer - {`^(\S+) can be (\S+)$`, "`${1}` can be `${2}`"}, - - // govet - {`^(\S+) arg list ends with redundant newline$`, "`${1}` arg list ends with redundant newline"}, - {`^(\S+) composite literal uses unkeyed fields$`, "`${1}` composite literal uses unkeyed fields"}, - - // gosec - {`^Blacklisted import (\S+): weak cryptographic primitive$`, - "Blacklisted import `${1}`: weak cryptographic primitive"}, - {`^TLS InsecureSkipVerify set true.$`, "TLS `InsecureSkipVerify` set true."}, - - // gosimple - {`^should replace loop with (.*)$`, "should replace loop with `${1}`"}, - - // megacheck - {`^this value of (\S+) is never used$`, "this value of `${1}` is never used"}, - {`^should use time.Since instead of time.Now().Sub$`, - "should use `time.Since` instead of `time.Now().Sub`"}, - {`^(func|const|field|type) (\S+) is unused$`, "${1} `${2}` is unused"}, -} - -func markIdentifiers(s string) string { - replaceRegexpsOnce.Do(func() { - for _, p := range replacePatterns { - r := replaceRegexp{ - re: regexp.MustCompile(p.re), - repl: p.repl, - } - replaceRegexps = append(replaceRegexps, r) - } - }) - - for _, rr := range replaceRegexps { - rs := rr.re.ReplaceAllString(s, rr.repl) - if rs != s { - return rs - } - } - - return s -} - func getAllFileNames(ctx *linter.Context) []string { var ret []string uniqFiles := map[string]bool{} // files are duplicated for test packages diff --git a/pkg/lint/linter/config.go b/pkg/lint/linter/config.go index c4ffaa42..32848eca 100644 --- a/pkg/lint/linter/config.go +++ b/pkg/lint/linter/config.go @@ -22,6 +22,7 @@ type Config struct { OriginalURL string // URL of original (not forked) repo, needed for autogenerated README ParentLinterName string // used only for megacheck's children now + CanAutoFix bool } func (lc *Config) WithTypeInfo() *Config { @@ -60,6 +61,11 @@ func (lc *Config) WithParent(parentLinterName string) *Config { return lc } +func (lc *Config) WithAutoFix() *Config { + lc.CanAutoFix = true + return lc +} + func (lc *Config) GetSpeed() int { return lc.Speed } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index b54e9d26..7a2027bb 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -169,10 +169,12 @@ func (Manager) GetAllSupportedLinterConfigs() []*linter.Config { linter.NewConfig(golinters.Gofmt{}). WithPresets(linter.PresetFormatting). WithSpeed(7). + WithAutoFix(). WithURL("https://golang.org/cmd/gofmt/"), linter.NewConfig(golinters.Gofmt{UseGoimports: true}). WithPresets(linter.PresetFormatting). WithSpeed(5). + WithAutoFix(). WithURL("https://godoc.org/golang.org/x/tools/cmd/goimports"), linter.NewConfig(golinters.Maligned{}). WithTypeInfo(). @@ -187,6 +189,7 @@ func (Manager) GetAllSupportedLinterConfigs() []*linter.Config { linter.NewConfig(golinters.Misspell{}). WithPresets(linter.PresetStyle). WithSpeed(7). + WithAutoFix(). WithURL("https://github.com/client9/misspell"), linter.NewConfig(golinters.Lll{}). WithPresets(linter.PresetStyle). diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 0ebdfd18..de9e9146 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -66,16 +66,18 @@ func NewRunner(astCache *astcache.Cache, cfg *config.Config, log logutils.Log, g skipDirsProcessor, // must be after path prettifier processors.NewAutogeneratedExclude(astCache), + processors.NewIdentifierMarker(), // must be befor exclude processors.NewExclude(excludeTotalPattern), processors.NewExcludeRules(excludeRules), processors.NewNolint(astCache, log.Child("nolint")), processors.NewUniqByLine(), processors.NewDiff(icfg.Diff, icfg.DiffFromRevision, icfg.DiffPatchFilePath), - processors.NewMaxPerFileFromLinter(), - processors.NewMaxSameIssues(icfg.MaxSameIssues, log.Child("max_same_issues")), - processors.NewMaxFromLinter(icfg.MaxIssuesPerLinter, log.Child("max_from_linter")), + processors.NewMaxPerFileFromLinter(cfg), + processors.NewMaxSameIssues(icfg.MaxSameIssues, log.Child("max_same_issues"), cfg), + processors.NewMaxFromLinter(icfg.MaxIssuesPerLinter, log.Child("max_from_linter"), cfg), processors.NewSourceCode(log.Child("source_code")), + processors.NewReplacementBuilder(log.Child("replacement_builder")), // must be after source code processors.NewPathShortener(), }, Log: log, diff --git a/pkg/result/issue.go b/pkg/result/issue.go index 3e27267f..dd144278 100644 --- a/pkg/result/issue.go +++ b/pkg/result/issue.go @@ -6,15 +6,26 @@ type Range struct { From, To int } +type Replacement struct { + NeedOnlyDelete bool // need to delete all lines of the issue without replacement with new lines + NewLines []string // is NeedDelete is false it's the replacement lines +} + type Issue struct { FromLinter string Text string Pos token.Position LineRange *Range `json:",omitempty"` - HunkPos int `json:",omitempty"` + // HunkPos is used only when golangci-lint is run over a diff + HunkPos int `json:",omitempty"` + + // Source lines of a code with the issue to show SourceLines []string + + // If we know how to fix the issue we can provide replacement lines + Replacement *Replacement } func (i *Issue) FilePath() string { diff --git a/pkg/result/processors/fixer.go b/pkg/result/processors/fixer.go new file mode 100644 index 00000000..599f1aa4 --- /dev/null +++ b/pkg/result/processors/fixer.go @@ -0,0 +1,149 @@ +package processors + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/golangci/golangci-lint/pkg/logutils" + + "github.com/pkg/errors" + + "github.com/golangci/golangci-lint/pkg/config" + "github.com/golangci/golangci-lint/pkg/result" +) + +type Fixer struct { + cfg *config.Config + log logutils.Log +} + +func NewFixer(cfg *config.Config, log logutils.Log) *Fixer { + return &Fixer{cfg: cfg, log: log} +} + +func (f Fixer) Process(issues <-chan result.Issue) <-chan result.Issue { + if !f.cfg.Issues.NeedFix { + return issues + } + + outCh := make(chan result.Issue, 1024) + + go func() { + issuesToFixPerFile := map[string][]result.Issue{} + for issue := range issues { + if issue.Replacement == nil { + outCh <- issue + continue + } + + issuesToFixPerFile[issue.FilePath()] = append(issuesToFixPerFile[issue.FilePath()], issue) + } + + for file, issuesToFix := range issuesToFixPerFile { + if err := f.fixIssuesInFile(file, issuesToFix); err != nil { + f.log.Errorf("Failed to fix issues in file %s: %s", file, err) + + // show issues only if can't fix them + for _, issue := range issuesToFix { + outCh <- issue + } + } + } + close(outCh) + }() + + return outCh +} + +func (f Fixer) fixIssuesInFile(filePath string, issues []result.Issue) error { + // TODO: don't read the whole file into memory: read line by line; + // can't just use bufio.scanner: it has a line length limit + origFileData, err := ioutil.ReadFile(filePath) + if err != nil { + return errors.Wrapf(err, "failed to read %s", filePath) + } + origFileLines := bytes.Split(origFileData, []byte("\n")) + + tmpFileName := filepath.Join(filepath.Dir(filePath), fmt.Sprintf(".%s.golangci_fix", filepath.Base(filePath))) + tmpOutFile, err := os.Create(tmpFileName) + if err != nil { + return errors.Wrapf(err, "failed to make file %s", tmpFileName) + } + + issues = f.findNotIntersectingIssues(issues) + + if err = f.writeFixedFile(origFileLines, issues, tmpOutFile); err != nil { + tmpOutFile.Close() + os.Remove(tmpOutFile.Name()) + return err + } + + tmpOutFile.Close() + if err = os.Rename(tmpOutFile.Name(), filePath); err != nil { + os.Remove(tmpOutFile.Name()) + return errors.Wrapf(err, "failed to rename %s -> %s", tmpOutFile.Name(), filePath) + } + + return nil +} + +func (f Fixer) findNotIntersectingIssues(issues []result.Issue) []result.Issue { + sort.SliceStable(issues, func(i, j int) bool { + return issues[i].Line() < issues[j].Line() //nolint:scopelint + }) + + var ret []result.Issue + var currentEnd int + for _, issue := range issues { + rng := issue.GetLineRange() + if rng.From <= currentEnd { + f.log.Infof("Skip issue %#v: intersects with end %d", issue, currentEnd) + continue // skip intersecting issue + } + f.log.Infof("Fix issue %#v with range %v", issue, issue.GetLineRange()) + ret = append(ret, issue) + currentEnd = rng.To + } + + return ret +} + +func (f Fixer) writeFixedFile(origFileLines [][]byte, issues []result.Issue, tmpOutFile *os.File) error { + // issues aren't intersecting + + nextIssueIndex := 0 + for i := 0; i < len(origFileLines); i++ { + var outLine string + var nextIssue *result.Issue + if nextIssueIndex != len(issues) { + nextIssue = &issues[nextIssueIndex] + } + + origFileLineNumber := i + 1 + if nextIssue == nil || origFileLineNumber != nextIssue.Line() { + outLine = string(origFileLines[i]) + } else { + nextIssueIndex++ + rng := nextIssue.GetLineRange() + i += rng.To - rng.From + if nextIssue.Replacement.NeedOnlyDelete { + continue + } + outLine = strings.Join(nextIssue.Replacement.NewLines, "\n") + } + + if i < len(origFileLines)-1 { + outLine += "\n" + } + if _, err := tmpOutFile.WriteString(outLine); err != nil { + return errors.Wrap(err, "failed to write output line") + } + } + + return nil +} diff --git a/pkg/result/processors/identifier_marker.go b/pkg/result/processors/identifier_marker.go new file mode 100644 index 00000000..a7d48f41 --- /dev/null +++ b/pkg/result/processors/identifier_marker.go @@ -0,0 +1,123 @@ +package processors + +import ( + "regexp" + + "github.com/golangci/golangci-lint/pkg/result" +) + +type replacePattern struct { + re string + repl string +} + +type replaceRegexp struct { + re *regexp.Regexp + repl string +} + +var replacePatterns = []replacePattern{ + // unparam + {`^(\S+) - (\S+) is unused$`, "`${1}` - `${2}` is unused"}, + {`^(\S+) - (\S+) always receives (\S+) \((.*)\)$`, "`${1}` - `${2}` always receives `${3}` (`${4}`)"}, + {`^(\S+) - (\S+) always receives (.*)$`, "`${1}` - `${2}` always receives `${3}`"}, + {`^(\S+) - result (\S+) is always (\S+)`, "`${1}` - result `${2}` is always `${3}`"}, + + // interfacer + {`^(\S+) can be (\S+)$`, "`${1}` can be `${2}`"}, + + // govet + {`^(\S+) arg list ends with redundant newline$`, "`${1}` arg list ends with redundant newline"}, + {`^(\S+) composite literal uses unkeyed fields$`, "`${1}` composite literal uses unkeyed fields"}, + + // gosec + {`^(\S+): Blacklisted import (\S+): weak cryptographic primitive$`, + "${1}: Blacklisted import `${2}`: weak cryptographic primitive"}, + {`^TLS InsecureSkipVerify set true.$`, "TLS `InsecureSkipVerify` set true."}, + + // gosimple + {`^should replace loop with (.*)$`, "should replace loop with `${1}`"}, + {`^should use a simple channel send/receive instead of select with a single case`, + "should use a simple channel send/receive instead of `select` with a single case"}, + {`^should omit comparison to bool constant, can be simplified to (.+)$`, + "should omit comparison to bool constant, can be simplified to `${1}`"}, + {`^should write (.+) instead of (.+)$`, "should write `${1}` instead of `${2}`"}, + {`^redundant return statement$`, "redundant `return` statement"}, + + // staticcheck + {`^this value of (\S+) is never used$`, "this value of `${1}` is never used"}, + {`^should use time.Since instead of time.Now\(\).Sub$`, + "should use `time.Since` instead of `time.Now().Sub`"}, + {`^should check returned error before deferring response.Close\(\)$`, + "should check returned error before deferring `response.Close()`"}, + {`^no value of type uint is less than 0$`, "no value of type `uint` is less than `0`"}, + + // unused + {`^(func|const|field|type|var) (\S+) is unused$`, "${1} `${2}` is unused"}, + + // typecheck + {`^unknown field (\S+) in struct literal$`, "unknown field `${1}` in struct literal"}, + {`^invalid operation: (\S+) \(variable of type (\S+)\) has no field or method (\S+)$`, + "invalid operation: `${1}` (variable of type `${2}`) has no field or method `${3}`"}, + {`^undeclared name: (\S+)$`, "undeclared name: `${1}`"}, + {`^cannot use addr \(variable of type (\S+)\) as (\S+) value in argument to (\S+)$`, + "cannot use addr (variable of type `${1}`) as `${2}` value in argument to `${3}`"}, + {`^other declaration of (\S+)$`, "other declaration of `${1}`"}, + {`^(\S+) redeclared in this block$`, "`${1}` redeclared in this block"}, + + // golint + {`^exported (type|method|function|var|const) (\S+) should have comment or be unexported$`, + "exported ${1} `${2}` should have comment or be unexported"}, + {`^comment on exported (type|method|function|var|const) (\S+) should be of the form "(\S+) ..."$`, + "comment on exported ${1} `${2}` should be of the form `${3} ...`"}, + {`^should replace (.+) with (.+)$`, "should replace `${1}` with `${2}`"}, + {`^if block ends with a return statement, so drop this else and outdent its block$`, + "`if` block ends with a `return` statement, so drop this `else` and outdent its block"}, + {`^(struct field|var|range var|const|type|(?:func|method|interface method) (?:parameter|result)) (\S+) should be (\S+)$`, + "${1} `${2}` should be `${3}`"}, + {`^don't use underscores in Go names; var (\S+) should be (\S+)$`, + "don't use underscores in Go names; var `${1}` should be `${2}`"}, +} + +type IdentifierMarker struct { + replaceRegexps []replaceRegexp +} + +func NewIdentifierMarker() *IdentifierMarker { + var replaceRegexps []replaceRegexp + for _, p := range replacePatterns { + r := replaceRegexp{ + re: regexp.MustCompile(p.re), + repl: p.repl, + } + replaceRegexps = append(replaceRegexps, r) + } + + return &IdentifierMarker{ + replaceRegexps: replaceRegexps, + } +} + +func (im IdentifierMarker) Process(issues []result.Issue) ([]result.Issue, error) { + return transformIssues(issues, func(i *result.Issue) *result.Issue { + iCopy := *i + iCopy.Text = im.markIdentifiers(iCopy.Text) + return &iCopy + }), nil +} + +func (im IdentifierMarker) markIdentifiers(s string) string { + for _, rr := range im.replaceRegexps { + rs := rr.re.ReplaceAllString(s, rr.repl) + if rs != s { + return rs + } + } + + return s +} + +func (im IdentifierMarker) Name() string { + return "identifier_marker" +} +func (im IdentifierMarker) Finish() {} diff --git a/pkg/result/processors/identifier_marker_test.go b/pkg/result/processors/identifier_marker_test.go new file mode 100644 index 00000000..c58987c3 --- /dev/null +++ b/pkg/result/processors/identifier_marker_test.go @@ -0,0 +1,54 @@ +package processors + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/golangci/golangci-lint/pkg/result" +) + +func TestIdentifierMarker(t *testing.T) { + //nolint:lll + cases := []struct{ in, out string }{ + {"unknown field Address in struct literal", "unknown field `Address` in struct literal"}, + {"invalid operation: res (variable of type github.com/iotexproject/iotex-core/explorer/idl/explorer.GetBlkOrActResponse) has no field or method Address", + "invalid operation: `res` (variable of type `github.com/iotexproject/iotex-core/explorer/idl/explorer.GetBlkOrActResponse`) has no field or method `Address`"}, + {"should use a simple channel send/receive instead of select with a single case", + "should use a simple channel send/receive instead of `select` with a single case"}, + {"var testInputs is unused", "var `testInputs` is unused"}, + {"undeclared name: stateIDLabel", "undeclared name: `stateIDLabel`"}, + {"exported type Metrics should have comment or be unexported", + "exported type `Metrics` should have comment or be unexported"}, + {`comment on exported function NewMetrics should be of the form "NewMetrics ..."`, + "comment on exported function `NewMetrics` should be of the form `NewMetrics ...`"}, + {"cannot use addr (variable of type string) as github.com/iotexproject/iotex-core/pkg/keypair.PublicKey value in argument to action.FakeSeal", + "cannot use addr (variable of type `string`) as `github.com/iotexproject/iotex-core/pkg/keypair.PublicKey` value in argument to `action.FakeSeal`"}, + {"other declaration of out", "other declaration of `out`"}, + {"should check returned error before deferring response.Close()", "should check returned error before deferring `response.Close()`"}, + {"should use time.Since instead of time.Now().Sub", "should use `time.Since` instead of `time.Now().Sub`"}, + {"TestFibZeroCount redeclared in this block", "`TestFibZeroCount` redeclared in this block"}, + {"should replace i += 1 with i++", "should replace `i += 1` with `i++`"}, + {"createEntry - result err is always nil", "`createEntry` - result `err` is always `nil`"}, + {"should omit comparison to bool constant, can be simplified to !projectIntegration.Model.Storage", + "should omit comparison to bool constant, can be simplified to `!projectIntegration.Model.Storage`"}, + {"if block ends with a return statement, so drop this else and outdent its block", + "`if` block ends with a `return` statement, so drop this `else` and outdent its block"}, + {"should write pupData := ms.m[pupID] instead of pupData, _ := ms.m[pupID]", + "should write `pupData := ms.m[pupID]` instead of `pupData, _ := ms.m[pupID]`"}, + {"no value of type uint is less than 0", "no value of type `uint` is less than `0`"}, + {"redundant return statement", "redundant `return` statement"}, + {"struct field Id should be ID", "struct field `Id` should be `ID`"}, + {"don't use underscores in Go names; var Go_lint should be GoLint", + "don't use underscores in Go names; var `Go_lint` should be `GoLint`"}, + {"G501: Blacklisted import crypto/md5: weak cryptographic primitive", + "G501: Blacklisted import `crypto/md5`: weak cryptographic primitive"}, + } + p := NewIdentifierMarker() + + for _, c := range cases { + out, err := p.Process([]result.Issue{{Text: c.in}}) + assert.NoError(t, err) + assert.Equal(t, []result.Issue{{Text: c.out}}, out) + } +} diff --git a/pkg/result/processors/max_from_linter.go b/pkg/result/processors/max_from_linter.go index f669cab2..0129cc8c 100644 --- a/pkg/result/processors/max_from_linter.go +++ b/pkg/result/processors/max_from_linter.go @@ -1,6 +1,7 @@ package processors import ( + "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/logutils" "github.com/golangci/golangci-lint/pkg/result" ) @@ -9,15 +10,17 @@ type MaxFromLinter struct { lc linterToCountMap limit int log logutils.Log + cfg *config.Config } var _ Processor = &MaxFromLinter{} -func NewMaxFromLinter(limit int, log logutils.Log) *MaxFromLinter { +func NewMaxFromLinter(limit int, log logutils.Log, cfg *config.Config) *MaxFromLinter { return &MaxFromLinter{ lc: linterToCountMap{}, limit: limit, log: log, + cfg: cfg, } } @@ -30,6 +33,11 @@ func (p *MaxFromLinter) Process(issues []result.Issue) ([]result.Issue, error) { return issues, nil } + if p.cfg.Issues.NeedFix { + // we need to fix all issues at once => we need to return all of them + return issues, nil + } + return filterIssues(issues, func(i *result.Issue) bool { p.lc[i.FromLinter]++ // always inc for stat return p.lc[i.FromLinter] <= p.limit diff --git a/pkg/result/processors/max_from_linter_test.go b/pkg/result/processors/max_from_linter_test.go index 15ccb8bb..7690bdf4 100644 --- a/pkg/result/processors/max_from_linter_test.go +++ b/pkg/result/processors/max_from_linter_test.go @@ -3,11 +3,13 @@ package processors import ( "testing" + "github.com/golangci/golangci-lint/pkg/config" + "github.com/golangci/golangci-lint/pkg/logutils" ) func TestMaxFromLinter(t *testing.T) { - p := NewMaxFromLinter(1, logutils.NewStderrLog("")) + p := NewMaxFromLinter(1, logutils.NewStderrLog(""), &config.Config{}) gosimple := newFromLinterIssue("gosimple") gofmt := newFromLinterIssue("gofmt") processAssertSame(t, p, gosimple) // ok diff --git a/pkg/result/processors/max_per_file_from_linter.go b/pkg/result/processors/max_per_file_from_linter.go index 29d535b1..e9842966 100644 --- a/pkg/result/processors/max_per_file_from_linter.go +++ b/pkg/result/processors/max_per_file_from_linter.go @@ -1,6 +1,7 @@ package processors import ( + "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/result" ) @@ -8,14 +9,26 @@ type linterToCountMap map[string]int type fileToLinterToCountMap map[string]linterToCountMap type MaxPerFileFromLinter struct { - flc fileToLinterToCountMap + flc fileToLinterToCountMap + maxPerFileFromLinterConfig map[string]int } var _ Processor = &MaxPerFileFromLinter{} -func NewMaxPerFileFromLinter() *MaxPerFileFromLinter { +func NewMaxPerFileFromLinter(cfg *config.Config) *MaxPerFileFromLinter { + maxPerFileFromLinterConfig := map[string]int{ + "typecheck": 3, + } + if !cfg.Issues.NeedFix { + // if we don't fix we do this limiting to not annoy user; + // otherwise we need to fix all issues in the file at once + maxPerFileFromLinterConfig["gofmt"] = 1 + maxPerFileFromLinterConfig["goimports"] = 1 + } + return &MaxPerFileFromLinter{ - flc: fileToLinterToCountMap{}, + flc: fileToLinterToCountMap{}, //nolint:goimports,gofmt + maxPerFileFromLinterConfig: maxPerFileFromLinterConfig, } } @@ -23,15 +36,9 @@ func (p MaxPerFileFromLinter) Name() string { return "max_per_file_from_linter" } -var maxPerFileFromLinterConfig = map[string]int{ - "gofmt": 1, - "goimports": 1, - "typecheck": 3, -} - func (p *MaxPerFileFromLinter) Process(issues []result.Issue) ([]result.Issue, error) { return filterIssues(issues, func(i *result.Issue) bool { - limit := maxPerFileFromLinterConfig[i.FromLinter] + limit := p.maxPerFileFromLinterConfig[i.FromLinter] if limit == 0 { return true } diff --git a/pkg/result/processors/max_per_file_from_linter_test.go b/pkg/result/processors/max_per_file_from_linter_test.go index 7b50758c..fa0c79d9 100644 --- a/pkg/result/processors/max_per_file_from_linter_test.go +++ b/pkg/result/processors/max_per_file_from_linter_test.go @@ -3,6 +3,7 @@ package processors import ( "testing" + "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/result" ) @@ -13,14 +14,14 @@ func newFromLinterIssue(linterName string) result.Issue { } func TestMaxPerFileFromLinterUnlimited(t *testing.T) { - p := NewMaxPerFileFromLinter() + p := NewMaxPerFileFromLinter(&config.Config{}) gosimple := newFromLinterIssue("gosimple") processAssertSame(t, p, gosimple) // collect stat processAssertSame(t, p, gosimple) // check not limits } func TestMaxPerFileFromLinter(t *testing.T) { - p := NewMaxPerFileFromLinter() + p := NewMaxPerFileFromLinter(&config.Config{}) for _, name := range []string{"gofmt", "goimports"} { limited := newFromLinterIssue(name) gosimple := newFromLinterIssue("gosimple") diff --git a/pkg/result/processors/max_same_issues.go b/pkg/result/processors/max_same_issues.go index fe3c2b22..4f0af44d 100644 --- a/pkg/result/processors/max_same_issues.go +++ b/pkg/result/processors/max_same_issues.go @@ -3,6 +3,8 @@ package processors import ( "sort" + "github.com/golangci/golangci-lint/pkg/config" + "github.com/golangci/golangci-lint/pkg/logutils" "github.com/golangci/golangci-lint/pkg/result" ) @@ -13,15 +15,17 @@ type MaxSameIssues struct { tc textToCountMap limit int log logutils.Log + cfg *config.Config } var _ Processor = &MaxSameIssues{} -func NewMaxSameIssues(limit int, log logutils.Log) *MaxSameIssues { +func NewMaxSameIssues(limit int, log logutils.Log, cfg *config.Config) *MaxSameIssues { return &MaxSameIssues{ tc: textToCountMap{}, limit: limit, log: log, + cfg: cfg, } } @@ -34,6 +38,11 @@ func (p *MaxSameIssues) Process(issues []result.Issue) ([]result.Issue, error) { return issues, nil } + if p.cfg.Issues.NeedFix { + // we need to fix all issues at once => we need to return all of them + return issues, nil + } + return filterIssues(issues, func(i *result.Issue) bool { p.tc[i.Text]++ // always inc for stat return p.tc[i.Text] <= p.limit diff --git a/pkg/result/processors/max_same_issues_test.go b/pkg/result/processors/max_same_issues_test.go index 3fad8fd5..cda09513 100644 --- a/pkg/result/processors/max_same_issues_test.go +++ b/pkg/result/processors/max_same_issues_test.go @@ -3,12 +3,13 @@ package processors import ( "testing" + "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/logutils" "github.com/golangci/golangci-lint/pkg/result" ) func TestMaxSameIssues(t *testing.T) { - p := NewMaxSameIssues(1, logutils.NewStderrLog("")) + p := NewMaxSameIssues(1, logutils.NewStderrLog(""), &config.Config{}) i1 := result.Issue{ Text: "1", } diff --git a/pkg/result/processors/replacement_builder.go b/pkg/result/processors/replacement_builder.go new file mode 100644 index 00000000..352fb66d --- /dev/null +++ b/pkg/result/processors/replacement_builder.go @@ -0,0 +1,92 @@ +package processors + +import ( + "fmt" + "regexp" + "strings" + + "github.com/golangci/golangci-lint/pkg/golinters" + "github.com/golangci/golangci-lint/pkg/logutils" + + "github.com/golangci/golangci-lint/pkg/result" +) + +type ReplacementBuilder struct { + log logutils.Log +} + +func NewReplacementBuilder(log logutils.Log) *ReplacementBuilder { + return &ReplacementBuilder{log: log} +} + +func (p ReplacementBuilder) Process(issues []result.Issue) ([]result.Issue, error) { + return transformIssues(issues, p.processIssue), nil +} + +func (p ReplacementBuilder) processIssue(i *result.Issue) *result.Issue { + misspellName := golinters.Misspell{}.Name() + if i.FromLinter == misspellName { + newIssue, err := p.processMisspellIssue(i) + if err != nil { + p.log.Warnf("Failed to build replacement for misspell issue: %s", err) + return i + } + return newIssue + } + + return i +} + +func (p ReplacementBuilder) processMisspellIssue(i *result.Issue) (*result.Issue, error) { + if len(i.SourceLines) != 1 { + return nil, fmt.Errorf("invalid count of source lines: %d", len(i.SourceLines)) + } + sourceLine := i.SourceLines[0] + + if i.Column() <= 0 { + return nil, fmt.Errorf("invalid column %d", i.Column()) + } + col0 := i.Column() - 1 + if col0 >= len(sourceLine) { + return nil, fmt.Errorf("too big column %d", i.Column()) + } + + issueTextRE := regexp.MustCompile("`(.+)` is a misspelling of `(.+)`") + submatches := issueTextRE.FindStringSubmatch(i.Text) + if len(submatches) != 3 { + return nil, fmt.Errorf("invalid count of submatches %d", len(submatches)) + } + + from, to := submatches[1], submatches[2] + if !strings.HasPrefix(sourceLine[col0:], from) { + return nil, fmt.Errorf("invalid prefix of source line `%s`", sourceLine) + } + + newSourceLine := "" + if col0 != 0 { + newSourceLine += sourceLine[:col0] + } + + newSourceLine += to + + sourceLineFromEnd := col0 + len(from) + if sourceLineFromEnd < len(sourceLine) { + newSourceLine += sourceLine[sourceLineFromEnd:] + } + + if i.Replacement != nil { + return nil, fmt.Errorf("issue replacement isn't nil") + } + + iCopy := *i + iCopy.Replacement = &result.Replacement{ + NewLines: []string{newSourceLine}, + } + return &iCopy, nil +} + +func (p ReplacementBuilder) Name() string { + return "replacement_builder" +} + +func (p ReplacementBuilder) Finish() {} diff --git a/test/.gitignore b/test/.gitignore new file mode 100644 index 00000000..9bfc4ac4 --- /dev/null +++ b/test/.gitignore @@ -0,0 +1 @@ +/testdata/fix.tmp/ diff --git a/test/fix_test.go b/test/fix_test.go new file mode 100644 index 00000000..b480a3ac --- /dev/null +++ b/test/fix_test.go @@ -0,0 +1,59 @@ +package test + +import ( + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "testing" + + assert "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v2" + + "github.com/golangci/golangci-lint/test/testshared" +) + +func TestFix(t *testing.T) { + findSources := func(pathPatterns ...string) []string { + sources, err := filepath.Glob(filepath.Join(pathPatterns...)) + assert.NoError(t, err) + assert.NotEmpty(t, sources) + return sources + } + + tmpDir := filepath.Join(testdataDir, "fix.tmp") + os.RemoveAll(tmpDir) // cleanup after previous runs + + if os.Getenv("GL_KEEP_TEMP_FILES") != "1" { + defer os.RemoveAll(tmpDir) + } + + err := exec.Command("cp", "-R", filepath.Join(testdataDir, "fix"), tmpDir).Run() + assert.NoError(t, err) + + inputs := findSources(tmpDir, "in", "*.go") + for _, input := range inputs { + input := input + t.Run(filepath.Base(input), func(t *testing.T) { + args := []string{ + "--disable-all", "--print-issued-lines=false", "--print-linter-name=false", "--out-format=line-number", + "--fix", + input, + } + rc := extractRunContextFromComments(t, input) + args = append(args, rc.args...) + + cfg, err := yaml.Marshal(rc.config) + assert.NoError(t, err) + + testshared.NewLintRunner(t).RunWithYamlConfig(string(cfg), args...) + output, err := ioutil.ReadFile(input) + assert.NoError(t, err) + + expectedOutput, err := ioutil.ReadFile(filepath.Join(testdataDir, "fix", "out", filepath.Base(input))) + assert.NoError(t, err) + + assert.Equal(t, string(expectedOutput), string(output)) + }) + } +} diff --git a/test/linters_test.go b/test/linters_test.go index 93566b25..e08480d4 100644 --- a/test/linters_test.go +++ b/test/linters_test.go @@ -214,7 +214,7 @@ func TestExtractRunContextFromComments(t *testing.T) { func TestGolintConsumesXTestFiles(t *testing.T) { dir := getTestDataDir("withxtest") - const expIssue = "if block ends with a return statement, so drop this else and outdent its block" + const expIssue = "`if` block ends with a `return` statement, so drop this `else` and outdent its block" r := testshared.NewLintRunner(t) r.Run("--no-config", "--disable-all", "-Egolint", dir).ExpectHasIssue(expIssue) diff --git a/test/run_test.go b/test/run_test.go index 96a54909..c59dfa11 100644 --- a/test/run_test.go +++ b/test/run_test.go @@ -47,7 +47,7 @@ func TestDeadline(t *testing.T) { func TestTestsAreLintedByDefault(t *testing.T) { testshared.NewLintRunner(t).Run(getTestDataDir("withtests")). - ExpectHasIssue("if block ends with a return") + ExpectHasIssue("`if` block ends with a `return`") } func TestCgoOk(t *testing.T) { @@ -68,8 +68,8 @@ func TestSkippedDirsNoMatchArg(t *testing.T) { r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config", "--skip-dirs", dir, "-Egolint", dir) r.ExpectExitCode(exitcodes.IssuesFound). - ExpectOutputEq("testdata/skipdirs/skip_me/nested/with_issue.go:8:9: if block ends with " + - "a return statement, so drop this else and outdent its block (golint)\n") + ExpectOutputEq("testdata/skipdirs/skip_me/nested/with_issue.go:8:9: `if` block ends with " + + "a `return` statement, so drop this `else` and outdent its block (golint)\n") } func TestSkippedDirsTestdata(t *testing.T) { @@ -114,7 +114,7 @@ func TestAbsPathDirAnalysis(t *testing.T) { assert.NoError(t, err) r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config", "-Egolint", absDir) - r.ExpectHasIssue("if block ends with a return statement") + r.ExpectHasIssue("`if` block ends with a `return` statement") } func TestAbsPathFileAnalysis(t *testing.T) { @@ -123,7 +123,7 @@ func TestAbsPathFileAnalysis(t *testing.T) { assert.NoError(t, err) r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config", "-Egolint", absDir) - r.ExpectHasIssue("if block ends with a return statement") + r.ExpectHasIssue("`if` block ends with a `return` statement") } func TestDisallowedOptionsInConfig(t *testing.T) { diff --git a/test/testdata/fix/in/gofmt.go b/test/testdata/fix/in/gofmt.go new file mode 100644 index 00000000..2c9f78d5 --- /dev/null +++ b/test/testdata/fix/in/gofmt.go @@ -0,0 +1,9 @@ +//args: -Egofmt +package p + + func gofmt(a, b int) int { + if a != b { + return 1 + } + return 2 +} diff --git a/test/testdata/fix/in/goimports.go b/test/testdata/fix/in/goimports.go new file mode 100644 index 00000000..975dc883 --- /dev/null +++ b/test/testdata/fix/in/goimports.go @@ -0,0 +1,14 @@ +//args: -Egofmt,goimports +package p + +import ( + "os" + "fmt" +) + + func goimports(a, b int) int { + if a != b { + return 1 + } + return 2 +} diff --git a/test/testdata/fix/in/misspell.go b/test/testdata/fix/in/misspell.go new file mode 100644 index 00000000..64c08695 --- /dev/null +++ b/test/testdata/fix/in/misspell.go @@ -0,0 +1,11 @@ +//args: -Emisspell +package p + +// langauge lala +// lala langauge +// langauge +// langauge langauge + +func langaugeMisspell() { // the function detects langauge of the text + +} diff --git a/test/testdata/fix/out/gofmt.go b/test/testdata/fix/out/gofmt.go new file mode 100644 index 00000000..c0d57910 --- /dev/null +++ b/test/testdata/fix/out/gofmt.go @@ -0,0 +1,9 @@ +//args: -Egofmt +package p + +func gofmt(a, b int) int { + if a != b { + return 1 + } + return 2 +} diff --git a/test/testdata/fix/out/goimports.go b/test/testdata/fix/out/goimports.go new file mode 100644 index 00000000..a5a698bb --- /dev/null +++ b/test/testdata/fix/out/goimports.go @@ -0,0 +1,9 @@ +//args: -Egofmt,goimports +package p + +func goimports(a, b int) int { + if a != b { + return 1 + } + return 2 +} diff --git a/test/testdata/fix/out/misspell.go b/test/testdata/fix/out/misspell.go new file mode 100644 index 00000000..1fc7dbcf --- /dev/null +++ b/test/testdata/fix/out/misspell.go @@ -0,0 +1,11 @@ +//args: -Emisspell +package p + +// language lala +// lala language +// language +// language langauge + +func langaugeMisspell() { // the function detects language of the text + +} diff --git a/test/testdata/golint.go b/test/testdata/golint.go index 31fb9e87..2fc70ed7 100644 --- a/test/testdata/golint.go +++ b/test/testdata/golint.go @@ -1,7 +1,7 @@ //args: -Egolint package testdata -var Go_lint string // ERROR "don't use underscores in Go names; var Go_lint should be GoLint" +var Go_lint string // ERROR "don't use underscores in Go names; var `Go_lint` should be `GoLint`" func ExportedFuncWithNoComment() { } diff --git a/test/testdata/notcompiles/typecheck_many_issues.go b/test/testdata/notcompiles/typecheck_many_issues.go index 50f1607a..ced4724d 100644 --- a/test/testdata/notcompiles/typecheck_many_issues.go +++ b/test/testdata/notcompiles/typecheck_many_issues.go @@ -2,8 +2,8 @@ package testdata func TypeCheckBadCalls() { - typecheckNotExists1.F1() // ERROR "undeclared name: typecheckNotExists1" - typecheckNotExists2.F2() // ERROR "undeclared name: typecheckNotExists2" - typecheckNotExists3.F3() // ERROR "undeclared name: typecheckNotExists3" + typecheckNotExists1.F1() // ERROR "undeclared name: `typecheckNotExists1`" + typecheckNotExists2.F2() // ERROR "undeclared name: `typecheckNotExists2`" + typecheckNotExists3.F3() // ERROR "undeclared name: `typecheckNotExists3`" typecheckNotExists4.F4() } diff --git a/test/testshared/testshared.go b/test/testshared/testshared.go index 7cc412fd..1ea71652 100644 --- a/test/testshared/testshared.go +++ b/test/testshared/testshared.go @@ -22,9 +22,11 @@ type LintRunner struct { } func NewLintRunner(t assert.TestingT) *LintRunner { + log := logutils.NewStderrLog("test") + log.SetLevel(logutils.LogLevelInfo) return &LintRunner{ t: t, - log: logutils.NewStderrLog("test"), + log: log, } } @@ -67,7 +69,7 @@ func (r *RunResult) ExpectOutputContains(s string) *RunResult { } func (r *RunResult) ExpectOutputEq(s string) *RunResult { - assert.Equal(r.t, r.output, s, "exit code is %d", r.exitCode) + assert.Equal(r.t, s, r.output, "exit code is %d", r.exitCode) return r }