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: prune noisy indirect references from textDocument/references results #42350

Closed
findleyr opened this issue Nov 2, 2020 · 4 comments
Closed
Assignees
Labels
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Nov 2, 2020

Currently, when finding references for a type T, gopls also returns references to interfaces that T implements.

This can be a source of significant noise in the results, particularly for LSP clients that apply their own sorting on top of the results returned by gopls. Consider the following:

type T struct{}
func (T) String() string { return "" }
func useT(T) {}

In this case, the type T implements the fmt.Stringer interface. Finding references to such a T defined in the x/tools repo results in both T.String and useT (the only direct references to T) not being in the first page of results.

I feel like we can do better, either by excluding all indirect references to T, restricting indirect references to those interfaces defined in the same package as T, or adding special handling for extremely common interfaces such as fmt.Stringer.

CC @heschik @stamblerre

@muirdm
Copy link

@muirdm muirdm commented Nov 4, 2020

To add a concrete example, our codebase has a type Option interface { ... } interface implemented and referenced in many places. I ran find-references on a particular option implementation type MyOption float64 to see where it is used, but I got a list of all references to the Option interface.

I think find-references should not list interfaces when the target object is a type name. An implemented interface is not a reference to the type in any stretch of the word "reference". @heschik indicated this behavior was not intentional in Slack.

As for find-references on concrete methods returning references to implemented interface methods, I'm of two minds. If there are no references to the concrete method and it isn't an "extremely common" interface, then returning references to the interface method is probably useful. If there are references to the concrete method, then I'm less sure.

What if instead of including all references to the interface method, we include only the declaration of the interface method(s) as a result? That would at least make it obvious to the user that the concrete method implements an interface. From there the user could find references on the interface method if they want, understanding now that the results aren't directly related to the concrete method.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 9, 2020

Change https://golang.org/cl/268462 mentions this issue: internal/lsp/source: don't find possible interface references to types

@muirdm
Copy link

@muirdm muirdm commented Nov 9, 2020

@heschik what did you think about:

What if instead of including all references to the interface method, we include only the declaration of the interface method(s) as a result?

I think that would further cut noise down but still make interface callers discoverable, albeit requiring a second find-references call.

@heschik
Copy link
Contributor

@heschik heschik commented Nov 9, 2020

Oh, sorry.

I requested this feature precisely because the second call is a huge irritant for me. Possibly it's because gopls is a little bit more interface-heavy than the average Go program, but keeping track of two sets of find references results is hard, and call hierarchy is even worse.

I think the real problem here is that we can't categorize results, and in the absence of that there's probably no perfect solution. I'm still open to excluding Closer and Stringer, or adding a config option. But that's not what this bug was originally about.

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