Skip to content

fix(compilers/openapi): keep a redeclaration's losing type - #436

Open
fuad-daoud wants to merge 4 commits into
stack/1-detectionfrom
stack/2-redeclaration
Open

fix(compilers/openapi): keep a redeclaration's losing type#436
fuad-daoud wants to merge 4 commits into
stack/1-detectionfrom
stack/2-redeclaration

Conversation

@fuad-daoud

@fuad-daoud fuad-daoud commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Closes #424. Not breaking.

When allOf branches declare the same field with incompatible types, the first declaration wins, a warning names both pointers, and the losing type was simply dropped — not written to Unmodeled.

Every other degradation in this compiler keeps what it could not model. That is the discipline: an Unmodeled entry with a Reason, so nothing reaches the IR in no form. This path was the exception, and it was silent to any consumer reading the document rather than the diagnostic stream.

102 occurrences in GitHub's published spec — e.g. webhook-fork/properties/forkee/allOf/1/properties/clone_url, where t/prim/url and t/prim/string disagree and the first wins. A consumer diffing two versions of that document sees "no change" across a release where the losing branch's type moved.

The whole ir.TypeRef is kept (so Nullable survives, not just the target ID), keyed by the redeclaration's own pointer so three conflicting branches keep three entries rather than the last overwriting the rest.

Scope note: the constraint half of the same diagnostic still discards its loser. That is left alone deliberately — #10 records the direction for it (intersect the bounds), and preserving the loser instead would settle a decision that already has one.


Stack 2 of 8. Base stack/1-detection — review and merge bottom-up. Every commit here passed make gate when it landed, and the full gate was re-run on the top of the stack. Run it as GOTOOLCHAIN=go1.26.3 make gate; this machine's Go 1.27 fails it for reasons unrelated to any change (#431).

🤖 Generated with Claude Code

https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1


Review round 2 (commit 2)

Addresses the review on this PR. Three findings shared one root — the discard was partial and the preservation gate was narrower than the drop — and are fixed together in reconcileProperty.

The discard was partial. After a type conflict fired, the fold ran to completion anyway, so {id: integer} allOf {id: string, maxLength: 10, default: "abc"} compiled to an integer carrying a string default and a string's maxLength, beside an entry saying the string declaration was discarded. Nothing downstream compares a Value's kind to its property's type. recordRedeclarationConflict now returns whether the type was discarded, and Default/Constraints/Examples are gated on it; Deprecation and XML are not shape bound and are adopted either way.

The gate was narrower than the drop. Preservation hung off typesConflict, which deliberately answers false for two composites of one kind, an unresolvable target, and the top type against anything. In each of those dst kept its type and src's vanished silently — #424's own failure. Preservation is now owed wherever a type is dropped; the diagnostic stays on the conflict predicate, since "dropped" and "contradictory" are different claims.

Nullability was neither reconciled nor preserved. typesConflict returns on a.Target == b.Target before Nullable is read, so {x: [string, "null"]} allOf {x: string} merged to nullable with no diagnostic and no entry — and swapping the branches gave the opposite answer. Targets that agree now intersect nullability. The order oracle could not have caught this: it never permutes sequences.

Also fixed: the type route's diagnostic now names the preservation, as every other preserving degradation here does; MergeProperty takes the declaration's position once, off p.Provenance, instead of as a second parameter equal to it by construction; keepLosingType guards an empty pointer or target, which would collapse the key to the bare prefix and let a second loser overwrite the first; diagnoseRedeclarationConflict is renamed for the recording it does and its comment corrected; the false universal about every degradation preserving what it could not model is deleted; ir-design.md gains the §4.8 entry and the §14 mapping key this key was already cited against; and the two tests stop re-implementing their helpers — one was failing on assert.JSONEq(want, "") rather than on the missing key.

Title shortened to 65 characters with the number, from 77.

Known gaps, deferred deliberately

Both are stated in code beside the relevant function and filed:

@Wahbeh-Mohammad Wahbeh-Mohammad left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two-pass review of this change (design/coherence, then adversarial), verified against the source at 8a42a7f. Inline comments carry the findings and suggested fixes. Three of the majors share one root: the "discard" is partial (annotations still fold) and the preservation gate (typesConflict) is narrower than the drop. They are cleanest fixed together in reconcileProperty.

One item with no line to anchor to:

Major — PR title over the cap. printf '%s (#436)' "<title>" | wc -c gives 77; the squashed subject cap is 72. Suggested: fix(compilers/openapi): keep a redeclaration's losing type (63 with the number).

Vetted and sound, for the record: site and layering (merge, not pass/); PreserveInto reuse and its empty-payload guard; entry on the merged property per §12.1; pointer in the key for order-invariance, RFC 6901-escaped so injective; MergeUnmodeled runs after and cannot clobber the new key; N-way conflicts yield N-1 entries; golden reddens on revert; the constraint-half carve-out is stated in code.

Comment thread compilers/openapi/internal/merge/merge.go
Comment thread compilers/openapi/internal/merge/merge.go Outdated
Comment thread compilers/openapi/internal/merge/merge.go
Comment thread compilers/openapi/internal/merge/merge.go
Comment thread testdata/conformance/openapi/allof-conflicting-type.yaml
Comment thread compilers/openapi/internal/merge/merge.go
Comment thread compilers/openapi/internal/merge/merge.go
Comment thread compilers/openapi/internal/merge/merge.go
Comment thread compilers/openapi/internal/schema/compose_test.go Outdated
Comment thread compilers/openapi/conformance_unmodeled_test.go Outdated
fuad-daoud added a commit that referenced this pull request Sep 9, 2026
…ord every drop

Addresses the review on #436. Three findings shared one root: the discard was
partial and the preservation gate was narrower than the drop.

**The discard was partial.** After a type conflict fired, reconcileProperty ran
to completion anyway, folding the loser's Default, Constraints and Examples onto
the winner. So `{id: integer}` allOf `{id: string, maxLength: 10, default: abc}`
compiled to an integer carrying a string default and a string's maxLength, beside
an Unmodeled entry saying the string declaration was discarded. Nothing in
pass/validate or irverify compares a Value's kind to its property's type, so an
emitter renders that pair into code that does not compile. The fold predates this
branch; recording the loser is what made the document self-contradictory.
recordRedeclarationConflict now returns whether the type was discarded and the
three shape-bound folds are gated on it. Deprecation and XML are not shape bound
and are adopted either way.

**The gate was narrower than the drop.** Preservation hung off typesConflict,
which deliberately answers false for two composites of one kind, an unresolvable
target, and the top type against anything — "conflict detection does not guess".
In every one of those dst kept its type and src's vanished with neither a
diagnostic nor an entry, which is #424's own failure: a consumer diffing two
versions sees no change. Preservation is now owed wherever a type is dropped;
the diagnostic stays on the conflict predicate, because "dropped" and
"contradictory" are different claims.

**Nullability was neither reconciled nor preserved.** typesConflict returns on
`a.Target == b.Target` before Nullable is read, so `{x: [string, null]}` allOf
`{x: string}` merged to nullable with no diagnostic and no entry — and swapping
the branches gave the opposite answer. The order oracle cannot see this: it
never permutes sequences. Targets that agree now intersect nullability, the same
conjunction foldNullVerdicts states for a single schema.

Also from the review: the type route's diagnostic says the loser is kept, as
every other preserving degradation in this compiler does; MergeProperty takes the
declaration's position once, off p.Provenance, rather than as a second parameter
equal to it by construction; keepLosingType guards an empty pointer or target,
which would collapse the key to the bare prefix and let a second loser overwrite
the first; the diagnose-named function is renamed for the recording it does; a
false universal about every degradation preserving what it could not model is
deleted; ir-design gains the §4.8 entry and the §14 mapping key that this key was
already being cited against; and two tests stop re-implementing their helpers,
one of which was failing on the wrong assertion when a key was absent.

Two findings are deferred with the gap stated in code and issue:
- **#445** — the Unmodeled value is an IR TypeRef where §12 asks for the source
  construct. Fixing it moves MergeProperty's signature and the golden.
- **#446** — typesConflict reads a format-narrowed primitive as conflicting with
  its bare primitive, so uri-vs-string reports a degradation that did not happen,
  102 times in the GitHub spec. The predicate is upstream of this change.

Refs #424

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
@fuad-daoud fuad-daoud changed the title fix(compilers/openapi): keep a conflicting redeclaration's losing type fix(compilers/openapi): keep a redeclaration's losing type Sep 9, 2026
@Wahbeh-Mohammad

Copy link
Copy Markdown

Review done by me + fable

fuad-daoud and others added 4 commits September 10, 2026 10:21
The Makefile's claim is that `make gate` is what CI runs, and that is what makes
a red gate worth believing. It was not true on a machine with a newer Go than
CI's: golangci-lint's bundled staticcheck builds its own IR of every package it
loads, the standard library included, so a stdlib it does not know panics it
before it reaches a line of this repo. On Go 1.27, `make gate` failed at `lint`
with five panics in `internal/poll`, and `coverage` then failed two rows that
pin an encoding/json escape 1.27 spells differently — neither having anything to
do with the change under test, which is the situation that teaches people to
ignore a red gate.

The gate now pins GOTOOLCHAIN, read from go.mod's own go directive so the
version has one definition, and exported so the scripts and golangci-lint see
it too — the linter reads the stdlib through `go list`. CI reads the same line
through setup-go's go-version-file, replacing the literal that was a second
copy of it. A GOTOOLCHAIN already set in the environment still wins and is
reported, exactly as a local golangci-lint of the wrong version is.

Measured on a Go 1.27 machine with nothing set: `make gate` exits 2 before this
change and 0 after.

This does not move the toolchain. Bumping it needs a golangci-lint whose
staticcheck knows the newer stdlib and a rewrite of the two rawDivergences rows,
which are now commented where each will be reached; that is a deliberate change
of its own, not a side effect of this one.

Closes #431


Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Format detection decoded a document's whole root mapping into a two-field
struct to read the `openapi` / `swagger` key. yaml.v3 compares every pair of a
mapping's keys before it reads any of them, so a mapping repeating one key n
times raises n(n-1)/2 errors and then abandons the mapping — the probe came
back empty as well as expensive. A 32 KB source repeating one key 6,553 times
produced 21,467,628 errors and a 1.2 GB diagnostic in 16.7 s; a 128 KB one did
not finish in 150 s. Both were reported as unreadable, though the parser the
compiler goes on to use reads them and reports the repeats itself, once each
and sited.

Detection now parses the document and reads the two keys off the tree, which is
linear and answers the same for a mapping whose keys repeat as for one whose
keys do not. The 32 KB case takes 0.048 s and prints 6,553 sited warnings; the
128 KB case takes 0.147 s.

Separately, diag.OneLine now bounds what a foreign error contributes to a
diagnostic message. That is the general form of the same defect — a message a
library can make arbitrarily large — and it covers the two overlay callers as
well, where the library's own decode is still slow but its complaint no longer
reaches the terminal whole. The cut lands on a rune boundary, so a message
never carries half a rune to a reader.

Two rules the walk now has and the decoder could not, since it refused any
mapping that repeated a key at all: a key written twice takes its last
spelling, matching the parser that later records the dialect on ir.SourceInfo,
so one document cannot get two answers; and a key written directly beats one
merged in through `<<`.

Deliberately out of scope: the merge chain is bounded at maxMergeDepth, where
the decoder followed one as far as yaml's own alias limits, and detection still
reports an unreadable version key only where declaresProbeKey sees it declared
at column 0 — widening that guard would claim documents of formats that nest a
key of the same name.

Closes #443


Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
When allOf branches declare one field with incompatible types the merge
keeps the first declaration and warns, naming both pointers. The losing
declaration was then dropped: it reached the IR in no form at all, so a
consumer reading the document rather than the diagnostic stream saw no
trace of it — and a diff across two revisions in which only the losing
branch's type moved reported no change. GitHub's published spec writes
this shape 102 times.

Every other degradation in this compiler keeps what it could not model.
This one now does too: the discarded ir.TypeRef is written to the merged
property's Unmodeled under ReasonDegradedLowering, keyed by the
redeclaration's own pointer so sibling branches never overwrite one
another, and stamped with the losing declaration's provenance.

The constraint half of the same diagnostic is deliberately left alone.
It also discards the redeclaration's keyword, but the recorded direction
there is to intersect the bounds so the merged field satisfies both
branches (#10), and preserving the loser instead would settle a decision
that already has one. The code comment on keepLosingType says so.

Fixes #424

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1
…ord every drop

Addresses the review on #436. Three findings shared one root: the discard was
partial and the preservation gate was narrower than the drop.

**The discard was partial.** After a type conflict fired, reconcileProperty ran
to completion anyway, folding the loser's Default, Constraints and Examples onto
the winner. So `{id: integer}` allOf `{id: string, maxLength: 10, default: abc}`
compiled to an integer carrying a string default and a string's maxLength, beside
an Unmodeled entry saying the string declaration was discarded. Nothing in
pass/validate or irverify compares a Value's kind to its property's type, so an
emitter renders that pair into code that does not compile. The fold predates this
branch; recording the loser is what made the document self-contradictory.
recordRedeclarationConflict now returns whether the type was discarded and the
three shape-bound folds are gated on it. Deprecation and XML are not shape bound
and are adopted either way.

**The gate was narrower than the drop.** Preservation hung off typesConflict,
which deliberately answers false for two composites of one kind, an unresolvable
target, and the top type against anything — "conflict detection does not guess".
In every one of those dst kept its type and src's vanished with neither a
diagnostic nor an entry, which is #424's own failure: a consumer diffing two
versions sees no change. Preservation is now owed wherever a type is dropped;
the diagnostic stays on the conflict predicate, because "dropped" and
"contradictory" are different claims.

**Nullability was neither reconciled nor preserved.** typesConflict returns on
`a.Target == b.Target` before Nullable is read, so `{x: [string, null]}` allOf
`{x: string}` merged to nullable with no diagnostic and no entry — and swapping
the branches gave the opposite answer. The order oracle cannot see this: it
never permutes sequences. Targets that agree now intersect nullability, the same
conjunction foldNullVerdicts states for a single schema.

Also from the review: the type route's diagnostic says the loser is kept, as
every other preserving degradation in this compiler does; MergeProperty takes the
declaration's position once, off p.Provenance, rather than as a second parameter
equal to it by construction; keepLosingType guards an empty pointer or target,
which would collapse the key to the bare prefix and let a second loser overwrite
the first; the diagnose-named function is renamed for the recording it does; a
false universal about every degradation preserving what it could not model is
deleted; ir-design gains the §4.8 entry and the §14 mapping key that this key was
already being cited against; and two tests stop re-implementing their helpers,
one of which was failing on the wrong assertion when a key was absent.

Two findings are deferred with the gap stated in code and issue:
- **#445** — the Unmodeled value is an IR TypeRef where §12 asks for the source
  construct. Fixing it moves MergeProperty's signature and the golden.
- **#446** — typesConflict reads a format-narrowed primitive as conflicting with
  its bare primitive, so uri-vs-string reports a degradation that did not happen,
  102 times in the GitHub spec. The predicate is upstream of this change.

Refs #424

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v
@fuad-daoud
fuad-daoud force-pushed the stack/2-redeclaration branch from 6728841 to 3f2b70d Compare September 10, 2026 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants