Harden the Go parser and printer against real-world code - #8517
Merged
Conversation
go/types panics rather than reporting an error on some parseable inputs — a type cycle routed through an alias is one — and the panic took the whole parse with it. It also leaves half-built types behind: walking a Named whose underlying never resolved trips an assertion inside go/types itself. Type attribution is best-effort, so the type check now contributes whatever it managed to fill in, and a type that cannot be walked degrades to Unknown while its neighbours keep their attribution.
A multi-line result list may end with a comma, as parameter lists may:
func f() (
a int,
b error,
) { ... }
go/ast records no position for it, and the comma was landing in the
whitespace before the closing paren, where recipes cannot see it and
the whitespace validator reports the tree corrupt.
Result lists now carry the same TrailingComma marker parameter lists
use, and the printer re-emits it from there. The scan for the comma
skips comments and string literals, so `(int /* a, b */)` is not
mistaken for one.
GoVisitor reaches only the Spaces its Visit methods were written to reach. A Container's Markers is not among them, so the spacing a TrailingComma marker carries was exempt from validation — and that is where a parser bug parks source text it mistook for punctuation. The validation service now walks the tree by reflection, so a Space is checked wherever it sits. Error messages gained the field path to the offending Space, which is what points at the code path that produced it.
The parser locates punctuation go/ast records no position for — the
`,` between arguments, the `=` of an alias, the `:` of a label — by
scanning the source for that character. Two of the three scanners took
the first byte that matched, so a comma written inside a comment split
an argument list at the wrong place and parked the rest of the comment
in the next element's prefix:
g(1 /* x, y */, 2)
The one scanner that already stepped over rune literals, strings, raw
strings and comments is now the only implementation, and every scan
goes through it.
A method whose parameter re-instantiates its own receiver describes an
infinite family of instantiations:
type R[P any] int
func (R[P]) m(R[R[P]]) {}
Each is a distinct *types.Named, so neither type cache collapses them
and the mapper recursed until the stack was exhausted — a fatal error
no recover can contain, taking the whole process with it.
Attribution now stops at 64 nested types and leaves the type Unknown
past that point.
Go's tokenizer inserts a semicolon at end of line and reports neither
the inserted nor a written one in the AST, so a `;` in the source is
recoverable only from the text. Only same-line separators were being
recovered, and only inside a block, so the far more common
g();
g();
left its semicolons in the following statement's prefix — invisible to
recipes — while `var x = 1;` at file scope and a `;` in a case body
were never recovered at all.
All three statement lists now claim the semicolon, capping the scan at
the line break past which the tokenizer would have inserted one anyway.
A multi-line type parameter list may end with a comma:
type S[
T any,
U any,
] struct{}
The comma was landing in the whitespace before the closing bracket,
hidden from recipes. It now rides the same TrailingComma marker that
parameter and result lists use.
A struct tag is any string literal. Two shapes were dropped outright,
so the tag vanished from the printed source:
Field string "json:\"a\"" // interpreted string, not a raw one
Field func() "x" // no key:"value" pair in it
The first was scanned for pairs without resolving its escapes, so no
pair was found; a tag yielding no pairs was then discarded, having
already been consumed. Escapes are now resolved before scanning, a tag
with no pairs becomes one annotation carrying its text, and a marker
records the delimiter so the printer re-emits the literal as written.
Soundness is invisible to the parser's own checks: a file whose source text sits in a Space round-trips byte-for-byte and passes a rewriteRun test. `make sweep` sorts a corpus of checked-out repositories into sound, unsound, and parse error, and groups the failures by normalized cause so a large cluster is not shattered across singletons. `make reduce` shrinks one failing file to a minimal reproducer, holding the failure's classification fixed so it converges on the construct at fault rather than on some other bug. Both stay out of CI behind the `parityaudit` tag, like the printer corpus. The sweep journals results as they land so a run resumes after a stack overflow, which no recover can contain.
The same `;` that statement lists already recover is legal after an
interface method, a struct field, and a spec of a parenthesized
`var`/`const`/`type` group:
type T interface {
M() string;
}
It was landing in the following declaration's prefix, or in the block's
trailing space when it came last. All three lists now claim it.
The space before a group's `)` normally rides the last spec's After;
where that spec claimed a semicolon it rides an Empty instead, the slot
an otherwise empty group's comments already use.
A generic instantiation spread over several lines may end its type
arguments with a comma:
collections.NewMap[
stackaddrs.AbsComponentInstance,
func(ctx context.Context) (Result, error),
]()
The comma was landing in the whitespace before the closing bracket. It
now rides the TrailingComma marker, and both the call and type-position
forms print through one routine.
A tag's own leading padding sits inside the delimiter, while the first annotation's Prefix is the space outside it, so `"\tx:\"y\""` lost its tab. The padding now rides the key, and the printer spells the tag back out with strconv.Quote — the inverse of the unquoting done on the way in, so a control character in a key survives the round trip.
Go allows the init statement to be empty while its `;` is written, and
reports no node for either half:
switch ; {
The semicolon was left to the header's next node to swallow into its
prefix. One routine now maps the clause for `if`, `switch` and type
switch alike, standing a java.Empty in for the absent statement, and
scans only as far as the header's end.
An import declaration takes the same optional `;` every other
declaration does:
import "go/ast";
Imports are mapped on their own path, which left the semicolon to the
following declaration's prefix, or to the space before a group's `)`.
They now claim it like the rest, and the routine that does so is
generic over the element type so both paths share it.
A tag may carry padding after its last pair, and hand-written ones
sometimes carry text that reads as no pair at all:
Challenges ACMEChallenges `json:"challenges,omitempty" `
Both were dropped, so the printed field differed from the source. The
pair scan now reports what it left unread: whitespace parks after the
last value, and anything else keeps the tag whole rather than
decomposed, since only whitespace has a slot to sit in.
A UTF-8 BOM, which editors on Windows write and Go's scanner ignores, was landing in the compilation unit's prefix, where the whitespace validator reports it as source text stowed in a Space. It is now carried the way GoMod and GoSum already carry it, on a CharsetBomMarked field that the printer re-emits and that the RPC layer sends in the slot it had been filling with a constant false.
A `//go:` or `//lint:` directive is modelled as an annotation whose
arguments are the text after the directive name. That text was found by
trimming the separator away and printed back with one space, so
//go:generate mockery --name Store
came out a space narrower, and a tab came out as a space.
The separator is now kept as the argument container's leading space.
An empty list has no element whose padding could hold what sits before
the closing delimiter, so `g( )` and `[]int{ }` came back without their
space. Both already stood an Empty in that position to hold a comment;
they now do so for any spacing.
Found by widening the gap between adjacent tokens of corpus files:
gofmt writes no space there, so real-world Go never exercises it.
`(h())` is a legal statement, but Parentheses is an expression only, so mapping the expression statement produced nothing — and by then the source text had been consumed, leaving the statement missing from the printed output rather than merely unmodelled. Go.ExpressionStatement is the counterpart to the StatementExpression this module already has for the other direction, and mirrors its shape on both sides of the RPC boundary. Expressions that are already statements, a method invocation among them, are unaffected.
`e.(error)` may be written with spacing, or a comment, between the expression and the dot. No node's prefix covers that position, and the parser was discarding it — deleting the comment outright. A marker carries it, as the trailing comma and struct tag quote already do for punctuation the AST does not record.
The separator between an init clause and the condition may be the one
Go's tokenizer inserts at a line break:
if g()
(x) {
The printer wrote a `;` whenever an init clause was present, adding one
the source did not have. A Semicolon marker records a written separator,
the same way statement lists already record theirs.
Recursion through method parameters — `func (R[P]) m(R[R[P]])`, and the same between two generics — names a fresh *types.Named at every nesting. Pointer identity never repeats, so neither type cache hits, and mapping one file ran until the process was killed. The depth bound alone did not help: the work branches, so bounding depth still leaves the tree exponential in it. An instantiation reached while its own generic is still being mapped now resolves to that in-progress class.
An empty statement is a `;` standing alone, which J.Empty renders as
nothing, so `func f() { ; }` and the `L: ;` that stops a label binding to
the following loop both lost the only text they had.
The explicit kind now carries a Semicolon marker for the printer. The
implicit kind stands for the semicolon the tokenizer inserts at a line
break and still renders as nothing.
The parser records an explicit `;` after a statement in a select clause the same way it does in a case clause, but the printer for the select form was not asking for it.
Scanning to the end of the line for an element's terminator reaches past
what the element can own. The last statement of a case or select clause
took the `;` that terminates the switch itself, and a switch with no init
clause took one written inside its tag expression, in both cases
swallowing the text in between:
func f() { switch { case true: g() }; g() }
Whitespace and comments are all that may separate an element from its
own `;`, so the scan now stops at the first token and takes it only if
that is what it is. The caller's boundary still applies where one is
known, keeping a `;` that stands as its own empty statement out of the
preceding element's reach.
TypeAssertionDot and StructTagQuote are registered by their Java names so a Go LST can cross the RPC boundary, but no such classes existed: RpcReceiveQueue resolves a marker by Class.forName, so any tree carrying one — every struct tag written as an interpreted string carries a StructTagQuote — failed on arrival. Both classes mirror the field order their Go counterparts send.
A closure maps to a MethodDeclaration whose MethodType was left empty: it has no name to look up in Defs, and nothing read the signature off the expression instead. Method declarations went from 56% attributed to 96% on a sample package, the remainder being the named declarations whose own lookup misses.
A callee is not always a declared function: a parameter, a local, or a struct field holding a func carries the signature on its own type. Only *types.Func was read, so `fn(1)`, `s.cb(2)` and `h(3)` came back with no method type for a recipe to match on. A builtin still has none, having no signature to give.
A corpus root holds many repositories, and one repository's sources cannot satisfy another's imports, so the importer resolves against the directory owning the go.mod above the package being parsed. Reading a repository's sources is the expensive part and every package in it resolves against the same set, so the importer is built once per module. It caches the packages it resolves and is not safe to share as it stands, so callers reach it through a lock.
A JavaType.Variable carried no owner, so nothing could say which type a field belongs to or which function a local lives in — the attribution a recipe needs to match `pkg.Type#field`. rewrite-java resolves it from the symbol's owner and asserts it is never null. A field's owner is recorded while its struct is mapped, since go/types leads from a struct to its fields but not back. A local or parameter takes the enclosing function, which the parse context now carries. Interface methods likewise name the interface as their declaring type.
`type optionFunc func(...)` is what a call on a value of that type resolves against, the way an interface is for a method, so it stands as the method's declaring type. A call through an unnamed `func(int)` still declares nothing, there being no type to name. The census stops reporting a missing declaring type for an unnamed signature, and a missing name or qualifier for the synthetic types Go's builtins, maps and channels take.
A file guarded by `//go:build` reaches the parser only under a context that takes it, and the sweep saw only the host's, leaving the platform-specific halves of a corpus ungraded — 3,452 files of 35,427 on the standard library. Seven GOOS/GOARCH combinations between them select nearly all of them, and a file is graded under the first that does.
Three faults in one slot. A nil *JavaTypeMethod assigned to the interface field made an absent owner read as present and panic on use, which also hid it from the attribution census. A package-scoped variable took whichever function happened to read it, so `os.Args` belonged to the caller. And a receiver was mapped before its method's scope opened, giving the same variable one owner at its declaration and another at every use. The scope now opens first, an absent owner is a nil interface, and a package-scoped variable belongs to its package.
A boundary computed as `int(pos) - file.Base()` goes negative when the position is absent, which a scan bounded by it read as "no bound" and ran to end of file, taking the first comma anywhere later as a trailing one. An absent position now yields a bound nothing matches. A named type reached from a call through a func value went to mapNamed directly, outside the depth bound and the recover that contain a type the checker left unwalkable; it goes through mapType with the rest. Also documents that findNextBefore cannot find a quote, backtick or slash, those being what opens the regions it steps over, and keeps the struct tag fallback from writing its delimiters twice.
A file the sweep skips has been excluded by every build context it tried, and a bug in constraint evaluation is indistinguishable from a file that was meant to be excluded: it leaves the denominator instead of failing. Differencing against go/build's own MatchFile names the ones it would have taken — 93 of 3,957 skipped files across two corpora.
…on-real-world-code # Conflicts: # rewrite-go/pkg/rpc/go_receiver.go
`x.(T)` was a J.TypeCast, which models Java's prefix cast `(T) x` and so has nowhere to hold what stands between the expression and the dot — a space, or a comment. A marker carried it, which is the wrong home for spacing when the shape of the node is what is wrong. Go.TypeAssertion follows JS.As: the left expression is right-padded, so its padding is the space before the operator, the way J.FieldAccess pads the name before its dot. It also carries the asserted-to type, which a type assertion had nowhere to record. J.TypeCast keeps its visitor, printer and matcher handling for trees that carry one from elsewhere.
A raw string's carriage returns are discarded from BasicLit.Value, and Go 1.25 computes BasicLit.End() as ValuePos plus that value's length, so the reported extent falls a byte short of the source for every raw string containing one. Reading the tag over that range left the closing backtick unconsumed and printed it twice. The literal is measured by scanning to its closing delimiter, which is the same on every release.
This was referenced Aug 17, 2026
knutwannheden
added a commit
to moderneinc/recipes-go
that referenced
this pull request
Aug 17, 2026
openrewrite/rewrite#8517 models `x.(T)` as `Go.TypeAssertion` instead of `J.TypeCast`, so the four recipes that reached for a type assertion stopped matching and made no change. The same PR emits an `if` init clause's `;` from a `Semicolon` marker, which a recipe that synthesizes such an `if` has to attach itself; without it `CheckTemplateExecuteError` printed `if err := f() err != nil`. This requires `rewrite-go/v0.0.30`, which is not yet published.
knutwannheden
added a commit
to moderneinc/recipes-go
that referenced
this pull request
Aug 18, 2026
* Adapt the type-assertion recipes to `Go.TypeAssertion` openrewrite/rewrite#8517 models `x.(T)` as `Go.TypeAssertion` instead of `J.TypeCast`, so the four recipes that reached for a type assertion stopped matching and made no change. The same PR emits an `if` init clause's `;` from a `Semicolon` marker, which a recipe that synthesizes such an `if` has to attach itself; without it `CheckTemplateExecuteError` printed `if err := f() err != nil`. This requires `rewrite-go/v0.0.30`, which is not yet published. * Resolve rewrite-go v0.0.30 and re-attribute what RelocateRawMessage rewrites `go.sum` gains the entry for the published `rewrite-go/v0.0.30`. That release also carries openrewrite/rewrite#8521, which makes `RemoveImport` keep an import the file still references. `RelocateRawMessage` rewrites the only `json.RawMessage` in a file to `jsontext.Value` and then drops the import, but the field, the type expression and the conversion call all still carried the `encoding/json` attribution they were given at parse. The reference check read that as a live use and kept the import, so the file came out importing both packages. The rewritten nodes now carry `encoding/json/jsontext.Value`, which is what the source says once the rewrite has run, and the conversion drops a method type that no longer describes the call it is attached to. The import goes because nothing claims to use it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
A Go file can round-trip byte-for-byte and still be broken for recipes: if the parser leaves source text sitting inside a
Space, the printer emits it again and everyrewriteRuntest passes, while no recipe can see or rewrite that code. Print-idempotency alone measures the smaller half of the problem. This change measures both halves against 151,382 files from 59 public repositories —golang/go,kubernetes,moby,containerd,cockroach,grafana,tidb,terraform,istioand others — and fixes what that surfaced.Three metrics, one class per file: parse error (rejected, panicked, crashed, or printed something other than its input), unsound (round-trips, but hides source text in a
Space), sound.Examples
The tooling is behind the existing
parityauditbuild tag, so it stays out of CI:make fuzzearns its place: real Go is gofmt'd, so the corpus contains exactly one canonical amount of space between any two tokens. Widening that gap found five bugs 151k files could not, including a silently deleted comment.Summary
Parser and printer, each with a minimal reproducer as an ordinary
go testcase:g(1 /* x, y */, 2)split the argument list inside the comment. One fix removed the family.var/const/typespec, import, and select-clause statement; as an empty statement; and as an emptyif/switchinit clause. A semicolon is claimed only when it is the next token, so it cannot reach past what owns it.TrailingCommamarker.tag.Value.Go.ExpressionStatement, the counterpart to theStatementExpressionthis module already has.x.(T)was aJ.TypeCast, which models Java's prefix cast(T) xand has nowhere to hold what stands between the expression and the dot.Go.TypeAssertionfollowsJS.As: the left expression is right-padded, so its padding holds that space or comment, the wayJ.FieldAccesspads the name before its dot. It also carries the asserted-to type, which a type assertion had nowhere to record.GoModandGoSumalready do.go/typespanics rather than reports an error on some parseable inputs and leaves half-built types behind. Attribution degrades; the file still parses.func (R[P]) m(R[R[P]])names a fresh*types.Namedper nesting, so no cache collapses it and mapping one file ran until the process was killed. An instantiation reached while its own generic is being mapped resolves to that one.Type attribution, measured per package with the same census:
J.Assignment.TypeandJ.ArrayType.Typewere never populated — every one of 196,775 slots nil. FillingArrayType.Typealso surfaced a latent RPC bug: the sender put the type on the wire as a scalar while the receiver read a whole type graph.MethodDeclaration.MethodTypewent from 56% to 96% attributed on a sample package.JavaType.Variablehad a nil owner. A field takes its declaring type, a package-scoped variable its package, a local or parameter its enclosing function — matching rewrite-java, which resolves owner from the symbol and asserts it non-null.WhitespaceValidationServicewalks by reflection rather than throughGoVisitor, which never visits aContainer's markers — so aTrailingCommastowing source text was exempt from the check meant to catch it.Test plan
./gradlew :rewrite-go:testgreengo test ./...green (83 new tests, each a minimal reproducer asserting byte-equal round trip and a clean tree)go/parseritself rejects — zero round-trip failures, zero panics, zero crashesmake fuzzclean on all three corpora: 56,561 mutations, no failuresGo.ExpressionStatement,Go.TypeAssertionandStructTagQuote, whose absence would have failedClass.forNameon arrivalKnown remaining, all non-compiling Go in
golang/go's compiler fixtures: 4 files with a value/name count mismatch (var c, d = 1, 2, 3), 3 with an empty receiver list. A struct tag key written with a\uXXXXescape normalizes on print; zero occurrences in the corpus.