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

Closed
adonovan opened this issue Dec 28, 2022 · 9 comments
Closed

go/parser: parser creates Stmt that causes token.File.Offset(stmt.End) to panic #57490

adonovan opened this issue Dec 28, 2022 · 9 comments
Assignees
@griesemer
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. gopls/parsing Issues related to parsing / poor parser recovery. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.23

Comments

@adonovan
Copy link
Member

adonovan commented Dec 28, 2022 •
edited

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
@adonovan adonovan mentioned this issue Dec 28, 2022
Closed
@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
@adonovan
gopls/internal/lsp/safetoken: fix bug in Offset at EOF
85e6ad7 
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 •
edited

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
@adonovan @gopherbot
gopls/internal/lsp/safetoken: funnel more calls through this package
81e741e 
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>
@adonovan adonovan mentioned this issue Mar 2, 2023
Open
@griesemer
Copy link
Contributor

@adonovan I looked into this:

  • As far as I can tell, where the parser reports end positions needed for error messages, it already uses safePos.
  • There's still the issue of the AST storing positions past the end, caused by syntax errors.

I see several approaches here:

  1. We remove the various assertions (panics) in go/token/position.go in favor of an automatic bracketing of the positions: if we are "before" the file, we set the position (or offset) to the start of the file; if we are "after" the file, we set the position (offset) to the end of the file.
  2. We add a few bytes to the end of a file as a safety measure, and we adjust the tests that check for exact sizes.
  3. We don't do anything because tools have already been adjusted.

Approach 2) seems problematic because it changes a major invariant with unforeseeable consequences. 3) may be unsatisfactory because we don't know if we fixed all places already.

I think 1) may be doable: removing the panics should not break existing code (parser.safePos notwithstanding), and if we have out-of-bounds positions, they will be automatically corrected.

If we decide to do 1) we should do in early in cycle to catch problems ASAP.

@griesemer griesemer added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 6, 2023
@griesemer griesemer modified the milestones: Go1.21, Go1.22 Jun 6, 2023
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jun 6, 2023
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.22.
That time is now, so a friendly reminder to look at it again.

@adonovan
Copy link
Member Author

I agree that (1) seems like the best we can do within the existing constraints.

@adonovan adonovan added the gopls/parsing Issues related to parsing / poor parser recovery. label Jul 25, 2023
@griesemer griesemer modified the milestones: Go1.22, Go1.23 Nov 2, 2023
@griesemer
Copy link
Contributor

Work-arounds exist but we should eventually address this somehow.
Moving to 1.23.

@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.23.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link

Change https://go.dev/cl/559436 mentions this issue: go/token: correct out-of-bounds token offsets and positions

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@griesemer @ezz-no
go/token: correct out-of-bounds token offsets and positions
89b539f 
Per the discussion on the issue, make methods that depend on
incoming offsets or positions tolerant in the presence of
out-of-bounds values by adjusting the values as needed.

Add an internal flag debug that can be set to enable the old
(not fault-tolerant) behavior.

Fixes golang#57490.

Change-Id: I8a7d422b9fd1d6f0980fd4e64da2f0489056d71e
Reviewed-on: https://go-review.googlesource.com/c/go/+/559436
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Bypass: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. gopls/parsing Issues related to parsing / poor parser recovery. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@adonovan @griesemer @gopherbot