proposal: go/ast: add Start token.Pos fields to BlockStmt and FieldList

[adonovan]

Background: The go/ast package was designed with batch-oriented compiler-like applications in mind, where well-formed ASTs are overwhelmingly the common case. However, it is now the basis of many tools, including gopls, an interactive IDE backend, where ill-formed trees are the norm. We have observed that a major source of crashes in gopls is out-of-bounds accesses of various kinds causes by the parser creating syntax trees whose Pos/End range is not a subrange of its parent node. This usually happens because the End value is either (a) zero, because the final token was missing, or (b) greater than File.FileEnd, because the value was derived by adding an offset from a prior token under the assumption that the file is well formed. For example a file containing only package has an invented name token, _, whose end position is beyond EOF.

We would like to establish the invariant that every node returned by the parser has a Pos/End range that includes the ranges of its children. (For Files, pretend that Pos/End refers to FileStart/FileEnd for this discussion.) This change is mostly straightforward and compatible. However, due to unfortunate constraints imposed by doc comments, some syntax nodes must be created even when their source text is entirely missing. They are BlockStmt and FieldList. Consider an "if" token at EOF. This produces an IfStmt, and every IfStmt must (regrettably) have a BlockStmt, even though there are no braces nor statements. Consequently, the BlockStmt exists but is zero, and its Pos and End methods have nothing to go on. Its End method returns zero, and so does the End method of the parent IfStmt. Similarly, a "func" token at EOF yields a FuncDecl, and every FuncDecl must (regrettably) have a Params, even though there are no parens nor parameter fields. Again, a zero FieldList is created, and its Pos and End are zero.

This problem can be fixed by having the parser record the start position of these missing nodes. Adding a non-exported field to BlockStmt abnd FieldList would permit their Pos and End methods to do the right thing. However, a lot of client code assumes that all fields of ast.Node struct types are public, and misbehaves otherwise. Therefore, these new fields must be public.

Proposal: We propose to add two new fields, both Start token.Pos, to FieldList and BlockStmt. They would be unconditionally set to the start position of the syntax node.

package ast

type BlockStmt struct {
+       Start token.Pos // start position of the block (even when missing, as in IfStmt.Body for "if" at EOF)
        ...
}
type FieldList struct {
+       Start token.Pos // start position of the field list (even when missing, as in FuncDecl.Params for "func" at EOF)
        ...
}

Neither addition increases the size class of its struct.

Sketch implementation:

• https://go.dev/cl/668238 - go/ast and go/parser changes. Patchset 5 shows limitations of using a nonexported field.
• https://go.dev/cl/668677 - gopls changes, mostly to the abstraction-breaking completion logic, and workarounds for non-exported fields.

[adonovan]

@griesemer @findleyr @jba @dominikh

[mateusz834]

So for a valid AST BlockStmt.Start == BlockStmt.Lbrace, right? And same for FieldList.

However, due to unfortunate constraints imposed by doc comments, some syntax nodes must be created even when their source text is entirely missing. They are BlockStmt and FieldList. Consider an "if" token at EOF. This produces an IfStmt, and every IfStmt must (regrettably) have a BlockStmt, even though there are no braces nor statements.

Have you considered a GODEBUG for this? I mean instead of this proposed change, allow a nil BlockStmt, FieldList and guard this with a GODEBUG setting?

[mateusz834]

To eleborate more about the GODEBUG approach, i always assumed that the doc comment around nil/non-nil guarantees were only corresponding to sucesfully parsed AST and that anything can be a nil when it is invalid.

[adonovan]

So for a valid AST BlockStmt.Start == BlockStmt.Lbrace, right? And same for FieldList.

Correct. But we can't make the existing field do double duty without it lying about the presence of a missing token.

Have you considered a GODEBUG for this? I mean instead of this proposed change, allow a nil BlockStmt, FieldList and guard this with a GODEBUG setting?

That would be a breaking change; we don't do breaking changes. GODEBUG is for risky changes that are in principle non-breaking.

[adonovan]

To eleborate more about the GODEBUG approach, i always assumed that the doc comment around nil/non-nil guarantees were only corresponding to successfully parsed AST and that anything can be a nil when it is invalid.

I had always assumed the opposite. I think all we can say with certainty is that there is inadequate documentation about what you can expect from go/ast (and go/types), both when the tree is well formed and when it is not. We plan to address that; see #70725 for example.

[mateusz834]

GODEBUG is for risky changes that are in principle non-breaking.

Not really, we have made such change before: #67620 which is technically a breaking change. But this for sure had a smaller impact.

[gabyhelp]

Related Issues

• go/ast: establish File.FileStart <= Node.Pos <= Node.End <= File.FileEnd for all Nodes in a File #73438
• go/ast: record start and end of file in File.FileStart, File.FileEnd #53202 (closed)
• go/parser: ast.File.File{Start,End} fields are uninitialized when parser bails out early #70162 (closed)

Related Code Changes

• go/ast: enforce FileStart <= Node.Pos <= Node.End <= FileEnd
• cmd/compile: track start/end positions of lexical blocks
• cmd/compile/internal/syntax: track precise [begin, end[ node positions
• cmd/compile: record lexical blocks start/end positions in parser

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