From 4393fcba0568e9dbfcaf6289a6b3f3f11cac2e63 Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Wed, 19 Mar 2025 19:38:57 +0300 Subject: [PATCH 01/10] gopls/internal/analysis/modernize: fix slicedelete triggers on non idempotent slice identifiers --- .../internal/analysis/modernize/modernize.go | 20 +++++++++++++++++++ .../analysis/modernize/slicesdelete.go | 7 ++++++- .../testdata/src/slicesdelete/slicesdelete.go | 4 ++++ .../src/slicesdelete/slicesdelete.go.golden | 4 ++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 75f5b4014b6..e38b5076c15 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -187,3 +187,23 @@ func enabledCategory(filter, category string) bool { } return exclude } + +// isIdempotentExpr returns true if we can determine that +// the passed expression is idempotent. +func isIdempotentExpr(expr ast.Expr) bool { + isDeterministic := true + ast.Inspect(expr, func(n ast.Node) bool { + switch n.(type) { + case nil, *ast.Ident, *ast.BasicLit, *ast.BinaryExpr, *ast.UnaryExpr, + *ast.ParenExpr, *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, + *ast.TypeAssertExpr, *ast.StarExpr, *ast.CompositeLit, + *ast.ArrayType, *ast.StructType, *ast.MapType, *ast.InterfaceType: + return true + default: + isDeterministic = false + return false + } + }) + return isDeterministic +} + diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index 3c3d880f62b..9a1d35097ee 100644 --- a/gopls/internal/analysis/modernize/slicesdelete.go +++ b/gopls/internal/analysis/modernize/slicesdelete.go @@ -94,7 +94,7 @@ func slicesdelete(pass *analysis.Pass) { slice2, ok2 := call.Args[1].(*ast.SliceExpr) if ok1 && slice1.Low == nil && !slice1.Slice3 && ok2 && slice2.High == nil && !slice2.Slice3 && - equalSyntax(slice1.X, slice2.X) && + equalSliceX(slice1, slice2) && increasingSliceIndices(info, slice1.High, slice2.Low) { // Have append(s[:a], s[b:]...) where we can verify a < b. report(file, call, slice1, slice2) @@ -105,6 +105,11 @@ func slicesdelete(pass *analysis.Pass) { } } +// equalSliceX checks that we're dealing with the same slice +func equalSliceX(slice1, slice2 *ast.SliceExpr) bool { + return equalSyntax(slice1.X, slice2.X) && isIdempotentExpr(slice1.X) +} + // Given two slice indices a and b, returns true if we can verify that a < b. // It recognizes certain forms such as i+k1 < i+k2 where k1 < k2. func increasingSliceIndices(info *types.Info, a, b ast.Expr) bool { diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go index a710d06f2fe..67c28720a42 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go @@ -2,6 +2,8 @@ package slicesdelete var g struct{ f []int } +func h() []int { return []int{} } + func slicesdelete(test, other []byte, i int) { const k = 1 _ = append(test[:i], test[i+1:]...) // want "Replace append with slices.Delete" @@ -26,6 +28,8 @@ func slicesdelete(test, other []byte, i int) { _ = append(g.f[:i], g.f[i+k:]...) // want "Replace append with slices.Delete" + _ = append(h()[:i], h()[i+1:]...) // potentially non idempotent expression + _ = append(test[:3], test[i+1:]...) // cannot verify a < b _ = append(test[:i-4], test[i-1:]...) // want "Replace append with slices.Delete" diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden index 2d9447af3a3..eabddfe0c25 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden @@ -4,6 +4,8 @@ import "slices" var g struct{ f []int } +func h() []int { return []int{} } + func slicesdelete(test, other []byte, i int) { const k = 1 _ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete" @@ -28,6 +30,8 @@ func slicesdelete(test, other []byte, i int) { _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete" + _ = append(h()[:i], h()[i+1:]...) // potentially non idempotent expression + _ = append(test[:3], test[i+1:]...) // cannot verify a < b _ = slices.Delete(test, i-4, i-1) // want "Replace append with slices.Delete" From ba9d32a5eed49ed4832dbc67d60a9ccea2c74526 Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Thu, 20 Mar 2025 15:30:42 +0300 Subject: [PATCH 02/10] fix formatting --- gopls/internal/analysis/modernize/modernize.go | 3 +-- gopls/internal/analysis/modernize/slicesdelete.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index e38b5076c15..e4703729b38 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -193,7 +193,7 @@ func enabledCategory(filter, category string) bool { func isIdempotentExpr(expr ast.Expr) bool { isDeterministic := true ast.Inspect(expr, func(n ast.Node) bool { - switch n.(type) { + switch n.(type) { case nil, *ast.Ident, *ast.BasicLit, *ast.BinaryExpr, *ast.UnaryExpr, *ast.ParenExpr, *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr, *ast.StarExpr, *ast.CompositeLit, @@ -206,4 +206,3 @@ func isIdempotentExpr(expr ast.Expr) bool { }) return isDeterministic } - diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index 9a1d35097ee..328f9418629 100644 --- a/gopls/internal/analysis/modernize/slicesdelete.go +++ b/gopls/internal/analysis/modernize/slicesdelete.go @@ -107,7 +107,7 @@ func slicesdelete(pass *analysis.Pass) { // equalSliceX checks that we're dealing with the same slice func equalSliceX(slice1, slice2 *ast.SliceExpr) bool { - return equalSyntax(slice1.X, slice2.X) && isIdempotentExpr(slice1.X) + return equalSyntax(slice1.X, slice2.X) && isIdempotentExpr(slice1.X) } // Given two slice indices a and b, returns true if we can verify that a < b. From 62347ec52db45f3bcf25787abb3775a9815df264 Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Thu, 20 Mar 2025 16:11:11 +0300 Subject: [PATCH 03/10] inline call exualSliceX --- gopls/internal/analysis/modernize/slicesdelete.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index 328f9418629..d0d39c77d8c 100644 --- a/gopls/internal/analysis/modernize/slicesdelete.go +++ b/gopls/internal/analysis/modernize/slicesdelete.go @@ -94,7 +94,7 @@ func slicesdelete(pass *analysis.Pass) { slice2, ok2 := call.Args[1].(*ast.SliceExpr) if ok1 && slice1.Low == nil && !slice1.Slice3 && ok2 && slice2.High == nil && !slice2.Slice3 && - equalSliceX(slice1, slice2) && + equalSyntax(slice1.X, slice2.X) && isIdempotentExpr(slice1.X) && increasingSliceIndices(info, slice1.High, slice2.Low) { // Have append(s[:a], s[b:]...) where we can verify a < b. report(file, call, slice1, slice2) @@ -105,11 +105,6 @@ func slicesdelete(pass *analysis.Pass) { } } -// equalSliceX checks that we're dealing with the same slice -func equalSliceX(slice1, slice2 *ast.SliceExpr) bool { - return equalSyntax(slice1.X, slice2.X) && isIdempotentExpr(slice1.X) -} - // Given two slice indices a and b, returns true if we can verify that a < b. // It recognizes certain forms such as i+k1 < i+k2 where k1 < k2. func increasingSliceIndices(info *types.Info, a, b ast.Expr) bool { From 66272980b4926971d292f41392831a2b0905350b Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Thu, 20 Mar 2025 17:16:07 +0300 Subject: [PATCH 04/10] change isIdempotentExpr to noEffects and refactor it --- .../internal/analysis/modernize/modernize.go | 29 ++++++++++++------- .../analysis/modernize/slicesdelete.go | 2 +- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index e4703729b38..c87716c8c1f 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -188,21 +188,28 @@ func enabledCategory(filter, category string) bool { return exclude } -// isIdempotentExpr returns true if we can determine that -// the passed expression is idempotent. -func isIdempotentExpr(expr ast.Expr) bool { - isDeterministic := true +// noEffects reports whether the expression has no side effects, i.e., it +// does not modify the memory state. This function is conservative: it may +// return false even when the expression has no effect. +func noEffects(expr ast.Expr) bool { + noEffects := true ast.Inspect(expr, func(n ast.Node) bool { - switch n.(type) { - case nil, *ast.Ident, *ast.BasicLit, *ast.BinaryExpr, *ast.UnaryExpr, - *ast.ParenExpr, *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, - *ast.TypeAssertExpr, *ast.StarExpr, *ast.CompositeLit, - *ast.ArrayType, *ast.StructType, *ast.MapType, *ast.InterfaceType: + switch v := n.(type) { + case nil, *ast.Ident, *ast.BasicLit, *ast.BinaryExpr, *ast.ParenExpr, + *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr, + *ast.StarExpr, *ast.CompositeLit, *ast.ArrayType, *ast.StructType, + *ast.MapType, *ast.InterfaceType, *ast.KeyValueExpr: return true + case *ast.UnaryExpr: + if v.Op == token.ARROW { + noEffects = false + return false + } + return true default: - isDeterministic = false + noEffects = false return false } }) - return isDeterministic + return noEffects } diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index d0d39c77d8c..80d5c57f695 100644 --- a/gopls/internal/analysis/modernize/slicesdelete.go +++ b/gopls/internal/analysis/modernize/slicesdelete.go @@ -94,7 +94,7 @@ func slicesdelete(pass *analysis.Pass) { slice2, ok2 := call.Args[1].(*ast.SliceExpr) if ok1 && slice1.Low == nil && !slice1.Slice3 && ok2 && slice2.High == nil && !slice2.Slice3 && - equalSyntax(slice1.X, slice2.X) && isIdempotentExpr(slice1.X) && + equalSyntax(slice1.X, slice2.X) && noEffects(slice1.X) && increasingSliceIndices(info, slice1.High, slice2.Low) { // Have append(s[:a], s[b:]...) where we can verify a < b. report(file, call, slice1, slice2) From ca10c1352214614d13c5cb2d20da71fe64e1848e Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Thu, 20 Mar 2025 17:22:48 +0300 Subject: [PATCH 05/10] add test and refactor old --- .../modernize/testdata/src/slicesdelete/slicesdelete.go | 6 +++++- .../testdata/src/slicesdelete/slicesdelete.go.golden | 8 ++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go index 67c28720a42..0ee608d8f9f 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go @@ -4,6 +4,8 @@ var g struct{ f []int } func h() []int { return []int{} } +var ch chan []int + func slicesdelete(test, other []byte, i int) { const k = 1 _ = append(test[:i], test[i+1:]...) // want "Replace append with slices.Delete" @@ -28,7 +30,9 @@ func slicesdelete(test, other []byte, i int) { _ = append(g.f[:i], g.f[i+k:]...) // want "Replace append with slices.Delete" - _ = append(h()[:i], h()[i+1:]...) // potentially non idempotent expression + _ = append(h()[:i], h()[i+1:]...) // potentially has side effects + + _ = append((<-ch)[:i], (<-ch)[i+1:]...) // has side effects _ = append(test[:3], test[i+1:]...) // cannot verify a < b diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden index eabddfe0c25..e119e3f303f 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden @@ -6,6 +6,8 @@ var g struct{ f []int } func h() []int { return []int{} } +var ch chan []int + func slicesdelete(test, other []byte, i int) { const k = 1 _ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete" @@ -30,7 +32,9 @@ func slicesdelete(test, other []byte, i int) { _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete" - _ = append(h()[:i], h()[i+1:]...) // potentially non idempotent expression + _ = append(h()[:i], h()[i+1:]...) // potentially has side effects + + _ = append((<-ch)[:i], (<-ch)[i+1:]...) // has side effects _ = append(test[:3], test[i+1:]...) // cannot verify a < b @@ -39,4 +43,4 @@ func slicesdelete(test, other []byte, i int) { _ = slices.Delete(test, 1+2, 3+4) // want "Replace append with slices.Delete" _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b -} \ No newline at end of file +} From 9fedd25cb74932d7982a10199ce9c490f5698e7a Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Thu, 20 Mar 2025 17:53:03 +0300 Subject: [PATCH 06/10] fix modernize format --- .../internal/analysis/modernize/modernize.go | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index c87716c8c1f..49bbd32565d 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -188,24 +188,24 @@ func enabledCategory(filter, category string) bool { return exclude } -// noEffects reports whether the expression has no side effects, i.e., it +// noEffects reports whether the expression has no side effects, i.e., it // does not modify the memory state. This function is conservative: it may // return false even when the expression has no effect. func noEffects(expr ast.Expr) bool { noEffects := true ast.Inspect(expr, func(n ast.Node) bool { - switch v := n.(type) { + switch v := n.(type) { case nil, *ast.Ident, *ast.BasicLit, *ast.BinaryExpr, *ast.ParenExpr, - *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr, - *ast.StarExpr, *ast.CompositeLit, *ast.ArrayType, *ast.StructType, - *ast.MapType, *ast.InterfaceType, *ast.KeyValueExpr: + *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr, + *ast.StarExpr, *ast.CompositeLit, *ast.ArrayType, *ast.StructType, + *ast.MapType, *ast.InterfaceType, *ast.KeyValueExpr: + return true + case *ast.UnaryExpr: + if v.Op == token.ARROW { + noEffects = false + return false + } return true - case *ast.UnaryExpr: - if v.Op == token.ARROW { - noEffects = false - return false - } - return true default: noEffects = false return false From 02d5a4afb589e64061a14b32ac0e4b3d01be231f Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Fri, 21 Mar 2025 19:57:03 +0300 Subject: [PATCH 07/10] format slicedelete.go.golden from testdata --- .../src/slicesdelete/slicesdelete.go.golden | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden index e119e3f303f..a15eb07dee9 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden @@ -9,38 +9,38 @@ func h() []int { return []int{} } var ch chan []int func slicesdelete(test, other []byte, i int) { - const k = 1 - _ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete" + const k = 1 + _ = slices.Delete(test, i, i+1) // want "Replace append with slices.Delete" - _ = slices.Delete(test, i+1, i+2) // want "Replace append with slices.Delete" + _ = slices.Delete(test, i+1, i+2) // want "Replace append with slices.Delete" - _ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements + _ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements - _ = append(test[:i], test[i-1:]...) // not deleting any slice elements + _ = append(test[:i], test[i-1:]...) // not deleting any slice elements - _ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete" + _ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete" - _ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete" + _ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete" - _ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other" + _ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other" - _ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b + _ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b - _ = append(test[:i-2], test[11:]...) // cannot verify a < b + _ = append(test[:i-2], test[11:]...) // cannot verify a < b - _ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete" + _ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete" - _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete" + _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete" _ = append(h()[:i], h()[i+1:]...) // potentially has side effects _ = append((<-ch)[:i], (<-ch)[i+1:]...) // has side effects - _ = append(test[:3], test[i+1:]...) // cannot verify a < b + _ = append(test[:3], test[i+1:]...) // cannot verify a < b - _ = slices.Delete(test, i-4, i-1) // want "Replace append with slices.Delete" + _ = slices.Delete(test, i-4, i-1) // want "Replace append with slices.Delete" - _ = slices.Delete(test, 1+2, 3+4) // want "Replace append with slices.Delete" + _ = slices.Delete(test, 1+2, 3+4) // want "Replace append with slices.Delete" - _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b + _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b } From 15aeda58b048872114b4d7c6d4848c00019829d3 Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Fri, 21 Mar 2025 20:17:40 +0300 Subject: [PATCH 08/10] speedup noEffects function --- gopls/internal/analysis/modernize/modernize.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 49bbd32565d..2389a74d1a6 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -199,17 +199,18 @@ func noEffects(expr ast.Expr) bool { *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr, *ast.StarExpr, *ast.CompositeLit, *ast.ArrayType, *ast.StructType, *ast.MapType, *ast.InterfaceType, *ast.KeyValueExpr: - return true + // no effect case *ast.UnaryExpr: + // channel send <-ch has effects if v.Op == token.ARROW { noEffects = false - return false } - return true default: + // all other expressions have effects noEffects = false - return false } + + return noEffects }) return noEffects } From 0f4fb6b6a8622ebbe4e8195ab0bb272ef935764b Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Fri, 21 Mar 2025 21:10:49 +0300 Subject: [PATCH 09/10] additional cases in noEffects --- .../internal/analysis/modernize/modernize.go | 19 +++++++++++++++---- .../analysis/modernize/slicesdelete.go | 2 +- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 2389a74d1a6..f980471aa45 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -191,7 +191,7 @@ func enabledCategory(filter, category string) bool { // noEffects reports whether the expression has no side effects, i.e., it // does not modify the memory state. This function is conservative: it may // return false even when the expression has no effect. -func noEffects(expr ast.Expr) bool { +func noEffects(info *types.Info, expr ast.Expr) bool { noEffects := true ast.Inspect(expr, func(n ast.Node) bool { switch v := n.(type) { @@ -199,14 +199,25 @@ func noEffects(expr ast.Expr) bool { *ast.SelectorExpr, *ast.IndexExpr, *ast.SliceExpr, *ast.TypeAssertExpr, *ast.StarExpr, *ast.CompositeLit, *ast.ArrayType, *ast.StructType, *ast.MapType, *ast.InterfaceType, *ast.KeyValueExpr: - // no effect + // No effect case *ast.UnaryExpr: - // channel send <-ch has effects + // Channel send <-ch has effects if v.Op == token.ARROW { noEffects = false } + case *ast.CallExpr: + // Type conversion has no effects + if !info.Types[v].IsType() { + // TODO(adonovan): Add a case for built-in functions without side + // effects (by using callsPureBuiltin from tools/internal/refactor/inline) + + noEffects = false + } + case *ast.FuncLit: + // A FuncLit has no effects, but do not descend into it. + return false default: - // all other expressions have effects + // All other expressions have effects noEffects = false } diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index 80d5c57f695..493009c35be 100644 --- a/gopls/internal/analysis/modernize/slicesdelete.go +++ b/gopls/internal/analysis/modernize/slicesdelete.go @@ -94,7 +94,7 @@ func slicesdelete(pass *analysis.Pass) { slice2, ok2 := call.Args[1].(*ast.SliceExpr) if ok1 && slice1.Low == nil && !slice1.Slice3 && ok2 && slice2.High == nil && !slice2.Slice3 && - equalSyntax(slice1.X, slice2.X) && noEffects(slice1.X) && + equalSyntax(slice1.X, slice2.X) && noEffects(info, slice1.X) && increasingSliceIndices(info, slice1.High, slice2.Low) { // Have append(s[:a], s[b:]...) where we can verify a < b. report(file, call, slice1, slice2) From 54e9082718e3d24ee82c681c494035fdc0e4e177 Mon Sep 17 00:00:00 2001 From: Zamir Ashurbekov Date: Fri, 21 Mar 2025 22:25:10 +0300 Subject: [PATCH 10/10] add TODOs for similar cases --- gopls/internal/analysis/modernize/slices.go | 3 +++ gopls/internal/analysis/modernize/slicescontains.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/gopls/internal/analysis/modernize/slices.go b/gopls/internal/analysis/modernize/slices.go index 7e0d9cbd92e..548a36638fb 100644 --- a/gopls/internal/analysis/modernize/slices.go +++ b/gopls/internal/analysis/modernize/slices.go @@ -210,6 +210,9 @@ func appendclipped(pass *analysis.Pass) { // x[:len(x):len(x)] (nonempty) res=x // x[:k:k] (nonempty) // slices.Clip(x) (nonempty) res=x +// +// TODO(adonovan): Add a check that the expression x has no side effects in +// case x[:len(x):len(x)] -> x. Now the program behavior may change. func clippedSlice(info *types.Info, e ast.Expr) (res ast.Expr, empty bool) { switch e := e.(type) { case *ast.SliceExpr: diff --git a/gopls/internal/analysis/modernize/slicescontains.go b/gopls/internal/analysis/modernize/slicescontains.go index b59ea452a0f..589efe7ffc8 100644 --- a/gopls/internal/analysis/modernize/slicescontains.go +++ b/gopls/internal/analysis/modernize/slicescontains.go @@ -46,6 +46,9 @@ import ( // It may change cardinality of effects of the "needle" expression. // (Mostly this appears to be a desirable optimization, avoiding // redundantly repeated evaluation.) +// +// TODO(adonovan): Add a check that needle/predicate expression from +// if-statement has no effects. Now the program behavior may change. func slicescontains(pass *analysis.Pass) { // Skip the analyzer in packages where its // fixes would create an import cycle.