x/tools/gopls/internal/analysis/modernize: append([]T{}, x...) -> slices.Clone(x) fix may unsoundly return nil

[aronatkins]

Go version

go version go1.24.2 darwin/arm64, golang.org/x/tools/gopls v0.18.1

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/Users/aron/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/aron/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/f5/d_lvj8s17bx46zzhfr5gqywc0000gp/T/go-build2208041228=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/Users/aron/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/aron/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/aron/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.24.2'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Original code:

package main

import (
  "fmt"
)

func main() {
  var m []string
  s := append([]string{}, m[0:]...)
  fmt.Printf("%#v\n", s)
}

Prior to any modification, running the program:

go run ./main.go
#> []string{}

What did you see happen?

Adjusting this code with the modernizer modifies its behavior.

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

After modification, running the program:

go run ./main.go
#> []string(nil)

The modified contents:

package main

import (
	"fmt"
	"slices"
)

func main() {
	var m []string
	s := slices.Clone(m[0:])
	fmt.Printf("%#v\n", s)
}

What did you expect to see?

I expected the result of the program to be unchanged.

Workaround: Detect when m or s is nil and use a []string{} value rather than the slices.Clone() result.

[gabyhelp]

Related Issues

• x/tools/gopls/internal/analysis/modernize: rangeint generates correct but not idiomatic transformation #71725 (closed)
• tools/gopls/internal/analysis/modernize: stringscutprefix offers invalid fix #73547
• x/tools/gopls/internal/analysis/modernize: replacing non-equivalent code #73073 (closed)
• strings.Split a empty string, and len([]string) equal 1 #39806 (closed)
• x/tools/gopls/internal/analysis/modernize: the conversion of variadic functions to `slices.ContainsFunc` causes type errors #73269 (closed)
• x/tools/gopls: No warning "loop variable xx captured by func literal". #60016 (closed)
• x/exp/slices: Clone returns nil for an empty non-nil slice #49660 (closed)
• affected/package: golang.org/x/exp/slices#Compact #51788 (closed)
• x/tools/gopls/internal/analysis/modernize: Incorrect fix for map clone #72009 (closed)
• x/tools/gopls/internal/analysis/modernize: Panic in modernize/sortslice.go #71786 (closed)

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

[gopherbot]

Change https://go.dev/cl/669295 mentions this issue: gopls/internal/analysis/modernize: preserve non-nil behavior when simplifying append([]T{}, s...)

[adonovan]

Here's a recap of the behavior of the relevant operations wrt nil:

• []T(nil) is nil
• T{} is non-nil
• x[:i:i] has the nilness of x.
• append(x, y...) preserves the nilness of x if the result is empty.
• slices.Clip(x) preserves the nilness of x (undocumented).
• slices.Clone(s) preserves the nilness of x (undocumented).
• slices.Concat(slices...) is nil if the result is empty (undocumented).

The inconsistency of Concat is frustrating, but perhaps too late to change.

Given that the nilness behavior of the slices package is undocumented, I wonder whether this modernizer's fixes can ever be safely offered, since they could change in future (though perhaps any such change to the slices package would be rejected as too risky). Assuming the current semantics of the slices package are here to stay, we should not offer the slices.Concat fix if we cannot prove its result is non-empty.

[gopherbot]

Change https://go.dev/cl/678135 mentions this issue: gopls/internal/analysis/modernize: add TODOs for nilness problem