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 <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go
index fe351ea..a2a1ae4 100644
--- a/gopls/internal/regtest/codelens/codelens_test.go
+++ b/gopls/internal/regtest/codelens/codelens_test.go
@@ -90,7 +90,7 @@
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 @@
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 0000000..7a361f9
--- /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 1d02107..985b79f 100644
--- a/internal/lsp/source/completion/format.go
+++ b/internal/lsp/source/completion/format.go
@@ -223,7 +223,7 @@
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 cd4e6e8..ee38dd7 100644
--- a/internal/lsp/source/hover.go
+++ b/internal/lsp/source/hover.go
@@ -97,7 +97,7 @@
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 @@
// 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 @@
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 @@
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 @@
// 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 @@
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 41534d0..77f4964 100644
--- a/internal/lsp/source/identifier.go
+++ b/internal/lsp/source/identifier.go
@@ -8,13 +8,16 @@
"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 @@
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 @@
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 @@
// 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 @@
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 @@
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 08ce807..a608f50 100644
--- a/internal/lsp/source/references.go
+++ b/internal/lsp/source/references.go
@@ -73,7 +73,7 @@
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 9270787..620a8cf 100644
--- a/internal/lsp/source/signature_help.go
+++ b/internal/lsp/source/signature_help.go
@@ -110,7 +110,7 @@
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
}
