Repository navigation
refactor(compilers/openapi): unify YAML reading and node naming - #1106
Merged
Merged
Conversation
Every reader of the source's raw YAML now asks nodeview, so none can disagree with the parser about which pairs a mapping holds, under which key, or in what order. The sites that read raw pairs by hand, each with a merge-key test or a key reading of its own, saw no key a `<<` merge supplied, and some kept `<<` itself as a member. nodeview presents a mapping's pairs in the decoder's order: the merged pairs first, an earlier source winning a key two share, then the mapping's own, which override them (docs/parser-library.md#merge-keys). KeyOf keys a pair as the parser does, reading a key that is no scalar as the empty string, and Repeat finds a key written twice wherever the view reads it. Child, Pairs and WrittenChildren replace annotation's RawChildNode family, and annotation and value may now import nodeview. nodeview.Walk is the one bounded, iterative walk of a raw tree. The load, overlay and source-index walks keep no stack or bound of their own, and the walk reports to each whether a bound cut it short. value.Classify is the one reading of a scalar's kind, which the value reader and the verbatim JSON renderer both call, so they agree on every tag and spelling, and a plain number past float64's range is a number in both. oas.CheckFormat holds every mapping to the Format section's rule that a key is a string, reporting a key that is no scalar and a `<<` merge key naming no mapping as errors; the parser reads the first as the empty string and merges nothing from the second, and says nothing of either. Its struct literal is the first in the module to key a promoted field, which crashed internal/lintrules' name-channel check: it judged a key against the literal's own fields by position. The check now resolves a key through the embedding path, so a promoted ir.Naming channel set in a literal is held to its rule too. What the IR holds changes where the old readings were wrong: - a field, extension or unknown key merged in through `<<` is read, kept and reported as a written one is, where it was lost; - a key YAML types as a number, a boolean or null is kept as the text it spells, where the mapping holding it could not be serialized; - a component entry keyed by a mapping or a list is kept under the empty name the parser reads it by, and reported; - a value whose mapping writes a key twice is refused, as a verbatim payload's already was, and both now find a key repeated through an alias or inside a mapping merged in; - a value's members come in the parser's order, the merged ones first; - `!!null abc` and a `!!timestamp` that is no timestamp are refused as any scalar its tag does not admit is.
A node's name is now a function of the documents and the node's coordinate alone, so which lowering reaches a position first, and from where, can no longer change what it is called. Before, every lowering took the hint as a parameter, the first declaration to reach a position recorded it, and a pointer replay guessed it where none had. The replay could not see an owner the pointer does not spell, so a union variant naming a response body or a parameter's schema took a name its node did not have. bind.Table.Kinds says how the grammar reads each position on the way to a pointer, reading a position the grammar holds as raw YAML as the reference that lands on it does. The naming package names a schema from that: its owner, a component schema's key, a request body's component key or the operationId of the operation declaring it, a response, a parameter's name or a header's key, then the structure beneath it. A composition branch holding a $ref takes the name of the position it lands on, within a bound of 64 hops. lowering.NameAt and NameThrough are the only ways a lowering asks for a name. The hint parameters, Ctx.DeclareHint and HintAt, OperationIDAt and the schema package's position replay are removed. The $dynamicAnchor index moves into the context: Ctx.DynamicAnchorSites builds it on the first ask, through the new anchors package's bounded iterative walk, and shares it with every copy, so no lowering carries it as a parameter either. No name changes across the real-world corpus swept against main; the variants the replay misnamed are pinned by tests.
The hoist step said a hint is computed from context in a single pass, which described the hint threading the naming package replaced. State what holds now, a function of the documents and the pointer alone, and show the package in the layout.
This was referenced Oct 23, 2026
Member
Author
|
This overlaps #1104, which moves
At |
Every reader keeping a field verbatim now reads it through nodeview, so one test at that choke point covers the reading, but nothing showed that each reader asks it of the right node. The yaml-merge-keys case now merges a field into each kind of reader: path-item and operation servers, a response's links, a header's style, a parameter's allowEmptyValue, an encoding's allowReserved, a security scheme's stray field and a schema's validation-only keyword. The 3.2 media type readers are witnessed beside their own tests: a merged description, and a merged prefixEncoding with its itemEncoding, which main reported as declared and unreadable. The library warns of bearerFormat on an apiKey scheme, merged or not, as it already does in security-schemes.yaml, so the new line in the validation disagreements carries the same over-strict reason.
nodeview.Walk walks a tree as written, through Content. Its comment called it the only walk of a tree, but a walk addressing positions as the parser reads them, where a merged pair's value is no child of the mapping merging it, keeps a frame per position: the anchor index does, and its comment now says why it cannot use Walk. micro-compiler-design §6 argued that a table replaying hints from the grammar would be a second reading of the declaration walk. Names now come from the table, and a dated note records why that is the only reading rather than a second one. Three test comments still described the removed position replay.
keepComponentMediaTypes built a view for two lookups and read neither answer's merge bound, which is the shape keepJSONSchemaDialect had before it moved to nodeview.Child. Child answers both lookups the same way, alias followed and a repeated key at its last copy, without a view of its own. retainUnreferencedComponents keeps its view, since it reads every entry of each section through it.
Reading a mapping as the parser does cost more than the readings it replaced, in three places. nodeview.Child built a view for each read of a mapping that merges, so every schema keyword a lowering asked re-expanded the same merge chain. On a document merging one wide mapping into each of its schemas, at the alias budget's ceiling, a compile took 1.9 times main's. Child now looks the one key up through the merges, the mapping's own last copy first and then each source's first copy in merge order, as flatten settles them, with the same depth bound and cycle check, and builds nothing. A differential test holds it to MappingPairs on each shape a lookup could part from it, and the existing fuzz does on random ones. Every mapping's expansion allocated sets for repeats and merges, though almost none writes a key twice or merges. One that does neither keeps its own pairs as they are, keys are compared without a set below 16, and the repeat and the merge-source copy sit out of line, so a memo entry is the size it was on main. oas.CheckFormat placed every node to report findings almost no document has. A first walk now only looks for a break, and the placing walk runs when it finds one. On that merge-heavy document a compile is now within 6% of main, and the Cloudflare schema within 2%, while both read what main did not. A corpus sweep gives the same output as before.
nodeview.Child now looks a key up through the merges by the library's order and copies, so a library release that changes either moves it as well as flatten. The merge-keys and duplicate-key entries name it, and its doc comment cites both, so a reader of either finds the other.
A name is now a function of a node's coordinate, so each node composes its hint along its whole path rather than from its parent's, as the threaded hint did. compile.SubHint neutralizes its growing parent on every call, so a fold of it costs n² bytes for n nested roles, and n nodes on one chain cost n³: a schema 800 items deep compiled in 2.6 times main's time. compile.SubHints composes a chain in one pass: since a composed hint is neutral already, SubHint(SubHint(p, a), b) is each part neutralized once and joined. A table and a fuzz target hold it to the fold. The namer collects a chain's parts and joins them once, so the same schema compiles in main's time. A corpus sweep gives the same names.
The namer took a schema's owner from the first schema on its way, and named a document's root read as a schema after nothing. Two kinds of name came out wrong: - A union variant or composition branch holding `$ref: './Cat.yaml'` took the positional `variant_N`, where main named it `cat_yaml`, and the node it holds kept main's `empty`. - A parameter, request body or response kept under a schema's x- key and reached through a `$ref` named its schema after the enclosing component (`schema`, `schema_item`), where main named it for the object (`foo`, `request`, `response_item`). A document read whole as a schema did the same to every object a fragment reference read in it. The owner is now the nearest object holding the schema: the innermost schema on the way, walked back through the schemas, maps and lists of its structure, so an object a reference reads inside raw YAML starts a structure of its own. A document's root read as a schema is named for the document, as a `$ref` spelling it suggests, so the variant and the node it holds both read `cat_yaml`. Over the real-world corpus, testdata and the Cloudflare schema this changes only whole-document roots and what composes off them: `empty` becomes the file's name in 11 documents, and nothing else moves.
load.spanned and overlay.countNodes bound their walk one node past the limit they test, as limit + 1. At a limit of math.MaxInt, which a caller can pass as a node budget, that wraps negative, the walk visits nothing, and the count reads 0. No refusal turned on it, since nothing lies past math.MaxInt, but the count was wrong. Both now stop at math.MaxInt, as sourceindex's walk already did.
An operationId or a parameter name written as a number names its owner by the text it spells, as the parser reads any scalar but null. The namer's tests read an aliased, merged, null and non-scalar owner name, and no test caught a reader that took only a string.
The package doc still described a view the schema lowering needed beside the scans. It is now the reading every reader of the parser's view of raw YAML takes, with the bounded walk of a tree beside it, and it names the one reader that mirrors another library reader on purpose.
The rewritten package doc ran to 108 words, past the 100-word cap internal/archtest holds every doc comment to.
Name the schema a document root is, rather than an ambiguous "it", in the namer's owner comment, and say plainly who takes nodeview's reading.
A key that is no scalar draws its Format error once, and every rule that judges a key's spelling then judges the empty string the key reads as. Say so where CheckFormat is documented, so the extra findings read as a known gap rather than as this check's own.
OmarAlJarrah
added a commit
that referenced
this pull request
Oct 23, 2026
#1106 changed two functions of meta.go and one fixture of meta_test.go, which this branch moved into internal/document. Each lands in the file that holds its moved copy: keepJSONSchemaDialect in dialect.go and keepComponentMediaTypes in components.go, both now reading through nodeview.Child, and the x-bad: {n: .nan} fixture in sites_test.go. openapitest keeps both sides' builders, RootMergeChain and NestedMerges, and the auth rule in internal/archtest keeps this branch's reason with the sentence #1106 appended about reading through nodeview.
OmarAlJarrah
added a commit
that referenced
this pull request
Oct 23, 2026
settledByOwnPairs walked the root mapping's raw pairs by hand, asking nodeview.KeyOf for each key, to learn which keys the root writes itself. nodeview.WrittenChildren, which #1106 added, answers exactly that: the keys a mapping writes, no merge read, each named as KeyOf names it. The probe now asks it, so the compiler has one reading of a mapping's own keys; #1106 left this site alone only because detection was moving. Nothing held the function to its own keys. Reading the root's keys through its merges instead passed every test, because the row named "a merged version a deeper one may outrank" defined that version but never merged it, and so repeated "a chain past it". It now merges the version after the chain, where the read finds it and the bound may hide one that outranks it, and the merged reading turns it red.
OmarAlJarrah
added a commit
that referenced
this pull request
Oct 23, 2026
The layout listed the compiler's internal packages as "its own packages", but left out anchors, which #1106 added, and fragment, navigation, overlay, sourceindex and spelling before it. nodeview's line still read "as the resolver reads it", where its own package comment now says the parser's reading and one bounded walk. The list now names every package, the test-only ones on a line of their own, in an order where each comes after every package it imports, which value importing nodeview and load importing resolve had broken.
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.
Summary
The OpenAPI compiler read the source's raw YAML in many places, each with its own merge-key test, key reading, scalar classification and tree walk, and it named the nodes it built by threading a hint through every lowering and replaying a pointer where no lowering had recorded one. Both disagreed with the parser or with themselves in ways that lost data, kept members the document does not write, and named a node after whichever lowering reached it. This PR gives each question one answer. The first two commits carry the change and each builds and passes the suite on its own; the later ones add tests and docs, move one more reader onto
nodeview.Child, make the new reading cost what the old one did, name a schema for its nearest owner, and keep two walk bounds from overflowing.Read raw YAML as the parser reads it.
nodeviewis the one reader of a mapping.View.MappingPairspresents the pairs in the decoder's order: merged pairs first, an earlier source winning a key two share, then the mapping's own, which override them (docs/parser-library.md#merge-keys).KeyOfkeys a pair as the parser does, a key that is no scalar as the empty string (#non-scalar-key).View.Repeatfinds a key written twice wherever the view reads, through an alias or inside a merged mapping.Child,PairsandWrittenChildrenreplace annotation'sRawChildNodefamily, and the readers inannotation,value,auth,operation,schema,loadandoverlayall go through them.Childlooks one key up through the merges by the same precedence and builds nothing, so asking it per schema keyword costs what the old raw scan did; a differential test and the fuzz target hold it toMappingPairs.nodeview.Walkis the one bounded, iterative walk of a tree as written. The load, overlay and source-index walks keep no stack or bound of their own, and the walk reports to each whether a bound cut it short. Deep and wide trees are tested once, at the walker. A walk that addresses positions as the parser reads them keeps a frame per position, since a merged pair's value is no child of the mapping merging it: the schema binder and the overlay attribution already did, and the new anchor index does too.value.Classifyis the one reading of a scalar's kind. The value reader and the verbatim JSON renderer both call it, and a table over tag, style and spelling holds them to the same kind.oas.CheckFormatholds every mapping, values and extensions included, to the Format section's rule that a key is a string. It reports a key that is no scalar and a<<naming no mapping asvalidation-type-mismatcherrors. The parser reads the first as""and merges nothing from the second, and says nothing of either.internal/lintrules' name-channel check judged a struct literal's keys by position against the literal's own fields.oas.CheckFormat's literal is the first in the module to key a promoted field, which crashedmake vet, and a promotedir.Namingchannel set that way went unchecked. The check now resolves a key through the embedding path, with plants for both cases.Name every node by its coordinate.
naming.Namer), so which lowering reaches a position first, and from where, cannot change what it is called.bind.Table.Kindssays how the grammar reads each position on the way to a pointer, a reference's reading included where the grammar holds raw YAML. The namer names the owner from that, the nearest object holding the schema (a component schema's key, a request body's component key or theoperationIddeclaring it, a response, a parameter's name, a header's key, or, for a document a$refreads whole as a schema, the document's file name), and composes the structure beneath it. An object a reference reads inside raw YAML, such as a parameter kept under a schema'sx-key, starts a structure of its own. A composition branch holding a$reftakes the name of the position it lands on, within 64 hops.lowering.NameAtandNameThroughare the only ways a lowering asks for a name. The hint parameters,Ctx.DeclareHint/HintAt,lowering.OperationIDAt/resolve.OperationIDAtand the schema package's position replay are removed.$dynamicAnchorindex moves into the context.Ctx.DynamicAnchorSitesbuilds it on first ask, through the newanchorspackage's bounded iterative walk, and shares it with every copy, so no lowering takes it as a parameter.What the IR holds now
Everything below changes only where the old reading disagreed with the parser or with itself.
<<is read, kept and reported as a written one is. Before, a path item's mergedsummary, a flow's merged stray field and a merged unknown key reached the IR in no form.{"100": "Continue"}). Before, the mapping holding it could not be serialized and was dropped.<<is no longer kept as a member. A value whose mapping writes a key twice is refused, as a verbatim payload's already was. Both now find a key repeated through an alias or inside a merged mapping.!!null abcand a!!timestampthat is no timestamp are refused, as any scalar its tag does not admit is.$reflands: a position its response or a parameter names (response_item,filter_by), where it took the pointer's (schema_item,schema), and a schema a$anchoror a$idnames (cat_deffor$ref: '#kitty'), where it took the reference's text (kitty).$refreaches, is named for its owner (the parameter'sname, the header's or request body's component key), where it took its last token (schema).$refreads whole as a schema is named for its file, and so is what composes off its root (./Pet.yamlnamespet_yamland its itemspet_yaml_item), wheremainnamed thememptyandempty_item. A union variant holding it keeps thepet_yamlit had onmain.Left as they are: a key merged in through a
<<chain deeper thannodeview.MergeDepthLimitlevels is still read by the parser and by no raw reader here, which reports it where it can (openapi/unknown-key-unreachable, the binder's merge-depth warning); a scalar its tag does not admit is still refused rather than kept as text, which #245 decides;internal/harness, which cannot importnodeview, keeps its own key reading, which #1105 tracks, and its own recursive permutation walk; and a key that is no scalar draws, beside its Format error, the findings of each rule that judges the empty name it is read as (a component name, a path key, a status key), which #1108 tracks.No exported identifier outside
internal/changes or goes away. One is added:compile.SubHints(parent, suffixes...),SubHintalong a chain in one pass, which the namer composes nested hints with.Closes #1089
Closes #788
Closes #395
Closes #384
Closes #1072
Closes #784
Closes #980
Closes #981
Closes #783
Closes #617
Closes #947
Closes #729
Closes #982
Closes #799
Test plan
go test ./...,make coverage(100%, all 15,138 statements),golangci-lint runat the pinned version (0 issues),make vetincludingscripts/vet-rules.sh,make lint-planted,make fuzzandmake bench-smokepass. The first commit passes the same suite, coverage gate (15,019 statements), lint and vet on its own.go test ./...,golangci-lint runat the pinned version (0 issues) andmake vetpass, andFuzzMappingPairs_ReadsWhatTheModelReadsandFuzzCompilerun two minutes each with no finding. At ad83c76, the last commit to change code, the three packages the later commits change (naming,load,overlay) are at 100% statement coverage. The full gate is left to CI.MappingPairsto the parser's model (nodeview.FuzzMappingPairs_ReadsWhatTheModelReads, 619k executions clean, its one finding kept as a seed); the walker's bounds on 100k-deep and 100k-wide trees; the scalar classifier's agreement table;oas.TestCheckFormat_*;naming.TestAt_*over every owner, structural step and branch shape, the hop cap at its boundary in both asking orders, and unresolved references;bind.TestKinds_*, including the lesser of two readings across binder rounds;anchors.TestBuild_*; and the conformance caseyaml-merge-keys, which merges a field into each kind of raw reader (path-item and operationservers, a response'slinks, a header'sstyle,allowEmptyValue, an encoding'sallowReserved, a security scheme's stray field, a validation-only keyword) beside the 3.2 media type readers' own merged cases. Each of those is lost onmain.Kinds, the anchor index, the union variant naming, the lint check andnodeview.Childwas mutated, and every mutant reddens a test. So does each guard of the nearest-owner rule (structureRoot,inStructure,documentName), each walk-bound clamp, and a namer that reads only a stringoperationId.main: the Cloudflare schema compiles within 2% of main's time, a document merging one wide mapping into each of its schemas, at the alias budget's ceiling, within 6%, and a schema nested 800itemsdeep in main's time, while all of them now read whatmainskipped. Earlier heads of this branch took 1.9 and 2.6 times main's on the last two; the twoperfcommits record why and what changed. Re-timed with the nearest-owner rule (f462067), the 800-deep chain is within 4% ofmainand the Cloudflare schema within 1%.mainat 2683dee, over the 1,881 real-world OpenAPI 3 documents, the 182 documents under the module'stestdata/directories that declare a version, and the Cloudflare schema at e3ada35b, built at f462067: the IR differs in 14 documents, each an intended change. Two keep their integer-keyed extensions (x-speakeasy-enums,x-fingerprint-mixed-keys) rather than drop them as unserializable, one reads its aliased schema constraints, and 11 name their whole-document schemas for their files: 32 names, each a document's root or what composes off it, and no other name. Diagnostics and exit codes are the same everywhere but the first two, which no longer warn. The Cloudflare IR is byte-identical. A whole-document$refheld by a union variant appears in none of these documents, which is why the c7254c8 sweep missed that this branch had renamed such a variantvariant_N;schema.TestUnionVariant_AWholeDocumentIsNamedForItnow holds it.morphic-harnessgives every corpus andtestdata/spec the same verdict onmainand at ad83c76 (2,063 of 2,063). At 0f7aec0 the two-order oracle, run directly over all 2,220 documents including those that stop at an error, finds the same 6 order-dependent documents onmainand on this branch, and no new one.naming.Namerequals the name the old lowering gave for 80,826 of 80,826 nodes and 8,862 of 8,862 union variants (details on openapi: derive a node's name from its coordinate in one function, and diff it against the names the lowering gives #980). The openapi: a union variant naming a position in a path's body takes the pointer's name #729 and openapi: a $ref into a component request body or header names the subtree beneath its target by the reference #747 reproductions name alike with and without the$refthat reaches them. That comparison predates the nearest-owner rule, whose one departure from those names on the corpus is the document roots listed above.