8000 x/tools/gopls/internal/analysis/modernize: append([]T{}, x...) -> slices.Clone(x) fix may unsoundly return nil · Issue #73557 · golang/go · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
aronatkins opened this issue Apr 30, 2025 · 3 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.19.0

Comments

@aronatkins
Copy link

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.
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Apr 30, 2025
@gopherbot gopherbot added this to the Unreleased milestone Apr 30, 2025
@adonovan adonovan changed the title x/tools/gopls: modernizer slices.Clone replacement results in nil slice x/tools/gopls/internal/analysis/modernize: append([]T{}, x...) -> slices.Clone(x) fix may unsoundly return nil Apr 30, 2025
@gabyhelp
Copy link

Related Issues

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

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Apr 30, 2025
@gopherbot
Copy link
Contributor

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

@adonovan adonovan modified the milestones: Unreleased, gopls/v0.19.0 May 5, 2025
@adonovan
Copy link
Member
adonovan commented May 5, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BugReport Issues describing a possible bug in the Go implementation. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.19.0
Development

No branches or pull requests
4 participants
@aronatkins @adonovan @gopherbot @gabyhelp
0