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,cmd/compile/internal/types2: extend types.Info with implicit conversions from assignability #47151

Open
mdempsky opened this issue Jul 12, 2021 · 6 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 12, 2021

Currently, a statement like a = b may require an implicit conversion of b to a's type before the assignment can happen. In particular, for conversions to interface type, these generally require a run-time operation to change the value representation. (An implicit conversion can also happen for assignments between defined types and their underlying type literal or assignments from unrestricted channel types to send/receive-only channel types; but these are more of a formality, since Go implementations in practice use identical value representations for these cases.)

go/types provides enough information for users to infer/reconstruct these implicit conversions, but go/types already has to handle this as part of type checking anyway. So we might as well surface the information to users, so they don't have to remember all of the cases where implicit conversions happen.

My thoughts were along the lines of either:

  1. Add a types.Info.ImplicitTypes map of type map[ast.Expr]types.Type, which will record the type that the expression needs to be implicitly converted, if it differs from types.Info.Types[e].Type. (E.g., for var x int = int(42), int(42) would not need an entry in this map, because int(42) is already the same type as it's assigned to; but var x interface{} = int(42) would have an entry.)
  2. Same idea, but extend types.TypeAndValue with an extra Implicit Type field. Might be simpler/cleaner in some use cases, but it also implies more memory consumption (which I think at least gopls is very sensitive to).

Further, for N:1 assignments, I'd suggest the ImplicitTypes be present and a synthesized Tuple type if any of the respective tuple elements need implicit conversion. For example:

type MyBool bool
var f func() (int, int)
var g func(interface{}, interface{})
var a int
var b interface{}
var x MyBool

a, b = f() // 'f()' has Type (int, int); but ImplicitType (int, interface{}), due to assignment to a,b

g(f()) // 'f()' has Type (int, int); but ImplicitType (interface{}, interface{}), due to passing to g()

b, x = b.(int) // 'b.(int)' has Type (int, MyBool†); but ImplicitType (interface{}, MyBool), due to assignment to b,x
// † cmd/compile's legacy typechecker produces 'MyBool' in this case, but I think 'bool' or 'untyped bool' would be okay too

Incidentally, I think having a way to record separate types for the expression in isolation vs expression in context could help with cases like #13061.

/cc @griesemer @findleyr

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 12, 2021

/cc @dominikh too

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jul 12, 2021

Seems ok at first glance.
@findleyr for issues with API backward-compatibility.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jul 12, 2021

I think this makes sense. A big chunk of gopls' memory footprint is types.TypeAndValue, so yes I'd argue against adding a word there, but adding an additional map to types.Info should be fine. I don't see any compatibility issues with the additional map, though fully addressing #13061 can introduce some tricky compatibilty problems (example).

Bikeshedding: I used 'implicit type' in CL 242084, but I'm not sure if this phrasing was used before then. Just pointing out that there may not be consensus on that naming, and we might want to re-evaluate before committing it to an API. It might be confusing with the existing types.Info.Implicits. I'd still vote for it, though.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 12, 2021

I don't see any compatibility issues with the additional map, though fully addressing #13061 can introduce some tricky compatibilty problems (example).

Fair warning. I think it would be nice if we can fix the inconsistencies around declared constants and nil, but I don't see it as essential either.

Bikeshedding: I used 'implicit type' in CL 242084, but I'm not sure if this phrasing was used before then. Just pointing out that there may not be consensus on that naming, and we might want to re-evaluate before committing it to an API. It might be confusing with the existing types.Info.Implicits. I'd still vote for it, though.

Ack, I don't love the name and am open to suggestions. What about ImplicitConversions or AssignedTypes? (as in, the type of the expression after implicit conversion due to assignment).

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 12, 2021

@mdempsky how about ImpliedTypes?

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 12, 2021

@mdempsky how about ImpliedTypes?

I was going to say "implied" doesn't appear in the spec, but actually it does: "A conversion may appear literally in the source, or it may be implied by the context in which an expression appears."

I think that works.

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