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: spurious "would shadow" error when renaming parameter #57479

Open
adonovan opened this issue Dec 27, 2022 · 3 comments
Open

x/tools/gopls: spurious "would shadow" error when renaming parameter #57479

adonovan opened this issue Dec 27, 2022 · 3 comments
Labels
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

@adonovan
Copy link
Member

adonovan commented Dec 27, 2022

When renaming a parameter using gopls's rename command, and the new name is identical to the name of an imported package referenced by one of the function parameters, gopls reports an error, even though the renaming might be sound. For example:

import "fmt"

func _(x fmt.Stringer) {} // rename x to "fmt" gives renaming this var "x" to "fmt" would shadow this reference to the imported package name declared here

(Unfortunately the pronouns in the error message don't actually link to source locations, at least not when I use Emacs+eglot.)

The same thing happens when the package is referenced by a later parameter (e.g. func(x int, y fmt.Stringer)) or by a named constant in an array type (e.g. func(x [math.MaxInt]bool)).

IIRC the gorename tool got this right.

@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 Dec 27, 2022
@gopherbot gopherbot added this to the Unreleased milestone Dec 27, 2022
@suzmue suzmue modified the milestones: Unreleased, gopls/later Dec 28, 2022
@suzmue suzmue added the Refactoring Issues related to refactoring tools label Dec 28, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/590976 mentions this issue: gopls/internal/test: add test case for parameter rename match import

@suzmue
Copy link
Contributor

suzmue commented Jun 5, 2024

Using a go version >= go1.22, fixes this issue.

I was able to investigate how this upgrade was able to fix this, and it appears the scopePos for the result object in the block.LookupParent call is NoPos in go < 1.22, and has a valid pos starting in go1.22.

Once new versions of gopls require >= go1.22 (see https://go.dev/issue/65917), this issue will be resolved.

@adonovan
Copy link
Member Author

adonovan commented Jun 6, 2024

Ah, that suggests this was a consequence of the scope bug #64292, fixed in go1.22 by https://go.dev/cl/544035. Thanks for investigating!

gopherbot pushed a commit to golang/tools that referenced this issue Jun 6, 2024
This adds a test case to verify that valid renamings can occur, when
the new name of a parameter is identical to an imported package, even
when referenced by a function paramater.

The bug where the rename was failing is fixed when using go1.22. The
new go version fills in a missing Pos for an object scope that allows
LookupPos to correctly determine if the scope is relevant.

For golang/go#57479

Change-Id: Ifebafb78483f174eac94cd1ec5ac68db9e88684f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/590976
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants