Skip to content

fix(compilers/openapi): preserve keywords no lowering reads - #348

Merged
OmarAlJarrah merged 4 commits into
mainfrom
fix/openapi-unhomed-keyword-census
Aug 10, 2026
Merged

fix(compilers/openapi): preserve keywords no lowering reads#348
OmarAlJarrah merged 4 commits into
mainfrom
fix/openapi-unhomed-keyword-census

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

A schema keyword that the lowering elected for a position never reads was
dropped: no IR field, no Unmodeled entry, no diagnostic. Two shapes of the
same bug:

  • Beside a $ref.format, enum, const, required and
    additionalProperties written next to a $ref vanished at every position —
    component, property, parameter, header, allOf branch. In JSON Schema 2020-12,
    and so in OpenAPI 3.1, $ref is an ordinary keyword and its siblings are
    conjoined with it, so this is real data.
  • Beside an elected keyword family.type: string next to an allOf that
    composed a Model, format and a bound next to a const that hoisted a
    Literal, and so on.

Both had one root cause. The "what did the winner not consume?" census
(preserveUnhomedKeywords) had exactly one call site, inside lower() on the
body path, so the $ref path ran no census at all; and the census that did run
walked a hand-maintained list of six shape applicators plus a special case for
format, so anything outside that list was dropped whatever it was written
beside.

The census now asks the node the position actually lowered to whether it has a
field for each keyword — a Model has no type token, a Literal has no encoding
and no Constraints, an alias over a $ref target has none of them — instead
of consulting a list of keywords worth keeping. It runs at every position a
schema is lowered at, including the three $ref sites (refSiteRef, an allOf
branch, and the annotation.HomeCarrier positions, which keep the keyword on
the carrier rather than hoisting a node). Anything homeless is kept verbatim
under Unmodeled with ReasonDegradedLowering and one info diagnostic naming
what the position lowered to.

Deliberately unchanged:

  • allOf beside type: object records nothing, because the composed Model does
    assert object. That is the case a keyword list cannot decide and the node
    can, and it is asserted as a control.
  • An allOf branch's required is left out of the branch census:
    applyCompositionRequired reads it, so recording it would report one keyword
    twice.
  • valueConstraintKeywords is now the single list behind both
    declaresValueConstraints and the recorder, replacing a second hand-written
    copy that read model fields where the predicate read raw nodes.

A bound written beside a co-declared oneOf is still dropped; that is a
different mechanism (the alias is interned with nil constraints) and is filed as
#347.

Test plan

  • Table-driven tests over both positions and both shapes:
    TestRefSiteKeywords_KeptAtEveryPosition covers all five $ref siblings at
    the component and property positions;
    TestUnhomedKeywords_ElectedLoweringKeepsWhatItCannotRead covers the elected-
    family cases and asserts the kept set whole, so keeping too much fails as
    loudly as keeping too little. Controls assert zero entries and zero
    diagnostics where nothing is lost.
  • Carrier coverage: TestParams_RefSiteKeywordsAreKeptOnTheParameter,
    TestHeaders_RefSiteKeywordsAreKeptOnTheHeader,
    TestRefSiteKeywords_AllOfBranchKeepsWhatTheAliasCannotHold.
  • TestKeywordHome_EveryCensusKeywordHasANodeThatCarriesIt guards the one
    agreement the census still has to keep by hand — a keyword listed with no arm
    answers "homeless" everywhere, which is exactly what a $ref site expects, so
    no ref-site test could see it. Verified by planting an armless keyword.
  • Conformance corpus: testdata/conformance/openapi/unhomed-keywords.yaml gains
    the $ref-site and elected-family cases, with the aliases declared above
    their targets so the two-order oracle exercises both interning orders.
    Verified that deleting a case from the fixture reddens the suite.
  • Every new row was confirmed RED with the production change reverted and the
    tests kept.
  • Full gate green: gofmt, go vet, golangci-lint, go build, and the
    coverage gate at exactly 100%.

Closes#268
Closes#283

schemaConstraints gained a residue destination on main (#346) while this branch
was hoisting the `owns` lookup above it; the merged call keeps both. main also
moved the schema and operation test helpers into openapitest, which this
branch's new cases still called unqualified — a conflict no textual merge sees,
since either side compiles on its own.
The four test conflicts are each two independent functions appended at one
spot, sharing the closer that followed them, so taking both needed the first
one's braces put back rather than the markers simply dropped.
minItems, maxItems and uniqueItems reached the IR in no form at all wherever
the position did not lower to a List: no field, no Unmodeled entry, no
diagnostic. Two documents differing only in them compiled to the same types and
services, which is the loss this census exists to stop — it was simply left
outside it.
ir.List.Constraints is their only home. listConstraints is their only reader
and only the array lowering calls it, so an object drops them; so does the
Tuple that prefixItems hoists, which has no Constraints field at all. They are
excluded from valueConstraintKeywords for that same reason, which is what left
them belonging to no reader rather than to declaredConstraints.
The census comment claimed the rest of the vocabulary was captured wherever it
was written. That was the claim these three falsified, so it now says what
holds and names them as the case that got past it.
Three rows on the elected-family table cover the object, the tuple and the
scalar that hoists an alias to carry the entry, and the array is the control
that keeps nothing — it stays green when the three keywords are taken back out
of the census, which is what makes it a control rather than a fourth instance.
The corpus carries the same three so the oracles reach them.
Two slips from admitting the collection bounds. The paragraph explaining why
the division is a claim rather than a proof repeated, verbatim, the sentence
the paragraph below it already carried about a keyword in neither list. And
"valueConstraintKeywords excludes them for that reason" pointed at the
sentence about ir.Tuple, when the reason is that the bounds are List-owned.
Comments only; no statement changed.
@OmarAlJarrah
OmarAlJarrah merged commit 77c7b1f into mainAug 10, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/openapi-unhomed-keyword-census branch August 10, 2026 21:34
OmarAlJarrah added a commit that referenced this pull request Aug 10, 2026
Three production conflicts. load.go took both sides' Options fields and kept
the byte budget where it was, before the parse, dropping this branch's copy of
the pre-parse cycle scan that main moved onto the decoded tree. compose.go and
architecture.md each had one side add a paragraph while the other rewrote the
one beside it. The rest were the test helpers main moved into openapitest,
which also gained a lowerer constructor this branch's fixtures had to adopt
while keeping the Limits argument they pass.
The merge also put the enum budget and the unhomed-keyword census (#348) in
each other's way, which neither could see alone. A member set past the budget
degrades to the top type, and ir.Any has a field for nothing, so the census
preserved the whole set verbatim: 5,000 members refused at a budget of 10 came
back as 50,001 bytes under Unmodeled, more than the source they were read from.
The budget bounded the per-member amplification and nothing else.
keywordHome now counts Any a home for `enum`, exactly as it already did for
`const` and for the same reason — lowerEnum read the members and announced the
degradation, so claiming them again both double-reports and undoes the refusal.
typeShapedBy counts it for `type` too: the top type contradicts no declared
shape, and a position only arrives there by a degradation that has already been
reported. The budget test now also pins that the refused node keeps nothing,
which is the half a diagnostic count does not state.
OmarAlJarrah added a commit that referenced this pull request Aug 10, 2026
lowerEnum grew a guard on each side: the empty member list here, the member
budget on main (#349). They cannot both hold — a count of zero exceeds no
positive budget — so both are kept, the degenerate list read first, beside the
members it lacks.
The conformance table gained a third column on main (#342), naming the matrix
rows a case witnesses. empty-enum names none: its subject is the degenerate
list rather than any enum capability, and that table's own comment warns that
reaching for a construct on the way to a different subject is not witnessing
it.
The empty-enum golden moved, because the unhomed-keyword census (#348) reaches
this branch for the first time here. `type: object` beside an Enum now lands
under Unmodeled with the property set that was already kept — an Enum carries
neither — so the case preserves strictly more than it did, and its diagnostic
names both keywords.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant

@OmarAlJarrah