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 Object.Origin [freeze exception] #51682

Closed
findleyr opened this issue Mar 15, 2022 · 20 comments
Closed

go/types: add Object.Origin [freeze exception] #51682

findleyr opened this issue Mar 15, 2022 · 20 comments

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 15, 2022

I propose to add the following methods to types.Var and types.Func:

// Origin returns the canonical object for its receiver, i.e. the object
// recorded in Info.Defs.
//
// For instantiated variables Origin returns the type-parameterized object from
// which the receiver was created. For all other variables Origin returns the
// receiver.
func (obj *Var) Origin() *Var 

// Origin returns the canonical object for its receiver, i.e. the object
// recorded in Info.Defs.
//
// For instantiated functions Origin returns the type-parameterized object from
// which the receiver was created. For all other functions, Origin returns
// receiver.
func (obj *Func) Origin() *Func

While instantiating types, we necessarily have to create instantiated copies of their methods/fields/parameters/etc. However, as we work on updating tools for generics, an emerging theme is that is useful to be able to answer the following two questions:

  1. Is this object canonical?
  2. What is the canonical object corresponding to this object?

Right now, these are not easy questions to answer. For methods, these can be answered by a lookup on the receiver base origin type. For fields / parameters / etc., it is more difficult and may require finding the relevant *ast.Ident and looking in Info.Defs.

Adding an Origin method to these types answers both of these questions trivially, at the cost of a pointer per object. If the added memory is a concern, there are several internal fields on types.object used during type-checking that may be externalized to free memory.

CC @griesemer @dominikh @timothy-king @mdempsky

@gopherbot gopherbot added this to the Proposal milestone Mar 15, 2022
@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 15, 2022

Does it make sense to have Origin on each Object? Isn't it sufficient to have it for TypeNames and Funcs?

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 15, 2022

Isn't it sufficient to have it for TypeNames and Funcs?

We'd need it for Vars as well (and technically we don't need it for TypeNames: we don't expose any non-canonical TypeNames). When we instantiate structs / signatures, we instantiate vars.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 15, 2022
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 15, 2022

Do we want more experience with the existing alternatives available before making a decision?

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 15, 2022

Do we want more experience with the existing alternatives available before making a decision?

It's fine to wait if we think we'll get more signal, but I do think we should add something for 1.19. This feels like a philosophical oversight: the canonical nature of Objects was broken without an API to easily 'canonicalize'.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 16, 2022

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

@rsc rsc moved this from Incoming to Active in Proposals Mar 16, 2022
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 23, 2022

We'd need it for Vars as well (and technically we don't need it for TypeNames: we don't expose any non-canonical TypeNames). When we instantiate structs / signatures, we instantiate vars.

I think it would be fine to simply add Origin to the objects that could possibly be instantiated: Var and Func, I think. That is more conservative, and we can always add it to the Object interface later.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 23, 2022

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

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 23, 2022
@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 23, 2022

@findleyr The more conservative approach seems fine. Can you please send out a CL soon so we can see the implications in detail?

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 24, 2022

Change https://go.dev/cl/395535 mentions this issue: go/types: set an origin object for vars and funcs

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Mar 29, 2022

https://go.dev/cl/395535 contains a prototype of the new API, which is added only to *Func and *Var.

A notably questionable decision: I opted to have the new methods return Object, even though they could return *Func and *Var, respectively, to leave open the possibility that this API is added to the Object interface in the future. By comparison, (*Named).Origin returns *Named, so maybe we should opt for the more specific results. ...as I write this I am thinking that returning concrete types is probably this is the better option, but I would pose the question to the audience in any case.

@dominikh
Copy link
Member

@dominikh dominikh commented Mar 29, 2022

I'm leaning towards returning Object. Even without the API being added to Object, users can define their own interface and get the origin object with a single type assertion/call. (*Named).Origin returning *Named doesn't seem problematic, since Named isn't an Object and thus isn't technically part of the same API.

@rsc rsc moved this from Likely Accept to Active in Proposals Mar 30, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Mar 30, 2022

Moving back to active to resolve the discussion about the result type.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Apr 6, 2022

Discussed this with @griesemer, and it seems like the arguments for concrete result types are compelling:

  • Fewer type assertions at the call site.
  • Less likely to introduce a typed nil.

On the other hand, we wouldn't be able to pass around interface{ Origin() Object } as @dominikh suggests, but it would of course be easy to write a helper func originObject(Object) Object.

By way of analogy, we document that Type() of a *Func is always *Signature, yet I have seen many places in x/tools where we use a commaok type assertion and handle the case that the Type() of a *Func is not a *Signature. I would guess that returning Object will lead to similar misuse.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 6, 2022

@findleyr, thanks for updating the top comment with the proposed API. Does anyone object to this change?

@rsc
Copy link
Contributor

@rsc rsc commented Apr 13, 2022

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

@rsc rsc moved this from Active to Likely Accept in Proposals Apr 13, 2022
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Apr 25, 2022

CC @adonovan :)

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 4, 2022
@rsc
Copy link
Contributor

@rsc rsc commented May 4, 2022

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

@rsc rsc changed the title proposal: go/types: add Object.Origin go/types: add Object.Origin May 4, 2022
@rsc rsc removed this from the Proposal milestone May 4, 2022
@rsc rsc added this to the Backlog milestone May 4, 2022
@findleyr findleyr changed the title go/types: add Object.Origin go/types: add Object.Origin [freeze exception] May 17, 2022
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented May 17, 2022

CC @golang/release

This issue was filed early in the dev cycle, but due to ongoing discussion and the abbreviated cycle, was not accepted until a few days before the freeze. With other priorities in go/types (particularly the urgent https://go.dev/issue/52715), I didn't get to the implementation of this feature before the freeze.

I'd like to request a freeze exception to land this proposal for 1.19, as it allows us to redefine a 'canonical' invariant that was broken with generics. Certain types of static analysis (such as dominikh/go-tools#1215 or some Google-internal analyses) would be facilitated by these new APIs.

Please let me know if you'd like me to file a new issue for the freeze exception request, or if you need any other information to help make a determination.

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented May 17, 2022

This freeze exemption has been approved. Nothing else is required for the exemption request.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented May 17, 2022

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests

7 participants