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
Open

x/tools/cmd/{gomvpkg, gorename}: change scope is too large #10917

rogpeppe opened this issue May 20, 2015 · 20 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@rogpeppe
Copy link
Contributor

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
Copy link
Member

minux commented May 20, 2015 via email

@rogpeppe
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

manishrjain 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
Copy link

Totally agree with @manishrjain

@pwaller
Copy link
Contributor

pwaller 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
Copy link
Contributor

cznic commented Jan 6, 2017

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

@zmb3
Copy link
Contributor

zmb3 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
Copy link
Contributor

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
Copy link

thomasf 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
Copy link
Contributor

cznic 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
Copy link

thomasf 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
Copy link

buyology 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
Copy link
Contributor

cznic 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
Copy link

buyology 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
Copy link
Member

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
Copy link
Contributor

cznic 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
Copy link

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.

@ysmolski ysmolski added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 21, 2018
@ryanavella
Copy link

ryanavella 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.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests