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 module-local implementation request #32973

Closed
litleleprikon opened this issue Jul 8, 2019 · 29 comments
Closed

x/tools/gopls: support module-local implementation request #32973

litleleprikon opened this issue Jul 8, 2019 · 29 comments

Comments

@litleleprikon
Copy link

@litleleprikon litleleprikon commented Jul 8, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/esharifu/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/esharifu/projects/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.6/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sh/vcq1w7n1547584p7d_kh_vf00000gn/T/go-build605958996=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Trying to get implementations of interface in go

What did you expect to see?

gopls returns the list of places with structures which implements interface

What did you see instead?

textDocument/implementation request is not implemented

@gopherbot gopherbot added this to the Unreleased milestone Jul 8, 2019
@litleleprikon
Copy link
Author

@litleleprikon litleleprikon commented Jul 8, 2019

As this feature is very valuable for my everyday development process I want to work on it if maintainers do not have any reasons against it.

@stamblerre stamblerre changed the title x/tools/gopls: Support for textDocument/implementation request x/tools/gopls: support implementation LSP request Jul 8, 2019
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 8, 2019

@litleleprikon: If you are interested in working on this feature, please go ahead! We will be happy to review any CLs.

@gertcuykens
Copy link
Contributor

@gertcuykens gertcuykens commented Jul 28, 2019

@stamblerre that response was cutting a corner a bit :) because I think it gave a false sense of how difficult it is to implement :P But then again I could be totally wrong here

The good news I think is we can copy paste stuff from godoc src code who apparently has this feature already https://golang.org/lib/godoc/analysis/help.html

Would appreciate for example more specific references to parts in the gopls source code to take a look at that may help in the right direction.

Also some references in the source code where godoc analysis (see link above) does it's interface implement list magic.

And maybe some CL boilerplate so somebody calling this feature can already see "Hello world" appearing as ouput and basically has a crash course example how gopls works and know where to start by just looking at the boilerplate CL code provided

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Aug 5, 2019

A lot of the code for this feature can actually be taken from guru, which has an implements feature (see https://github.com/golang/tools/blob/master/cmd/guru/implements.go).

I intend to compile a document that explains how to begin contributing to gopls, so I won't write in much detail here, but https://github.com/golang/tools/tree/master/internal/lsp/source is a good place to start looking to see how other language features are implemented in gopls. I would also suggest reading the LSP spec for textDocument/implementation, as well as taking a look here to see how to expose the feature via the LSP.

For those with specific questions, I'm available via Slack on the #gopls channel or via DM.

@stamblerre stamblerre added help wanted and removed Suggested labels Aug 8, 2019
@nezorflame
Copy link

@nezorflame nezorflame commented Aug 16, 2019

Actually, the author of bingo have already done this work and took the guru implementation.
He now manages the fork of gopls at https://github.com/saibing/tools
I don't know if @saibing wishes to integrate his work into the main repo, but I can try to merge the code on top of the current master.

@tcard
Copy link

@tcard tcard commented Sep 4, 2019

I think the implementation command should support function types and interface methods too. @litleleprikon Maybe add it to the issue description? Or maybe we should add separate issues for each case?

Actually, the author of bingo have already done this work and took the guru implementation.
He now manages the fork of gopls at https://github.com/saibing/tools

I've tried this and it seems to work just like "go to defintion". So, not what we want. Or probably it just doesn't work that well, and it falls back to "go to definition" if no implementation is found.

In any case, from a quick glance, guru (and bingo) seem to work on a per-request basis by:

  • Fetching all named types reachable from the current package (yes, all of them).
  • Trying to mach each one of them with the specified type.

This should be optimized so that gopls indexes this stuff, ie. as gopls typechecks (which I guess doesn't happen in every request, but incrementally in every file change), keep mappings from:

  • Named type definitions to the interfaces they implement.
  • Function declarations and literals to the function types they can be assigned to.

And direct requests to those mappings.

That tells us which types/values could implement a type. Alternatively, or additionally, it might be interesting to keep track of which values are actually assigned to the type. So, if you do this:

/* myfile.go:13 */ type myImpl struct { ... } // implements MyInterface

/* myfile.go:42 */ var i MyInterface = myImpl{}

then a implementation request on MyInterface could return both myfile.go:13 (as guru would do, only faster) and myfile.go:42.

This may be more interesting than the find-all-possible-implementors approach because if an interface or function is simple enough, many types may implement it accidentally, while only in a few places we find actual implementations.

E. g.:

type Decoder func(interface{}) error

func Foo(decoder Decoder) { ... }

// actually create a Decoder value, so return this position in a `implementation`
// response
Foo(func(interface{}) error {
	...
})


// this function could also be used as a Decoder, but it isn't, and it would be
// noisy that it shows up in an `implementation` response
func Inspect(interface{}) error 

What do you think?

@litleleprikon
Copy link
Author

@litleleprikon litleleprikon commented Sep 8, 2019

Hi everyone in this chat! Thank you all for your suggestions. Excuse me for long silence. For personal reasons I had a lack of time but now I am ready to continue development of this feature.

@tcard I think your suggestion is pretty good and caching types and implementations will reduce the time for implementations search. I am right now not sure about a ways how to implement it and probably I need some time to be more familiar with caching in gopls.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 29, 2019

Change https://golang.org/cl/203918 mentions this issue: internal/lsp: add support for implements to the LSP

@stamblerre stamblerre changed the title x/tools/gopls: support implementation LSP request x/tools/gopls: support module-local implementation request Nov 4, 2019
@stamblerre stamblerre reopened this Nov 5, 2019
@nezorflame
Copy link

@nezorflame nezorflame commented Nov 5, 2019

@stamblerre thanks for merging this!
While using latest VSCode Go extension (v0.11.8) which now supports using gopls for implementation requests, Go to implementation works only for the interfaces in the same package.
It's currently unable to:

  • find interface implementation from the other package (if the interface is defined in the dependency)
  • find interface's function implementation (doesn't work even in the same package or file)

Example code:

package main

import "fmt"

type SomeInterface interface {
	SomeFunc() error
}

type someImpl struct{}

func (i *someImpl) SomeFunc() error { return nil }

func main() {
	var i SomeInterface
	i = &someImpl{}
	fmt.Println(i.SomeFunc())
}

Calling Go to implementation results:

  1. on SomeInterface - works, points to the someImpl
  2. on i.SomeFunc() - doesn't work
  3. on SomeFunc() inside of the interface - doesn't work
@inliquid
Copy link

@inliquid inliquid commented Nov 5, 2019

For me it doesn't work. How do I enable this feature?

@nezorflame
Copy link

@nezorflame nezorflame commented Nov 5, 2019

@inliquid you need to update gopls to any version or commit newer than golang/tools@02d0efc and update vscode-go extension (or your IDE's extension) to the latest one.

@inliquid
Copy link

@inliquid inliquid commented Nov 5, 2019

@nezorflame thanks, I'm on latest from master.

@inliquid
Copy link

@inliquid inliquid commented Nov 5, 2019

I have this error when press Ctrl-F12 in VS Code:
Cursor is at interface definition:
изображение

Implementation is in the same package.

@nezorflame
Copy link

@nezorflame nezorflame commented Nov 5, 2019

@inliquid is the implementation of this interface in the same package?

@inliquid
Copy link

@inliquid inliquid commented Nov 5, 2019

Yes sure, I edited comment.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 5, 2019

@inliquid: If you add the following your VSCode settings, you should see more detailed logging. Could you add those logs to this issue? Thanks!

"go.languageServerFlags": [
    "serve",
    "-rpc.trace",
]
@inliquid
Copy link

@inliquid inliquid commented Nov 5, 2019

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 8, 2019

Change https://golang.org/cl/206150 mentions this issue: internal/lsp: support implements for methods on an interface

@matloob
Copy link
Contributor

@matloob matloob commented Nov 8, 2019

Change https://golang.org/cl/206150 will fix the second of @nezorflame's bullet points (implements on interface functions). I plan to work on implementations across packages (but still only within the same workspace) next week.

gopherbot pushed a commit to golang/tools that referenced this issue Nov 11, 2019
This change copies the code in guru's implements implementation
that finds implementations of methods over to gopls, and uses
the information determined to resolve implements requests on
methods. Implements still only works only within packages.

Updates golang/go#32973

Change-Id: I0bd7849a9224fbef7ab8385070b18fbb30703e2b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206150
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2019

Change https://golang.org/cl/206518 mentions this issue: internal/lsp: support implementations requests for implementations in other packages

@matloob
Copy link
Contributor

@matloob matloob commented Nov 11, 2019

Change https://golang.org/cl/206518 will fix the first of @nezorflame's bullet points (implements across packages in the same module). For it to work the LSP needs to be given the entire module as its View/workspace.

@matloob
Copy link
Contributor

@matloob matloob commented Nov 12, 2019

The change has been submitted. Please try it out and let me know if you encounter any issues.

Thanks!!

@inliquid
Copy link

@inliquid inliquid commented Nov 12, 2019

Is there any way to find which interface does some type implement? This worked fine with bingo.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 12, 2019

@inliquid: Was bingo returning this information as part of the textDocument/implements feature? It seems to me that this would not fully conform to the LSP's Goto implementation language feature.

@inliquid
Copy link

@inliquid inliquid commented Nov 12, 2019

No idea. It worked very well when both invoking Ctrl-Shift-F12 at interface and implementation. In last case it was showing interfaces implemented by type.

@makkes
Copy link

@makkes makkes commented Nov 13, 2019

Change https://golang.org/cl/206150 will fix the second of @nezorflame's bullet points (implements on interface functions). I plan to work on implementations across packages (but still only within the same workspace) next week.

Great, works like a charm. Is there a tracking issue for the workspace-wide work?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 13, 2019

Change https://golang.org/cl/206518 will fix the first of @nezorflame's bullet points (implements across packages in the same module). For it to work the LSP needs to be given the entire module as its View/workspace.

This has been submitted as of this CL. You will have to rebuild gopls at master to use this feature, however.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 13, 2019

Filed #35550 to track your request, @inliquid.

@inliquid
Copy link

@inliquid inliquid commented Nov 13, 2019

@stamblerre thank you!

@golang golang locked and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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