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

Jump to bottom

go/types,cmd/compile/internal/types2: extend types.Info with implicit conversions from assignability #47151

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

go/types,cmd/compile/internal/types2: extend types.Info with implicit conversions from assignability #47151

mdempsky opened this issue Jul 12, 2021 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@mdempsky
Copy link
Member

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 mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 12, 2021
@mdempsky
Copy link
Member Author

/cc @dominikh too

@griesemer
Copy link
Contributor

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

@findleyr
Copy link
Contributor

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

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 commented Jul 12, 2021

@mdempsky how about ImpliedTypes?

@mdempsky
Copy link
Member Author

@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.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@seankhliao seankhliao added this to the Backlog milestone Aug 20, 2022
@mdempsky mdempsky mentioned this issue Sep 5, 2023
Open
@adonovan
Copy link
Member

adonovan commented Sep 6, 2023

This is essentially a dup of #8970. In other words, this feature would obviate the golang.org/x/tools/refactor/satisfy package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Backlog
Development

No branches or pull requests
7 participants
@mdempsky @cuonglm @adonovan @griesemer @gopherbot @seankhliao @findleyr