x/tools/gopls/internal/analysis/modernize: minmax removes important comment

[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 modernize a Go code base:

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

What did you see happen?

it modernized code but removed some comment that explained stuff.

ie: on the following code:

package modernize

var CutOff float64

func F(v float64) float64 {
	o := v - 42
	// some important comment. DO NOT REMOVE.
	if o < CutOff {
		o = CutOff
	}
	return o
}

it generated the following diff:

diff --git a/old.go b/old.go
index f8d7b3d..7758b71 100644
--- a/old.go
+++ b/old.go
@@ -3,10 +3,6 @@ package modernize
 var CutOff float64
 
 func F(v float64) float64 {
-       o := v - 42
-       // some important comment. DO NOT REMOVE.
-       if o < CutOff {
-               o = CutOff
-       }
+       o := max(v-42, CutOff)
        return o
 }

What did you expect to see?

the comment should have been kept:

diff --git a/old.go b/old.go
index f8d7b3d..a67cbc1 100644
--- a/old.go
+++ b/old.go
@@ -3,10 +3,7 @@ package modernize
 var CutOff float64
 
 func F(v float64) float64 {
-       o := v - 42
        // some important comment. DO NOT REMOVE.
-       if o < CutOff {
-               o = CutOff
-       }
+       o := max(v-42, CutOff)
        return o
 }

[gabyhelp]

Related Issues

• cmd/gofmt: rewrite removes comments #42031
• x/tools/gopls: modernizer max changes logic #71878 (closed)
• cmd/go: go mod tidy reverts upgrade #38985 (closed)
• x/tools/gopls/internal/analysis/modernize: rangeint generates correct but not idiomatic transformation #71725 (closed)
• x/tools/gopls: modernize x/exp/maps.Clear for Go >= 1.21 #70717 (closed)
• golint: strange behaviour when processing code involving go.mod replace statements #57658 (closed)
• cmd/gofmt: -r strips some comments when pattern matching #57574
• x/tools/gopls/internal/analysis/modernize/cmd/modernize: generates illegal for-range code #72726
• x/tools/gopls/internal/analysis/modernize: Panic in modernize/sortslice.go #71786 (closed)
• cmd/go: go.mod formatting drops unattached comments within blocks #33280

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

[adonovan]

We should probably preserve all comments when replacing a block with a function call, even when that means moving them to places where they no longer make perfect sense.

[gopherbot]

Change https://go.dev/cl/656655 mentions this issue: gopls/internal/analysis/modernize: preserves comments in minmax

[nightlyone]

I am not sure it makes sense to rewrite to min/max in those cases. The comments may be talking about an else branch or something like this. It probably stays more readable, when not rewritten.

I would flag that code as should be changed, but not automatically rewrite it.