[tools] go/analysis/passes/modernize: preserve evaluation effects in slicessort

0 views

Skip to first unread message

shuang cui (Gerrit)

unread,

1:16 AM (2 hours ago) 1:16 AM

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

shuang cui has uploaded the change for review

Commit message

go/analysis/passes/modernize: preserve evaluation effects in slicessort

The slicessort analyzer matches slice expressions in the comparison
function using syntactic equality. If the expression has side effects,
replacing sort.Slice with slices.Sort reduces repeated evaluations to one.

Only offer the modernization when the slice expression has no effects.
Add a regression test for a function call used as the slice expression.

Fixes golang/go#80844

Change-Id: I16fa33103cabd363766a147dcd06243d4fa32347

Change diff

diff --git a/go/analysis/passes/modernize/sortslice.go b/go/analysis/passes/modernize/sortslice.go
index 08d8667..c291e6f 100644
--- a/go/analysis/passes/modernize/sortslice.go
+++ b/go/analysis/passes/modernize/sortslice.go
@@ -15,6 +15,7 @@
 	typeindexanalyzer "golang.org/x/tools/internal/analysis/typeindex"
 	"golang.org/x/tools/internal/astutil"
 	"golang.org/x/tools/internal/refactor"
+	"golang.org/x/tools/internal/typesinternal"
 	"golang.org/x/tools/internal/typesinternal/typeindex"
 	"golang.org/x/tools/internal/versions"
 )
@@ -84,6 +85,7 @@
 					}
 					file := astutil.EnclosingFile(curCall)
 					if isIndex(compare.X, i) && isIndex(compare.Y, j) &&
+						typesinternal.NoEffects(info, s) &&
 						analyzerutil.FileUsesGoVersion(pass, file, versions.Go1_21) {
 						// Have: sort.Slice(s, func(i, j int) bool { return s[i] < s[j] })
 
diff --git a/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go b/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go
index 50c9021..df1577a 100644
--- a/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go
+++ b/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go
@@ -12,6 +12,20 @@
 	sort.Slice(x.s, func(first, second int) bool { return x.s[first] < x.s[second] }) // want "sort.Slice can be modernized using slices.Sort"
 }
 
+var sideEffectSlice []int
+var sliceCalls int
+
+func getSlice() []int {
+	sliceCalls++
+	return sideEffectSlice
+}
+
+func _() {
+	// Replacing this call with slices.Sort(getSlice()) would reduce the
+	// number of evaluations of getSlice from many to one.
+	sort.Slice(getSlice(), func(i, j int) bool { return getSlice()[i] < getSlice()[j] }) // nope: slice expression may have effects
+}
+
 func _(s []int) {
 	sort.Slice(s, func(i, j int) bool { return s[i] > s[j] }) // nope: wrong comparison operator
 }
diff --git a/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go.golden b/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go.golden
index 58be9ab..1eb7bfd 100644
--- a/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go.golden
+++ b/go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go.golden
@@ -14,6 +14,20 @@
 	slices.Sort(x.s) // want "sort.Slice can be modernized using slices.Sort"
 }
 
+var sideEffectSlice []int
+var sliceCalls int
+
+func getSlice() []int {
+	sliceCalls++
+	return sideEffectSlice
+}
+
+func _() {
+	// Replacing this call with slices.Sort(getSlice()) would reduce the
+	// number of evaluations of getSlice from many to one.
+	sort.Slice(getSlice(), func(i, j int) bool { return getSlice()[i] < getSlice()[j] }) // nope: slice expression may have effects
+}
+
 func _(s []int) {
 	sort.Slice(s, func(i, j int) bool { return s[i] > s[j] }) // nope: wrong comparison operator
 }

Change information

Files:

M go/analysis/passes/modernize/sortslice.go
M go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go
M go/analysis/passes/modernize/testdata/src/slicessort/slicessort.go.golden

Change size: S

Delta: 3 files changed, 30 insertions(+), 0 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

unsatisfied_requirement

satisfied_requirement

open

diffy

shuang cui (Gerrit)

unread,

1:18 AM (2 hours ago) 1:18 AM

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

shuang cui voted and added 1 comment

Votes added by shuang cui

Commit-Queue

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

shuang cui . resolved

The `waitgroupgo` and `stringscutprefix` analyzers have similar repeated evaluation concerns. If this approach is accepted, I plan to address them in separate follow-up changes.