internal/lsp/source: improve logic for finding full syntax in hover
When enriching identifier info with full syntax, it's cleaner to find
the enclosing decl. Use the full decl in hover if we were unable to find
a node in the original type-checked package.
Update the regtest to exercise hovering in a non-workspace package.
Updates golang/go#46158
Change-Id: Ic1772a38fb1758fb57a09da9483a8853cc5498f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/333690
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/misc/hover_test.go b/gopls/internal/regtest/misc/hover_test.go
index 79e60e2..1442178 100644
--- a/gopls/internal/regtest/misc/hover_test.go
+++ b/gopls/internal/regtest/misc/hover_test.go
@@ -26,6 +26,10 @@
Exported int
unexported string
}
+
+func printMixed(m Mixed) {
+ println(m)
+}
`
const mod = `
-- go.mod --
@@ -35,7 +39,7 @@
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 h1:3DlrFfd3OsEen7FnCHfqtnJvjBZ8ZFKmrD/+HjpdJj0=
golang.org/x/structs v1.0.0/go.mod h1:47gkSIdo5AaQaWJS0upVORsxfEr1LL1MWv9dmYF3iq4=
-- main.go --
package main
@@ -51,9 +55,16 @@
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"))
+ mixedPos := env.RegexpSearch("main.go", "Mixed")
+ got, _ := env.Hover("main.go", mixedPos)
if !strings.Contains(got.Value, "unexported") {
- t.Errorf("Hover: missing expected field 'unexported'. Got:\n%q", got.Value)
+ t.Errorf("Workspace hover: missing expected field 'unexported'. Got:\n%q", got.Value)
+ }
+ cacheFile, _ := env.GoToDefinition("main.go", mixedPos)
+ argPos := env.RegexpSearch(cacheFile, "printMixed.*(Mixed)")
+ got, _ = env.Hover(cacheFile, argPos)
+ if !strings.Contains(got.Value, "unexported") {
+ t.Errorf("Non-workspace hover: missing expected field 'unexported'. Got:\n%q", got.Value)
}
})
}
diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go
index d3be098..10fb541 100644
--- a/internal/lsp/source/hover.go
+++ b/internal/lsp/source/hover.go
@@ -98,7 +98,7 @@
defer done()
fset := i.Snapshot.FileSet()
- h, err := HoverInfo(ctx, i.Snapshot, i.pkg, i.Declaration.obj, i.Declaration.node, i.Declaration.fullSpec)
+ h, err := HoverInfo(ctx, i.Snapshot, i.pkg, i.Declaration.obj, i.Declaration.node, i.Declaration.fullDecl)
if err != nil {
return nil, err
}
@@ -284,15 +284,28 @@
}
// 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, spec ast.Spec) (*HoverInformation, error) {
+// object. node should be the actual node used in type checking, while fullNode
+// could be a separate node with more complete syntactic information.
+func HoverInfo(ctx context.Context, s Snapshot, pkg Package, obj types.Object, pkgNode ast.Node, fullDecl ast.Decl) (*HoverInformation, error) {
var info *HoverInformation
+ // This is problematic for a number of reasons. We really need to have a more
+ // general mechanism to validate the coherency of AST with type information,
+ // but absent that we must do our best to ensure that we don't use fullNode
+ // when we actually need the node that was type checked.
+ //
+ // pkgNode may be nil, if it was eliminated from the type-checked syntax. In
+ // that case, use fullDecl if available.
+ node := pkgNode
+ if node == nil && fullDecl != nil {
+ node = fullDecl
+ }
+
switch node := node.(type) {
case *ast.Ident:
// The package declaration.
for _, f := range pkg.GetSyntax() {
- if f.Name == node {
+ if f.Name == pkgNode {
info = &HoverInformation{comment: f.Doc}
}
}
@@ -316,6 +329,23 @@
case *ast.GenDecl:
switch obj := obj.(type) {
case *types.TypeName, *types.Var, *types.Const, *types.Func:
+ // Always use the full declaration here if we have it, because the
+ // dependent code doesn't rely on pointer identity. This is fragile.
+ if d, _ := fullDecl.(*ast.GenDecl); d != nil {
+ node = d
+ }
+ // obj may not have been produced by type checking the AST containing
+ // node, so we need to be careful about using token.Pos.
+ tok := s.FileSet().File(obj.Pos())
+ offset := tok.Offset(obj.Pos())
+ tok2 := s.FileSet().File(node.Pos())
+ var spec ast.Spec
+ for _, s := range node.Specs {
+ if tok2.Offset(s.Pos()) <= offset && offset <= tok2.Offset(s.End()) {
+ spec = s
+ break
+ }
+ }
var err error
info, err = formatGenDecl(node, spec, obj, obj.Type())
if err != nil {
@@ -397,14 +427,6 @@
}
}
if spec == nil {
- for _, s := range node.Specs {
- if s.Pos() <= obj.Pos() && obj.Pos() <= s.End() {
- spec = s
- break
- }
- }
- }
- if spec == nil {
return nil, errors.Errorf("no spec for node %v at position %v", node, obj.Pos())
}
diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go
index ee8684b..36eacf4 100644
--- a/internal/lsp/source/identifier.go
+++ b/internal/lsp/source/identifier.go
@@ -57,10 +57,11 @@
// The typechecked node.
node ast.Node
- // Optional: the fully parsed spec, to be used for formatting in cases where
+
+ // Optional: the fully parsed node, 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
+ fullDecl ast.Decl
// The typechecked object.
obj types.Object
@@ -290,8 +291,7 @@
}
// 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 {
+ if result.Declaration.fullDecl, err = fullNode(snapshot, result.Declaration.obj, declPkg); err != nil {
return nil, err
}
typ := pkg.GetTypesInfo().TypeOf(result.ident)
@@ -314,10 +314,10 @@
return result, nil
}
-// fullSpec tries to extract the full spec corresponding to obj's declaration.
+// fullNode 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) {
+func fullNode(snapshot Snapshot, obj types.Object, pkg Package) (ast.Decl, error) {
// declaration in a different package... make sure we have full AST information.
tok := snapshot.FileSet().File(obj.Pos())
uri := span.URIFromPath(tok.Name())
@@ -338,9 +338,9 @@
}
}
path, _ := astutil.PathEnclosingInterval(file, pos, pos)
- if len(path) > 1 {
- if spec, _ := path[1].(*ast.TypeSpec); spec != nil {
- return spec, nil
+ for _, n := range path {
+ if decl, ok := n.(ast.Decl); ok {
+ return decl, nil
}
}
return nil, nil
