ast: tag node types and omit absent fields in JSON output - #4591
Closed
kyleconroy wants to merge 2 commits into
Closed
ast: tag node types and omit absent fields in JSON output#4591kyleconroy wants to merge 2 commits into
kyleconroy wants to merge 2 commits into
Conversation
Node is an interface, so the JSON that parse and analyze --ast print carried
no record of which node a given object was. Nodes with no fields of their own
all encoded as "{}": a star in a RETURNING clause was indistinguishable from
an untranslated clause, and an empty List was indistinguishable from both.
Encode RawStmt and everything beneath it through a marshaller that emits each
node's Go type name under a "tag" key. RawStmt is the root of the AST both
commands print, so implementing MarshalJSON there covers the whole tree.
"tag" is the one candidate that does not collide with an existing field name.
encoding/json matches field names case-insensitively, so "kind" would capture
A_Expr.Kind, "type" would capture the eight nodes with a Type field, and
"node" would silently capture SortBy.Node, which holds an interface and so
accepts the tag string without error. A test guards the invariant.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T9LCt1mwdY3mE14x3Zz5Vw
Most of what parse printed was absent fields. On the four author queries,
25% of the keys were null and another 9% were empty containers, so reading an
AST meant scanning past clauses the statement never had.
Leave a field out when it holds a nil pointer, interface, slice or map. Zeros
stay: StmtLocation is 0 for the first statement in a file and LIMIT 0 parses
to an Ival of 0, so dropping zero-valued scalars would lose what the parser
found rather than what it did not.
This depends on the type tags. Omitting a nil Items turns an empty List into
"{}", which without a tag would be indistinguishable from A_Star and TODO --
it would have roughly tripled the number of unidentifiable nodes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T9LCt1mwdY3mE14x3Zz5Vw
Collaborator
Author
|
Closing in favor of a redesign. The reflective encoder here worked, but the replacement gets the same output from stock Generated by Claude Code |
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
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.
Two changes to the JSON that
sqlc parseandsqlc analyze --astprint. Both are in the AST encoding only;analyze's owncolumnsandparamsoutput is untouched.Tag every node with its type
ast.Nodeis an interface, so the JSON encoding carried no record of which node a given object was. Nodes with no fields of their own —A_Star,Null,TODO— all encoded as{}, and an emptyListwas indistinguishable from them. OnINSERT ... RETURNING *, the star and four untranslated clauses all printed as the same empty object.RawStmtnow marshals itself and everything beneath it through a reflective encoder that emits each node's Go type name first:tagis the one candidate that does not collide with an existing field name.encoding/jsonmatches field names case-insensitively, sokindwould captureA_Expr.Kind(10 node types have one),typewould capture the eight nodes with aTypefield, andnodewould silently captureSortBy.Node— which holds an interface, so it accepts the tag string without an error and replaces the sort expression.TestNoFieldShadowsTagKeyparses the package and fails if a node ever declares a field that collides.Omit absent fields
Most of what
parseprinted was fields the statement did not have: on a four-query file, 25% of keys werenulland another 9% were empty containers.A field holding a nil pointer, interface, slice or map is now left out. Zero-valued scalars stay, because zero is a value the parser can find:
StmtLocationis 0 for the first statement in every file, andLIMIT 0parses to anIvalof 0, so omitting zeros would drop what the parser found rather than what it did not.This depends on the tags landing first. Omitting a nil
Itemsturns an emptyListinto{}, which without a tag would be indistinguishable fromA_StarandTODO— it would have roughly tripled the number of unidentifiable nodes.Notes
The output is longer than before, not shorter: on the four author queries, 113 lines → 158 with tags → 137 after omission. Tags cost a line per node; what they buy is that every object says what it is.
docs/howto/parse.mdalready carries a beta notice about the JSON shape changing. It gains sections for both, including that aTODOtag means a clause was parsed but is not represented in the AST, rather than that it was absent from the query.Testing
go test --tags=examples -timeout 20m ./...passes with PostgreSQL and MySQL running, as dogo vetandgofmt.Eight goldens are regenerated: the seven
parse_basiccases andanalyze_ast/postgresql. I regenerated all 34parseandanalyzecases; the other 26 came back byte-identical, confirming nothing outside AST output moved. I also checked every regenerated golden programmatically for objects lacking atag— there are none.Not covered: no end-to-end case exercises an enum-valued field, since
parse_basicisSELECT 1;. That predates this change and is worth adding separately.Generated by Claude Code