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 <rfindley@google.com> Run-TryBot: Robert Findley <rfindley@google.com> Reviewed-by: Heschi Kreinick <heschi@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
2022-02-10 18:17:04 -05:00 · 2022-02-10 18:17:04 -05:00 · 45aeaf7b04
parent a317113201
commit 45aeaf7b04
4 changed files with 99 additions and 60 deletions
--- 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
 	}
--- 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)
 }
--- 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\)
--- 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