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 <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/golang/definition.go b/gopls/internal/golang/definition.go
index 6184e29..8b1adbf 100644
--- a/gopls/internal/golang/definition.go
+++ b/gopls/internal/golang/definition.go
@@ -103,12 +103,12 @@
// 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
}
@@ -116,29 +116,59 @@
}
// 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 @@
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 @@
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
diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go
index 5e91228..1291f64 100644
--- a/gopls/internal/golang/hover.go
+++ b/gopls/internal/golang/hover.go
@@ -132,6 +132,41 @@
//
// 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,13 +627,16 @@
}, 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:
@@ -606,17 +644,17 @@
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.
diff --git a/gopls/internal/test/integration/misc/hover_test.go b/gopls/internal/test/integration/misc/hover_test.go
index 2d5687e..3526a93 100644
--- a/gopls/internal/test/integration/misc/hover_test.go
+++ b/gopls/internal/test/integration/misc/hover_test.go
@@ -10,6 +10,7 @@
"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 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)
+ }
+ }
+ })
+}
