From 63859f3815cb436d25749575cb14aef1153e5634 Mon Sep 17 00:00:00 2001 From: Muir Manders Date: Fri, 10 May 2019 14:32:25 +0000 Subject: [PATCH] internal/lsp: add definition support for packages Now the "type" of a *ast.PkgName is the package it points to. Of course, a package is not a real types.Type, but we can still jump you there. We have to pick one of the package's files, so we choose the longest one, hoping it is the most interesting. Similarly, the "definition" of an *ast.ImportSpec is the package being imported. I also added a nil check for the package in SignatureHelp. This panics for me occasionally. Change-Id: Ide4640530a28bcec9da6de36723eb7f0e4cc941c GitHub-Last-Rev: 8190baa0b908065db5b53f236de03d2f3bff39b5 GitHub-Pull-Request: golang/tools#92 Reviewed-on: https://go-review.googlesource.com/c/tools/+/174081 Run-TryBot: Rebecca Stambler Reviewed-by: Rebecca Stambler --- internal/lsp/cache/pkg.go | 10 ++++ internal/lsp/source/hover.go | 8 +-- internal/lsp/source/identifier.go | 73 ++++++++++++++++++++--- internal/lsp/source/signature_help.go | 2 +- internal/lsp/source/view.go | 1 + internal/lsp/testdata/foo/foo.go | 2 +- internal/lsp/testdata/godef/b/b.go | 9 ++- internal/lsp/testdata/godef/b/b.go.golden | 4 ++ internal/lsp/tests/tests.go | 2 +- 9 files changed, 95 insertions(+), 16 deletions(-) diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index a1d1a305..64a11fbb 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -155,3 +155,13 @@ func (pkg *Package) GetTypesSizes() types.Sizes { func (pkg *Package) IsIllTyped() bool { return pkg.types == nil && pkg.typesInfo == nil } + +func (pkg *Package) GetImport(pkgPath string) source.Package { + imported := pkg.imports[pkgPath] + // Be careful not to return a nil pointer because that still satisfies the + // interface. + if imported != nil { + return imported + } + return nil +} diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index 375b2cbf..fb9a194e 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -5,13 +5,13 @@ package source import ( - "bytes" "context" "fmt" "go/ast" "go/format" "go/token" "go/types" + "strings" ) // formatter returns the a hover value formatted with its documentation. @@ -23,9 +23,9 @@ func (i *IdentifierInfo) Hover(ctx context.Context, qf types.Qualifier, enhanced pkg := i.File.GetPackage(ctx) qf = qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()) } - b := bytes.NewBuffer(nil) + var b strings.Builder f := func(x interface{}, c *ast.CommentGroup) (string, error) { - return writeHover(x, i.File.GetFileSet(ctx), b, c, markdownSupported, qf) + return writeHover(x, i.File.GetFileSet(ctx), &b, c, markdownSupported, qf) } obj := i.Declaration.Object // TODO(rstambler): Remove this configuration when hover behavior is stable. @@ -113,7 +113,7 @@ func formatVar(node ast.Spec, obj types.Object, f formatter) (string, error) { } // writeHover writes the hover for a given node and its documentation. -func writeHover(x interface{}, fset *token.FileSet, b *bytes.Buffer, c *ast.CommentGroup, markdownSupported bool, qf types.Qualifier) (string, error) { +func writeHover(x interface{}, fset *token.FileSet, b *strings.Builder, c *ast.CommentGroup, markdownSupported bool, qf types.Qualifier) (string, error) { if c != nil { b.WriteString(c.Text()) b.WriteRune('\n') diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index a76b24b6..aaf4696b 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -10,6 +10,7 @@ import ( "go/ast" "go/token" "go/types" + "strconv" "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/internal/span" @@ -54,19 +55,25 @@ func Identifier(ctx context.Context, v View, f File, pos token.Pos) (*Identifier func identifier(ctx context.Context, v View, f File, pos token.Pos) (*IdentifierInfo, error) { fAST := f.GetAST(ctx) pkg := f.GetPackage(ctx) - if pkg == nil { - return nil, fmt.Errorf("no package for %s", f.URI()) - } - if pkg.IsIllTyped() { + if pkg == nil || pkg.IsIllTyped() { return nil, fmt.Errorf("package for %s is ill typed", f.URI()) } + path, _ := astutil.PathEnclosingInterval(fAST, pos, pos) - result := &IdentifierInfo{ - File: f, - } if path == nil { return nil, fmt.Errorf("can't find node enclosing position") } + + // Handle import specs first because they can contain *ast.Idents, and + // we don't want the default *ast.Ident behavior below. + if result, err := checkImportSpec(f, fAST, pkg, pos); result != nil || err != nil { + return result, err + } + + result := &IdentifierInfo{ + File: f, + } + switch node := path[0].(type) { case *ast.Ident: result.ident = node @@ -121,6 +128,58 @@ func identifier(ctx context.Context, v View, f File, pos token.Pos) (*Identifier return result, nil } +func checkImportSpec(f File, fAST *ast.File, pkg Package, pos token.Pos) (*IdentifierInfo, error) { + // Check if pos is in an *ast.ImportSpec. + for _, imp := range fAST.Imports { + if imp.Pos() <= pos && pos < imp.End() { + pkgPath, err := strconv.Unquote(imp.Path.Value) + if err != nil { + return nil, fmt.Errorf("import path not quoted: %s (%v)", imp.Path.Value, err) + } + + result := &IdentifierInfo{ + File: f, + Name: pkgPath, + Range: span.NewRange(f.View().FileSet(), imp.Pos(), imp.End()), + } + + // Consider the definition of an import spec to be the imported package. + result.Declaration.Range, err = importedPkg(f.View(), pkg, pkgPath) + if err != nil { + return nil, err + } + + return result, nil + } + } + + return nil, nil +} + +func importedPkg(v View, pkg Package, importPath string) (span.Range, error) { + otherPkg := pkg.GetImport(importPath) + if otherPkg == nil { + return span.Range{}, fmt.Errorf("no import for %q", importPath) + } + if otherPkg.GetSyntax() == nil { + return span.Range{}, fmt.Errorf("no syntax for for %q", importPath) + } + + // Heuristic: Jump to the longest file of the package, assuming it's the most "interesting." + // TODO: Consider alternative approaches, if necessary. + var longest *ast.File + for _, astFile := range otherPkg.GetSyntax() { + if longest == nil || astFile.End()-astFile.Pos() > longest.End()-longest.Pos() { + longest = astFile + } + } + if longest == nil { + return span.Range{}, fmt.Errorf("package %q has no files", importPath) + } + + return span.NewRange(v.FileSet(), longest.Name.Pos(), longest.Name.End()), nil +} + func typeToObject(typ types.Type) types.Object { switch typ := typ.(type) { case *types.Named: diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 44872fab..1d9f81f4 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -28,7 +28,7 @@ type ParameterInformation struct { func SignatureHelp(ctx context.Context, f File, pos token.Pos) (*SignatureInformation, error) { fAST := f.GetAST(ctx) pkg := f.GetPackage(ctx) - if pkg.IsIllTyped() { + if pkg == nil || pkg.IsIllTyped() { return nil, fmt.Errorf("package for %s is ill typed", f.URI()) } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 6661696d..13e8d741 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -59,6 +59,7 @@ type Package interface { GetTypesSizes() types.Sizes IsIllTyped() bool GetActionGraph(ctx context.Context, a *analysis.Analyzer) (*Action, error) + GetImport(pkgPath string) Package } // TextEdit represents a change to a section of a document. diff --git a/internal/lsp/testdata/foo/foo.go b/internal/lsp/testdata/foo/foo.go index ab588f1d..0e33467f 100644 --- a/internal/lsp/testdata/foo/foo.go +++ b/internal/lsp/testdata/foo/foo.go @@ -1,4 +1,4 @@ -package foo +package foo //@mark(PackageFoo, "foo") type StructFoo struct { //@item(StructFoo, "StructFoo", "struct{...}", "struct") Value int //@item(Value, "Value", "int", "field") diff --git a/internal/lsp/testdata/godef/b/b.go b/internal/lsp/testdata/godef/b/b.go index 51d5aaf0..a881d1a6 100644 --- a/internal/lsp/testdata/godef/b/b.go +++ b/internal/lsp/testdata/godef/b/b.go @@ -1,6 +1,9 @@ package b -import "golang.org/x/tools/internal/lsp/godef/a" +import ( + myFoo "golang.org/x/tools/internal/lsp/foo" //@godef("foo", PackageFoo),godef("myFoo", PackageFoo) + "golang.org/x/tools/internal/lsp/godef/a" //@mark(AImport, "\"") +) type S1 struct { //@S1 F1 int //@mark(S1F1, "F1") @@ -11,7 +14,7 @@ type S1 struct { //@S1 type S2 struct { //@S2 F1 string //@mark(S2F1, "F1") F2 int //@mark(S2F2, "F2") - *a.A //@godef("A", A) + *a.A //@godef("A", A),godef("a",AImport) } type S3 struct { @@ -27,4 +30,6 @@ func Bar() { _ = x.F1 //@godef("F1", S1F1) _ = x.F2 //@godef("F2", S2F2) _ = x.S2.F1 //@godef("F1", S2F1) + + var _ *myFoo.StructFoo } diff --git a/internal/lsp/testdata/godef/b/b.go.golden b/internal/lsp/testdata/godef/b/b.go.golden index 6a89f263..5fae4270 100644 --- a/internal/lsp/testdata/godef/b/b.go.golden +++ b/internal/lsp/testdata/godef/b/b.go.golden @@ -1,5 +1,9 @@ -- A-hover -- type a.A string +-- AImport-hover -- +package a ("golang.org/x/tools/internal/lsp/godef/a") +-- PackageFoo-hover -- + -- S1-hover -- type S1 struct{F1 int; S2; a.A} -- S1F1-hover -- diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index d810f045..326c79f2 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -31,7 +31,7 @@ const ( ExpectedCompletionsCount = 85 ExpectedDiagnosticsCount = 17 ExpectedFormatCount = 5 - ExpectedDefinitionsCount = 21 + ExpectedDefinitionsCount = 24 ExpectedTypeDefinitionsCount = 2 ExpectedHighlightsCount = 2 ExpectedSymbolsCount = 1