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/ast, go/token: additions to support type parameters #47781

Closed
findleyr opened this issue Aug 18, 2021 · 18 comments
Closed

go/ast, go/token: additions to support type parameters #47781

findleyr opened this issue Aug 18, 2021 · 18 comments

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Aug 18, 2021

This proposal formally introduces the changes we’ve made to support parameterized functions and types in the go/ast and go/token packages. See the full write-up here:
https://go.googlesource.com/proposal/+/master/design/47781-parameterized-go-ast.md

There will be a separate issue for go/types, where type parameters add a much larger API surface.

We’ve gotten some experience with these new APIs over the last few months by using them in go/types. The additional MultiIndexExprIndexListExpr node type is the most significant change, but felt cleanest of the alternatives we considered.

Any feedback is appreciated.

CC @griesemer

Changelog:

  • TParams fields were renamed to TypeParams
  • MultiIndexExpr was renamed to IndexListExpr
@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2021
@findleyr findleyr changed the title proposal: go/ast, go/token: additions to support parameterized functions and types proposal: go/ast, go/token: additions to support type parameters Aug 18, 2021
@scott-cotton
Copy link

@scott-cotton scott-cotton commented Aug 19, 2021

Thanks for the proposal.

Typo: "invalid or invalid" -> "valid or invalid"?

I think the general representation of optional arguments/parameter is ok.

Perhaps a better name can be found for MultiIndexExpr: BracketedExpr? (I do not think of type parameters or instantiations as "indices")

But, I don't quite understand why suppositions about the completeness of a set of node types is a concern in the stdlib but the same is not a concern about token: The dev.typeparams commit 7c5d7a4 introduced token.TILDE. Why does the same concern not hold there? I think that one way to handle these issues is documentation: we could state that for go/* stdlib libraries, all exported sets of constants and types can be extended to facilitate language changes: do not assume switches handle all (possibly future) cases.

Could you post a link to the issue for the type proposal here when you are ready?

gopherbot pushed a commit to golang/proposal that referenced this issue Aug 19, 2021
invalid or invalid -> valid or invalid, as pointed out in
golang/go#47781 (comment)

(thanks)

Change-Id: I203a06a3280bb6a20ac87f78ddf49990b1812331
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/343549
Reviewed-by: Robert Findley <rfindley@google.com>
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Aug 19, 2021

@scott-cotton thank you for the feedback.

Typo: "invalid or invalid" -> "valid or invalid"?

Fixed, thanks.

Perhaps a better name can be found for MultiIndexExpr: BracketedExpr? (I do not think of type parameters or instantiations as "indices")

I think an advantage of MultiIndexExpr is similarity with IndexExpr. If we could start over these would be the same node, but since we can't it's important to keep them associated.

But, I don't quite understand why suppositions about the completeness of a set of node types is a concern in the stdlib but the same is not a concern about token

You're right that we should extend the same concern to TILDE. I was more concerned about node completeness because I was already aware of places in the x/tools codebase that would panic when encountering an unknown node.

One additional argument for adding a new node is that the panic exists to be sure your type switch covers all syntax. In this context, it would actually be unhelpful to hide the change by packing the new syntax into existing nodes. Perhaps that's overly optimistic.

Could you post a link to the issue for the type proposal here when you are ready?

Yes, will do.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Aug 23, 2021

@scott-cotton the go/types proposal is filed as #47916.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 24, 2021

What's the rationale for adding TParams to FuncType instead of FuncDecl? I can see it allows interface methods to have type parameters (in case we ever decide to support those). Are there any other reasons?

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Aug 24, 2021

@mdempsky that is the rationale. If we put TParams on the FuncDecl and then type parameters are allowed on interface methods, we'd likely end up with TParams on both FuncDecl and FuncType (or Field?!). Doing it this way hopefully keeps things clean, at the cost of a bit of memory now.

It wasn't an obvious choice though.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 25, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 25, 2021

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 Aug 25, 2021
@mvdan
Copy link
Member

@mvdan mvdan commented Sep 1, 2021

I'm slightly surprised by the choice of TParams as a name. TypeParams is just slightly longer, and much clearer. I also might mistake TParams for Params or vice versa when quickly reading or reviewing code.

There were several alternatives considered for representing this (MultiIndexExpr) syntax.

I agree with the decision to go with MultiIndexExpr. One major disadvantage of overloading existing types by adding fields is that existing tools might silently break in confusing ways, as they used to inspect or modify node types that can now represent new features. MultiIndexExpr is a new node, so it makes that transition much clearer.

There is code in the wild that assumes the completeness of node sets, i.e. panicking if an unknown node is encountered.

Continuing on the point above: this is how I write my type switches in general, including those using go/ast types. And I think this is good, even in the face of new node types. If a tool of mine needs updating to support type parameters, the tool will start panicking when the new syntax is used, and it will be abundantly clear (and probably rather easy) for me to add the extra switch case.

If the addition was made to an existing type, the strong and clear signal via a default case panic would not be there, and I might not notice that my tool actually needs changes to support new or changed node struct fields in existing types.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Sep 1, 2021

@mvdan

I'm slightly surprised by the choice of TParams as a name. TypeParams is just slightly longer, and much clearer.

Thanks for the feedback. I don't feel the same way, but I've been staring at this for a while. With TParams (and TArgs), I've grown accustomed to seeing 'T' as a modifier. With that said, we already have the TypeParam type in go/types, and you are right that TypeParams is not significantly more verbose. We can consider changing the naming convention, but of course if we decide to switch we should apply this change to go/types as well.

I agree with the decision to go with MultiIndexExpr.

Yes, I was concerned at first but have come around to adding a new node as the clear best option. With something like #47783 I don't think it will be a large burden on tool authors to update their code to avoid the panic, especially since many tools will want to have explicit support for type parameters anyway.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 1, 2021

It might be nice to have MultiIndexExpr next to IndexExpr in the docs. Maybe IndexListExpr?

@rsc
Copy link
Contributor

@rsc rsc commented Sep 1, 2021

It does seem a bit odd to have the type params in ast.FuncType and not ast.FuncDecl.
The receiver, for example, is only in ast.FuncDecl.

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Sep 2, 2021

@griesemer and I just discussed, and here is our current thinking:

  • As suggested by @mvdan here, and @rsc regarding #47916, it's clearer to just spell out Type rather than use T as a modifier. I'll go ahead and change TParam to TypeParam and TArg to TypeArg throughout both proposals.
  • IndexListExpr sounds good. I mildly prefer how MultiIndexExpr reads, but it does seem worthwhile being adjacent to IndexExpr in the docs.
  • Leaving the type params on ast.FuncType has the one distinct advantage of being forward compatible with type parameters on interface methods (and function literals, though it's less likely that matters). I think it's a valid critique to say that we shouldn't design our APIs around future language features that may never materialize, but I also don't think it's that odd to have type parameters associated with the func type. We're inclined to keep it as-is, but can reconsider if there are strong objections.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 8, 2021

Change https://golang.org/cl/348375 mentions this issue: go/ast: rename TParams fields to TypeParams

gopherbot pushed a commit that referenced this issue Sep 8, 2021
As discussed in the ast proposal (#47781), there's not really a strong
reason to avoid spelling out 'Type'.

Change-Id: I0ba1bf03b112ea60509a78a89a050a302779d9d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/348375
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@rsc
Copy link
Contributor

@rsc rsc commented Sep 8, 2021

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 Sep 8, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 8, 2021

Change https://golang.org/cl/348609 mentions this issue: go/ast: rename MultiIndexExpr to IndexListExpr

gopherbot pushed a commit that referenced this issue Sep 8, 2021
As discussed in #47781, IndexListExpr is one character shorter and has
the advantage of being next to IndexExpr in documentation.

Updates #47781

Change-Id: I709d5c1a79b4f9aebcd6445e4ab0cd6dae45bab7
Reviewed-on: https://go-review.googlesource.com/c/go/+/348609
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 8, 2021

Change https://golang.org/cl/348382 mentions this issue: 47781-parameterized-go-ast.md: rename MultiIndexExpr to IndexListExpr

gopherbot pushed a commit to golang/proposal that referenced this issue Sep 8, 2021
Following discussion in golang/go#47781, MultiIndexExpr was renamed to
IndexListExpr.

Change-Id: Ib8b0d138265d34b5764f5684a12c560838718ae2
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/348382
Reviewed-by: Robert Griesemer <gri@golang.org>
@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 15, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Sep 15, 2021

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/ast, go/token: additions to support type parameters go/ast, go/token: additions to support type parameters Sep 15, 2021
@rsc rsc removed this from the Proposal milestone Sep 15, 2021
@rsc rsc added this to the Backlog milestone Sep 15, 2021
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Sep 17, 2021

Thanks for the feedback, all!

Closing, as the accepted proposal matches the current implementation at tip.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 4, 2021

Change https://golang.org/cl/353882 mentions this issue: cmd/cgo: update to handle ast.IndexListExpr

gopherbot pushed a commit that referenced this issue Oct 7, 2021
Allows cgo to work with generics.

Updates #47781.

Change-Id: Id1a5d1a0a8193c5b157e3e671b1490d687d10384
Reviewed-on: https://go-review.googlesource.com/c/go/+/353882
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
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

6 participants