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: type-conversions are not scored correctly #36015

Closed
quasilyte opened this issue Dec 6, 2019 · 8 comments
Closed

x/tools/gopls: type-conversions are not scored correctly #36015

quasilyte opened this issue Dec 6, 2019 · 8 comments
Labels

Comments

@quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Dec 6, 2019

After CL183137, there is a problem with typenames completion where we want to do a type conversion.

It's caused by this check:

func (c *completer) matchingTypeName(cand *candidate) bool {
+	if !c.wantTypeName() {
+		return false
+	}

(Although removing it does solve this particular issue, we get other issues out of it, this is why someone more experienced with gopls needs to take a look.)

It bites when we try to autocomplete inside expression context and expect to use a type conversion:

package main

import (
	"fmt"

)

type coordX float64
type coordY float64

type Point struct {
	X coordX
	Y coordY
}

func main() {
	fmt.Println(Point{
		// <> completion
		// a want to do coordX(value) here
	})	
}

In my understanding, a type name is a valid candidate even when not inside type expression context, since it can be an attempt to write a type-converting expression.

Actual behavior

coordX is scored with 1.0, as all other "non matching" candidates.

Expected behavior

coordX is at least a little bit higher than 1.0.

Note that a575db70c06744141b7a52bb6c4aff8d860588c6 (a commit before mentioned CL works as expected):

image

If we need to define "when a type name is appropriate outside of wantTypeName", I would say it's "an expression context that expects a type that can be acquired by doing a conversion to the given type name". So, if we autocomplete Foo-typed expression, Foo is a matching candidate (score>1).

@gopherbot gopherbot added this to the Unreleased milestone Dec 6, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 6, 2019

Thank you for the report!

/cc @muirdm for completion expertise

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Dec 6, 2019

What if instead of completing to just coordX it offered a "literal" completion of coordX(<>)? Literal completions get a default score of highScore/2, so its score would be 5 instead of 1, i.e. still below the struct field names "X" and "Y".

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 6, 2019

Change https://golang.org/cl/210288 mentions this issue: internal/lsp: offer basic type conversion candidates

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Dec 6, 2019

@quasilyte please try out the above change if possible.

@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 10, 2019
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Dec 10, 2019

We could also take it further and offer pre-type converted candidates. For example:

func complicatedMath() int { return 0 }

var result float64 = complicat<> // completes straight to "float64(complicatedMath())"

It would be cool in some situations, but it runs a bit counter to Go's "no automatic number type conversion" policy.

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

@quasilyte quasilyte commented Dec 10, 2019

please try out the above change if possible.

Sorry for the late response.

It does work as I would have expected, thank you. 👐

It would be cool in some situations, but it runs a bit counter to Go's "no automatic number type conversion" policy.

That sounds interesting, although I usually omit the explicit type on the left, so the type doesn't need to be repeated:

var result = float64(complicatedMath())
gopherbot pushed a commit to golang/tools that referenced this issue Dec 12, 2019
When the expected type is a basic type, we will now offer a
corresponding type conversion candidate. For example:

var foo int64
foo = // offer "int64(<>)" as a candidate

The type conversion candidate will be ranked below matching concrete
candidates but above the sea of non-matching candidates.

This change broke almost every completion test. I added a new
completion option for literal candidates so tests can selectively ask
for literal completions.

Updates golang/go#36015.

Change-Id: I63fbdb33436d662a666c1ffd3b2d918d840dccc7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210288
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Dec 21, 2019

I think this can be closed out now?

@quasilyte

This comment has been minimized.

Copy link
Contributor Author

@quasilyte quasilyte commented Dec 22, 2019

I believe so.

@stamblerre stamblerre closed this Dec 23, 2019
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
4 participants
You can’t perform that action at this time.