diff --git a/gopls/internal/regtest/misc/hover_test.go b/gopls/internal/regtest/misc/hover_test.go index 79e60e2751..1442178034 100644 --- a/gopls/internal/regtest/misc/hover_test.go +++ b/gopls/internal/regtest/misc/hover_test.go @@ -26,6 +26,10 @@ type Mixed struct { Exported int unexported string } + +func printMixed(m Mixed) { + println(m) +} ` const mod = ` -- go.mod -- @@ -35,7 +39,7 @@ 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 h1:3DlrFfd3OsEen7FnCHfqtnJvjBZ8ZFKmrD/+HjpdJj0= golang.org/x/structs v1.0.0/go.mod h1:47gkSIdo5AaQaWJS0upVORsxfEr1LL1MWv9dmYF3iq4= -- main.go -- package main @@ -51,9 +55,16 @@ func main() { 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 d3be0989ef..10fb5417ae 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -98,7 +98,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, 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 @@ func objectString(obj types.Object, qf types.Qualifier, inferred *types.Signatur } // 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 @@ func HoverInfo(ctx context.Context, s Snapshot, pkg Package, obj types.Object, n 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 { @@ -396,14 +426,6 @@ func formatGenDecl(node *ast.GenDecl, spec ast.Spec, obj types.Object, typ types return formatGenDecl(node, spec, obj, typ.Underlying()) } } - 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 ee8684bdee..36eacf44eb 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -57,10 +57,11 @@ type Declaration struct { // 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 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa } // 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 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, pgf *Pa 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 @@ func fullSpec(snapshot Snapshot, obj types.Object, pkg Package) (ast.Spec, error } } 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