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

proposal: go/parser: add a mode flag to disallow the new syntax for type parameters / instantiation #47783

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

Comments

@findleyr
Copy link
Contributor

findleyr commented Aug 18, 2021

In #47781, we propose to add a new node type to go/ast to support type and function instantiation. One of our concerns with this change is that there is code in the wild that assumes the completeness of the current set of go/ast nodes, and that this code will now panic when encountering an unknown node type (see golang/vscode-go#1551, for example). This concern was also raised in the monthly tools call.

Whatever final form the new APIs take, it would be helpful to provide a way for tool authors to prevent go/parser from producing the new syntax nodes. By analogy, users of go/types can set the Config.GoVersion field (see #46648).

While it might be a good idea to eventually add a go/parser.Config type with a GoVersion field. That is a large change that would be a burden on calling code: users would have to switch away from the parser.Parse* APIs. It is much easier for users to add a mode flag.

Therefore this issue proposes to add a new mode flag that tells go/parser not to accept the new syntax for type parameters and instantiation. This is actually already implemented via an unexported flag value, so the hardest part of implementing this proposal would be coming up with a name.

I suggest NoTypeParameters, but I don't love it. Other ideas are welcome, though note that we have been trying to avoid the word 'generics' in favor of the more precise 'type parameter', 'parameterized', etc.

CC @griesemer

@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 25, 2021
@rsc
Copy link
Contributor

rsc commented Aug 25, 2021

As long as the default behavior is to accept the full Go language, then it seems fine to introduce DisallowTypeParameters as a temporary bandaid that old tools can adopt as a quick fix before they add proper support.

@griesemer
Copy link
Contributor

For reference, the syntax parser has a mode called AllowGenerics. Since the default is now to allow them, we might invert that (internal) mode and rename it to DisallowGenerics (or DisallowTypeParameters).

@rsc
Copy link
Contributor

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

@mvdan
Copy link
Member

mvdan commented Sep 1, 2021

I'm not sure I understand the worry that leads to this proposal. Tools or libraries that don't yet support type parameters might panic or error if they see the new go/ast node types in Go 1.18. Presumably, that would lead to them either adding support for the new feature, or to add a few lines of code that gives an error like "type parameters not supported" in those cases.

How would a NoTypeParameters mode help in that situation? The end user would see some form of parse error, so it's about the same end result. The tool or library still needs to be modified to start using the mode, so in my eyes it's not making the transition smoother either.

I'm also slightly worried about scenarios where the code calling the Parse APIs is in a different module than the code that uses type switches on go/ast node types. For example: gopls parses code, and feeds an *ast.File to an external module, like https://pkg.go.dev/mvdan.cc/gofumpt/format#File. From the point of view of the library, it's too late to say if it wants to avoid type parameters or not. And if it wishes to simply not support type parameters, it can return a user-friendly error message without this proposed API.

@findleyr
Copy link
Contributor Author

findleyr commented Sep 1, 2021

@mvdan these are great points.

The intent of this flag is to help with cases where it might not be trivial to update code to support type parameters, and a parser error is preferable to a panic. But as you point out, this can only ever really be a temporary workaround, and is only helpful in certain conditions. Maybe these conditions are rare enough that it is not worth a new mode flag.

CC @marwan-at-work who had brought this up in the tools call.

@mvdan
Copy link
Member

mvdan commented Sep 1, 2021

I get that some tools or libraries might take effort to support type parameters. They could return a temporary error message in the meantime, perhaps pointing to an issue tracking the progress. Or if they don't support type parameters by definition, perhaps link to a page explaining the reason why. Both of those seem more useful to me than a blanket "no type parameters" mode, do not require the Parse and type switch to happen in the same module, and do not require new API :)

@findleyr
Copy link
Contributor Author

findleyr commented Sep 1, 2021

Yes, in that context this would only be helpful where setting this mode flag is significantly easier than finding and updating all the places that might panic or behave incorrectly when encountering the new syntax.

That's the critical coefficient that justifies the new mode flag, and perhaps I overestimated it.

@mvdan
Copy link
Member

mvdan commented Sep 1, 2021

Ah I see, that makes sense. To provide a data point, I just did some quick grepping over my 6 modules using go/ast, and they seem to have about 2-5 node type switches each. About a quarter of them are "exhaustive" with a default case that panics; the others just care about a few node types in particular.

So, at least for my software, I likely would only need to update one or two type switches per module to add the error returns. I wouldn't mind that, given that introducing new AST node types should be rare. And I'd have to edit those files anyway, if I wanted to add support for type parameters, which I hope we all do :)

@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

It sounds like maybe this flag doesn't add as much as we thought.
Certainly having generics off by default and opt-in would avoid breaking users,
but that's not the right default for the next ten years.
Having the default be on with an opt-out, the opt-out seems like maybe not significantly less work than putting a new case in a type switch here and there to say "no generics yet".

Do I have that right?

@mvdan
Copy link
Member

mvdan commented Sep 1, 2021

That seems like a good summary. My only brief addition would be: some tools expose themselves as libraries taking go/ast nodes, so they don't have the ability to provide go/parser options. https://pkg.go.dev/golang.org/x/tools/go/analysis is a good example; an analysis simply receives a bunch of ASTs.

@findleyr
Copy link
Contributor Author

findleyr commented Sep 3, 2021

Unless anyone has concerns, I'll retract this proposal.

I'll leave it open until Tuesday (before the next proposal meeting) in case anyone wants to object.

@scott-cotton
Copy link

It sounds like maybe this flag doesn't add as much as we thought.
Certainly having generics off by default and opt-in would avoid breaking users,
but that's not the right default for the next ten years.
Having the default be on with an opt-out, the opt-out seems like maybe not significantly less work than putting a new case in a type switch here and there to say "no generics yet".

Do I have that right?

It may be nice to document "new in 1.18" or exactly which cases of switches suffice to check for type parameters.

Scott

@findleyr
Copy link
Contributor Author

findleyr commented Sep 6, 2021

It may be nice to document "new in 1.18" or exactly which cases of switches suffice to check for type parameters.

Ack. We'll be sure to highlight this in the 1.18 release notes, perhaps linking to the go/ast proposal.

@findleyr
Copy link
Contributor Author

findleyr commented Sep 7, 2021

Closing as nobody has objected. Please ping this issue if you have any concerns.

@findleyr findleyr closed this as completed Sep 7, 2021
@rsc rsc moved this from Active to Declined in Proposals (old) Sep 22, 2021
@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

This proposal has been declined as retracted.
— rsc for the proposal review group

@golang golang locked and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants