From d73c11bfcb55cf77a7fd00a3b7684fc88a03c4b2 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 31 Oct 2014 15:39:22 -0400 Subject: [PATCH] refactor/rename: make -from syntax support string literals for complex import paths. (They may contain any character, after all.) Also, allow but don't require parens and stars. e.g. (*"encoding/json".Decoder).Decode or "encoding/json".Decoder.Decode but not encoding/json.Decoder.Decode. Since -from queries are now Go expressions, we use the Go parser. (Thanks to Rog Peppe for the suggestion.) LGTM=sameer R=sameer CC=golang-codereviews, gri, rogpeppe https://golang.org/cl/154610043 --- cmd/gorename/main.go | 4 +- refactor/rename/rename_test.go | 2 +- refactor/rename/spec.go | 129 +++++++++++++++++++++------------ refactor/rename/util.go | 13 ++++ 4 files changed, 99 insertions(+), 49 deletions(-) diff --git a/cmd/gorename/main.go b/cmd/gorename/main.go index 255eb49d..8ba61f8f 100644 --- a/cmd/gorename/main.go +++ b/cmd/gorename/main.go @@ -86,7 +86,7 @@ Examples: Rename the object whose identifier is at byte offset 123 within file file.go. -% gorename -from '(bytes.Buffer).Len' -to Size +% gorename -from '"bytes".Buffer.Len' -to Size Rename the "Len" method of the *bytes.Buffer type to "Size". @@ -98,6 +98,7 @@ Correctness: - sketch a proof of exhaustiveness. Features: +- support running on packages specified as *.go files on the command line - support running on programs containing errors (loader.Config.AllowErrors) - allow users to specify a scope other than "global" (to avoid being stuck by neglected packages in $GOPATH that don't build). @@ -114,7 +115,6 @@ Features: all local variables of a given type, all PkgNames for a given package. - emit JSON output for other editors and tools. -- integration with editors other than Emacs. ` func main() { diff --git a/refactor/rename/rename_test.go b/refactor/rename/rename_test.go index 2e46569f..808e9495 100644 --- a/refactor/rename/rename_test.go +++ b/refactor/rename/rename_test.go @@ -305,7 +305,7 @@ var _ interface {f()} = C(0) `would conflict with this method`, }, { - from: "(main.I).f", to: "h", + from: `("main".I).f`, to: "h", // NB: exercises quoted import paths too want: `renaming this interface method "f" to "h".*` + `would conflict with this method.*` + `in named interface type "J"`, diff --git a/refactor/rename/spec.go b/refactor/rename/spec.go index d8d320a6..dc390f00 100644 --- a/refactor/rename/spec.go +++ b/refactor/rename/spec.go @@ -68,16 +68,19 @@ type spec struct { const FromFlagUsage = ` A legal -from query has one of the following forms: - (encoding/json.Decoder).Decode method of package-level named type - (encoding/json.Decoder).buf field of package-level named struct type - encoding/json.HTMLEscape package member (const, func, var, type) - (encoding/json.Decoder).Decode::x local object x within a method - encoding/json.HTMLEscape::x local object x within a function - encoding/json::x object x anywhere within a package - json.go::x object x within file json.go + "encoding/json".Decoder.Decode method of package-level named type + (*"encoding/json".Decoder).Decode ditto, alternative syntax + "encoding/json".Decoder.buf field of package-level named struct type + "encoding/json".HTMLEscape package member (const, func, var, type) + "encoding/json".Decoder.Decode::x local object x within a method + "encoding/json".HTMLEscape::x local object x within a function + "encoding/json"::x object x anywhere within a package + json.go::x object x within file json.go - For methods attached to a pointer type, the '*' must not be specified. - [TODO(adonovan): fix that.] + For methods, the parens and '*' on the receiver type are both optional. + + Double-quotes may be omitted for single-segment import paths such as + fmt. They may need to be escaped when writing a shell command. It is an error if one of the ::x queries matches multiple objects. ` @@ -100,14 +103,8 @@ func parseFromFlag(ctxt *build.Context, fromFlag string) (*spec, error) { return nil, fmt.Errorf("-from %q: invalid identifier specification (see -help for formats)", fromFlag) } - // main is one of: - // filename.go - // importpath - // importpath.member - // (importpath.type).fieldormethod - if strings.HasSuffix(main, ".go") { - // filename.go + // main is "filename.go" if spec.searchFor == "" { return nil, fmt.Errorf("-from: filename %q must have a ::name suffix", main) } @@ -122,30 +119,14 @@ func parseFromFlag(ctxt *build.Context, fromFlag string) (*spec, error) { } spec.pkg = bp.ImportPath - } else if a, b := splitAtLastDot(main); b == "" { - // importpath e.g. "encoding/json" - if spec.searchFor == "" { - return nil, fmt.Errorf("-from %q: package import path %q must have a ::name suffix", - main, a) - } - spec.pkg = a - - } else if strings.HasPrefix(a, "(") && strings.HasSuffix(a, ")") { - // field/method of type e.g. (encoding/json.Decoder).Decode - c, d := splitAtLastDot(a[1 : len(a)-1]) - if d == "" { - return nil, fmt.Errorf("-from %q: not a package-level named type: %q", a) - } - spec.pkg = c // e.g. "encoding/json" - spec.pkgMember = d // e.g. "Decoder" - spec.typeMember = b // e.g. "Decode" - spec.fromName = b - } else { - // package member e.g. "encoding/json.HTMLEscape" - spec.pkg = a // e.g. "encoding/json" - spec.pkgMember = b // e.g. "HTMLEscape" - spec.fromName = b + // main is one of: + // "importpath" + // "importpath".member + // (*"importpath".type).fieldormethod (parens and star optional) + if err := parseObjectSpec(&spec, main); err != nil { + return nil, err + } } if spec.searchFor != "" { @@ -171,14 +152,70 @@ func parseFromFlag(ctxt *build.Context, fromFlag string) (*spec, error) { return &spec, nil } -// "encoding/json.HTMLEscape" -> ("encoding/json", "HTMLEscape") -// "encoding/json" -> ("encoding/json", "") -func splitAtLastDot(s string) (string, string) { - i := strings.LastIndex(s, ".") - if i == -1 { - return s, "" +// parseObjectSpec parses main as one of the non-filename forms of +// object specification. +func parseObjectSpec(spec *spec, main string) error { + // Parse main as a Go expression, albeit a strange one. + e, _ := parser.ParseExpr(main) + + if pkg := parseImportPath(e); pkg != "" { + // e.g. bytes or "encoding/json": a package + spec.pkg = pkg + if spec.searchFor == "" { + return fmt.Errorf("-from %q: package import path %q must have a ::name suffix", + main, main) + } + return nil } - return s[:i], s[i+1:] + + if e, ok := e.(*ast.SelectorExpr); ok { + x := unparen(e.X) + + // Strip off star constructor, if any. + if star, ok := x.(*ast.StarExpr); ok { + x = star.X + } + + if pkg := parseImportPath(x); pkg != "" { + // package member e.g. "encoding/json".HTMLEscape + spec.pkg = pkg // e.g. "encoding/json" + spec.pkgMember = e.Sel.Name // e.g. "HTMLEscape" + spec.fromName = e.Sel.Name + return nil + } + + if x, ok := x.(*ast.SelectorExpr); ok { + // field/method of type e.g. ("encoding/json".Decoder).Decode + y := unparen(x.X) + if pkg := parseImportPath(y); pkg != "" { + spec.pkg = pkg // e.g. "encoding/json" + spec.pkgMember = x.Sel.Name // e.g. "Decoder" + spec.typeMember = e.Sel.Name // e.g. "Decode" + spec.fromName = e.Sel.Name + return nil + } + } + } + + return fmt.Errorf("-from %q: invalid expression") +} + +// parseImportPath returns the import path of the package denoted by e. +// Any import path may be represented as a string literal; +// single-segment import paths (e.g. "bytes") may also be represented as +// ast.Ident. parseImportPath returns "" for all other expressions. +func parseImportPath(e ast.Expr) string { + switch e := e.(type) { + case *ast.Ident: + return e.Name // e.g. bytes + + case *ast.BasicLit: + if e.Kind == token.STRING { + pkgname, _ := strconv.Unquote(e.Value) + return pkgname // e.g. "encoding/json" + } + } + return "" } // parseOffsetFlag interprets the "-offset" flag value as a renaming specification. diff --git a/refactor/rename/util.go b/refactor/rename/util.go index f9354d86..67f48e82 100644 --- a/refactor/rename/util.go +++ b/refactor/rename/util.go @@ -5,6 +5,7 @@ package rename import ( + "go/ast" "os" "path/filepath" "reflect" @@ -98,3 +99,15 @@ func sameFile(x, y string) bool { } return false } + +// unparen returns e with any enclosing parentheses stripped. +func unparen(e ast.Expr) ast.Expr { + for { + p, ok := e.(*ast.ParenExpr) + if !ok { + break + } + e = p.X + } + return e +}