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: renaming mishandles embedded fields #43616

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

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

mdempsky opened this issue Jan 10, 2021 · 7 comments
Assignees
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jan 10, 2021

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

@gopherbot gopherbot added this to the Unreleased milestone Jan 10, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 10, 2021

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

@findleyr findleyr commented Jan 11, 2021

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

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jan 12, 2021

@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

@findleyr findleyr commented Jan 12, 2021

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

@findleyr findleyr commented Jan 12, 2021

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

@mdempsky mdempsky commented Jan 12, 2021

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

@gopherbot gopherbot commented Jan 15, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.