Skip to content
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.

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

x/tools/gopls: modernize x/exp/maps.Clear for Go >= 1.21 #70717

Closed
BieHDC opened this issue Dec 6, 2024 · 11 comments
Closed

x/tools/gopls: modernize x/exp/maps.Clear for Go >= 1.21 #70717

BieHDC opened this issue Dec 6, 2024 · 11 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@BieHDC
Copy link

BieHDC commented Dec 6, 2024

gopls version

Build info

golang.org/x/tools/gopls v0.16.2
golang.org/x/tools/gopls@v0.16.2 h1:K1z03MlikHfaMTtG01cUeL5FAOTJnITuNe0TWOcg8tM=
github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak=
github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y=
golang.org/x/mod@v0.20.0 h1:utOm6MM3R3dnawAiJgn0y+xvuYRsm1RKM/4giyfDgV0=
golang.org/x/sync@v0.8.0 h1:3NFvSEYkUoMifnESzZl15y791HH1qU2xm6eCJU5ZPXQ=
golang.org/x/telemetry@v0.0.0-20240829154258-f29ab539cc98 h1:Wm3cG5X6sZ0RSVRc/H1/sciC4AT6HAKgLCSH2lbpR/c=
golang.org/x/text@v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
golang.org/x/tools@v0.22.1-0.20240829175637-39126e24d653 h1:6bJEg2w2kUHWlfdJaESYsmNfI1LKAZQi6zCa7LUn7eI=
golang.org/x/vuln@v1.0.4 h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I=
honnef.co/go/tools@v0.4.7 h1:9MDAWxMoSnB6QoSqiVr7P5mtkT9pOc1kSxchzPCnqJs=
mvdan.cc/gofumpt@v0.6.0 h1:G3QvahNDmpD+Aek/bNOLrFR2XC6ZAdo62dZu65gmwGo=
mvdan.cc/xurls/v2@v2.5.0 h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.23.4

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/REDACTED/.cache/go-build'
GOENV='/home/REDACTED/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/REDACTED/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/REDACTED/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.4'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/REDACTED/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build637431612=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I was using maps.Clear() in my code where the go version in the go.mod file is >1.21. I later figured out by coincidence that since Go 1.21 the built in clear() method does that task.

What did you see happen?

Nothing.

What did you expect to see?

gopls suggesting to replace the call to maps.Clear() with the built in clear() method when the go.mod go version is >= 1.21.

Editor and settings

Kate

Logs

No response

@BieHDC BieHDC added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Dec 6, 2024
@gopherbot gopherbot added this to the Unreleased milestone Dec 6, 2024
@findleyr
Copy link
Member

findleyr commented Dec 6, 2024

You're using x/exp/maps? The standard library maps package does not have Clear.

We'd like to support this type of modernizer, but probably wouldn't add one for a package in x/exp.

@BieHDC
Copy link
Author

BieHDC commented Dec 7, 2024

You're using x/exp/maps? The standard library maps package does not have Clear.

Yes. I searxed the internet when looking for this functionality and it said "maps.Clear", i typed this in my editor and the auto import organisation put up that package.

@findleyr
Copy link
Member

findleyr commented Dec 9, 2024

Thanks @BieHDC for describing your use-case. I think there are some actionable improvements we can make here:

  1. Avoid deprecated symbols and/or packages in import resolution.
  2. Deprecate x/exp/maps.

Looking at x/exp/maps, which has over 5000 importers, maybe we should use this opportunity to offer API modernizers to transition users to the standard library maps package.

The way I would imagine this working is as follows: define API wrappers for every x/exp/maps function, which delegate to either the standard library package or clear builtin, and then offer quick fixes, both over the x/exp import and over each call site, to modernize the API usage via inlining the synthetic wrappers.

Unlike normal inlining, which is in the refactoring menu, this would be in the quickfix menu, which would be more visible.

CC @adonovan

@findleyr findleyr modified the milestones: Unreleased, gopls/backlog Dec 9, 2024
@findleyr findleyr added the Refactoring Issues related to refactoring tools label Dec 9, 2024
@findleyr findleyr added the FeatureRequest Issues asking for a new feature that does not need a proposal. label Dec 9, 2024
@findleyr findleyr changed the title x/tools/gopls: flag maps.Clear for Go >= 1.21 x/tools/gopls: modernize x/exp/maps.Clear for Go >= 1.21 Dec 9, 2024
@adonovan
Copy link
Member

BTW: maps.Clear is generic, and our inliner still can't handle generic functions yet. (It shouldn't be a huge task, but @findleyr tackled it recently and discovered that it was trickier than it looked.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/634919 mentions this issue: internal/refactor/inline/analyzer: use "//go:fix inline" annotation

@findleyr
Copy link
Member

BTW: maps.Clear is generic, and our inliner still can't handle generic functions yet. (It shouldn't be a huge task, but @findleyr tackled it recently and discovered that it was trickier than it looked.)

It wasn't that much trickier! The tricky part was just writing out type expressions for caller types that may not have previously been explicitly imported. I will get it working soon...

gopherbot pushed a commit to golang/tools that referenced this issue Dec 11, 2024
This CL changes the proof-of-concept "inliner" analyzer to
use "//go:fix inline" as the marker of a function that should
be inlined. This seems to be the syntax emerging from the
proposal golang/go#32816.

Also, skip inlinings that cause literalization of the callee.
Users of batch tools only want to see reductions.

Updates golang/go#32816
Updates golang/go#70717

Change-Id: I6d70118f69b7c35f51e1d51863a0312b5dcc3b63
Reviewed-on: https://go-review.googlesource.com/c/tools/+/634919
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@adonovan
Copy link
Member

adonovan commented Dec 12, 2024

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/635678 mentions this issue: gopls/internal/analysis/modernize: maps.Clear -> clear(m)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/635680 mentions this issue: slices,maps: delegate to go1.21 functions of the same name

@adonovan
Copy link
Member

I've closed this issue because https://go.dev/cl/635680 reduces maps.Clear to the same status as all the other exp/maps functions: wrappers around the std maps package. In the near future, if proposal #32816 is accepted, we can simply annotate such functions as candidates for auto-inlining and let the tools take care of it. This work is tracked as part of #70815.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. gopls Issues related to the Go language server, gopls. Refactoring Issues related to refactoring tools Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants