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

Jump to bottom

go/parser: audit error recovery for go1.17 #46403

Closed
findleyr opened this issue May 26, 2021 · 5 comments
Closed

go/parser: audit error recovery for go1.17 #46403

findleyr opened this issue May 26, 2021 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.17

Comments

@findleyr
Copy link
Contributor

With all the changes to go/parser merged from dev.typeparams, at least some error recovery behavior has changed in go 1.17. For example, a[] parses to a[BadExpr] on go1.16, but just BadExpr on go1.17.

Error recovery in the parser is best-effort, but we should audit changes to ensure they are acceptable. We should probably avoid changes such as the example above that lose information.

CC @griesemer
@findleyr findleyr added release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels May 26, 2021
@findleyr findleyr added this to the Go1.17 milestone May 26, 2021
@findleyr findleyr self-assigned this May 26, 2021
@findleyr findleyr added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 26, 2021
@heschi heschi removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 10, 2021
@toothrot
Copy link
Contributor

@findleyr We're close to RC1, and this issue is a release blocker. Any updates?

@findleyr
Copy link
Contributor Author

Thanks.

This is mostly procedural, though I'm aware of one minor parser change that must be made. I can make sure to close this out by this Monday the 21st, does that work?

@findleyr findleyr mentioned this issue Jun 17, 2021
Closed
@ianlancetaylor
Copy link
Contributor

Sure, next Monday is fine. Thanks.

@gopherbot
Copy link

Change https://golang.org/cl/329791 mentions this issue: go/parser: parse an ast.IndexExpr for _[]

@findleyr
Copy link
Contributor Author

In addition to #46855, and _[], fuzzing has found a number of test cases that differ between 1.16 and 1.17 related to the parsing of function arguments or index expressions. For example:

  • package p; func f\xado() {}: func f(o BadExpr, _ BadExpr) on 1.16, func f(o BadExpr) on 1.17
  • package p;var x=a[[": a[BadExpr] on 1.16, a[[BadExpr]BadExpr] on 1.17

These seem to be reasonable changes in behavior related to the refactoring of function signature parsing (to allow for type parameter lists), and index expressions (to allow for type instantiation). In the cases I've seen, the 1.17 recovery is generally equivalent to or better than the 1.16 error recovery, though of course this is subjective. It's also hard to filter out the noise of these classes of errors without rewriting the 1.16 parser.

Therefore, I think we should close this bug after CL 329791 is merged. There may be more classes of differences, but I don't have an easy way to detect them.

gopherbot pushed a commit that referenced this issue Jun 22, 2021
@findleyr
go/parser: parse an ast.IndexExpr for a[]
9afd158 
To be consistent with Go 1.16, and to preserve as much information in
the AST as possible, parse an ast.IndexExpr with BadExpr Index for the
invalid expression a[].

A go/types test had to be adjusted to account for an additional error
resulting from this change.

We don't have a lot of test coverage for parser error recovery, so
rather than write an ad-hoc test for this issue, add a new go/types test
that checks that the indexed operand is used.

Updates #46403

Change-Id: I21e6ff4179746aaa50e530d4091fded450e69824
Reviewed-on: https://go-review.googlesource.com/c/go/+/329791
Trust: Robert Findley <rfindley@google.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
5 participants
@toothrot @ianlancetaylor @gopherbot @heschi @findleyr