gopls/internal/lsp/source: hovering over broken packages is not an error

Fixes golang/go#64237

Change-Id: I5b326e084c31a7835684fb08491bd71af606fb1c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/549117
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/internal/lsp/source/hover.go

@@ -273,7 +273,7 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro
 	{
 		linkMeta = findFileInDeps(snapshot, pkg.Metadata(), declPGF.URI)
 		if linkMeta == nil {
-			return protocol.Range{}, nil, bug.Errorf("no metadata for %s", declPGF.URI)
+			return protocol.Range{}, nil, bug.Errorf("no package data for %s", declPGF.URI)
 		}
 
 		// For package names, we simply link to their imported package.
@@ -283,7 +283,10 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro
 			impID := linkMeta.DepsByPkgPath[PackagePath(pkgName.Imported().Path())]
 			linkMeta = snapshot.Metadata(impID)
 			if linkMeta == nil {
-				return protocol.Range{}, nil, bug.Errorf("no metadata for %s", declPGF.URI)
+				// Broken imports have fake package paths, so it is not a bug if we
+				// don't have metadata. As of writing, there is no way to distinguish
+				// broken imports from a true bug where expected metadata is missing.
+				return protocol.Range{}, nil, fmt.Errorf("no package data for %s", declPGF.URI)
 			}
 		} else {
 			// For all others, check whether the object is in the package scope, or

gopls/internal/test/marker/doc.go

@@ -151,18 +151,18 @@ The following markers are supported within marker tests:
     TODO(adonovan): in the older marker framework, the annotation asserted
     two additional fields (source="compiler", kind="error"). Restore them?
 
-  - def(src, dst location): perform a textDocument/definition request at
+  - def(src, dst location): performs a textDocument/definition request at
     the src location, and check the result points to the dst location.
 
   - documentLink(golden): asserts that textDocument/documentLink returns
     links as described by the golden file.
 
-  - foldingrange(golden): perform a textDocument/foldingRange for the
+  - foldingrange(golden): performs a textDocument/foldingRange for the
     current document, and compare with the golden content, which is the
     original source annotated with numbered tags delimiting the resulting
     ranges (e.g. <1 kind="..."> ... </1>).
 
-  - format(golden): perform a textDocument/format request for the enclosing
+  - format(golden): performs a textDocument/format request for the enclosing
     file, and compare against the named golden file. If the formatting
     request succeeds, the golden file must contain the resulting formatted
     source. If the formatting request fails, the golden file must contain
@@ -172,9 +172,12 @@ The following markers are supported within marker tests:
     textDocument/highlight request at the given src location, which should
     highlight the provided dst locations.
 
-  - hover(src, dst location, sm stringMatcher): perform a
-    textDocument/hover at the src location, and checks that the result is
-    the dst location, with matching hover content.
+  - hover(src, dst location, sm stringMatcher): performs a textDocument/hover
+    at the src location, and checks that the result is the dst location, with
+    matching hover content.
+
+  - hovererr(src, sm stringMatcher): performs a textDocument/hover at the src
+    location, and checks that the error matches the given stringMatcher.
 
   - implementations(src location, want ...location): makes a
     textDocument/implementation query at the src location and

gopls/internal/test/marker/marker_test.go

@@ -231,6 +231,9 @@ func (m marker) ctx() context.Context { return m.run.env.Ctx }
 // T returns the testing.TB for this mark.
 func (m marker) T() testing.TB { return m.run.env.T }
 
+// server returns the LSP server for the marker test run.
+func (m marker) editor() *fake.Editor { return m.run.env.Editor }
+
 // server returns the LSP server for the marker test run.
 func (m marker) server() protocol.Server { return m.run.env.Editor.Server }
 
@@ -251,7 +254,7 @@ func (mark marker) path() string {
 
 // mapper returns a *protocol.Mapper for the current file.
 func (mark marker) mapper() *protocol.Mapper {
-	mapper, err := mark.run.env.Editor.Mapper(mark.path())
+	mapper, err := mark.editor().Mapper(mark.path())
 	if err != nil {
 		mark.T().Fatalf("failed to get mapper for current mark: %v", err)
 	}
@@ -418,6 +421,7 @@ var actionMarkerFuncs = map[string]func(marker){
 	"format":           actionMarkerFunc(formatMarker),
 	"highlight":        actionMarkerFunc(highlightMarker),
 	"hover":            actionMarkerFunc(hoverMarker),
+	"hovererr":         actionMarkerFunc(hoverErrMarker),
 	"implementation":   actionMarkerFunc(implementationMarker),
 	"incomingcalls":    actionMarkerFunc(incomingCallsMarker),
 	"inlayhints":       actionMarkerFunc(inlayhintsMarker),
@@ -1456,10 +1460,6 @@ func highlightMarker(mark marker, src protocol.Location, dsts ...protocol.Locati
 	}
 }
 
-// hoverMarker implements the @hover marker, running textDocument/hover at the
-// given src location and asserting that the resulting hover is over the dst
-// location (typically a span surrounding src), and that the markdown content
-// matches the golden content.
 func hoverMarker(mark marker, src, dst protocol.Location, sc stringMatcher) {
 	content, gotDst := mark.run.env.Hover(src)
 	if gotDst != dst {
@@ -1472,6 +1472,11 @@ func hoverMarker(mark marker, src, dst protocol.Location, sc stringMatcher) {
 	sc.check(mark, gotMD)
 }
 
+func hoverErrMarker(mark marker, src protocol.Location, em stringMatcher) {
+	_, _, err := mark.editor().Hover(mark.ctx(), src)
+	em.checkErr(mark, err)
+}
+
 // locMarker implements the @loc marker. It is executed before other
 // markers, so that locations are available.
 func locMarker(mark marker, loc protocol.Location) protocol.Location { return loc }

gopls/internal/test/marker/testdata/hover/issue64239.txt

@@ -0,0 +1,22 @@
+This test verifies fixes for various issues reported for hover.
+
+-- go.mod --
+module golang.org/lsptests
+
+-- issue64239/p.go --
+package issue64239
+
+// golang/go#64239: hover fails for objects in the unsafe package.
+
+import "unsafe"
+
+var _ = unsafe.Sizeof(struct{}{}) //@hover("Sizeof", "Sizeof", "`Sizeof` on pkg.go.dev")
+
+-- issue64237/p.go --
+package issue64237
+
+// golang/go#64237: hover panics for broken imports.
+
+import "golang.org/lsptests/nonexistant" //@diag("\"golang", re"could not import")
+
+var _ = nonexistant.Value //@hovererr("nonexistant", "no package data")