go/analysis/passes/modernize: stringsbuilder: allow multiple rvalue uses

The stringsbuilder analyzer previously rejected any candidate variable
that had more than one rvalue use of the finished string. Multiple
rvalue uses are valid and each gets .String() appended.

Fixes golang/go#77659

Change-Id: Ice14b56d639809c789ffee10744244d4eb27fd2f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/746540
Commit-Queue: Alan Donovan <adonovan@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Madeline Kalil <mkalil@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: noamcohen97

go/analysis/passes/modernize/doc.go

@@ -460,14 +460,15 @@ is replaced by:
 
 This avoids quadratic memory allocation and improves performance.
 
-The analyzer requires that all references to s except the final one
+The analyzer requires that all references to s before the final uses
 are += operations. To avoid warning about trivial cases, at least one
 must appear within a loop. The variable s must be a local
 variable, not a global or parameter.
 
-The sole use of the finished string must be the last reference to the
-variable s. (It may appear within an intervening loop or function literal,
-since even s.String() is called repeatedly, it does not allocate memory.)
+All uses of the finished string must come after the last += operation.
+Each such use will be replaced by a call to strings.Builder's String method.
+(These may appear within an intervening loop or function literal, since even
+if s.String() is called repeatedly, it does not allocate memory.)
 
 Often the addend is a call to fmt.Sprintf, as in this example:
 

go/analysis/passes/modernize/stringsbuilder.go

@@ -254,8 +254,8 @@ nextcand:
 		//    var s string
 		//    for ... { s += expr }
 		//
-		// - The final use of s must be as an rvalue (e.g. use(s), not &s).
-		//   This will become s.String().
+		// - All uses of s after the last += must be rvalue uses (e.g. use(s), not &s).
+		//   Each of these will become s.String().
 		//
 		//   Perhaps surprisingly, it is fine for there to be an
 		//   intervening loop or lambda w.r.t. the declaration of s:
@@ -270,7 +270,7 @@ nextcand:
 		var (
 			numLoopAssigns int             // number of += assignments within a loop
 			loopAssign     *ast.AssignStmt // first += assignment within a loop
-			seenRvalueUse  bool            // => we've seen the sole final use of s as an rvalue
+			seenRvalueUse  bool            // => we've seen at least one rvalue use of s
 		)
 		for curUse := range index.Uses(v) {
 			// Strip enclosing parens around Ident.
@@ -280,11 +280,6 @@ nextcand:
 				ek = curUse.ParentEdgeKind()
 			}
 
-			// The rvalueUse must be the lexically last use.
-			if seenRvalueUse {
-				continue nextcand
-			}
-
 			// intervening reports whether cur has an ancestor of
 			// one of the given types that is within the scope of v.
 			intervening := func(types ...ast.Node) bool {
@@ -297,6 +292,11 @@ nextcand:
 			}
 
 			if ek == edge.AssignStmt_Lhs {
+				// After an rvalue use, no more assignments are allowed.
+				if seenRvalueUse {
+					continue nextcand
+				}
+
 				assign := curUse.Parent().Node().(*ast.AssignStmt)
 				if assign.Tok != token.ADD_ASSIGN {
 					continue nextcand

go/analysis/passes/modernize/testdata/src/stringsbuilder/stringsbuilder.go

@@ -153,3 +153,26 @@ func nopeStringIsNotLastValueSpecInVarDecl() {
 	}
 	println(str, after)
 }
+
+// Regression test for go.dev/issue/77659.
+// Multiple rvalue uses of the accumulated string should all be converted.
+func _() {
+	acc := "s1"
+	for _, s := range []string{"foo", "bar"} {
+		acc += s // want "using string \\+= string in a loop is inefficient"
+	}
+	_ = acc
+	_ = acc + "footer2"
+}
+
+// Regression test for go.dev/issue/77659 (negative case).
+// += after an rvalue use should still be rejected.
+func _() {
+	var s string
+	for _, x := range []string{"a", "b"} {
+		s += x
+	}
+	print(s)
+	s += "extra" // nope: += after rvalue use
+	print(s)
+}

go/analysis/passes/modernize/testdata/src/stringsbuilder/stringsbuilder.go.golden

@@ -158,3 +158,27 @@ func nopeStringIsNotLastValueSpecInVarDecl() {
 	}
 	println(str, after)
 }
+
+// Regression test for go.dev/issue/77659.
+// Multiple rvalue uses of the accumulated string should all be converted.
+func _() {
+	var acc strings.Builder
+	acc.WriteString("s1")
+	for _, s := range []string{"foo", "bar"} {
+		acc.WriteString(s) // want "using string \\+= string in a loop is inefficient"
+	}
+	_ = acc.String()
+	_ = acc.String() + "footer2"
+}
+
+// Regression test for go.dev/issue/77659 (negative case).
+// += after an rvalue use should still be rejected.
+func _() {
+	var s string
+	for _, x := range []string{"a", "b"} {
+		s += x
+	}
+	print(s)
+	s += "extra" // nope: += after rvalue use
+	print(s)
+}

gopls/doc/analyzers.md

@@ -4037,9 +4037,9 @@ is replaced by:
 
 This avoids quadratic memory allocation and improves performance.
 
-The analyzer requires that all references to s except the final one are += operations. To avoid warning about trivial cases, at least one must appear within a loop. The variable s must be a local variable, not a global or parameter.
+The analyzer requires that all references to s before the final uses are += operations. To avoid warning about trivial cases, at least one must appear within a loop. The variable s must be a local variable, not a global or parameter.
 
-The sole use of the finished string must be the last reference to the variable s. (It may appear within an intervening loop or function literal, since even s.String() is called repeatedly, it does not allocate memory.)
+All uses of the finished string must come after the last += operation. Each such use will be replaced by a call to strings.Builder's String method. (These may appear within an intervening loop or function literal, since even if s.String() is called repeatedly, it does not allocate memory.)
 
 Often the addend is a call to fmt.Sprintf, as in this example:
 

gopls/internal/doc/api.json

@@ -1702,7 +1702,7 @@
 						},
 						{
 							"Name": "\"stringsbuilder\"",
-							"Doc": "replace += with strings.Builder\n\nThis analyzer replaces repeated string += string concatenation\noperations with calls to Go 1.10's strings.Builder.\n\nFor example:\n\n\tvar s = \"[\"\n\tfor x := range seq {\n\t\ts += x\n\t\ts += \".\"\n\t}\n\ts += \"]\"\n\tuse(s)\n\nis replaced by:\n\n\tvar s strings.Builder\n\ts.WriteString(\"[\")\n\tfor x := range seq {\n\t\ts.WriteString(x)\n\t\ts.WriteString(\".\")\n\t}\n\ts.WriteString(\"]\")\n\tuse(s.String())\n\nThis avoids quadratic memory allocation and improves performance.\n\nThe analyzer requires that all references to s except the final one\nare += operations. To avoid warning about trivial cases, at least one\nmust appear within a loop. The variable s must be a local\nvariable, not a global or parameter.\n\nThe sole use of the finished string must be the last reference to the\nvariable s. (It may appear within an intervening loop or function literal,\nsince even s.String() is called repeatedly, it does not allocate memory.)\n\nOften the addend is a call to fmt.Sprintf, as in this example:\n\n\tvar s string\n\tfor x := range seq {\n\t\ts += fmt.Sprintf(\"%v\", x)\n\t}\n\nwhich, once the suggested fix is applied, becomes:\n\n\tvar s strings.Builder\n\tfor x := range seq {\n\t\ts.WriteString(fmt.Sprintf(\"%v\", x))\n\t}\n\nThe WriteString call can be further simplified to the more efficient\nfmt.Fprintf(\u0026s, \"%v\", x), avoiding the allocation of an intermediary.\nHowever, stringsbuilder does not perform this simplification;\nit requires staticcheck analyzer QF1012. (See https://go.dev/issue/76918.)",
+							"Doc": "replace += with strings.Builder\n\nThis analyzer replaces repeated string += string concatenation\noperations with calls to Go 1.10's strings.Builder.\n\nFor example:\n\n\tvar s = \"[\"\n\tfor x := range seq {\n\t\ts += x\n\t\ts += \".\"\n\t}\n\ts += \"]\"\n\tuse(s)\n\nis replaced by:\n\n\tvar s strings.Builder\n\ts.WriteString(\"[\")\n\tfor x := range seq {\n\t\ts.WriteString(x)\n\t\ts.WriteString(\".\")\n\t}\n\ts.WriteString(\"]\")\n\tuse(s.String())\n\nThis avoids quadratic memory allocation and improves performance.\n\nThe analyzer requires that all references to s before the final uses\nare += operations. To avoid warning about trivial cases, at least one\nmust appear within a loop. The variable s must be a local\nvariable, not a global or parameter.\n\nAll uses of the finished string must come after the last += operation.\nEach such use will be replaced by a call to strings.Builder's String method.\n(These may appear within an intervening loop or function literal, since even\nif s.String() is called repeatedly, it does not allocate memory.)\n\nOften the addend is a call to fmt.Sprintf, as in this example:\n\n\tvar s string\n\tfor x := range seq {\n\t\ts += fmt.Sprintf(\"%v\", x)\n\t}\n\nwhich, once the suggested fix is applied, becomes:\n\n\tvar s strings.Builder\n\tfor x := range seq {\n\t\ts.WriteString(fmt.Sprintf(\"%v\", x))\n\t}\n\nThe WriteString call can be further simplified to the more efficient\nfmt.Fprintf(\u0026s, \"%v\", x), avoiding the allocation of an intermediary.\nHowever, stringsbuilder does not perform this simplification;\nit requires staticcheck analyzer QF1012. (See https://go.dev/issue/76918.)",
 							"Default": "true",
 							"Status": ""
 						},
@@ -3625,7 +3625,7 @@
 		},
 		{
 			"Name": "stringsbuilder",
-			"Doc": "replace += with strings.Builder\n\nThis analyzer replaces repeated string += string concatenation\noperations with calls to Go 1.10's strings.Builder.\n\nFor example:\n\n\tvar s = \"[\"\n\tfor x := range seq {\n\t\ts += x\n\t\ts += \".\"\n\t}\n\ts += \"]\"\n\tuse(s)\n\nis replaced by:\n\n\tvar s strings.Builder\n\ts.WriteString(\"[\")\n\tfor x := range seq {\n\t\ts.WriteString(x)\n\t\ts.WriteString(\".\")\n\t}\n\ts.WriteString(\"]\")\n\tuse(s.String())\n\nThis avoids quadratic memory allocation and improves performance.\n\nThe analyzer requires that all references to s except the final one\nare += operations. To avoid warning about trivial cases, at least one\nmust appear within a loop. The variable s must be a local\nvariable, not a global or parameter.\n\nThe sole use of the finished string must be the last reference to the\nvariable s. (It may appear within an intervening loop or function literal,\nsince even s.String() is called repeatedly, it does not allocate memory.)\n\nOften the addend is a call to fmt.Sprintf, as in this example:\n\n\tvar s string\n\tfor x := range seq {\n\t\ts += fmt.Sprintf(\"%v\", x)\n\t}\n\nwhich, once the suggested fix is applied, becomes:\n\n\tvar s strings.Builder\n\tfor x := range seq {\n\t\ts.WriteString(fmt.Sprintf(\"%v\", x))\n\t}\n\nThe WriteString call can be further simplified to the more efficient\nfmt.Fprintf(\u0026s, \"%v\", x), avoiding the allocation of an intermediary.\nHowever, stringsbuilder does not perform this simplification;\nit requires staticcheck analyzer QF1012. (See https://go.dev/issue/76918.)",
+			"Doc": "replace += with strings.Builder\n\nThis analyzer replaces repeated string += string concatenation\noperations with calls to Go 1.10's strings.Builder.\n\nFor example:\n\n\tvar s = \"[\"\n\tfor x := range seq {\n\t\ts += x\n\t\ts += \".\"\n\t}\n\ts += \"]\"\n\tuse(s)\n\nis replaced by:\n\n\tvar s strings.Builder\n\ts.WriteString(\"[\")\n\tfor x := range seq {\n\t\ts.WriteString(x)\n\t\ts.WriteString(\".\")\n\t}\n\ts.WriteString(\"]\")\n\tuse(s.String())\n\nThis avoids quadratic memory allocation and improves performance.\n\nThe analyzer requires that all references to s before the final uses\nare += operations. To avoid warning about trivial cases, at least one\nmust appear within a loop. The variable s must be a local\nvariable, not a global or parameter.\n\nAll uses of the finished string must come after the last += operation.\nEach such use will be replaced by a call to strings.Builder's String method.\n(These may appear within an intervening loop or function literal, since even\nif s.String() is called repeatedly, it does not allocate memory.)\n\nOften the addend is a call to fmt.Sprintf, as in this example:\n\n\tvar s string\n\tfor x := range seq {\n\t\ts += fmt.Sprintf(\"%v\", x)\n\t}\n\nwhich, once the suggested fix is applied, becomes:\n\n\tvar s strings.Builder\n\tfor x := range seq {\n\t\ts.WriteString(fmt.Sprintf(\"%v\", x))\n\t}\n\nThe WriteString call can be further simplified to the more efficient\nfmt.Fprintf(\u0026s, \"%v\", x), avoiding the allocation of an intermediary.\nHowever, stringsbuilder does not perform this simplification;\nit requires staticcheck analyzer QF1012. (See https://go.dev/issue/76918.)",
 			"URL": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/modernize#stringbuilder",
 			"Default": true
 		},