From 45aeaf7b047b11157ded42272c7dfac5d5544aa5 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 10 Feb 2022 18:17:04 -0500 Subject: [PATCH] internal/lsp/source: improve the heuristics for linkable identifiers We should not offer links to variable or type declarations in function scope. Improve our heuristics to check that the declaration object is actually reachable from the package scope. Also push down handling of private import paths, so that the HoverJSON.importPath field can be removed. Change-Id: I6edb3be3c37f479667c838beb49f97e7167b47a1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/385016 Trust: Robert Findley Run-TryBot: Robert Findley Reviewed-by: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Gopher Robot --- internal/lsp/source/hover.go | 117 ++++++++++-------- .../testdata/godef/hover_generics/hover.go | 9 +- .../godef/hover_generics/hover.go.golden | 31 ++++- .../lsp/testdata/summary_go1.18.txt.golden | 2 +- 4 files changed, 99 insertions(+), 60 deletions(-) diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index a3cbf10b8e..c739b9a4d9 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -63,10 +63,6 @@ type HoverJSON struct { // For example, the "Node" part of "pkg.go.dev/go/ast#Node". LinkAnchor string `json:"linkAnchor"` - // importPath is the import path for the package containing the given - // symbol. - importPath string - // symbolName is the types.Object.Name for the given symbol. symbolName string @@ -89,10 +85,6 @@ func Hover(ctx context.Context, snapshot Snapshot, fh FileHandle, position proto if err != nil { return nil, err } - // See golang/go#36998: don't link to modules matching GOPRIVATE. - if snapshot.View().IsGoPrivatePath(h.importPath) { - h.LinkPath = "" - } hover, err := FormatHover(h, snapshot.View().Options()) if err != nil { return nil, err @@ -314,23 +306,7 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverJSON, error) if obj == nil { return h, nil } - switch obj := obj.(type) { - case *types.PkgName: - h.importPath = obj.Imported().Path() - h.LinkPath = h.importPath - h.symbolName = obj.Name() - if mod, version, ok := moduleAtVersion(h.LinkPath, i); ok { - h.LinkPath = strings.Replace(h.LinkPath, mod, mod+"@"+version, 1) - } - return h, nil - } - if obj.Parent() == types.Universe { - h.importPath = "builtin" - h.LinkPath = h.importPath - h.LinkAnchor = obj.Name() - h.symbolName = h.LinkAnchor - return h, nil - } + // Check if the identifier is test-only (and is therefore not part of a // package's API). This is true if the request originated in a test package, // and if the declaration is also found in the same test package. @@ -339,18 +315,49 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverJSON, error) return h, nil } } - // Don't return links for other unexported types. - if !obj.Exported() { - return h, nil + + h.symbolName, h.LinkPath, h.LinkAnchor = linkData(obj, i.enclosing) + + // See golang/go#36998: don't link to modules matching GOPRIVATE. + // + // The path returned by linkData is an import path. + if i.Snapshot.View().IsGoPrivatePath(h.LinkPath) { + h.LinkPath = "" + } else if mod, version, ok := moduleAtVersion(h.LinkPath, i); ok { + h.LinkPath = strings.Replace(h.LinkPath, mod, mod+"@"+version, 1) } - var recvName string // receiver type name + + return h, nil +} + +// linkData returns the name, import path, and anchor to use in building links +// to obj. +// +// If obj is not visible in documentation, the returned name will be empty. +func linkData(obj types.Object, enclosing *types.TypeName) (name, importPath, anchor string) { + // Package names simply link to the package. + if obj, ok := obj.(*types.PkgName); ok { + return obj.Name(), obj.Imported().Path(), "" + } + + // Builtins link to the special builtin package. + if obj.Parent() == types.Universe { + return obj.Name(), "builtin", obj.Name() + } + + // In all other cases, the object must be exported. + if !obj.Exported() { + return "", "", "" + } + + var recv types.Object // If non-nil, the field or method receiver base. switch obj := obj.(type) { case *types.Var: // If the object is a field, and we have an associated selector // composite literal, or struct, we can determine the link. - if obj.IsField() && i.enclosing != nil { - recvName = i.enclosing.Name() + if obj.IsField() && enclosing != nil { + recv = enclosing } case *types.Func: typ, ok := obj.Type().(*types.Signature) @@ -359,7 +366,7 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverJSON, error) // *Funcs are Signatures. // // TODO(rfindley): given a 'debug' mode, we should panic here. - return nil, fmt.Errorf("BUG: incorrect type %T for *Func", obj.Type()) + return "", "", "" } if r := typ.Recv(); r != nil { if rtyp, _ := Deref(r.Type()).(*types.Named); rtyp != nil { @@ -367,38 +374,46 @@ func HoverIdentifier(ctx context.Context, i *IdentifierInfo) (*HoverJSON, error) // exported (we may have an interface or struct we can link // to). If not, don't show any link. if !rtyp.Obj().Exported() { - if i.enclosing != nil && i.enclosing.Exported() { - recvName = i.enclosing.Name() + if enclosing != nil { + recv = enclosing } else { - return h, nil + return "", "", "" } } else { - recvName = rtyp.Obj().Name() + recv = rtyp.Obj() } } } } - if obj.Pkg() == nil { - event.Log(ctx, fmt.Sprintf("nil package for %s", obj)) - return h, nil + + if recv != nil && !recv.Exported() { + return "", "", "" } - h.importPath = obj.Pkg().Path() - h.LinkPath = h.importPath - if mod, version, ok := moduleAtVersion(h.LinkPath, i); ok { - h.LinkPath = strings.Replace(h.LinkPath, mod, mod+"@"+version, 1) + + // Either the object or its receiver must be in the package scope. + scopeObj := obj + if recv != nil { + scopeObj = recv } - if recvName != "" { - h.LinkAnchor = fmt.Sprintf("%s.%s", recvName, obj.Name()) - h.symbolName = fmt.Sprintf("(%s.%s).%s", obj.Pkg().Name(), recvName, obj.Name()) - return h, nil + if scopeObj.Pkg() == nil || scopeObj.Pkg().Scope().Lookup(scopeObj.Name()) != scopeObj { + return "", "", "" } - // For most cases, the link is "package/path#symbol". - h.LinkAnchor = obj.Name() - h.symbolName = fmt.Sprintf("%s.%s", obj.Pkg().Name(), obj.Name()) - return h, nil + + importPath = obj.Pkg().Path() + if recv != nil { + anchor = fmt.Sprintf("%s.%s", recv.Name(), obj.Name()) + name = fmt.Sprintf("(%s.%s).%s", obj.Pkg().Name(), recv.Name(), obj.Name()) + } else { + // For most cases, the link is "package/path#symbol". + anchor = obj.Name() + name = fmt.Sprintf("%s.%s", obj.Pkg().Name(), obj.Name()) + } + return name, importPath, anchor } func moduleAtVersion(path string, i *IdentifierInfo) (string, string, bool) { + // TODO(rfindley): moduleAtVersion should not be responsible for deciding + // whether or not the link target supports module version links. if strings.ToLower(i.Snapshot.View().Options().LinkTarget) != "pkg.go.dev" { return "", "", false } diff --git a/internal/lsp/testdata/godef/hover_generics/hover.go b/internal/lsp/testdata/godef/hover_generics/hover.go index 912f75a1c0..61a8218751 100644 --- a/internal/lsp/testdata/godef/hover_generics/hover.go +++ b/internal/lsp/testdata/godef/hover_generics/hover.go @@ -1,6 +1,11 @@ package hover type value[T any] struct { //@mark(value, "value"),hoverdef("value", value) - val T //@mark(Tparam, "T"),hoverdef("T", Tparam) - q int + val T //@mark(valueTparam, "T"),hoverdef("T", valueTparam) + Q int //@mark(valueQfield, "Q"),hoverdef("Q", valueQfield) +} + +type Value[T any] struct { + val T //@mark(ValueTparam, "T"),hoverdef("T", ValueTparam) + Q int //@mark(ValueQfield, "Q"),hoverdef("Q", ValueQfield) } diff --git a/internal/lsp/testdata/godef/hover_generics/hover.go.golden b/internal/lsp/testdata/godef/hover_generics/hover.go.golden index d7cda28471..4de8206c33 100644 --- a/internal/lsp/testdata/godef/hover_generics/hover.go.golden +++ b/internal/lsp/testdata/godef/hover_generics/hover.go.golden @@ -1,16 +1,35 @@ -- value-hoverdef -- ```go type value[T any] struct { - val T //@mark(Tparam, "T"),hoverdef("T", Tparam) - q int + val T //@mark(valueTparam, "T"),hoverdef("T", valueTparam) + Q int //@mark(valueQfield, "Q"),hoverdef("Q", valueQfield) } ``` --- Tparam-hoverdef -- +-- valueTparam-hoverdef -- ```go type value[T any] struct { - val T //@mark(Tparam, "T"),hoverdef("T", Tparam) - q int + val T //@mark(valueTparam, "T"),hoverdef("T", valueTparam) + Q int //@mark(valueQfield, "Q"),hoverdef("Q", valueQfield) } ``` +-- valueQfield-hoverdef -- +```go +field Q int +``` -[`hover.T` on pkg.go.dev](https://pkg.go.dev/golang.org/x/tools/internal/lsp/godef/hover_generics?utm_source=gopls#T) +\@mark\(valueQfield, \"Q\"\),hoverdef\(\"Q\", valueQfield\) +-- ValueTparam-hoverdef -- +```go +type Value[T any] struct { + val T //@mark(ValueTparam, "T"),hoverdef("T", ValueTparam) + Q int //@mark(ValueQfield, "Q"),hoverdef("Q", ValueQfield) +} +``` +-- ValueQfield-hoverdef -- +```go +field Q int +``` + +[`(hover.Value).Q` on pkg.go.dev](https://pkg.go.dev/golang.org/x/tools/internal/lsp/godef/hover_generics?utm_source=gopls#Value.Q) + +\@mark\(ValueQfield, \"Q\"\),hoverdef\(\"Q\", ValueQfield\) diff --git a/internal/lsp/testdata/summary_go1.18.txt.golden b/internal/lsp/testdata/summary_go1.18.txt.golden index f8ec4df2d6..b2706d6589 100644 --- a/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/internal/lsp/testdata/summary_go1.18.txt.golden @@ -16,7 +16,7 @@ SemanticTokenCount = 3 SuggestedFixCount = 61 FunctionExtractionCount = 25 MethodExtractionCount = 6 -DefinitionsCount = 101 +DefinitionsCount = 104 TypeDefinitionsCount = 18 HighlightsCount = 69 ReferencesCount = 27