Issue 139150044: code review 139150044: cmd/gorename: a precise, type-aware renaming tool for G... (Closed)
|
Created:
9 years, 8 months ago by adonovan Modified:
9 years, 7 months ago CC:
gri, Sameer Ajmani, Dominik Honnef, golang-codereviews Visibility:
Public. |
Descriptioncmd/gorename: a precise, type-aware renaming tool for Go identifiers.
See the usage message in main.go for orientation.
To the best of my knowledge, the tool implements all required
soundness checks, except:
- the dynamic behaviour of reflection is obviously undecidable.
- it rejects method renamings that change the "implements" relation.
It should probably be more aggressive.
- actually it only checks the part of the "implements" relation
needed for compilation. Understanding the dynamic behaviour
of interfaces is obviously undecidable.
- a couple of minor gaps are indicated by TODO comments.
Also:
- Emacs integration.
- tests of all safety checks and (some) successful rewrites.
Patch Set 1 #Patch Set 2 : diff -r b0670ff992964911f0ce10e7fe50aab5829ac8ab https://code.google.com/p/go.tools #Patch Set 3 : diff -r b0670ff992964911f0ce10e7fe50aab5829ac8ab https://code.google.com/p/go.tools #Patch Set 4 : diff -r 01f8610e891f7af6ba5a468ff65855aba60c4430 https://code.google.com/p/go.tools #Patch Set 5 : diff -r 01f8610e891f7af6ba5a468ff65855aba60c4430 https://code.google.com/p/go.tools #Patch Set 6 : diff -r 01f8610e891f7af6ba5a468ff65855aba60c4430 https://code.google.com/p/go.tools #Patch Set 7 : diff -r 01f8610e891f7af6ba5a468ff65855aba60c4430 https://code.google.com/p/go.tools #
Total comments: 55
Patch Set 8 : diff -r 223c284e2435f15b967f3e0bf8e6388598e71678 https://code.google.com/p/go.tools #
Total comments: 18
Patch Set 9 : diff -r 223c284e2435f15b967f3e0bf8e6388598e71678 https://code.google.com/p/go.tools #Patch Set 10 : diff -r 223c284e2435f15b967f3e0bf8e6388598e71678 https://code.google.com/p/go.tools #
Total comments: 13
Patch Set 11 : diff -r 223c284e2435f15b967f3e0bf8e6388598e71678 https://code.google.com/p/go.tools #
Total comments: 4
Patch Set 12 : diff -r 13423edebb30677d71918291b73798dbeeb5f2a7 https://code.google.com/p/go.tools #
MessagesTotal messages: 19
Hello gri@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.tools
Sign in to reply to this message.
Hello gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello gri@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Stopping here today. More to do. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go File cmd/gorename/main.go (right): https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:17: offsetFlag = flag.String("offset", "", "file and byte offset of identifier to be renamed, e.g. 'file.go:#123'. For use by editors.") is the "file:#offset" syntax standard across editors? If not, separate flags for each might be simpler --file=string; --offset=int. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:80: It might be useful to include examples of the kinds of renamings this will reject. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:93: - support renaming the package clause (no object) YES! https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:95: (requires filesystem + SCM updates). so does renaming the package ... https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:101: - support renaming all instances of a pattern, e.g. This is starting to sound more like the eg tool. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:106: - integration support for editors other than Emacs. vi and Sublime, in particular. These could be filed as their own issues, with an invitation out to the community for contributions. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:122: if err != rename.ConflictError { are conflicts printed out elsewhere? https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go File refactor/rename/check.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:43: r.errorf(from.Pos(), "unexpected %s object %s (please report a bug)\n", %q for from Throughout, I'll assume you want %s for Kinds and %q for other identifiers, per our chat. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:109: "renaming this func %q to %s would make it a package initializer", %q https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:114: r.errorf(from.Pos(), "you cannot have a %s at package level named %s", the latter %s should be %q https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:148: // case int: print(y) // Implicits[CaseClase(int)] = Var(y_int) typo CaseClause https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:149: // case string: print(y) // Implicits[CaseClase(string)] = Var(y_string) ditto https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:258: // requires renaming the field too. Can you provide an example for this case? https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:413: "\twould shadow this selection") I'm not sure most users would understand the phrase "shadow this selection". https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:427: r.errorf(from.Pos(), "you cannot rename the built-in error.Error method") I'd feel better if error.Error were replaced by from's own type and method, so this doesn't encode an assumption about the current spec, but it's probably fine :-) https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:451: // The assignments.Finder enumerates these. satisfy.Finder now https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:501: r.errorf(obj.Pos(), "\tin named interface type %s", obj.Name()) %q https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:532: r.errorf(pos, "\twould make %s no longer assignable to it", other) %q https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:566: r.errorf(pos, "\twould make it no longer assignable to %s", iface) %q https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.el File refactor/rename/rename.el (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.e... refactor/rename/rename.el:33: ;; (if (string-equal "" go-rename-scope) rm? TODO? https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.e... refactor/rename/rename.el:67: ;; TODO(adonovan): have oracle give us JSON s/oracle/gorename/? https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.e... refactor/rename/rename.el:99: ;; Reload the modified filed, saving line/col. typo files https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.go File refactor/rename/rename.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:37: // (It is distinguished because the interested errors are the conflicts themselves.) interested or interesting? https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:228: if ast.IsExported(to) && tfm { return ast.IsExported(to) && tfm https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:234: // used as an embeddeed struct field entails a field typo embedded https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:288: fmt.Fprintf(os.Stderr, "Renamed %d occurrence%s in %d file%s in %d package%s.\n", verbose? Consider wrapping the if-verbose-fprint-stderr logic into a helper vlog function. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:299: if n > 1 { if n != 1, since you want "s" for 0 https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename_t... File refactor/rename/rename_test.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename_t... refactor/rename/rename_test.go:376: func fakeContext(pkgs map[string][]string) *build.Context { define what pkgs is; what are the keys and values?
Sign in to reply to this message.
Thanks for the review. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go File cmd/gorename/main.go (right): https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:17: offsetFlag = flag.String("offset", "", "file and byte offset of identifier to be renamed, e.g. 'file.go:#123'. For use by editors.") On 2014/09/19 21:03:35, Sameer Ajmani wrote: > is the "file:#offset" syntax standard across editors? If not, separate flags for > each might be simpler --file=string; --offset=int. It's standard across the tools we're building: the oracle supports it too. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:80: On 2014/09/19 21:03:35, Sameer Ajmani wrote: > It might be useful to include examples of the kinds of renamings this will > reject. Done. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:93: - support renaming the package clause (no object) On 2014/09/19 21:03:35, Sameer Ajmani wrote: > YES! It's not hard, but it feels like an almost an entirely different tool: its -from and -to are import paths (not identifiers), it must move files and directories, and must update SCM state. Only the logic to check for name conflicts is similar. I wonder if it would be best as a "gomvpkg" tool. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:95: (requires filesystem + SCM updates). On 2014/09/19 21:03:35, Sameer Ajmani wrote: > so does renaming the package ... By convention, the package name is the last segment of its import path, so it's unlikely you'd want a renaming to break that invariant. Thus these two are really the same thing. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:101: - support renaming all instances of a pattern, e.g. On 2014/09/19 21:03:35, Sameer Ajmani wrote: > This is starting to sound more like the eg tool. For certain pre-canned patterns, yes. I'm not sure what the UI should be, especially for editor-initiated renames. We should discuss. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:106: - integration support for editors other than Emacs. On 2014/09/19 21:03:35, Sameer Ajmani wrote: > vi and Sublime, in particular. These could be filed as their own issues, with > an invitation out to the community for contributions. Yep. https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:122: if err != rename.ConflictError { On 2014/09/19 21:03:35, Sameer Ajmani wrote: > are conflicts printed out elsewhere? Yes, they're printed to stderr, and a dummy error (that doesn't need printing) is returned. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go File refactor/rename/check.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:43: r.errorf(from.Pos(), "unexpected %s object %s (please report a bug)\n", On 2014/09/19 21:03:36, Sameer Ajmani wrote: > %q for from > > Throughout, I'll assume you want %s for Kinds and %q for other identifiers, per > our chat. All done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:148: // case int: print(y) // Implicits[CaseClase(int)] = Var(y_int) On 2014/09/19 21:03:36, Sameer Ajmani wrote: > typo CaseClause Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:149: // case string: print(y) // Implicits[CaseClase(string)] = Var(y_string) On 2014/09/19 21:03:36, Sameer Ajmani wrote: > ditto Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:258: // requires renaming the field too. On 2014/09/19 21:03:36, Sameer Ajmani wrote: > Can you provide an example for this case? type T int // if we rename this to U.. var s struct {T} print(s.T) // ...this must change too https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:413: "\twould shadow this selection") On 2014/09/19 21:03:36, Sameer Ajmani wrote: > I'm not sure most users would understand the phrase "shadow this selection". We use this terminology in other tools. I think it's a pretty good metaphor, since the inner declaration "casts a shadow" in the light (scope) of the outer one. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:427: r.errorf(from.Pos(), "you cannot rename the built-in error.Error method") On 2014/09/19 21:03:36, Sameer Ajmani wrote: > I'd feel better if error.Error were replaced by from's own type and method, so > this doesn't encode an assumption about the current spec, but it's probably fine > :-) Done. But encoding assumptions about the spec is pretty much my job description. :) https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:451: // The assignments.Finder enumerates these. On 2014/09/19 21:03:36, Sameer Ajmani wrote: > satisfy.Finder now Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.el File refactor/rename/rename.el (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.e... refactor/rename/rename.el:33: ;; (if (string-equal "" go-rename-scope) On 2014/09/19 21:03:36, Sameer Ajmani wrote: > rm? TODO? This whole file needs some love. I'd like to factor it in common with oracle's .el file before I commit it. I should also add back the "analysis scope" feature I removed when I added GOPATH scanning. Sometimes the GOPATH contains packages that don't compile and it would be nice to explicitly exclude them. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.e... refactor/rename/rename.el:67: ;; TODO(adonovan): have oracle give us JSON On 2014/09/19 21:03:36, Sameer Ajmani wrote: > s/oracle/gorename/? Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.e... refactor/rename/rename.el:99: ;; Reload the modified filed, saving line/col. On 2014/09/19 21:03:36, Sameer Ajmani wrote: > typo files Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.go File refactor/rename/rename.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:37: // (It is distinguished because the interested errors are the conflicts themselves.) On 2014/09/19 21:03:37, Sameer Ajmani wrote: > interested or interesting? Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:228: if ast.IsExported(to) && tfm { On 2014/09/19 21:03:36, Sameer Ajmani wrote: > return ast.IsExported(to) && tfm This way lets me document it better. The compiler generates the same code. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:234: // used as an embeddeed struct field entails a field On 2014/09/19 21:03:37, Sameer Ajmani wrote: > typo embedded Done. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:288: fmt.Fprintf(os.Stderr, "Renamed %d occurrence%s in %d file%s in %d package%s.\n", On 2014/09/19 21:03:36, Sameer Ajmani wrote: > verbose? > > Consider wrapping the if-verbose-fprint-stderr logic into a helper vlog > function. For now I pretty much always want to see this. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename.g... refactor/rename/rename.go:299: if n > 1 { On 2014/09/19 21:03:37, Sameer Ajmani wrote: > if n != 1, since you want "s" for 0 Done. Good catch. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename_t... File refactor/rename/rename_test.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/rename_t... refactor/rename/rename_test.go:376: func fakeContext(pkgs map[string][]string) *build.Context { On 2014/09/19 21:03:37, Sameer Ajmani wrote: > define what pkgs is; what are the keys and values? Done.
Sign in to reply to this message.
https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go File cmd/gorename/main.go (right): https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:93: - support renaming the package clause (no object) On 2014/09/19 21:31:50, adonovan wrote: > On 2014/09/19 21:03:35, Sameer Ajmani wrote: > > YES! > > It's not hard, but it feels like an almost an entirely different tool: its -from > and -to are import paths (not identifiers), it must move files and directories, > and must update SCM state. Only the logic to check for name conflicts is > similar. > > I wonder if it would be best as a "gomvpkg" tool. I agree this sounds like a different tool. Not sure about the name. One could imagine other "package level" operations like merge & split that would overlap with move, so perhaps there's a gopkg tool here. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go File refactor/rename/check.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:258: // requires renaming the field too. On 2014/09/19 21:31:50, adonovan wrote: > On 2014/09/19 21:03:36, Sameer Ajmani wrote: > > Can you provide an example for this case? > > type T int // if we rename this to U.. > var s struct {T} > print(s.T) // ...this must change too Thanks. Perhaps include this example with this comment in the code. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go File refactor/rename/spec.go (right): https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:31: // If the query had a ::from component, this is that; what query? https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:37: // -- The remaining fields are private to this file. All are optional. -- Are the preceding fields required? https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:44: pkgmember string pkgMember https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:47: typemember string typeMember https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:58: const FromFlagUsage = ` Aha, the spec makes a lot more sense with this usage message as explanation :-) Add a comment to the spec doc comment: // A spec specifies an entity to rename. // It is created from a query; see FromFlagUsage for the allowed query forms. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:64: (encoding/json.Decoder).Decode::x local object x within a method "anywhere within a method", ot mirror "anywhere in package", below? Specifically, does this rename x inside a function literal in the method? func (*Decoder) Decode() { ... f := func() { var x int // will this be renamed? } } https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:66: encoding/json::x object x anywhere in package Perhaps you should use encoding/json::HTMLEscape here to contrast with encoding/json.HTMLEscape https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:69: For methods attached to a pointer type, the '*' must not be specified. "must not" seems like an odd constraint here. When a method on a pointer is referred to in runtime.Stack output, it includes the *. Why not here? https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:84: fallthrough This logic seems unnecessarily clever.
Sign in to reply to this message.
Thanks, all done. I've added tests of successful rewrites. They found a bug, now fixed, when renaming an anonymous field (the corresponding type must be renamed too). (I'd implemented only the reverse check before). https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go File cmd/gorename/main.go (right): https://codereview.appspot.com/139150044/diff/120001/cmd/gorename/main.go#new... cmd/gorename/main.go:93: - support renaming the package clause (no object) On 2014/09/22 14:46:37, Sameer Ajmani wrote: > On 2014/09/19 21:31:50, adonovan wrote: > > On 2014/09/19 21:03:35, Sameer Ajmani wrote: > > > YES! > > > > It's not hard, but it feels like an almost an entirely different tool: its > -from > > and -to are import paths (not identifiers), it must move files and > directories, > > and must update SCM state. Only the logic to check for name conflicts is > > similar. > > > > I wonder if it would be best as a "gomvpkg" tool. > > I agree this sounds like a different tool. Not sure about the name. One could > imagine other "package level" operations like merge & split that would overlap > with move, so perhaps there's a gopkg tool here. Acknowledged. https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go File refactor/rename/check.go (right): https://codereview.appspot.com/139150044/diff/120001/refactor/rename/check.go... refactor/rename/check.go:258: // requires renaming the field too. On 2014/09/22 14:46:37, Sameer Ajmani wrote: > On 2014/09/19 21:31:50, adonovan wrote: > > On 2014/09/19 21:03:36, Sameer Ajmani wrote: > > > Can you provide an example for this case? > > > > type T int // if we rename this to U.. > > var s struct {T} > > print(s.T) // ...this must change too > > Thanks. Perhaps include this example with this comment in the code. Done. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go File refactor/rename/spec.go (right): https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:31: // If the query had a ::from component, this is that; On 2014/09/22 14:46:38, Sameer Ajmani wrote: > what query? See file documentation; the -from flag is a kind of query over the identifiers defined in a file or package; it must return a singleton result. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:37: // -- The remaining fields are private to this file. All are optional. -- On 2014/09/22 14:46:37, Sameer Ajmani wrote: > Are the preceding fields required? They are both defined; pkg may be "" but fromName may not. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:44: pkgmember string On 2014/09/22 14:46:37, Sameer Ajmani wrote: > pkgMember Done. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:47: typemember string On 2014/09/22 14:46:38, Sameer Ajmani wrote: > typeMember Done. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:58: const FromFlagUsage = ` On 2014/09/22 14:46:37, Sameer Ajmani wrote: > Aha, the spec makes a lot more sense with this usage message as explanation :-) > Add a comment to the spec doc comment: > // A spec specifies an entity to rename. > // It is created from a query; see FromFlagUsage for the allowed query forms. Done. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:64: (encoding/json.Decoder).Decode::x local object x within a method On 2014/09/22 14:46:37, Sameer Ajmani wrote: > "anywhere within a method", ot mirror "anywhere in package", below? Done. > Specifically, does this rename x inside a function literal in the method? > func (*Decoder) Decode() { > ... > f := func() { > var x int // will this be renamed? > } > } If that is the sole matching x, then yes. The foo::x syntax means "a definition of x syntactically within the AST of foo:, where foo is a var, func, file or package. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:66: encoding/json::x object x anywhere in package On 2014/09/22 14:46:38, Sameer Ajmani wrote: > Perhaps you should use encoding/json::HTMLEscape here to contrast with > encoding/json.HTMLEscape But it's unlikely you would ever specify HTMLEscape in this way, since you know it's a package-level entity. ::x is really intended to make it possible to specify local names that don't have good unambiguous name. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:69: For methods attached to a pointer type, the '*' must not be specified. On 2014/09/22 14:46:37, Sameer Ajmani wrote: > "must not" seems like an odd constraint here. When a method on a pointer is > referred to in runtime.Stack output, it includes the *. Why not here? It should probably be allowed but (for convenience) not required. Added a TODO to fix that. https://codereview.appspot.com/139150044/diff/140001/refactor/rename/spec.go#... refactor/rename/spec.go:84: fallthrough On 2014/09/22 14:46:37, Sameer Ajmani wrote: > This logic seems unnecessarily clever. Rephrased.
Sign in to reply to this message.
Hello gri@golang.org, sameer@golang.org, dominik.honnef@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Comments from just reading the code. More might follow once I get gorename up and running. https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.el File refactor/rename/rename.el (right): https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:23: (defcustom go-rename-command (concat (car (go-root-and-paths)) "/bin/gorename") Is there any harm in letting PATH deal with this and just invoke "gorename"? The name seems sufficiently unique to me. Assuming GOROOT will probably mean that almost everyone will have to change this setting. https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:38: (error "Please save the current buffer before invoking go-rename")) I'm still hoping that one day your tools will support reading the target file from stdin ;) https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:46: (1- (position-bytes (point))))) Use go--position-bytes for (some) compatibility with XEmacs https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:57: (message nil) ; clears/shrinks minibuffer Do we need to clear if we're printing another message right after?
Sign in to reply to this message.
https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.el File refactor/rename/rename.el (right): https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:95: (replace-regexp-in-string "[\t\n ]*$" This regex replaces all newlines, not just the last one, as $ matches end of line, not end of string. you want \\' instead of $ https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:101: (defun go--hide-filename-links () I'm not sure this substitution is all that helpful for gorename. For the oracle it makes sense, because it can possibly output dozens of lines, during normal operation, which are only interesting as navigational targets. gorename, however, will only display a couple of lines, and only in the case of an error. This substitution makes it harder to see what is going on, especially as it requires additional interaction to get to the file name/line number. Since we're actually dealing with error output here, being able to read the full error, including file name etc, might be more helpful than reduced clutter.
Sign in to reply to this message.
https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.el File refactor/rename/rename.el (right): https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:43: (let* ((filename (file-truename buffer-file-name)) Using file-truename here means that the following setup breaks: GOPATH=~/foo ~/foo is a symlink to ~/bar Opening ~/foo/baz.go will make gorename.el look at ~/bar/baz.go, and then it won't be able to guess the package from the file name, as ~/bar isn't in GOPATH – Symlinks are a fun thing, considering that when you worked on oracle.el, you actually needed file-truename to make symlinks work, now it's the other way around. Are there any cases that would break if *not* using file-truename? Maybe I should adjust my setup and stop using symlinks like that.
Sign in to reply to this message.
Thanks for the review. https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.el File refactor/rename/rename.el (right): https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:23: (defcustom go-rename-command (concat (car (go-root-and-paths)) "/bin/gorename") On 2014/09/22 17:57:04, Dominik Honnef wrote: > Is there any harm in letting PATH deal with this and just invoke "gorename"? The > name seems sufficiently unique to me. Assuming GOROOT will probably mean that > almost everyone will have to change this setting. I copied this from oracle.el, but you're probably right. Done. https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:38: (error "Please save the current buffer before invoking go-rename")) On 2014/09/22 17:57:04, Dominik Honnef wrote: > I'm still hoping that one day your tools will support reading the target file > from stdin ;) Yeah, me too. :) https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:43: (let* ((filename (file-truename buffer-file-name)) On 2014/09/22 18:58:12, Dominik Honnef wrote: > Using file-truename here means that the following setup breaks: > > GOPATH=~/foo > ~/foo is a symlink to ~/bar > > Opening ~/foo/baz.go will make gorename.el look at ~/bar/baz.go, and then it > won't be able to guess the package from the file name, as ~/bar isn't in GOPATH > – Symlinks are a fun thing, considering that when you worked on oracle.el, you > actually needed file-truename to make symlinks work, now it's the other way > around. > > Are there any cases that would break if *not* using file-truename? Maybe I > should adjust my setup and stop using symlinks like that. I've changed it to not use truename here; I suppose we should match the buffer's apparent filename and the textual directory in $GOPATH, and not try to be clever. (I'm struggling to remember why the oracle behaviour is different. The operations they perform internally are very similar.) https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:46: (1- (position-bytes (point))))) On 2014/09/22 17:57:04, Dominik Honnef wrote: > Use go--position-bytes for (some) compatibility with XEmacs More oracle copypasta. Will fix there too. https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:57: (message nil) ; clears/shrinks minibuffer On 2014/09/22 17:57:04, Dominik Honnef wrote: > Do we need to clear if we're printing another message right after? No. Deleted. (Ditto oracle.el.) https://codereview.appspot.com/139150044/diff/180001/refactor/rename/rename.e... refactor/rename/rename.el:101: (defun go--hide-filename-links () On 2014/09/22 18:31:39, Dominik Honnef wrote: > I'm not sure this substitution is all that helpful for gorename. For the oracle > it makes sense, because it can possibly output dozens of lines, during normal > operation, which are only interesting as navigational targets. gorename, > however, will only display a couple of lines, and only in the case of an error. > > This substitution makes it harder to see what is going on, especially as it > requires additional interaction to get to the file name/line number. Since we're > actually dealing with error output here, being able to read the full error, > including file name etc, might be more helpful than reduced clutter. Good point. Deleted.
Sign in to reply to this message.
emacs bits LGTM. gorename LGTM in that it worked for me.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/139150044/diff/200001/refactor/rename/spec.go File refactor/rename/spec.go (right): https://codereview.appspot.com/139150044/diff/200001/refactor/rename/spec.go#... refactor/rename/spec.go:82: [TODO: fix that]. TODO(who) https://codereview.appspot.com/139150044/diff/200001/refactor/rename/spec.go#... refactor/rename/spec.go:231: var wd string Perhaps simpler as: var wd = func() string { wd, err := os.GetWd() if err != nil {...} return wd }()
Sign in to reply to this message.
Thanks for the review. https://codereview.appspot.com/139150044/diff/200001/refactor/rename/spec.go File refactor/rename/spec.go (right): https://codereview.appspot.com/139150044/diff/200001/refactor/rename/spec.go#... refactor/rename/spec.go:82: [TODO: fix that]. On 2014/09/23 13:53:37, Sameer Ajmani wrote: > TODO(who) Done. https://codereview.appspot.com/139150044/diff/200001/refactor/rename/spec.go#... refactor/rename/spec.go:231: var wd string On 2014/09/23 13:53:37, Sameer Ajmani wrote: > Perhaps simpler as: > > var wd = func() string { > wd, err := os.GetWd() > if err != nil {...} > return wd > }() Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=511801758bb9&repo=tools *** cmd/gorename: a precise, type-aware renaming tool for Go identifiers. See the usage message in main.go for orientation. To the best of my knowledge, the tool implements all required soundness checks, except: - the dynamic behaviour of reflection is obviously undecidable. - it rejects method renamings that change the "implements" relation. It should probably be more aggressive. - actually it only checks the part of the "implements" relation needed for compilation. Understanding the dynamic behaviour of interfaces is obviously undecidable. - a couple of minor gaps are indicated by TODO comments. Also: - Emacs integration. - tests of all safety checks and (some) successful rewrites. LGTM=dominik.honnef, sameer R=gri, sameer, dominik.honnef CC=golang-codereviews https://codereview.appspot.com/139150044
Sign in to reply to this message.
Message was sent while issue was closed.
"gorename" reads to me like "gore name". why not just call it rename?
Sign in to reply to this message.
'rename' is already present in /usr/bin on the many POSIX systems as a non-standard script for renaming files according to a pattern. I think it comes from the Perl distribution. Plus, this tool gores the names of entities in your Go program. :) On 23 September 2014 10:41, <r@golang.org> wrote: > "gorename" reads to me like "gore name". why not just call it rename? > > https://codereview.appspot.com/139150044/ >
Sign in to reply to this message.
Message was sent while issue was closed.
adonovan wrote: > 'rename' is already present in /usr/bin on the many POSIX systems as a > non-standard script for renaming files according to a pattern. (Yes, it's very useful; rename 's/(\d+)-(\d+)/$2-$1/' *.foo) I too read "gore". How about renamego, since that's what it does? Re-order the examples so the one I'm likely to need is first, not the "for use by editors" one. Why are -from and -to needed? I don't have to "mv -from foo -to bar". Can't it take positional parameters instead for the common case, with a flag without argument to say the "from" is a byte location?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
