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

go/types: review/document AssignableTo code path for untyped value types #32146

Open
muirrn opened this issue May 19, 2019 · 4 comments

Comments

@muirrn
Copy link

commented May 19, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

What did you do?

I called types.AssignableTo to see if an untyped int constant was assignable to a named int type.

https://play.golang.org/p/Xq33qWpFsy5

What did you expect to see?

I expected it to be assignable.

What did you see instead?

It was reported as not assignable.

At first I thought it was due to an implicit type conversion and they weren't technically assignable, but in the spec assignability seems to include implicit conversions, at least for non-constant values. For untyped constants in particular it says an untyped constant x is assignable to type T if x is in the set of values determined by T.. I'm not sure exactly what that means or if it disqualifies this case. In general since the untyped constant is assignable to the named type, I assumed the AssignableTo function would agree.

Also note that reflect.Type.AssignableTo behaves consistently with types.AssignableTo.

/cc @griesemer since you are primary owner of go/types

@muirrn

This comment has been minimized.

Copy link
Author

commented May 20, 2019

On further thought I suspect the problem is AssignableTo doesn't know the constant's value. If it were to return true in this case then there would be false positives due to integer sign mismatch and overflow.

Taking a step back, I'm trying to make gopls (the x/tools LSP implementation) be smarter when suggesting untyped constants as completion candidates (i.e. it should only suggest a constant if it is assignable to the expected type at the cursor). In other words, I want to determine if a given *types.Const is assignable to a given types.Type. It looks like the AssignableTo code path would do the right thing if there was a way to set the operand's "val".

@bcmills bcmills added this to the Unplanned milestone May 20, 2019
@griesemer

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I agree it that the go/types behavior deserves at least a closer look in this case.

@griesemer griesemer self-assigned this May 22, 2019
@griesemer griesemer modified the milestones: Unplanned, Go1.14 May 22, 2019
@griesemer

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@muirrn Indeed, AssignableTo doesn't know about the untyped int value in this case. That said, returning false is also not very satisfactory; it should probably return some error (or panic) - but we can't change that.

Barring adding another entry point to the API, I don't know what else can be done here, but I leave this open (with reframed title) as a reminder to review the code path (and document it better).

@griesemer griesemer changed the title go/types: AssignableTo false negative for untyped constants go/types: review/document AssignableTo code path for untyped value types May 22, 2019
@muirrn

This comment has been minimized.

Copy link
Author

commented May 22, 2019

Thanks, sounds reasonable.

For others' future reference, to prefer false positives over false negatives I did something like https://play.golang.org/p/1G9aEFaLenH

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.