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/parser: parser creates Stmt that causes token.File.Offset(stmt.End) to panic #57490

Open
adonovan opened this issue Dec 28, 2022 · 3 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Dec 28, 2022

Given an input of src, the parser reserves a block of the token.Pos space of exactly len(src), even though it may synthesize virtual tokens (e.g. close brackets) at the EOF position during error recovery. The End position of the incomplete syntax node is then computed as Rparen + len(")"), which is out of bounds. See #57484 for a minimal example.

The parser should reserve one extra byte in its call to FileSet.AddFile. Or use safePos in many more places.

@mdempsky @findleyr

@adonovan adonovan changed the title go/parser: parser creates Stmt that cause token.File.Offset(stmt.End) to panic go/parser: parser creates Stmt that causes token.File.Offset(stmt.End) to panic Dec 28, 2022
@gopherbot
Copy link

Change https://go.dev/cl/459735 mentions this issue: gopls/internal/lsp/safetoken: fix bug in Offset at EOF

gopherbot pushed a commit to golang/tools that referenced this issue Dec 28, 2022
During parser error recovery, it may synthesize tokens such as RBRACE
at EOF, causing the End position of the incomplete syntax nodes to
be computed as Rbrace+len("}"), which is out of bounds, and would
cause token.File.Offset to panic, or safetoken.Offset to return an
error.

This change is a workaround in gopls so that such End positions are
considered valid, and are mapped to the end of the file.

Also
- a regression test.
- remove safetoken.InRange, to avoid ambiguity. It was used in
  only one place (and dubiously even there).

Fixes golang/go#57484
Updates golang/go#57490

Change-Id: I75bbe4f3b3c54aedf47a36649e8ee56bca205c8d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459735
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/459736 mentions this issue: gopls/internal/lsp/safetoken: funnel more calls through this package

@griesemer
Copy link
Contributor

griesemer commented Dec 28, 2022

Panicking example: https://go.dev/play/p/JXREnQBsSPe (from #57484).

@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 28, 2022
@griesemer griesemer added this to the Go1.21 milestone Dec 28, 2022
gopherbot pushed a commit to golang/tools that referenced this issue Dec 29, 2022
This change expands the dominion of safetoken to include calls to
token.File{,Set}.Position{,For}, since all need workarounds similar
to that in Offset. As a side benefit, we now have a centralized place
to implement the workaround for other bugs such as golang/go#41029,
the newline at EOF problem).

Unfortunately the former callers of FileSet.Position must stipulate
whether the Pos is a start or an end, as the same value may denote
the position 1 beyond the end of one file, or the start of the
following file in the file set. Hence the two different functions,
{Start,End}Position.

The static check has been expanded, as have the tests.

Of course, this approach is not foolproof: gopls has many dependencies
that call methods of File and FileSet directly.

Updates golang/go#41029
Updates golang/go#57484
Updates golang/go#57490

Change-Id: Ia727e3f55ca2703dd17ff2cac05e786793ca38eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459736
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants