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: increase max number of completion candidates for unimported packages #60988

Open
110y opened this issue Jun 25, 2023 · 7 comments · May be fixed by golang/tools#442
Open

x/tools/gopls: increase max number of completion candidates for unimported packages #60988

110y opened this issue Jun 25, 2023 · 7 comments · May be fixed by golang/tools#442
Labels
FeatureRequest gopls/completion Issues related to auto-completion in gopls. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@110y
Copy link

110y commented Jun 25, 2023

This is a feature request.

gopls version

latest master branch (fa10359)

What did you want to see?

  • Make the max number of completion candidates for unimported packages configurable.

What did you see instead?

The current implementation uses hard-coded number (5) for the unimported package completion. Though I know the unimported package completion is expensive, I think it's worth making the number of candidates for it configurable (actually, I'm using my own forked version of gopls that uses over 100 for unimported completion candidates for a long time.)

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jun 25, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jun 25, 2023
@110y
Copy link
Author

110y commented Jun 25, 2023

I send a PR that implements this feature request: golang/tools#442

@findleyr
Copy link
Contributor

@110y thank you for the request, and your report that the current number of unimported completion candidates was too low -- I had not heard this, but it makes sense.

Before we add a new configuration options, I want to answer the question: why couldn't we just increase the current hard-coded default? Why would it be the case that 100 works better for you, but not for others?

In general, I think we need a framework for addressing these types of requests for new configuration, so I took this opportunity to file #61001.

@suzmue suzmue added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 29, 2023
@suzmue suzmue modified the milestones: Unreleased, gopls/later Jun 29, 2023
@adonovan
Copy link
Member

adonovan commented Jul 4, 2023

Note that #60964 is a dup of this issue. Bumping up the threshold constant (to, say, 10) would solve that specific problem, though I suspect it would cause some other package to fall just beyond the threshold, moving the problem rather than fixing the whole class of problems. But maybe that's acceptable if the larger value is enough to dampen the expectations of exhaustiveness (like the way that no-one looks at page 2 of the Google search results). My point is that increasing this constant from 5 to 10 could be a tactical fix that doesn't require adding a configuration knob, or deciding on the framework for such knobs.

@findleyr findleyr modified the milestones: gopls/later, gopls/v0.13.0 Jul 5, 2023
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 29, 2023
@110y
Copy link
Author

110y commented Jul 31, 2023

@findleyr

Hi, thank you for taking a look at this issue.

While a discussion on the issue #61001 has not progressed, I think this issue related to the unimported package candidates is not resolved. So, I'd say this issue should not be closed.

why couldn't we just increase the current hard-coded default?

I think if the value is larger enough, this problem may not be a matter.

Why would it be the case that 100 works better for you, but not for others?

In fact, I set the value to be 10000 and it's working well for me so far though I'm using a ordinary PC (24 Core, 64 GB).
So, I think the value 100 or 1000 should work well for others (not exactly sure though).

How do yo think about this?

@findleyr findleyr changed the title x/tools/gopls: make max number of completion candidates for unimported packages configurable x/tools/gopls: increase max number of completion candidates for unimported packages Jul 31, 2023
@findleyr
Copy link
Contributor

@110y sorry, gopherbot automatically closes after a certain amount of time in WaitingForInfo. I agree, this should not have been closed.

Let's try increasing it to see if the problem is fixed. I'll slot this for our next minor release (v0.14.0).

I'm not sure if there is a performance penalty to having arbitrarily many candidates. I suspect not, as filtering by package name generally restricts to only a few packages.

@findleyr findleyr reopened this Jul 31, 2023
@findleyr findleyr removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jul 31, 2023
@adonovan adonovan added the gopls/completion Issues related to auto-completion in gopls. label Aug 31, 2023
@findleyr
Copy link
Contributor

I did a bit of digging, and the number of unimported package completions was 100 before https://go.dev/cl/218879, where it was decreased to 5 for a combination of noise reduction and performance.

Before increasing the number, I think we should implement completionItem/resolve support. I see that VS Code advertises additionalTextEdits as a completion item property that may be lazily resolved. @110y which LSP client are you using?

@findleyr findleyr modified the milestones: gopls/v0.14.0, gopls/v0.15.0 Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls/completion Issues related to auto-completion in gopls. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
5 participants