Feature/enable autofix on whitespace (#674)

The whitespace linter was added in #673. Enable it and fix found issues.
Add auto-fixing to the whitespace linter.

Author: jirfag

.golangci.yml

@@ -75,6 +75,7 @@ linters:
     - unparam
     - unused
     - varcheck
+    # - whitespace - TODO: enable it when golangci.com will support it.
 
   # don't enable:
   # - depguard - until https://github.com/OpenPeeDeeP/depguard/issues/7 gets fixed

Makefile

@@ -3,11 +3,13 @@
 
 # Build
 
+fast_build: FORCE
+	go build -o golangci-lint ./cmd/golangci-lint
 build: golangci-lint
 clean:
 	rm -f golangci-lint test/path
 	rm -rf tools
-.PHONY: build clean
+.PHONY: fast_build build clean
 
 # Test
 

README.md

@@ -216,7 +216,7 @@ scopelint: Scopelint checks for unpinned variables in go programs [fast: true, a
 stylecheck: Stylecheck is a replacement for golint [fast: false, auto-fix: false]
 unconvert: Remove unnecessary type conversions [fast: true, auto-fix: false]
 unparam: Reports unused function parameters [fast: false, auto-fix: false]
-whitespace: Tool for detection of leading and trailing whitespace [fast: true, auto-fix: false]
+whitespace: Tool for detection of leading and trailing whitespace [fast: true, auto-fix: true]
 ```
 
 Pass `-E/--enable` to enable linter and `-D/--disable` to disable:
@@ -925,6 +925,7 @@ linters:
     - unparam
     - unused
     - varcheck
+    # - whitespace - TODO: enable it when golangci.com will support it.
 
   # don't enable:
   # - depguard - until https://github.com/OpenPeeDeeP/depguard/issues/7 gets fixed

pkg/commands/config.go

@@ -34,7 +34,6 @@ func (e *Executor) initConfig() {
 	}
 	e.initRunConfiguration(pathCmd) // allow --config
 	cmd.AddCommand(pathCmd)
-
 }
 
 func (e *Executor) executePathCmd(_ *cobra.Command, args []string) {

pkg/golinters/goanalysis/checker/checker.go

@@ -348,7 +348,6 @@ func (act *action) execOnce() {
 			// in-memory outputs of prerequisite analyzers
 			// become inputs to this analysis pass.
 			inputs[dep.a] = dep.result
-
 		} else if dep.a == act.a { // (always true)
 			// Same analysis, different package (vertical edge):
 			// serialized facts produced by prerequisite analysis

pkg/golinters/gocritic.go

@@ -134,7 +134,6 @@ func (lint Gocritic) Run(ctx context.Context, lintCtx *linter.Context) ([]result
 
 func (lint Gocritic) runOnPackage(lintpackCtx *lintpack.Context, checkers []*lintpack.Checker,
 	pkgInfo *loader.PackageInfo, ret chan<- result.Issue) {
-
 	for _, f := range pkgInfo.Files {
 		filename := filepath.Base(lintpackCtx.FileSet.Position(f.Pos()).Filename)
 		lintpackCtx.SetFileInfo(filename, f)
@@ -145,7 +144,6 @@ func (lint Gocritic) runOnPackage(lintpackCtx *lintpack.Context, checkers []*lin
 
 func (lint Gocritic) runOnFile(ctx *lintpack.Context, f *ast.File, checkers []*lintpack.Checker,
 	ret chan<- result.Issue) {
-
 	var wg sync.WaitGroup
 	wg.Add(len(checkers))
 	for _, c := range checkers {

pkg/golinters/whitespace.go

@@ -4,13 +4,16 @@ import (
 	"context"
 	"go/token"
 
+	"github.com/pkg/errors"
+
 	"github.com/golangci/golangci-lint/pkg/lint/linter"
 	"github.com/golangci/golangci-lint/pkg/result"
 
 	"github.com/ultraware/whitespace"
 )
 
-type Whitespace struct{}
+type Whitespace struct {
+}
 
 func (Whitespace) Name() string {
 	return "whitespace"
@@ -32,14 +35,40 @@ func (w Whitespace) Run(ctx context.Context, lintCtx *linter.Context) ([]result.
 
 	res := make([]result.Issue, len(issues))
 	for k, i := range issues {
-		res[k] = result.Issue{
+		issue := result.Issue{
 			Pos: token.Position{
 				Filename: i.Pos.Filename,
 				Line:     i.Pos.Line,
 			},
-			Text:       i.Message,
-			FromLinter: w.Name(),
+			Text:        i.Message,
+			FromLinter:  w.Name(),
+			Replacement: &result.Replacement{},
+		}
+
+		// TODO(jirfag): return more information from Whitespace to get rid of string comparisons
+		if i.Message == "unnecessary leading newline" {
+			// cover two lines by the issue: opening bracket "{" (issue.Pos.Line) and following empty line
+			issue.LineRange = &result.Range{From: issue.Pos.Line, To: issue.Pos.Line + 1}
+
+			bracketLine, err := lintCtx.LineCache.GetLine(issue.Pos.Filename, issue.Pos.Line)
+			if err != nil {
+				return nil, errors.Wrapf(err, "failed to get line %s:%d", issue.Pos.Filename, issue.Pos.Line)
+			}
+			issue.Replacement.NewLines = []string{bracketLine}
+		} else {
+			// cover two lines by the issue: closing bracket "}" (issue.Pos.Line) and preceding empty line
+			issue.LineRange = &result.Range{From: issue.Pos.Line - 1, To: issue.Pos.Line}
+
+			bracketLine, err := lintCtx.LineCache.GetLine(issue.Pos.Filename, issue.Pos.Line)
+			if err != nil {
+				return nil, errors.Wrapf(err, "failed to get line %s:%d", issue.Pos.Filename, issue.Pos.Line)
+			}
+			issue.Replacement.NewLines = []string{bracketLine}
+
+			issue.Pos.Line-- // set in sync with LineRange.From to not break fixer and other code features
 		}
+
+		res[k] = issue
 	}
 
 	return res, nil

pkg/lint/lintersdb/manager.go

@@ -245,6 +245,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
 		linter.NewConfig(golinters.Whitespace{}).
 			WithPresets(linter.PresetStyle).
 			WithSpeed(10).
+			WithAutoFix().
 			WithURL("https://github.com/ultraware/whitespace"),
 	}
 

pkg/lint/load.go

@@ -39,7 +39,6 @@ type ContextLoader struct {
 
 func NewContextLoader(cfg *config.Config, log logutils.Log, goenv *goutil.Env,
 	lineCache *fsutils.LineCache, fileCache *fsutils.FileCache) *ContextLoader {
-
 	return &ContextLoader{
 		cfg:         cfg,
 		log:         log,

pkg/lint/runner.go

@@ -31,7 +31,6 @@ type Runner struct {
 
 func NewRunner(astCache *astcache.Cache, cfg *config.Config, log logutils.Log, goenv *goutil.Env,
 	lineCache *fsutils.LineCache, dbManager *lintersdb.Manager) (*Runner, error) {
-
 	icfg := cfg.Issues
 	excludePatterns := icfg.ExcludePatterns
 	if icfg.UseDefaultExcludes {
@@ -101,7 +100,6 @@ type lintRes struct {
 
 func (r *Runner) runLinterSafe(ctx context.Context, lintCtx *linter.Context,
 	lc *linter.Config) (ret []result.Issue, err error) {
-
 	defer func() {
 		if panicData := recover(); panicData != nil {
 			err = fmt.Errorf("panic occurred: %s", panicData)
@@ -125,7 +123,6 @@ func (r *Runner) runLinterSafe(ctx context.Context, lintCtx *linter.Context,
 
 func (r Runner) runWorker(ctx context.Context, lintCtx *linter.Context,
 	tasksCh <-chan *linter.Config, lintResultsCh chan<- lintRes, name string) {
-
 	sw := timeutils.NewStopwatch(name, r.Log)
 	defer sw.Print()
 

pkg/result/processors/fixer.go

@@ -8,6 +8,8 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/golangci/golangci-lint/pkg/timeutils"
+
 	"github.com/golangci/golangci-lint/pkg/fsutils"
 
 	"github.com/golangci/golangci-lint/pkg/logutils"
@@ -22,10 +24,20 @@ type Fixer struct {
 	cfg       *config.Config
 	log       logutils.Log
 	fileCache *fsutils.FileCache
+	sw        *timeutils.Stopwatch
 }
 
 func NewFixer(cfg *config.Config, log logutils.Log, fileCache *fsutils.FileCache) *Fixer {
-	return &Fixer{cfg: cfg, log: log, fileCache: fileCache}
+	return &Fixer{
+		cfg:       cfg,
+		log:       log,
+		fileCache: fileCache,
+		sw:        timeutils.NewStopwatch("fixer", log),
+	}
+}
+
+func (f Fixer) printStat() {
+	f.sw.PrintStages()
 }
 
 func (f Fixer) Process(issues <-chan result.Issue) <-chan result.Issue {
@@ -47,7 +59,11 @@ func (f Fixer) Process(issues <-chan result.Issue) <-chan result.Issue {
 		}
 
 		for file, issuesToFix := range issuesToFixPerFile {
-			if err := f.fixIssuesInFile(file, issuesToFix); err != nil {
+			var err error
+			f.sw.TrackStage("all", func() {
+				err = f.fixIssuesInFile(file, issuesToFix) //nolint:scopelint
+			})
+			if err != nil {
 				f.log.Errorf("Failed to fix issues in file %s: %s", file, err)
 
 				// show issues only if can't fix them
@@ -56,6 +72,7 @@ func (f Fixer) Process(issues <-chan result.Issue) <-chan result.Issue {
 				}
 			}
 		}
+		f.printStat()
 		close(outCh)
 	}()
 

test/fix_test.go

@@ -24,7 +24,9 @@ func TestFix(t *testing.T) {
 	tmpDir := filepath.Join(testdataDir, "fix.tmp")
 	os.RemoveAll(tmpDir) // cleanup after previous runs
 
-	if os.Getenv("GL_KEEP_TEMP_FILES") != "1" {
+	if os.Getenv("GL_KEEP_TEMP_FILES") == "1" {
+		t.Logf("Temp dir for fix test: %s", tmpDir)
+	} else {
 		defer os.RemoveAll(tmpDir)
 	}
 

test/testdata/fix/in/whitespace.go

@@ -0,0 +1,47 @@
+//args: -Ewhitespace
+package p
+
+import "fmt"
+
+func oneLeadingNewline() {
+
+	fmt.Println("Hello world")
+}
+
+func oneNewlineAtBothEnds() {
+
+	fmt.Println("Hello world")
+
+}
+
+func noNewlineFunc() {
+}
+
+func oneNewlineFunc() {
+
+}
+
+func twoNewlinesFunc() {
+
+
+}
+
+func noNewlineWithCommentFunc() {
+	// some comment
+}
+
+func oneTrailingNewlineWithCommentFunc() {
+	// some comment
+
+}
+
+func oneLeadingNewlineWithCommentFunc() {
+
+	// some comment
+}
+
+func twoLeadingNewlines() {
+
+
+	fmt.Println("Hello world")
+}

test/testdata/fix/out/whitespace.go

@@ -0,0 +1,43 @@
+//args: -Ewhitespace
+package p
+
+import "fmt"
+
+func oneLeadingNewline() {
+	fmt.Println("Hello world")
+}
+
+func oneNewlineAtBothEnds() {
+	fmt.Println("Hello world")
+}
+
+func noNewlineFunc() {
+}
+
+func oneNewlineFunc() {
+
+}
+
+func twoNewlinesFunc() {
+
+
+}
+
+func noNewlineWithCommentFunc() {
+	// some comment
+}
+
+func oneTrailingNewlineWithCommentFunc() {
+	// some comment
+
+}
+
+func oneLeadingNewlineWithCommentFunc() {
+
+	// some comment
+}
+
+func twoLeadingNewlines() {
+
+	fmt.Println("Hello world")
+}