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: 'rename' in noncanonical std module causes breakage #57965

Open
adonovan opened this issue Jan 23, 2023 · 2 comments
Open

x/tools/gopls: 'rename' in noncanonical std module causes breakage #57965

adonovan opened this issue Jan 23, 2023 · 2 comments
Labels
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/backlog

Comments

@adonovan
Copy link
Member

adonovan commented Jan 23, 2023 •
edited

@dr2chase reports that, while working with two clones of the goroot repo, initiating a rename in a clone that does not hold the std module used by the go command on gopls's PATH causes gopls to fail to accurately discover the set of references that need updating, and thus render the package non-compiling. It does print a bunch of warnings (to a log that the user can't see).

$ goroot1=$HOME/goroot
$ goroot2=$HOME/goroot2
$ cd $goroot1
$ git log -1 | grep ^commit
commit 9411075748e305d0b31372cab48bc4ca2dd745fb
$ gopls version | head -n 1
golang.org/x/tools/gopls v0.11.0
$ $goroot1/bin/go version
go version devel go1.21-9411075748 Mon Jan 23 18:00:36 2023 +0000 darwin/arm64

$ PATH=$goroot1/bin:$PATH gopls references $goroot1/src/fmt/scan.go:#5137  # fmt.Scanln
goroot1/src/encoding/gob/example_encdec_test.go:35:16-23
goroot1/src/fmt/example_test.go:52:17-24
goroot1/src/fmt/scan.go:106:9-16
goroot1/src/fmt/scan.go:70:9-16
goroot1/src/fmt/scan_test.go:543:21-28
goroot1/src/fmt/scan_test.go:762:12-19
goroot1/src/fmt/scan_test.go:789:12-19
goroot1/src/fmt/scan_test.go:800:11-18

Now let's change the go command (and thus its notion of the std module) to goroot2 but continue to work on files in goroot1. Observe that only intra-package references are reported, along with a slew of warnings. Nonetheless it's a success:

$ PATH=$goroot2/bin:$PATH gopls references $goroot1/src/fmt/scan.go:#5137
2023/01/23 13:20:32 Error:2023/01/23 13:20:32 tidy: diagnosing file:///Users/adonovan/w/goroot/src/cmd/go.mod: err: exit status 1: stderr: cmd/addr2line: ambiguous import: found package cmd/addr2line in multiple modules:
        cmd (/Users/adonovan/w/goroot/src/cmd/addr2line)
         (/Users/adonovan/w/goroot2/src/cmd/addr2line)
cmd/api: ambiguous import: found package cmd/api in multiple modules:
...ad nauseam...
$goroot1/src/fmt/scan.go:106:9-16
$goroot1/src/fmt/scan.go:70:9-16
$ echo $?
0

Now we attempt the same thing with a rename (fmt.Scanln -> fmt.SCANLN). Operations on files of the std module associated with the go command on the PATH work just fine:

$ PATH=$goroot1/bin:$PATH gopls rename -w $goroot1/src/fmt/scan.go:#5137 SCANLN
/Users/adonovan/w/goroot/src/encoding/gob/example_encdec_test.go
/Users/adonovan/w/goroot/src/fmt/example_test.go
/Users/adonovan/w/goroot/src/fmt/scan.go
/Users/adonovan/w/goroot/src/fmt/scan_test.go
$ echo $?
0

But when we rename files in a copy of the std module different from that used by the go command on the path, only intra-package references are renamed:

$ PATH=$goroot2/bin:$PATH gopls rename -w $goroot1/src/fmt/scan.go:#5137 SCANNL
... lots of warnings ...
/Users/adonovan/w/goroot/src/fmt/scan.go
$ echo $?
0

Thus leaving the rest of that std module in a broken state. Still the operation is a success.

gopls should try harder to avoid beginning a rename that might mess things up, possibly by promoting those warnings into errors.
@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 Jan 23, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jan 23, 2023
@bcmills
Copy link
Contributor

bcmills commented Jan 25, 2023 •
edited

when we rename files in a copy of the std module different from that used by the go command on the path, only intra-package references are renamed

That makes intuitive sense to me. Outside of GOROOT/src, a module named std is just that, not part of the standard library (see #30756), and the fmt package within that module has the import path std/fmt, not fmt (so there really aren't any external references to it.

In general, for things to work within a GOROOT/src tree it needs to match go env GOROOT, either by using the toolchain from that GOROOT/bin or by setting GOROOT explicitly.

@findleyr
Copy link
Contributor

I think we can leave this open to improve the UX.

@findleyr findleyr modified the milestones: Unreleased, gopls/later Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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/backlog
Development

No branches or pull requests
4 participants
@bcmills @adonovan @gopherbot @findleyr