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: revisit mapping of TypeNames to TypeParams #45935

Closed
mdempsky opened this issue May 4, 2021 · 14 comments
Closed

go/types: revisit mapping of TypeNames to TypeParams #45935

mdempsky opened this issue May 4, 2021 · 14 comments
Labels
NeedsInvestigation
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented May 4, 2021

When typechecking dev.go2go/src/cmd/go2go/testdata/go2path/src/chans/chans.go, go/types gives Exclusive the type:

type chans.Exclusive struct{c chan Val₁₂₆}

but its corresponding method has type:

func (*chans.Exclusive[Val₁₂₈]).Acquire() Val₁₂₈

I'm a bit surprised that Val₁₂₈ is a separate *types.TypeParam instance from Val₁₂₆. I get that it's a separate identifier with its own binding and scoping (and possibly different spelling), but I would expect the identifiers to map to the same underlying *types.TypeParam instance.

Are the separate TypeParam instances intentional/necessary? It seems a little tedious (and I'd imagine error-prone) translating between them.

/cc @griesemer @findleyr

@mdempsky mdempsky added the NeedsInvestigation label May 4, 2021
@mdempsky mdempsky added this to the Go1.18 milestone May 4, 2021
@griesemer
Copy link
Contributor

griesemer commented May 4, 2021

This is intentional. Each method declares its own type parameters. Also, methods are not instantiated when a type is instantiated (to avoid all kinds of problems with cycles). They are instantiated when the method is looked up.

There may be better implementation strategies down the road, but at least this is not a bug.

@mdempsky
Copy link
Member Author

mdempsky commented May 4, 2021

This is intentional. Each method declares its own type parameters.

It's a bit ambiguous to me what you mean here, since there's the Object vs Type distinction within go/types. Specifically, I'm suggesting that methods on parameterized receiver types should still have their own TypeName instances, but these should point to the same TypeParam as the receiver type's declaration uses. Similar to how type aliases work.

Also, methods are not instantiated when a type is instantiated (to avoid all kinds of problems with cycles). They are instantiated when the method is looked up.

Can you elaborate on how type instantiation is relevant here? E.g., if Exclusive.Acquire used Val₁₂₆ directly instead of Val₁₂₈, at lazy instantiation time isn't it just a matter of substituting a different variable?

@griesemer
Copy link
Contributor

griesemer commented May 4, 2021

To clarify: Each method declares its own TypeName objects (one per type parameter) which are in a 1:1 correspondence to their own local type parameters. It may be possible for TypeName objects to share corresponding type parameters - I don't know and haven't focussed on it - I am just stating that this is the current implementation. The best representation of type parameters is something that I think we're still learning about.

The comment on instantiations of methods is unrelated. I just mentioned it as an additional thing that is "not coupled" with the receiver type.

@mdempsky
Copy link
Member Author

mdempsky commented May 4, 2021

Ack that this is currently working as intended, and it's not urgent to change (or even necessarily desirable). But as a user-visible detail of the go/types API, it does seem like something we want to consider before Go 1.18. That's why I filed this issue: I think this might be a worthwhile way to simplify/improve the API, and I wanted to make sure it's on our radar when the time comes to consider tweaks like this.

Is there somewhere else to track API feedback?

@griesemer griesemer self-assigned this May 4, 2021
@griesemer griesemer changed the title go/types: methods have separate types.TypeParams from their receiver type go/types: revisit mapping of TypeNames to TypeParams May 4, 2021
@griesemer
Copy link
Contributor

griesemer commented May 4, 2021

Ack that this is user-visible and may need a revisit.

@findleyr
Copy link
Contributor

findleyr commented May 5, 2021

Is there somewhere else to track API feedback?

I'm going to file issue(s) soon for the go/ast and go/types API changes, though to be honest this wasn't on my radar. Let's keep this issue open, and I can reference it in the go/types API proposal.

@griesemer
Copy link
Contributor

griesemer commented Sep 24, 2021

@findleyr Is there anything left to do here? (If not, please close.) We're not going to change the mapping at this point (and we already moved from lists of TypeNames to lists of TypeParams internally, which was an improvement).

@findleyr
Copy link
Contributor

findleyr commented Sep 24, 2021

Agreed, I don't think there's anything to do here.

In particular, it has proven useful to have a bijection of TypeName<->TypeParam (via the .Obj() method), similar to Named. This would not be possible if the multiple TypeNames referred to the same TypeParam.

@schroederc
Copy link

schroederc commented Oct 5, 2021

I've run into this myself. I understand that the current implementation may be useful, but I'm wondering what is the intended method of mapping between these separate TypeParam instances if a tool wants to union their references.

@griesemer
Copy link
Contributor

griesemer commented Oct 5, 2021

Type parameters have an index (go/types.TypeParam.Index()) which identifies them. We are also considering providing an "owner" (the object which declares them).

cc: @findleyr in case he wants to add something

@findleyr
Copy link
Contributor

findleyr commented Oct 5, 2021

As @griesemer said, Index() is the way to do the join.

@schroederc can you say more about what you're trying to do? It would help us to understand your use-case.

@schroederc
Copy link

schroederc commented Oct 5, 2021

I'm working on adding support for generics to the Kythe indexer that provides xrefs for CodeSearch.

Looks like that API is still in flux: https://github.com/golang/go/blob/master/src/go/types/typeparam.go#L54. Is there an ETA or a bug I can subscribe to for updates?

@findleyr
Copy link
Contributor

findleyr commented Oct 5, 2021

Oh, that was an oversight. I thought Index was exported.

https://golang.org/issue/47916 is the tracking bug.

@findleyr
Copy link
Contributor

findleyr commented Oct 5, 2021

Slight correction: I remember now that we intentionally held off on exporting Index, with the assumption that it would always be apparent from the surrounding code. It sounds like that's not the case for you. I see no harm in exporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

4 participants