From 50ff896a1cbefcfb06368aec7d4c3904bf5c8651 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Wed, 27 Apr 2016 14:13:18 -0400 Subject: [PATCH] cmd/guru: definition: opt: avoid type checker for qualified identifiers For a 'definition' query on X in p.X, use special logic to load and parse package p and find the declaration of package member X, without using the type checker. Such queries now typically take under 10ms (faster than godef). The logic assumes that import "something/p" defines the name p. If this assumption is false, p.X could be a selection of a field or method X on a member p of the same package, defined in another file. So don't write code like that. Added missing test of 'definitions'. JSON tests now sanitize absolute $GOPATH filenames in the output. Fixes issue #15458 Change-Id: I21e75fcc9372aaedd56851cace444aef205c7a97 Reviewed-on: https://go-review.googlesource.com/22526 Reviewed-by: Dominik Honnef Reviewed-by: Michael Matloob --- cmd/guru/definition.go | 99 +++++++++++++++++++ cmd/guru/guru_test.go | 8 +- cmd/guru/testdata/src/definition-json/main.go | 44 +++++++++ .../testdata/src/definition-json/main.golden | 67 +++++++++++++ .../testdata/src/referrers-json/main.golden | 33 +++++++ cmd/guru/testdata/src/referrers/main.golden | 7 ++ 6 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 cmd/guru/testdata/src/definition-json/main.go create mode 100644 cmd/guru/testdata/src/definition-json/main.golden diff --git a/cmd/guru/definition.go b/cmd/guru/definition.go index 828316da..575ed92a 100644 --- a/cmd/guru/definition.go +++ b/cmd/guru/definition.go @@ -7,9 +7,15 @@ package main import ( "fmt" "go/ast" + "go/build" + "go/parser" "go/token" + pathpkg "path" + "path/filepath" + "strconv" "golang.org/x/tools/cmd/guru/serial" + "golang.org/x/tools/go/buildutil" "golang.org/x/tools/go/loader" ) @@ -30,6 +36,7 @@ func definition(q *Query) error { return fmt.Errorf("no identifier here") } + // Did the parser resolve it to a local object? if obj := id.Obj; obj != nil && obj.Pos().IsValid() { q.Output(qpos.fset, &definitionResult{ pos: obj.Pos(), @@ -37,6 +44,22 @@ func definition(q *Query) error { }) return nil // success } + + // Qualified identifier? + if pkg := packageForQualIdent(qpos.path, id); pkg != "" { + srcdir := filepath.Dir(qpos.fset.File(qpos.start).Name()) + tok, pos, err := findPackageMember(q.Build, qpos.fset, srcdir, pkg, id.Name) + if err != nil { + return err + } + q.Output(qpos.fset, &definitionResult{ + pos: pos, + descr: fmt.Sprintf("%s %s.%s", tok, pkg, id.Name), + }) + return nil // success + } + + // Fall back on the type checker. } // Run the type checker. @@ -82,6 +105,82 @@ func definition(q *Query) error { return nil } +// packageForQualIdent returns the package p if id is X in a qualified +// identifier p.X; it returns "" otherwise. +// +// Precondition: id is path[0], and the parser did not resolve id to a +// local object. For speed, packageForQualIdent assumes that p is a +// package iff it is the basename of an import path (and not, say, a +// package-level decl in another file or a predeclared identifier). +func packageForQualIdent(path []ast.Node, id *ast.Ident) string { + if sel, ok := path[1].(*ast.SelectorExpr); ok && sel.Sel == id && ast.IsExported(id.Name) { + if pkgid, ok := sel.X.(*ast.Ident); ok && pkgid.Obj == nil { + f := path[len(path)-1].(*ast.File) + for _, imp := range f.Imports { + path, _ := strconv.Unquote(imp.Path.Value) + if imp.Name != nil { + if imp.Name.Name == pkgid.Name { + return path // renaming import + } + } else if pathpkg.Base(path) == pkgid.Name { + return path // ordinary import + } + } + } + } + return "" +} + +// findPackageMember returns the type and position of the declaration of +// pkg.member by loading and parsing the files of that package. +// srcdir is the directory in which the import appears. +func findPackageMember(ctxt *build.Context, fset *token.FileSet, srcdir, pkg, member string) (token.Token, token.Pos, error) { + bp, err := ctxt.Import(pkg, srcdir, 0) + if err != nil { + return 0, token.NoPos, err // no files for package + } + + // TODO(adonovan): opt: parallelize. + for _, fname := range bp.GoFiles { + filename := filepath.Join(bp.Dir, fname) + + // Parse the file, opening it the file via the build.Context + // so that we observe the effects of the -modified flag. + f, _ := buildutil.ParseFile(fset, ctxt, nil, ".", filename, parser.Mode(0)) + if f == nil { + continue + } + + // Find a package-level decl called 'member'. + for _, decl := range f.Decls { + switch decl := decl.(type) { + case *ast.GenDecl: + for _, spec := range decl.Specs { + switch spec := spec.(type) { + case *ast.ValueSpec: + // const or var + for _, id := range spec.Names { + if id.Name == member { + return decl.Tok, id.Pos(), nil + } + } + case *ast.TypeSpec: + if spec.Name.Name == member { + return token.TYPE, spec.Name.Pos(), nil + } + } + } + case *ast.FuncDecl: + if decl.Recv == nil && decl.Name.Name == member { + return token.FUNC, decl.Name.Pos(), nil + } + } + } + } + + return 0, token.NoPos, fmt.Errorf("couldn't find declaration of %s in %q", member, pkg) +} + type definitionResult struct { pos token.Pos // (nonzero) location of definition descr string // description of object it denotes diff --git a/cmd/guru/guru_test.go b/cmd/guru/guru_test.go index 96374640..b1cb335a 100644 --- a/cmd/guru/guru_test.go +++ b/cmd/guru/guru_test.go @@ -160,13 +160,18 @@ func doQuery(out io.Writer, q *query, json bool) { buildContext.GOPATH = "testdata" pkg := filepath.Dir(strings.TrimPrefix(q.filename, "testdata/src/")) + gopathAbs, _ := filepath.Abs(buildContext.GOPATH) + var outputMu sync.Mutex // guards out, jsons var jsons []string output := func(fset *token.FileSet, qr guru.QueryResult) { outputMu.Lock() defer outputMu.Unlock() if json { - jsons = append(jsons, string(qr.JSON(fset))) + jsonstr := string(qr.JSON(fset)) + // Sanitize any absolute filenames that creep in. + jsonstr = strings.Replace(jsonstr, gopathAbs, "$GOPATH", -1) + jsons = append(jsons, jsonstr) } else { // suppress position information qr.PrintPlain(func(_ interface{}, format string, args ...interface{}) { @@ -226,6 +231,7 @@ func TestGuru(t *testing.T) { // TODO(adonovan): most of these are very similar; combine them. "testdata/src/calls-json/main.go", "testdata/src/peers-json/main.go", + "testdata/src/definition-json/main.go", "testdata/src/describe-json/main.go", "testdata/src/implements-json/main.go", "testdata/src/implements-methods-json/main.go", diff --git a/cmd/guru/testdata/src/definition-json/main.go b/cmd/guru/testdata/src/definition-json/main.go new file mode 100644 index 00000000..ba13b6ba --- /dev/null +++ b/cmd/guru/testdata/src/definition-json/main.go @@ -0,0 +1,44 @@ +package definition + +// Tests of 'definition' query, -json output. +// See go.tools/guru/guru_test.go for explanation. +// See definition.golden for expected query results. + +// TODO(adonovan): test: selection of member of same package defined in another file. + +import ( + "lib" + lib2 "lib" + "nosuchpkg" +) + +func main() { + var _ int // @definition builtin "int" + + var _ undef // @definition lexical-undef "undef" + var x lib.T // @definition lexical-pkgname "lib" + f() // @definition lexical-func "f" + print(x) // @definition lexical-var "x" + if x := ""; x == "" { // @definition lexical-shadowing "x" + } + + var _ lib.Type // @definition qualified-type "Type" + var _ lib.Func // @definition qualified-func "Func" + var _ lib.Var // @definition qualified-var "Var" + var _ lib.Const // @definition qualified-const "Const" + var _ lib2.Type // @definition qualified-type-renaming "Type" + var _ lib.Nonesuch // @definition qualified-nomember "Nonesuch" + var _ nosuchpkg.T // @definition qualified-nopkg "nosuchpkg" + + var u U + print(u.field) // @definition select-field "field" + u.method() // @definition select-method "method" +} + +func f() + +type T struct{ field int } + +func (T) method() + +type U struct{ T } diff --git a/cmd/guru/testdata/src/definition-json/main.golden b/cmd/guru/testdata/src/definition-json/main.golden new file mode 100644 index 00000000..c2e1349c --- /dev/null +++ b/cmd/guru/testdata/src/definition-json/main.golden @@ -0,0 +1,67 @@ +-------- @definition builtin -------- + +Error: int is built in +-------- @definition lexical-undef -------- + +Error: no object for identifier +-------- @definition lexical-pkgname -------- +{ + "objpos": "testdata/src/definition-json/main.go:10:2", + "desc": "package lib" +} +-------- @definition lexical-func -------- +{ + "objpos": "$GOPATH/src/definition-json/main.go:38:6", + "desc": "func f" +} +-------- @definition lexical-var -------- +{ + "objpos": "$GOPATH/src/definition-json/main.go:19:6", + "desc": "var x" +} +-------- @definition lexical-shadowing -------- +{ + "objpos": "$GOPATH/src/definition-json/main.go:22:5", + "desc": "var x" +} +-------- @definition qualified-type -------- +{ + "objpos": "testdata/src/lib/lib.go:3:6", + "desc": "type lib.Type" +} +-------- @definition qualified-func -------- +{ + "objpos": "testdata/src/lib/lib.go:9:6", + "desc": "func lib.Func" +} +-------- @definition qualified-var -------- +{ + "objpos": "testdata/src/lib/lib.go:14:5", + "desc": "var lib.Var" +} +-------- @definition qualified-const -------- +{ + "objpos": "testdata/src/lib/lib.go:12:7", + "desc": "const lib.Const" +} +-------- @definition qualified-type-renaming -------- +{ + "objpos": "testdata/src/lib/lib.go:3:6", + "desc": "type lib.Type" +} +-------- @definition qualified-nomember -------- + +Error: couldn't find declaration of Nonesuch in "lib" +-------- @definition qualified-nopkg -------- + +Error: no object for identifier +-------- @definition select-field -------- +{ + "objpos": "testdata/src/definition-json/main.go:40:16", + "desc": "field field int" +} +-------- @definition select-method -------- +{ + "objpos": "testdata/src/definition-json/main.go:42:10", + "desc": "func (T).method()" +} diff --git a/cmd/guru/testdata/src/referrers-json/main.golden b/cmd/guru/testdata/src/referrers-json/main.golden index 3fcd288a..9c3ee98b 100644 --- a/cmd/guru/testdata/src/referrers-json/main.golden +++ b/cmd/guru/testdata/src/referrers-json/main.golden @@ -2,6 +2,39 @@ { "desc": "package lib" } +{ + "package": "definition-json", + "refs": [ + { + "pos": "testdata/src/definition-json/main.go:19:8", + "text": "\tvar x lib.T // @definition lexical-pkgname \"lib\"" + }, + { + "pos": "testdata/src/definition-json/main.go:25:8", + "text": "\tvar _ lib.Type // @definition qualified-type \"Type\"" + }, + { + "pos": "testdata/src/definition-json/main.go:26:8", + "text": "\tvar _ lib.Func // @definition qualified-func \"Func\"" + }, + { + "pos": "testdata/src/definition-json/main.go:27:8", + "text": "\tvar _ lib.Var // @definition qualified-var \"Var\"" + }, + { + "pos": "testdata/src/definition-json/main.go:28:8", + "text": "\tvar _ lib.Const // @definition qualified-const \"Const\"" + }, + { + "pos": "testdata/src/definition-json/main.go:29:8", + "text": "\tvar _ lib2.Type // @definition qualified-type-renaming \"Type\"" + }, + { + "pos": "testdata/src/definition-json/main.go:30:8", + "text": "\tvar _ lib.Nonesuch // @definition qualified-nomember \"Nonesuch\"" + } + ] +} { "package": "describe", "refs": [ diff --git a/cmd/guru/testdata/src/referrers/main.golden b/cmd/guru/testdata/src/referrers/main.golden index 044c54c7..b3da0376 100644 --- a/cmd/guru/testdata/src/referrers/main.golden +++ b/cmd/guru/testdata/src/referrers/main.golden @@ -13,6 +13,13 @@ references to package lib var v lib.Type = lib.Const // @referrers ref-package "lib" var v lib.Type = lib.Const // @referrers ref-package "lib" var v lib.Type = lib.Const // @referrers ref-package "lib" + var x lib.T // @definition lexical-pkgname "lib" + var _ lib.Type // @definition qualified-type "Type" + var _ lib.Func // @definition qualified-func "Func" + var _ lib.Var // @definition qualified-var "Var" + var _ lib.Const // @definition qualified-const "Const" + var _ lib2.Type // @definition qualified-type-renaming "Type" + var _ lib.Nonesuch // @definition qualified-nomember "Nonesuch" var _ lib.Outer // @describe lib-outer "Outer" const c = lib.Const // @describe ref-const "Const" lib.Func() // @describe ref-func "Func"