godoctor / godoctor

Go Doctor - The Golang Refactoring Engine
BSD 3-Clause "New" or "Revised" License
445 stars 31 forks source link

Does it work with go modules? #50

Closed ghost closed 2 years ago

ghost commented 4 years ago

I've set up a project with go modules (go 1.11 with GO11MODULES=on) and I can't get godoctor working there. When I let godoctor run it chokes on the modules, like:

Defaulting to package scope github.com/blah/blah/cmd for refactoring (provide an explicit scope to change this)
filter.go:6:2: Error: could not import github.com/cnkei/gospline (cannot find package "github.com/cnkei/gospline" in any of:
    /usr/lib/go-1.11/src/github.com/cnkei/gospline (from $GOROOT)
    /home/gru/go/src/github.com/cnkei/gospline (from $GOPATH))

Is there a way to make it work?

rdallman commented 4 years ago

Hi @Osram-Gru - it does seem to work with go modules for me, however, I think this may be the case only after the modules are vendored (in order for us to be able to load them). double checking that GO111MODULES=on and not GO11MODULES=on, which is possibly an issue (probably just a type, making sure there's 3 1's). you may try to run go mod vendor in the package you are attempting to refactor before running the refactoring, that may fix things.

additional info would be that I have GOFLAGS="-mod=vendor" set as well, and if it helps, I'm running the refactoring against the ctx variable here.

could you give some of that a whirl and report back? if you can't rename the same variable as I did then I expect it's something with the environment that's off, and it would be useful to have some more information. hopefully we can figure this out and update the docs if there's additional things we need to do to get it working. thanks for filing the bug!

KScaesar commented 4 years ago

I have same problem. Try to use go1.11.13 and go1.13.3 , both these version don't work refactor. My enviroment is GO11MODULES=on and no vendor directory.

Defaulting to file scope d:\gameserver\src\GameServer\QueueProcess.go for refactoring (provide an explicit scope to change this)
QueueProcess.go:4:2: Error: could not import CommonLib/src/Base (cannot find package "CommonLib/src/Base" in any of:
    c:\go\src\CommonLib\src\Base (from $GOROOT)
    D:\go\src\CommonLib\src\Base (from $GOPATH))
QueueProcess.go:5:2: Error: could not import CommonLib/src/LibBetCluster (cannot find package "CommonLib/src/LibBetCluster" in any of:
    c:\go\src\CommonLib\src\LibBetCluster (from $GOROOT)
    D:\go\src\CommonLib\src\LibBetCluster (from $GOPATH))
QueueProcess.go:6:2: Error: could not import CommonLib/src/LibGame/LibFish (cannot find package "CommonLib/src/LibGame/LibFish" in any of:
    c:\go\src\CommonLib\src\LibGame\LibFish (from $GOROOT)
    D:\go\src\CommonLib\src\LibGame\LibFish (from $GOPATH))
QueueProcess.go:7:2: Error: could not import CommonLib/src/LibGame/LibSlot (cannot find package "CommonLib/src/LibGame/LibSlot" in any of:
    c:\go\src\CommonLib\src\LibGame\LibSlot (from $GOROOT)
    D:\go\src\CommonLib\src\LibGame\LibSlot (from $GOPATH))
QueueProcess.go:8:2: Error: could not import CommonLib/src/LibGameInfo (cannot find package "CommonLib/src/LibGameInfo" in any of:
    c:\go\src\CommonLib\src\LibGameInfo (from $GOROOT)
    D:\go\src\CommonLib\src\LibGameInfo (from $GOPATH))
QueueProcess.go:822:4: Error: undeclared name: QueueInsertPerSecond
QueueProcess.go:503:3: Error: undeclared name: QueueInsertDurationTime
QueueProcess.go:512:5: Error: undeclared name: MUTEX_QUEUE1
QueueProcess.go:521:22: Error: undeclared name: ServerConfig
ghost commented 4 years ago

Thanks for your hints! I did like you told me and it's working again:

GOFLAGS="-mod=vendor" go mod vendor

That did the trick. This is only a workaround, it seems. "Native" support for modules without the ugly vendor branches would be much nicer... Unfortunately, I can't volunteer for that task :(

(Oh, and GO11MODULES was just a typo. A "Not-Copy-Paste"-Error)

rdallman commented 4 years ago

thanks both of you for updates. my immediate thoughts:

hmm, this makes me wander what to do here, I see a couple options I guess. we could not load dependencies and block refactorings that affect downstream dependencies, overall this has some benefits, namely speed, but also preventing what is likely to be spurious behavior (eg a user probably didn't mean to try to rename a dependency's exported variable). this would resolve the issue of needing to vendor dependencies (the old way) or have them in the $GOPATH (the old old way) in order for godoctor to work.

the possibly easier thing to do is try to dig around the file system to load these, we may just need to update the version of the go loader that we use (did not look, but will later). at some level the files have to be present locally, at least in order to build, and since godoctor itself doesn't run the go command, go mod won't run and do it's magic in the background, putting us in a bit of a pickle. basically, we can only run godoctor on go programs that are 'buildable' in the sense that they already have the dependencies locally somewhere -- that is: not necessarily vendored, but at least in go mod's cache so that go build would work without needing to pull them. it seems unwise to add a go command to godoctor in order to have the dependency fetching magic, it seems kind of like a violation of responsibilities on godoctors part. as stated though, we do need to figure out how to make the loader work with mod (possibly just by updating the loader) unless we want to require dependencies to be vendored though.

will marinate on this, feedback about real user expectations would of course help very much in deciding how to proceed!

rdallman commented 4 years ago

good news: seems like the go loader has been updated with modules support and got a bit of a face lift: https://godoc.org/golang.org/x/tools/go/packages#NeedImports - I'll see what I can manage as far as retrofitting in the near future. it seems like we can go for #1 above, not load the modules at all, which should provide a nice speed up as well as fixing the user xp issue - need to dig in possibly to figure out if we need to / can we / should we log a warning when a user attempts to refactor something that effects dependencies (feedback welcome).

ghost commented 4 years ago

That's good news indeed :) I would vote for a warning if one refactors a dependency. Or even disallow it...

nezorflame commented 4 years ago

This doesn't work for me on a simple test project with any command (using GO111MODULE=on and with/without GOFLAGS="-mod=vendor"):

изображение

go.mod:

module example

go 1.13