x/tools/gopls: modernize bloop: Can't use b.Loop() if loop contains b.StopTimer/b.StartTimer

[Jille]

Go version

go version go1.24.1 linux/amd64

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/jille/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/jille/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2881335881=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/jille/src/ilsa-sanity/go.mod'
GOMODCACHE='/home/jille/go/pkg/mod'
GONOPROXY=''
GONOSUMDB='src.hexon.nl/*'
GOOS='linux'
GOPATH='/home/jille/go'
GOPRIVATE=''
GOROOT='/usr/lib/go-1.24'
GOSUMDB='sum.golang.org'
GOTELEMETRY='on'
GOTELEMETRYDIR='/home/jille/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go-1.24/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.1'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I ran modernize which upgraded some benchmarking loops from for i := 0; i < b.N; i++ {} to for b.Loop {} on https://github.com/bluele/psort/blob/master/psort_test.go.

What did you see happen?

The benchmark times out after 10 minutes, whereas it previously finished in seconds.

What did you expect to see?

The code changed by modernize should do the exact same thing.

I suspect we need to teach modernize not to touch loops which call b.StopTimer() :(

[gabyhelp]

Related Issues

• x/tools/gopls/internal/analysis/modernize: replacing non-equivalent code #73073 (closed)
• testing: b.StopTimer breaks b.Loop #72922 (closed)
• x/tools/gopls: modernizer max changes logic #71878 (closed)
• x/tools/gopls/internal/analysis/modernize: rangeint: transformation unsound when loop variable is defined outside loop #73009 (closed)
• x/tools/gopls: modernize's rangeint check is too aggressive #74295 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[Jille]

As @gabyhelp nicely found is that this issue is fixed in #72922, which was backported to Go 1.24.2.

What is the policy for this? Do we expect users to just upgrade, or should modernize take this into account somehow?

[findleyr]

Hmm, @adonovan is doing a talk at GopherCon about the difficulty of writing modernizers. This would be a good edge case to include!

Generally speaking, modernizers should never break your code (that's part of the idea), but this issue illustrates part of why that's so hard to guarantee!

Milestoning for the next release. I suppose the simplest fix is to disable the Bloop modernizer if the Go version is <1.24.2. Too much work to allow it if StopTimer/StartTimer are not called.

[adonovan]

As @gabyhelp nicely found is that this issue is fixed in #72922, which was backported to Go 1.24.2.

For the record, #72922 is about the bug in the dynamic behavior of b.Loop; this issue is about the static analysis of b.Loop, which still has a bug.