Repository navigation
refactor(compilers/openapi): reduce the root to its public face - #1104
Merged
Merged
Conversation
The root package lowered document-level metadata itself: meta.go's info, extensions, unknown keys, retained components and dialect, and servers.go's server lowering. That is lowering, not the compiler's public face, so it moves to internal/document, split by what each file lowers. Servers move with it rather than staying behind a callback the root would hand in. The document's list is the only one the IR holds, so the document is the lowering's one reader; a field for path-item and operation servers would move it to a package both walks reach, whichever package it leaves. docMeta and lowerMeta become document.Meta and document.Lower. Every other moved declaration is byte-identical, and the diagnostics keep their order, since Lower still calls the server lowering where lowerMeta did. The tests follow the code: unit tests into _internal_test.go files, the specs compiled end to end into the external test package, as auth's are. The root no longer imports the parser library, so its allowlist drops it.
Most of detect.go was not the Detect method but how detection reads a source: the parse and the probe of its root mapping, and the byte scan that decides whether a source that will not parse is this compiler's to report. That moves to internal/detect behind one entry, Recognize, which takes the source index, the bytes and the byte budget and answers as Detect does. Detect is left turning the caller's options into that budget. One entry, rather than exporting the probe's parts for Detect to switch on: which decline to report needs what only the probe knows, its cut-short merge read and its scan of unreadable bytes, so the switch belongs with it. Recognize takes no path, which makes Detect's "the path is not consulted" true by construction, and the test tables lose the path column they carried for nothing. The tests follow the code, renamed TestRecognize_* where they called Detect. The root keeps those of its own: the byte budget read from the caller's options and detection's agreement with the compile, now in its external test package. Two new tests pin from inside the package what only the root's tests reached: the byte budget, and that every decline names the source index it is handed. rootMergeChain, wanted by both packages, moves to openapitest. The root no longer imports yaml.v3, so its allowlist drops it.
The moves carried comments that described their old home. Detection's comments named Detect where Recognize now does the asking; two test comments pointed "below" at tests in another file; one called a test of Recognize a test of the public face; Meta's comment spoke of "every function below" in a file that now holds one. Each names what it means now. Lower's comment also states its precondition, that it runs after every other lowering, which only retainUnreferencedComponents' comment said. The root package comment no longer lists recognition and document metadata among what the package holds, and docs/architecture.md places version sniffing in internal/detect and adds both packages to the layout.
The x-* and the undeclared keys of the objects around the document metadata were each collected over a list of their own: rootExtensions and tagExtensions for one, rootUnknownSites and tagUnknownSites for the other. The two lists named the same objects at the same scopes and pointers, gave the same reasons twice, and needed a pair of twin tests to keep their nil guards alike. One list, sites, now serves both readers. Each site holds the parsed object, and the extensions are read off it, so a site cannot name one object for its keys and another for its x-*. The order is unchanged, and so is the order of the diagnostics each reader reports.
This was referenced Oct 23, 2026
Section 5.1's target layout put meta and auth under operation/. Neither landed there: auth founded a package because both halves of the compiler reach it, and document metadata and detection stayed in the root until each became a package of its own. The section's as-landed note now says so, as the document records the other places where its layout and the tree parted.
document's package comment said a field for path-item and operation servers would move their lowering to a package both walks reach. That is one of two choices: the operation walk could reach internal/document instead, and the change that adds the field decides which. The comment now names both. Detect's comment said only that unreadable bytes declaring a version are reported. A root merging deeper than detection reads is declined with an error as well, as Recognize's comment already says; Detect's says so too.
Comments still described the old layout, or named nothing in the new: - load's server-repeats test placed TestDuplicateServerName_CanonicalCollisionsAreNotReported in the openapi package; it is the document package's now. - resolve.Reached said no lowering reads it and the compiler asks it. Its one reader is document.Lower, a lowering itself, which asks once every other lowering has returned; lowering.Ctx.Reached says the same. - Compile's step comment named the last lowering meta. - serverVariables likened its direct field read to an unqualified XMLHints, which names nothing in its package: annotation.XMLHints. TestLowerServers_EveryEntrySkippedIsNil also spoke of "the same guard" with nothing beside it to say which, a reference the move put further away. It names the schemes' test whose guard it mirrors.
micro-compiler-design.md's note on resolve.Reached, and the field comment in lowering.Ctx, both said no lowering reads the record and the compiler reads it once the last lowering returns. Its one reader is document.Lower, itself a lowering, which reads it once every other lowering has returned. Both now say that, as resolve.Reached's own comment does. The note on where meta and auth landed also mixed two kinds of note. The document marks how the restructuring landed with "As landed" and what changed after it with "Later", so the part #1091 did is now a "Later" note of its own.
Detect's and Recognize's comments said a decline explains itself only when the source is this compiler's own, and counted a root merged past the depth detection reads among those. It is not one. Unless the root's own keys settle the version, it is declined with an openapi/budget-exceeded error whatever its format, since a version past the bound may outrank one read above it, as docs/compilers/openapi.md#yaml-decoding says. A YAML file of another format whose root merges 65 levels deep and declares no version anywhere draws the error, on main as here. Both comments now name the two declines that report whatever the format, the byte budget and the cut-short read, and keep the source's being this compiler's own for the one that declares a version and cannot be read. The recognize test helper said it calls Recognize as Detect does, with no byte budget; Detect passes the caller's. It now says the helper reads Detect's source with no budget.
TestRecognize_AReadCutShortIsReported held the bound to a chain with an openapi version at its foot, so a decline that reported only bytes naming that key passed it. A second row puts another format's keys there instead, and reporting the cut only when the bytes name openapi now turns it red.
TestSites_ListEveryObjectAroundTheMetadata named every site's scope and pointer but not the object it holds, so a site reading the wrong object passed it and only the root package's conformance tests noticed. It now checks each by identity, the one way to tell the root's externalDocs from a tag's. Giving the contact's site the license, a tag's externalDocs site the root's, or the components site the root each turns it red.
auth's package comment, and its archtest rule, said the document walk lowers the schemes. With internal/document a package, that reads as document.Lower, which lowers none; the root's run does. They also said neither half of the compiler may reach the other, and that putting either half's lowering with its caller would make the other import it. The root imports the operation walk, and the two halves share no code, so neither holds. The comment now gives the reason that does: the requirements cannot sit in the root, which the operation walk may not import, and the schemes are the document's, no part of lowering an operation. The as-landed note in micro-compiler-design.md, which repeated the old reason, now says which part of the compiler reaches each half instead.
lowering's package comment gave the reason auth's did, that the schema walk and the operation walk both need it and neither may reach the other. The operation walk imports the schema walk, as micro-compiler-design.md §5.1, which the comment cites, lays out. What holds is that every lowering package reads the context and only a package beneath them all serves each: the schema walk may not reach the operation walk, and auth and document reach neither.
detectionBudget gives options of another compiler's type this compiler's default byte budget, as compilers.Compiler.Detect requires. Nothing held it to that: reading them as no budget at all passed every test, because Detect's tests exceed only a budget the caller sets, and a source past the 64 MiB default costs more than the gate's race run should pay. A table now asks detectionBudget directly for no options, another compiler's, a budget of the caller's own and one turned off, and both that change and leaving a negative budget untranslated turn it red.
Two document tests name an auth test as their twin, TestAuth_ and TestLowerSecuritySchemes_, as though it sat beside them. Both say auth's now, as load's test already says the document package's of the one it names.
serverName likened its hint to operationName, which is the operation package's. Recognize and its budget test named the refusal Load gives, which is load.Load's. Two detect test comments said what Detect passes, which is the compiler's, and a document test said Load's tests for the load package's. Each now names where the thing lives, as serverVariables already says annotation.XMLHints.
#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.
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.
keepJSONSchemaDialect unwrapped the document node before asking nodeview.Child, which unwraps it itself. Every other caller, its sibling keepComponentMediaTypes included, hands Child the root node as the parser gives it; this one now does too.
The yaml-decoding rule declined a root that merges past the depth detection reads only when it writes no version of its own. One that writes swagger itself is declined too: detection reads openapi first, so an openapi a merge past the bound may hold would outrank it. The rule now states the condition settledByOwnPairs checks, that the root writes openapi with a version or writes both keys, and that the decline holds whatever the source's format. A row of TestSniff_ReadsAMergeChainAsFarAsTheViewExpandsIt pins the swagger case, and the rule names that test.
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.
CLAUDE.md said the order among a compiler's internal packages runs from diag up to operation. operation was the one package nothing else internal imports while the root held document lowering and detection; document and detect sit beside it at the top now, so the line names all three.
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
compilers/openapiis meant to hold the compiler's public face and the assembly behind it:Compiler, itsOptionsandrun. It also held document-level lowering (meta.go,servers.go) and most of detection (detect.go: the probe of a parsed root, and the byte scan for a source that will not parse). Both move into packages underinternal/. The root's production files are nowdoc.go,openapi.go,options.goanddetect.go, the last holding onlyDetectand the byte budget it reads from the caller's options.internal/documentlowers what a document says about itself: info, rootexternalDocsand servers. It also keeps verbatim what the objects around them write that lowers to no node of its own: extensions, undeclared keys,jsonSchemaDialect,components/mediaTypesand unreferenced components.docMetaandlowerMetabecomedocument.Metaanddocument.Lower. The files split by what each lowers:info.go,servers.go,sites.go,components.goanddialect.go.internal/detectrecognizes a source from its bytes behind one entry,Recognize(srcIndex, data, maxBytes), which answers whatDetectanswers.Detectis now one call.rootExtensionsandtagExtensionsfor one,rootUnknownSitesandtagUnknownSitesfor the other. The lists named the same objects at the same scopes and pointers, and twin tests kept their nil guards alike. One list,sites, now serves both readers, and the extensions are read off the object each site holds, so a site cannot name one object for its keys and another for its x-*.TestSites_ListEveryObjectAroundTheMetadatareplaces the twin tests and holds each site to its scope, its pointer and, by identity, the object it reads.ir,compilers,compilers/compileand its owninternal/tree, and each new package has a rule of its own.internal/document's names two more than openapi: move document lowering and detection's byte reading out of the root package #1091 listed:compilers/compile, which names a server, and the library'sextensionspackage, which types the site list.The IR, the diagnostics, the goldens and the exported API are all unchanged.
Where this departs from #1091's draft
servers.gomoves too, intointernal/document. Kept in the root, it would have had to reachdocument.Loweras a function parameter, a seam that exists only because of where a file sits. Keeping it there does nothing for path-item and operation servers (ir: decide whether four no_ir_home operation constructs get real fields #285) either.internal/operationcan never import the root, which imports it, but it could importinternal/document, or the lowering could move to a package both walks reach. That choice belongs to the change that gives those servers a field. Until then the document's list is the lowering's one reader, andinternal/document's package comment says so.internal/detectexports one entry, not four parts. The draft exportedsniff,declaresProbeKey,majorMinorandsniffProbeforDetectto switch on. Deciding which decline to report needs what only the probe knows: a merge read it cut short, and its scan of unreadable bytes. So the switch stays with the probe,recognizedwith it, and nothing else is exported.Recognizetakes no path, which makesDetect's "the path is not consulted" true by construction.auth's do. Coverage is counted per package, so a test left in the root would cover nothing indocumentordetect. The root keeps the tests of its own code: the byte budgetDetectreads from the options, now asked ofdetectionBudgetdirectly as well, and detection's agreement withCompile. Detection tests that calledDetectnow callRecognizeand are namedTestRecognize_*, and two of their tables lose apathcolumn thatRecognizecannot read.rootMergeChain, now wanted by two test packages, moves toopenapitest.Recognizenow does the asking, "below" pointing at tests in another file, aloadtest placing a moved test in the root package,resolve.Reachedsaying no lowering reads it whendocument.Lowerdoes, and the root package comment anddocs/architecture.md's layout. More comments said what the code does not:Detect's andRecognize's comments counted a root merged past the depth detection reads among this compiler's own sources. Unless its own keys settle the version, such a root is declined with an error whatever its format, asdocs/compilers/openapi.md#yaml-decodingsays: a YAML file of another format merging 65 levels deep draws it, onmainas here. Both comments now name the two declines that report whatever the format, andTestRecognize_AReadCutShortIsReportedgains a row whose chain holds another format's keys. The rule's own wording is corrected too, below.auth's package comment and archtest rule called the root's run "the document walk", which now reads asinternal/document. Beside it they said neither half of the compiler may reach the other and that each half's lowering, put with its caller, would make the other import it. The root imports the operation walk, and the halves share no code, so the comment now gives the reason that holds.lowering's package comment gave the same reason for the schema and operation walks, though the operation walk imports the schema walk. It now says why the context sits beneath every lowering package.CLAUDE.mdran the order among a compiler's internal packages "up to operation", the one package at its top while the root held document lowering and detection.documentanddetectsit there beside it now, and the line names all three.documenttests'authtwins,serverName'soperationName, the refusalload.Loadgives, and what the compiler'sDetectpasses. Each now names its package, asserverVariablesalready saidannotation.XMLHints.docs/micro-compiler-design.md§5.1 gains an as-landed note. Its target layout putmetaunderoperation/, as the draft recalls, andauthtoo; the note records where both went and what reaches each.#1096's done check listed
servers.goamong the root's files; it is edited to match, with a comment there saying why.Carried across #1106
#1106 landed first, for #1096's reason: a behaviour change goes before a move. It edited two files this PR moves, so
mainis merged in (9b74bd5d) and each edit carried to where its code now lives:compilers/openapi/meta.go:keepJSONSchemaDialectandkeepComponentMediaTypes, which now read throughnodeview.Child, replace their moved copies ininternal/document/dialect.goandcomponents.go.compilers/openapi/meta_test.go: thex-bad: {n: .nan}fixture ofTestMeta_UnserializableExtensionStillWarnsgoes tointernal/document/sites_test.go.internal/openapitest/spec.goandspec_test.gokeep both sides' fixture builders and their tests,RootMergeChainandNestedMerges.internal/archtest/arch_test.go'sauthrule keeps this branch's reason with the sentence refactor(compilers/openapi): unify YAML reading and node naming #1106 appended about reading throughnodeview.Built on what #1106 introduced
settledByOwnPairsasksnodeview.WrittenChildrenfor the keys the root writes itself, instead of walking its raw pairs throughnodeview.KeyOfby hand. refactor(compilers/openapi): unify YAML reading and node naming #1106 added that function for exactly this question and left the site alone only because detection was moving. Nothing held the function to the root's own keys: reading them through its merges passed every test, because the row named "a merged version a deeper one may outrank" defined its version and never merged it. It now merges it after the chain, and that reading turns the row red.keepJSONSchemaDialecthandsnodeview.Childthe root node, as every other caller does. It unwrapped the document node first, whichChilddoes itself.yaml-decodingrule says which roots past the merge bound are declined. It said a root "writing no version of its own" is; one that writesswaggeralone is declined too, since anopenapipast the bound would outrank it. The rule now states whatsettledByOwnPairschecks, a row ofTestSniff_ReadsAMergeChainAsFarAsTheViewExpandsItpins theswaggercase, and the rule names that test.docs/architecture.mdlists every package under the compiler, each after every package it imports. The layout left outanchors, which refactor(compilers/openapi): unify YAML reading and node naming #1106 added, and older packages besides, the test-only ones among them, and still callednodeviewthe resolver's reading.retainUnreferencedComponentskeeps itsnodeview.ViewbesidekeepComponentMediaTypes'Child: it enumerates each section's entries and warns when the merge bound cut that short, which only a view can tell it. Past the bound the two do not disagree: a section's entries are reported as not kept, and amediaTypesthe bound hides as unreachable.Closes #1091. Closes #1107.
Test plan
gofmt,go vet ./...,go build ./...,golangci-lintat the pinned v2.14.0, and the tests ofcompilers/...,internal/archtest,internal/harness,internal/conformance,cmd/...andengine/.... On the head,make fmt vet buildtoo, which adds the repository's analyzers.make coverageon the head,mainmerged in:go test ./... -race, andcmd/morphic-covergatereports all 15,144 statements covered.internal/documentreceived are byte-identical with their doc comments, apart from theMetaandLowerrenames. The one exception is a test comment that namedlowerMetaand now namesLower.internal/detectreceived are byte-identical.Recognizeis new: it holds the old body ofDetect, minus the options.TestDetect_→TestRecognize_names, theDetect→recognizecall (one of them now two lines), the twopathcolumns and theopenapitest.RootMergeChainqualifier.keepJSONSchemaDialectandkeepComponentMediaTypesby the merge with refactor(compilers/openapi): unify YAML reading and node naming #1106, andsettledByOwnPairsby the commit built on it.go test -listovercompilers/openapi/...before and after gives the same test names, apart from theTestDetect_→TestRecognize_renames,TestSites_ListEveryObjectAroundTheMetadatain place ofTestTagExtensions_NilEntrySkippedandTestTagUnknownSites_NilEntrySkipped, and four new tests:TestRecognize_HoldsTheParseToItsByteBudget,TestRecognize_ADeclineNamesTheSourceItWasHanded,TestRootMergeChain_MergesEachLevelIntoTheNextandTestDetectionBudget_ReadsTheCallersOptions.morphic compileandmorphic validate, built atmain(87e5d11f, refactor(compilers/openapi): unify YAML reading and node naming #1106 included) and at this branch's head, over 6,728 files: everything undertestdata/plus 6,388 YAML and JSON files from real-world API projects, most of which are not specs, so detection's declines are compared too. Stdout, stderr and exit code are byte-identical for every file.morphic-harnessovertestdata/and the same files gives identical output.siteschanges the output for 2 files undertestdata/,extensions-x.yamlundercompileandunknown_keys.yamlunder both commands, and silencing the undecodable-source report changes the one malformed spec in the corpus, the one file detection reports as undecodable.TestRecognize_AReadCutShortIsReported, which turns red when the cut-short decline is silenced.Recognizenaming source 0 whatever it is handed:TestRecognize_ADeclineNamesTheSourceItWasHanded.Recognizeignoring its byte budget: that test andTestRecognize_HoldsTheParseToItsByteBudget.Detectpassing no budget, or another source index:TestDetect_DeclinesASourcePastTheCallersByteBudget.detectionBudgetreading another compiler's options as no budget, or leaving a negative budget untranslated:TestDetectionBudget_ReadsTheCallersOptions. The first passed every test before it.RootMergeChainone level short:TestRootMergeChain_MergesEachLevelIntoTheNext.settledByOwnPairsreading the root's keys through its merges, or aswaggerthe root writes alone settling the version:TestSniff_ReadsAMergeChainAsFarAsTheViewExpandsIt. The first passed every test before its row merged the version it names.Recognizereporting a cut-short read only when the bytes nameopenapi:TestRecognize_AReadCutShortIsReported.siteshanding a site another object, each of the contact's site the license, a tag's externalDocs site the root's, and the components site the root:TestSites_ListEveryObjectAroundTheMetadata. Before it held the objects, the first drew only the root package's conformance tests.