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/{gomvpkg, gorename}: change scope is too large #10917

Open
rogpeppe opened this issue May 20, 2015 · 20 comments

Comments

@rogpeppe
Copy link
Contributor

commented May 20, 2015

These commands go through the entire GOPATH changing anything
that refers to the moved package or renamed symbol.
I see that this would work well in the Google "one big repository" world,
but is never what I want, as the GOPATH is filled with thousands
of packages in different repositories which I do not necessarily have commit
rights to.

Moreover, it's not really sufficient, as I might not have
all the packages that use the renamed symbol in my import path.

I'd find gorename much more useful if it could rename within one
repository only (possibly with an flag to cause it to error if anything
in $GOPATH refers to the renamed symbol), and there was
a way to update a package that imports another package that
has already had a symbol renamed (i.e. it is currently
broken, but renaming the symbol will fix it).

Instead of gomvpkg, I would find a copy-package utility more useful,
with the scope strictly limited to the package itself, although
govers covers most of my needs here.

Submitting this as an issue because I'd really like to find these tools useful
but I have never encountered as situation where I was able to use them,
even though I'm refactoring and renaming all the time.

@minux

This comment has been minimized.

Copy link
Member

commented May 20, 2015

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2015

Thanks for pointing out the other issue.

If preparing a new GOPATH what you need to do each time, then it seems to me
that the tool should make it easy to avoid that (for example by providing
a way to scope the changes).

The problem is that if you scope the changes, and then later encounter
a package that you'd actually want to have included in the scope,
these tools won't help and you have to revert to the usual
gofmt -r, compile, manual edit, run tests. It would be great to have tools
that help in the distributed repository case.

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2015

Hi Rog, can you clarify a few things for me?

You say you never want to rename within all the packages that refer to the renamed symbol, because you do not necessarily have commit rights to them. Does that mean you are intentionally breaking certain packages?

Similarly, when you say you might not have all the packages that use the renamed symbol in your GOPATH, is this another case of intentional breakage?

The notion of repository isn't very well defined in 'go build'. Do you mean the directories immediately beneath GOPATH/src?

It should be straightforward to add flags that allow the user to express the disposition of each package, perhaps using a glob or regexp. There are several dispositions of possible interest:
(a) this package is to be updated normally.
(b) this package is to be skipped during the update phase.
(c) the whole refactoring should be called off if it would update this package.
(d) this package should not even be looked at.
I'm not sure (b) is necessary.

Let me think about it for a bit.

@jmhodges

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

Movement on this would be cool. This is still a big bummer and one of the reasons why I can't reliably use or recommend gorename to other folks.

@manishrjain

This comment has been minimized.

Copy link

commented Sep 18, 2016

When I try to run :GoRename in my project at github.com/dgraph-io/dgraph, it complains about unsafe.Pointers in github.com/dgraph-io/experiments, which is an unmaintained, generally uncompiled repository, which have literally nothing to do with the code.

So, there should be a way to tell rename function to look within the current project, and not have it ensure that the entire GOPATH is compilable.

@alaingilbert

This comment has been minimized.

Copy link

commented Nov 23, 2016

Totally agree with @manishrjain

@pwaller

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

Yes, gorename has never worked reliably for me and is always very slow.

I wonder if this comes from some different philosophy. Like many others here, I set GOPATH once and be done with it. Clearly there are other ways of doing it but this is what I have been doing for years now and I generally have had few problems with this. (This issue notwithstanding). @davecheney and others have liked to tell users like me that my behaviour is wrong and that I should do extra work to set GOPATH all the time for different projects (and teach others to do the same). But I still haven't been compelled to do this (apologies to @davecheney and others if what I write here is a poor straw man). This might just be that I am an uneducated heathen, in which case I would love to be pointed at some documentation which should tell me how I develop correctly.

As another commenter noted, maybe this comes down to the fact that at Google you're blessed because you have a pristine monorepo which is always perfectly maintained and has people thinking carefully how to organize it, and not doing things like putting the Go sources so that they would appear in your GOPATH. Or maybe you have software which sets your GOPATH for you. I don't really know. I can only assume you don't spend your days setting your GOPATH.

My monorepo is github.com the entire internet. Muaha, haha, ha. aha. Cough, sorry. I have a cold.

My GOPATH contains code in various states of repair, and in fact it also contains the Go repository itself at $HOME/.local/src/go. Gorename for instance fails with many errors referring to regression tests inside the go compiler sources.

As a user trying to rename a single symbol in a small package I'm developing inside my monorepo of insanity it feels like gorename is failing me when it takes 20 seconds and then fails.

For the experience of most Go devs to be smooth, gorename should probably default to not trying to change everything everywhere and be scoped unless they ask for it. I could see an argument though that this should be the default of any UX which wraps gorename rather than gorename's problem itself. But in any case I'd love to see this "just work" for people who don't spend their time thinking much about GOPATH.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

The only thing I ever want from gorename is to change things in exactly one package, ie. one directory.

@zmb3

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

Just an idea - what if we used the presence of a vendor directory in the project as an indicator that gorename should not look outside the project? The assumption would be that a project that vendors some dependencies vendors all dependencies, and depends on nothing else in the #GOPATH.

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2017

FYI: Our team is currently working to build a foundation for analysis tools such as gorename that allows them to work equally well in workspaces laid out using the conventions of 'go build', Bazel, and Google's internal build system, at which point we'll have more available effort (and incentive) to improve the usability of the tools for everyone.

@thomasf

This comment has been minimized.

Copy link

commented Aug 18, 2017

#16427 is related to the reason why I want to restrict where gorename chooses to work.. I have more or less stopped using gorename because it takes well over a minute for it to work through my src tree. For most cases it's just faster to edit by hand and even if it's not I prefer having something to do instead of the gorename/wait cycle that happens for a bunch of changes.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2017

I like gorename integrated in vim-go, but I desperately need a way to limit it's scope to exactly one package. That's what I ever want. I even consider it silently messing with other repositories a dangerous bug, TBH.

Currently gorename works no more for me in vim-go since I have the github.com/gcc-mirror repository in my $GOPATH and gorename chokes on some Go test file in that repo.

"Package "github.com/gcc-mirror/gcc/gcc/testsuite/go.test/test/syntax": /home/jnml/src/github.com/gcc-mirror/gcc/gcc/testsuite/go.test/test/syntax/import.go" [New DIRECTORY]
Error detected while processing function <SNR>15_template_autocreate..go#template#create:
line    6:
E344: Can't find directory "Package "github.com/gcc-mirror/gcc/gcc/testsuite/go.test/test/syntax": /home/jnml/src/github.com/gcc-mirror/gcc/gcc/testsuite/go.test/test/syntax" in cdpath
E472: Command failed

I realize this is probably a vim-go bug rather that gorename's, but if I could limit gorename's scope to only the package I'm editing the bug will probably not show up at all.

@thomasf

This comment has been minimized.

Copy link

commented Nov 11, 2017

It's probably not vim-go. gorename doesnt work for me anymore even after waiting the 1-2 minutes it takes for it to go throgh my ~/src. A couple of GCC source trees are examples where gorename fails hard enough to refuse a rename to even go through.

@buyology

This comment has been minimized.

Copy link

commented Nov 18, 2017

+1 on all the experiences. For now, you are better off using find and replace — which is tedious.

There really needs to be an option to say current package as @cznic suggested or a white/blacklist/glob of some sort. This could then ofc easily be exploited by IDEs to improve the refactoring experience (e.g. vscode supports workspaces).

@cznic

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2018

I today tried to fork and rip off the repair-whole-world functionality from gorename. I haven't really studied the code, so as expected, the first try ran into run-time panics - as I've obviously broken some invariants the code relies on. I've put only about 10 minutes into this and gave up at that point.

What about crowdfunding someone to do the fork? I'd happily participate with, say some tens of €. If there would be like at least a dozen of us I guess the thing can possibly happen.

Thougths?

cc @dominikh

@buyology

This comment has been minimized.

Copy link

commented Apr 16, 2018

@cznic — I think it sounds like a great idea. (I did a similar fork recently, where I just commented out the global renaming part rename.go and got an unsurprising drop in execution time from 52 down to 1.5 seconds which made me dream of how life would be with a better tool.)

Create a funding page and let the fundraising begin? :)

On a more general note, it would be really cool to have a fund where such efforts/ideas for static analysis and refactoring tools could be continuously funded. That should have the potential to attract corporate sponsors too.

@dominikh

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Just a bit of information on my own plans: in 2018, I want to implement something akin to GOPATH/pkg for my own tools, i.e. cache type information as well as ASTs. Additionally, there would be indices to quickly look up identifiers, among other information.

After this database is in place, I'll port tools like guru and gorename to make use of it, which should result in significant speed improvements.

As for funding, there isn't a pool of funding for multiple devs, but there is my own Patreon to support specifically my tools.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

After this database is in place, I'll port tools like guru and gorename to make use of it, which should result in significant speed improvements.

Nice. Wrt speed improvements, those are welcome, but much more important for me is that the tool never silently mutates other packages. (vim-go reports only the total number of renames, IIRC). Something like GORENAME_SCOPE=package or similar would be wonderful.

As for funding, there isn't a pool of funding for multiple devs, but there is my own Patreon to support specifically my tools.

Yup, that's why I have CC'd you ;-)

@TheoBrigitte

This comment has been minimized.

Copy link

commented Jul 10, 2018

Any movement on this ?

Just came across the same issue, where gorename -from '"mypackage".RenameMe -to NewName would fail because mypackage is also used as vendor in some other package, and it is broken there.
Also I am not comfortable with renaming across my entire workspace, all I want is to rename for the current package I am working.

I ended up using https://github.com/prateek/gorename fork which pretty much does the job.

@ysmolsky ysmolsky added the NeedsFix label Nov 21, 2018

@ryanavella

This comment has been minimized.

Copy link

commented Jan 10, 2019

Here is a package that achieves the expected behavior, perhaps it is useful as an example of how to limit gorename's scope?

https://github.com/amitbet/gorename

For the time being I will remove gorename from my path and add this as a functional replacement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.