ruleguard: autofix some interpolated expressions (#353)

Given this pattern:

	io.WriteString($w, $s)

And this input string:

	io.WriteString(&buf, s)

With this replacement template:

	$w.WriteString($s)

We'll get this result:

	&buf.WriteString(s)

That's incorrect.

To avoid that, ruleguard attempts to fix some interpolated
expressions when it can understand the context.

In the cases above, it sees that variable is followed by `.`
while `$w` contains unary `&` expression. It knows that
`$w.x` is identical to `(&$w).w`, so we can simply insert
`buf` instead of `&buf` and get `buf.WriteString(s)`.

This greatly simplifies the creation of reliable rules
that just work.

analyzer/testdata/src/quickfix/rangeClause.go

@@ -1,5 +1,10 @@
 package quickfix
 
+import (
+	"bytes"
+	"io"
+)
+
 func rangeRuneSlice(s string) {
 	for _, ch := range []rune(s) { // want `\Qsuggestion: range s`
 		println(ch)
@@ -28,6 +33,43 @@ func rangeRuneSlice(s string) {
 	}
 }
 
+func writeString() {
+	{
+		var buf bytes.Buffer
+		io.WriteString(&buf, "example") // want `\Qbuf.WriteString("example")`
+	}
+	{
+		var buf bytes.Buffer
+		io.WriteString((&buf), "example") // want `\Q(&buf).WriteString("example")`
+		(&buf).WriteString("example")
+	}
+	{
+		buf := &bytes.Buffer{}
+		io.WriteString(buf, "example") // want `\Qbuf.WriteString("example")`
+	}
+	{
+		var buffers [4]bytes.Buffer
+		io.WriteString(&buffers[0], "str") // want `\Qbuffers[0].WriteString("str")`
+		buffers[0].WriteString("str")
+	}
+	{
+		type withBuffer struct {
+			buf bytes.Buffer
+		}
+		var o withBuffer
+		io.WriteString(&o.buf, "foo") // want `\Qo.buf.WriteString("foo")`
+		o.buf.WriteString("foo")
+	}
+	{
+		type withBufferPtr struct {
+			buf *bytes.Buffer
+		}
+		var o withBufferPtr
+		io.WriteString(o.buf, "foo") // want `\Qo.buf.WriteString("foo")`
+		o.buf.WriteString("foo")
+	}
+}
+
 func getString() string {
 	return "123"
 }

analyzer/testdata/src/quickfix/rangeClause.go.golden

@@ -1,5 +1,10 @@
 package quickfix
 
+import (
+	"bytes"
+	"io"
+)
+
 func rangeRuneSlice(s string) {
 	for _, ch := range s { // want `\Qsuggestion: range s`
 		println(ch)
@@ -28,6 +33,43 @@ func rangeRuneSlice(s string) {
 	}
 }
 
+func writeString() {
+	{
+		var buf bytes.Buffer
+		buf.WriteString("example") // want `\Qbuf.WriteString("example")`
+	}
+	{
+		var buf bytes.Buffer
+		(&buf).WriteString("example") // want `\Q(&buf).WriteString("example")`
+		(&buf).WriteString("example")
+	}
+	{
+		buf := &bytes.Buffer{}
+		buf.WriteString("example") // want `\Qbuf.WriteString("example")`
+	}
+	{
+		var buffers [4]bytes.Buffer
+		buffers[0].WriteString("str") // want `\Qbuffers[0].WriteString("str")`
+		buffers[0].WriteString("str")
+	}
+	{
+		type withBuffer struct {
+			buf bytes.Buffer
+		}
+		var o withBuffer
+		o.buf.WriteString("foo") // want `\Qo.buf.WriteString("foo")`
+		o.buf.WriteString("foo")
+	}
+	{
+		type withBufferPtr struct {
+			buf *bytes.Buffer
+		}
+		var o withBufferPtr
+		o.buf.WriteString("foo") // want `\Qo.buf.WriteString("foo")`
+		o.buf.WriteString("foo")
+	}
+}
+
 func getString() string {
 	return "123"
 }

analyzer/testdata/src/quickfix/rules.go

@@ -10,3 +10,9 @@ func rangeRuneSlice(m dsl.Matcher) {
 		Where(m["s"].Type.Is(`string`)).
 		Suggest(`range $s`)
 }
+
+func writeString(m dsl.Matcher) {
+	m.Match(`io.WriteString($w, $s)`).
+		Where(m["w"].Type.HasMethod(`io.StringWriter.WriteString`)).
+		Suggest(`$w.WriteString($s)`)
+}

ruleguard/ruleguard_test.go

@@ -29,7 +29,7 @@ func TestTruncateText(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		have := truncateText(test.input, test.maxLen)
+		have := string(truncateText([]byte(test.input), test.maxLen))
 		if len(have) > test.maxLen {
 			t.Errorf("truncateText(%q, %v): len %d exceeeds max len",
 				test.input, test.maxLen, len(have))

ruleguard/runner.go

@@ -7,6 +7,7 @@ import (
 	"go/ast"
 	"go/build"
 	"go/printer"
+	"go/token"
 	"io/ioutil"
 	"path/filepath"
 	"reflect"
@@ -370,57 +371,107 @@ func (rr *rulesRunner) collectImports(f *ast.File) {
 }
 
 func (rr *rulesRunner) renderMessage(msg string, m matchData, truncate bool) string {
-	var buf strings.Builder
-	if strings.Contains(msg, "$$") {
-		buf.Write(rr.nodeText(m.Node()))
-		msg = strings.ReplaceAll(msg, "$$", buf.String())
-	}
-	if len(m.CaptureList()) == 0 {
+	if !strings.Contains(msg, "$") {
 		return msg
 	}
 
-	capture := make([]gogrep.CapturedNode, len(m.CaptureList()))
-	copy(capture, m.CaptureList())
-	sort.Slice(capture, func(i, j int) bool {
-		return len(capture[i].Name) > len(capture[j].Name)
-	})
+	var capture []gogrep.CapturedNode
+	if len(m.CaptureList()) != 0 {
+		capture = make([]gogrep.CapturedNode, 0, len(m.CaptureList()))
+		for _, c := range m.CaptureList() {
+			n := c.Node
+			// Some captured nodes are typed, but nil.
+			// We can't really get their text, so skip them here.
+			// For example, pattern `func $_() $results { $*_ }` may
+			// match a nil *ast.FieldList for $results if executed
+			// against a function with no results.
+			if reflect.ValueOf(n).IsNil() && !gogrep.IsEmptyNodeSlice(n) {
+				continue
+			}
+			capture = append(capture, c)
+		}
+		if len(capture) > 1 {
+			sort.Slice(capture, func(i, j int) bool {
+				return len(capture[i].Name) > len(capture[j].Name)
+			})
+		}
+	}
 
-	for _, c := range capture {
-		n := c.Node
-		// Some captured nodes are typed, but nil.
-		// We can't really get their text, so skip them here.
-		// For example, pattern `func $_() $results { $*_ }` may
-		// match a nil *ast.FieldList for $results if executed
-		// against a function with no results.
-		if reflect.ValueOf(n).IsNil() && !gogrep.IsEmptyNodeSlice(n) {
-			continue
+	result := make([]byte, 0, len(msg)*2)
+	i := 0
+	for {
+		j := strings.IndexByte(msg[i:], '$')
+		if j == -1 {
+			result = append(result, msg[i:]...)
+			break
 		}
-		key := "$" + c.Name
-		if !strings.Contains(msg, key) {
-			continue
+		dollarPos := i + j
+		result = append(result, msg[i:dollarPos]...)
+		var n ast.Node
+		var nameLen int
+		if strings.HasPrefix(msg[dollarPos+1:], "$") {
+			n = m.Node()
+			nameLen = 1
+		} else {
+			for _, c := range capture {
+				if strings.HasPrefix(msg[dollarPos+1:], c.Name) {
+					n = c.Node
+					nameLen = len(c.Name)
+					break
+				}
+			}
 		}
-		buf.Reset()
-		buf.Write(rr.nodeText(n))
-		replacement := buf.String()
-		if truncate {
-			replacement = truncateText(replacement, 60)
+		if n != nil {
+			text := rr.nodeText(n)
+			text = rr.fixedText(text, n, msg[dollarPos+1+nameLen:])
+			if truncate {
+				text = truncateText(text, 60)
+			}
+			result = append(result, text...)
+		} else {
+			result = append(result, '$')
+		}
+		i = dollarPos + len("$") + nameLen
+	}
+
+	return string(result)
+}
+
+func (rr *rulesRunner) fixedText(text []byte, n ast.Node, following string) []byte {
+	// pattern=`$x.y` $x=`&buf` following=`.y`
+	// Insert $x as `buf`, so we get `buf.y` instead of incorrect `&buf.y`.
+	if n, ok := n.(*ast.UnaryExpr); ok && n.Op == token.AND {
+		shouldFix := false
+		switch n.X.(type) {
+		case *ast.Ident, *ast.IndexExpr, *ast.SelectorExpr:
+			shouldFix = true
+		}
+		if shouldFix && strings.HasPrefix(following, ".") {
+			return bytes.TrimPrefix(text, []byte("&"))
 		}
-		msg = strings.ReplaceAll(msg, key, replacement)
 	}
-	return msg
+
+	return text
 }
 
-func truncateText(s string, maxLen int) string {
-	const placeholder = "<...>"
-	if len(s) <= maxLen-len(placeholder) {
+var longTextPlaceholder = []byte("<...>")
+
+func truncateText(s []byte, maxLen int) []byte {
+	if len(s) <= maxLen-len(longTextPlaceholder) {
 		return s
 	}
-	maxLen -= len(placeholder)
+	maxLen -= len(longTextPlaceholder)
 	leftLen := maxLen / 2
 	rightLen := (maxLen % 2) + leftLen
 	left := s[:leftLen]
 	right := s[len(s)-rightLen:]
-	return left + placeholder + right
+
+	result := make([]byte, 0, len(left)+len(longTextPlaceholder)+len(right))
+	result = append(result, left...)
+	result = append(result, longTextPlaceholder...)
+	result = append(result, right...)
+
+	return result
 }
 
 var multiMatchTags = [nodetag.NumBuckets]bool{