From ee35f8ea92b83ca45ebd96714001e9312bfc96a0 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 12 Dec 2023 11:10:41 -0500 Subject: [PATCH] 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 Reviewed-by: Alan Donovan --- gopls/internal/lsp/source/hover.go | 7 ++++-- gopls/internal/test/marker/doc.go | 15 ++++++++----- gopls/internal/test/marker/marker_test.go | 15 ++++++++----- .../test/marker/testdata/hover/issue64239.txt | 9 -------- .../test/marker/testdata/hover/issues.txt | 22 +++++++++++++++++++ 5 files changed, 46 insertions(+), 22 deletions(-) delete mode 100644 gopls/internal/test/marker/testdata/hover/issue64239.txt create mode 100644 gopls/internal/test/marker/testdata/hover/issues.txt diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go index 9e9f5ddd806..4dc747dabdc 100644 --- a/gopls/internal/lsp/source/hover.go +++ b/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 diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go index 88ed89db328..755812fd592 100644 --- a/gopls/internal/test/marker/doc.go +++ b/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="..."> ... ). - - 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 diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index 10aba7e404c..2fa4197550e 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/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 } diff --git a/gopls/internal/test/marker/testdata/hover/issue64239.txt b/gopls/internal/test/marker/testdata/hover/issue64239.txt deleted file mode 100644 index 493942da988..00000000000 --- a/gopls/internal/test/marker/testdata/hover/issue64239.txt +++ /dev/null @@ -1,9 +0,0 @@ -This test verifies the fix for issue #64239: hover fails for objects in the -unsafe package. - --- p.go -- -package p - -import "unsafe" - -var _ = unsafe.Sizeof(struct{}{}) //@hover("Sizeof", "Sizeof", "`Sizeof` on pkg.go.dev") diff --git a/gopls/internal/test/marker/testdata/hover/issues.txt b/gopls/internal/test/marker/testdata/hover/issues.txt new file mode 100644 index 00000000000..6212964dff2 --- /dev/null +++ b/gopls/internal/test/marker/testdata/hover/issues.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")