[tools] go/analysis/passes/modernize: slicesbackward - skip is s[i] is mutated
Madeline Kalil (Gerrit)
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
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 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
- 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
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 6 comments![]( "Open in Gerrit")
case edge.RangeStmt_Key, edge.RangeStmt_Value:For viewers at home, the only delta here is this addition.
// This function is valid only for scalars (x = ...),
// not for aggregates (x.a[i] = ...)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 ...
}
}
}
```
idxCur := curUse.Parent()curIdx ("cur" first seems to be our convention)
for cur := range idxCur.Enclosing() {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.
// Calling a method on a pointer receiver may result in mutation.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.
return true(Don't forget all the other checks from isScalarLvalue.)
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
if sel.Indirect() {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.
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 6 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
6 comments
// This function is valid only for scalars (x = ...),
// not for aggregates (x.a[i] = ...)Thanks, done. I moved the IsLValue function to typesinternal and added test cases for it that use a comment scheme like in zerovalue_test.go
curIdx ("cur" first seems to be our convention)
Done
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.
Done
if sel.Indirect() {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.
Thanks, I saw that comment but should have been more careful. For this function I think it's okay, and we don't need the workaround? If both the receiver argument and parameter are pointers, the selection is not an l-value use, and we return false when sel.Indirect() is true (it's spuriously true for this case).
// Calling a method on a pointer receiver may result in mutation.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.
Done
(Don't forget all the other checks from isScalarLvalue.)
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 11 comments![]( "Open in Gerrit")
if sel.Indirect() {Madeline KalilBeware 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.
Thanks, I saw that comment but should have been more careful. For this function I think it's okay, and we don't need the workaround? If both the receiver argument and parameter are pointers, the selection is not an l-value use, and we return false when sel.Indirect() is true (it's spuriously true for this case).
Yeah, I think you're exactly right. But it would be good to leave a note that the Indirect bug is benign here.
func IsLValue(info *types.Info, cur inspector.Cursor) bool {Let's rename it yet again (sorry) for maximum clarity. How about:
```
// IsAssignedOrAddressTaken reports whether the expression cur
// denotes a variable and appears in a context that assigns it
// or that takes its address, potentially leading to indirect
// assignment.
//
// These examples cause IsAssignedOrAddressTaken on the identifier
// for x to return true:
//
// x = 1
// x++
// x[i] = 1 (assume x is an array)
// x.a[i] = 1 (assume x.a is a non-pointer struct field)
// use(&x)
//
// whereas these cause it to return false:
//
// y = x
// f(x)
// use(x.a[i])
// use(*x)
//
// The expression may itself be a compound, for example:
//
// use(&(*ptr)) => IsAssignedOrAddressTaken("*ptr") = true
// x.a[i] = 1 => IsAssignedOrAddressTaken("x.a") = true
// _ = x.a[i] => IsAssignedOrAddressTaken("x.a") = false
//
// A variable's declaration is not considered to be an assignment:
//
// var x int => IsAssignedOrAddressTaken(x) = false
// x := 1 => IsAssignedOrAddressTaken(x) = false
//
// TODO(adonovan): revisit the surprising behavior for declarations.
func IsAssignedOrAddressTaken(...)
```
(Now that I think about it, I wonder why isScalarValue went to the trouble of handling the declaration of a variable differently from an assignment to it. Possibly so that scanning for lvalue uses in `var x int; print(x)` would not say "yes, x is used as an lvalue"? But this is not a change, and it's fine for now.)
if assign.Tok != token.DEFINE {
return true // x = j or x += j
}
id := cur.Node().(*ast.Ident)
// Re-assigned identifiers are recorded in the Uses map.
if _, ok := info.Uses[id]; ok {
return true // reassignment of x (x, y := 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
}Now that I think about it, I wonder why isScalarValue went to the trouble of handling the declaration of a variable differently from an assignment to it. (Possibly so that scanning for lvalue uses in `var x int; print(x)` would not say "yes, x is used as an lvalue!"?)
But this is not a change, and it's fine for now.
ptr(&(x) /*false*/)```
ptr(&(x /*true*/)) // x has is address taken
ptr(&(x) /*false*/) // &x is copied during argument passing
```
y /*false*/ := 1// For our purposes, declarations do
// not count as l-value references.
s /*false*/ [0] = 1
s[0] /*true*/ = 1Since these are quite subtle, let's add comments:
```
s /*false*/ [0] = 1 // a load of the pointer in s
s[0] /*true*/ = 1 // a store to the array element s[0]
```
for _, c := range cg.List {There's no need for a slice. Just replace the append with the body of the L162 loop:
```
for _, comment := range file.Comments {
pos := comment.Pos()
want := strings.TrimSpace(comment.Text()) == "true"
...FindByPos etc...
}
```
text := strings.TrimSpace(strings.Trim(c.Text, "/*"))Let CommentGroup.Text() do all the heavy lifting (removing /* etc).
testCases = append(testCases, testCase{comment: c, want: cond(text == "true", true, false)})This is just `text == "true"`. ;-)
cur, ok := inspect.Root().FindByPos(pos-1, pos-1)This is a little fragile: it assumes there's exactly one space.
Rather than make the code more complex, let's just say "no spaces allowed", replace ` /*` with `/*`, and remove -1.
func cond[T any](cond bool, t, f T) T {delete
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 13 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
13 comments
if sel.Indirect() {Madeline KalilBeware 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.
Alan DonovanThanks, I saw that comment but should have been more careful. For this function I think it's okay, and we don't need the workaround? If both the receiver argument and parameter are pointers, the selection is not an l-value use, and we return false when sel.Indirect() is true (it's spuriously true for this case).
Yeah, I think you're exactly right. But it would be good to leave a note that the Indirect bug is benign here.
Agreed, done!
func IsLValue(info *types.Info, cur inspector.Cursor) bool {Thank you!! This is much clearer. I also added test cases to fully reflect all the cases mentioned in the doc comment.
Looking at how isScalarLValue was used previously in rangeint, slicesbackward and stringscut, it makes sense that it would want to report false for the case you mentioned.
Also, isScalarLValue was mishandling assignments:
```
if v, ok := info.Defs[id]; ok && v.Pos() != id.Pos() {
return true // reassignment of i (i, j := 1, 2)
}
```
(i is reassigned and j is a new var)
the Defs maps has the invariant: Defs[id] == nil || Defs[id].Pos() == id.Pos(), so this case is impossible. Reassignments are stored in the Uses map.
if assign.Tok != token.DEFINE {
return true // x = j or x += j
}
id := cur.Node().(*ast.Ident)
// Re-assigned identifiers are recorded in the Uses map.
if _, ok := info.Uses[id]; ok {
return true // reassignment of x (x, y := 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
}Now that I think about it, I wonder why isScalarValue went to the trouble of handling the declaration of a variable differently from an assignment to it. (Possibly so that scanning for lvalue uses in `var x int; print(x)` would not say "yes, x is used as an lvalue!"?)
But this is not a change, and it's fine for now.
Acknowledged
ptr(&(x) /*false*/)```
ptr(&(x /*true*/)) // x has is address taken
ptr(&(x) /*false*/) // &x is copied during argument passing
```
I think adding this comment might mess up my comment testing logic?
```
ptr(&(x /*true*/)) // x has is address taken
ptr(&(x) /*false*/) // &x is copied during argument passing
```
Used slightly different wording but done.
// For our purposes, declarations do
// not count as l-value references.
I don't think we want to use the term l-value anymore, but I added a different comment.
s /*false*/ [0] = 1
s[0] /*true*/ = 1Since these are quite subtle, let's add comments:
```
s /*false*/ [0] = 1 // a load of the pointer in s
s[0] /*true*/ = 1 // a store to the array element s[0]
```
Thanks! Also adding a comment to highlight that this shows the contrast between array and slice memory model.
s /*false*/ [0] = 1
s[0] /*true*/ = 1Since these are quite subtle, let's add comments:
```
s /*false*/ [0] = 1 // a load of the pointer in s
s[0] /*true*/ = 1 // a store to the array element s[0]
```
Done.
There's no need for a slice. Just replace the append with the body of the L162 loop:
```
for _, comment := range file.Comments {
pos := comment.Pos()
want := strings.TrimSpace(comment.Text()) == "true"
...FindByPos etc...
}
```
Done
Let CommentGroup.Text() do all the heavy lifting (removing /* etc).
Done
testCases = append(testCases, testCase{comment: c, want: cond(text == "true", true, false)})This is just `text == "true"`. ;-)
Oooops.
This is a little fragile: it assumes there's exactly one space.
Rather than make the code more complex, let's just say "no spaces allowed", replace ` /*` with `/*`, and remove -1.
Done
func cond[T any](cond bool, t, f T) T {Madeline Kalildelete
Done
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
3 comments
if text != "true" && text != "false" {
continue // skip other comments
}
That's one solution. Another is to use the /*..*/ vs // distinction.
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil submitted the change![]( "Open in Gerrit")
Change information
go/analysis/passes/modernize: slicesbackward - skip if 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.
Promote a new function IsAssignedOrAddressTaken that replaces the uses
of isScalarLValue in modernizers and isLValueUse
in the inliner and add tests for it.
Fixes golang/go#80410
- M go/analysis/passes/modernize/minmax.go
- 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/stringscut.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
- M gopls/internal/golang/inline.go
- A internal/typesinternal/assignedaddress.go
- A internal/typesinternal/assignedaddress_test.go
Code-Review: +2 by Alan Donovan
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |