Repository: VKCOM/noverify
Pull request #565 (2020-07-05 07:09:08)

all: add experimental quickfix support
Dynamic rules get a new @fix attribute that specifies
the re-write template. When NoVerify is executed with a
new -fix flag, these templates are used to replace
matched code with a text generated by them.

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>


Patch:
diff --git a/README.md b/README.md
index e5f7aff..5e0d3d3 100644
--- a/README.md
+++ b/README.md
@@ -10,9 +10,10 @@ NoVerify is a PHP linter: it finds possible bugs and style violations in your co
 
 ## Features
 
-1. Fast: analyze ~100k LOC/s (lines of code per second) on Core i7
+1. Fast: analyze ~100k LOC/s (lines of code per second) on Core i7.
 2. Incremental: can analyze changes in git and show only new reports. Indexing speed is ~1M LOC/s.
 3. Experimental language server for VS Code and other editors that support language server protocol.
+4. Auto fixes for some warnings (when -fix flag is provided).
 
 ## Default lints
 
diff --git a/docs/dynamic-rules.md b/docs/dynamic-rules.md
index 3b9b368..f2d0876 100644
--- a/docs/dynamic-rules.md
+++ b/docs/dynamic-rules.md
@@ -210,6 +210,7 @@ TODO.
 | `@warning message...` | Set severity=warning and report text to `message`. |
 | `@info message...` | Set severity=info and report text to `message`. |
 | `@maybe message...` | Set severity=maybe and report text to `message`. |
+| `@fix template...` | Provide a quickfix template for the rule. |
 | `@scope scope_kind` | Controls where rule can be applied. `scope_kind` is `all`, `root` or `local`. |
 | `@location $var` | Selects a sub-expr from a match by a matcher var that defines report cursor position. |
 | `@type type_expr $var` | Adds "type equals to" filter, applied to `$var`. |
@@ -227,6 +228,7 @@ namespace custom;
 function redundantCast() {
   /**
    * @info excessive int cast: expression is already int-typed
+   * @fix $x
    * @location $x
    * @type int $x
    */
@@ -234,6 +236,7 @@ function redundantCast() {
 
   /**
    * @info excessive string cast: expression is already string-typed
+   * @fix $x
    * @location $x
    * @type string $x
    */
diff --git a/src/cmd/embeddedrules/rules.go b/src/cmd/embeddedrules/rules.go
index f7a507b..ce2ebed 100644
--- a/src/cmd/embeddedrules/rules.go
+++ b/src/cmd/embeddedrules/rules.go
@@ -77,7 +77,7 @@ func (fi bindataFileInfo) Sys() interface{} {
 	return nil
 }
 
-var _embeddedrulesRulesPhp = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\xac\xcd\x31\x6e\xf4\x20\x14\x04\xe0\x9e\x53\xcc\x2f\x51\xd8\x6e\xdc\xef\x1f\x89\xa4\xdf\x2e\x17\x58\x6c\x3f\xcb\x44\x2c\x20\x78\x68\x8d\xa2\xdc\x3d\xc2\x8e\x15\x57\x5b\xa5\x41\xe2\x69\xe6\x9b\x17\x15\x96\x20\x44\xdf\x75\x02\x1d\x5e\x9d\x37\x2e\x05\x1a\xd9\x78\x87\xb7\xeb\x75\x3b\x5a\xe3\x98\x22\x26\x93\xf4\x60\x49\xa0\xeb\x85\x98\xb3\xdb\x43\x4c\xd1\xe9\x58\xde\xcd\x3d\x58\x33\x97\xa6\xc5\xa7\x00\x36\x0f\xb5\x7c\xd7\x65\x20\x8c\x3e\xdb\x09\x91\x82\xd5\x23\x81\x17\x3a\x7a\x78\x18\x5e\xf0\x91\x13\x43\x8e\xde\x4d\x3f\x2d\x2e\x81\x30\x78\x6f\x4f\xd7\x5e\x60\xff\x41\x81\x63\x26\x5c\x30\x6b\x9b\xe8\xbf\x78\x32\xf8\x88\x86\x09\x3a\xe1\xd6\x54\xae\xdd\x80\xdb\x79\xe5\xdf\xdf\xce\xc8\x15\xea\x02\x59\x8e\x8d\x90\x23\x41\xae\xbf\xf4\x0a\x55\x9f\x9a\x39\xc8\xe7\x9a\xaa\xda\x5e\x36\x29\x11\x37\x72\x6d\xcf\xc6\xd7\x77\x00\x00\x00\xff\xff\x8a\x8a\x47\x68\xc4\x01\x00\x00")
+var _embeddedrulesRulesPhp = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\xac\x8d\xc1\x6a\xb4\x30\x14\x46\xf7\xf7\x29\xbe\x1f\x5c\xa8\x1b\xf7\xf3\x17\xd2\xee\x67\xd7\x17\x98\xa8\x57\xbc\x25\x93\x84\xe4\xca\x28\xa5\xef\x5e\x74\x46\xea\xa2\x14\x0a\xdd\x04\x72\xb8\xdf\x39\x4f\x26\x8e\x91\xa8\xa9\x6b\x42\x8d\x67\x1f\xc4\xe7\xc8\x9d\x4a\xf0\x78\x39\x9f\x37\xe8\xc4\x2b\x27\xf4\x92\x6d\xeb\x98\x50\x37\x44\xc3\xe4\xef\x47\xca\xc9\xdb\xb4\xbc\xca\x35\x3a\x19\x96\xb2\xc2\x3b\x01\x9b\x0f\xeb\xf8\x6a\x97\x96\xd1\x85\xc9\xf5\x48\x1c\x9d\xed\x18\x3a\xf2\xbe\xc3\x4d\x74\xc4\xdb\x94\x15\x45\x17\x7c\xff\x58\xe9\x12\x19\x6d\x08\xee\x40\x1b\xc2\xfd\x07\x03\x4d\x13\xe3\x84\xc1\xba\xcc\xff\xe9\x87\xe0\x2d\x89\x32\x6c\xc6\xa5\x5c\x75\xd5\x26\xb8\x1c\x2b\xff\xfe\x36\x53\xcc\x30\x27\x14\xcb\xde\x18\x64\xc6\xce\x1e\x28\x4e\x89\x51\xcc\x5f\xb5\x19\x66\x7d\xd6\x93\x5f\x54\xcc\x37\x15\xb3\x57\x56\xaf\xe4\xcc\x5a\x16\x73\x75\xd4\x7f\xd0\x67\x00\x00\x00\xff\xff\x38\xe8\x4b\x41\xf3\x01\x00\x00")
 
 func embeddedrulesRulesPhpBytes() ([]byte, error) {
 	return bindataRead(
diff --git a/src/cmd/embeddedrules/rules.php b/src/cmd/embeddedrules/rules.php
index f761717..612340e 100644
--- a/src/cmd/embeddedrules/rules.php
+++ b/src/cmd/embeddedrules/rules.php
@@ -20,10 +20,14 @@ function ternarySimplify() {
 
   /**
    * @maybe could rewrite as `$x ?: $y`
+   * @fix $x ?: $y
    * @pure $x
    */
   $x ? $x : $y;
 
-  /** @maybe could rewrite as `$x ?? $y` */
+  /**
+   * @maybe could rewrite as `$x ?? $y`
+   * @fix $x ?? $y
+   */
   isset($x) ? $x : $y;
-}
\ No newline at end of file
+}
diff --git a/src/cmd/flags.go b/src/cmd/flags.go
index 38767f8..44687eb 100644
--- a/src/cmd/flags.go
+++ b/src/cmd/flags.go
@@ -22,6 +22,8 @@ type cmdlineArguments struct {
 	cpuProfile string
 	memProfile string
 
+	fix bool
+
 	fullAnalysisFiles string
 	indexOnlyFiles    string
 
@@ -121,6 +123,9 @@ func bindFlags(ruleSets []*rules.Set, args *cmdlineArguments) {
 	flag.StringVar(&args.rulesList, "rules", "",
 		"Comma-separated list of rules files")
 
+	flag.BoolVar(&args.fix, "fix", false,
+		"Apply a quickfix where possible (updates source files)")
+
 	flag.StringVar(&args.gitRepo, "git", "", "Path to git repository to analyze")
 	flag.StringVar(&args.mutable.gitCommitFrom, "git-commit-from", "", "Analyze changes between commits <git-commit-from> and <git-commit-to>")
 	flag.StringVar(&args.mutable.gitCommitTo, "git-commit-to", "", "")
diff --git a/src/cmd/linter_runner.go b/src/cmd/linter_runner.go
index 8312374..709e61e 100644
--- a/src/cmd/linter_runner.go
+++ b/src/cmd/linter_runner.go
@@ -49,6 +49,8 @@ func (l *linterRunner) Init(ruleSets []*rules.Set, args *cmdlineArguments) error
 		l.outputFp = outputFp
 	}
 
+	linter.ApplyQuickFixes = l.args.fix
+
 	if err := l.compileRegexes(); err != nil {
 		return err
 	}
diff --git a/src/linter/conf.go b/src/linter/conf.go
index a92638c..3bc4c2a 100644
--- a/src/linter/conf.go
+++ b/src/linter/conf.go
@@ -20,6 +20,8 @@ var (
 	BaselineProfile      *baseline.Profile
 	ConservativeBaseline bool
 
+	ApplyQuickFixes bool
+
 	CacheDir string
 
 	// TypoFixer is a rule set for English typos correction.
diff --git a/src/linter/parser.go b/src/linter/parser.go
index 95bcbf1..53e6545 100644
--- a/src/linter/parser.go
+++ b/src/linter/parser.go
@@ -21,6 +21,7 @@ import (
 	"github.com/VKCOM/noverify/src/meta"
 	"github.com/VKCOM/noverify/src/php/parser/node"
 	"github.com/VKCOM/noverify/src/php/parser/php7"
+	"github.com/VKCOM/noverify/src/quickfix"
 	"github.com/VKCOM/noverify/src/rules"
 	"github.com/karrick/godirwalk"
 	"github.com/quasilyte/regex/syntax"
@@ -176,6 +177,12 @@ func analyzeFile(filename string, contents []byte, parser *php7.Parser, lineRang
 	}
 	w.afterLeaveFile()
 
+	if len(w.ctx.fixes) != 0 {
+		if err := quickfix.Apply(filename, contents, w.ctx.fixes); err != nil {
+			linterError(filename, "apply quickfix: %v", err)
+		}
+	}
+
 	for _, e := range parser.GetErrors() {
 		w.Report(nil, LevelError, "syntax", "Syntax error: "+e.String())
 	}
diff --git a/src/linter/root.go b/src/linter/root.go
index 2a3901a..fa6472c 100644
--- a/src/linter/root.go
+++ b/src/linter/root.go
@@ -24,6 +24,7 @@ import (
 	"github.com/VKCOM/noverify/src/php/parser/walker"
 	"github.com/VKCOM/noverify/src/phpdoc"
 	"github.com/VKCOM/noverify/src/phpgrep"
+	"github.com/VKCOM/noverify/src/quickfix"
 	"github.com/VKCOM/noverify/src/rules"
 	"github.com/VKCOM/noverify/src/solver"
 	"github.com/VKCOM/noverify/src/state"
@@ -1668,6 +1669,17 @@ func (d *RootWalker) runRule(n node.Node, sc *meta.Scope, rule *rules.Rule) {
 
 	message := d.renderRuleMessage(rule.Message, n, m)
 	d.Report(location, rule.Level, rule.Name, message)
+
+	if ApplyQuickFixes && rule.Fix != "" {
+		// As rule sets contain only enabled rules,
+		// we should be OK without any filtering here.
+		pos := n.GetPosition()
+		d.ctx.fixes = append(d.ctx.fixes, quickfix.TextEdit{
+			StartPos:    pos.StartPos,
+			EndPos:      pos.EndPos,
+			Replacement: d.renderRuleMessage(rule.Fix, n, m),
+		})
+	}
 }
 
 func (d *RootWalker) checkTypeFilter(wantType *phpdoc.Type, sc *meta.Scope, nn node.Node) bool {
diff --git a/src/linter/root_context.go b/src/linter/root_context.go
index 200b773..e7ed98f 100644
--- a/src/linter/root_context.go
+++ b/src/linter/root_context.go
@@ -7,6 +7,7 @@ import (
 	"github.com/VKCOM/noverify/src/baseline"
 	"github.com/VKCOM/noverify/src/meta"
 	"github.com/VKCOM/noverify/src/phpdoc"
+	"github.com/VKCOM/noverify/src/quickfix"
 )
 
 type rootContext struct {
@@ -20,6 +21,8 @@ type rootContext struct {
 
 	baseline     baseline.FileProfile
 	hashCounters map[uint64]int // Allocated lazily
+
+	fixes []quickfix.TextEdit
 }
 
 func newRootContext(st *meta.ClassParseState) rootContext {
diff --git a/src/quickfix/quickfix.go b/src/quickfix/quickfix.go
new file mode 100644
index 0000000..02b1c98
--- /dev/null
+++ b/src/quickfix/quickfix.go
@@ -0,0 +1,56 @@
+package quickfix
+
+import (
+	"bytes"
+	"os"
+)
+
+// TODO: add quickfixes support in lang server?
+
+// TextEdit is a suggested issue fix.
+//
+// More or less, it represents our version of the https://godoc.org/golang.org/x/tools/go/analysis#TextEdit
+// which is a part of https://godoc.org/golang.org/x/tools/go/analysis#SuggestedFix.
+type TextEdit struct {
+	StartPos int
+	EndPos   int
+
+	// Replacement is a text to be inserted as a Replacement.
+	Replacement string
+}
+
+func Apply(filename string, contents []byte, fixes []TextEdit) error {
+	if len(fixes) == 0 {
+		return nil
+	}
+
+	var buf bytes.Buffer
+	buf.Grow(len(contents))
+	writeFixes(&buf, contents, fixes)
+
+	// We don't want to create a file if it doesn't exist,
+	// hence using open instead of ioutil.WriteFile.
+	f, err := os.OpenFile(filename, os.O_WRONLY|os.O_TRUNC, 0)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	_, err = f.Write(buf.Bytes())
+	return err
+}
+
+func writeFixes(buf *bytes.Buffer, contents []byte, fixes []TextEdit) {
+	offset := 0
+	for _, fix := range fixes {
+		// If we have a nested replacement, apply only outer replacement.
+		if offset > fix.StartPos {
+			continue
+		}
+
+		buf.Write(contents[offset:fix.StartPos])
+		buf.WriteString(fix.Replacement)
+
+		offset = fix.EndPos
+	}
+	buf.Write(contents[offset:])
+}
diff --git a/src/rules/parser.go b/src/rules/parser.go
index 8aa3447..0f206d9 100644
--- a/src/rules/parser.go
+++ b/src/rules/parser.go
@@ -183,6 +183,12 @@ func (p *parser) parseRule(st node.Node) error {
 			rule.Level = lintapi.LevelMaybe
 			rule.Message = part.ParamsText
 
+		case "fix":
+			if rule.Fix != "" {
+				return p.errorf(st, "duplicated @fix")
+			}
+			rule.Fix = part.ParamsText
+
 		case "or":
 			rule.Filters = append(rule.Filters, filterSet)
 			filterSet = nil
diff --git a/src/rules/rules.go b/src/rules/rules.go
index 3fd1f08..0d4c63f 100644
--- a/src/rules/rules.go
+++ b/src/rules/rules.go
@@ -62,6 +62,9 @@ type Rule struct {
 	// Message is a report text that is printed when this rule matches.
 	Message string
 
+	// Fix is a quickfix template.
+	Fix string
+
 	// Location is a phpgrep variable name that should be used as a warning location.
 	// Empty string selects the root node.
 	Location string


Test patch:
diff --git a/src/quickfix/quickfix_test.go b/src/quickfix/quickfix_test.go
new file mode 100644
index 0000000..eb0d2de
--- /dev/null
+++ b/src/quickfix/quickfix_test.go
@@ -0,0 +1,133 @@
+package quickfix
+
+import (
+	"bytes"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+func TestQuickfixNested(t *testing.T) {
+	// Test that we don't end up with broken code if one replacement
+	// occurs inside another one.
+
+	// Imagine that we had a `$f($x)` -> `$x` rule.
+	// There are at least 3 possible outcomes:
+	// 1. g(10); -- outer call replaced
+	// 2. 10;    -- both calls replaced
+	// 3. f(10); -- inner call replaced
+	//
+	// We don't care which one will be chosen as long as the
+	// result is consistent and doesn't result in some garbage.
+	input := `f(g(10));`
+	fix1 := TextEdit{
+		StartPos:    0,
+		EndPos:      8,
+		Replacement: `g(10)`,
+	}
+	fix2 := TextEdit{
+		StartPos:    2,
+		EndPos:      7,
+		Replacement: `10`,
+	}
+
+	runTest := func(want string, fixes []TextEdit) {
+		var buf bytes.Buffer
+		writeFixes(&buf, []byte(input), fixes)
+		if buf.String() != want {
+			t.Errorf("%q %v:\nhave: `%s`\nwant: `%s`", input, fixes, buf.String(), want)
+		}
+	}
+
+	runTest(`g(10);`, []TextEdit{fix1})
+	runTest(`g(10);`, []TextEdit{fix1, fix2})
+	runTest(`f(10);`, []TextEdit{fix2})
+	runTest(`f(10);`, []TextEdit{fix2, fix1})
+
+	// Also test that we can tolerate duplicated rewrites.
+	runTest(`g(10);`, []TextEdit{fix1, fix1, fix2, fix1})
+	runTest(`f(10);`, []TextEdit{fix2, fix1, fix2, fix2})
+}
+
+func TestQuickfix(t *testing.T) {
+	tests := map[string][]string{
+		// No replacements.
+		``:      {},
+		` abc `: {},
+
+		// Empty replacements.
+		`$a$`:       {``},
+		`$a$$b$`:    {``, ``},
+		`1$a$2$b$3`: {``, ``},
+
+		// Replacement and original of the same length.
+		`  $a$$b$ `:  {`x`, `y`},
+		` $a$ $b$  `: {`x`, `y`},
+
+		// Replacement longer than original.
+		`$aa$ $bb$ $cc$`:     {`xxx`, `yyy`, `zzz`},
+		`  $aa$  $bb$ $cc$`:  {`xxx`, `yyy`, `zzz`},
+		` $aa$ $bb$  $cc$  `: {`xxx`, `yyy`, `zzz`},
+
+		// Original longer than replacement.
+		`$aa$ $bbb$ $cc$`:    {`x`, `yy`, `z`},
+		` $aa$  $bbb$ $cc$ `: {`x`, `yy`, `z`},
+		`$aa$ $bbb$  $cc$`:   {`x`, `yy`, `z`},
+
+		// Mixed lengths.
+		` $a$    $bb$  $ccc$ `:    {`xx`, `y`, `zz`},
+		` $aaa$    $bb$   $cc$  `: {`xxx`, `y`, `z`},
+		` $aaa$ $bbb$ $cccc$ $d$`: {`x`, `yyyyyyy`, `zzz`, ``},
+		` $d$$aaa$  $bbb$$cccc$`:  {`xxxxxx`, `y`, ``, `zz`},
+		`$aaaaaaaaaaaaa$$b$`:      {`x`, `yyyy`},
+		`$a$$bbbbb$`:              {`yyyy`, `x`},
+
+		// Multiline.
+		`<?php
+foo = $x ?
+       x :
+       y$;`: {`x ?: y`},
+	}
+
+	type testCase struct {
+		input string
+		want  string
+		fixes []TextEdit
+	}
+	varRegex := regexp.MustCompile(`\$[^$]+?\$`)
+	createTestCase := func(s string, replacements []string) testCase {
+		i := -1
+		withReplacements := varRegex.ReplaceAllStringFunc(s, func(x string) string {
+			i++
+			return replacements[i]
+		})
+
+		fixes := make([]TextEdit, len(replacements))
+
+		for i, m := range varRegex.FindAllStringIndex(s, -1) {
+			fixup := i * len("$") * 2
+			begin := m[0]
+			end := m[1]
+			fixes[i] = TextEdit{
+				StartPos:    begin - fixup,
+				EndPos:      (end - len("$")*2) - fixup,
+				Replacement: replacements[i],
+			}
+		}
+
+		return testCase{
+			input: strings.ReplaceAll(s, "$", ""),
+			want:  withReplacements,
+			fixes: fixes,
+		}
+	}
+
+	for s, replacements := range tests {
+		test := createTestCase(s, replacements)
+		var buf bytes.Buffer
+		writeFixes(&buf, []byte(test.input), test.fixes)
+		if buf.String() != test.want {
+			t.Errorf("%q %q:\nhave: `%s`\nwant: `%s`", test.input, replacements, buf.String(), test.want)
+		}
+	}
+}