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/refactor/rename: provide a way to enumerate symbols in gorename notation #60831

Closed
ghost opened this issue Jun 16, 2023 · 19 comments
Closed
Assignees
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@ghost
Copy link

ghost commented Jun 16, 2023

currently this package and this tool are available:

which accepts input like this:

"bytes".Buffer.Len

however to my knowledge, no package exists that can emit all the identifiers of a package, in a format friendly to gorename. this is somewhat close:

go doc -all -u bytes

but the output is not machine readable. this is somewhat close:

https://pkg.go.dev/cmd/api

but again not machine readable, and I dont think it supports anything outside the standard library.

@ghost ghost added the Proposal label Jun 16, 2023
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jun 16, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jun 16, 2023
@ianlancetaylor
Copy link
Contributor

What about go doc -short bytes ?

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Jun 22, 2023
@dmitshur
Copy link
Contributor

CC @golang/tools-team.

@adonovan adonovan self-assigned this Jun 22, 2023
@adonovan
Copy link
Member

Ostensibly you are asking for a function that would enumerate all the methods (etc) of a package in the correct form for cmd/gorename so that you can then choose the one you want without having to think too hard about the syntax. Such a function would be easy to write, but I suspect that's not really the problem you're trying to solve. Do you perhaps wish gorename used a different syntax? Or offered some kind of menu or interactive choice? Or that there was a programmatic API through which you could express the renaming request? The solution to each of those problems is different.

@ghost
Copy link
Author

ghost commented Jun 22, 2023

Do you perhaps wish gorename used a different syntax? Or offered some kind of menu or interactive choice? Or that there was a programmatic API through which you could express the renaming request?

the tool could be better, but its probably good enough for now. the real issue is that the tool accepts some input, but some method should also be available to generate that input. that could be any of these:

  • an addition to the gorename tool
  • some new tool
  • a new function in x/tools/refactor/rename
  • some new package

if this new item was called "identifiers", someone could call it to generate all the identifiers for a package, then use the list as is, or narrow it down based on some criteria, and feed the result to x/tools/refactor/rename or gorename. as it currently stands, no tool or package is available to generate a list of identifiers, which makes using x/tools/refactor/rename and gorename painful for all but the most simple of use cases.

@adonovan
Copy link
Member

  • an addition to the gorename tool

gorename is not well supported: it dates from a time before Go modules, and uses an old library to load source code that cannot work well with modules. We don't plan to extend it.

  • some new tool

We built gopls, among other reasons, to improve the experience for users of tools like this one. Have you tried it? If you want to rename from within your editor, you'll find it works much better than gorename.

gopls does have a command-line too, but it uses file names and byte offsets to identify the symbol to be renamed, for example: gopls rename file.go:#123 newname. On the one hand, these identifiers are not very readable, but on the other, they are easy to write. I'm not sure if that's better or worse from your point of view---you still haven't said what you're actually trying to do. ;-)

@ghost
Copy link
Author

ghost commented Jun 22, 2023

thats way, way worse. I dont have an editor that supports a language server (GVIM), or if it does, I dont know or care how to set it up or use it. I think a simpler case would be something like what I have already described. I am interested in doing mass renaming inside a package, so I dont wish to do that process via an editor. I would like to do it from the command line, either via a premade tool, or one I can conjure from a package that gets most of the way there.

If GoPLS has a way to accept or produce non line number based identifiers, then I would be interested.

@adonovan
Copy link
Member

I dont have an editor that supports a language server (GVIM), or if it does, I dont know or care how to set it up or use it.

Vim does have an LSP client, for what it's worth, and if Gvim is fundamentally the same application then it probably does too.

I am interested in doing mass renaming inside a package, so I don't wish to do that process via an editor. I would like to do it from the command line, either via a premade tool, or one I can conjure from a package that gets most of the way there.

Ah! In that case you may be interested in https://github.com/rsc/rf, which is a scriptable batch refactoring tool. It's based on the same refactoring algorithm as gorename but has a much nicer batch interface.

If GoPLS has a way to accept or produce non line number based identifiers, then I would be interested.

Currently it does not, sorry. We could add one, but I wouldn't want to use the syntax of parseFromFlag in gorename: it's too hard to type (parens and quotation marks need escaping), and too hard to remember (what does :: mean?). The whole 'search' feature was added in pursuit of a generality (the ability to specify any object, even those declared within a function) that just isn't interesting in practice, since such a renaming is inherently local to that function. If users are going to go to the trouble of specifying 'from' symbols by some kind of name, it's because they have a larger scope. 'go doc' uses much more user-friendly heuristics to interpret dotted identifiers such as bytes.Buffer.Len, and I should have followed that approach.

If you want to take a crack at writing a function to print the names of package symbols in gorename syntax, I suggest you use this command as a starting point:

$ go run ./go/packages/gopackages -mode=types bytes
Go package "bytes":
	...
	type Buffer struct{buf []byte; off int; lastRead readOp}
	method (*Buffer) Bytes() []byte
	func Clone(b []byte) []byte
	var ErrTooLarge error
	const MinRead untyped int
	....

Unfortunately it is unlikely to be a priority for us, given the existing problems with gorename that I mentioned.

Best of luck.

@ghost
Copy link
Author

ghost commented Jun 25, 2023

@adonovan I checked both those suggestions, they both seems to have issues. I linked them above. Also regarding the actual renaming, I noticed that rsc.io/rf is about twice as fast as golang.org/x/tools/cmd/gorename:

> Measure-Command { rf 'mv FinishedHash _FinishedHash' }
TotalSeconds      : 1.6925358

> Measure-Command { rf 'mv FinishedHash _FinishedHash' }
TotalSeconds      : 1.7182022

> Measure-Command { gorename -from '\".\".FinishedHash' -to _FinishedHash }
TotalSeconds      : 3.5773823

> Measure-Command { gorename -from '\".\".FinishedHash' -to _FinishedHash }
TotalSeconds      : 3.4577449

@adonovan
Copy link
Member

@adonovan I checked both those suggestions, they both seems to have issues. I linked them above.

Regarding the first problem (missing unexported declarations), try -mode=syntax, as explained in #60995.

I noticed that rsc.io/rf is about twice as fast as golang.org/x/tools/cmd/gorename:

gorename uses the old go/loader package, and loading, parsing and type-checking is the dominant cost (not renaming itself), so I'm not surprised that it's slower.

@ghost
Copy link
Author

ghost commented Jun 26, 2023

Regarding the first problem (missing unexported declarations), try -mode=syntax, as explained in #60995.

thanks, that does fix it. one strange issue I noticed with x/tools/go/packages and the tool, is you seem to have no control in regards to the output of derivative types. so if you have this code:

package tls

import "math/big"

type DsaSignature struct {
   R, S *big.Int
}

type EcdsaSignature DsaSignature

you get this result:

> gopackages -mode syntax .
Go package "exports/tls":
        package tls
        has complete exported type info and typed ASTs
        file D:\Desktop\exports\tls\common.go
        import "math/big"
        type DsaSignature struct{R *math/big.Int; S *math/big.Int}
        type EcdsaSignature struct{R *math/big.Int; S *math/big.Int}

compared to something like go doc, which I think is a better representation of the type relationship:

> go doc -all
package tls // import "exports/tls"


TYPES

type DsaSignature struct {
        R, S *big.Int
}

type EcdsaSignature DsaSignature

what I mean is, the gopackages output makes it seem like you could rename the fields of one type without the other, when thats not the case. I believe go/ast gives you more control over this, but maybe I am missing something.

@adonovan
Copy link
Member

adonovan commented Jun 26, 2023

one strange issue I noticed

The reason for this difference is that go doc is formatting the syntax tree, whereas gopackages is formatting the type information, and in the go/types representation of Named types resulting from type A struct { ... }; type B A) there is no relationship between A and B. (You can figure it out based on positions, but that's a bit of a hack.)

See #8178 for details.

@adonovan adonovan changed the title x/tools/refactor/rename: companion package x/tools/refactor/rename: provide a way to enumerate symbols in gorename notation Jul 4, 2023
@ghost
Copy link
Author

ghost commented Jul 12, 2023

I cobbled together my own solution using go/ast and Russ tool rf, with a fallback of x/tools/cmd/gorename. I am moving on. If any wants to know more feel free to comment. closing this. Alan you are welcome to reopen if you think its worth it.

@ghost ghost closed this as completed Jul 12, 2023
@adonovan
Copy link
Member

I am curious about approach you took in the end; if you're comfortable, please share a link to your code. Glad you got it working; sorry the process was not smoother.

@ghost
Copy link
Author

ghost commented Jul 13, 2023

heres the code I used in the end:

https://github.com/1268/exports

I tried to keep it as generic as possible, but it is somewhat specific to my use case - hopefully it can at least be a starting point to anyone in a similar situation - thanks for the help along the way

@adonovan
Copy link
Member

What was the actual problem you were trying to solve? It seems like it may have been "rename unnecessarily exported identifiers to lowercase". In that case you may be interested in a tool I wrote (by chance) this week, golang.org/x/tools/internal/cmd/deadcode@master, that reports dead code (functions that cannot be reached from main), perhaps more precisely and efficiently than whatever tool you were using before.

@ghost
Copy link
Author

ghost commented Jul 13, 2023

I want to be able to take some arbitrary package and and remove everything that I personally dont care about. previously you could use StaticCheck with whole-program mode, but that was removed in 2020:

dominikh/go-tools@5cfc85b

so now items are only reported as unused, if they are not exported. so to solve that, I needed some method to un export all items of an arbitrary package, which Russ RF and x/tools/cmd/gorename can do. however the missing piece is some method to report all exported items, to feed into one of those tools. thats what my package does. it sounds like your tool might be a better option, I will check it out. I can report back my findings if you want.

@adonovan
Copy link
Member

Conceptually there are two parameters: the set of roots (executables and tests), and the filter subset. The packages arguments and the -test flag determine the roots; and the -filter flag determines what is shown.

deadcode -test ./hello/... reports all the functions that are linked into a executable or test in the hello tree but are unreachable.
deadcode ./hello/world reports functions linked into the hello/world executable that are unreachable by it (but may be reachable by tests, or other binaries).

I'm open to suggestions for a better command-line interface; it worked for my needs but I sense it may be tricky for others.

@ghost ghost mentioned this issue Sep 11, 2023
@ghost ghost reopened this Sep 11, 2023
@adonovan
Copy link
Member

Was this issue reopened intentionally? Could you elaborate on what changed? Thanks.

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 1, 2023
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants