x/tools/gopls: cannot find refs to function type

[notrobpike]

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

$ go version
go version go1.20.7 darwin/amd64

Does this issue reproduce with the latest release?

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

go env Output

$ go env
GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

I tried to use vscode to "find all references", "find all implementations", "go to definition" of an function type alias embedded in a struct. vscode is unable to find it. vscode uses gopls so I believe it is gopls that is at fault.

What did you expect to see?

I expected to be taken to the method that the type alias references.

What did you see instead?

"go to definition" took me to the method that the function alias was itself part of (where it was being called from).

Load the following program into vscode. right click on the commented function call and try to find what actually would get called.

package main

import (
	"context"
	"fmt"
)

type cmdable func(ctx context.Context, arg string) string

type client struct {
	cmdable
}

func (c *client) init() {
	c.cmdable = c.Process
}

func (c *client) Process(ctx context.Context, arg string) string {
	return arg
}

func (c cmdable) DoIt(ctx context.Context, arg string) string {
	return c(ctx, arg) // really?!?
}

func main() {
	var c client
	c.init()
	fmt.Println(c.DoIt(context.Background(), "wtf"))
}

[hyangah]

@notrobpike can you clarify what you meant by "the method that the type alias references"?
There is no type alias involved in the code.
Or did you expect "Go to definition" to lead you to c.Process through some dynamic or callgraph analysis, because there is only one call path calling DoIt in this example?

[notrobpike]

sorry, i did misspeak. there is no alias here. And yes, I did expect to find c.Process although I now realize that is just because my thinking that this is a function that should be "findable". But if cmdable were a string, I wouldn't expect to find the literal string value assignment, even in this simplified example. So that was a wrong expectation.

However, please consider this slightly updated version.

package main

import (
	"context"
	"fmt"
)

type cmdable func(ctx context.Context, arg string) string

type client struct {
	cmdable
	fn func(ctx context.Context, arg string) string
}

func (c *client) init() {
	c.cmdable = c.Process
	c.fn = c.Process
}

func (c *client) Process(ctx context.Context, arg string) string {
	return arg
}

func (c cmdable) DoIt(ctx context.Context, arg string) string {
	return c(ctx, arg) // really?!?
}

func main() {
	var c client
	c.init()
	c.fn(context.Background(), "wtf")
	fmt.Println(c.DoIt(context.Background(), "wtf"))
}

At L31, "Find all References" to c.fn locates (among others) the assignment in c.init(). So I expect to similarly be able to find the assignment to c.cmdable.

If I highlight the receiver cmdable in DoIt, and Find all References, "everything" is found, except again, the assignment in c.init().

[hyangah]

If I hover over the cmdable in the client type definition and trigger "Find all References", gopls returns c.cmdable = c.Process in c.init().

Name cmdable is used both a type and a field of a struct type client.

It's a field name in the context of c.cmdable = c.Process, like fn in the context of c.fn = c.Process or in c.fn(ctx, ...). When triggered where cmdable is used as a field name, "Find all references" finds c.cmdable = c.Process in c.init(). For example,

func main() {
  ...
  c.cmdable(ctx, "foo") // find all references includes the line of `c.cmdable = c.Process` 
} 

On the other hand, cmdable is a type name in the context of func (cmdable) DoIt(...) string. So, "find all references" returns all locations where cmdable as "type" is used. I am not sure if it's desirable to include the assignment in c.init in the "find all references" result. For example, consider the following case:

type client struct {
  cmdable
  other cmdable
}

func (c *client) init() {
  c.cmdable = c.Process
  c.other = c.Process
} 

func (c cmdable) DoIt(ctx context.Context, arg string) string { // cmdable is a type
}

Should Find all References on cmdable (as type) show both assignments in c.init()?

I agree that embedding makes things so confusing in this case. However, I don't know how to express this Go-specific unique feature within LSP designed for general languages.
@adonovan @findleyr @suzmue

Finally, the receiver name c in func (c cmdable) DoIt(...) string is like a local variable or parameter name, so I think it makes sense "find all references" returning only the places in func (c cmdable) DoIt.

[notrobpike]

I agree, in your example, Find all References should NOT return the assignment in c.init()! So perhaps likewise highlighting cmdable in c.Doit() shouldn't locate that assignment either. I guess I muddied the waters a little there.

The function call c() in c.Doit() is known to be a function call. So eg, gopls properly identifies if the arguments are wrong. And so just like how Find all References on c.fn() in main() locates the assignment, Find all References on c() in c.Doit() should locate the assignment.

I agree that c looks like the receiver parameter. But because of the embedding (promotion?) it's actually the invocation of the cmdable field.

Yes this is hella confusing.

This is not a contrived example for the sake of opening a bug. The popular library go-redis uses this construction, since v7 I believe. The second commit of this PR. Because gopls cannot find the function c.Process(), it's difficult to follow a call tree. eg, start at a command in commands.go. These all invoke a "processor" (c.Process()) with all the command parameters set. There's not a reasonable way to discover that assignment.

You can Go to Type Definition on any method, that will take you to the function type declaration. Then you do Find all References, which gives you a bazillion hits (at least for cmdable; for others like statefulCmdable you get just a small set). Then you "know" to look at one of the embeddings in a struct, eg in pipeline.go. Then Find all References on the embedded field in that struct, and that's where you finally see the assignment of c.Process(). Obviously, I had to back into this description. You would never just figure all that out, and it's unreasonable anyway.

[adonovan]

I think there may be two separate issues here:

1. There's no easy way to ask the question "what are all the functions that satisfy the selected named func type?", or the inverse question, "what func types does this function satisfy?". These are the func analogues of "Find all implementations" for interfaces. Ultimately I think this is a problem imposed by the vocabulary of the LSP protocol, though one could define an extension to it that performs this query.
2. When you search for references to an embedded field, gopls returns only the references to the field declared at that point, but not to the type to which that field also refers. This is an edge case in the Go grammar: struct{I} is the only form in which an identifier is both a definition (of a field) and a reference (to a type). Ideally gopls would return the union of the results to both queries. This is something we can fix. (As a workaround, try temporarily naming the field, as in struct{cmdable cmdable}, and then you can choose the identifier for the query you want.) Filed as x/tools/gopls: 'references' on embedded field returns refs only to field, not type #63521.

[adonovan]

I think we should treat an Implementations request on any 'func' type without methods, or on any value of such a type, as a request for all the address-taken functions that are assignable to that type. (A request on a named func type with methods should continue to be treated only as an interface query to avoid ambiguity; the informed user can always issue an Implementations query from the func portion of type N func(...))

Computing address-taken functions is a little tricky.

If we assume (like unusedparams) that every exported function is potentially address-taken, then we'll get a lot of spurious matches. Perhaps that's ok, because there are always going to be tons of spurious matches for common signatures like func() or func() String, and almost no spurious matches when the signature type has esoteric parameters.

If we conduct a workspace-wide search for address-taking operations (e.g. package p; var f = fmt.Println), then we can't record this information in the package of the symbol itself: the address of fmt.Println is taken in package p, not in fmt. That said, an Implementations query for interfaces must already gather information from ~every package in the worst case, and this is no worse than that.

So, a rough plan:

• If the selected expression has type func with no methods, treat it as a request for address-taken functions
• When indexing each package, record every function address-taking operation in the package, regardless of the functions declaring package.
• To search the index, load every index record in the workspace and match, in analogous fashion to an interface with one method.

Should we return the position of each function declaration (e.g. func Println, analogous to the behavior for Interfaces), or of each address-taking operation (e.g. var f = fmt.Println)? The latter is inconsistent and possibly duplicative but may be more informative.

[findleyr]

@adonovan is this included in the improvements you plan to make for refs/implementations?