From 6da3d7ace9da93514c50281436bcf0fb66d50f18 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 17 May 2021 16:24:09 -0400 Subject: [PATCH] internal/lsp/source: re-parse if needed when collecting identifier info With the new ParseExported logic, we can lose some unexported fields on exported structs. This can lead to misleading or malformatted hover information. Fix this by ensuring we always extract the Spec from a full parse. Since this path is only hit via user-initiated requests (and should only be hit ~once per request), it is preferable to do the parse on-demand rather than parse via the cache and risk pinning the full AST for the remaining duration of the session. For golang/go#46158 Change-Id: Ib3eb61c3f75e16199eb492e3e129ba875bd8553e Reviewed-on: https://go-review.googlesource.com/c/tools/+/320550 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- .../regtest/codelens/codelens_test.go | 6 +- gopls/internal/regtest/misc/hover_test.go | 58 +++++++++++++++++++ internal/lsp/source/completion/format.go | 2 +- internal/lsp/source/hover.go | 51 ++++++++-------- internal/lsp/source/identifier.go | 58 +++++++++++++++++-- internal/lsp/source/references.go | 2 +- internal/lsp/source/signature_help.go | 2 +- 7 files changed, 146 insertions(+), 33 deletions(-) create mode 100644 gopls/internal/regtest/misc/hover_test.go diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index fe351ea5290..a2a1ae45589 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/gopls/internal/regtest/codelens/codelens_test.go @@ -90,7 +90,7 @@ go 1.12 package hi var Goodbye error - -- golang.org/x/hello@v1.2.3/go.mod -- +-- golang.org/x/hello@v1.2.3/go.mod -- module golang.org/x/hello go 1.12 @@ -108,8 +108,8 @@ go 1.14 require golang.org/x/hello v1.2.3 -- go.sum -- -golang.org/x/hello v1.2.3 h1:jOtNXLsiCuLzU6KM3wRHidpc29IxcKpofHZiOW1hYKA= -golang.org/x/hello v1.2.3/go.mod h1:X79D30QqR94cGK8aIhQNhCZLq4mIr5Gimj5qekF08rY= +golang.org/x/hello v1.2.3 h1:7Wesfkx/uBd+eFgPrq0irYj/1XfmbvLV8jZ/W7C2Dwg= +golang.org/x/hello v1.2.3/go.mod h1:OgtlzsxVMUUdsdQCIDYgaauCTH47B8T8vofouNJfzgY= -- main.go -- package main diff --git a/gopls/internal/regtest/misc/hover_test.go b/gopls/internal/regtest/misc/hover_test.go new file mode 100644 index 00000000000..7a361f9150f --- /dev/null +++ b/gopls/internal/regtest/misc/hover_test.go @@ -0,0 +1,58 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package misc + +import ( + "strings" + "testing" + + . "golang.org/x/tools/internal/lsp/regtest" +) + +func TestHoverUnexported(t *testing.T) { + const proxy = ` +-- golang.org/x/structs@v1.0.0/go.mod -- +module golang.org/x/structs + +go 1.12 + +-- golang.org/x/structs@v1.0.0/types.go -- +package structs + +type Mixed struct { + Exported int + unexported string +} +` + const mod = ` +-- go.mod -- +module mod.com + +go 1.12 + +require golang.org/x/structs v1.0.0 +-- go.sum -- +golang.org/x/structs v1.0.0 h1:oxD5q25qV458xBbXf5+QX+Johgg71KFtwuJzt145c9A= +golang.org/x/structs v1.0.0/go.mod h1:47gkSIdo5AaQaWJS0upVORsxfEr1LL1MWv9dmYF3iq4= +-- main.go -- +package main + +import "golang.org/x/structs" + +func main() { + var _ structs.Mixed +} +` + // TODO: use a nested workspace folder here. + WithOptions( + ProxyFiles(proxy), + ).Run(t, mod, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + got, _ := env.Hover("main.go", env.RegexpSearch("main.go", "Mixed")) + if !strings.Contains(got.Value, "unexported") { + t.Errorf("Hover: missing expected field 'unexported'. Got:\n%q", got.Value) + } + }) +} diff --git a/internal/lsp/source/completion/format.go b/internal/lsp/source/completion/format.go index 1d021074399..985b79fd30c 100644 --- a/internal/lsp/source/completion/format.go +++ b/internal/lsp/source/completion/format.go @@ -223,7 +223,7 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e if err != nil { return CompletionItem{}, err } - hover, err := source.HoverInfo(ctx, c.snapshot, pkg, obj, decl) + hover, err := source.HoverInfo(ctx, c.snapshot, pkg, obj, decl, nil) if err != nil { event.Error(ctx, "failed to find Hover", err, tag.URI.Of(uri)) return item, nil diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index cd4e6e8d875..ee38dd7fe22 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -97,7 +97,7 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverInformation, defer done() fset := i.Snapshot.FileSet() - h, err := HoverInfo(ctx, i.Snapshot, i.pkg, i.Declaration.obj, i.Declaration.node) + h, err := HoverInfo(ctx, i.Snapshot, i.pkg, i.Declaration.obj, i.Declaration.node, i.Declaration.fullSpec) if err != nil { return nil, err } @@ -260,7 +260,7 @@ func objectString(obj types.Object, qf types.Qualifier) string { // HoverInfo returns a HoverInformation struct for an ast node and its type // object. -func HoverInfo(ctx context.Context, s Snapshot, pkg Package, obj types.Object, node ast.Node) (*HoverInformation, error) { +func HoverInfo(ctx context.Context, s Snapshot, pkg Package, obj types.Object, node ast.Node, spec ast.Spec) (*HoverInformation, error) { var info *HoverInformation switch node := node.(type) { @@ -292,7 +292,7 @@ func HoverInfo(ctx context.Context, s Snapshot, pkg Package, obj types.Object, n switch obj := obj.(type) { case *types.TypeName, *types.Var, *types.Const, *types.Func: var err error - info, err = formatGenDecl(node, obj, obj.Type()) + info, err = formatGenDecl(node, spec, obj, obj.Type()) if err != nil { return nil, err } @@ -364,18 +364,19 @@ func isFunctionParam(obj types.Object, node *ast.FuncDecl) bool { return false } -func formatGenDecl(node *ast.GenDecl, obj types.Object, typ types.Type) (*HoverInformation, error) { +func formatGenDecl(node *ast.GenDecl, spec ast.Spec, obj types.Object, typ types.Type) (*HoverInformation, error) { if _, ok := typ.(*types.Named); ok { switch typ.Underlying().(type) { case *types.Interface, *types.Struct: - return formatGenDecl(node, obj, typ.Underlying()) + return formatGenDecl(node, spec, obj, typ.Underlying()) } } - var spec ast.Spec - for _, s := range node.Specs { - if s.Pos() <= obj.Pos() && obj.Pos() <= s.End() { - spec = s - break + if spec == nil { + for _, s := range node.Specs { + if s.Pos() <= obj.Pos() && obj.Pos() <= s.End() { + spec = s + break + } } } if spec == nil { @@ -390,19 +391,7 @@ func formatGenDecl(node *ast.GenDecl, obj types.Object, typ types.Type) (*HoverI // Handle types. switch spec := spec.(type) { case *ast.TypeSpec: - comment := spec.Doc - if comment == nil { - comment = node.Doc - } - if comment == nil { - comment = spec.Comment - } - return &HoverInformation{ - source: spec.Type, - comment: comment, - typeName: spec.Name.Name, - isTypeAlias: spec.Assign.IsValid(), - }, nil + return formatTypeSpec(spec, node), nil case *ast.ValueSpec: return &HoverInformation{source: spec, comment: spec.Doc}, nil case *ast.ImportSpec: @@ -411,6 +400,22 @@ func formatGenDecl(node *ast.GenDecl, obj types.Object, typ types.Type) (*HoverI return nil, errors.Errorf("unable to format spec %v (%T)", spec, spec) } +func formatTypeSpec(spec *ast.TypeSpec, decl *ast.GenDecl) *HoverInformation { + comment := spec.Doc + if comment == nil && decl != nil { + comment = decl.Doc + } + if comment == nil { + comment = spec.Comment + } + return &HoverInformation{ + source: spec.Type, + comment: comment, + typeName: spec.Name.Name, + isTypeAlias: spec.Assign.IsValid(), + } +} + func formatVar(node ast.Spec, obj types.Object, decl *ast.GenDecl) *HoverInformation { var fieldList *ast.FieldList switch spec := node.(type) { diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 41534d070af..77f496455c4 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -8,13 +8,16 @@ import ( "context" "fmt" "go/ast" + "go/parser" "go/token" "go/types" "sort" "strconv" + "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/span" errors "golang.org/x/xerrors" ) @@ -48,8 +51,16 @@ func (i *IdentifierInfo) IsImport() bool { type Declaration struct { MappedRange []MappedRange - node ast.Node - obj types.Object + + // The typechecked node. + node ast.Node + // Optional: the fully parsed spec, to be used for formatting in cases where + // node has missing information. This could be the case when node was parsed + // in ParseExported mode. + fullSpec ast.Spec + + // The typechecked object. + obj types.Object // typeSwitchImplicit indicates that the declaration is in an implicit // type switch. Its type is the type of the variable on the right-hand @@ -88,7 +99,7 @@ func Identifier(ctx context.Context, snapshot Snapshot, fh FileHandle, pos proto return nil, err } var ident *IdentifierInfo - ident, findErr = findIdentifier(ctx, snapshot, pkg, pgf.File, rng.Start) + ident, findErr = findIdentifier(ctx, snapshot, pkg, pgf, rng.Start) if findErr == nil { return ident, nil } @@ -99,7 +110,8 @@ func Identifier(ctx context.Context, snapshot Snapshot, fh FileHandle, pos proto // ErrNoIdentFound is error returned when no identifer is found at a particular position var ErrNoIdentFound = errors.New("no identifier found") -func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { +func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *ParsedGoFile, pos token.Pos) (*IdentifierInfo, error) { + file := pgf.File // Handle import specs separately, as there is no formal position for a // package declaration. if result, err := importSpec(snapshot, pkg, file, pos); result != nil || err != nil { @@ -269,6 +281,12 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a if result.Declaration.node, err = snapshot.PosToDecl(ctx, declPkg, result.Declaration.obj.Pos()); err != nil { return nil, err } + // Ensure that we have the full declaration, in case the declaration was + // parsed in ParseExported and therefore could be missing information. + result.Declaration.fullSpec, err = fullSpec(snapshot, result.Declaration.obj, declPkg) + if err != nil { + return nil, err + } typ := pkg.GetTypesInfo().TypeOf(result.ident) if typ == nil { return result, nil @@ -287,6 +305,38 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a return result, nil } +// fullSpec tries to extract the full spec corresponding to obj's declaration. +// If the package was not parsed in full, the declaration file will be +// re-parsed to ensure it has complete syntax. +func fullSpec(snapshot Snapshot, obj types.Object, pkg Package) (ast.Spec, error) { + // declaration in a different package... make sure we have full AST information. + tok := snapshot.FileSet().File(obj.Pos()) + uri := span.URIFromPath(tok.Name()) + pgf, err := pkg.File(uri) + if err != nil { + return nil, err + } + file := pgf.File + pos := obj.Pos() + if pgf.Mode != ParseFull { + fset := snapshot.FileSet() + file2, _ := parser.ParseFile(fset, tok.Name(), pgf.Src, parser.AllErrors|parser.ParseComments) + if file2 != nil { + offset := tok.Offset(obj.Pos()) + file = file2 + tok2 := fset.File(file2.Pos()) + pos = tok2.Pos(offset) + } + } + path, _ := astutil.PathEnclosingInterval(file, pos, pos) + if len(path) > 1 { + if spec, _ := path[1].(*ast.TypeSpec); spec != nil { + return spec, nil + } + } + return nil, nil +} + func searchForEnclosing(info *types.Info, path []ast.Node) types.Type { for _, n := range path { switch n := n.(type) { diff --git a/internal/lsp/source/references.go b/internal/lsp/source/references.go index 08ce8078594..a608f504be0 100644 --- a/internal/lsp/source/references.go +++ b/internal/lsp/source/references.go @@ -73,7 +73,7 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i if err != nil { return nil, err } - declIdent, err := findIdentifier(ctx, snapshot, qos[0].pkg, pgf.File, qos[0].obj.Pos()) + declIdent, err := findIdentifier(ctx, snapshot, qos[0].pkg, pgf, qos[0].obj.Pos()) if err != nil { return nil, err } diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 9270787d686..620a8cf4505 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -110,7 +110,7 @@ FindCall: node: node, } decl.MappedRange = append(decl.MappedRange, rng) - d, err := HoverInfo(ctx, snapshot, pkg, decl.obj, decl.node) + d, err := HoverInfo(ctx, snapshot, pkg, decl.obj, decl.node, nil) if err != nil { return nil, 0, err }