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.

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

x/tools/gopls: support renaming an embedded field at its declaration #45199

Open
myitcv opened this issue Mar 24, 2021 · 2 comments
Open

x/tools/gopls: support renaming an embedded field at its declaration #45199

myitcv opened this issue Mar 24, 2021 · 2 comments
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/unplanned

Comments

@myitcv
Copy link
Member

myitcv commented Mar 24, 2021

What version of Go are you using (go version)?

$ go version
go version devel +f4b918384d Tue Mar 23 05:12:39 2021 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.1.1-0.20210319205745-d7a25adab0d9
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20210319205745-d7a25adab0d9

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel +f4b918384d Tue Mar 23 05:12:39 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1525600331=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Follow up from #43616 (comment) (cc @mdempsky)

Relatively frequently I find myself in the following situation:

type T int

type S struct {
    T
}

With the cursor on the embedded T in S.

I then initiate a rename, but get:

gopls.Rename failed: can't rename embedded fields: rename the type directly or name the field

The fix in this instance is to jump to the definition of T, rename, then jump back. But obviously gopls can do that 😄

To my mind this case is distinct from the case of selector expressions mentioned in #43616 (comment): such a situation is more ambiguous because we can't be sure the user knows the field in question is an embedded field.

But in this case our cursor is on the declaration of the embedded field, so there can be no ambiguity.

cc @findleyr

FYI @leitzler
@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Mar 24, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 24, 2021
@gopherbot gopherbot added this to the Unreleased milestone Mar 24, 2021
@danishprakash
Copy link

If I'm hovering over the field selector x.T, and rename T, it is surprising that this would end up renaming a type.

^from the CL associated with #43616.

I think I agree with that statement but even I find myself doing this quite often and would love for :GoRename to support this. Could there be a middle ground? for instance, only allowing rename if the field(type) is part of the same package and throw an error otherwise?

@stamblerre stamblerre changed the title x/tools/gopls: support renaming an embedded field x/tools/gopls: support renaming an embedded field at its declaration Jun 4, 2021
@adonovan
Copy link
Member

for instance, only allowing rename if the field(type) is part of the same package and throw an error otherwise?

That seems a little arbitrary.

I think the feature seems reasonable as requested. It's analogous to the way the tool handles renaming of methods: if you rename a concrete method so that it would no longer satisfy an interface, you can an error; but if you rename the interface method, this is taken as intent to change all implementations too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/unplanned
Development

No branches or pull requests
5 participants
@myitcv @adonovan @stamblerre @gopherbot @danishprakash