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

Jump to bottom

x/tools/gopls: renaming mishandles embedded fields #43616

Closed
mdempsky opened this issue Jan 10, 2021 · 7 comments
Closed

x/tools/gopls: renaming mishandles embedded fields #43616

mdempsky opened this issue Jan 10, 2021 · 7 comments
Labels
FrozenDueToAge 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.6.5

Comments

@mdempsky
Copy link
Member

I'd expect that if I try to use gopls to rename any of the foo identifiers to bar in the package below, then all 3 should change. However, gopls only updates 2 of them, resulting in an invalid source file.

package p

type foo int

var x struct { foo }

var _ = x.foo

I'm using gopls v0.6.2.

/cc @stamblerre @findleyr
@mdempsky mdempsky 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 Jan 10, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jan 10, 2021
@gopherbot
Copy link

Change https://golang.org/cl/282932 mentions this issue: internal/lsp/source: rename uses of embedded fields

@findleyr findleyr self-assigned this Jan 11, 2021
@findleyr
Copy link
Contributor

Thanks for reporting. It looks like we just missed this case.

@mdempsky
Copy link
Member Author

@findleyr Thanks for the quick fix. QQ: In the test case, it looks like it's testing renaming at the type declaration. Will the same fix also work for renaming at selector expressions (e.g., L7)?

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.6.3 Jan 12, 2021
@findleyr
Copy link
Contributor

Err, no, that doesn't work. My bad for missing that.

We need to update the corresponding logic to find the object being renamed.

@findleyr findleyr reopened this Jan 12, 2021
@findleyr
Copy link
Contributor

Upon a bit more consideration, I'm not sure this feature is perfectly symmetric. When renaming the type name it makes sense that we should also update uses of embeddings of that type. But it's not so clear that the reverse should hold. If I am positioned on x.foo and perform the renaming, it could be surprising that this results in the foo type being renamed.

Perhaps we should disallow renaming embedded fields: the user should either rename the type OR give the field a name...

@mdempsky
Copy link
Member Author

Yeah, I agree reporting an error instead of also renaming the type makes sense.

@stamblerre stamblerre modified the milestones: gopls/v0.6.3, gopls/v1.0.0 Jan 14, 2021
@gopherbot
Copy link

Change https://golang.org/cl/284312 mentions this issue: internal/lsp/source: make it an error to rename embedded fields

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.6.5 Jan 26, 2021
@myitcv myitcv mentioned this issue Mar 24, 2021
Open
@golang golang locked and limited conversation to collaborators Jan 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge 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.6.5
Development

No branches or pull requests
4 participants
@mdempsky @stamblerre @gopherbot @findleyr