S1011: flag index-based loops

Closes gh-881
Closes gh-978

Co-authored-by: Dominik Honnef <[email protected]>

Author: matsuyoshi30

simple/analysis.go

@@ -55,7 +55,7 @@ var Analyzers = lint.InitializeAnalyzers(Docs, map[string]*analysis.Analyzer{
 	},
 	"S1011": {
 		Run:      CheckLoopAppend,
-		Requires: []*analysis.Analyzer{inspect.Analyzer, facts.Generated},
+		Requires: []*analysis.Analyzer{inspect.Analyzer, facts.Generated, facts.Purity},
 	},
 	"S1012": {
 		Run:      CheckTimeSince,

simple/doc.go

@@ -137,8 +137,21 @@ making \'s[n:len(s)]\' and \'s[n:]\' equivalent.`,
 		Before: `
 for _, e := range y {
     x = append(x, e)
+}
+
+for i := range y {
+    x = append(x, y[i])
+}
+
+for i := range y {
+    v := y[i]
+    x = append(x, v)
 }`,
-		After: `x = append(x, y...)`,
+
+		After: `
+x = append(x, y...)
+x = append(x, y...)
+x = append(x, y...)`,
 		Since: "2017.1",
 		// MergeIfAll because y might not be a slice under all build tags.
 		MergeIf: lint.MergeIfAll,
@@ -267,9 +280,9 @@ Given the following shared definitions
 
     type T1 string
     type T2 int
-    
+
     func (T2) String() string { return "Hello, world" }
-    
+
     var x string
     var y T1
     var z T2

simple/lint.go

@@ -13,6 +13,7 @@ import (
 
 	"honnef.co/go/tools/analysis/code"
 	"honnef.co/go/tools/analysis/edit"
+	"honnef.co/go/tools/analysis/facts"
 	"honnef.co/go/tools/analysis/lint"
 	"honnef.co/go/tools/analysis/report"
 	"honnef.co/go/tools/go/ast/astutil"
@@ -767,22 +768,44 @@ func refersTo(pass *analysis.Pass, expr ast.Expr, ident types.Object) bool {
 }
 
 var checkLoopAppendQ = pattern.MustParse(`
+(Or
 	(RangeStmt
 		(Ident "_")
 		val@(Object _)
 		_
 		x
-		[(AssignStmt [lhs] "=" [(CallExpr (Builtin "append") [lhs val])])]) `)
+		[(AssignStmt [lhs] "=" [(CallExpr (Builtin "append") [lhs val])])])
+	(RangeStmt
+		idx@(Ident _)
+		nil
+		_
+		x
+		[(AssignStmt [lhs] "=" [(CallExpr (Builtin "append") [lhs (IndexExpr x idx)])])])
+	(RangeStmt
+		idx@(Ident _)
+		nil
+		_
+		x
+		[(AssignStmt val@(Object _) ":=" (IndexExpr x idx))
+		(AssignStmt [lhs] "=" [(CallExpr (Builtin "append") [lhs val])])]))`)
 
 func CheckLoopAppend(pass *analysis.Pass) (interface{}, error) {
+	pure := pass.ResultOf[facts.Purity].(facts.PurityResult)
+
 	fn := func(node ast.Node) {
 		m, ok := code.Match(pass, checkLoopAppendQ, node)
 		if !ok {
 			return
 		}
 
-		val := m.State["val"].(types.Object)
-		if refersTo(pass, m.State["lhs"].(ast.Expr), val) {
+		val, ok := m.State["val"].(types.Object)
+		if ok && refersTo(pass, m.State["lhs"].(ast.Expr), val) {
+			return
+		}
+
+		if m.State["idx"] != nil && code.MayHaveSideEffects(pass, m.State["x"].(ast.Expr), pure) {
+			// When using an index-based loop, x gets evaluated repeatedly and thus should be pure.
+			// This doesn't matter for value-based loops, because x only gets evaluated once.
 			return
 		}
 

simple/testdata/src/loop-append/loop-append.go

@@ -17,6 +17,22 @@ func fn1() {
 		b = append(b, v)
 	}
 
+	var a2, b2 []int
+	for i := range a2 { // want `should replace loop`
+		b2 = append(b2, a2[i])
+	}
+
+	var a3, b3 []int
+	for i := range a3 { // want `should replace loop`
+		v := a3[i]
+		b3 = append(b3, v)
+	}
+
+	var a4 []int
+	for i := range fn6() {
+		a4 = append(a4, fn6()[i])
+	}
+
 	var m map[string]int
 	var c []int
 	for _, v := range m {
@@ -76,3 +92,20 @@ func fn5() {
 	}
 	_ = out
 }
+
+func fn6() []int {
+	return []int{1, 2, 3}
+}
+
+func fn7() {
+	var x []int
+	for _, v := range fn6() { // want `should replace loop`
+		// Purity doesn't matter here
+		x = append(x, v)
+	}
+
+	for i := range fn6() {
+		// Purity does matter here
+		x = append(x, fn6()[i])
+	}
+}

simple/testdata/src/loop-append/loop-append.go.golden

@@ -15,6 +15,17 @@ func fn1() {
 	var a, b []int
 	b = append(b, a...)
 
+	var a2, b2 []int
+	b2 = append(b2, a2...)
+
+	var a3, b3 []int
+	b3 = append(b3, a3...)
+
+	var a4 []int
+	for i := range fn6() {
+		a4 = append(a4, fn6()[i])
+	}
+
 	var m map[string]int
 	var c []int
 	for _, v := range m {
@@ -74,3 +85,17 @@ func fn5() {
 	}
 	_ = out
 }
+
+func fn6() []int {
+	return []int{1, 2, 3}
+}
+
+func fn7() {
+	var x []int
+	x = append(x, fn6()...)
+
+	for i := range fn6() {
+		// Purity does matter here
+		x = append(x, fn6()[i])
+	}
+}

website/data/checks.json

@@ -304,8 +304,8 @@
 			"Text": "",
 			"TitleMarkdown": "Use a single `append` to concatenate two slices",
 			"TextMarkdown": "",
-			"Before": "for _, e := range y {\n    x = append(x, e)\n}",
-			"After": "x = append(x, y...)",
+			"Before": "for _, e := range y {\n    x = append(x, e)\n}\n\nfor i := range y {\n    x = append(x, y[i])\n}\n\nfor i := range y {\n    v := y[i]\n    x = append(x, v)\n}",
+			"After": "x = append(x, y...)\nx = append(x, y...)\nx = append(x, y...)",
 			"Since": "2017.1",
 			"NonDefault": false,
 			"Options": null,