gopls/internal/analysis/modernize: rangeint: respect side effects

This CL fixes a serious bug in rangeint that caused it to offer
a fix from "for i := 0; i < len(s); i++" to "for range s" even
when the loop may modify the value of slice s. (The for loop
reads the length on each iteration, whereas the range loop reads
it only once.)

+ test

Fixes golang/go#72917

Change-Id: Id0c926f0590241736c7fe7589c2796a075d05744
Reviewed-on: https://go-review.googlesource.com/c/tools/+/660435
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>

Author: adonovan

gopls/internal/analysis/modernize/rangeint.go

@@ -15,8 +15,11 @@ import (
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/go/types/typeutil"
 	"golang.org/x/tools/internal/analysisinternal"
+	typeindexanalyzer "golang.org/x/tools/internal/analysisinternal/typeindex"
 	"golang.org/x/tools/internal/astutil/cursor"
 	"golang.org/x/tools/internal/astutil/edge"
+	"golang.org/x/tools/internal/typesinternal"
+	"golang.org/x/tools/internal/typesinternal/typeindex"
 )
 
 // rangeint offers a fix to replace a 3-clause 'for' loop:
@@ -38,13 +41,23 @@ import (
 //   - The limit must not be b.N, to avoid redundancy with bloop's fixes.
 //
 // Caveats:
-//   - The fix will cause the limit expression to be evaluated exactly
-//     once, instead of once per iteration. The limit may be a function call
-//     (e.g. seq.Len()). The fix may change the cardinality of side effects.
+//
+// The fix causes the limit expression to be evaluated exactly once,
+// instead of once per iteration. So, to avoid changing the
+// cardinality of side effects, the limit expression must not involve
+// function calls (e.g. seq.Len()) or channel receives. Moreover, the
+// value of the limit expression must be loop invariant, which in
+// practice means it must take one of the following forms:
+//
+//   - a local variable that is assigned only once and not address-taken;
+//   - a constant; or
+//   - len(s), where s has the above properties.
 func rangeint(pass *analysis.Pass) {
 	info := pass.TypesInfo
 
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	typeindex := pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index)
+
 	for curFile := range filesUsing(inspect, info, "go1.22") {
 	nextLoop:
 		for curLoop := range curFile.Preorder((*ast.ForStmt)(nil)) {
@@ -62,19 +75,39 @@ func rangeint(pass *analysis.Pass) {
 					// Have: for i = 0; i < limit; ... {}
 
 					limit := compare.Y
-					curLimit, _ := curLoop.FindNode(limit)
-					// Don't offer a fix if the limit expression depends on the loop index.
-					for cur := range curLimit.Preorder((*ast.Ident)(nil)) {
-						if cur.Node().(*ast.Ident).Name == index.Name {
-							continue nextLoop
-						}
+
+					// If limit is "len(slice)", simplify it to "slice".
+					//
+					// (Don't replace "for i := 0; i < len(map); i++"
+					// with "for range m" because it's too hard to prove
+					// that len(m) is loop-invariant).
+					if call, ok := limit.(*ast.CallExpr); ok &&
+						typeutil.Callee(info, call) == builtinLen &&
+						is[*types.Slice](info.TypeOf(call.Args[0]).Underlying()) {
+						limit = call.Args[0]
 					}
 
-					// Skip loops up to b.N in benchmarks; see [bloop].
-					if sel, ok := limit.(*ast.SelectorExpr); ok &&
-						sel.Sel.Name == "N" &&
-						analysisinternal.IsPointerToNamed(info.TypeOf(sel.X), "testing", "B") {
-						continue // skip b.N
+					// Check the form of limit: must be a constant,
+					// or a local var that is not assigned or address-taken.
+					limitOK := false
+					if info.Types[limit].Value != nil {
+						limitOK = true // constant
+					} else if id, ok := limit.(*ast.Ident); ok {
+						if v, ok := info.Uses[id].(*types.Var); ok &&
+							!(v.Exported() && typesinternal.IsPackageLevel(v)) {
+							// limit is a local or unexported global var.
+							// (An exported global may have uses we can't see.)
+							for cur := range typeindex.Uses(v) {
+								if isScalarLvalue(info, cur) {
+									// Limit var is assigned or address-taken.
+									continue nextLoop
+								}
+							}
+							limitOK = true
+						}
+					}
+					if !limitOK {
+						continue nextLoop
 					}
 
 					if inc, ok := loop.Post.(*ast.IncDecStmt); ok &&
@@ -93,7 +126,7 @@ func rangeint(pass *analysis.Pass) {
 								// Reject if any is an l-value (assigned or address-taken):
 								// a "for range int" loop does not respect assignments to
 								// the loop variable.
-								if isScalarLvalue(curId) {
+								if isScalarLvalue(info, curId) {
 									continue nextLoop
 								}
 							}
@@ -213,7 +246,7 @@ func rangeint(pass *analysis.Pass) {
 //
 // This function is valid only for scalars (x = ...),
 // not for aggregates (x.a[i] = ...)
-func isScalarLvalue(curId cursor.Cursor) bool {
+func isScalarLvalue(info *types.Info, curId cursor.Cursor) bool {
 	// Unfortunately we can't simply use info.Types[e].Assignable()
 	// as it is always true for a variable even when that variable is
 	// used only as an r-value. So we must inspect enclosing syntax.
@@ -229,7 +262,14 @@ func isScalarLvalue(curId cursor.Cursor) bool {
 
 	switch ek {
 	case edge.AssignStmt_Lhs:
-		return true // i = j
+		assign := cur.Parent().Node().(*ast.AssignStmt)
+		if assign.Tok == token.ASSIGN {
+			return true // i = j
+		}
+		id := curId.Node().(*ast.Ident)
+		if v, ok := info.Defs[id]; ok && v.Pos() != id.Pos() {
+			return true // reassignment of i (i, j := 1, 2)
+		}
 	case edge.IncDecStmt_X:
 		return true // i++, i--
 	case edge.UnaryExpr_X:

gopls/internal/analysis/modernize/testdata/src/rangeint/rangeint.go

@@ -48,7 +48,7 @@ func _(i int, s struct{ i int }, slice []int) {
 
 	{
 		var i int
-		for i = 0; i < f(); i++ { // want "for loop can be modernized using range over int"
+		for i = 0; i < 10; i++ { // want "for loop can be modernized using range over int"
 		}
 		// NB: no uses of i after loop.
 	}
@@ -90,8 +90,54 @@ func _(i int, s struct{ i int }, slice []int) {
 	for i := 0; i < 10; i++ { // nope: assigns i
 		i = 8
 	}
+
+	// The limit expression must be loop invariant;
+	// see https://github.com/golang/go/issues/72917
+	for i := 0; i < f(); i++ { // nope
+	}
+	{
+		var s struct{ limit int }
+		for i := 0; i < s.limit; i++ { // nope: limit is not a const or local var
+		}
+	}
+	{
+		const k = 10
+		for i := 0; i < k; i++ { // want "for loop can be modernized using range over int"
+		}
+	}
+	{
+		var limit = 10
+		for i := 0; i < limit; i++ { // want "for loop can be modernized using range over int"
+		}
+	}
+	{
+		var limit = 10
+		for i := 0; i < limit; i++ { // nope: limit is address-taken
+		}
+		print(&limit)
+	}
+	{
+		limit := 10
+		limit++
+		for i := 0; i < limit; i++ { // nope: limit is assigned other than by its declaration
+		}
+	}
+	for i := 0; i < Global; i++ { // nope: limit is an exported global var; may be updated elsewhere
+	}
+	for i := 0; i < len(table); i++ { // want "for loop can be modernized using range over int"
+	}
+	{
+		s := []string{}
+		for i := 0; i < len(s); i++ { // nope: limit is not loop-invariant
+			s = s[1:]
+		}
+	}
 }
 
+var Global int
+
+var table = []string{"hello", "world"}
+
 func f() int { return 0 }
 
 // Repro for part of #71847: ("for range n is invalid if the loop body contains i++"):

gopls/internal/analysis/modernize/testdata/src/rangeint/rangeint.go.golden

@@ -48,7 +48,7 @@ func _(i int, s struct{ i int }, slice []int) {
 
 	{
 		var i int
-		for i = range f() { // want "for loop can be modernized using range over int"
+		for i = range 10 { // want "for loop can be modernized using range over int"
 		}
 		// NB: no uses of i after loop.
 	}
@@ -90,8 +90,54 @@ func _(i int, s struct{ i int }, slice []int) {
 	for i := 0; i < 10; i++  { // nope: assigns i
 		i = 8
 	}
+	
+	// The limit expression must be loop invariant;
+	// see https://github.com/golang/go/issues/72917
+	for i := 0; i < f(); i++ { // nope
+	}
+	{
+		var s struct{ limit int }
+		for i := 0; i < s.limit; i++ { // nope: limit is not a const or local var
+		}
+	}
+	{
+		const k = 10
+		for range k { // want "for loop can be modernized using range over int"
+		}
+	}
+	{
+		var limit = 10
+		for range limit { // want "for loop can be modernized using range over int"
+		}
+	}
+	{
+		var limit = 10
+		for i := 0; i < limit; i++ { // nope: limit is address-taken
+		}
+		print(&limit)
+	}
+	{
+		limit := 10
+		limit++
+		for i := 0; i < limit; i++ { // nope: limit is assigned other than by its declaration
+		}
+	}
+	for i := 0; i < Global; i++ { // nope: limit is an exported global var; may be updated elsewhere
+	}
+	for range table { // want "for loop can be modernized using range over int"
+	}
+	{
+		s := []string{}
+		for i := 0; i < len(s); i++ { // nope: limit is not loop-invariant
+			s = s[1:]
+		}
+	}
 }
 
+var Global int
+
+var table = []string{"hello", "world"}
+
 func f() int { return 0 }
 
 // Repro for part of #71847: ("for range n is invalid if the loop body contains i++"):