diff --git a/gopls/internal/analysis/modernize/modernize.go b/gopls/internal/analysis/modernize/modernize.go index 75f5b4014b6..f980471aa45 100644 --- a/gopls/internal/analysis/modernize/modernize.go +++ b/gopls/internal/analysis/modernize/modernize.go @@ -187,3 +187,41 @@ func enabledCategory(filter, category string) bool { } return exclude } + +// 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(info *types.Info, expr ast.Expr) bool { + noEffects := true + ast.Inspect(expr, func(n ast.Node) bool { + 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: + // No effect + case *ast.UnaryExpr: + // 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 + noEffects = false + } + + return noEffects + }) + return noEffects +} 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. diff --git a/gopls/internal/analysis/modernize/slicesdelete.go b/gopls/internal/analysis/modernize/slicesdelete.go index 3c3d880f62b..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) && + 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) diff --git a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go index a710d06f2fe..0ee608d8f9f 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go @@ -2,6 +2,10 @@ package slicesdelete 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" @@ -26,6 +30,10 @@ 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 has side effects + + _ = append((<-ch)[:i], (<-ch)[i+1:]...) // has side effects + _ = 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..a15eb07dee9 100644 --- a/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden +++ b/gopls/internal/analysis/modernize/testdata/src/slicesdelete/slicesdelete.go.golden @@ -4,35 +4,43 @@ import "slices" 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" + 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" + + _ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements - _ = slices.Delete(test, i+1, i+2) // want "Replace append with slices.Delete" + _ = append(test[:i], test[i-1:]...) // not deleting any slice elements - _ = append(test[:i+1], test[i+1:]...) // not deleting any slice elements + _ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete" - _ = append(test[:i], test[i-1:]...) // not deleting any slice elements + _ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete" - _ = slices.Delete(test, i-1, i) // want "Replace append with slices.Delete" + _ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other" - _ = slices.Delete(test, i-2, i+1) // want "Replace append with slices.Delete" + _ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b - _ = append(test[:i-2], other[i+1:]...) // different slices "test" and "other" + _ = append(test[:i-2], test[11:]...) // cannot verify a < b - _ = append(test[:i-2], other[i+1+k:]...) // cannot verify a < b + _ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete" - _ = append(test[:i-2], test[11:]...) // cannot verify a < b + _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete" - _ = slices.Delete(test, 1, 3) // want "Replace append with slices.Delete" + _ = append(h()[:i], h()[i+1:]...) // potentially has side effects - _ = slices.Delete(g.f, i, i+k) // want "Replace append with slices.Delete" + _ = 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 -} \ No newline at end of file + _ = append(test[:1+2], test[i-1:]...) // cannot verify a < b +}