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
diff --git a/cmd/gorename/main.go b/cmd/gorename/main.go
index 255eb49..8ba61f8 100644
--- a/cmd/gorename/main.go
+++ b/cmd/gorename/main.go
@@ -86,7 +86,7 @@
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 @@
- 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 @@
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 2e46569..808e949 100644
--- a/refactor/rename/rename_test.go
+++ b/refactor/rename/rename_test.go
@@ -305,7 +305,7 @@
`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 d8d320a..dc390f0 100644
--- a/refactor/rename/spec.go
+++ b/refactor/rename/spec.go
@@ -68,16 +68,19 @@
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 @@
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 @@
}
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 @@
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 f9354d8..67f48e8 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 @@
}
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
+}
