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: improve unimported completion ranking #38104

Closed
stamblerre opened this issue Mar 27, 2020 · 10 comments
Closed

x/tools/gopls: improve unimported completion ranking #38104

stamblerre opened this issue Mar 27, 2020 · 10 comments
Labels
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Mar 27, 2020

I recently noticed a few issues with unimported completion:

  1. Each completion produces different results (see below for an example). I'd expect consistent results here.

Screen Shot 2020-03-26 at 11 41 16 PM

Screen Shot 2020-03-26 at 11 40 22 PM

  1. It doesn't seem to be prioritizing packages in my module above packages in my module cache. In this case, I wanted to import golang.org/x/tools/internal/gocommand, but I got suggested github.com/govim/govim but not gocommand.

  2. We should mark completion results with unimported completions in them incomplete (I thought we already did?). VS Code did not trigger completion again as I typed, resulting in this:

Screen Shot 2020-03-26 at 11 44 08 PM

/cc @heschik @muirdm

@stamblerre stamblerre added this to the gopls/v0.4.0 milestone Mar 27, 2020
@heschik
Copy link
Contributor

@heschik heschik commented Mar 30, 2020

The goimports code makes no attempt to return a consistent set of results, so some randomness is expected when there are many candidates with equal prioritization. But these don't have equal priorities.

The problem is that when it scans the disk, it scans in priority order. But the cache iteration is in a random order, and gopls cancels the iteration as soon as it gets some number of results. There's currently no attempt to keep receiving results until it gets them at a high enough priority. I'm not sure what to do about this; the cache can't know priorities because it's not dependent on the go.mod, so we'd have to scan the entire cache, which could be slow. (We could stop after receiving "enough" hits at the highest priority, but generally speaking that won't happen.)

I'm not sure what to do about this.

@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Mar 30, 2020
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Mar 30, 2020

The problem is that when it scans the disk, it scans in priority order. But the cache iteration is in a random order, and gopls cancels the iteration as soon as it gets some number of results.

Is there a way to change the cache iteration order? Or to at least have it prefer the local module? It seems to me you're much more likely to be looking for a package in your module.

@heschik
Copy link
Contributor

@heschik heschik commented Mar 31, 2020

Forgot a trick, mailed https://golang.org/cl/226559 which should improve things significantly. We can revisit if there's more complaints. I can't think of any more quick fixes.

Haven't looked at the complete-ness of the results at all yet.

@atombender
Copy link

@atombender atombender commented Apr 30, 2020

With current master (including patch mentioned above) I'm still getting import reorgs that are horribly wrong.

For example, we use github.com/stretchr/testify/assert, yet writing assert.Equal(t, a, b) in a test and saving will have an import added to some other random package that isn't in our project's go.mod or in go.sum, such as gotest.tools/assert, or some random package that's not in go.mod but is in go.sum, such as gopkg.in/go-playground/assert.v1.

As for autocompletion, if I just type assert. (when it's not yet imported), the top symbols offered are from the right package, but then if I start typing Equal, it will suggest functions from the wrong package as the top hits.

CleanShot 2020-04-30 at 10 27 39

Why can't the import reorg simply check each candidate against go.mod?

@heschik
Copy link
Contributor

@heschik heschik commented Apr 30, 2020

The imports-on-save path doesn't rank by module relevance; that's #36077.

If you already use testify in your project, it should be at the top of the rankings, because it should be in memory. I think the behavior in your screencast is simply be a scoring bug -- untyped completions are ending up before typed completions, which really shouldn't happen. @muirdm might have a guess here. I can try to reproduce it at some point, it might be pretty easy, but I don't have time right now.

Why can't the import reorg simply check each candidate against go.mod?

In general the answer to any question of the form "why can't X just Y" is going to be that it's harder than you think it is. I went into some detail above. If you want a full explanation of the architecture of the autocomplete feature, I would appreciate you asking more politely.

@muirdm
Copy link

@muirdm muirdm commented Apr 30, 2020

I looked briefly and all unimported packages seem to have a relevance of 3, which is the lowest score. github.com/stretchr/testify/assert should have a relevance of 6 if it is a direct dependency of the main module.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 30, 2020

Change https://golang.org/cl/231238 mentions this issue: internal/lsp/source: improve unimported package member ranking

@heschik
Copy link
Contributor

@heschik heschik commented May 1, 2020

The CL above didn't fix Rebecca's issue, but I don't want to leave this open as a magnet for every ranking complaint, so I'm inclined to leave it closed for now.

@muirdm
Copy link

@muirdm muirdm commented May 1, 2020

Oops. I was working off @atombender's comment which I now realize does not overlap with the original items in this issue. I think the other unimported package issue you were working on should fix 2. It doesn't seem to be prioritizing packages in my module above packages in my module cache..

We should mark completion results with unimported completions in them incomplete

All completion results are marked as incomplete if you have deep completions or fuzzy matching enabled. Not sure what was going on here. Do you have the rpc trace?

@atombender
Copy link

@atombender atombender commented May 1, 2020

I was working off @atombender's comment

Apologies. It's often hard to know some behaviour is the same issue. My own completion problem sounded to me like what this issue described.

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.