gopls/internal/golang: fix hovering from the builtin file

The builtin file represents a psuedo package for builtin documentation.
When hovering over a builtin declared in the builtin file, its
corresponding hover content should be the same as hovering at the call
site.

Fix this by intercepting hover requests originating from the builtin
file. Along the way, fix a bug that jumping to a definition in the
builtin file went to the declaring node, not declaring identifier.

For golang/go#68026

Change-Id: I070ec701cb7e067dd1cbb14a968c063169bb541c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/594795
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Findley <[email protected]>

Author: findleyr

gopls/internal/golang/definition.go

@@ -103,42 +103,72 @@ func Definition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 // builtinDefinition returns the location of the fake source
 // declaration of a built-in in {builtin,unsafe}.go.
 func builtinDefinition(ctx context.Context, snapshot *cache.Snapshot, obj types.Object) ([]protocol.Location, error) {
-	pgf, decl, err := builtinDecl(ctx, snapshot, obj)
+	pgf, ident, err := builtinDecl(ctx, snapshot, obj)
 	if err != nil {
 		return nil, err
 	}
 
-	loc, err := pgf.PosLocation(decl.Pos(), decl.Pos()+token.Pos(len(obj.Name())))
+	loc, err := pgf.NodeLocation(ident)
 	if err != nil {
 		return nil, err
 	}
 	return []protocol.Location{loc}, nil
 }
 
 // builtinDecl returns the parsed Go file and node corresponding to a builtin
-// object, which may be a universe object or part of types.Unsafe.
-func builtinDecl(ctx context.Context, snapshot *cache.Snapshot, obj types.Object) (*parsego.File, ast.Node, error) {
-	// getDecl returns the file-level declaration of name
-	// using legacy (go/ast) object resolution.
-	getDecl := func(file *ast.File, name string) (ast.Node, error) {
+// object, which may be a universe object or part of types.Unsafe, as well as
+// its declaring identifier.
+func builtinDecl(ctx context.Context, snapshot *cache.Snapshot, obj types.Object) (*parsego.File, *ast.Ident, error) {
+	// declaringIdent returns the file-level declaration node (as reported by
+	// ast.Object) and declaring identifier of name using legacy (go/ast) object
+	// resolution.
+	declaringIdent := func(file *ast.File, name string) (ast.Node, *ast.Ident, error) {
 		astObj := file.Scope.Lookup(name)
 		if astObj == nil {
 			// Every built-in should have documentation syntax.
 			// However, it is possible to reach this statement by
 			// commenting out declarations in {builtin,unsafe}.go.
-			return nil, fmt.Errorf("internal error: no object for %s", name)
+			return nil, nil, fmt.Errorf("internal error: no object for %s", name)
 		}
 		decl, ok := astObj.Decl.(ast.Node)
 		if !ok {
-			return nil, bug.Errorf("internal error: no declaration for %s", obj.Name())
+			return nil, nil, bug.Errorf("internal error: no declaration for %s", obj.Name())
 		}
-		return decl, nil
+		var ident *ast.Ident
+		switch node := decl.(type) {
+		case *ast.Field:
+			for _, id := range node.Names {
+				if id.Name == name {
+					ident = id
+				}
+			}
+		case *ast.ValueSpec:
+			for _, id := range node.Names {
+				if id.Name == name {
+					ident = id
+				}
+			}
+		case *ast.TypeSpec:
+			ident = node.Name
+		case *ast.Ident:
+			ident = node
+		case *ast.FuncDecl:
+			ident = node.Name
+		case *ast.ImportSpec, *ast.LabeledStmt, *ast.AssignStmt:
+			// Not reachable for imported objects.
+		default:
+			return nil, nil, bug.Errorf("internal error: unexpected decl type %T", decl)
+		}
+		if ident == nil {
+			return nil, nil, bug.Errorf("internal error: no declaring identifier for %s", obj.Name())
+		}
+		return decl, ident, nil
 	}
 
 	var (
-		pgf  *parsego.File
-		decl ast.Node
-		err  error
+		pgf   *parsego.File
+		ident *ast.Ident
+		err   error
 	)
 	if obj.Pkg() == types.Unsafe {
 		// package "unsafe":
@@ -154,11 +184,13 @@ func builtinDecl(ctx context.Context, snapshot *cache.Snapshot, obj types.Object
 		if err != nil {
 			return nil, nil, err
 		}
+		// TODO(rfindley): treat unsafe symmetrically with the builtin file. Either
+		// pre-parse them both, or look up metadata for both.
 		pgf, err = snapshot.ParseGo(ctx, fh, parsego.Full&^parser.SkipObjectResolution)
 		if err != nil {
 			return nil, nil, err
 		}
-		decl, err = getDecl(pgf.File, obj.Name())
+		_, ident, err = declaringIdent(pgf.File, obj.Name())
 		if err != nil {
 			return nil, nil, err
 		}
@@ -172,23 +204,24 @@ func builtinDecl(ctx context.Context, snapshot *cache.Snapshot, obj types.Object
 
 		if obj.Parent() == types.Universe {
 			// built-in function or type
-			decl, err = getDecl(pgf.File, obj.Name())
+			_, ident, err = declaringIdent(pgf.File, obj.Name())
 			if err != nil {
 				return nil, nil, err
 			}
 		} else if obj.Name() == "Error" {
 			// error.Error method
-			decl, err = getDecl(pgf.File, "error")
+			decl, _, err := declaringIdent(pgf.File, "error")
 			if err != nil {
 				return nil, nil, err
 			}
-			decl = decl.(*ast.TypeSpec).Type.(*ast.InterfaceType).Methods.List[0]
-
+			field := decl.(*ast.TypeSpec).Type.(*ast.InterfaceType).Methods.List[0]
+			ident = field.Names[0]
 		} else {
 			return nil, nil, bug.Errorf("unknown built-in %v", obj)
 		}
 	}
-	return pgf, decl, nil
+
+	return pgf, ident, nil
 }
 
 // referencedObject returns the identifier and object referenced at the

gopls/internal/golang/hover.go

@@ -132,6 +132,41 @@ func Hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, positi
 //
 // TODO(adonovan): strength-reduce file.Handle to protocol.DocumentURI.
 func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp protocol.Position) (protocol.Range, *hoverJSON, error) {
+	// Check for hover inside the builtin file before attempting type checking
+	// below. NarrowestPackageForFile may or may not succeed, depending on
+	// whether this is a GOROOT view, but even if it does succeed the resulting
+	// package will be command-line-arguments package. The user should get a
+	// hover for the builtin object, not the object type checked from the
+	// builtin.go.
+	if snapshot.IsBuiltin(fh.URI()) {
+		pgf, err := snapshot.BuiltinFile(ctx)
+		if err != nil {
+			return protocol.Range{}, nil, err
+		}
+		pos, err := pgf.PositionPos(pp)
+		if err != nil {
+			return protocol.Range{}, nil, err
+		}
+		path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos)
+		if id, ok := path[0].(*ast.Ident); ok {
+			rng, err := pgf.NodeRange(id)
+			if err != nil {
+				return protocol.Range{}, nil, err
+			}
+			var obj types.Object
+			if id.Name == "Error" {
+				obj = types.Universe.Lookup("error").Type().Underlying().(*types.Interface).Method(0)
+			} else {
+				obj = types.Universe.Lookup(id.Name)
+			}
+			if obj != nil {
+				h, err := hoverBuiltin(ctx, snapshot, obj)
+				return rng, h, err
+			}
+		}
+		return protocol.Range{}, nil, nil // no object to hover
+	}
+
 	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
 	if err != nil {
 		return protocol.Range{}, nil, err
@@ -592,31 +627,34 @@ func hoverBuiltin(ctx context.Context, snapshot *cache.Snapshot, obj types.Objec
 		}, nil
 	}
 
-	pgf, node, err := builtinDecl(ctx, snapshot, obj)
+	pgf, ident, err := builtinDecl(ctx, snapshot, obj)
 	if err != nil {
 		return nil, err
 	}
 
-	var comment *ast.CommentGroup
-	path, _ := astutil.PathEnclosingInterval(pgf.File, node.Pos(), node.End())
+	var (
+		comment *ast.CommentGroup
+		decl    ast.Decl
+	)
+	path, _ := astutil.PathEnclosingInterval(pgf.File, ident.Pos(), ident.Pos())
 	for _, n := range path {
 		switch n := n.(type) {
 		case *ast.GenDecl:
 			// Separate documentation and signature.
 			comment = n.Doc
 			node2 := *n
 			node2.Doc = nil
-			node = &node2
+			decl = &node2
 		case *ast.FuncDecl:
 			// Ditto.
 			comment = n.Doc
 			node2 := *n
 			node2.Doc = nil
-			node = &node2
+			decl = &node2
 		}
 	}
 
-	signature := formatNodeFile(pgf.Tok, node)
+	signature := formatNodeFile(pgf.Tok, decl)
 	// Replace fake types with their common equivalent.
 	// TODO(rfindley): we should instead use obj.Type(), which would have the
 	// *actual* types of the builtin call.

gopls/internal/test/integration/misc/hover_test.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"golang.org/x/tools/gopls/internal/protocol"
 	. "golang.org/x/tools/gopls/internal/test/integration"
 	"golang.org/x/tools/gopls/internal/test/integration/fake"
@@ -603,3 +604,59 @@ func main() {
 		}
 	})
 }
+
+func TestHoverBuiltinFile(t *testing.T) {
+	testenv.NeedsGo1Point(t, 21) // uses 'min'
+
+	// This test verifies that hovering in the builtin file provides the same
+	// hover content as hovering over a use of a builtin.
+
+	const src = `
+-- p.go --
+package p
+
+func _() {
+	const (
+		_ = iota
+		_ = true
+	)
+	var (
+		_ any
+		err error = e{} // avoid nil deref warning
+	)
+	_ = err.Error
+	println("Hello")
+	_ = min(1, 2)
+}
+
+// e implements Error, for use above.
+type e struct{}
+func (e) Error() string
+`
+
+	// Test hovering over various builtins with different kinds of declarations.
+	tests := []string{
+		"iota",
+		"true",
+		"any",
+		"error",
+		"Error",
+		"println",
+		"min",
+	}
+
+	Run(t, src, func(t *testing.T, env *Env) {
+		env.OpenFile("p.go")
+		env.AfterChange(NoDiagnostics()) // avoid accidental compiler errors
+
+		for _, builtin := range tests {
+			useLocation := env.RegexpSearch("p.go", builtin)
+			calleeHover, _ := env.Hover(useLocation)
+			declLocation := env.GoToDefinition(useLocation)
+			declHover, _ := env.Hover(declLocation)
+			if diff := cmp.Diff(calleeHover, declHover); diff != "" {
+				t.Errorf("Hover mismatch (-callee hover +decl hover):\n%s", diff)
+			}
+		}
+	})
+}