From 3d78f64b6043878b153c012eec9501ee125a5202 Mon Sep 17 00:00:00 2001
From: Denis Isaev <denis@golangci.com>
Date: Sun, 9 Jun 2019 17:30:45 +0300
Subject: [PATCH] fix #522: run misspell in text mode

Treat Go source files as plain text files by misspell: it allows detecting
issues in strings, variable names, etc. Also, it's the default mode of
a standalone misspell tool.

Also, implement richer and more stable auto-fix of misspell issues:
now it can fix multiple issues in one line.
---
 pkg/commands/executor.go                     |  2 +-
 pkg/golinters/gocritic.go                    |  2 +-
 pkg/golinters/misspell.go                    | 66 ++++++++++----
 pkg/lint/linter/context.go                   | 10 ++-
 pkg/lint/load.go                             | 18 +++-
 pkg/lint/runner.go                           |  3 +-
 pkg/result/issue.go                          |  7 ++
 pkg/result/processors/fixer.go               | 82 ++++++++++++++++-
 pkg/result/processors/max_from_linter.go     | 10 +--
 pkg/result/processors/max_same_issues.go     | 10 +--
 pkg/result/processors/replacement_builder.go | 92 --------------------
 pkg/result/processors/uniq_by_line.go        | 11 ++-
 pkg/result/processors/uniq_by_line_test.go   |  4 +-
 test/testdata/fix/in/misspell.go             |  7 +-
 test/testdata/fix/out/misspell.go            |  9 +-
 15 files changed, 194 insertions(+), 139 deletions(-)
 delete mode 100644 pkg/result/processors/replacement_builder.go

diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go
index 6bf64fd4..1f6231e9 100644
--- a/pkg/commands/executor.go
+++ b/pkg/commands/executor.go
@@ -96,9 +96,9 @@ func NewExecutor(version, commit, date string) *Executor {
 	e.EnabledLintersSet = lintersdb.NewEnabledSet(e.DBManager,
 		lintersdb.NewValidator(e.DBManager), e.log.Child("lintersdb"), e.cfg)
 	e.goenv = goutil.NewEnv(e.log.Child("goenv"))
-	e.contextLoader = lint.NewContextLoader(e.cfg, e.log.Child("loader"), e.goenv)
 	e.fileCache = fsutils.NewFileCache()
 	e.lineCache = fsutils.NewLineCache(e.fileCache)
+	e.contextLoader = lint.NewContextLoader(e.cfg, e.log.Child("loader"), e.goenv, e.lineCache, e.fileCache)
 
 	return e
 }
diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go
index 669c57b3..63b9b154 100644
--- a/pkg/golinters/gocritic.go
+++ b/pkg/golinters/gocritic.go
@@ -109,7 +109,7 @@ func (lint Gocritic) Run(ctx context.Context, lintCtx *linter.Context) ([]result
 	go func() {
 		defer func() {
 			if err := recover(); err != nil {
-				panicErr = fmt.Errorf("panic occured: %s", err)
+				panicErr = fmt.Errorf("panic occurred: %s", err)
 				lintCtx.Log.Warnf("Panic: %s", debug.Stack())
 			}
 		}()
diff --git a/pkg/golinters/misspell.go b/pkg/golinters/misspell.go
index 3936966d..d7afceac 100644
--- a/pkg/golinters/misspell.go
+++ b/pkg/golinters/misspell.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"go/token"
-	"io/ioutil"
 	"strings"
 
 	"github.com/golangci/misspell"
@@ -15,6 +14,10 @@ import (
 
 type Misspell struct{}
 
+func NewMisspell() *Misspell {
+	return &Misspell{}
+}
+
 func (Misspell) Name() string {
 	return "misspell"
 }
@@ -50,25 +53,52 @@ func (lint Misspell) Run(ctx context.Context, lintCtx *linter.Context) ([]result
 
 	var res []result.Issue
 	for _, f := range getAllFileNames(lintCtx) {
-		fileContent, err := ioutil.ReadFile(f)
+		issues, err := lint.runOnFile(f, &r, lintCtx)
 		if err != nil {
-			return nil, fmt.Errorf("can't read file %s: %s", f, err)
-		}
-
-		_, diffs := r.ReplaceGo(string(fileContent))
-		for _, diff := range diffs {
-			text := fmt.Sprintf("`%s` is a misspelling of `%s`", diff.Original, diff.Corrected)
-			pos := token.Position{
-				Filename: f,
-				Line:     diff.Line,
-				Column:   diff.Column + 1,
-			}
-			res = append(res, result.Issue{
-				Pos:        pos,
-				Text:       text,
-				FromLinter: lint.Name(),
-			})
+			return nil, err
 		}
+		res = append(res, issues...)
+	}
+
+	return res, nil
+}
+
+func (lint Misspell) runOnFile(fileName string, r *misspell.Replacer, lintCtx *linter.Context) ([]result.Issue, error) {
+	var res []result.Issue
+	fileContent, err := lintCtx.FileCache.GetFileBytes(fileName)
+	if err != nil {
+		return nil, fmt.Errorf("can't get file %s contents: %s", fileName, err)
+	}
+
+	// use r.Replace, not r.ReplaceGo because r.ReplaceGo doesn't find
+	// issues inside strings: it searches only inside comments. r.Replace
+	// searches all words: it treats input as a plain text. A standalone misspell
+	// tool uses r.Replace by default.
+	_, diffs := r.Replace(string(fileContent))
+	for _, diff := range diffs {
+		text := fmt.Sprintf("`%s` is a misspelling of `%s`", diff.Original, diff.Corrected)
+		pos := token.Position{
+			Filename: fileName,
+			Line:     diff.Line,
+			Column:   diff.Column + 1,
+		}
+		var replacement *result.Replacement
+		if lintCtx.Cfg.Issues.NeedFix {
+			replacement = &result.Replacement{
+				Inline: &result.InlineFix{
+					StartCol:  diff.Column,
+					Length:    len(diff.Original),
+					NewString: diff.Corrected,
+				},
+			}
+		}
+
+		res = append(res, result.Issue{
+			Pos:         pos,
+			Text:        text,
+			FromLinter:  lint.Name(),
+			Replacement: replacement,
+		})
 	}
 
 	return res, nil
diff --git a/pkg/lint/linter/context.go b/pkg/lint/linter/context.go
index f9aeb233..9c3d5040 100644
--- a/pkg/lint/linter/context.go
+++ b/pkg/lint/linter/context.go
@@ -5,6 +5,8 @@ import (
 	"golang.org/x/tools/go/packages"
 	"golang.org/x/tools/go/ssa"
 
+	"github.com/golangci/golangci-lint/pkg/fsutils"
+
 	"github.com/golangci/golangci-lint/pkg/config"
 	"github.com/golangci/golangci-lint/pkg/lint/astcache"
 	"github.com/golangci/golangci-lint/pkg/logutils"
@@ -19,9 +21,11 @@ type Context struct {
 
 	SSAProgram *ssa.Program // for unparam and interfacer but not for megacheck (it change it)
 
-	Cfg      *config.Config
-	ASTCache *astcache.Cache
-	Log      logutils.Log
+	Cfg       *config.Config
+	ASTCache  *astcache.Cache
+	FileCache *fsutils.FileCache
+	LineCache *fsutils.LineCache
+	Log       logutils.Log
 }
 
 func (c *Context) Settings() *config.LintersSettings {
diff --git a/pkg/lint/load.go b/pkg/lint/load.go
index fa041509..63e2076f 100644
--- a/pkg/lint/load.go
+++ b/pkg/lint/load.go
@@ -11,6 +11,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/golangci/golangci-lint/pkg/fsutils"
+
 	"github.com/pkg/errors"
 	"golang.org/x/tools/go/loader"
 	"golang.org/x/tools/go/packages"
@@ -31,15 +33,21 @@ type ContextLoader struct {
 	debugf      logutils.DebugFunc
 	goenv       *goutil.Env
 	pkgTestIDRe *regexp.Regexp
+	lineCache   *fsutils.LineCache
+	fileCache   *fsutils.FileCache
 }
 
-func NewContextLoader(cfg *config.Config, log logutils.Log, goenv *goutil.Env) *ContextLoader {
+func NewContextLoader(cfg *config.Config, log logutils.Log, goenv *goutil.Env,
+	lineCache *fsutils.LineCache, fileCache *fsutils.FileCache) *ContextLoader {
+
 	return &ContextLoader{
 		cfg:         cfg,
 		log:         log,
 		debugf:      logutils.Debug("loader"),
 		goenv:       goenv,
 		pkgTestIDRe: regexp.MustCompile(`^(.*) \[(.*)\.test\]`),
+		lineCache:   lineCache,
+		fileCache:   fileCache,
 	}
 }
 
@@ -356,9 +364,11 @@ func (cl ContextLoader) Load(ctx context.Context, linters []*linter.Config) (*li
 			Cwd:   "",  // used by depguard and fallbacked to os.Getcwd
 			Build: nil, // used by depguard and megacheck and fallbacked to build.Default
 		},
-		Cfg:      cl.cfg,
-		ASTCache: astCache,
-		Log:      cl.log,
+		Cfg:       cl.cfg,
+		ASTCache:  astCache,
+		Log:       cl.log,
+		FileCache: cl.fileCache,
+		LineCache: cl.lineCache,
 	}
 
 	separateNotCompilingPackages(ret)
diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go
index cf3a3784..fd700a49 100644
--- a/pkg/lint/runner.go
+++ b/pkg/lint/runner.go
@@ -79,13 +79,12 @@ func NewRunner(astCache *astcache.Cache, cfg *config.Config, log logutils.Log, g
 			processors.NewExcludeRules(excludeRules, lineCache, log.Child("exclude_rules")),
 			processors.NewNolint(astCache, log.Child("nolint"), dbManager),
 
-			processors.NewUniqByLine(),
+			processors.NewUniqByLine(cfg),
 			processors.NewDiff(icfg.Diff, icfg.DiffFromRevision, icfg.DiffPatchFilePath),
 			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(lineCache, 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 aaa24eee..d3499216 100644
--- a/pkg/result/issue.go
+++ b/pkg/result/issue.go
@@ -9,6 +9,13 @@ type Range struct {
 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
+	Inline         *InlineFix
+}
+
+type InlineFix struct {
+	StartCol  int // zero-based
+	Length    int // length of chunk to be replaced
+	NewString string
 }
 
 type Issue struct {
diff --git a/pkg/result/processors/fixer.go b/pkg/result/processors/fixer.go
index 2c7888e9..a5497b0c 100644
--- a/pkg/result/processors/fixer.go
+++ b/pkg/result/processors/fixer.go
@@ -77,6 +77,19 @@ func (f Fixer) fixIssuesInFile(filePath string, issues []result.Issue) error {
 		return errors.Wrapf(err, "failed to make file %s", tmpFileName)
 	}
 
+	// merge multiple issues per line into one issue
+	issuesPerLine := map[int][]result.Issue{}
+	for _, i := range issues {
+		issuesPerLine[i.Line()] = append(issuesPerLine[i.Line()], i)
+	}
+
+	issues = issues[:0] // reuse the same memory
+	for line, lineIssues := range issuesPerLine {
+		if mergedIssue := f.mergeLineIssues(line, lineIssues, origFileLines); mergedIssue != nil {
+			issues = append(issues, *mergedIssue)
+		}
+	}
+
 	issues = f.findNotIntersectingIssues(issues)
 
 	if err = f.writeFixedFile(origFileLines, issues, tmpOutFile); err != nil {
@@ -94,9 +107,76 @@ func (f Fixer) fixIssuesInFile(filePath string, issues []result.Issue) error {
 	return nil
 }
 
+//nolint:gocyclo
+func (f Fixer) mergeLineIssues(lineNum int, lineIssues []result.Issue, origFileLines [][]byte) *result.Issue {
+	origLine := origFileLines[lineNum-1] // lineNum is 1-based
+
+	if len(lineIssues) == 1 && lineIssues[0].Replacement.Inline == nil {
+		return &lineIssues[0]
+	}
+
+	// check issues first
+	for _, i := range lineIssues {
+		if i.LineRange != nil {
+			f.log.Infof("Line %d has multiple issues but at least one of them is ranged: %#v", lineNum, lineIssues)
+			return &lineIssues[0]
+		}
+
+		r := i.Replacement
+		if r.Inline == nil || len(r.NewLines) != 0 || r.NeedOnlyDelete {
+			f.log.Infof("Line %d has multiple issues but at least one of them isn't inline: %#v", lineNum, lineIssues)
+			return &lineIssues[0]
+		}
+
+		if r.Inline.StartCol < 0 || r.Inline.Length <= 0 || r.Inline.StartCol+r.Inline.Length > len(origLine) {
+			f.log.Warnf("Line %d (%q) has invalid inline fix: %#v, %#v", lineNum, origLine, i, r.Inline)
+			return nil
+		}
+	}
+
+	return f.applyInlineFixes(lineIssues, origLine, lineNum)
+}
+
+func (f Fixer) applyInlineFixes(lineIssues []result.Issue, origLine []byte, lineNum int) *result.Issue {
+	sort.Slice(lineIssues, func(i, j int) bool {
+		return lineIssues[i].Replacement.Inline.StartCol < lineIssues[j].Replacement.Inline.StartCol
+	})
+
+	var newLineBuf bytes.Buffer
+	newLineBuf.Grow(len(origLine))
+
+	//nolint:misspell
+	// example: origLine="it's becouse of them", StartCol=5, Length=7, NewString="because"
+
+	curOrigLinePos := 0
+	for _, i := range lineIssues {
+		fix := i.Replacement.Inline
+		if fix.StartCol < curOrigLinePos {
+			f.log.Warnf("Line %d has multiple intersecting issues: %#v", lineNum, lineIssues)
+			return nil
+		}
+
+		if curOrigLinePos != fix.StartCol {
+			newLineBuf.Write(origLine[curOrigLinePos:fix.StartCol])
+		}
+		newLineBuf.WriteString(fix.NewString)
+		curOrigLinePos = fix.StartCol + fix.Length
+	}
+	if curOrigLinePos != len(origLine) {
+		newLineBuf.Write(origLine[curOrigLinePos:])
+	}
+
+	mergedIssue := lineIssues[0] // use text from the first issue (it's not really used)
+	mergedIssue.Replacement = &result.Replacement{
+		NewLines: []string{newLineBuf.String()},
+	}
+	return &mergedIssue
+}
+
 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
+		a, b := issues[i], issues[j] //nolint:scopelint
+		return a.Line() < b.Line()
 	})
 
 	var ret []result.Issue
diff --git a/pkg/result/processors/max_from_linter.go b/pkg/result/processors/max_from_linter.go
index 0129cc8c..c58666c5 100644
--- a/pkg/result/processors/max_from_linter.go
+++ b/pkg/result/processors/max_from_linter.go
@@ -33,12 +33,12 @@ 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 {
+		if i.Replacement != nil && p.cfg.Issues.NeedFix {
+			// we need to fix all issues at once => we need to return all of them
+			return true
+		}
+
 		p.lc[i.FromLinter]++ // always inc for stat
 		return p.lc[i.FromLinter] <= p.limit
 	}), nil
diff --git a/pkg/result/processors/max_same_issues.go b/pkg/result/processors/max_same_issues.go
index 4f0af44d..43a05936 100644
--- a/pkg/result/processors/max_same_issues.go
+++ b/pkg/result/processors/max_same_issues.go
@@ -38,12 +38,12 @@ 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 {
+		if i.Replacement != nil && p.cfg.Issues.NeedFix {
+			// we need to fix all issues at once => we need to return all of them
+			return true
+		}
+
 		p.tc[i.Text]++ // always inc for stat
 		return p.tc[i.Text] <= p.limit
 	}), nil
diff --git a/pkg/result/processors/replacement_builder.go b/pkg/result/processors/replacement_builder.go
deleted file mode 100644
index 352fb66d..00000000
--- a/pkg/result/processors/replacement_builder.go
+++ /dev/null
@@ -1,92 +0,0 @@
-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/pkg/result/processors/uniq_by_line.go b/pkg/result/processors/uniq_by_line.go
index 402ed5e2..d0960d83 100644
--- a/pkg/result/processors/uniq_by_line.go
+++ b/pkg/result/processors/uniq_by_line.go
@@ -1,6 +1,7 @@
 package processors
 
 import (
+	"github.com/golangci/golangci-lint/pkg/config"
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
@@ -9,11 +10,13 @@ type fileToLineToCount map[string]lineToCount
 
 type UniqByLine struct {
 	flc fileToLineToCount
+	cfg *config.Config
 }
 
-func NewUniqByLine() *UniqByLine {
+func NewUniqByLine(cfg *config.Config) *UniqByLine {
 	return &UniqByLine{
 		flc: fileToLineToCount{},
+		cfg: cfg,
 	}
 }
 
@@ -25,6 +28,12 @@ func (p UniqByLine) Name() string {
 
 func (p *UniqByLine) Process(issues []result.Issue) ([]result.Issue, error) {
 	return filterIssues(issues, func(i *result.Issue) bool {
+		if i.Replacement != nil && p.cfg.Issues.NeedFix {
+			// if issue will be auto-fixed we shouldn't collapse issues:
+			// e.g. one line can contain 2 misspellings, they will be in 2 issues and misspell should fix both of them.
+			return true
+		}
+
 		lc := p.flc[i.FilePath()]
 		if lc == nil {
 			lc = lineToCount{}
diff --git a/pkg/result/processors/uniq_by_line_test.go b/pkg/result/processors/uniq_by_line_test.go
index fc13a8a5..6246f632 100644
--- a/pkg/result/processors/uniq_by_line_test.go
+++ b/pkg/result/processors/uniq_by_line_test.go
@@ -4,6 +4,8 @@ import (
 	"go/token"
 	"testing"
 
+	"github.com/golangci/golangci-lint/pkg/config"
+
 	"github.com/golangci/golangci-lint/pkg/result"
 )
 
@@ -17,7 +19,7 @@ func newFLIssue(file string, line int) result.Issue {
 }
 
 func TestUniqByLine(t *testing.T) {
-	p := NewUniqByLine()
+	p := NewUniqByLine(&config.Config{})
 	i1 := newFLIssue("f1", 1)
 
 	processAssertSame(t, p, i1)
diff --git a/test/testdata/fix/in/misspell.go b/test/testdata/fix/in/misspell.go
index 151c8473..171c6b9b 100644
--- a/test/testdata/fix/in/misspell.go
+++ b/test/testdata/fix/in/misspell.go
@@ -1,6 +1,8 @@
 //args: -Emisspell
 package p
 
+import "log"
+
 // langauge lala
 // lala langauge
 // langauge
@@ -9,6 +11,7 @@ package p
 // check Langauge
 // and check langAuge
 
-func langaugeMisspell() { // the function detects langauge of the text
-
+func langaugeMisspell() {
+	var langauge, langaugeAnd string
+	log.Printf("it's becouse of them: %s, %s", langauge, langaugeAnd)
 }
diff --git a/test/testdata/fix/out/misspell.go b/test/testdata/fix/out/misspell.go
index 075911cb..98ea5144 100644
--- a/test/testdata/fix/out/misspell.go
+++ b/test/testdata/fix/out/misspell.go
@@ -1,14 +1,17 @@
 //args: -Emisspell
 package p
 
+import "log"
+
 // language lala
 // lala language
 // language
-// language langauge
+// language language
 
 // check Language
 // and check langAuge
 
-func langaugeMisspell() { // the function detects language of the text
-
+func langaugeMisspell() {
+	var language, langaugeAnd string
+	log.Printf("it's because of them: %s, %s", language, langaugeAnd)
 }