go/ast: establish File.FileStart <= Node.Pos <= Node.End <= File.FileEnd for all Nodes in a File

[adonovan]

Background: Some ast.Node types, such as ast.StructType and ast.InterfaceType, compute their End position based on assumptions about where "}" braces appear in well-formed input. However, in a truncated file--a common input to gopls when one is composing a new file--the brace may be missing and the computed End position may be beyond EOF; or it may be zero. Both have been a widespread source of bugs (e.g. 71659).

Proposal: We propose to establish
(a) the invariant that, in all ast.File trees produced by the parser, every Node has a non-zero Pos and End value, and
(b) the inequality File.FileStart <= Node.Pos <= Node.End <= and File.End.

This will likely require the addition of new fields to record token.Pos values (or their absence) in the AST.

@jba @findleyr @griesemer

[findleyr]

Thank you for filing this issue. It should be a proposal, since it will require adding new fields to the AST.

However, I consider it a bug that End overflows the file. It should be the case that if the parser accepts a struct keyword as a struct type, with no braces, then it must record the absence of those braces.

The alternative would be to use BadExpr for a dangling struct keyword. The only downside I see there is that we could not record valuable information about struct{ Fields... with no closing brace, although given the current poor recovery in the presence of mismatching delimiters, I'm not sure that makes much difference.

My preference would be to:

1. Add LBrace and RBrace to StructType.
2. Document the invariant.

Note that InterfaceType should be fine since FieldList records its Opening and Closing position.

[adonovan]

I agree it will need a proposal, but the first step is to investigate what changes are needed to the parser and go/ast. Once that's done, we can turn this into a proposal.

A complete audit is required; StructType is just one of the known problematic cases. It's possible that StructType can be fixed with only a change to the logic in End. For example:

• struct EOF -> Pos + len("struct")
• struct { EOF -> s.Fields.Opening + 1

[gabyhelp]

Related Issues

• go/ast: record start and end of file in File.FileStart, File.FileEnd #53202 (closed)
• go/parser: StructType.End() can overflow the File #48300
• go/parser: StructType.End() can underflow the File #66683
• go/parser: ast.File.File{Start,End} fields are uninitialized when parser bails out early #70162 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[adonovan]

@jba, I am stealing this from you.

[gopherbot]

Change https://go.dev/cl/668238 mentions this issue: go/ast: enforce FileStart <= Node.Pos <= Node.End <= FileEnd