[tools] go/analysis/passes/modernize: slicesbackward - skip is s[i] is mutated

[Madeline Kalil (Gerrit)]

Madeline Kalil has uploaded the change for review

Commit message

go/analysis/passes/modernize: slicesbackward - skip is s[i] is mutated

In the slicesbackward modernizer, we replace backward loops over
slices with a call to slices.Backward. However, if the index
expression s[i] is address-taken or mutated within the loop,
we should not offer this fix. For example:

for i := len(s) - 1; i > 0; i-- {
  s[i].n = 5
}

for _, v := range slices.Backward(s) {
  v.n = 5
}

v is a local copy, so the original s will not get mutated.
To avoid changing the program behavior, we should
not suggest a modernization.

Also, add a case to isScalarLValue for completeness: an ident
appearing in the value of a range statement is the equivalent
of an assignment (we already covered the key of a range statement).
Add tests in rangeint modernizer for this case.

Fixes golang/go#80410

Change-Id: I873afe8965dee39994547e16f284ca80541334d6

Change diff

diff --git a/go/analysis/passes/modernize/modernize.go b/go/analysis/passes/modernize/modernize.go
index 755c0d6..1e2fb6a 100644
--- a/go/analysis/passes/modernize/modernize.go
+++ b/go/analysis/passes/modernize/modernize.go
@@ -17,8 +17,10 @@
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/edge"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/analysis/analyzerutil"
+	"golang.org/x/tools/internal/astutil"
 	"golang.org/x/tools/internal/refactor"
 	"golang.org/x/tools/internal/typesinternal/typeindex"
 
@@ -187,3 +189,40 @@
 	}
 	return depth >= 4
 }
+
+// isScalarLvalue reports whether the specified identifier is
+// address-taken or appears on the left side of an assignment.
+//
+// This function is valid only for scalars (x = ...),
+// not for aggregates (x.a[i] = ...)
+func isScalarLvalue(info *types.Info, curId inspector.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.
+
+	cur := astutil.UnparenEnclosingCursor(curId)
+
+	switch cur.ParentEdgeKind() {
+	case edge.AssignStmt_Lhs:
+		assign := cur.Parent().Node().(*ast.AssignStmt)
+		if assign.Tok != token.DEFINE {
+			return true // i = j or 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.RangeStmt_Key, edge.RangeStmt_Value:
+		rng := cur.Parent().Node().(*ast.RangeStmt)
+		if rng.Tok == token.ASSIGN {
+			return true // "for k, v = range x" is like an AssignStmt to k, v
+		}
+	case edge.IncDecStmt_X:
+		return true // i++, i--
+	case edge.UnaryExpr_X:
+		if cur.Parent().Node().(*ast.UnaryExpr).Op == token.AND {
+			return true // &i
+		}
+	}
+	return false
+}
diff --git a/go/analysis/passes/modernize/rangeint.go b/go/analysis/passes/modernize/rangeint.go
index f7cb965..d882374 100644
--- a/go/analysis/passes/modernize/rangeint.go
+++ b/go/analysis/passes/modernize/rangeint.go
@@ -13,8 +13,6 @@
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/edge"
-	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/go/types/typeutil"
 	"golang.org/x/tools/internal/analysis/analyzerutil"
 	typeindexanalyzer "golang.org/x/tools/internal/analysis/typeindex"
@@ -345,40 +343,3 @@
 	}
 	return nil, nil
 }
-
-// isScalarLvalue reports whether the specified identifier is
-// address-taken or appears on the left side of an assignment.
-//
-// This function is valid only for scalars (x = ...),
-// not for aggregates (x.a[i] = ...)
-func isScalarLvalue(info *types.Info, curId inspector.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.
-
-	cur := astutil.UnparenEnclosingCursor(curId)
-
-	switch cur.ParentEdgeKind() {
-	case edge.AssignStmt_Lhs:
-		assign := cur.Parent().Node().(*ast.AssignStmt)
-		if assign.Tok != token.DEFINE {
-			return true // i = j or 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.RangeStmt_Key:
-		rng := cur.Parent().Node().(*ast.RangeStmt)
-		if rng.Tok == token.ASSIGN {
-			return true // "for k, v = range x" is like an AssignStmt to k, v
-		}
-	case edge.IncDecStmt_X:
-		return true // i++, i--
-	case edge.UnaryExpr_X:
-		if cur.Parent().Node().(*ast.UnaryExpr).Op == token.AND {
-			return true // &i
-		}
-	}
-	return false
-}
diff --git a/go/analysis/passes/modernize/slicesbackward.go b/go/analysis/passes/modernize/slicesbackward.go
index 02cd30a..ee5be5c 100644
--- a/go/analysis/passes/modernize/slicesbackward.go
+++ b/go/analysis/passes/modernize/slicesbackward.go
@@ -14,6 +14,7 @@
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/edge"
+	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/go/types/typeutil"
 	"golang.org/x/tools/internal/analysis/analyzerutil"
 	typeindexanalyzer "golang.org/x/tools/internal/analysis/typeindex"
@@ -161,10 +162,10 @@
 				// mutating the slice or taking an element's address, a fix will not
 				// be offered.
 				if curUse.ParentEdgeKind() == edge.IndexExpr_Index {
-					if isScalarLvalue(pass.TypesInfo, curUse.Parent()) {
+					idxCur := curUse.Parent()
+					if isScalarLvalue(info, idxCur) || isMutating(info, idxCur) {
 						continue nextLoop
 					}
-					idxCur := curUse.Parent()
 					idxExpr := idxCur.Node().(*ast.IndexExpr)
 					if astutil.EqualSyntax(idxExpr.X, sliceExpr) {
 						sliceIdxs++
@@ -268,3 +269,56 @@
 	}
 	return "v"
 }
+
+// isMutating reports whether the index expression idxCur (s[i]) is mutated or
+// address-taken within its enclosing statement.
+// Modernization to "for _, v := range slices.Backward(s)" is unsafe if s[i] or
+// its fields are mutated or address-taken (since v would be a local copy of the
+// element so s[i] wouldn't get mutated).
+// We don't need to worry about indirect selections (e.g. s[i].n++ where s is
+// []*item) or indirect references like indexing a slice of slices.
+func isMutating(info *types.Info, idxCur inspector.Cursor) bool {
+	for cur := range idxCur.Enclosing() {
+		switch cur.ParentEdgeKind() {
+		case edge.SelectorExpr_X:
+			selExpr := cur.Parent().Node().(*ast.SelectorExpr)
+			if sel, ok := info.Selections[selExpr]; ok {
+				if sel.Indirect() {
+					return false
+				}
+				if sel.Kind() == types.MethodVal {
+					// s[i].Method()
+					if sig, ok := sel.Obj().Type().(*types.Signature); ok && sig.Recv() != nil {
+						if _, isPtr := sig.Recv().Type().Underlying().(*types.Pointer); isPtr {
+							// Calling a method on a pointer receiver may result in mutation.
+							return true
+						}
+					}
+				}
+				// Else, look at the next enclosing node in case we have a chain of selections.
+			}
+		case edge.IndexExpr_X, edge.SliceExpr_X:
+			// s[i][0] or s[i][0:2]
+			if t := info.TypeOf(cur.Node().(ast.Expr)); t != nil {
+				switch t.Underlying().(type) {
+				// Indexing/slicing a slice of pointers, slice of slices, or slices of
+				// maps is an indirect reference (slice of arrays, slice of structs,
+				// etc. is a direct reference).
+				case *types.Pointer, *types.Slice, *types.Map:
+					return false
+				}
+			}
+		// s[i] = 2; s[i]++; for s[i], _ = range s; for _, s[i] = range s
+		case edge.AssignStmt_Lhs, edge.IncDecStmt_X, edge.RangeStmt_Key, edge.RangeStmt_Value:
+			return true
+		// &s[i]
+		case edge.UnaryExpr_X:
+			return cur.Parent().Node().(*ast.UnaryExpr).Op == token.AND
+		case edge.ParenExpr_X:
+			// Skip parentheses and evaluate next enclosing node
+		default:
+			return false
+		}
+	}
+	return false
+}
diff --git a/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go b/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go
index d3ea1e9..4f41813 100644
--- a/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go
+++ b/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go
@@ -317,6 +317,14 @@
 	}
 }
 
+func issue77034_value(slice []int) {
+	for i := 0; i < 5; i++ { // nope: inner loop modifies i
+		for _, i = range slice {
+		}
+	}
+}
+
+
 func issue77034_define_inner() {
 	for i := 0; i < 5; i++ { // want "for loop can be modernized using range over int"
 		for i := range 10 { // inner "i" doesn't modify outer "i"
diff --git a/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden b/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
index f85e986..7cc05a9 100644
--- a/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
@@ -316,6 +316,14 @@
 	}
 }
 
+func issue77034_value(slice []int) {
+	for i := 0; i < 5; i++ { // nope: inner loop modifies i
+		for _, i = range slice {
+		}
+	}
+}
+
+
 func issue77034_define_inner() {
 	for range 5 { // want "for loop can be modernized using range over int"
 		for i := range 10 { // inner "i" doesn't modify outer "i"
diff --git a/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go b/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go
index fe3fc25..a56dcc7 100644
--- a/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go
+++ b/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go
@@ -4,7 +4,10 @@
 
 package slicesbackward
 
-import "slices"
+import (
+	"slices"
+	"sync"
+)
 
 var _ = slices.Backward[[]int] // force import of "slices" to avoid duplicate import edits
 
@@ -157,3 +160,108 @@
 		_ = &s[i]
 	}
 }
+
+type item struct {
+	n int
+}
+
+// Should NOT fire: field mutation
+func indexExprMutated(s []item) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].n++
+	}
+}
+
+type outer struct {
+	inner item
+}
+
+// Should NOT fire: nested field mutation
+func indexExprNestedField(s []outer) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].inner.n++
+	}
+}
+
+// Should NOT fire: field assignment
+func indexExprFieldAssign(s []item) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].n = 5
+	}
+}
+
+// Should NOT fire: multi-field assignment
+func indexExprMultiAssign(s []int) {
+	for i := len(s) - 2; i >= 0; i-- {
+		s[i], s[i+1] = 1, 2
+	}
+}
+
+// Should NOT fire: parenthesized assignment
+func indexExprParenthesizedAssign(s []int) {
+	for i := len(s) - 1; i >= 0; i-- {
+		(s[i]) = 5
+	}
+}
+
+// Should NOT fire: address-taken
+func indexExprFieldAddr(s []item) {
+	for i := len(s) - 1; i >= 0; i-- {
+		_ = &s[i].n
+	}
+}
+
+// Should NOT fire: method call with pointer receiver may mutate element
+func indexExprMethodCall(s []sync.Mutex) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].Lock()
+		s[i].Unlock()
+	}
+}
+
+type stringer struct{}
+
+func (stringer) String() string { return "" }
+
+// SHOULD fire: method call with value receiver does not mutate element
+func indexExprValueReceiver(s []stringer) {
+	for i := len(s) - 1; i >= 0; i-- { // want "backward loop over slice can be modernized using slices.Backward"
+		_ = s[i].String()
+	}
+}
+
+// Should NOT fire: range assignment mutating slice elements
+func indexExprRangeAssign(s []int, x []int) {
+	for i := len(s) - 1; i >= 0; i-- {
+		for s[i] = range x {
+		}
+	}
+}
+
+// SHOULD fire: slice of slices is indirect reference
+func indexExprSliceOfSlices(s [][]int) {
+	for i := len(s) - 1; i >= 0; i-- { // want "backward loop over slice can be modernized using slices.Backward"
+		s[i][0]++
+	}
+}
+
+// SHOULD fire: slice of pointers is indirect reference
+func indexExprPointerSlice(s []*item) {
+	for i := len(s) - 1; i >= 0; i-- { // want "backward loop over slice can be modernized using slices.Backward"
+		s[i].n++
+	}
+}
+
+// Should NOT fire: slice of arrays, mutating array element is direct mutation
+func indexExprSliceOfArrays(s [][3]int) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i][0]++
+	}
+}
+
+// SHOULD fire: index expression used as index of another slice which is mutated
+func indexExprUsedAsIndex(s []int) {
+	for i := len(s) - 1; i >= 0; i-- { // want "backward loop over slice can be modernized using slices.Backward"
+		s[s[i]] = 5
+	}
+}
diff --git a/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go.golden b/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go.golden
index e3fb892..dffe408 100644
--- a/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go.golden
@@ -4,7 +4,10 @@
 
 package slicesbackward
 
-import "slices"
+import (
+	"slices"
+	"sync"
+)
 
 var _ = slices.Backward[[]int] // force import of "slices" to avoid duplicate import edits
 
@@ -157,3 +160,109 @@
 		_ = &s[i]
 	}
 }
+
+type item struct {
+	n int
+}
+
+// Should NOT fire: field mutation
+func indexExprMutated(s []item) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].n++
+	}
+}
+
+type outer struct {
+	inner item
+}
+
+// Should NOT fire: nested field mutation
+func indexExprNestedField(s []outer) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].inner.n++
+	}
+}
+
+// Should NOT fire: field assignment
+func indexExprFieldAssign(s []item) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].n = 5
+	}
+}
+
+// Should NOT fire: multi-field assignment
+func indexExprMultiAssign(s []int) {
+	for i := len(s) - 2; i >= 0; i-- {
+		s[i], s[i+1] = 1, 2
+	}
+}
+
+// Should NOT fire: parenthesized assignment
+func indexExprParenthesizedAssign(s []int) {
+	for i := len(s) - 1; i >= 0; i-- {
+		(s[i]) = 5
+	}
+}
+
+// Should NOT fire: address-taken
+func indexExprFieldAddr(s []item) {
+	for i := len(s) - 1; i >= 0; i-- {
+		_ = &s[i].n
+	}
+}
+
+// Should NOT fire: method call with pointer receiver may mutate element
+func indexExprMethodCall(s []sync.Mutex) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i].Lock()
+		s[i].Unlock()
+	}
+}
+
+type stringer struct{}
+
+func (stringer) String() string { return "" }
+
+// SHOULD fire: method call with value receiver does not mutate element
+func indexExprValueReceiver(s []stringer) {
+	for _, v := range slices.Backward(s) { // want "backward loop over slice can be modernized using slices.Backward"
+		_ = v.String()
+	}
+}
+
+// Should NOT fire: range assignment mutating slice elements
+func indexExprRangeAssign(s []int, x []int) {
+	for i := len(s) - 1; i >= 0; i-- {
+		for s[i] = range x {
+		}
+	}
+}
+
+// SHOULD fire: slice of slices is indirect reference
+func indexExprSliceOfSlices(s [][]int) {
+	for _, v := range slices.Backward(s) { // want "backward loop over slice can be modernized using slices.Backward"
+		v[0]++
+	}
+}
+
+// SHOULD fire: slice of pointers is indirect reference
+func indexExprPointerSlice(s []*item) {
+	for _, v := range slices.Backward(s) { // want "backward loop over slice can be modernized using slices.Backward"
+		v.n++
+	}
+}
+
+// Should NOT fire: slice of arrays, mutating array element is direct mutation
+func indexExprSliceOfArrays(s [][3]int) {
+	for i := len(s) - 1; i >= 0; i-- {
+		s[i][0]++
+	}
+}
+
+// SHOULD fire: index expression used as index of another slice which is mutated
+func indexExprUsedAsIndex(s []int) {
+	for _, v := range slices.Backward(s) { // want "backward loop over slice can be modernized using slices.Backward"
+		s[v] = 5
+	}
+}
+

Change information

Files:

• M go/analysis/passes/modernize/modernize.go
• M go/analysis/passes/modernize/rangeint.go
• M go/analysis/passes/modernize/slicesbackward.go
• M go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go
• M go/analysis/passes/modernize/testdata/src/rangeint/rangeint.go.golden
• M go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go
• M go/analysis/passes/modernize/testdata/src/slicesbackward/slicesbackward.go.golden

Change size: L

Delta: 7 files changed, 330 insertions(+), 43 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I873afe8965dee39994547e16f284ca80541334d6

Gerrit-Change-Number: 802260

Gerrit-PatchSet: 1

Gerrit-Owner: Madeline Kalil <mka...@google.com>

[Madeline Kalil (Gerrit)]

Attention needed from Alan Donovan

Madeline Kalil voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

• Alan Donovan

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I873afe8965dee39994547e16f284ca80541334d6

Gerrit-Change-Number: 802260

Gerrit-PatchSet: 1

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Attention: Alan Donovan <adon...@google.com>

Gerrit-Comment-Date: Fri, 17 Jul 2026 19:03:35 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Madeline Kalil (Gerrit)]

Attention needed from Alan Donovan

Madeline Kalil uploaded new patchset

Madeline Kalil uploaded patch set #2 to this change.

Open in Gerrit

Related details

Attention is currently required from:

• Alan Donovan

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I873afe8965dee39994547e16f284ca80541334d6

Gerrit-Change-Number: 802260

Gerrit-PatchSet: 2

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-CC: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Alan Donovan <adon...@google.com>

[Alan Donovan (Gerrit)]

Attention needed from Madeline Kalil

Alan Donovan added 6 comments

File go/analysis/passes/modernize/modernize.go

Line 215, Patchset 2 (Latest): case edge.RangeStmt_Key, edge.RangeStmt_Value:

Alan Donovan . resolved

For viewers at home, the only delta here is this addition.

File go/analysis/passes/modernize/rangeint.go

Line 352, Patchset 2 (Parent):// This function is valid only for scalars (x = ...),


// not for aggregates (x.a[i] = ...)

Alan Donovan . unresolved

This comment describes the fundamental problem: rangeint cares only about mutations of integer variables, but slicesbackward wants to observe more general mutations of aggregates (structs and arrays), as your isMutation function does. In effect, this comment is a TODO that needs to be done before the function is fit to be renamed just `isLvalue` and used more widely, such as by slicesbackward, which of course is using it already. :)

So I think you can just fold the ideas of isMutation into this function. In place of L359, which ascends upwards from x to (x), you can broaden it to also ascend to x.f or x[i] or x[i:j] if the operations don't traverse a pointer. Then the base case is exactly the logic that's already below.

For example:

```
// isLvalue reports whether the specified expression appears on the
// left side of an assignment or in a context where its address is taken.
func isLvalue(info *types.Info, cur inspector.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.

	for cur := range cur.Enclosing() {
		// Ascend to outermost aggregate of which
		// original cur is a part:
		//    x -> (x) | x.f | x[i] | x[i:j]

		switch cur.ParentEdgeKind() {
		case edge.ParenExpr_X:
			// If x is an lvalue, then (x) is an lvalue.

		case edge.SelectorExpr_X:
			// If x is an lvalue, then x.f is an lvalue iff
			// the selection does not traverse a pointer.
			sel := cur.Parent().Node().(*ast.SelectorExpr)
			seln := info.Selections[sel]
			if seln.Indirect() {
				return false
			}
			if seln.Kind() == types.MethodVal {
				sig := seln.Obj().Type().(*types.Signature)
				if !is[*types.Pointer](TypeOf(cur.Node().(ast.Expr))) && 
				    is[*types.Pointer](sig.Recv().Type().Underlying()) {
					// Calling (*T).method on an lvalue of type T takes its address.
					return true
				}
			}

		case edge.IndexExpr_X, edge.SliceExpr_X:
			// If x[i] or x[i:j] is an lvalue,
			// then x is an lvalue iff x is an array.
			if !is[*types.Array](info.TypeOf(cur.Node().(ast.Expr)).Underlying()) {
				return false
			}

		default:
			// base case
			// ... all the old logic of isScalarValue ...
		}
	}
}
```

File go/analysis/passes/modernize/slicesbackward.go

Line 165, Patchset 2 (Latest): idxCur := curUse.Parent()

Alan Donovan . unresolved

curIdx ("cur" first seems to be our convention)

Line 281, Patchset 2 (Latest): for cur := range idxCur.Enclosing() {

Alan Donovan . unresolved

The way to think about this loop is not in terms of s[i], but as asking how far can we walk up the expression tree while still referring to (part of) the same variable? Three cases (Paren, Selector, Index) are inductive cases that walk up the tree. The remaining cases (Assign, etc) are the base case: they are outside the loop, and they correspond exactly to what isScalarLvalue does.

Line 293, Patchset 2 (Latest): // Calling a method on a pointer receiver may result in mutation.

Alan Donovan . unresolved

It's ok to call a method `(*T).method` if you already have a value of type `*T`, as that just makes a copy of the pointer; the interesting case is when you call it on a variable of type T, because that implicitly takes the address. So I think you need to check that TypeOf(x) is not also a pointer.

Line 313, Patchset 2 (Latest): return true

Alan Donovan . unresolved

(Don't forget all the other checks from isScalarLvalue.)

Open in Gerrit

Related details

Attention is currently required from:

• Madeline Kalil

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I873afe8965dee39994547e16f284ca80541334d6

Gerrit-Change-Number: 802260

Gerrit-PatchSet: 2

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Fri, 17 Jul 2026 21:53:04 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Alan Donovan (Gerrit)]

Attention needed from Madeline Kalil

Alan Donovan added 1 comment

File go/analysis/passes/modernize/slicesbackward.go

Line 286, Patchset 2 (Latest): if sel.Indirect() {

Alan Donovan . unresolved

Beware that this has an annoyingly subtle bug that we have decided not to fix (#8353).
See indirectSelection in internal/refactor/inline/util.go for a workaround.

It would be nice to elevate the final isLvalue function to the typesinternal package and use it in gopls/internal/golang/inline.go instead of isLvalueUse. It's a sufficiently subtle function that a test suite for it there would be nice too.

Open in Gerrit

Related details

Attention is currently required from:

• Madeline Kalil

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I873afe8965dee39994547e16f284ca80541334d6

Gerrit-Change-Number: 802260

Gerrit-PatchSet: 2

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Fri, 17 Jul 2026 21:59:31 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No