Add rule 'range-val-address' (#353)

* Add rule 'range-val-address'

* range-val-address: replace pick with visitor; avoid nesting; improve failure message

* range-val-address: not all callexprs are bad. limit to 'append'

RULES_DESCRIPTIONS.md

@@ -44,6 +44,7 @@ List of all available rules.
   - [package-comments](#package-comments)
   - [range](#range)
   - [range-val-in-closure](#range-val-in-closure)
+  - [range-val-address](#range-val-address)
   - [receiver-naming](#receiver-naming)
   - [redefines-builtin-id](#redefines-builtin-id)
   - [string-of-int](#string-of-int)
@@ -394,6 +395,12 @@ This rule warns when a range value (or index) is used inside a closure
 
 _Configuration_: N/A
 
+## range-val-address
+
+_Description_: Range variables in a loop are reused at each iteration. This rule warns when assigning the address of the variable.
+
+_Configuration_: N/A
+
 ## receiver-naming
 
 _Description_: By convention, receiver names in a method should reflect their identity. For example, if the receiver is of type `Parts`, `p` is an adequate name for it. Contrary to other languages, it is not idiomatic to name receivers as `this` or `self`.

config.go

@@ -70,6 +70,7 @@ var allRules = append([]lint.Rule{
 	&rule.FunctionResultsLimitRule{},
 	&rule.MaxPublicStructsRule{},
 	&rule.RangeValInClosureRule{},
+	&rule.RangeValAddress{},
 	&rule.WaitGroupByValueRule{},
 	&rule.AtomicRule{},
 	&rule.EmptyLinesRule{},

fixtures/range-val-address.go

@@ -0,0 +1,47 @@
+package fixtures
+
+func rangeValAddress() {
+	m := map[string]*string{}
+
+	mySlice := []string{"A", "B", "C"}
+	for _, value := range mySlice {
+		m["address"] = &value // MATCH /suspicious assignment of 'value'. range-loop variables always have the same address/
+	}
+}
+
+func rangeValAddress2() {
+	m := map[string]*string{}
+
+	mySlice := []string{"A", "B", "C"}
+	for i := range mySlice {
+		m["address"] = &mySlice[i]
+	}
+}
+
+func rangeValAddress3() {
+	m := map[string]*string{}
+
+	mySlice := []string{"A", "B", "C"}
+	for _, value := range mySlice {
+		a := &value // MATCH /suspicious assignment of 'value'. range-loop variables always have the same address/
+		m["address"] = a
+	}
+}
+
+func rangeValAddress4() {
+	m := []*string{}
+
+	mySlice := []string{"A", "B", "C"}
+	for _, value := range mySlice {
+		m = append(m, &value) // MATCH /suspicious assignment of 'value'. range-loop variables always have the same address/
+	}
+}
+
+func rangeValAddress5() {
+	m := map[*string]string{}
+
+	mySlice := []string{"A", "B", "C"}
+	for _, value := range mySlice {
+		m[&value] = value // MATCH /suspicious assignment of 'value'. range-loop variables always have the same address/
+	}
+}

rule/range-val-address.go

@@ -0,0 +1,113 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// RangeValAddress lints
+type RangeValAddress struct{}
+
+// Apply applies the rule to given file.
+func (r *RangeValAddress) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	walker := rangeValAddress{
+		onFailure: func(failure lint.Failure) {
+			failures = append(failures, failure)
+		},
+	}
+
+	ast.Walk(walker, file.AST)
+
+	return failures
+}
+
+// Name returns the rule name.
+func (r *RangeValAddress) Name() string {
+	return "range-val-address"
+}
+
+type rangeValAddress struct {
+	onFailure func(lint.Failure)
+}
+
+func (w rangeValAddress) Visit(node ast.Node) ast.Visitor {
+	n, ok := node.(*ast.RangeStmt)
+	if !ok {
+		return w
+	}
+
+	value, ok := n.Value.(*ast.Ident)
+	if !ok {
+		return w
+	}
+
+	ast.Walk(rangeBodyVisitor{
+		valueID:   value.Obj,
+		onFailure: w.onFailure,
+	}, n.Body)
+
+	return w
+}
+
+type rangeBodyVisitor struct {
+	valueID   *ast.Object
+	onFailure func(lint.Failure)
+}
+
+func (bw rangeBodyVisitor) Visit(node ast.Node) ast.Visitor {
+	asgmt, ok := node.(*ast.AssignStmt)
+	if !ok {
+		return bw
+	}
+
+	for _, exp := range asgmt.Lhs {
+		e, ok := exp.(*ast.IndexExpr)
+		if !ok {
+			continue
+		}
+		if bw.isAccessingRangeValueAddress(e.Index) { // e.g. a[&value]...
+			bw.onFailure(bw.newFailure(e.Index))
+		}
+	}
+
+	for _, exp := range asgmt.Rhs {
+		switch e := exp.(type) {
+		case *ast.UnaryExpr: // e.g. ...&value
+			if bw.isAccessingRangeValueAddress(e) {
+				bw.onFailure(bw.newFailure(e))
+			}
+		case *ast.CallExpr:
+			if fun, ok := e.Fun.(*ast.Ident); ok && fun.Name == "append" { // e.g. ...append(arr, &value)
+				for _, v := range e.Args {
+					if bw.isAccessingRangeValueAddress(v) {
+						bw.onFailure(bw.newFailure(e))
+					}
+				}
+			}
+		}
+	}
+	return bw
+}
+
+func (bw rangeBodyVisitor) isAccessingRangeValueAddress(exp ast.Expr) bool {
+	u, ok := exp.(*ast.UnaryExpr)
+	if !ok {
+		return false
+	}
+
+	v, ok := u.X.(*ast.Ident)
+	return ok && u.Op == token.AND && v.Obj == bw.valueID
+}
+
+func (bw rangeBodyVisitor) newFailure(node ast.Node) lint.Failure {
+	return lint.Failure{
+		Node:       node,
+		Confidence: 1,
+		Failure:    fmt.Sprintf("suspicious assignment of '%s'. range-loop variables always have the same address", bw.valueID.Name),
+	}
+}

test/range-val-address_test.go

@@ -0,0 +1,12 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/mgechev/revive/lint"
+	"github.com/mgechev/revive/rule"
+)
+
+func TestRangeValAddress(t *testing.T) {
+	testRule(t, "range-val-address", &rule.RangeValAddress{}, &lint.RuleConfig{})
+}