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/cmd/gorename: doesn't work outside $GOPATH #27571

Closed
jclc opened this issue Sep 8, 2018 · 34 comments
Closed

x/tools/cmd/gorename: doesn't work outside $GOPATH #27571

jclc opened this issue Sep 8, 2018 · 34 comments
Assignees
Milestone

Comments

@jclc
Copy link

@jclc jclc commented Sep 8, 2018

What version of Go are you using (go version)?

go1.11

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

Tried running gorename in a project outside of $GOPATH, using vscode-go and also manually. Both times the program errors with:

$ gorename -from ./world.go::World -to "Warudo"
gorename: can't find package containing /home/vhns/projects/phys/world.go

Commands work as expected when run on the same project inside the $GOPATH.

What did you expect to see?

The symbol being renamed.

What did you see instead?

An error message claiming the package could not be found.

@gopherbot gopherbot added this to the Unreleased milestone Sep 8, 2018
@ianthehat ianthehat self-assigned this Sep 9, 2018
@ianthehat ianthehat added the modules label Sep 9, 2018
@myitcv
Copy link
Member

@myitcv myitcv commented Sep 9, 2018

Just noting for those following along that @ianthehat has linked this issue to the tracking umbrella issue of #24661

@ysmolsky
Copy link
Member

@ysmolsky ysmolsky commented Nov 21, 2018

One more datapoint:
I use emacs and the go-rename package, that uses the gorename tool.

Would love to see this fixed, almost to the point of trying it myself. But I have so little time in next month...

@anjmao
Copy link

@anjmao anjmao commented Nov 22, 2018

@myitcv @ianthehat Hi, could you share what is the status if this issue? Do you need any help on solving it?

@myitcv
Copy link
Member

@myitcv myitcv commented Dec 4, 2018

@anjmao - per the last golang-tools call, nobody is currently working on this. I suspect @ianthehat and team are open to contributions. Might just be worth fleshing out the approach/details here or in a thread on https://groups.google.com/forum/#!forum/golang-tools first?

@anjmao
Copy link

@anjmao anjmao commented Dec 6, 2018

I like how bingo is doing rename https://github.com/saibing/bingo/blob/master/langserver/rename.go

  1. Find references.
  2. Update with new changes.

Probably it's better to just focus on golsp since gorename need's to be rewritten completely anyway.

@keegancsmith
Copy link
Contributor

@keegancsmith keegancsmith commented Dec 6, 2018

I like how bingo is doing rename https://github.com/saibing/bingo/blob/master/langserver/rename.go

Note: This is just copied from go-langserver, which has existing performance issues. https://github.com/sourcegraph/go-langserver/blob/master/langserver/rename.go
So yes golsp will be exciting to use when it is ready.

Edit: I see bingo uses a better caching strategy for typechecked dependencies. So even though the rename implementation was copied, the fact that typechecking is better means it should be better than go-langserver.

@typingduck
Copy link

@typingduck typingduck commented Dec 6, 2018

@ysmolsky Do you know if anyone is working on this? I never contributed to golang tools before but with a pointer in the rought direction to start I can probably do it (I also noticed there's a buck of TODOs in the main file).

@myitcv
Copy link
Member

@myitcv myitcv commented Dec 6, 2018

@typingduck - please see my comment in #27571 (comment). That linked comment was a reference to this comment from @ianthehat on the last golang-tools call.

@typingduck
Copy link

@typingduck typingduck commented Dec 6, 2018

Thanks @myitcv.

I just wanted to double check before starting that nobody else is working on this. It sounds like that's a 'no' (having no prior experience on go tools I'm sure it will take me a few days longer to get up to speed than someone with experience).

I will start looking into it, but might wait until the next tools call before starting in earnest just to make sure I'm not stepping on someone else's existing plans.

@kerak19
Copy link

@kerak19 kerak19 commented Feb 7, 2019

Is there any update on this issue?

@adg
Copy link
Contributor

@adg adg commented Feb 28, 2019

Is there a plan for how this might be done? I haven't seen any discussion of it. If @typingduck has lost enthusiasm, I am happy to dig into this.

@velovix
Copy link

@velovix velovix commented Feb 28, 2019

My understanding from listening in on the golang-tools catch-ups is that gorename has some fundamental issues preventing it from working in a module world, and that it would need a re-write. I also may have misunderstood. @ianthehat has discussed this in the past, maybe he has some insights as to how a future gorename might work.

@tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Feb 28, 2019

Sorry for that naive question, but with the LSP support coming with gopls, is it still relevant to maintain isolate tools like gorename ?

@myitcv
Copy link
Member

@myitcv myitcv commented Feb 28, 2019

Ignoring for one second how gorename is currently implemented, I jotted down a few points a while back on how I would (naively) go about this:

  • all renames need to take place in the context of the main module
  • if an identifier is exported, a cheap reverse dependency graph can be calculated by using go/packages.Load with a LoadMode of LoadFiles. This will also give the relevant details about where .GoFiles are located
  • the case for a non-exported identifier is simpler
  • renames can only take place in modules that are writable. In a cmd/go world, gorename should fail if you attempt to rename an exported identifier declared by a package that is contained within a module that exists in the module cache. (If the user does attempt such a rename they could/should be encouraged to use gohack to work with a writable copy of that module). I'm not sure how this maps to Blaze etc.
  • therefore, determining whether a module is writable or not is probably build system-specific. For cmd/go it should be done by checking whether the package's .GoFiles exist in a directory (not forgetting all module/package details are resolved from the main module) that is contained within any directory that is part of go env GOPATH

Perhaps @ianthehat could add a few notes here on the bits I've likely missed :)

@adg
Copy link
Contributor

@adg adg commented Feb 28, 2019

In the near term I'd be happy with a gorename that only works inside a given module. This seems like a reasonable goal given that the module system doesn't provide any mechanism to know who the users of a module are.

Can anyone working on gopls say how they see that fitting into the gorename story?

@myitcv
Copy link
Member

@myitcv myitcv commented Mar 1, 2019

@adg:

In the near term I'd be happy with a gorename that only works inside a given module. This seems like a reasonable goal given that the module system doesn't provide any mechanism to know who the users of a module are.

Agreed.

The scenario that I find myself in often enough, and which formed the basis for those notes, is:

  • I'm working on module M1 (i.e. this is the main module)
  • M1 (transitively) requires M2
  • I want to, within the scope of my work on M1, rename an exported member of package P2 in M2

i.e. the scope of the identifier rename in M2 would be limited to M1's module requirement graph. It may be that the identifier is used by packages in modules other than M1 in M1's requirement graph, for example. Again, that seems like a sensible initial restriction.

But starting with renames of identifiers (exported or otherwise) within the main module (M1 in this example) is also a good starting point.

We should probably not try to solve for the case of cyclic module requirements for now either.

@stamblerre stamblerre added the gopls label Mar 12, 2019
@nf
Copy link

@nf nf commented Mar 27, 2019

@stamblerre can you tell me whether gopls will cover the functionality of gorename? Is it worth working on gorename?

@ianthehat
Copy link

@ianthehat ianthehat commented Mar 27, 2019

gopls will definitely grow these kinds of features, and if you want to try contributing it would be appreciated.
The right first step is to implement find references, and then use that functionality to support renames.
We have no plans at this point to fix the gorename tool itself, we think that focusing our efforts on improving gopls produces a better overall experience for the community, especially as we start exposing all of its functionality on the command line interface.

@douglarek
Copy link

@douglarek douglarek commented Mar 28, 2019

Excuse me for my abrupt question, when will gopls have a fully functional(able to replace gocode and others) and available version ?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 28, 2019

@douglarek: gopls is pretty close to being able to replace gocode, but we don't have any formalized timeline. People are using gopls now, but it still doesn't have feature parity with available tools / language servers. We will be sure to send out an update when we are confident that gopls is fully ready.

@newtorn

This comment was marked as off-topic.

@thepudds
Copy link

@thepudds thepudds commented Jun 20, 2019

Related CL https://golang.org/cl/182585 for gopls was recently merged.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 28, 2019

Closing this now as gopls now supports rename. If anyone has issues with gopls renaming, please feel free to open new issues against gopls.

@stamblerre stamblerre closed this Jun 28, 2019
@kostix
Copy link

@kostix kostix commented Jun 28, 2019

How would I use that gopls thing for renaming (provided I've built the tip of the master branch)?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 28, 2019

Have you configured gopls to work with your editor of choice, as described here (https://github.com/golang/go/wiki/gopls)?

@apmckinlay
Copy link

@apmckinlay apmckinlay commented Jun 28, 2019

It's great that gopls is moving forward. I think it's the right direction. But be aware that it may not be very stable. I find it crashes or hangs or quits working for various reasons, YMMV. It was bad enough that I was forced to switch back to the old tools.

@hellt
Copy link

@hellt hellt commented Jun 28, 2019

@stamblerre I did; updated the gopls to a version listed in the wiki page you referred

~/go/bin/gopls version
golang.org/x/tools/gopls v0.1.0
    golang.org/x/tools/gopls@v0.1.0 h1:e5o2xK2HU//kzIRypLBw6/8pXdWuYDd8pliYpnQuNw8=

made the changes to the settings.json of the VSCode

    "go.useLanguageServer": true,
    "[go]": {
        "editor.snippetSuggestions": "none",
        "editor.formatOnSave": true,
        "editor.codeActionsOnSave": {
            "source.organizeImports": true
        }
    }

but renaming still does not work:

Starting linting the current package at /Users/romandodin/Dropbox/projects/vspk-go-quickstart
/Users/romandodin/Dropbox/projects/vspk-go-quickstart>Finished running tool: /Users/romandodin/go/bin/golint

Rename failed: gorename: can't find package containing /Users/romandodin/Dropbox/projects/vspk-go-quickstart/filter.go 
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 28, 2019

@apmckinlay: There has been some instability with gopls recently, but we just tagged a new version (go get golang.org/x/tools/gopls@latest) which should be a bit more stable.

@hellt: It looks VSCode is still trying to run gorename. Are you on the most recent version of the VSCode extension? Rename should happen through gopls by default, but if not you can add

"go.languageServerExperimentalFeatures": {
    "rename": true
}

to your settings.json. Also, it may be worth updating to master for now (go get -u golang.org/x/tools/gopls@master) as we just tagged a new release earlier today, and it looks like you haven't picked that up yet. Also, remember, you have to reload your VSCode window after updating your settings.json so that it restarts gopls.

@bdimcheff
Copy link

@bdimcheff bdimcheff commented Jun 28, 2019

As of a couple days ago, gopls was only renaming within a single package (versus in the entire workspace). As a consequence, I had to turn rename to false in vscode so that it'd fall back to gorename. I'm not sure if this is an expected limitation of gopls right now, so ymmv.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jun 28, 2019

The issue here is that gorename does not work at all outside of $GOPATH, so when it comes to renaming for users in module mode, gopls is strictly an improvement.

@bdimcheff
Copy link

@bdimcheff bdimcheff commented Jun 28, 2019

yeah that's fair - I still develop my module-enabled project in gopath mode, but with gopls on for some features. gopls is soooo much faster and I'm really excited about where it's headed, but I frequently have to revert to older tooling for certain features that haven't made it to gopls yet.

@kostix
Copy link

@kostix kostix commented Jun 29, 2019

Have you configured gopls to work with your editor of choice, as described here (https://github.com/golang/go/wiki/gopls)?

Oh! It's the language server; too bad :-(

Do I understand correctly that there will no more be a standalone tool to carry out project-wide renames? I mean, callable from the command-line, without the need to set a separate service up beforehand.

@hellt
Copy link

@hellt hellt commented Jun 29, 2019

@stamblerre this helped immediately

Also, it may be worth updating to master for now (go get -u golang.org/x/tools/gopls@master

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 1, 2019

@kostix: We intend to support using gopls on the command line. You can follow along here: #32875.

pjvds added a commit to pjvds/SpaceVim that referenced this issue Mar 18, 2020
Vim-go uses the deprecated gorename command that doesn't support Go modules. The latest release go gopls support renaming and is the new way forward.

See: 
* golang/go#27571
* fatih/vim-go#2366
wsdjeg pushed a commit to SpaceVim/SpaceVim that referenced this issue Mar 23, 2020
Vim-go uses the deprecated gorename command that doesn't support Go modules. The latest release go gopls support renaming and is the new way forward.

See: 
* golang/go#27571
* fatih/vim-go#2366
@golang golang locked and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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