x/tools/gopls/internal/analysis/modernize: rangeint should be silent when RHS of 'i < limit' depends on loop var

[sbinet]

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='/home/binet/work/gonum/bin'
GOCACHE='/home/binet/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/binet/.config/go/env'
GOEXE=''
GOEXPERIMENT='rangefunc'
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3343970528=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/binet/work/gonum/src/gonum.org/v1/gonum/go.mod'
GOMODCACHE='/home/binet/work/gonum/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/binet/work/gonum'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/binet/sdk/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/binet/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/binet/sdk/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.1'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

I ran the following command to test the modernize command of gopls on Gonum source code:

$> go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test ./...

What did you see happen?

it generated some non-compileable code, replacing:

for i := 0; i < 1e6; i++ {
   ...
}

with:

for range 1e6 {
   ...
}

resulting in the following compilation error:

$> go test ./stat/sampleuv
# gonum.org/v1/gonum/stat/sampleuv [gonum.org/v1/gonum/stat/sampleuv.test]
stat/sampleuv/weighted_test.go:71:12: cannot range over 1e6 (untyped float constant 1e+06)
FAIL	gonum.org/v1/gonum/stat/sampleuv [build failed]

What did you expect to see?

A still buildable repository.

I must admit I am not sure whether it's a short-coming of the for-range syntax or semantics (should it silently accept 1e6 as an untype int constant ?), or a shortcoming of cmd/modernize ?

should cmd/modernize automatically generate code that convert floaty-like constants into an int ?
ie:

for range int(1e6) {
   ...
}

[gabyhelp]

Related Issues

• x/tools/gopls/internal/analysis/modernize: rangeint generates correct but not idiomatic transformation #71725 (closed)
• x/tools/gopls: yet more modernizer bugs #71847 (closed)
• cmd/compile: untyped float constants representable as int are rejected in range clauses #66561 (closed)
• x/tools/gopls: range over func without variable shows 'simplify range expression' warning #65799 (closed)
• x/tools/gopls: issues errors for range-over-func (InvalidRangeExpr) in go1.23rc1 projects #68248 (closed)
• cmd/go: build tags do not work quite as expected using devel version of Go #66299 (closed)
• cmd/compile: out of range integer in range loop crashes the compiler #65133 (closed)
• x/tools/gopls: InvalidRangeExpr when using GOEXPERIMENT=rangefunc on iter.Seq #65667 (closed)
• cmd/go: per-file Go versions via //go:build do not play well with goexperiment #66399
• x/tools/gopls: #52425 (closed)

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

[sbinet]

another failure mode:

diff --git a/lapack/testlapack/matgen.go b/lapack/testlapack/matgen.go
index b35f8601..6711029d 100644
--- a/lapack/testlapack/matgen.go
+++ b/lapack/testlapack/matgen.go
@@ -962,14 +962,14 @@ func dlattb(kind int, uplo blas.Uplo, trans blas.Transpose, n, kd int, ab []floa
                // Generate a triangular matrix with elements between -1 and 1.
                // Give the diagonal norm 2 to make it well-conditioned.
                // Make the right hand side large so that it requires scaling.
                if uplo == blas.Upper {
-                       for i := 0; i < n; i++ {
-                               for j := 0; j < min(n-j, kd+1); j++ {
+                       for i := range n {
+                               for j := range min(n-j, kd+1) {
                                        ab[i*ldab+j] = uniformM11()
                                }
                                ab[i*ldab] = math.Copysign(2, ab[i*ldab])
                        }

[dagood]

I must admit I am not sure whether it's a short-coming of the for-range syntax or semantics (should it silently accept 1e6 as an untype int constant ?), or a shortcoming of cmd/modernize ?

I was curious about this, and found this issue where relaxing range was considered but not done (for now). Maybe the final reasoning has some hint about the right move here.

• cmd/compile: untyped float constants representable as int are rejected in range clauses #66561
  (I see now that gabyhelp also found it!)

[adonovan]

Thanks for the report. I think this is a dup of the issue mentioned in #71847 and since fixed by https://go.dev/cl/653616, but not yet released. If you git clone and build gopls from source you should observe the fix. (Unfortunately you can't use go install gopls@arbitraryref because our go.mod file has a replace directive.)

[sbinet]

yes, the first issue is a dup (gaby did a better job than me at finding these). (I'll check over the week-end)

what about #72726 (comment) though ?
(should I create another dedicated issue for this ?)

thanks for the tool :)

[adonovan]

-                       for i := 0; i < n; i++ {
-                               for j := 0; j < min(n-j, kd+1); j++ {
+                       for i := range n {
+                               for j := range min(n-j, kd+1) {

Oh, thanks for reporting. I forgot to check for uses of the loop var within the limit expression. Fortunately it generates a compile error.

There's also a secondary issue that the rangeint fix causes the limit to be evaluated once only, instead of once per iteration. For most loops, this is a nice performance benefit, but there may be some subtle loops that rely on the limit expression's value changing as the loop progresses. I'm not sure what the correct balance of strictness in that case.

[gopherbot]

Change https://go.dev/cl/656975 mentions this issue: gopls/internal/analysis/modernize: rangeint: avoid offering wrong fix