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: allow 'any' in completion results everywhere (remove filtering to constraints) #47669

Open
findleyr opened this issue Aug 12, 2021 · 6 comments
Assignees
Labels
gopls NeedsInvestigation Tools
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 12, 2021

When built with Go 1.18, gopls completion results erroneously include the new predefined type any in contexts where it is invalid (any is only valid for constraints).

We should exclude it whenever we are not completing constraints.

CC @stamblerre @griesemer

@findleyr findleyr self-assigned this Aug 12, 2021
@gopherbot gopherbot added Tools gopls labels Aug 12, 2021
@gopherbot gopherbot added this to the Unreleased milestone Aug 12, 2021
@stamblerre stamblerre removed this from the Unreleased milestone Aug 12, 2021
@stamblerre stamblerre added this to the gopls/v1.0.0 milestone Aug 12, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 12, 2021

Change https://golang.org/cl/341850 mentions this issue: internal/lsp/source/completion: exclude 'any' from lexical results

gopherbot pushed a commit to golang/tools that referenced this issue Aug 12, 2021
The predeclared 'any' type is only valid when completing constraints. We
should support that properly, but for now exclude it from results so
that our completion tests don't fail on Go 1.18.

For golang/go#47669

Change-Id: I7852f844684a6c03da90bf367d45d732e5d1e9bb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/341850
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jan 19, 2022

Repurposing this issue: now any is allowed everywhere!

So we need to revert the above CL, as as well as other places where we exclude any.

@findleyr findleyr changed the title x/tools/gopls: completion includes 'any' in invalid contexts x/tools/gopls: allow 'any' in completion results everywhere (remove filtering to constraints) Jan 19, 2022
@findleyr findleyr removed this from the gopls/on-deck milestone Jan 19, 2022
@findleyr findleyr added this to the gopls/v0.8.0 milestone Jan 19, 2022
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Feb 22, 2022

I just looked into this, and we seem to be suggesting completion to 'any' in places where we shouldn't. Needs more investigation.

@findleyr findleyr added the NeedsInvestigation label Feb 22, 2022
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Feb 23, 2022

Ok, it seems that completion just includes more results than I had assumed.

For example, from internal/lsp/testdata/rank/binexpr_rank.go.in:

func _() {
  _ = 5 +  ; //@complete(" ;", apple, pear)
}

We suggest any as a completion here, albeit with a low score, even though its type does not match. Specifically, we get here, and keep going even though c.matchingCandidate(cand) returns false.

Given that we downrank, this is fine, but it does seem a bit noisy. @muirdm do you have an opinion on whether we should be suggesting any at all in this context? I would trust your intuition as to whether (1) we shouldn't offer completions that don't match the inferred type, or (2) we should offer (but downrank) all lexical completions. It may eb

I don't think this it is a high priority to resolve this for v0.8.0, so moving it to the v0.8.1 milestone while we discuss.

@findleyr findleyr removed this from the gopls/v0.8.0 milestone Feb 23, 2022
@findleyr findleyr added this to the gopls/v0.8.1 milestone Feb 23, 2022
@muirdm
Copy link

@muirdm muirdm commented Feb 23, 2022

I think you want to add "any" to tools/internal/lsp/tests/util.go:isBuiltin. That is used to filter buiiltin candidates in completion tests.

We don't filter type names in general because it's hard to predict what the user wants to do. For example, they may want to complete to an incompatible type "someType" so they can write "someType(foo).someMethod()". Or maybe they don't realize the type is incompatible and would be confused by it not showing up. Or maybe they are composing an expression inside-out where they first insert "someType{}" and then wrap it in "somethingElse(someType{})".

"any" in particular doesn't seem useful, but unless there is a compelling reason to filter it I wouldn't bother.

@findleyr findleyr removed this from the gopls/v0.8.1 milestone Mar 14, 2022
@findleyr findleyr added this to the gopls/v0.8.2 milestone Mar 14, 2022
@findleyr findleyr removed this from the gopls/v0.8.2 milestone Mar 31, 2022
@findleyr findleyr added this to the gopls/v0.8.3 milestone Mar 31, 2022
@findleyr findleyr removed this from the gopls/v0.8.3 milestone Apr 5, 2022
@findleyr findleyr added this to the gopls/v0.8.4 milestone Apr 5, 2022
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented May 10, 2022

Haven't gotten to this, and it's a low priority because including 'any' in results doesn't add a huge amount of value. Bumping to the backlog.

@findleyr findleyr removed this from the gopls/v0.8.4 milestone May 10, 2022
@findleyr findleyr added this to the gopls/on-deck milestone May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls NeedsInvestigation Tools
Projects
None yet
Development

No branches or pull requests

4 participants