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: add Alias.Rhs method #66559

Closed
adonovan opened this issue Mar 27, 2024 · 14 comments
Closed

go/types: add Alias.Rhs method #66559

adonovan opened this issue Mar 27, 2024 · 14 comments

Comments

@adonovan
Copy link
Member

adonovan commented Mar 27, 2024

Proposal Details

Background: Go 1.22's go/types defined new API for materialized alias types, as a step towards generic alias types. Alias has two operations: Underlying, which removes all Alias and Named constructors, recursively, returning the representation type; and Unalias, which removes only Alias constructors, again recursively, returning the first non-alias type. (A judicious sprinkling of Unalias operations throughout existing code is the minimal change to keep most existing go/types clients working.)

However, the API provides no way to remove a single Alias constructor. That means, for example, that there is no way for a client of go/types, given the types for:

type A = B
type B = int

to write a function that prints "type A = B" (without calling the existing ObjectString method). More importantly, import/export tools are unable to preserve the structure of the original types.

(Unalias is nonetheless useful, and Underlying is of course a requirement of the Type interface, but Alias types are not isomorphic to defined types in the way that, given type A B; type B int, the spec defines the underlying type of A as int, and otherwise stipulates no relationship between A and B.)

Proposal: We add an Alias.RHS accessor method, which removes exactly one Alias constructor. So, Alias.RHS(A) would return B.

@gri @findleyr @mdempsky

@gopherbot gopherbot added this to the Proposal milestone Mar 27, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/574716 mentions this issue: go/types: ObjectString: remove only 1 Alias for "type A = RHS"

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

Seems fine but we already spell it Rhs in other places, so we should do the same here.

@rsc rsc changed the title proposal: go/types: add Alias.RHS accessor, which removes exactly one Alias constructor proposal: go/types: add Alias.Rhs method Mar 27, 2024
@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@adonovan
Copy link
Member Author

Seems fine but we already spell it Rhs in other places, so we should do the same here.

I'm not convinced: Rhs is not idiomatic spelling, and ast.AssignStmt.Rhs is essentially unrelated to types.Alias.RHS (you wouldn't plumb one through to the other, for example) so I don't see any downside to using the correct capitalization.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Mar 27, 2024

Note that go/types.Initializer also spells it Rhs.

That is, it seems like we shouldn't use two different spellings in the same package.

@rsc
Copy link
Contributor

rsc commented Mar 28, 2024

The downside is that I am a gopher of very little brain, and I will never remember when to use Rhs and when to use RHS.
The general rule is spell the same words the same way, so my brain can remember other things instead.

@findleyr
Copy link
Contributor

I'm not opposed, but I don't really follow the argument that Aliases are fundamentally different from Named types here. If we want to be able to print type A = B, why would we not also want to be able to print type A B? Isn't the question whether we preserve (and expose) the syntactic relationship in the type system?

Historically, we've intentionally avoided preserving inessential syntax in types, with Interface embeddings being the one notable exception. In fact, I think this philosophy is why we didn't have an Alias type in the first place.

Can we clarify our philosophical approach? Have we decided that it's generally a good idea to preserve the syntax of type definitions in the type system? If so, should we add Named.Rhs?

Is this proposal also suggesting that we change the String value of an Alias? If so, we could of course do that without also exposing the Rhs accessor (though that would be inconsistent).

@adonovan
Copy link
Member Author

OK, Rhs is is.

I don't really follow the argument that Aliases are fundamentally different from Named types here.

The main motive is that transiting export data should not make material changes to types, as this would cause tools like gopls to display different behavior depending on whether it obtained the types from a cache of export data or from type-annotated syntax. In the case of type A = B, the string representation shows the RHS type as declared; if the exporter were forced to use Unalias, then the reimported type would be type A = int, which would be confusing and even nondeterministic.

By contrast, Named types do not reveal their middlemen in their API or their printed representation. (You raise an interesting question as to whether we would be better off if they did, but I don't think we need to address it now. IMHO the more troublesome difficulty with named types is not being able to distinguish A.f from B.f in type A B; type B struct{f int}, but we can't fix that.)

Is this proposal also suggesting that we change the String value of an Alias?

No.

@findleyr
Copy link
Contributor

Got it. So the problem is that there is an observable behavior of Aliases (their current type string) that can't be preserved through import. I agree that this needs to be fixed. I also agree that the behavior of showing the RHS in the type string is correct.

We can worry about philosophy with respect to Named types later.

gopherbot pushed a commit that referenced this issue Apr 3, 2024
As we migrate towards materialized Alias types, the ObjectString
for a type A such as
   type A = B
   type B = int
should be "type A = B", removing exactly one Alias constructor
from the type of A. (The previous behavior was "type A = int".)

I suspect the existing Alias.{Unalias,Underlying} API is
inadequate and that we will need an Alias.RHS accessor that
removes exactly one Alias. Other clients such as the import/
export packages will need it, because aliases are not
isomorphic to defined types, in which, given
   type A B
   type B int
the Underlying of A is indeed int. See #66559.

Change-Id: I11a4aacbe6dbeeafc3aee31b3c096296b5970cd8
Reviewed-on: https://go-review.googlesource.com/c/go/+/574716
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add in go/types

func (*Alias) Rhs() (Type)

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add in go/types

func (*Alias) Rhs() (Type)

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add in go/types

func (*Alias) Rhs() (Type)

@rsc rsc changed the title proposal: go/types: add Alias.Rhs method go/types: add Alias.Rhs method Apr 24, 2024
@rsc rsc modified the milestones: Proposal, Backlog Apr 24, 2024
@adonovan adonovan self-assigned this Apr 24, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/581615 mentions this issue: go/types: add Alias.Rhs

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/581635 mentions this issue: internal/gcimporter: use Alias.Rhs, not unsafe hack

gopherbot pushed a commit to golang/tools that referenced this issue Apr 29, 2024
Unfortunately we need an aliases.Rhs shim to handle the
three cases (before, at, after go1.22).

(Alias.Rhs was recently added in CL CL 581615.)

Updates golang/go#66559

Change-Id: I25a35c14f3ef5ddb77712afcce17f960dd181b5c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/581635
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

6 participants