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.

Sign up for GitHub

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: StructType.End() can underflow the File #66683

Open
adonovan opened this issue Apr 4, 2024 · 6 comments
Open

go/parser: StructType.End() can underflow the File #66683

adonovan opened this issue Apr 4, 2024 · 6 comments
Labels
gopls/parsing Issues related to parsing / poor parser recovery. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Refactoring Issues related to refactoring tools
Milestone
Go1.23

Comments

@adonovan
Copy link
Member

adonovan commented Apr 4, 2024 •
edited

A struct type with a missing close brace has an StructType.End pos of zero. That means the enclosing nodes such as ReturnStmt may also have a computed End that is zero, or perhaps slightly larger:

Screenshot 2024-04-04 at 2 43 08 PM

I believe this is the root cause of at least some of the many gopls crashes recorded in #64547.

Ultimately this is a problem of parser error recovery. We need to establish the invariants that:

  • all tree nodes have valid Pos/End positions; and
  • all tree nodes have nested Pos/End subranges (with a possible exception for the func keyword in FuncDecl/FuncType).

See also:
@adonovan adonovan added gopls/parsing Issues related to parsing / poor parser recovery. Refactoring Issues related to refactoring tools labels Apr 4, 2024
@adonovan adonovan changed the title go/parser: StructType.End() can overflow the File go/parser: StructType.End() can underflow the File Apr 4, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 4, 2024
@gopherbot
Copy link

Change https://go.dev/cl/576655 mentions this issue: gopls/internal/cache: analysis: repair start/end and refine bug report

gopherbot pushed a commit to golang/tools that referenced this issue Apr 5, 2024
@adonovan
gopls/internal/cache: analysis: repair start/end and refine bug report
c7b6b8d 
Poor parser error recovery may cause Node.End to be zero, or
a small positive displacement from zero due to recursive Node.End
computation (#66683).

This change further refines the bug.Reports for such problems,
and additionally repairs the values heuristically to avoid
downstream bug.Reports after toGobDiagnostics (#64547).

Updates golang/go#66683
Updates golang/go#64547

Change-Id: I7c795622ec6b63574978d2953c82036fcc4425af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@findleyr
Copy link
Contributor

findleyr commented Apr 8, 2024

Let's try to fix this for Go 1.23. Adding to the milestone.

@findleyr findleyr added this to the Go1.23 milestone Apr 8, 2024
@gopherbot
Copy link

Change https://go.dev/cl/577595 mentions this issue: [gopls-release-branch.0.15] gopls/internal/cache: analysis: repair start/end and refine bug report

gopherbot pushed a commit to golang/tools that referenced this issue Apr 9, 2024
@adonovan @gopherbot
[gopls-release-branch.0.15] gopls/internal/cache: analysis: repair st…
91b4992 
…art/end and refine bug report

Poor parser error recovery may cause Node.End to be zero, or
a small positive displacement from zero due to recursive Node.End
computation (#66683).

This change further refines the bug.Reports for such problems,
and additionally repairs the values heuristically to avoid
downstream bug.Reports after toGobDiagnostics (#64547).

Updates golang/go#66683
Updates golang/go#64547
Updates golang/go#66730

Change-Id: I7c795622ec6b63574978d2953c82036fcc4425af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
(cherry picked from commit c7b6b8d)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577595
Auto-Submit: Alan Donovan <adonovan@google.com>
@adonovan
Copy link
Member Author

@findleyr suggests that a fuzzer connected to an invariant checker would be a good source of regression test cases.

@findleyr findleyr mentioned this issue Apr 5, 2024
Open
@findleyr
Copy link
Contributor

Shall we expand this issue to introducing and enforcing our stated invariant?

@adonovan
Copy link
Member Author

Shall we expand this issue to introducing and enforcing our stated invariant?

Perhaps we need an umbrella issue. See also #66790 (comment), which is the beginning of a theory of pos/end in ill-formed trees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls/parsing Issues related to parsing / poor parser recovery. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Refactoring Issues related to refactoring tools
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
4 participants
@dmitshur @adonovan @gopherbot @findleyr