Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: render package documentation when hovering over imported package name identifiers #71248

Open
GSmithApps opened this issue Jan 10, 2025 · 15 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. gopls Issues related to the Go language server, gopls. help wanted
Milestone

Comments

@GSmithApps
Copy link

GSmithApps commented Jan 10, 2025

Is your feature request related to a problem? Please describe.
Package-level description isn't rendering

Describe the solution you'd like
I'd like the package-level description to render when hovering over an imported package, as shown in the screenshots below. ChatGPT suggests that if a package is spread across multiple files, Go uses first (alphabetically) nonempty comment, but I'm not totally sure. I don't know if VSCode is just getting confused.

Describe alternatives you've considered
One option is to ignore it. Another is to use GoLand. Another is to just always have the docs up

Additional context

GoLand

I know the docs are there. They're in doc.go, and they're rendering in Godoc, and they're showing in GoLand

https://pkg.go.dev/go.temporal.io/sdk@v1.31.0/workflow#Go

Screenshot 2025-01-10 at 4 01 35 PM docs-are-in-doc-dot-go Using-GoLand

VSCode

Using-vscode
@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Jan 10, 2025
@GSmithApps
Copy link
Author

GSmithApps commented Jan 10, 2025

I might be able to help out on this too

Here's the Godoc github. I figure their logic is in here somewhere. Maybe we could use that

@firelizzard18
Copy link
Contributor

@GSmithApps There's already logic in gopls for rendering package documentation. If you set "gopls": { "ui.documentation.hoverKind": "FullDocumentation" } and hover over the import URL it will render the full package documentation (the image below). The hover implementation is here. I'm not sure precisely what needs to be changed, but I believe it would only require a minor modification to hover.go. But this might not be something most users want to see. CC @findleyr

image

@firelizzard18 firelizzard18 added gopls Issues related to the Go language server, gopls. and removed Documentation Issues describing a change to documentation. labels Jan 10, 2025
@GSmithApps
Copy link
Author

GSmithApps commented Jan 11, 2025

Hey @firelizzard18 thanks so much for looking at this! Yeah I'm having luck when I hover over the import statement, but not when I hover over the variable name that it imports to. In your example, it would be if you hover over testeval somewhere later in your code where you're using it. For my example, it's when I hover over the workflow variable later in my code.

And that includes if I add that note you posted to both my project and user-level settings.json

@firelizzard18
Copy link
Contributor

Yeah, that matches what I saw. While I'm confident that implementing your feature would involve tweaking the Hover function, it's not clear to me what the code path is for hovering on a package identifier (e.g. testeval) vs a package import, which is why I said I'm not certain what needs to change.

@findleyr
Copy link
Member

This is quite old logic, and it looks like https://go.dev/cl/230417 just added special handling for import specs. The case of a PkgName object in the code falls back to default behavior for object references.

We could certainly do this, if people would find it helpful. I will move this to the gopls issue tracker as a feature request.

@findleyr findleyr changed the title Package-level documentation not rendering on hover x/tools/gopls: render package documentation when hovering over imported package name identifiers Jan 13, 2025
@findleyr findleyr transferred this issue from golang/vscode-go Jan 13, 2025
@findleyr findleyr added FeatureRequest Issues asking for a new feature that does not need a proposal. help wanted labels Jan 13, 2025
@findleyr findleyr added this to the gopls/unplanned milestone Jan 13, 2025
@gopherbot gopherbot modified the milestones: gopls/unplanned, Unreleased Jan 13, 2025
@firelizzard18
Copy link
Contributor

@findleyr Are you thinking to use the same verbosity level for the import hover and the package identifier hover? If that's the case it seems like it wouldn't be too hard for an external contributor to implement.

@findleyr
Copy link
Member

@firelizzard18 yes, I should have been clearer: I think it would be reasonable to serve the same hover content for the package name in a qualified identifier as we do for an import spec. If anything, the lack of consistency is surprising. Given a bit of historical digging, I don't think this was intentional.

@GSmithApps
Copy link
Author

GSmithApps commented Jan 13, 2025

Yeah same here — I also was surprised and couldn’t determine if I had misconfigured something.

I can take a stab at implementing it, but someone else would probably be much faster. I see firelizzard's link and can poke around in here, but if anyone has a recommendation on where to start, I’m all ears.

Thanks again @firelizzard18 and @findleyr !

@firelizzard18
Copy link
Contributor

@GSmithApps It will probably be fastest for you to implement it. I'm interested myself but I have multiple in-progress CLs and I don't plan on opening any more until those are merged, which may take some time. Personally I'd start with using tests and the debugger to step through the code to understand how it flows and how it might be changed. TestHoverImport tests hovering an import URL; for hovering a package identifier I wrote the test below (copied from TestHoverImport) and it appears to work as expected though it currently fails since Hover does not return the package documentation.

TestHoverPackageIdent
func TestHoverPackageIdent(t *testing.T) {
	const packageDoc1 = "Package lib1 hover documentation"
	const packageDoc2 = "Package lib2 hover documentation"
	tests := []struct {
		hoverIdent string
		want       string
		wantError  bool
	}{
		{
			"lib1",
			packageDoc1,
			false,
		},
		{
			"lib2",
			packageDoc2,
			false,
		},
		{
			"lib3",
			"",
			false,
		},
		{
			"lib4",
			"",
			true,
		},
	}
	source := fmt.Sprintf(`
-- go.mod --
module mod.com

go 1.12
-- lib1/a.go --
// %s
package lib1

const C = 1

-- lib1/b.go --
package lib1

const D = 1

-- lib2/a.go --
// %s
package lib2

const E = 1

-- lib3/a.go --
package lib3

const F = 1

-- main.go --
package main

import (
	"mod.com/lib1"
	"mod.com/lib2"
	"mod.com/lib3"
	"mod.com/lib4"
)

func main() {
	println(lib1.C)
	println(lib2.E)
	println(lib3.F)
	println(lib4.Z)
}
	`, packageDoc1, packageDoc2)
	Run(t, source, func(t *testing.T, env *Env) {
		env.OpenFile("main.go")
		for _, test := range tests {
			got, _, err := env.Editor.Hover(env.Ctx, env.RegexpSearch("main.go", "("+test.hoverIdent+")\\."))
			if test.wantError {
				if err == nil {
					t.Errorf("Hover(%q) succeeded unexpectedly", test.hoverIdent)
				}
			} else if !strings.Contains(got.Value, test.want) {
				t.Errorf("Hover(%q): got:\n%q\nwant:\n%q", test.hoverIdent, got.Value, test.want)
			}
		}
	})
}

@GSmithApps
Copy link
Author

Oh wow MVP status @firelizzard18 thank you so much!

I went ahead and forked the repo (here) and added your test and added you two as contributors in case you get some inspiration.

Thanks again -- hopefully we/I can get this up and running!

@GSmithApps
Copy link
Author

Okay I think we've found the first point at which the existing test (for hovering over the import statement) and the second test (to test the identifier) differ. It's in hover.go. The left side of the image is the old test, and it gets caught in this if statement, but the new test doesn't get caught in there. This is because this if statement is for imports.

So I think what we need to do is figure out where in the remainder of that function we want it to hit for the identifier test 👍

Image

@firelizzard18
Copy link
Contributor

You'll probably need to refactor hoverImport since it takes a *ast.ImportSpec as a parameter which you won't have when handling a package identifier. Maybe move the common code to a new function, hoverPackageRef, with hoverImport calling that and a new function hoverPackageIdent that also calls hoverPackageRef. Then if you add the following after ident, obj, selectedType := referencedObject(pkg, pgf, pos) it should do the trick.

if _, ok := obj.(*types.PkgName); ok {
	rng, hoverRes, err := hoverPackageIdent(ctx, snapshot, pkg, pgf, ident)
	if err != nil {
		return protocol.Range{}, nil, err
	}
	if hoverRange == nil {
		hoverRange = &rng
	}
	return *hoverRange, hoverRes, nil // (hoverRes may be nil)
}

I pretty much just copied that from the hoverImport case so you may have to change it.

GSmithApps added a commit to GSmithApps/tools that referenced this issue Jan 15, 2025
golang/go#71248 (comment)

I basically just copy-pasted and didn't think much. I'm hoping to take
a closer look tomorrow or something
@GSmithApps
Copy link
Author

Okay awesome -- thank so much again @firelizzard18 !

I went ahead and copy-pasted in what you suggested and did a little bit of tweaking, but it's not quite there yet. I think I/we'll have to look at it again one more time tomorrow or something

(Here's the repo) again and some screenshots

Image

Image

Image

@findleyr findleyr modified the milestones: Unreleased, gopls/backlog Jan 15, 2025
@findleyr
Copy link
Member

@GSmithApps please send me the CL when it is ready for review, thanks!

@GSmithApps
Copy link
Author

GSmithApps commented Jan 20, 2025

Hey @findleyr here's the pull request and CL

Ethan's test is passing (screenshot below), and the implementation is pretty close to what he suggested

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. gopls Issues related to the Go language server, gopls. help wanted
Projects
None yet
Development

No branches or pull requests

4 participants