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: support showing all interfaces that a given type implements #35550

Closed
stamblerre opened this issue Nov 13, 2019 · 27 comments
Closed

x/tools/gopls: support showing all interfaces that a given type implements #35550

stamblerre opened this issue Nov 13, 2019 · 27 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 13, 2019

Should this be part of the textDocument/implementation request or is this a separate feature? Perhaps we need to request clarification in the protocol.

@muirdm
Copy link

@muirdm muirdm commented Feb 14, 2020

@stamblerre I've run into this a couple times. I come across a concrete method and run find-references on it, but get nothing back. I'm mildly confused until I figure the method might be used to satisfy an interface, but there is no way to find that interface easily to continue my code exploration.

Would you be opposed to making textDocument/implementation also work backwards from concrete type/method -> interface type/method for now?

We should also explore a proposal to add "implements" to the LSP spec, but it is sort of a unique problem to Go since most languages require you to explicitly declare when a type implements an interface, so going from concrete -> interface isn't hard for the user.

Personally, I don't really want another flavor of find-something-something to choose from as a user, but of course I could always just make my editor search both and combine results if I don't want to think about it.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 14, 2020

Would you be opposed to making textDocument/implementation also work backwards from concrete type/method -> interface type/method for now?

Yes, I think it's time to give this a try. I think it's a reasonable solution. But I think we could also try having find references include these results.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 16, 2020

Change https://golang.org/cl/219678 mentions this issue: internal/lsp/source: support inverse "implementations"

gopherbot pushed a commit to golang/tools that referenced this issue Mar 2, 2020
Now "implementations" supports finding interfaces implemented by the
specified concrete type. This is the inverse of what "implementations"
normally does. There isn't currently a better place in LSP to put this
functionality. The reverse lookup isn't an important feature for most
languages because types often must explicitly declare what interfaces
they implement.

An argument can be made that this functionality fits better into find-
references, but it is still a stretch. Plus, that would require
find-references to search all packages instead of just transitive
dependents.

Updates golang/go#35550.

Change-Id: I72dc78ef52b5bf501da2f39692e99cd304b5406e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219678
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

Any progress?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Mar 3, 2020

The above CL has been merged at master, but it is not part of the gopls/v0.3.3 release.

@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

I use @master. Does it work only with interfaces/types located within same module? I don't see this feature working:
изображение

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Mar 3, 2020

Yes, it only works on packages in your module and their transitive dependencies.

@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

So there is no chance to see if we implement smth like fmt.Stringer, or io.Reader? Do you think it's okay to have such limited fuctionality?

Still, it doesn't work:

изображение

@muirdm
Copy link

@muirdm muirdm commented Mar 3, 2020

Are you sure you have the changes in your running gopls? Your example works for me, and fmt.Stringer/io.Reader also work.

@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

Well, I update from master every day, so I suppose this was merged few hours ago? Let me check. Is there any chance, for instance, if I ask to find implementations navigating at Stringer in fmt package, that it will also find that it's implemented by some of my project's types?

@muirdm
Copy link

@muirdm muirdm commented Mar 3, 2020

Yes, find-implementations on fmt.Stringer should list a ton of things including any types in your workspace that implement fmt.Stringer.

@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

Nice, finally it looks like something usable and helpful. Like it was with bingo > than a year ago :P

@muirdm
Copy link

@muirdm muirdm commented Mar 3, 2020

So does it work for you now? What was the problem?

@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

When I open this project: https://github.com/benthosdev/benthos-plugin-example
and navigate to https://pkg.go.dev/github.com/Jeffail/benthos/v3/lib/types?tab=doc#Output

It shows that there is no local type implementing it:
изображение

However this type implements it:
https://github.com/benthosdev/benthos-plugin-example/blob/master/output/blue_stdout.go#L35

To be honest before that I saw just one single time that feature worked (but this particular type was missing as well in list of implementations of types.Output, so I was going to file that particular issue), but after several restarts I never seen this been working again, just "no implementaions" message.

@muirdm
Copy link

@muirdm muirdm commented Mar 3, 2020

I can reproduce it not working when you run find-implementations on the declaration of the Output type. But, it works when you run find-implementations on types.Output within blue_stdout.go, and running find-implementations on the declaration of BlueStdout also works (i.e. it lists the "Output" interface). Is that consistent with what you see, or does it not work at all for you?

@inliquid
Copy link

@inliquid inliquid commented Mar 3, 2020

Yes, right now it's same.

@muirdm
Copy link

@muirdm muirdm commented Mar 4, 2020

The issue is the github.com/Jeffail/benthos/v3/lib/types dependency is loaded in ParseExported mode when github.com/benthosdev/benthos-plugin-example/output is loaded, but when you run find-implementations from github.com/Jeffail/benthos/v3/lib/types/interface.go it uses the ParseFull version, so the named types in the interface no longer match.

@stamblerre I guess the solution is to re-type check a package's dependents when it transitions from ParseExported to ParseFull? In other words if you directly open a file that was previously parsed in ParseExported, you need to invalidate type data of any dependent packages so they properly depend on the ParseFull version.

@inliquid
Copy link

@inliquid inliquid commented Mar 4, 2020

Actually I don't open it directly, I just Ctrl-Click on types.Output, and that's it. Another question is that I don't see unexported interfaces or implementations. This forces me to use exported things most of the time, otherwise how do I know there is something else if tool doesn't show it? Goland shows both exported and unexported, and this makes it very useful.

@muirdm
Copy link

@muirdm muirdm commented Mar 4, 2020

gopls shows unexported interfaces and implementations. Do you have steps to reproduce them not showing up?

@inliquid
Copy link

@inliquid inliquid commented Mar 4, 2020

For example, mentioned above BlueStdout implements this interface: https://github.com/Jeffail/benthos/blob/v3.10.0/lib/broker/fan_in.go#L79
It's shown in Goland but missing with gopls.

@muirdm
Copy link

@muirdm muirdm commented Mar 4, 2020

That is related to the other issue. The broker package is loaded in ParseExported mode which means the function bodies and un-exported objects are elided. That type is defined in the function body (and is unexported), so gopls can't find it.

@inliquid
Copy link

@inliquid inliquid commented Mar 4, 2020

Is this gonna be fixed?

@muirdm
Copy link

@muirdm muirdm commented Mar 4, 2020

Fixing the first issue of finding implementations starting from a non-workspace dependency seems relatively straight forward, but fixing the second issue will take more consideration. gopls loads non-workspace dependencies in export-only mode to reduce memory usage, which is a concern for many users.

@inliquid
Copy link

@inliquid inliquid commented Mar 4, 2020

I understand it's not trivial. Would be nice to have it anyways, thank you.

@kingfolk
Copy link

@kingfolk kingfolk commented May 21, 2020

I've reached the same problem. But another maybe unrelated puzzle: why ubuntu(in a virtual machine) vscode can successfully find all references even without gopls? I compared them side by side and cannot figure out why... But the same time, ubuntu vscode seems missing a lot of syntax check.

image
image

@kingfolk
Copy link

@kingfolk kingfolk commented May 21, 2020

Ok, I got what i am missing. The references result turned normal after I moved the project into $GOPATH/src. Noticed in ubuntu, the project "gocaml" has a copy under $GOPATH/src. When I use "Find All References" inside other copy, it will show result in $GOPATH copy. The ubuntu pic shows the "references" followed by relative path showing the reference is outside of project path.
Sorry, newbie to Go world. A lot "weired" to learn to make sense. The "sense" is for old go project without go.mod, maybe move into $GOPATH/src is the right way to make everything work.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Jun 24, 2020

I think this issue falls under the broader category of figuring out how to better handle the full vs exported ASTs. This needs to be resolved in the context of running analyses correctly too, but I don't think it will happen until after gopls/v1.0.0. We can perhaps continue the conversation on #38278. Closing this as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.