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

syntax: parser doesn't recognize valid type parameter list #49482

Closed
griesemer opened this issue Nov 9, 2021 · 16 comments
Closed

syntax: parser doesn't recognize valid type parameter list #49482

griesemer opened this issue Nov 9, 2021 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@griesemer
Copy link
Contributor

There is a (known) ambiguity for type parameter lists. For

type T[P *int] struct{}

the parser cannot tell if this is a generic type declaration or an array type declaration with length P*int. In general, people will write ~*int and there is the work-around interface{*int}; one just has to be aware of it.

But the parser also assumes that

type T[P *int, Q any] struct{}

is starting an array type declaration, yet this is clearly a valid type parameter list.

Should be fixed for 1.18 but is not a release blocker as there are work-arounds.

cc: @findleyr @ianlancetaylor

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 9, 2021
@griesemer griesemer added this to the Go1.18 milestone Nov 9, 2021
@griesemer griesemer self-assigned this Nov 9, 2021
@griesemer
Copy link
Contributor Author

Another work-around for the single type-parameter case would be a trailing comma, as in

type T[P *int,] struct{}

because type parameter lists (like all parameter lists) permit a trailing comma, while array type declarations don't.

@ianlancetaylor
Copy link
Contributor

I assume that this also works:

type T[P (*int)] struct{}

@griesemer
Copy link
Contributor Author

It should but currently doesn't, it appears. Filed a separate issue #49485.

@griesemer
Copy link
Contributor Author

It turns out that using parentheses does not work in general.

@gopherbot
Copy link

Change https://golang.org/cl/370774 mentions this issue: cmd/compile/internal/parser: fix parsing of type parameter lists

@gopherbot
Copy link

Change https://golang.org/cl/372874 mentions this issue: spec: describe constraint parsing ambiguity and work-around more precisely

gopherbot pushed a commit that referenced this issue Dec 17, 2021
…isely

The new description matches the implementation (CL 370774).

Also, in the section on type constraints, use "defines" instead of
"determines" because the constraint interface defines the type set
which is precisely the set of acceptable type arguments.

For #49482.

Change-Id: I6f30f49100e8ba8bec0a0f1b450f88cae54312eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/372874
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/385575 mentions this issue: go/parser, go/printer: fix parsing of ambiguous type parameter lists

@findleyr findleyr changed the title cmd/compile: parser doesn't recognize valid type parameter list go/parser: parser doesn't recognize valid type parameter list Feb 14, 2022
@findleyr
Copy link
Contributor

findleyr commented Feb 14, 2022

Reopening as while auditing our port sheet I realized this fix did not get ported to go/parser. I have mailed the port in https://go.dev/cl/385575. Tentatively marking as a release blocker: the original bug may not have been a release blocker, but it seems like a release blocker that the compiler accepts these expressions and go/parser does not, even if they will be uncommon in practice.

@findleyr
Copy link
Contributor

CC @golang/release @mdempsky

As mentioned above, I think this may be a release blocker. Since the CL is mailed, I have added the release-blocker label in hopes that it can get merged before the RC1. Please reach out to me if any concerns.

@findleyr
Copy link
Contributor

While working on the port, I realized that there are valid cases that both parsers can't currently handle. For example:

type _[P *struct{}|int] struct{}  // example 1
type _[P *struct{}|~int] struct{}  // example 2

Example 1 is parsed as an array type, though it can be unambiguously parsed as a generic type declaration. Example 2 is a parser error.

I have a fix for both cases, but since this is not trivial I think it must wait until Go 1.19. I believe the CL above makes go/parser bug-compatible with the compiler.

@gopherbot
Copy link

Change https://go.dev/cl/385756 mentions this issue: go/types, types2: add tests for literals in type parameter lists

@gopherbot
Copy link

Change https://go.dev/cl/385757 mentions this issue: go/parser: fix parsing of unambiguous type parameter lists

gopherbot pushed a commit that referenced this issue Feb 15, 2022
This is a port of CL 370774 to go/parser and go/printer. It is adjusted
for the slightly different factoring of parameter list parsing and
printing in go/parser and go/printer.

For #49482

Change-Id: I1c5b1facddbfcb7f7b2be356c817fc7e608223f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/385575
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@findleyr findleyr modified the milestones: Go1.18, Go1.19 Feb 15, 2022
@findleyr
Copy link
Contributor

Thanks to @mdempsky's help, we landed the fix to make go/parser and go/printer consistent with the compiler. Moved this to 1.19, when we can consider further improvements to our parsing of ambiguous type parameter lists.

gopherbot pushed a commit that referenced this issue Feb 15, 2022
Add tests that verify consistent behavior of go/types and types2 with
respect to potentially ambiguous type parameter lists.

For #49482

Change-Id: I3386d4fa3eb91f2a8ea0987372ca40a6962de886
Reviewed-on: https://go-review.googlesource.com/c/go/+/385756
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@griesemer griesemer changed the title go/parser: parser doesn't recognize valid type parameter list syntax, go/parser: parser doesn't recognize valid type parameter list Mar 30, 2022
@gopherbot
Copy link

Change https://go.dev/cl/402255 mentions this issue: cmd/compile/internal/syntax: parser to accept ~x as unary expression

@gopherbot
Copy link

Change https://go.dev/cl/402256 mentions this issue: cmd/compile/internal/syntax: accept all valid type parameter lists

@griesemer griesemer changed the title syntax, go/parser: parser doesn't recognize valid type parameter list syntax: parser doesn't recognize valid type parameter list Apr 26, 2022
gopherbot pushed a commit that referenced this issue Apr 26, 2022
Accept ~x as ordinary unary expression in the parser but recognize
such expressions as invalid in the type checker.

This change opens the door to recognizing complex type constraint
literals such as `*E|~int` in `[P *E|~int]` and parse them correctly
instead of reporting a parse error because `P*E|~int` syntactically
looks like an incorrect array length expression (binary expression
where the RHS of | is an invalid unary expression ~int).

As a result, the parser is more forgiving with expressions but the
type checker will reject invalid uses as before.

We could pass extra information into the binary/unary expression
parse functions to prevent the use of ~ in invalid situations but
it doesn't seem worth the trouble. In fact it may be advantageous
to allow a more liberal expression syntax especially in the presence
of errors (better parser synchronization after an error).

Preparation for fixing #49482.

Change-Id: I119e8bd9445dfa6460fcd7e0658e3554a34b2769
Reviewed-on: https://go-review.googlesource.com/c/go/+/402255
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/404397 mentions this issue: cmd/compile/internal/syntax: fix printing of ambiguous constraint literals

gopherbot pushed a commit that referenced this issue May 6, 2022
…erals

Without this change, the type parameter list "[P T | T]" is printed
as "[P T | T,]" in an attempt to avoid an ambiguity. But the type
parameter P cannot syntactically combine with the constraint T | T
and make a new valid expression.

This change introduces a specific combinesWithName predicate that
reports whether a constraint expression can combine with a type
parameter name to form a new valid (value) expression.

Use combinesWithName to accurately determine when a comma is needed.

For #49482.

Change-Id: Id1d17a18f0c9af04495da7b0453e83798f32b04a
Reviewed-on: https://go-review.googlesource.com/c/go/+/404397
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants