gopls/internal/analysis/modernize: disable slicesdelete pass

Unfortunately, it is not safe to replace append(s[:i], s[j:]...)
with slices.Delete(s, i, j) because the latter clears out the
vacated array slots, which may have unexpected effects on
program behavior in esoteric cases.

So we disable the pass for now. Perhaps we can later enable it
after either making it more sophisticated or revising our safety
goals.

Fixes golang/go#73686

Change-Id: Ia8be98c8f26c1699b7d75f25e2efdb5a71696031
Reviewed-on: https://go-review.googlesource.com/c/tools/+/673516
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Commit-Queue: Alan Donovan <[email protected]>

Author: adonovan

gopls/doc/analyzers.md

@@ -3741,9 +3741,6 @@ Categories of modernize diagnostic:
     benchmark with "for b.Loop()", and remove any preceding calls
     to b.StopTimer, b.StartTimer, and b.ResetTimer.
 
-  - slicesdelete: replace append(s[:i], s[i+1]...) by
-    slices.Delete(s, i, i+1), added in go1.21.
-
   - rangeint: replace a 3-clause "for i := 0; i < n; i++" loop by
     "for i := range n", added in go1.22.
 

gopls/internal/analysis/modernize/doc.go

@@ -82,9 +82,6 @@
 //     benchmark with "for b.Loop()", and remove any preceding calls
 //     to b.StopTimer, b.StartTimer, and b.ResetTimer.
 //
-//   - slicesdelete: replace append(s[:i], s[i+1]...) by
-//     slices.Delete(s, i, i+1), added in go1.21.
-//
 //   - rangeint: replace a 3-clause "for i := 0; i < n; i++" loop by
 //     "for i := range n", added in go1.22.
 //

gopls/internal/analysis/modernize/modernize_test.go

@@ -12,6 +12,8 @@ import (
 )
 
 func Test(t *testing.T) {
+	modernize.EnableSlicesDelete = true
+
 	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), modernize.Analyzer,
 		"appendclipped",
 		"bloop",

gopls/internal/analysis/modernize/slicesdelete.go

@@ -16,11 +16,26 @@ import (
 	"golang.org/x/tools/internal/analysisinternal"
 )
 
+// slices.Delete is not equivalent to append(s[:i], [j:]...):
+// it clears the vacated array slots; see #73686.
+// Until we either fix it or revise our safety goals,
+// we disable this analyzer for now.
+//
+// Its former documentation in doc.go was:
+//
+//   - slicesdelete: replace append(s[:i], s[i+1]...) by
+//     slices.Delete(s, i, i+1), added in go1.21.
+var EnableSlicesDelete = false
+
 // The slicesdelete pass attempts to replace instances of append(s[:i], s[i+k:]...)
 // with slices.Delete(s, i, i+k) where k is some positive constant.
 // Other variations that will also have suggested replacements include:
 // append(s[:i-1], s[i:]...) and append(s[:i+k1], s[i+k2:]) where k2 > k1.
 func slicesdelete(pass *analysis.Pass) {
+	if !EnableSlicesDelete {
+		return
+	}
+
 	// Skip the analyzer in packages where its
 	// fixes would create an import cycle.
 	if within(pass, "slices", "runtime") {

gopls/internal/doc/api.json

@@ -1492,7 +1492,7 @@
 						},
 						{
 							"Name": "\"modernize\"",
-							"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go and its standard\nlibrary.\n\nEach diagnostic provides a fix. Our intent is that these fixes may\nbe safely applied en masse without changing the behavior of your\nprogram. In some cases the suggested fixes are imperfect and may\nlead to (for example) unused imports or unused local variables,\ncausing build breakage. However, these problems are generally\ntrivial to fix. We regard any modernizer whose fix changes program\nbehavior to have a serious bug and will endeavor to fix it.\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...\n\n(Do not use \"go get -tool\" to add gopls as a dependency of your\nmodule; gopls commands must be built from their release branch.)\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.\n\nChanges produced by this tool should be reviewed as usual before\nbeing merged. In some cases, a loop may be replaced by a simple\nfunction call, causing comments within the loop to be discarded.\nHuman judgment may be required to avoid losing comments of value.\n\nEach diagnostic reported by modernize has a specific category. (The\ncategories are listed below.) Diagnostics in some categories, such\nas \"efaceany\" (which replaces \"interface{}\" with \"any\" where it is\nsafe to do so) are particularly numerous. It may ease the burden of\ncode review to apply fixes in two passes, the first change\nconsisting only of fixes of category \"efaceany\", the second\nconsisting of all others. This can be achieved using the -category flag:\n\n\t$ modernize -category=efaceany  -fix -test ./...\n\t$ modernize -category=-efaceany -fix -test ./...\n\nCategories of modernize diagnostic:\n\n  - forvar: remove x := x variable declarations made unnecessary by the new semantics of loops in go1.22.\n\n  - slicescontains: replace 'for i, elem := range s { if elem == needle { ...; break }'\n    by a call to slices.Contains, added in go1.21.\n\n  - minmax: replace an if/else conditional assignment by a call to\n    the built-in min or max functions added in go1.21.\n\n  - sortslice: replace sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n    by a call to slices.Sort(s), added in go1.21.\n\n  - efaceany: replace interface{} by the 'any' type added in go1.18.\n\n  - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or\n    slices.Concat(s), added in go1.21.\n\n  - mapsloop: replace a loop around an m[k]=v map update by a call\n    to one of the Collect, Copy, Clone, or Insert functions from\n    the maps package, added in go1.21.\n\n  - fmtappendf: replace []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n    added in go1.19.\n\n  - testingcontext: replace uses of context.WithCancel in tests\n    with t.Context, added in go1.24.\n\n  - omitzero: replace omitempty by omitzero on structs, added in go1.24.\n\n  - bloop: replace \"for i := range b.N\" or \"for range b.N\" in a\n    benchmark with \"for b.Loop()\", and remove any preceding calls\n    to b.StopTimer, b.StartTimer, and b.ResetTimer.\n\n  - slicesdelete: replace append(s[:i], s[i+1]...) by\n    slices.Delete(s, i, i+1), added in go1.21.\n\n  - rangeint: replace a 3-clause \"for i := 0; i \u003c n; i++\" loop by\n    \"for i := range n\", added in go1.22.\n\n  - stringsseq: replace Split in \"for range strings.Split(...)\" by go1.24's\n    more efficient SplitSeq, or Fields with FieldSeq.\n\n  - stringscutprefix: replace some uses of HasPrefix followed by TrimPrefix with CutPrefix,\n    added to the strings package in go1.20.\n\n  - waitgroup: replace old complex usages of sync.WaitGroup by less complex WaitGroup.Go method in go1.25.",
+							"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go and its standard\nlibrary.\n\nEach diagnostic provides a fix. Our intent is that these fixes may\nbe safely applied en masse without changing the behavior of your\nprogram. In some cases the suggested fixes are imperfect and may\nlead to (for example) unused imports or unused local variables,\ncausing build breakage. However, these problems are generally\ntrivial to fix. We regard any modernizer whose fix changes program\nbehavior to have a serious bug and will endeavor to fix it.\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...\n\n(Do not use \"go get -tool\" to add gopls as a dependency of your\nmodule; gopls commands must be built from their release branch.)\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.\n\nChanges produced by this tool should be reviewed as usual before\nbeing merged. In some cases, a loop may be replaced by a simple\nfunction call, causing comments within the loop to be discarded.\nHuman judgment may be required to avoid losing comments of value.\n\nEach diagnostic reported by modernize has a specific category. (The\ncategories are listed below.) Diagnostics in some categories, such\nas \"efaceany\" (which replaces \"interface{}\" with \"any\" where it is\nsafe to do so) are particularly numerous. It may ease the burden of\ncode review to apply fixes in two passes, the first change\nconsisting only of fixes of category \"efaceany\", the second\nconsisting of all others. This can be achieved using the -category flag:\n\n\t$ modernize -category=efaceany  -fix -test ./...\n\t$ modernize -category=-efaceany -fix -test ./...\n\nCategories of modernize diagnostic:\n\n  - forvar: remove x := x variable declarations made unnecessary by the new semantics of loops in go1.22.\n\n  - slicescontains: replace 'for i, elem := range s { if elem == needle { ...; break }'\n    by a call to slices.Contains, added in go1.21.\n\n  - minmax: replace an if/else conditional assignment by a call to\n    the built-in min or max functions added in go1.21.\n\n  - sortslice: replace sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n    by a call to slices.Sort(s), added in go1.21.\n\n  - efaceany: replace interface{} by the 'any' type added in go1.18.\n\n  - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or\n    slices.Concat(s), added in go1.21.\n\n  - mapsloop: replace a loop around an m[k]=v map update by a call\n    to one of the Collect, Copy, Clone, or Insert functions from\n    the maps package, added in go1.21.\n\n  - fmtappendf: replace []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n    added in go1.19.\n\n  - testingcontext: replace uses of context.WithCancel in tests\n    with t.Context, added in go1.24.\n\n  - omitzero: replace omitempty by omitzero on structs, added in go1.24.\n\n  - bloop: replace \"for i := range b.N\" or \"for range b.N\" in a\n    benchmark with \"for b.Loop()\", and remove any preceding calls\n    to b.StopTimer, b.StartTimer, and b.ResetTimer.\n\n  - rangeint: replace a 3-clause \"for i := 0; i \u003c n; i++\" loop by\n    \"for i := range n\", added in go1.22.\n\n  - stringsseq: replace Split in \"for range strings.Split(...)\" by go1.24's\n    more efficient SplitSeq, or Fields with FieldSeq.\n\n  - stringscutprefix: replace some uses of HasPrefix followed by TrimPrefix with CutPrefix,\n    added to the strings package in go1.20.\n\n  - waitgroup: replace old complex usages of sync.WaitGroup by less complex WaitGroup.Go method in go1.25.",
 							"Default": "true",
 							"Status": ""
 						},
@@ -3212,7 +3212,7 @@
 		},
 		{
 			"Name": "modernize",
-			"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go and its standard\nlibrary.\n\nEach diagnostic provides a fix. Our intent is that these fixes may\nbe safely applied en masse without changing the behavior of your\nprogram. In some cases the suggested fixes are imperfect and may\nlead to (for example) unused imports or unused local variables,\ncausing build breakage. However, these problems are generally\ntrivial to fix. We regard any modernizer whose fix changes program\nbehavior to have a serious bug and will endeavor to fix it.\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...\n\n(Do not use \"go get -tool\" to add gopls as a dependency of your\nmodule; gopls commands must be built from their release branch.)\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.\n\nChanges produced by this tool should be reviewed as usual before\nbeing merged. In some cases, a loop may be replaced by a simple\nfunction call, causing comments within the loop to be discarded.\nHuman judgment may be required to avoid losing comments of value.\n\nEach diagnostic reported by modernize has a specific category. (The\ncategories are listed below.) Diagnostics in some categories, such\nas \"efaceany\" (which replaces \"interface{}\" with \"any\" where it is\nsafe to do so) are particularly numerous. It may ease the burden of\ncode review to apply fixes in two passes, the first change\nconsisting only of fixes of category \"efaceany\", the second\nconsisting of all others. This can be achieved using the -category flag:\n\n\t$ modernize -category=efaceany  -fix -test ./...\n\t$ modernize -category=-efaceany -fix -test ./...\n\nCategories of modernize diagnostic:\n\n  - forvar: remove x := x variable declarations made unnecessary by the new semantics of loops in go1.22.\n\n  - slicescontains: replace 'for i, elem := range s { if elem == needle { ...; break }'\n    by a call to slices.Contains, added in go1.21.\n\n  - minmax: replace an if/else conditional assignment by a call to\n    the built-in min or max functions added in go1.21.\n\n  - sortslice: replace sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n    by a call to slices.Sort(s), added in go1.21.\n\n  - efaceany: replace interface{} by the 'any' type added in go1.18.\n\n  - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or\n    slices.Concat(s), added in go1.21.\n\n  - mapsloop: replace a loop around an m[k]=v map update by a call\n    to one of the Collect, Copy, Clone, or Insert functions from\n    the maps package, added in go1.21.\n\n  - fmtappendf: replace []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n    added in go1.19.\n\n  - testingcontext: replace uses of context.WithCancel in tests\n    with t.Context, added in go1.24.\n\n  - omitzero: replace omitempty by omitzero on structs, added in go1.24.\n\n  - bloop: replace \"for i := range b.N\" or \"for range b.N\" in a\n    benchmark with \"for b.Loop()\", and remove any preceding calls\n    to b.StopTimer, b.StartTimer, and b.ResetTimer.\n\n  - slicesdelete: replace append(s[:i], s[i+1]...) by\n    slices.Delete(s, i, i+1), added in go1.21.\n\n  - rangeint: replace a 3-clause \"for i := 0; i \u003c n; i++\" loop by\n    \"for i := range n\", added in go1.22.\n\n  - stringsseq: replace Split in \"for range strings.Split(...)\" by go1.24's\n    more efficient SplitSeq, or Fields with FieldSeq.\n\n  - stringscutprefix: replace some uses of HasPrefix followed by TrimPrefix with CutPrefix,\n    added to the strings package in go1.20.\n\n  - waitgroup: replace old complex usages of sync.WaitGroup by less complex WaitGroup.Go method in go1.25.",
+			"Doc": "simplify code by using modern constructs\n\nThis analyzer reports opportunities for simplifying and clarifying\nexisting code by using more modern features of Go and its standard\nlibrary.\n\nEach diagnostic provides a fix. Our intent is that these fixes may\nbe safely applied en masse without changing the behavior of your\nprogram. In some cases the suggested fixes are imperfect and may\nlead to (for example) unused imports or unused local variables,\ncausing build breakage. However, these problems are generally\ntrivial to fix. We regard any modernizer whose fix changes program\nbehavior to have a serious bug and will endeavor to fix it.\n\nTo apply all modernization fixes en masse, you can use the\nfollowing command:\n\n\t$ go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...\n\n(Do not use \"go get -tool\" to add gopls as a dependency of your\nmodule; gopls commands must be built from their release branch.)\n\nIf the tool warns of conflicting fixes, you may need to run it more\nthan once until it has applied all fixes cleanly. This command is\nnot an officially supported interface and may change in the future.\n\nChanges produced by this tool should be reviewed as usual before\nbeing merged. In some cases, a loop may be replaced by a simple\nfunction call, causing comments within the loop to be discarded.\nHuman judgment may be required to avoid losing comments of value.\n\nEach diagnostic reported by modernize has a specific category. (The\ncategories are listed below.) Diagnostics in some categories, such\nas \"efaceany\" (which replaces \"interface{}\" with \"any\" where it is\nsafe to do so) are particularly numerous. It may ease the burden of\ncode review to apply fixes in two passes, the first change\nconsisting only of fixes of category \"efaceany\", the second\nconsisting of all others. This can be achieved using the -category flag:\n\n\t$ modernize -category=efaceany  -fix -test ./...\n\t$ modernize -category=-efaceany -fix -test ./...\n\nCategories of modernize diagnostic:\n\n  - forvar: remove x := x variable declarations made unnecessary by the new semantics of loops in go1.22.\n\n  - slicescontains: replace 'for i, elem := range s { if elem == needle { ...; break }'\n    by a call to slices.Contains, added in go1.21.\n\n  - minmax: replace an if/else conditional assignment by a call to\n    the built-in min or max functions added in go1.21.\n\n  - sortslice: replace sort.Slice(x, func(i, j int) bool) { return s[i] \u003c s[j] }\n    by a call to slices.Sort(s), added in go1.21.\n\n  - efaceany: replace interface{} by the 'any' type added in go1.18.\n\n  - slicesclone: replace append([]T(nil), s...) by slices.Clone(s) or\n    slices.Concat(s), added in go1.21.\n\n  - mapsloop: replace a loop around an m[k]=v map update by a call\n    to one of the Collect, Copy, Clone, or Insert functions from\n    the maps package, added in go1.21.\n\n  - fmtappendf: replace []byte(fmt.Sprintf...) by fmt.Appendf(nil, ...),\n    added in go1.19.\n\n  - testingcontext: replace uses of context.WithCancel in tests\n    with t.Context, added in go1.24.\n\n  - omitzero: replace omitempty by omitzero on structs, added in go1.24.\n\n  - bloop: replace \"for i := range b.N\" or \"for range b.N\" in a\n    benchmark with \"for b.Loop()\", and remove any preceding calls\n    to b.StopTimer, b.StartTimer, and b.ResetTimer.\n\n  - rangeint: replace a 3-clause \"for i := 0; i \u003c n; i++\" loop by\n    \"for i := range n\", added in go1.22.\n\n  - stringsseq: replace Split in \"for range strings.Split(...)\" by go1.24's\n    more efficient SplitSeq, or Fields with FieldSeq.\n\n  - stringscutprefix: replace some uses of HasPrefix followed by TrimPrefix with CutPrefix,\n    added to the strings package in go1.20.\n\n  - waitgroup: replace old complex usages of sync.WaitGroup by less complex WaitGroup.Go method in go1.25.",
 			"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/modernize",
 			"Default": true
 		},