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 <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Rebecca Stambler <[email protected]>

gopls/internal/regtest/codelens/codelens_test.go

@@ -90,7 +90,7 @@ go 1.12
 package hi
 
 var Goodbye error
-	-- golang.org/x/[email protected]/go.mod --
+-- golang.org/x/[email protected]/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
 

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/[email protected]/go.mod --
+module golang.org/x/structs
+
+go 1.12
+
+-- golang.org/x/[email protected]/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)
+		}
+	})
+}

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

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) {

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) {

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
 	}

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
 		}