From be356160a90aa3c65f43fb07f0fdca472c2c195a Mon Sep 17 00:00:00 2001 From: Fuad Daoud <61579692+fuad-daoud@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:21:04 +0300 Subject: [PATCH 1/4] build: pin the gate's Go toolchain to the one go.mod names (#449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/gate.yml | 5 ++- Makefile | 34 +++++++++++++++++++ .../annotation/rawjson_internal_test.go | 7 ++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/.github/workflows/gate.yml b/.github/workflows/gate.yml index c425be59..396bcce3 100644 --- a/.github/workflows/gate.yml +++ b/.github/workflows/gate.yml @@ -17,7 +17,10 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 with: - go-version: "1.26.3" + # Read from go.mod, so the toolchain has one definition and this file + # holds no copy of it to drift. `make gate` pins the same version from + # the same line, which is what makes the two the same run. + go-version-file: go.mod - name: gofmt run: make fmt - name: vet diff --git a/Makefile b/Makefile index e744502b..9ddfa494 100644 --- a/Makefile +++ b/Makefile @@ -12,6 +12,37 @@ GO ?= go +# The Go toolchain the gate runs on, read from go.mod so the pin has one +# definition and no copy — the same discipline GOLANGCI_LINT_VERSION gets below, +# and for a sharper reason: the gate is only "the commands CI runs" if it runs +# them on the toolchain CI uses. golangci-lint bundles a staticcheck that builds +# its own IR of every package it loads, the standard library included, so a +# stdlib newer than that release knows panics the linter before it reaches a +# line of this repo. Exported, because the scripts below and golangci-lint each +# invoke go themselves, and the linter reads the stdlib through `go list`. +# +# Moving this means moving GOLANGCI_LINT_VERSION with it, to a release whose +# staticcheck knows the new stdlib, and rewriting the two rows of +# rawDivergences that pin an encoding/json escape (#431). Bump the three +# together or the gate fails on something other than the change under test. +GO_VERSION := $(shell sed -n 's/^go //p' go.mod) +ifeq ($(GO_VERSION),) +$(error no go directive found in go.mod; the toolchain pin has nothing to read) +endif + +GOTOOLCHAIN ?= go$(GO_VERSION) +export GOTOOLCHAIN + +# A GOTOOLCHAIN already set in the environment wins, as an override should. It is +# reported rather than refused, exactly as a local golangci-lint of the wrong +# version is: a developer running the gate on another toolchain should know the +# result is not CI's, and a toolchain setting is not a reason to decline to run +# the gate at all. +ifneq ($(GOTOOLCHAIN),go$(GO_VERSION)) +$(warning warning: GOTOOLCHAIN is $(GOTOOLCHAIN), go.mod pins go$(GO_VERSION)) +$(warning warning: unset GOTOOLCHAIN to run the gate as CI runs it) +endif + # The golangci-lint release CI installs. The workflow reads it back from # `make print-lint-version`, so the pin has one definition and no copy: without # it the action installs whatever it resolves as latest that day, and an @@ -91,3 +122,6 @@ bench-smoke: print-lint-version: @echo $(GOLANGCI_LINT_VERSION) + +print-go-version: + @echo $(GO_VERSION) diff --git a/compilers/openapi/internal/annotation/rawjson_internal_test.go b/compilers/openapi/internal/annotation/rawjson_internal_test.go index a92dde9f..753fbd19 100644 --- a/compilers/openapi/internal/annotation/rawjson_internal_test.go +++ b/compilers/openapi/internal/annotation/rawjson_internal_test.go @@ -341,6 +341,13 @@ var rawDivergences = map[string]struct{ old, want string }{ // The old spelling is the escape encoding/json writes for a byte no UTF-8 // can name, which is the loss itself: 0xFF and a source that really wrote // U+FFFD both reached the IR as this, with nothing to tell them apart. + // + // The escape is written out because it is what encoding/json produced, and + // that is what ties this row to a toolchain: Go 1.27 writes the replacement + // character raw where 1.26 escaped it, so this row and the one below redden + // there. Whichever change moves the go directive in go.mod owns rewriting + // both — the claim they make is that the old conversion lost the byte, not + // that a version of encoding/json spelled the loss one way (#431). `!!binary /w==`: {"\"\\ufffd\"", `"/w=="`}, // Nesting is the same rule one level down: one divergent scalar makes the // whole construct diverge, which is how every raw site holding a structure From d0e25a050e15ba67e5c2dc3d1c59eee6efe01ef1 Mon Sep 17 00:00:00 2001 From: Fuad Daoud <61579692+fuad-daoud@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:43:47 +0300 Subject: [PATCH 2/4] fix(compilers/openapi): detect without decoding the root mapping (#448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- compilers/openapi/detect.go | 191 ++++++++++++- compilers/openapi/detect_test.go | 285 +++++++++++++++++++ compilers/openapi/internal/diag/diag.go | 47 ++- compilers/openapi/internal/diag/diag_test.go | 76 ++++- internal/archtest/recursion_test.go | 4 + 5 files changed, 592 insertions(+), 11 deletions(-) diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index c54fc80d..3f5a463e 100644 --- a/compilers/openapi/detect.go +++ b/compilers/openapi/detect.go @@ -3,6 +3,7 @@ package openapi import ( "bytes" "encoding/json" + "fmt" yaml "gopkg.in/yaml.v3" @@ -27,12 +28,29 @@ const maxSniffBytes = 64 << 10 // which is the whole reason the byte cap alone does not answer the question. const maxSniffEntries = 512 +// maxMergeDepth bounds how far a root mapping's merge keys are followed. A `<<` +// value may be an alias to a mapping that merges another, and an anchor may name +// a mapping that reaches itself, so the chain is not bounded by the document. +// Detection reads two keys off the root, which a document that merges at all +// reaches in one step; eight leaves room for a written chain and none for a +// crafted one. +const maxMergeDepth = 8 + +// mergeTag is the tag YAML resolves `<<` to. The tag is read rather than the +// key's text, because a mapping may legitimately hold a key spelled "<<" that +// was quoted into a plain string and merges nothing. +const mergeTag = "!!merge" + // sniffProbe holds the two discriminating top-level keys. Which one is present // is the whole of the format question: an OpenAPI 3.x document declares // `openapi`, a Swagger 2.0 document declares `swagger`. +// +// It carries no struct tags: nothing decodes into it. Both readers — the flow +// one over a JSON token stream and the block one over a parsed tree — name the +// two keys themselves, in recordEntry and fieldFor. type sniffProbe struct { - OpenAPI string `yaml:"openapi"` - Swagger string `yaml:"swagger"` + OpenAPI string + Swagger string } // Detect implements compilers.Compiler. It reports the dialect src declares, @@ -169,14 +187,179 @@ func sniffWhole(data []byte) (sniffProbe, error) { // decodeYAML reads the probe keys from a complete YAML (or JSON, its subset) // document. +// +// The document is parsed and its root mapping read; it is never decoded into +// sniffProbe. That is the whole of the fix for a 32 KB source producing a 1.2 GB +// diagnostic: 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 — +// 21 million of them for the 6,553-line case — and then abandons the mapping, so +// the probe came back empty as well as expensive. Reading the two keys off the +// parsed tree is linear, and answers for a document whose keys repeat exactly as +// for one whose keys do not. The parser this compiler goes on to use reports +// those repeats itself, once each and sited, which is where a reader wants them. func decodeYAML(data []byte) (sniffProbe, error) { - var probe sniffProbe - if err := yaml.Unmarshal(data, &probe); err != nil { + var doc yaml.Node + if err := yaml.Unmarshal(data, &doc); err != nil { return sniffProbe{}, err } + + root := documentRoot(&doc) + switch { + case root == nil: + // A stream that carried no document declares no key, which is a decline + // and not a failure: empty bytes are no more this compiler's than + // anybody else's. + return sniffProbe{}, nil + case root.Kind != yaml.MappingNode: + return sniffProbe{}, fmt.Errorf("document root is %s, not a mapping", root.ShortTag()) + default: + return probeFromMapping(root, maxMergeDepth) + } +} + +// documentRoot returns the content node of a decoded stream's first document, or +// nil for a stream that carried none. Decoding into a yaml.Node yields the +// document node itself, and only the first: a multi-document stream is read to +// its first document here exactly as the compiler's own load reads it. +func documentRoot(doc *yaml.Node) *yaml.Node { + if doc.Kind != yaml.DocumentNode || len(doc.Content) != 1 { + return nil + } + return doc.Content[0] +} + +// probeFromMapping reads the probe keys off a root mapping, following its merge +// keys for a key the mapping does not write itself. +// +// A key written directly wins over one merged in, which is the precedence YAML +// gives a merge. A key written twice takes its last spelling, which is what the +// parser this compiler goes on to use takes: detection names the dialect that +// routes the source, load records the one it read, and a document must not get +// two answers. Neither rule could be had before, since the decoder this replaces +// refused any mapping that repeated a key at all. +// +// depth is the merge chain still allowed. It is the bound on this recursion, +// checked before every descent, and the recursion is otherwise over a parsed +// tree of finite size. +func probeFromMapping(root *yaml.Node, depth int) (sniffProbe, error) { + probe, merges, err := probeFromEntries(root) + if err != nil || depth <= 0 { + return probe, err + } + + for _, merge := range merges { + merged, err := probeFromMerge(merge, depth-1) + if err != nil { + return sniffProbe{}, err + } + probe.fillFrom(merged) + } return probe, nil } +// probeFromEntries reads a mapping's own entries, and returns the values of its +// merge keys separately for the caller to follow. A mapping may write more than +// one `<<`, and their order is the order they are answered in. +func probeFromEntries(root *yaml.Node) (sniffProbe, []*yaml.Node, error) { + var probe sniffProbe + var merges []*yaml.Node + + for i := 0; i+1 < len(root.Content); i += 2 { + key, value := root.Content[i], root.Content[i+1] + if key.Tag == mergeTag { + merges = append(merges, value) + continue + } + field := probe.fieldFor(key) + if field == nil { + continue + } + version, err := probeVersion(value) + if err != nil { + return sniffProbe{}, nil, err + } + *field = version + } + return probe, merges, nil +} + +// probeFromMerge reads the probe keys out of one `<<` value, which YAML admits +// as an alias to a mapping, a mapping written out, or a sequence of either. +// Anything else merges nothing, which is the source's problem to be reported by +// the parser that reads it and not a reason for detection to refuse. +func probeFromMerge(merge *yaml.Node, depth int) (sniffProbe, error) { + if depth <= 0 { + return sniffProbe{}, nil + } + + switch merge.Kind { + case yaml.AliasNode: + if merge.Alias == nil { + return sniffProbe{}, nil + } + return probeFromMerge(merge.Alias, depth-1) + case yaml.MappingNode: + return probeFromMapping(merge, depth-1) + case yaml.SequenceNode: + // A sequence merges each of its entries, earlier ones winning over later, + // which is the precedence YAML gives them. + var probe sniffProbe + for _, item := range merge.Content { + merged, err := probeFromMerge(item, depth-1) + if err != nil { + return sniffProbe{}, err + } + probe.fillFrom(merged) + } + return probe, nil + default: + return sniffProbe{}, nil + } +} + +// probeVersion returns the version string a probe key's value declares, and an +// error for a value that is not a scalar at all. +// +// The scalar's text is taken as written rather than decoded, because the two +// disagree only for tags no version carries — a version key is not !!binary — +// and because decoding is what must not happen here: a mapping handed back to +// the decoder is the quadratic path decodeYAML exists to avoid, and a probe +// key's own value is the last place one could still be handed to it. +func probeVersion(value *yaml.Node) (string, error) { + if value.Kind != yaml.ScalarNode { + return "", fmt.Errorf("version key is %s, not a scalar", value.ShortTag()) + } + return value.Value, nil +} + +// fieldFor returns the probe field that key names, or nil for a key that names +// neither. Only a scalar names one: a mapping or sequence used as a key is legal +// YAML and is not one of the two spellings this looks for. +func (p *sniffProbe) fieldFor(key *yaml.Node) *string { + if key.Kind != yaml.ScalarNode { + return nil + } + switch key.Value { + case "openapi": + return &p.OpenAPI + case "swagger": + return &p.Swagger + default: + return nil + } +} + +// fillFrom takes from other only what p does not already declare, which is what +// makes a merged key lose to a written one. +func (p *sniffProbe) fillFrom(other sniffProbe) { + if p.OpenAPI == "" { + p.OpenAPI = other.OpenAPI + } + if p.Swagger == "" { + p.Swagger = other.Swagger + } +} + // decodeFlowEntries reads the top-level entries of data, which may be a whole // document or a prefix of one, and reports whether it opened a flow mapping. The // JSON decoder is used because it streams: a prefix cut mid-document still diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index 7523650e..433d7d78 100644 --- a/compilers/openapi/detect_test.go +++ b/compilers/openapi/detect_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v3" "github.com/dexpace/morphic/compilers" "github.com/dexpace/morphic/compilers/openapi/internal/diag" @@ -303,3 +304,287 @@ func codesOf(diags []ir.Diagnostic) []string { } return codes } + +// dupRepeats is how many times the fixtures below repeat a root key. It is not a +// threshold: the wrong answer reproduces at two repeats, and this many only +// makes the fixture recognizably a document rather than a corner. It is +// deliberately far below the 6,553 of the report — yaml.v3 raises one error per +// pair of matching keys, so that count produced 21,467,628 of them and a 1.2 GB +// message, and a fixture that large turns a revert into an out-of-memory kill +// instead of a failing assertion. What guards the cost is +// TestSniff_CostIsNotQuadraticInRepeatedKeys, which measures growth rather than +// paying for it. +const dupRepeats = 512 + +// TestDetect_RepeatedKeysDoNotDecideTheFormat pins the fix for the blow-up. A +// document that repeats a top-level key is a document with a duplicate key — +// the parser this compiler goes on to use says so, once per repeat and sited — +// and it is not a document of another format, nor one that cannot be read. +// Detection used to answer both of those, because it decoded the root mapping to +// read two keys and yaml.v3 abandons a mapping that repeats any key at all. +// +// Both orders are pinned: where a writer put the version key says nothing about +// what the document is, and a fixture that declares it first cannot see a +// regression that loses it to the repeats that follow. +func TestDetect_RepeatedKeysDoNotDecideTheFormat(t *testing.T) { + t.Parallel() + repeats := strings.Repeat("x: y\n", dupRepeats) + cases := []struct{ name, src string }{ + {"version first", "openapi: 3.0.3\ninfo: {title: t, version: v}\n" + repeats}, + {"version last", "info: {title: t, version: v}\n" + repeats + "openapi: 3.0.3\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, diags, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(tc.src)}) + assert.True(t, ok, "a document this compiler can lower must not be declined over a repeated key") + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: "3.0"}, got) + assert.Nil(t, codesOf(diags), "the repeats are the parser's to report, sited, not detection's") + }) + } +} + +// TestDetect_ARepeatedVersionKeyAgreesWithTheParser holds detection to the +// answer the lowering will reach. load reads the version off the parsed document +// and records it on ir.SourceInfo, and that parser takes a repeated key's last +// spelling; detection naming the first would give one document two dialects, +// one routing it and one describing it. +// +// The two orders are the test: a single order passes whichever spelling is +// taken. +func TestDetect_ARepeatedVersionKeyAgreesWithTheParser(t *testing.T) { + t.Parallel() + cases := []struct{ first, second, want string }{ + {"3.1.0", "3.0.3", "3.0"}, + {"3.0.3", "3.1.0", "3.1"}, + } + for _, tc := range cases { + t.Run(tc.first+" then "+tc.second, func(t *testing.T) { + t.Parallel() + src := "openapi: " + tc.first + "\nopenapi: " + tc.second + "\ninfo: {}\n" + got, _, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(src)}) + require.True(t, ok) + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: tc.want}, got, + "the last spelling is the one the parser reads and records") + }) + } +} + +// TestDetect_ReadsAVersionKeyThroughAMergeKey holds the merge cases the decoder +// this replaced handled for free. A root that merges another mapping declares +// what that mapping declares, and dropping it would put a new instance of "a +// field supplied through a merge key reaches the IR in no form" into the one +// place that decides whether the document is read at all. +func TestDetect_ReadsAVersionKeyThroughAMergeKey(t *testing.T) { + t.Parallel() + cases := []struct { + name, src, want string + }{ + {"alias", "base: &b\n openapi: 3.1.0\n<<: *b\ninfo: {}\n", "3.1"}, + {"mapping written out", "<<: {openapi: 3.1.0}\ninfo: {}\n", "3.1"}, + {"sequence of aliases", "one: &o\n unrelated: x\ntwo: &t\n openapi: 3.1.0\n<<: [*o, *t]\n", "3.1"}, + {"earlier merge wins", "one: &o\n openapi: 3.0.3\ntwo: &t\n openapi: 3.1.0\n<<: [*o, *t]\n", "3.0"}, + {"a written key beats a merged one", "base: &b\n openapi: 3.0.3\n<<: *b\nopenapi: 3.1.0\n", "3.1"}, + {"a quoted << merges nothing", "base: &b\n openapi: 3.1.0\n\"<<\": *b\ninfo: {}\n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, _, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(tc.src)}) + if tc.want == "" { + assert.False(t, ok, "a key spelled << as a plain string merges nothing") + return + } + require.True(t, ok) + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: tc.want}, got) + }) + } +} + +// TestSniff_BoundsAMergeChain pins the bound on the one recursion this file has. +// A merge key's value may be an alias to a mapping that merges another, so the +// chain is a property of the document and not of its size, and an anchor may +// name a mapping that reaches itself. The bound is what makes the walk finite; +// what it costs is a version key buried deeper than any document writes one. +func TestSniff_BoundsAMergeChain(t *testing.T) { + t.Parallel() + var b strings.Builder + for i := range maxMergeDepth + 2 { + fmt.Fprintf(&b, "l%d: &a%d\n", i, i) + if i == 0 { + b.WriteString(" openapi: 3.1.0\n") + continue + } + fmt.Fprintf(&b, " <<: *a%d\n", i-1) + } + deep := b.String() + fmt.Sprintf("<<: *a%d\n", maxMergeDepth+1) + + probe, err := sniff([]byte(deep)) + require.NoError(t, err, "a chain past the bound is declined, not failed") + assert.Empty(t, probe.OpenAPI, "past the bound the key is not followed to") + + shallow := "l0: &a0\n openapi: 3.1.0\n<<: *a0\n" + probe, err = sniff([]byte(shallow)) + require.NoError(t, err) + assert.Equal(t, "3.1.0", probe.OpenAPI, "the bound must not refuse the depth a document writes") +} + +// TestDecodeYAML_RefusesARootThatIsNoMapping pins the shape complaint. A source +// with no root mapping has no top-level keys, and saying so is what lets Detect +// report bytes that declare a key it serves and will not read. +func TestDecodeYAML_RefusesARootThatIsNoMapping(t *testing.T) { + t.Parallel() + cases := []struct{ name, src, wantErr string }{ + {"sequence", "- openapi: 3.1.0\n", "!!seq"}, + {"scalar", "just a string\n", "!!str"}, + {"empty", "", ""}, + {"comment only", "# openapi: 3.1.0\n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe, err := decodeYAML([]byte(tc.src)) + assert.Empty(t, probe.OpenAPI) + if tc.wantErr == "" { + assert.NoError(t, err, "bytes that carry no document decline rather than fail") + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr, "the complaint names the shape that was read") + }) + } +} + +// TestSniff_CostIsNotQuadraticInRepeatedKeys guards the half of the defect an +// answer cannot see. Reading two keys off a parsed tree is linear in the +// document; decoding the root mapping to read them is quadratic in how often a +// key repeats, because yaml.v3 compares every pair of keys before it reads any +// of them. Both spellings answer alike on a small fixture, and only one of them +// still answers on a large one. +// +// Allocation count is the probe because it is deterministic where wall time is +// not. Doubling the repeats doubles the parse, so the bound is loose enough for +// that and nowhere near a quadratic term: measured at this size the linear +// reading grows by 1.97 and the quadratic one by 4.64. +func TestSniff_CostIsNotQuadraticInRepeatedKeys(t *testing.T) { + head := "openapi: 3.0.3\ninfo: {}\n" + small := []byte(head + strings.Repeat("x: y\n", dupRepeats)) + large := []byte(head + strings.Repeat("x: y\n", dupRepeats*2)) + + smallAllocs := testing.AllocsPerRun(2, func() { _, _ = sniff(small) }) + largeAllocs := testing.AllocsPerRun(2, func() { _, _ = sniff(large) }) + + require.Positive(t, smallAllocs, "a measurement of nothing bounds nothing") + assert.Less(t, largeAllocs, smallAllocs*3, + "twice the repeats must cost about twice, not about four times") +} + +// TestDecodeYAML_RefusesAVersionKeyThatIsNoScalar pins the complaint for a +// version key whose value is a mapping or a sequence, written directly and +// reached through a `<<`. Such bytes name a key this compiler serves and do not +// say what dialect, which is unreadable here and nobody else's. +// +// Whether Detect reports that or declines in silence is declaresProbeKey's +// answer and not this one's, and it is asserted separately below: the guard +// reads a key at column 0, and a merged key is indented under the mapping that +// carries it. +func TestDecodeYAML_RefusesAVersionKeyThatIsNoScalar(t *testing.T) { + t.Parallel() + cases := []struct{ name, src, wantErr string }{ + {"mapping", "openapi: {a: b}\ninfo: {}\n", "!!map"}, + {"sequence", "openapi: [3.1.0]\ninfo: {}\n", "!!seq"}, + {"merged mapping", "base: &b\n openapi: {a: b}\n<<: *b\n", "!!map"}, + {"merged through a sequence", "base: &b\n openapi: {a: b}\n<<: [*b]\n", "!!map"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe, err := decodeYAML([]byte(tc.src)) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr, "the complaint names the shape that was read") + assert.Empty(t, probe.OpenAPI) + }) + } +} + +// TestDetect_ReportsAnUnreadableVersionKeyOnlyWhereItIsDeclared pins the split +// the guard makes. A version key written at column 0 makes the source +// recognizably this compiler's, so a value that is no version is reported; the +// same value reached through a `<<` is indented under the mapping that carries +// it, which declaresProbeKey does not read, so the source is declined in silence +// instead. +// +// The silent half is deliberate and is the direction to be wrong in: the guard +// may not be widened to "the name occurs somewhere followed by a colon" without +// claiming documents of formats that nest a key of that name, and reporting +// those under this compiler's parse error is the one thing detection must not +// do. +func TestDetect_ReportsAnUnreadableVersionKeyOnlyWhereItIsDeclared(t *testing.T) { + t.Parallel() + cases := []struct { + name, src string + wantCode []string + }{ + {"declared at column 0", "openapi: {a: b}\ninfo: {}\n", []string{diag.UndecodableSource}}, + {"reached through a merge", "base: &b\n openapi: {a: b}\n<<: *b\n", nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, diags, ok := New().Detect(compilers.Source{Path: "api.yaml", Data: []byte(tc.src)}) + assert.False(t, ok) + assert.Equal(t, tc.wantCode, codesOf(diags)) + }) + } +} + +// TestDecodeYAML_PassesOverWhatNamesNoVersion holds the walk to reading only +// what it came for. A mapping may key an entry with a sequence, and a `<<` may +// be written with a value that merges nothing; both are the source's business +// and neither stops the two keys beside them from being read. +func TestDecodeYAML_PassesOverWhatNamesNoVersion(t *testing.T) { + t.Parallel() + cases := []struct{ name, src string }{ + {"a key that is a sequence", "? [a, b]\n: v\nopenapi: 3.1.0\n"}, + {"a merge of a scalar", "<<: not-a-mapping\nopenapi: 3.1.0\n"}, + {"a merge of a sequence of scalars", "<<: [x, y]\nopenapi: 3.1.0\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + probe, err := decodeYAML([]byte(tc.src)) + require.NoError(t, err) + assert.Equal(t, "3.1.0", probe.OpenAPI) + }) + } +} + +// TestProbeFromMapping_StopsAtTheBound reaches the bound at the mapping rather +// than at the merge, which is the other of the two places the count is spent. +// It is called directly because the depth a chain lands on is a property of the +// chain, and pinning the bound through one is pinning the chain instead. +func TestProbeFromMapping_StopsAtTheBound(t *testing.T) { + t.Parallel() + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("base: &b\n openapi: 3.1.0\n<<: *b\n"), &root)) + + spent, err := probeFromMapping(documentRoot(&root), 0) + require.NoError(t, err, "a walk that stops at the bound declines; it does not fail") + assert.Empty(t, spent.OpenAPI, "at the bound the merge is not followed") + + within, err := probeFromMapping(documentRoot(&root), maxMergeDepth) + require.NoError(t, err) + assert.Equal(t, "3.1.0", within.OpenAPI, "the same mapping within the bound is read") +} + +// TestProbeFromMerge_DeclinesAnAliasThatResolvedToNothing covers the guard on an +// alias node carrying no target. A parser resolves every alias it accepts, so the +// node is built here rather than parsed: the guard exists because dereferencing +// the field is what the next line does, and a nil there is a panic in detection, +// which runs before the compiler has decided the bytes are even its own. +func TestProbeFromMerge_DeclinesAnAliasThatResolvedToNothing(t *testing.T) { + t.Parallel() + probe, err := probeFromMerge(&yaml.Node{Kind: yaml.AliasNode}, maxMergeDepth) + require.NoError(t, err) + assert.Empty(t, probe.OpenAPI) +} diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index 80d7a0ec..b8216407 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -13,6 +13,7 @@ package diag import ( "fmt" "strings" + "unicode/utf8" "github.com/dexpace/morphic/ir" ) @@ -315,8 +316,25 @@ func HasError(diags []ir.Diagnostic) bool { return ir.HasError(diags) } +// MaxQuotedErrorBytes bounds what a foreign error contributes to a diagnostic +// message. A diagnostic is read by a person and stored by a log, and an error +// raised by a library obeys neither: yaml.v3 reports a duplicated mapping key +// once per prior occurrence of it, so a 32 KB source repeating one key 6,553 +// times raises an error of 1.2 GB. Quoting that whole is not a report. +// +// The cap is generous because the errors worth quoting are lists — the overlay +// validator writes one sentence per finding — and a list cut to its first entry +// says less than the reader came for. What a cut costs is the tail; what it +// buys is that a message is always a message. +const MaxQuotedErrorBytes = 4 << 10 + +// elidedMarker ends a message the cap cut. It carries no count: a marker whose +// text depends on how much was dropped makes the message depend on the whole +// error again, which is the dependency the cap exists to remove. +const elidedMarker = "… (elided)" + // OneLine collapses err's text onto a single line, for a diagnostic that carries -// an error raised by something else. +// an error raised by something else, and cuts it at MaxQuotedErrorBytes. // // A diagnostic is rendered one per line, so an embedded newline splits one // report into several — and every line after the first carries no severity, code @@ -328,9 +346,15 @@ func HasError(diags []ir.Diagnostic) bool { // Parts are joined with "; " so a flat list reads as a list, except after a part // that already ends in a colon, where the next line is that header's content and // a semicolon would read as a break in it. +// +// The scan stops at the cap rather than trimming afterwards, so the work is +// bounded by what is kept and not by what the library wrote. func OneLine(err error) string { var out strings.Builder - for _, line := range strings.Split(err.Error(), "\n") { + for rest := err.Error(); rest != "" && out.Len() < MaxQuotedErrorBytes; { + var line string + line, rest, _ = strings.Cut(rest, "\n") + part := strings.Join(strings.Fields(line), " ") if part == "" { continue @@ -344,5 +368,22 @@ func OneLine(err error) string { } out.WriteString(part) } - return out.String() + return cutToCap(out.String()) +} + +// cutToCap returns msg bounded by MaxQuotedErrorBytes, marked when it cut. +// +// The cut lands on a rune boundary. The bytes are a foreign library's and may be +// multi-byte, and half a rune in a diagnostic is ill-formed text put in front of +// a reader — the one thing a report must not do, and the reason checkDiagnostics +// refuses to quote invalid UTF-8 back at all. +func cutToCap(msg string) string { + if len(msg) <= MaxQuotedErrorBytes { + return msg + } + cut := MaxQuotedErrorBytes + for cut > 0 && !utf8.RuneStart(msg[cut]) { + cut-- + } + return msg[:cut] + elidedMarker } diff --git a/compilers/openapi/internal/diag/diag_test.go b/compilers/openapi/internal/diag/diag_test.go index 76fec0c9..5697c2bd 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -174,7 +174,10 @@ func TestCodes_MatchTheDeclaredSet(t *testing.T) { } // declaredCodeCount returns how many exported string constants the package -// declares, read from its own source. +// declares, read from its own source. A code is a string, so an exported +// constant of any other kind — MaxQuotedErrorBytes is one — is not one and is +// not counted; the kind is read off the declaration rather than the name, so a +// code added here is counted whatever it is called. // // It is parsed rather than written down because a maintained count is exactly // the claim that rots silently: a code added without touching this file would @@ -204,7 +207,8 @@ func declaredCodeCount(t *testing.T) int { return n } -// constNamesIn returns how many exported names decl declares as constants. +// constNamesIn returns how many exported names decl declares as string +// constants. func constNamesIn(decl ast.Decl) int { gen, ok := decl.(*ast.GenDecl) if !ok || gen.Tok != token.CONST { @@ -216,8 +220,8 @@ func constNamesIn(decl ast.Decl) int { if !ok { continue } - for _, name := range vs.Names { - if name.IsExported() { + for i, name := range vs.Names { + if name.IsExported() && isStringLiteral(vs, i) { n++ } } @@ -225,6 +229,17 @@ func constNamesIn(decl ast.Decl) int { return n } +// isStringLiteral reports whether the i'th name of vs is bound to a string +// literal. A ValueSpec with no values at position i is an iota-style or repeated +// declaration, which no code in this package uses and which names no string. +func isStringLiteral(vs *ast.ValueSpec, i int) bool { + if i >= len(vs.Values) { + return false + } + lit, ok := vs.Values[i].(*ast.BasicLit) + return ok && lit.Kind == token.STRING +} + // TestOneLine_CollapsesWhatALibraryWrote pins both join rules and the reason for // each: a flat list of findings reads as a list, while a header that ends in a // colon owns the line after it and must not be cut from it by a semicolon. @@ -251,3 +266,56 @@ func TestOneLine_CollapsesWhatALibraryWrote(t *testing.T) { }) } } + +// TestOneLine_BoundsWhatALibraryWrote pins the cap. A diagnostic message is +// something a person reads and something a log stores, and neither survives an +// unbounded one: yaml.v3 reports a duplicated mapping key once per prior +// occurrence, so a 32 KB source with one key repeated 6,553 times produces a +// 1.2 GB error string, which this used to copy whole into a message the CLI +// then printed. +func TestOneLine_BoundsWhatALibraryWrote(t *testing.T) { + t.Parallel() + huge := errors.New(strings.Repeat("a line of complaint\n", 1<<16)) + got := diag.OneLine(huge) + + assert.Less(t, len(got), diag.MaxQuotedErrorBytes+64, + "a foreign error may be any size; what it contributes to a message may not") + assert.True(t, strings.HasPrefix(got, "a line of complaint; a line of complaint"), + "the cut keeps the head, which is the part that says what went wrong") + assert.Contains(t, got, "elided", "a cut message says it was cut") +} + +// TestOneLine_CutsOnARuneBoundary holds the cut to well-formed output. The bytes +// being quoted are a foreign library's and may be multi-byte; cutting one in +// half would put ill-formed UTF-8 into a diagnostic, which is the one thing a +// report must never do to a reader. +func TestOneLine_CutsOnARuneBoundary(t *testing.T) { + t.Parallel() + for pad := range 8 { + got := diag.OneLine(errors.New(strings.Repeat("x", pad) + strings.Repeat("é", diag.MaxQuotedErrorBytes))) + assert.True(t, utf8.ValidString(got), "pad %d: a cut message is still text", pad) + } +} + +// TestOneLine_IsBoundedInWorkNotOnlyOutput holds the cap to being a bound on +// work. A message capped by collapsing the whole error and trimming the result +// still walks the whole error, which is the half that costs the time: the 1.2 GB +// case spent 7.4 s building the parts it was about to throw away. +// +// Allocation count is the probe because the per-line work is what allocates — +// one strings.Fields join per line — so a scan that stops at the cap allocates +// the same for two errors that both exceed it, and one that does not scales with +// the error. It is not run in parallel: AllocsPerRun measures the process. +func TestOneLine_IsBoundedInWorkNotOnlyOutput(t *testing.T) { + small := errors.New(strings.Repeat("line\n", 1<<10)) + large := errors.New(strings.Repeat("line\n", 1<<20)) + require.Greater(t, len(small.Error()), diag.MaxQuotedErrorBytes, + "both inputs must exceed the cap, or the comparison is between two uncapped runs") + + assert.Equal(t, diag.OneLine(small), diag.OneLine(large), + "past the cap the answer no longer depends on how much more there was") + assert.Equal(t, + testing.AllocsPerRun(2, func() { _ = diag.OneLine(small) }), + testing.AllocsPerRun(2, func() { _ = diag.OneLine(large) }), + "past the cap the work no longer depends on how much more there was") +} diff --git a/internal/archtest/recursion_test.go b/internal/archtest/recursion_test.go index 28c78744..07d709cc 100644 --- a/internal/archtest/recursion_test.go +++ b/internal/archtest/recursion_test.go @@ -55,6 +55,10 @@ var loweringRecursions = [][]string{ // refuses past maxDynamicAnchorDepth or a spent node budget and records the // refusal, so a caller learns the index is partial. {"anchorWalk.walk", "anchorWalk.walkMapping"}, + // Format detection's read of a root mapping's merge keys. A `<<` may name a + // mapping that merges another, so following one is following a chain the + // document's size does not bound. Bounded by maxMergeDepth. + {"probeFromMapping", "probeFromMerge"}, // Property lookup through a composition. Finding a property by wire name // descends into a model's base and mixins, each of which is a model whose // properties are looked up the same way. From e7bc50612efa77e0de3837da3b2deb65c6687ca6 Mon Sep 17 00:00:00 2001 From: Fuad Daoud Date: Sat, 5 Sep 2026 20:04:39 +0300 Subject: [PATCH 3/4] fix(compilers/openapi): keep a conflicting redeclaration's losing type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1 --- compilers/openapi/conformance_test.go | 1 + .../openapi/conformance_unmodeled_test.go | 42 ++++ compilers/openapi/internal/diag/diag.go | 6 + compilers/openapi/internal/merge/merge.go | 53 ++++- .../internal/merge/reconcile_internal_test.go | 106 +++++++++ .../openapi/internal/schema/compose_test.go | 10 + .../allof-conflicting-type.golden.json | 215 ++++++++++++++++++ .../openapi/allof-conflicting-type.yaml | 26 +++ 8 files changed, 458 insertions(+), 1 deletion(-) create mode 100644 testdata/conformance/openapi/allof-conflicting-type.golden.json create mode 100644 testdata/conformance/openapi/allof-conflicting-type.yaml diff --git a/compilers/openapi/conformance_test.go b/compilers/openapi/conformance_test.go index bc05c2e9..327a768a 100644 --- a/compilers/openapi/conformance_test.go +++ b/compilers/openapi/conformance_test.go @@ -170,6 +170,7 @@ func conformanceCases() []conformanceCase { {"allof-inline-residue", assertAllOfInlineResidue, []string{"intersection"}}, {"allof-ref-branch-siblings", assertAllOfRefBranchSiblings, []string{"intersection", "untagged-unions"}}, {"allof-boolean-branch", assertAllOfBooleanBranch, []string{"intersection"}}, + {"allof-conflicting-type", assertAllOfConflictingType, nil}, {"oneof-discriminated", assertOneOfDiscriminated, []string{"tagged-unions"}}, {"discriminator-inheritance", assertDiscriminatorInheritance, []string{"tagged-unions", "inheritance"}}, {"discriminator-default-mapping", assertDiscriminatorDefaultMapping, []string{"tagged-unions"}}, diff --git a/compilers/openapi/conformance_unmodeled_test.go b/compilers/openapi/conformance_unmodeled_test.go index 9ec4d106..10ec29fb 100644 --- a/compilers/openapi/conformance_unmodeled_test.go +++ b/compilers/openapi/conformance_unmodeled_test.go @@ -251,6 +251,48 @@ func assertAllOfInlineResidue(t *testing.T, doc *ir.Document, diags []ir.Diagnos "a branch excluding object contradicts the composed model and is a warning") } +// assertAllOfConflictingType pins what an unsatisfiable redeclaration leaves in +// the document. allOf is an intersection, so a field one branch types `uri` and +// another types `string` describes a shape the IR has no combinator for: the +// merge keeps the first declaration and, under ir-design §4.8, keeps the loser +// verbatim beside it rather than dropping it (GitHub #424). +// +// The diagnostic is not what is being checked here. A consumer that diffs two +// revisions of a document reads the document, and before this entry existed a +// release in which the losing branch's type changed showed no change at all. +// +// The nullable case is the second half: a redeclaration says both what a field +// is and whether it admits null, so an entry keeping only the target ID would +// still be losing half of what it claims to preserve. +func assertAllOfConflictingType(t *testing.T, doc *ir.Document, diags []ir.Diagnostic) { + repo, ok := doc.Types[namedID("Repository")].(*ir.Model) + require.True(t, ok) + clone, ok := propByWire(repo, "clone_url") + require.True(t, ok, "the two declarations still reconcile to one property") + assert.Equal(t, ir.TypeID("t/prim/url"), clone.Type.Target, "the first declaration wins the shape") + + entry := unmodeledEntry(t, clone.Unmodeled, + "openapi:conflicting-redeclaration/components/schemas/Repository/allOf/1/properties/clone_url") + assert.Equal(t, ir.ReasonDegradedLowering, entry.Reason) + assert.JSONEq(t, `{"target":"t/prim/string","nullable":false}`, string(entry.Value)) + assert.Equal(t, "/components/schemas/Repository/allOf/1/properties/clone_url", entry.Provenance.Pointer, + "the entry locates the losing declaration, not the merged property") + assert.Equal(t, []ir.Severity{ir.SeverityWarning}, + diagsAt(diags, "openapi/conflicting-redeclaration", + "/components/schemas/Repository/allOf/1/properties/clone_url"), + "and the conflict is still reported") + + identified, ok := doc.Types[namedID("Identified")].(*ir.Model) + require.True(t, ok) + id, ok := propByWire(identified, "id") + require.True(t, ok) + assert.Equal(t, ir.TypeID("t/prim/integer"), id.Type.Target) + entry = unmodeledEntry(t, id.Unmodeled, + "openapi:conflicting-redeclaration/components/schemas/Identified/allOf/1/properties/id") + assert.JSONEq(t, `{"target":"t/prim/string","nullable":true}`, string(entry.Value), + "a nullable loser keeps its nullability, which the target ID alone would drop") +} + // assertAllOfRefBranchSiblings covers the other branch kind: keywords written // beside a `$ref` in an allOf branch bind that branch, not the schema it names, // so they cannot go on the shared target's node. The branch position gets a node diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index b8216407..a0374270 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -145,6 +145,12 @@ const ( // representable by a simple merge, so the merge keeps an arbitrary // source-order winner — possibly the looser bound — and surfaces the // disagreement instead of silently discarding it. + // + // The type half additionally keeps the losing reference under the merged + // property's Unmodeled, so it survives for a consumer reading the document + // rather than the diagnostic stream (merge.keepLosingType). The constraint + // half does not: the recorded direction there is to intersect the bounds + // (GitHub #10), so this code alone still carries it. ConflictingRedecl = "openapi/conflicting-redeclaration" // DisjointVisibility reports one field restricted to lifecycle sets that // share nothing — readOnly against writeOnly — so no lifecycle admits it at diff --git a/compilers/openapi/internal/merge/merge.go b/compilers/openapi/internal/merge/merge.go index 36ad0afc..373a4866 100644 --- a/compilers/openapi/internal/merge/merge.go +++ b/compilers/openapi/internal/merge/merge.go @@ -9,6 +9,7 @@ package merge import ( "cmp" + "encoding/json" "fmt" "slices" @@ -72,7 +73,10 @@ func (g *Merger) MergeProperty(m *ir.Model, byWire map[string]int, p ir.Property // differs between branches, an incompatible type, or a contradictory constraint // keyword are genuine conflicts the merge cannot represent (see // diag.ConflictingRedecl); each is diagnosed before any detail is folded in, -// rather than silently picking an arbitrary winner. +// rather than silently picking an arbitrary winner. The incompatible type is +// additionally kept beside the winner under Unmodeled (keepLosingType), so the +// discarded declaration survives in the document and not only in the diagnostic +// stream. func (g *Merger) reconcileProperty(dst *ir.Property, src ir.Property, pointer string) { g.diagnoseRedeclarationConflict(dst, &src, pointer) @@ -239,6 +243,7 @@ const maxTypeResolveDepth = 64 // subsumes any constraint conflict. func (g *Merger) diagnoseRedeclarationConflict(dst, src *ir.Property, pointer string) { if g.typesConflict(dst.Type, src.Type) { + keepLosingType(dst, src, pointer) g.redeclarationConflictDiag(dst, pointer, fmt.Sprintf("incompatible types %s and %s", dst.Type.Target, src.Type.Target)) return @@ -248,6 +253,52 @@ func (g *Merger) diagnoseRedeclarationConflict(dst, src *ir.Property, pointer st } } +// losingTypeKey prefixes the Unmodeled entry a discarded redeclaration type is +// kept under. The "openapi:" namespace is what keeps two source formats' keys +// from colliding on one node (ir-design §12); the redeclaration's own pointer +// completes it, the way an allOf branch's index completes the composition's +// keys. +const losingTypeKey = "openapi:conflicting-redeclaration" + +// keepLosingType keeps the redeclaration's discarded type beside the merged +// property, so a consumer reading the document rather than the diagnostic stream +// can still see what the losing declaration said (GitHub #424). Every other +// degradation in this compiler preserves what it could not model; this path was +// the exception. +// +// ReasonDegradedLowering is the reason: the IR has no combinator for "string +// here, integer there", so the pair is lowered to the weaker shape of the first +// declaration with the original kept beside it, which is that reason's own +// definition (ir-design §4.8). Not ReasonNoIRHome — the position has a field and +// it is holding the winner, so nothing is waiting on an IR gap to close; not +// ReasonValidationOnly, since a type is data shape rather than validation. +// +// The value is the discarded ir.TypeRef rather than the branch's source schema: +// this package never sees the document by design (see the package comment), and +// the reference is the whole of what was dropped — Nullable included, which the +// target ID alone would lose. Writing back an IR value already read rather than +// re-reading a raw node is what annotation's redundant-bound preservation does +// too. +// +// The redeclaration's pointer is part of the key rather than only of the +// provenance, so a field three branches type three incompatible ways keeps all +// three entries; a fixed key would leave whichever branch ran last. Provenance +// locates the losing declaration itself, which is the entry's own position and +// not the merged property's. +// +// Only the type is kept here. A constraint conflict discards the redeclaration's +// keyword too, but the recorded direction for that is to intersect the bounds so +// the merged field satisfies both branches (GitHub #10), and preserving the +// loser instead would settle a decision that already has one. +func keepLosingType(dst, src *ir.Property, pointer string) { + // A TypeID string and a bool: json.Marshal fails on neither, and rewrites + // ill-formed UTF-8 rather than refusing it, so the error it declares is + // discarded the way annotation.jsonString discards it for a key. + raw, _ := json.Marshal(src.Type) + annotation.PreserveInto(&dst.Unmodeled, losingTypeKey+pointer, raw, + ir.ReasonDegradedLowering, pointer, src.Provenance.Source) +} + // redeclarationConflictDiag emits the shared conflicting-redeclaration warning, // naming the field and both declaration sites (dst's own, and the redeclaration // at pointer). detail is the caller-formatted disagreement — two type IDs, or a diff --git a/compilers/openapi/internal/merge/reconcile_internal_test.go b/compilers/openapi/internal/merge/reconcile_internal_test.go index e9a88eea..cce93752 100644 --- a/compilers/openapi/internal/merge/reconcile_internal_test.go +++ b/compilers/openapi/internal/merge/reconcile_internal_test.go @@ -392,3 +392,109 @@ func TestDifferentTypeKind_ComparesResolvedKinds(t *testing.T) { assert.True(t, g.typesConflict(ir.TypeRef{Target: "t/opaque"}, ir.TypeRef{Target: "t/union"}), "and typesConflict reaches that comparison when neither side is a primitive") } + +// TestKeepLosingType_RecordsTheDiscardedDeclaration pins what the fix for +// GitHub #424 adds: the redeclaration whose type loses is kept verbatim beside +// the winner, so a consumer reading the document — not the diagnostic stream — +// still sees what the second declaration said. +// +// The reference is asserted whole rather than by its target alone: Nullable is +// half of what a redeclaration says about a field, and an entry that kept only +// the ID would lose it while still looking like a preservation. +func TestKeepLosingType_RecordsTheDiscardedDeclaration(t *testing.T) { + t.Parallel() + g, recorded := stubMerger(map[ir.TypeID]ir.TypeDef{ + "t/str": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/str"}, Prim: ir.PrimString}, + "t/int": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/int"}, Prim: ir.PrimInt32}, + }) + dst := ir.Property{ + WireName: "id", + Type: ir.TypeRef{Target: "t/str"}, + Provenance: ir.Provenance{Source: 2, Pointer: "/components/schemas/S/allOf/0/properties/id"}, + } + src := ir.Property{ + WireName: "id", + Type: ir.TypeRef{Target: "t/int", Nullable: true}, + Provenance: ir.Provenance{Source: 2, Pointer: "/components/schemas/S/allOf/1/properties/id"}, + } + + g.reconcileProperty(&dst, src, "/components/schemas/S/allOf/1/properties/id") + + require.Len(t, *recorded, 1, "the conflict is still diagnosed") + assert.Equal(t, ir.TypeID("t/str"), dst.Type.Target, "the first declaration still wins the shape") + + key := "openapi:conflicting-redeclaration/components/schemas/S/allOf/1/properties/id" + entry, ok := dst.Unmodeled[key] + require.True(t, ok, "the losing declaration is kept under a pointer-namespaced key; got %v", dst.Unmodeled) + assert.Equal(t, ir.ReasonDegradedLowering, entry.Reason) + assert.JSONEq(t, `{"target":"t/int","nullable":true}`, string(entry.Value), + "the whole reference is kept, nullability included") + assert.Equal(t, ir.Provenance{Source: 2, Pointer: "/components/schemas/S/allOf/1/properties/id"}, + entry.Provenance, "the entry locates the losing declaration, not the merged property") +} + +// TestKeepLosingType_EveryLoserSurvivesItsSiblings pins the key's namespacing. +// A field three branches type three incompatible ways discards two +// declarations, and a fixed key would leave only whichever branch ran last — +// the same silent overwrite the entry exists to prevent. +func TestKeepLosingType_EveryLoserSurvivesItsSiblings(t *testing.T) { + t.Parallel() + g, _ := stubMerger(map[ir.TypeID]ir.TypeDef{ + "t/str": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/str"}, Prim: ir.PrimString}, + "t/int": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/int"}, Prim: ir.PrimInt32}, + "t/bool": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/bool"}, Prim: ir.PrimBool}, + }) + m := &ir.Model{} + byWire := WireNameIndex(m.Properties) + + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}}, "/a") + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/int"}}, "/b") + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/bool"}}, "/c") + + require.Len(t, m.Properties, 1, "three declarations still reconcile to one property") + assert.Equal(t, ir.Unmodeled{ + "openapi:conflicting-redeclaration/b": { + Reason: ir.ReasonDegradedLowering, + Value: ir.RawValue(`{"target":"t/int","nullable":false}`), + Provenance: ir.Provenance{Pointer: "/b"}, + }, + "openapi:conflicting-redeclaration/c": { + Reason: ir.ReasonDegradedLowering, + Value: ir.RawValue(`{"target":"t/bool","nullable":false}`), + Provenance: ir.Provenance{Pointer: "/c"}, + }, + }, m.Properties[0].Unmodeled) +} + +// TestKeepLosingType_LeavesAgreeingAndConstraintOnlyConflictsAlone pins the two +// sides the entry must not appear on. Agreeing declarations discard nothing, so +// an entry would be noise; a constraint conflict does discard the +// redeclaration's keyword, but the recorded direction there is to intersect the +// bounds rather than preserve the loser (GitHub #10), so preserving it here +// would settle a decision that already has one. +func TestKeepLosingType_LeavesAgreeingAndConstraintOnlyConflictsAlone(t *testing.T) { + t.Parallel() + g, recorded := stubMerger(map[ir.TypeID]ir.TypeDef{ + "t/str": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/str"}, Prim: ir.PrimString}, + }) + ten, twenty := int64(10), int64(20) + agreeing := ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}} + + g.reconcileProperty(&agreeing, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}}, "/b") + + assert.Empty(t, *recorded, "agreeing declarations are not a conflict") + assert.Empty(t, agreeing.Unmodeled, "and nothing was discarded to keep") + + bounded := ir.Property{ + WireName: "code", Type: ir.TypeRef{Target: "t/str"}, + Constraints: &ir.Constraints{MaxLength: &ten}, + } + g.reconcileProperty(&bounded, ir.Property{ + WireName: "code", Type: ir.TypeRef{Target: "t/str"}, + Constraints: &ir.Constraints{MaxLength: &twenty}, + }, "/b") + + require.Len(t, *recorded, 1, "the constraint conflict is still diagnosed") + assert.Equal(t, diag.ConflictingRedecl, (*recorded)[0].Code) + assert.Empty(t, bounded.Unmodeled, "a constraint conflict keeps no type entry") +} diff --git a/compilers/openapi/internal/schema/compose_test.go b/compilers/openapi/internal/schema/compose_test.go index 28810922..1f626e0b 100644 --- a/compilers/openapi/internal/schema/compose_test.go +++ b/compilers/openapi/internal/schema/compose_test.go @@ -180,6 +180,16 @@ func TestAllOf_ConflictingRedeclaredTypeDiagnosed(t *testing.T) { assert.Contains(t, d.Message, `"id"`, "the diagnostic names the conflicting field") assert.Contains(t, d.Message, "allOf/0", "the diagnostic names the first branch site") assert.Contains(t, d.Message, "allOf/1", "the diagnostic names the second branch site") + + // A diagnostic is not part of the document, so the losing declaration is + // also kept where a consumer reading the IR will reach it (GitHub #424). + lost := m.Properties[0].Unmodeled["openapi:conflicting-redeclaration"+ + "/components/schemas/Conflictish/allOf/1/properties/id"] + assert.Equal(t, ir.ReasonDegradedLowering, lost.Reason, + "the discarded type is kept beside the winner; got %v", m.Properties[0].Unmodeled) + assert.JSONEq(t, `{"target":"t/prim/integer","nullable":false}`, string(lost.Value), + "and it names the type the second branch declared, not the one that won") + assert.Equal(t, "/components/schemas/Conflictish/allOf/1/properties/id", lost.Provenance.Pointer) } func TestAllOf_ConflictingRedeclaredConstraintDiagnosed(t *testing.T) { diff --git a/testdata/conformance/openapi/allof-conflicting-type.golden.json b/testdata/conformance/openapi/allof-conflicting-type.golden.json new file mode 100644 index 00000000..65e7f546 --- /dev/null +++ b/testdata/conformance/openapi/allof-conflicting-type.golden.json @@ -0,0 +1,215 @@ +{ + "irVersion": "0.3.0", + "name": "AllOfConflictingType", + "version": "1.0.0", + "docs": {}, + "services": [ + { + "id": "s/openapi/0", + "name": { + "source": "AllOfConflictingType", + "canonical": "all_of_conflicting_type" + }, + "docs": {}, + "auth": null, + "provenance": { + "source": 0 + } + } + ], + "types": { + "t/openapi/components/schemas/Identified": { + "kind": "model", + "id": "t/openapi/components/schemas/Identified", + "name": { + "source": "Identified", + "canonical": "identified" + }, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0, + "pointer": "/components/schemas/Identified" + }, + "properties": [ + { + "id": "p/openapi/components/schemas/Identified/allOf/0/properties/id", + "name": { + "source": "id", + "canonical": "id" + }, + "wireName": "id", + "type": { + "target": "t/prim/integer", + "nullable": false + }, + "required": false, + "clientOptional": false, + "defaultAdded": false, + "visibility": { + "none": false + }, + "flatten": false, + "eventHeader": false, + "eventPayload": false, + "secret": false, + "docs": {}, + "unmodeled": { + "openapi:conflicting-redeclaration/components/schemas/Identified/allOf/1/properties/id": { + "reason": "degraded_lowering", + "value": { + "target": "t/prim/string", + "nullable": true + }, + "provenance": { + "source": 0, + "pointer": "/components/schemas/Identified/allOf/1/properties/id" + } + } + }, + "provenance": { + "source": 0, + "pointer": "/components/schemas/Identified/allOf/0/properties/id" + } + } + ], + "abstract": false, + "positional": false, + "inputOnly": false + }, + "t/openapi/components/schemas/Repository": { + "kind": "model", + "id": "t/openapi/components/schemas/Repository", + "name": { + "source": "Repository", + "canonical": "repository" + }, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0, + "pointer": "/components/schemas/Repository" + }, + "properties": [ + { + "id": "p/openapi/components/schemas/Repository/allOf/0/properties/clone_url", + "name": { + "source": "clone_url", + "canonical": "clone_url" + }, + "wireName": "clone_url", + "type": { + "target": "t/prim/url", + "nullable": false + }, + "required": false, + "clientOptional": false, + "defaultAdded": false, + "visibility": { + "none": false + }, + "flatten": false, + "eventHeader": false, + "eventPayload": false, + "secret": false, + "docs": {}, + "unmodeled": { + "openapi:conflicting-redeclaration/components/schemas/Repository/allOf/1/properties/clone_url": { + "reason": "degraded_lowering", + "value": { + "target": "t/prim/string", + "nullable": false + }, + "provenance": { + "source": 0, + "pointer": "/components/schemas/Repository/allOf/1/properties/clone_url" + } + } + }, + "provenance": { + "source": 0, + "pointer": "/components/schemas/Repository/allOf/0/properties/clone_url" + } + } + ], + "abstract": false, + "positional": false, + "inputOnly": false + }, + "t/prim/integer": { + "kind": "primitive", + "id": "t/prim/integer", + "name": {}, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0 + }, + "prim": "integer" + }, + "t/prim/string": { + "kind": "primitive", + "id": "t/prim/string", + "name": {}, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0 + }, + "prim": "string" + }, + "t/prim/url": { + "kind": "primitive", + "id": "t/prim/url", + "name": {}, + "anonymous": false, + "docs": {}, + "sensitive": false, + "provenance": { + "source": 0 + }, + "prim": "url" + } + }, + "servers": [ + { + "name": { + "hint": "server" + }, + "urlTemplate": "/", + "description": {}, + "auth": null + } + ], + "diagnostics": [ + { + "severity": "warning", + "code": "openapi/conflicting-redeclaration", + "message": "declarations of field \"clone_url\" disagree: incompatible types t/prim/url and t/prim/string; kept the first declaration (/components/schemas/Repository/allOf/0/properties/clone_url) over the redeclaration (/components/schemas/Repository/allOf/1/properties/clone_url)", + "provenance": { + "source": 0, + "pointer": "/components/schemas/Repository/allOf/1/properties/clone_url" + } + }, + { + "severity": "warning", + "code": "openapi/conflicting-redeclaration", + "message": "declarations of field \"id\" disagree: incompatible types t/prim/integer and t/prim/string; kept the first declaration (/components/schemas/Identified/allOf/0/properties/id) over the redeclaration (/components/schemas/Identified/allOf/1/properties/id)", + "provenance": { + "source": 0, + "pointer": "/components/schemas/Identified/allOf/1/properties/id" + } + } + ], + "sources": [ + { + "format": "openapi@3.1", + "path": "allof-conflicting-type.yaml", + "hash": "6b80f51a048c29fc5a1adb6b092114770c07b37140485b92da702d4cc309a3df" + } + ] +} diff --git a/testdata/conformance/openapi/allof-conflicting-type.yaml b/testdata/conformance/openapi/allof-conflicting-type.yaml new file mode 100644 index 00000000..0ad1c0b5 --- /dev/null +++ b/testdata/conformance/openapi/allof-conflicting-type.yaml @@ -0,0 +1,26 @@ +openapi: 3.1.0 +info: {title: AllOfConflictingType, version: "1.0.0"} +paths: {} +components: + schemas: + # The shape GitHub's own spec writes 102 times: one branch narrows a string + # with a format, the other leaves it bare, and the two lower to different + # primitives. The first declaration wins; the second is kept beside it. + Repository: + allOf: + - type: object + properties: + clone_url: {type: string, format: uri} + - type: object + properties: + clone_url: {type: string} + # A nullable loser, so the entry is held to keeping the whole reference + # rather than only the type it names. + Identified: + allOf: + - type: object + properties: + id: {type: integer} + - type: object + properties: + id: {type: [string, "null"]} From 3f2b70df22e66748680281af0a7bfea5bf8bbaf6 Mon Sep 17 00:00:00 2001 From: Fuad Daoud Date: Wed, 9 Sep 2026 23:56:16 +0300 Subject: [PATCH 4/4] fix(compilers/openapi): discard a losing redeclaration whole, and record every drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01R65r3qXXNM9jNbQu5gGj9v --- .../openapi/conformance_unmodeled_test.go | 18 +- compilers/openapi/internal/merge/merge.go | 118 ++++++++++--- .../internal/merge/merge_internal_test.go | 10 +- .../internal/merge/reconcile_internal_test.go | 157 +++++++++++++++--- .../openapi/internal/schema/compose_test.go | 7 +- compilers/openapi/internal/schema/schema.go | 2 +- docs/ir-design.md | 23 ++- .../allof-conflicting-type.golden.json | 4 +- 8 files changed, 277 insertions(+), 62 deletions(-) diff --git a/compilers/openapi/conformance_unmodeled_test.go b/compilers/openapi/conformance_unmodeled_test.go index 10ec29fb..b5af0493 100644 --- a/compilers/openapi/conformance_unmodeled_test.go +++ b/compilers/openapi/conformance_unmodeled_test.go @@ -271,11 +271,10 @@ func assertAllOfConflictingType(t *testing.T, doc *ir.Document, diags []ir.Diagn require.True(t, ok, "the two declarations still reconcile to one property") assert.Equal(t, ir.TypeID("t/prim/url"), clone.Type.Target, "the first declaration wins the shape") - entry := unmodeledEntry(t, clone.Unmodeled, - "openapi:conflicting-redeclaration/components/schemas/Repository/allOf/1/properties/clone_url") - assert.Equal(t, ir.ReasonDegradedLowering, entry.Reason) - assert.JSONEq(t, `{"target":"t/prim/string","nullable":false}`, string(entry.Value)) - assert.Equal(t, "/components/schemas/Repository/allOf/1/properties/clone_url", entry.Provenance.Pointer, + const cloneKey = "openapi:conflicting-redeclaration/components/schemas/Repository/allOf/1/properties/clone_url" + assertKeptRaw(t, clone.Unmodeled, cloneKey, `{"target":"t/prim/string","nullable":false}`) + assert.Equal(t, "/components/schemas/Repository/allOf/1/properties/clone_url", + unmodeledEntry(t, clone.Unmodeled, cloneKey).Provenance.Pointer, "the entry locates the losing declaration, not the merged property") assert.Equal(t, []ir.Severity{ir.SeverityWarning}, diagsAt(diags, "openapi/conflicting-redeclaration", @@ -287,10 +286,11 @@ func assertAllOfConflictingType(t *testing.T, doc *ir.Document, diags []ir.Diagn id, ok := propByWire(identified, "id") require.True(t, ok) assert.Equal(t, ir.TypeID("t/prim/integer"), id.Type.Target) - entry = unmodeledEntry(t, id.Unmodeled, - "openapi:conflicting-redeclaration/components/schemas/Identified/allOf/1/properties/id") - assert.JSONEq(t, `{"target":"t/prim/string","nullable":true}`, string(entry.Value), - "a nullable loser keeps its nullability, which the target ID alone would drop") + // Held to the same check as the first: the Reason assertion above was not + // repeated here, so two cases in one fixture were not equally pinned. + assertKeptRaw(t, id.Unmodeled, + "openapi:conflicting-redeclaration/components/schemas/Identified/allOf/1/properties/id", + `{"target":"t/prim/string","nullable":true}`) } // assertAllOfRefBranchSiblings covers the other branch kind: keywords written diff --git a/compilers/openapi/internal/merge/merge.go b/compilers/openapi/internal/merge/merge.go index 373a4866..7d162411 100644 --- a/compilers/openapi/internal/merge/merge.go +++ b/compilers/openapi/internal/merge/merge.go @@ -48,9 +48,14 @@ func WireNameIndex(props []ir.Property) map[string]int { // (and properties co-declared alongside allOf) redeclare one logical field under // allOf's intersection semantics (ir-design §4.3). Callers must set p.WireName // (fillModelProperties always does); it keys byWire directly. -func (g *Merger) MergeProperty(m *ir.Model, byWire map[string]int, p ir.Property, pointer string) { +// +// p.Provenance locates the declaration, and is the only source of that fact +// here. It used to arrive twice — as a parameter and on p — equal by +// construction at the one call site, which left nothing able to catch them +// disagreeing. +func (g *Merger) MergeProperty(m *ir.Model, byWire map[string]int, p ir.Property) { if i, ok := byWire[p.WireName]; ok { - g.reconcileProperty(&m.Properties[i], p, pointer) + g.reconcileProperty(&m.Properties[i], p) return } byWire[p.WireName] = len(m.Properties) @@ -77,8 +82,9 @@ func (g *Merger) MergeProperty(m *ir.Model, byWire map[string]int, p ir.Property // additionally kept beside the winner under Unmodeled (keepLosingType), so the // discarded declaration survives in the document and not only in the diagnostic // stream. -func (g *Merger) reconcileProperty(dst *ir.Property, src ir.Property, pointer string) { - g.diagnoseRedeclarationConflict(dst, &src, pointer) +func (g *Merger) reconcileProperty(dst *ir.Property, src ir.Property) { + pointer := src.Provenance.Pointer + discarded := g.recordRedeclarationConflict(dst, &src) dst.Required = dst.Required || src.Required dst.Secret = dst.Secret || src.Secret @@ -101,15 +107,25 @@ func (g *Merger) reconcileProperty(dst *ir.Property, src ir.Property, pointer st g.Report(ir.SeverityInfo, diag.DegradedConstruct, pointer, "allOf branches describe field %q differently; kept the first declaration", dst.WireName) } - dst.Default = cmp.Or(dst.Default, src.Default) - dst.Constraints = mergeConstraints(dst.Constraints, src.Constraints) + // Skipped when src's type was discarded as incompatible: default, + // constraints and examples describe the shape that lost, so folding them + // onto the winner makes the document assert two contradictory things about + // one field — an integer carrying a string default, beside an Unmodeled + // entry saying the string declaration was dropped. Nothing downstream + // compares a Value's kind to its property's type, so an emitter renders that + // pair into code that does not compile. Deprecation and XML are not shape + // bound and are adopted either way. + if !discarded { + dst.Default = cmp.Or(dst.Default, src.Default) + dst.Constraints = mergeConstraints(dst.Constraints, src.Constraints) + if len(dst.Examples) == 0 { + // Examples is a slice, not comparable, so it cannot go through + // cmp.Or like its neighbors; the len()==0 predicate is the rule. + dst.Examples = src.Examples + } + } dst.Deprecation = cmp.Or(dst.Deprecation, src.Deprecation) dst.XML = cmp.Or(dst.XML, src.XML) - if len(dst.Examples) == 0 { - // Examples is a slice, not comparable, so it cannot go through cmp.Or - // like its neighbors above; the len()==0 predicate is the adoption rule. - dst.Examples = src.Examples - } dst.Unmodeled = annotation.MergeUnmodeled(dst.Unmodeled, src.Unmodeled) } @@ -167,7 +183,7 @@ func mergeConstraints(dst, src *ir.Constraints) *ir.Constraints { // on the same field) intersect to ∅ too: every lifecycle is excluded. That is // recorded as None — the shape the IR already has for "invisible everywhere" // (TypeSpec's @invisible) — rather than raised through -// diagnoseRedeclarationConflict. Unlike an incompatible-type redeclaration, +// recordRedeclarationConflict. Unlike an incompatible-type redeclaration, // nothing here is arbitrarily discarded: ∅ is the exact intersection, not a // guess between two unrepresentable shapes. // @@ -233,24 +249,51 @@ func intersectLifecycles(a, b []ir.Lifecycle) []ir.Lifecycle { // guards a pathological or malformed registry. const maxTypeResolveDepth = 64 -// diagnoseRedeclarationConflict reports when redeclaration src contradicts dst -// under allOf intersection — an incompatible target type, or a constraint -// keyword both branches pin to different values — without altering the merge -// (dst keeps its shape). A type conflict is genuinely unsatisfiable; a +// recordRedeclarationConflict folds src's type into dst and reports what the +// fold could not represent: an incompatible target type, or a constraint keyword +// both branches pin to different values. It alters dst — intersecting +// nullability, and keeping a dropped type under Unmodeled — as well as +// diagnosing, which is why it is named for recording rather than for diagnosis. +// +// It returns whether src's type was discarded as incompatible, which is what +// tells reconcileProperty not to carry that shape's details onto the winner. +// +// Preservation is owed wherever a type is dropped, which is a wider set than the +// conflicts worth reporting: typesConflict deliberately does not guess about two +// composites of one kind, an unresolvable target, or the top type against +// anything, and in each of those dst keeps its own type while src's vanishes. +// Recording it there too is what stops a consumer diffing two versions from +// seeing no change (GitHub #424); the diagnostic stays on the narrower +// predicate, because "dropped" and "contradictory" are different claims. +// +// A type conflict is genuinely unsatisfiable; a // constraint conflict is usually satisfiable alone, but the merge can't // represent the true intersection and may keep the looser bound // (diag.ConflictingRedecl). At most one diagnostic fires: a type conflict // subsumes any constraint conflict. -func (g *Merger) diagnoseRedeclarationConflict(dst, src *ir.Property, pointer string) { +func (g *Merger) recordRedeclarationConflict(dst, src *ir.Property) bool { + pointer := src.Provenance.Pointer + if dst.Type.Target != src.Type.Target { + keepLosingType(dst, src) + } else { + // Same referent, and the only thing left for the two to disagree about + // is whether null is admitted. Under intersection it is admitted only + // where both branches admit it — the rule foldNullVerdicts already + // states for a single schema. Without this the answer came from + // whichever branch was written first, so reordering two allOf branches + // changed the merged field. + dst.Type.Nullable = dst.Type.Nullable && src.Type.Nullable + } if g.typesConflict(dst.Type, src.Type) { - keepLosingType(dst, src, pointer) g.redeclarationConflictDiag(dst, pointer, - fmt.Sprintf("incompatible types %s and %s", dst.Type.Target, src.Type.Target)) - return + fmt.Sprintf("incompatible types %s and %s (the redeclaration's type is kept verbatim under Unmodeled)", + dst.Type.Target, src.Type.Target)) + return true } if detail, ok := constraintsConflict(dst.Constraints, src.Constraints); ok { g.redeclarationConflictDiag(dst, pointer, detail) } + return false } // losingTypeKey prefixes the Unmodeled entry a discarded redeclaration type is @@ -262,9 +305,7 @@ const losingTypeKey = "openapi:conflicting-redeclaration" // keepLosingType keeps the redeclaration's discarded type beside the merged // property, so a consumer reading the document rather than the diagnostic stream -// can still see what the losing declaration said (GitHub #424). Every other -// degradation in this compiler preserves what it could not model; this path was -// the exception. +// can still see what the losing declaration said (GitHub #424). // // ReasonDegradedLowering is the reason: the IR has no combinator for "string // here, integer there", so the pair is lowered to the weaker shape of the first @@ -273,6 +314,16 @@ const losingTypeKey = "openapi:conflicting-redeclaration" // it is holding the winner, so nothing is waiting on an IR gap to close; not // ReasonValidationOnly, since a type is data shape rather than validation. // +// KNOWN GAP (GitHub #445): ir-design §12 defines an Unmodeled value as the +// source construct verbatim, and a TypeID is a compiler-minted registry ID +// rather than anything the document wrote. Two things follow — irverify's +// reference walk cannot see this reference dangle, since it reads []byte rather +// than a typed ref; and a losing branch that also carries residue is preserved +// twice, here and verbatim under openapi:allOf/. The losing property's raw +// node is still in scope at the MergeProperty call site and is what should be +// kept instead. Left as it is here because changing it moves MergeProperty's +// signature and the golden, where this change's scope is GitHub #424. +// // The value is the discarded ir.TypeRef rather than the branch's source schema: // this package never sees the document by design (see the package comment), and // the reference is the whole of what was dropped — Nullable included, which the @@ -284,13 +335,24 @@ const losingTypeKey = "openapi:conflicting-redeclaration" // provenance, so a field three branches type three incompatible ways keeps all // three entries; a fixed key would leave whichever branch ran last. Provenance // locates the losing declaration itself, which is the entry's own position and -// not the merged property's. +// not the merged property's. Both are read off src, so the key and the +// provenance cannot disagree about where the loser was written. +// +// A loser with no pointer, or no target, is not recorded: the key would collapse +// to the bare prefix, and PreserveInto is a plain overwrite, so a second such +// loser would silently replace the first. No production path produces either — +// ProvenanceAt always stamps the pointer it was given — which is why this is a +// guard rather than a diagnostic. // // Only the type is kept here. A constraint conflict discards the redeclaration's // keyword too, but the recorded direction for that is to intersect the bounds so // the merged field satisfies both branches (GitHub #10), and preserving the // loser instead would settle a decision that already has one. -func keepLosingType(dst, src *ir.Property, pointer string) { +func keepLosingType(dst, src *ir.Property) { + pointer := src.Provenance.Pointer + if pointer == "" || src.Type.Target == "" { + return + } // A TypeID string and a bool: json.Marshal fails on neither, and rewrites // ill-formed UTF-8 rather than refusing it, so the error it declares is // discarded the way annotation.jsonString discards it for a key. @@ -321,6 +383,12 @@ func (g *Merger) redeclarationConflictDiag(dst *ir.Property, pointer, detail str // Two distinct composite types of the same kind (two models, two lists) are not // provably contradictory, so they are never reported — conflict detection does // not guess. +// KNOWN GAP (GitHub #446): PrimKinds are compared for equality with no notion +// of narrowing, so {string, format: uri} against a bare {string} reads as a +// conflict though the intersection is exactly the url the merge keeps — nothing +// is lost, and both a diagnostic and a preserved entry claim otherwise. The same +// holds for every format-narrowing pair (date-time/string, int32/integer, +// double/number, uuid/string): 102 occurrences in the published GitHub spec. func (g *Merger) typesConflict(a, b ir.TypeRef) bool { if a.Target == b.Target || g.isAnyType(a) || g.isAnyType(b) { return false diff --git a/compilers/openapi/internal/merge/merge_internal_test.go b/compilers/openapi/internal/merge/merge_internal_test.go index 1fa17923..c6324632 100644 --- a/compilers/openapi/internal/merge/merge_internal_test.go +++ b/compilers/openapi/internal/merge/merge_internal_test.go @@ -17,6 +17,11 @@ import ( // previously meant standing up a compiler and feeding it a spec that happened to // produce the pair of declarations under test; the registry dependency is narrow // enough to pass as a function, so the lattice can be driven directly. +// ptrAt is the provenance a declaration at pointer carries. MergeProperty reads +// the position off the property rather than taking it alongside, so a test that +// merges one has to say where it was written. +func ptrAt(pointer string) ir.Provenance { return ir.Provenance{Pointer: pointer} } + func stubMerger(reg map[ir.TypeID]ir.TypeDef) (*Merger, *[]ir.Diagnostic) { recorded := &[]ir.Diagnostic{} g := &Merger{ @@ -59,9 +64,10 @@ func TestMerger_ReconcileReportsDisagreementAndKeepsAWinner(t *testing.T) { "t/int": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/int"}, Prim: ir.PrimInt32}, }) dst := ir.Property{Name: ir.Naming{Source: "id"}, WireName: "id", Type: ir.TypeRef{Target: "t/str"}} - src := ir.Property{Name: ir.Naming{Source: "id"}, WireName: "id", Type: ir.TypeRef{Target: "t/int"}} + src := ir.Property{Name: ir.Naming{Source: "id"}, WireName: "id", Type: ir.TypeRef{Target: "t/int"}, + Provenance: ptrAt("/components/schemas/S/properties/id")} - g.reconcileProperty(&dst, src, "/components/schemas/S/properties/id") + g.reconcileProperty(&dst, src) require.Len(t, *recorded, 1, "one disagreement, one diagnostic") d := (*recorded)[0] diff --git a/compilers/openapi/internal/merge/reconcile_internal_test.go b/compilers/openapi/internal/merge/reconcile_internal_test.go index cce93752..02b16ec9 100644 --- a/compilers/openapi/internal/merge/reconcile_internal_test.go +++ b/compilers/openapi/internal/merge/reconcile_internal_test.go @@ -38,13 +38,13 @@ func TestMergeProperty_AppendsThenFolds(t *testing.T) { m := &ir.Model{} byWire := WireNameIndex(m.Properties) - g.MergeProperty(m, byWire, ir.Property{WireName: "id", Required: true}, "/p") - g.MergeProperty(m, byWire, ir.Property{WireName: "name"}, "/p") + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Required: true, Provenance: ptrAt("/p")}) + g.MergeProperty(m, byWire, ir.Property{WireName: "name", Provenance: ptrAt("/p")}) require.Len(t, m.Properties, 2, "two distinct wire names are two properties") assert.Equal(t, map[string]int{"id": 0, "name": 1}, byWire) - g.MergeProperty(m, byWire, ir.Property{WireName: "id", Secret: true}, "/other") + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Secret: true, Provenance: ptrAt("/other")}) require.Len(t, m.Properties, 2, "a redeclaration folds rather than appending") assert.True(t, m.Properties[0].Required, "required is kept from the first declaration") @@ -61,7 +61,7 @@ func TestReconcileProperty_DifferingDescriptionsKeepTheFirst(t *testing.T) { g, recorded := stubMerger(nil) dst := ir.Property{WireName: "id", Docs: ir.Docs{Description: "first"}} - g.reconcileProperty(&dst, ir.Property{WireName: "id", Docs: ir.Docs{Description: "second"}}, "/other") + g.reconcileProperty(&dst, ir.Property{WireName: "id", Docs: ir.Docs{Description: "second"}, Provenance: ptrAt("/other")}) assert.Equal(t, "first", dst.Docs.Description) require.Len(t, *recorded, 1) @@ -78,7 +78,7 @@ func TestReconcileProperty_AnIdenticalDescriptionIsNotADisagreement(t *testing.T g, recorded := stubMerger(nil) dst := ir.Property{WireName: "id", Docs: ir.Docs{Description: "same"}} - g.reconcileProperty(&dst, ir.Property{WireName: "id", Docs: ir.Docs{Description: "same"}}, "/other") + g.reconcileProperty(&dst, ir.Property{WireName: "id", Docs: ir.Docs{Description: "same"}, Provenance: ptrAt("/other")}) assert.Equal(t, "same", dst.Docs.Description) assert.Empty(t, *recorded) @@ -212,7 +212,7 @@ func TestReconcileProperty_VisibilityAdoptedFromRedeclaration(t *testing.T) { g, _ := stubMerger(nil) dst := ir.Property{WireName: "id"} - g.reconcileProperty(&dst, ir.Property{WireName: "id", Visibility: readOnlyVisibility}, "/other") + g.reconcileProperty(&dst, ir.Property{WireName: "id", Visibility: readOnlyVisibility, Provenance: ptrAt("/other")}) assert.Equal(t, readOnlyVisibility, dst.Visibility) } @@ -221,7 +221,7 @@ func TestReconcileProperty_VisibilityAdoptedFromRedeclaration(t *testing.T) { // halves of the choice for an allOf pairing that admits no lifecycle at all // (readOnly against writeOnly on the same field). mergeVisibility represents it // exactly as None, so it is recorded rather than routed through -// diagnoseRedeclarationConflict — nothing is arbitrarily discarded, as it would +// recordRedeclarationConflict — nothing is arbitrarily discarded, as it would // be for an incompatible-type redeclaration. But exact is not unremarkable: a // field neither a request nor a response can carry is a composition that cannot // take effect, so it is warned about under its own code. @@ -230,7 +230,7 @@ func TestReconcileProperty_DisjointVisibilityIsARestrictionNotAConflict(t *testi g, recorded := stubMerger(nil) dst := ir.Property{WireName: "id", Visibility: readOnlyVisibility} - g.reconcileProperty(&dst, ir.Property{WireName: "id", Visibility: writeOnlyVisibility}, "/other") + g.reconcileProperty(&dst, ir.Property{WireName: "id", Visibility: writeOnlyVisibility, Provenance: ptrAt("/other")}) assert.Equal(t, ir.Visibility{None: true}, dst.Visibility) require.Len(t, *recorded, 1, "an emptied visibility set is reported, not passed over in silence") @@ -261,7 +261,7 @@ func TestReconcileProperty_AlreadyInvisibleVisibilityIsNotReAnnounced(t *testing g, recorded := stubMerger(nil) dst := ir.Property{WireName: "id", Visibility: tc.dst} - g.reconcileProperty(&dst, ir.Property{WireName: "id", Visibility: tc.src}, "/other") + g.reconcileProperty(&dst, ir.Property{WireName: "id", Visibility: tc.src, Provenance: ptrAt("/other")}) assert.Equal(t, ir.Visibility{None: true}, dst.Visibility) assert.Empty(t, *recorded, "None was already the answer; this merge announced nothing new") @@ -269,11 +269,11 @@ func TestReconcileProperty_AlreadyInvisibleVisibilityIsNotReAnnounced(t *testing } } -// TestDiagnoseRedeclarationConflict_ConstraintDisagreementIsReported pins the +// TestRecordRedeclarationConflict_ConstraintDisagreementIsReported pins the // second of the two conflict routes. Two branches that agree on type but pin one // keyword to different values produce a merge that keeps dst's bound, so the // stricter one is discarded and must be announced. -func TestDiagnoseRedeclarationConflict_ConstraintDisagreementIsReported(t *testing.T) { +func TestRecordRedeclarationConflict_ConstraintDisagreementIsReported(t *testing.T) { t.Parallel() g, recorded := stubMerger(nil) dst := ir.Property{ @@ -281,9 +281,13 @@ func TestDiagnoseRedeclarationConflict_ConstraintDisagreementIsReported(t *testi Provenance: ir.Provenance{Pointer: "/components/schemas/A/properties/age"}, Constraints: &ir.Constraints{Min: bigVal("10")}, } - src := ir.Property{WireName: "age", Constraints: &ir.Constraints{Min: bigVal("20")}} + src := ir.Property{ + WireName: "age", + Provenance: ptrAt("/components/schemas/B/properties/age"), + Constraints: &ir.Constraints{Min: bigVal("20")}, + } - g.diagnoseRedeclarationConflict(&dst, &src, "/components/schemas/B/properties/age") + g.recordRedeclarationConflict(&dst, &src) require.Len(t, *recorded, 1) assert.Equal(t, ir.SeverityWarning, (*recorded)[0].Severity) @@ -418,7 +422,7 @@ func TestKeepLosingType_RecordsTheDiscardedDeclaration(t *testing.T) { Provenance: ir.Provenance{Source: 2, Pointer: "/components/schemas/S/allOf/1/properties/id"}, } - g.reconcileProperty(&dst, src, "/components/schemas/S/allOf/1/properties/id") + g.reconcileProperty(&dst, src) require.Len(t, *recorded, 1, "the conflict is still diagnosed") assert.Equal(t, ir.TypeID("t/str"), dst.Type.Target, "the first declaration still wins the shape") @@ -447,9 +451,9 @@ func TestKeepLosingType_EveryLoserSurvivesItsSiblings(t *testing.T) { m := &ir.Model{} byWire := WireNameIndex(m.Properties) - g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}}, "/a") - g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/int"}}, "/b") - g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/bool"}}, "/c") + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}, Provenance: ptrAt("/a")}) + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/int"}, Provenance: ptrAt("/b")}) + g.MergeProperty(m, byWire, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/bool"}, Provenance: ptrAt("/c")}) require.Len(t, m.Properties, 1, "three declarations still reconcile to one property") assert.Equal(t, ir.Unmodeled{ @@ -480,7 +484,7 @@ func TestKeepLosingType_LeavesAgreeingAndConstraintOnlyConflictsAlone(t *testing ten, twenty := int64(10), int64(20) agreeing := ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}} - g.reconcileProperty(&agreeing, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}}, "/b") + g.reconcileProperty(&agreeing, ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/str"}, Provenance: ptrAt("/b")}) assert.Empty(t, *recorded, "agreeing declarations are not a conflict") assert.Empty(t, agreeing.Unmodeled, "and nothing was discarded to keep") @@ -492,9 +496,124 @@ func TestKeepLosingType_LeavesAgreeingAndConstraintOnlyConflictsAlone(t *testing g.reconcileProperty(&bounded, ir.Property{ WireName: "code", Type: ir.TypeRef{Target: "t/str"}, Constraints: &ir.Constraints{MaxLength: &twenty}, - }, "/b") + Provenance: ptrAt("/b"), + }) require.Len(t, *recorded, 1, "the constraint conflict is still diagnosed") assert.Equal(t, diag.ConflictingRedecl, (*recorded)[0].Code) assert.Empty(t, bounded.Unmodeled, "a constraint conflict keeps no type entry") } + +// TestReconcileProperty_AConflictingTypeTakesNothingWithIt pins that a +// discarded declaration is discarded whole. The type conflict is diagnosed and +// the loser's type kept under Unmodeled, but the fold below it used to run to +// completion regardless, adopting the loser's default, constraints and examples +// onto the winner. That leaves the document asserting two contradictory things +// about one field — an integer carrying a string default — and nothing in +// pass/validate or irverify compares a Value's kind to its property's type, so +// an emitter renders it into non-compiling code. +func TestReconcileProperty_AConflictingTypeTakesNothingWithIt(t *testing.T) { + t.Parallel() + g, recorded := stubMerger(map[ir.TypeID]ir.TypeDef{ + "t/str": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/str"}, Prim: ir.PrimString}, + "t/int": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/int"}, Prim: ir.PrimInt32}, + }) + dst := ir.Property{WireName: "id", Type: ir.TypeRef{Target: "t/int"}} + maxLen := int64(10) + src := ir.Property{ + WireName: "id", + Type: ir.TypeRef{Target: "t/str"}, + Default: &ir.Value{Kind: ir.ValueString, Str: "abc"}, + Constraints: &ir.Constraints{MaxLength: &maxLen}, + Examples: []ir.Example{{Value: &ir.Value{Kind: ir.ValueString, Str: "abc"}}}, + } + + g.reconcileProperty(&dst, src) + + require.Len(t, *recorded, 1, "the conflict is diagnosed") + assert.Equal(t, ir.TypeID("t/int"), dst.Type.Target, "the first declaration wins the shape") + assert.Nil(t, dst.Default, "a string default does not belong to an integer field") + assert.Nil(t, dst.Constraints, "maxLength constrains the string that was discarded") + assert.Empty(t, dst.Examples, "the examples are of the discarded shape") +} + +// TestReconcileProperty_NullabilityIntersectsWhenTargetsAgree pins the case +// typesConflict returns on before it reads Nullable: one branch admits null and +// the other does not, so the intersection forbids it. Neither reconciled nor +// preserved before, and the answer flipped with branch order — the same source +// compiled to nullable: true or false depending on which branch was written +// first. foldNullVerdicts already states the conjunction rule for the +// single-schema case; this is the same rule across a redeclaration. +func TestReconcileProperty_NullabilityIntersectsWhenTargetsAgree(t *testing.T) { + t.Parallel() + reg := map[ir.TypeID]ir.TypeDef{ + "t/str": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/str"}, Prim: ir.PrimString}, + } + cases := []struct { + name string + dstNull, srcNull, want bool + }{ + {"nullable then not", true, false, false}, + {"not then nullable", false, true, false}, + {"both nullable", true, true, true}, + {"neither nullable", false, false, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g, recorded := stubMerger(reg) + dst := ir.Property{WireName: "x", Type: ir.TypeRef{Target: "t/str", Nullable: tc.dstNull}} + src := ir.Property{WireName: "x", Type: ir.TypeRef{Target: "t/str", Nullable: tc.srcNull}} + + g.reconcileProperty(&dst, src) + + assert.Equal(t, tc.want, dst.Type.Nullable, "the merged field admits null only where both branches do") + assert.Empty(t, *recorded, "an intersection the IR can express is not a conflict") + }) + } +} + +// TestReconcileProperty_ADroppedTypeIsKeptEvenWhenItDoesNotConflict pins the +// gap between what the merge drops and what it used to record. typesConflict +// deliberately answers false for two composites of one kind and for the top type +// against anything — it does not guess — but reconcileProperty keeps dst.Type +// regardless, so in every one of those the redeclaration vanished with neither a +// diagnostic nor an entry. That is #424's own failure: a consumer diffing two +// versions sees no change. Preservation is owed wherever a type is dropped; +// the diagnostic stays with the conflict, which is a narrower claim. +func TestReconcileProperty_ADroppedTypeIsKeptEvenWhenItDoesNotConflict(t *testing.T) { + t.Parallel() + reg := map[ir.TypeID]ir.TypeDef{ + "t/A": &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/A"}}, + "t/B": &ir.Model{TypeCommon: ir.TypeCommon{ID: "t/B"}}, + "t/any": &ir.Any{TypeCommon: ir.TypeCommon{ID: "t/any"}}, + "t/str": &ir.Primitive{TypeCommon: ir.TypeCommon{ID: "t/str"}, Prim: ir.PrimString}, + } + cases := []struct { + name string + dstTarget, srcTarget ir.TypeID + }{ + {"two models of one kind", "t/A", "t/B"}, + {"the top type keeps the position", "t/any", "t/str"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g, recorded := stubMerger(reg) + dst := ir.Property{WireName: "id", Type: ir.TypeRef{Target: tc.dstTarget}} + src := ir.Property{ + WireName: "id", + Type: ir.TypeRef{Target: tc.srcTarget}, + Provenance: ir.Provenance{Source: 1, Pointer: "/p/allOf/1/properties/id"}, + } + + g.reconcileProperty(&dst, src) + + assert.Equal(t, tc.dstTarget, dst.Type.Target, "the first declaration still wins") + assert.Empty(t, *recorded, "a drop the predicate does not call a conflict is not diagnosed") + entry, ok := dst.Unmodeled[losingTypeKey+"/p/allOf/1/properties/id"] + require.True(t, ok, "the dropped declaration is kept; got %v", dst.Unmodeled) + assert.Equal(t, ir.ReasonDegradedLowering, entry.Reason) + }) + } +} diff --git a/compilers/openapi/internal/schema/compose_test.go b/compilers/openapi/internal/schema/compose_test.go index 1f626e0b..11a2b509 100644 --- a/compilers/openapi/internal/schema/compose_test.go +++ b/compilers/openapi/internal/schema/compose_test.go @@ -149,7 +149,7 @@ func TestAllOf_ConflictingRedeclaredDescriptionDiagnosed(t *testing.T) { "a differing redeclared description is surfaced, not dropped silently") } -func TestAllOf_ConflictingRedeclaredTypeDiagnosed(t *testing.T) { +func TestAllOf_ConflictingRedeclaredTypeDiagnosedAndKept(t *testing.T) { t.Parallel() // allOf is an intersection, so a field one branch types `string` and another // types `integer` describes an unsatisfiable schema. Reconciliation keeps the @@ -183,10 +183,11 @@ func TestAllOf_ConflictingRedeclaredTypeDiagnosed(t *testing.T) { // A diagnostic is not part of the document, so the losing declaration is // also kept where a consumer reading the IR will reach it (GitHub #424). - lost := m.Properties[0].Unmodeled["openapi:conflicting-redeclaration"+ + lost, ok := m.Properties[0].Unmodeled["openapi:conflicting-redeclaration"+ "/components/schemas/Conflictish/allOf/1/properties/id"] - assert.Equal(t, ir.ReasonDegradedLowering, lost.Reason, + require.True(t, ok, "the discarded type is kept beside the winner; got %v", m.Properties[0].Unmodeled) + assert.Equal(t, ir.ReasonDegradedLowering, lost.Reason) assert.JSONEq(t, `{"target":"t/prim/integer","nullable":false}`, string(lost.Value), "and it names the type the second branch declared, not the one that won") assert.Equal(t, "/components/schemas/Conflictish/allOf/1/properties/id", lost.Provenance.Pointer) diff --git a/compilers/openapi/internal/schema/schema.go b/compilers/openapi/internal/schema/schema.go index f9136cbb..3a177b5d 100644 --- a/compilers/openapi/internal/schema/schema.go +++ b/compilers/openapi/internal/schema/schema.go @@ -1070,7 +1070,7 @@ func fillModelProperties(c lowering.Ctx, ts *compile.Types, anchors *AnchorIndex diags = append(diags, FillPropertyDetail(c, ts, anchors, &p, js, ppointer)...) var mergeDiags []ir.Diagnostic mg := merger(c, ts, &mergeDiags) - mg.MergeProperty(m, byWire, p, ppointer) + mg.MergeProperty(m, byWire, p) diags = append(diags, mergeDiags...) } return diags diff --git a/docs/ir-design.md b/docs/ir-design.md index 31a29cfa..97c8193d 100644 --- a/docs/ir-design.md +++ b/docs/ir-design.md @@ -824,6 +824,27 @@ from it is exact and nothing is reported. alias §4.3 describes rather than kept as residue, and a boolean branch declares no keywords at all, so there is no residue to derive — it is a decision about the composed node instead, stated with the `false`-schema rule above. +- **A redeclared property whose type the merge drops** — `allOf` intersection folds a field + declared in more than one branch into one `Property`, and the first declaration wins its shape + (§4.3). The IR has no combinator for "string here, integer there", so the losing declaration's + `TypeRef` has nowhere to sit on the merged property, and before this rule it went nowhere at + all: a release in which a losing branch's type changed produced no change in the document, which + a consumer diffing two versions reads as nothing having happened. The discarded reference is + kept in `Unmodeled["openapi:conflicting-redeclaration"]` under `ReasonDegradedLowering`, + keyed by the losing declaration's own JSON Pointer so a field three branches type three + incompatible ways keeps all three entries, and with the entry's `Provenance` locating that + declaration rather than the merged property. The whole `TypeRef` is kept rather than the target + ID alone, since a redeclaration states both what a field is and whether it admits null. + Preservation is owed wherever a type is dropped, which is wider than the disagreements worth + reporting: the `openapi/conflicting-redeclaration` diagnostic fires only where the two + declarations are genuinely unsatisfiable, while two composites of one kind, an unresolvable + target, and the top type against anything are dropped without one and kept all the same. The + loser's `default`, constraints and examples go with its type rather than folding onto the + winner — they describe the shape that lost, and adopting them would leave one field asserting + two contradictory things, with nothing downstream comparing a value's kind to its property's + type. Two declarations naming the same target and differing only in nullability are not a drop + and take no entry: the merged field admits null only where both branches do, which is the same + conjunction a single schema's null verdicts fold under. - **A shape applicator the lowered node cannot carry** — `properties`, `patternProperties`, `additionalProperties`, `required`, `items` and `prefixItems` each constrain an instance whether or not a `type` is written beside them, and each has exactly one IR home: a `Model`'s property @@ -1873,7 +1894,7 @@ How each format's distinctive concepts land in the IR (full details live with ea | Format | Lowering highlights | |---|---| -| **OpenAPI 3.x** | components/schemas → registry (IDs from pointers); inline schemas hoisted with hints; `allOf` → Base/Mixins per §4.3; `oneOf`/`anyOf` → Union (Exclusive bit), null-variant → Nullable ref, co-declared with structural keywords → the composition distributed across the variants per §4.3, or — for the five shapes that cannot be distributed — structural body + verbatim union per §4.8 (branches that declare no shape at all are `validation_only` per §4.7); competing keywords at one position — `const`/`enum`/`allOf`, elected in that order; `oneOf` beside `anyOf`, where oneOf wins; and a parameter's or header's `schema` beside its `content`, where content wins, since a media-type entry names both a schema and the media type serializing it and the IR models both — lower as the elected keyword with every passed-over one verbatim Unmodeled (`degraded_lowering`) at its own pointer per §4.8, and a `{X, null}` oneOf beside an anyOf stays a Union rather than collapsing to a nullable ref; `discriminator` → Discriminator (3.2 `defaultMapping` → Discriminator.Default); `nullable`/type-arrays → Nullable; readOnly/writeOnly → Visibility and schema-level `default` → the referencing Property/Parameter's Default: both bind a *use* of the type rather than the type, so a declaration-site one is pushed down to referencing properties with use-site precedence and the declaration keeps its own copy verbatim (`no_ir_home` Unmodeled + diagnostic) — which is what a component nothing references would otherwise lose silently; `additionalProperties: false` → Additional=closed, `unevaluatedProperties: false` → closed_after_composition, `minProperties`/`maxProperties` → Model.Constraints (the property set's cardinality, as against Additional's openness); parameters → Params + HTTPBinding locations w/ style/explode, `allowEmptyValue` → Parameter.Unmodeled (`no_ir_home`: HTTPParamBinding holds its neighbours but not this one), a header parameter named Accept/Content-Type/Authorization lowered as declared + `reserved-header-name` warning (OpenAPI says such a definition SHALL be ignored; dropping declared content is an emitter's call, not a compiler's), 3.2 `in: querystring` → querystring location; requestBody/responses all content types → Payload.Contents, a non-required requestBody → Payload.Unmodeled (`no_ir_home`, presence is the IR's only body optionality); 3.2 `itemSchema` → Content.Item and `itemEncoding` → Content.ItemEncoding — except beside a positional `prefixEncoding`, where both go to Content.Unmodeled (`no_ir_home`) because a single every-item encoding cannot state ordinals; an Encoding Object's `allowReserved` → Content.Unmodeled (`no_ir_home`) under `openapi:encoding//allowReserved` or `openapi:itemEncoding/allowReserved`, ir.PartEncoding holding `style` and `explode` beside it but no field for this one, and read before the entry's emptiness is judged so an entry declaring nothing else is not dropped with the empty PartEncoding it lowers to; per-status responses/default → Conditions + ranges, error-response `headers` and its `content` map whatever its arity → ErrorCase.Unmodeled (`no_ir_home`, ErrorCase has neither field: it holds one TypeRef and no media type, so one entry loses the key it was written under just as several lose all but the first); response/encoding header `style` and `explode` → Property.Unmodeled (`no_ir_home`, ir.Property has neither field), and a `Content-Type` entry in either headers map lowered as declared + the same `reserved-header-name` warning the parameter position gets; webhooks → HTTPBinding.IsWebhook; callbacks → Callbacks; links → Response.Unmodeled and ErrorCase.Unmodeled alike (`no_ir_home`, promotable later): the two are lowerings of one Response Object, so a construct kept on only the success one makes a declaration survive or vanish on nothing but its status code; path-item `servers`, under `paths`, `webhooks` and a callback expression alike → Operation.Unmodeled (`no_ir_home`: §10 scopes servers by index list at service and channel, and an operation has no such list yet), with an operation's own `servers` — which OpenAPI says override the path item's — kept beside them under `openapi:operationServers`, the one key here not named for the keyword it holds, since two declarations at two pointers cannot share one map key without the survivor depending on lowering order; a path item's own `summary`/`description`, at the same three mounts → Operation.Unmodeled under `openapi:pathItemSummary`/`openapi:pathItemDescription` (`no_ir_home`) rather than merged into Docs: ir.Docs holds the operation's own pair and a path item's documents the path, so merging would need a precedence rule and would attach documentation the operation's author never wrote — an inference, which §6 places in policy rather than in a lowering; every operation a path item declares — the fixed method fields, 3.2 `query`, and 3.2 `additionalOperations` keyed by method — → an Operation apiece, mounted at its own pointer, with the `additionalOperations` key used verbatim as HTTPBinding.Method since OpenAPI reads a method name case-sensitively, and a key naming no method at all lowered as declared + `invalid-method-key` warning (the binding is unusable, but dropping the entry would lose every operation it declares); securitySchemes/security → Auth OR-of-ANDs, 3.2 device flow + `oauth2MetadataUrl` → Flows/OAuth2MetadataURL; servers+variables (3.2 named) → Servers; tags (3.2 parent/kind) → groups + TagDefs; info contact/license → Document; schema `example(s)` → Examples; `xml` object (incl. 3.2 nodeType) → XMLHints at type and property level; `not`/`if-then-else`/`dependentSchemas`/`dependentRequired`/`contains`/`propertyNames`/`unevaluated*` → verbatim Unmodeled per §4.7; `contentEncoding`/`contentMediaType` → `Encoding` on the scalar the position lowers to and `contentSchema` → Unmodeled (`no_ir_home`) per §4.7; `$id`/`$schema`/`$vocabulary` → Unmodeled (`out_of_scope`, `$id` not honoured for resolution); `$dynamicRef` → the anchored type by compiler expansion, else verbatim Unmodeled with the reason it was irreducible; an inline `allOf` branch declaring more than the merge consumes → verbatim Unmodeled (`degraded_lowering`) per §4.8; a boolean `false` `allOf` branch → the composed Model closed, branch verbatim Unmodeled (`degraded_lowering`) per §4.8, a `true` branch a silent no-op; a shape applicator (`properties`/`patternProperties`/`additionalProperties`/`required`/`items`/`prefixItems`, and `format` where no type is declared) the lowered node has no field for → verbatim Unmodeled (`degraded_lowering`) per §4.8; a parameter schema's `xml` and its `readOnly`/`writeOnly` → Parameter.Unmodeled (`no_ir_home`: Parameter has no field for either); `patternProperties` → AdditionalProps.Patterns; `prefixItems` → Tuple, with any trailing `items` → Tuple.Unmodeled (`degraded_lowering` per §4.8: an open tuple has no IR combinator, so the fixed head is lowered and the tail kept beside it); `x-*` → namespaced Unmodeled (legal on every object — hence Unmodeled on every node), read at every object that admits one: an object lowering to a node with a map of its own keeps them unscoped there, and one lowering to no node of its own is keyed by the path from its carrier down to it — on the document, `openapi:info/x-*`, `openapi:info/contact/x-*`, `openapi:info/license/x-*`, `openapi:externalDocs/x-*`, `openapi:components/x-*`, `openapi:tags//x-*`, `openapi:tags//externalDocs/x-*`; on the service, `openapi:paths/x-*`; on each operation the path item's `openapi:pathItem/x-*` plus `openapi:responses/x-*` and `openapi:externalDocs/x-*`; on the HTTP binding, `openapi:callbacks//x-*`; on the content, `openapi:encoding//x-*` and `openapi:itemEncoding/x-*`, `` and `` alike escaped RFC 6901 style so a document-chosen name stays one segment (§12); on the schema's type, `openapi:xml/x-*`, `openapi:discriminator/x-*`, `openapi:externalDocs/x-*`; on the scheme, `openapi:flows/x-*` — since several such objects reach one map and an unscoped key would leave the survivor to lowering order (§12); a Link Object's own ride inside the verbatim `links` entry rather than taking a key beside it; `$ref`-adjacent sibling keywords (3.1) and ref-target annotations merge onto the referencing Property/Parameter with **use-site precedence**, applied uniformly (oagen's ad-hoc per-site patching is the counterexample), and at a position carrying no Property/Parameter — an `allOf`/`oneOf`/`anyOf` branch, `items`, a component — bind an alias hoisted at that position instead, per §4.3; a oneOf/anyOf whose variants are all string consts normalizes to a closed `Enum` in a `pass/` normalization — not in the compiler — so per-variant `Docs` survive until the collapse is chosen; mutually-exclusive parameter groups (`x-mutually-exclusive-parameter-groups`) stay as namespaced Unmodeled entries, and their documented *promotion* (no dedicated node needed) is a pass that synthesizes one logical `Parameter` typed by a `Union` of variant models, bound via `HTTPParamBinding.ParamPath` per field; pagination only via injectable policy, marked Inferred | +| **OpenAPI 3.x** | components/schemas → registry (IDs from pointers); inline schemas hoisted with hints; `allOf` → Base/Mixins per §4.3; `oneOf`/`anyOf` → Union (Exclusive bit), null-variant → Nullable ref, co-declared with structural keywords → the composition distributed across the variants per §4.3, or — for the five shapes that cannot be distributed — structural body + verbatim union per §4.8 (branches that declare no shape at all are `validation_only` per §4.7); competing keywords at one position — `const`/`enum`/`allOf`, elected in that order; `oneOf` beside `anyOf`, where oneOf wins; and a parameter's or header's `schema` beside its `content`, where content wins, since a media-type entry names both a schema and the media type serializing it and the IR models both — lower as the elected keyword with every passed-over one verbatim Unmodeled (`degraded_lowering`) at its own pointer per §4.8, and a `{X, null}` oneOf beside an anyOf stays a Union rather than collapsing to a nullable ref; `discriminator` → Discriminator (3.2 `defaultMapping` → Discriminator.Default); `nullable`/type-arrays → Nullable; readOnly/writeOnly → Visibility and schema-level `default` → the referencing Property/Parameter's Default: both bind a *use* of the type rather than the type, so a declaration-site one is pushed down to referencing properties with use-site precedence and the declaration keeps its own copy verbatim (`no_ir_home` Unmodeled + diagnostic) — which is what a component nothing references would otherwise lose silently; `additionalProperties: false` → Additional=closed, `unevaluatedProperties: false` → closed_after_composition, `minProperties`/`maxProperties` → Model.Constraints (the property set's cardinality, as against Additional's openness); parameters → Params + HTTPBinding locations w/ style/explode, `allowEmptyValue` → Parameter.Unmodeled (`no_ir_home`: HTTPParamBinding holds its neighbours but not this one), a header parameter named Accept/Content-Type/Authorization lowered as declared + `reserved-header-name` warning (OpenAPI says such a definition SHALL be ignored; dropping declared content is an emitter's call, not a compiler's), 3.2 `in: querystring` → querystring location; requestBody/responses all content types → Payload.Contents, a non-required requestBody → Payload.Unmodeled (`no_ir_home`, presence is the IR's only body optionality); 3.2 `itemSchema` → Content.Item and `itemEncoding` → Content.ItemEncoding — except beside a positional `prefixEncoding`, where both go to Content.Unmodeled (`no_ir_home`) because a single every-item encoding cannot state ordinals; an Encoding Object's `allowReserved` → Content.Unmodeled (`no_ir_home`) under `openapi:encoding//allowReserved` or `openapi:itemEncoding/allowReserved`, ir.PartEncoding holding `style` and `explode` beside it but no field for this one, and read before the entry's emptiness is judged so an entry declaring nothing else is not dropped with the empty PartEncoding it lowers to; per-status responses/default → Conditions + ranges, error-response `headers` and its `content` map whatever its arity → ErrorCase.Unmodeled (`no_ir_home`, ErrorCase has neither field: it holds one TypeRef and no media type, so one entry loses the key it was written under just as several lose all but the first); response/encoding header `style` and `explode` → Property.Unmodeled (`no_ir_home`, ir.Property has neither field), and a `Content-Type` entry in either headers map lowered as declared + the same `reserved-header-name` warning the parameter position gets; webhooks → HTTPBinding.IsWebhook; callbacks → Callbacks; links → Response.Unmodeled and ErrorCase.Unmodeled alike (`no_ir_home`, promotable later): the two are lowerings of one Response Object, so a construct kept on only the success one makes a declaration survive or vanish on nothing but its status code; path-item `servers`, under `paths`, `webhooks` and a callback expression alike → Operation.Unmodeled (`no_ir_home`: §10 scopes servers by index list at service and channel, and an operation has no such list yet), with an operation's own `servers` — which OpenAPI says override the path item's — kept beside them under `openapi:operationServers`, the one key here not named for the keyword it holds, since two declarations at two pointers cannot share one map key without the survivor depending on lowering order; a path item's own `summary`/`description`, at the same three mounts → Operation.Unmodeled under `openapi:pathItemSummary`/`openapi:pathItemDescription` (`no_ir_home`) rather than merged into Docs: ir.Docs holds the operation's own pair and a path item's documents the path, so merging would need a precedence rule and would attach documentation the operation's author never wrote — an inference, which §6 places in policy rather than in a lowering; every operation a path item declares — the fixed method fields, 3.2 `query`, and 3.2 `additionalOperations` keyed by method — → an Operation apiece, mounted at its own pointer, with the `additionalOperations` key used verbatim as HTTPBinding.Method since OpenAPI reads a method name case-sensitively, and a key naming no method at all lowered as declared + `invalid-method-key` warning (the binding is unusable, but dropping the entry would lose every operation it declares); securitySchemes/security → Auth OR-of-ANDs, 3.2 device flow + `oauth2MetadataUrl` → Flows/OAuth2MetadataURL; servers+variables (3.2 named) → Servers; tags (3.2 parent/kind) → groups + TagDefs; info contact/license → Document; schema `example(s)` → Examples; `xml` object (incl. 3.2 nodeType) → XMLHints at type and property level; `not`/`if-then-else`/`dependentSchemas`/`dependentRequired`/`contains`/`propertyNames`/`unevaluated*` → verbatim Unmodeled per §4.7; `contentEncoding`/`contentMediaType` → `Encoding` on the scalar the position lowers to and `contentSchema` → Unmodeled (`no_ir_home`) per §4.7; `$id`/`$schema`/`$vocabulary` → Unmodeled (`out_of_scope`, `$id` not honoured for resolution); `$dynamicRef` → the anchored type by compiler expansion, else verbatim Unmodeled with the reason it was irreducible; an inline `allOf` branch declaring more than the merge consumes → verbatim Unmodeled (`degraded_lowering`) per §4.8; a property redeclared across branches with a type the merge drops → the discarded `TypeRef` verbatim Unmodeled (`degraded_lowering`) under `openapi:conflicting-redeclaration` per §4.8, keyed by the losing declaration's pointer, with the `openapi/conflicting-redeclaration` diagnostic only where the two are unsatisfiable, and nullability intersecting rather than dropping where the targets agree; a boolean `false` `allOf` branch → the composed Model closed, branch verbatim Unmodeled (`degraded_lowering`) per §4.8, a `true` branch a silent no-op; a shape applicator (`properties`/`patternProperties`/`additionalProperties`/`required`/`items`/`prefixItems`, and `format` where no type is declared) the lowered node has no field for → verbatim Unmodeled (`degraded_lowering`) per §4.8; a parameter schema's `xml` and its `readOnly`/`writeOnly` → Parameter.Unmodeled (`no_ir_home`: Parameter has no field for either); `patternProperties` → AdditionalProps.Patterns; `prefixItems` → Tuple, with any trailing `items` → Tuple.Unmodeled (`degraded_lowering` per §4.8: an open tuple has no IR combinator, so the fixed head is lowered and the tail kept beside it); `x-*` → namespaced Unmodeled (legal on every object — hence Unmodeled on every node), read at every object that admits one: an object lowering to a node with a map of its own keeps them unscoped there, and one lowering to no node of its own is keyed by the path from its carrier down to it — on the document, `openapi:info/x-*`, `openapi:info/contact/x-*`, `openapi:info/license/x-*`, `openapi:externalDocs/x-*`, `openapi:components/x-*`, `openapi:tags//x-*`, `openapi:tags//externalDocs/x-*`; on the service, `openapi:paths/x-*`; on each operation the path item's `openapi:pathItem/x-*` plus `openapi:responses/x-*` and `openapi:externalDocs/x-*`; on the HTTP binding, `openapi:callbacks//x-*`; on the content, `openapi:encoding//x-*` and `openapi:itemEncoding/x-*`, `` and `` alike escaped RFC 6901 style so a document-chosen name stays one segment (§12); on the schema's type, `openapi:xml/x-*`, `openapi:discriminator/x-*`, `openapi:externalDocs/x-*`; on the scheme, `openapi:flows/x-*` — since several such objects reach one map and an unscoped key would leave the survivor to lowering order (§12); a Link Object's own ride inside the verbatim `links` entry rather than taking a key beside it; `$ref`-adjacent sibling keywords (3.1) and ref-target annotations merge onto the referencing Property/Parameter with **use-site precedence**, applied uniformly (oagen's ad-hoc per-site patching is the counterexample), and at a position carrying no Property/Parameter — an `allOf`/`oneOf`/`anyOf` branch, `items`, a component — bind an alias hoisted at that position instead, per §4.3; a oneOf/anyOf whose variants are all string consts normalizes to a closed `Enum` in a `pass/` normalization — not in the compiler — so per-variant `Docs` survive until the collapse is chosen; mutually-exclusive parameter groups (`x-mutually-exclusive-parameter-groups`) stay as namespaced Unmodeled entries, and their documented *promotion* (no dedicated node needed) is a pass that synthesizes one logical `Parameter` typed by a `Union` of variant models, bound via `HTTPParamBinding.ParamPath` per field; pagination only via injectable policy, marked Inferred | | **Swagger 2.0** | lifted to OpenAPI 3.x shape first (body/formData → Payload; host/basePath/schemes → Servers; consumes/produces → content types), then the OpenAPI lowering runs | | **TypeSpec** | consumed post-check (monomorphized, `isFinished`); template instances → TypeCommon.Instantiation incl. value args → TemplateArg; models → Model w/ Base + spread provenance → Mixins; scalars → Scalar chains, constructors in values → Value.Ctor; `@encode`/`@format` → Encoding triple; `@encodedName` → WireNameByFormat at property AND type level; unions w/ named variants → Union, `@discriminated` → Discriminator.PropertyName/Envelope/EnvelopeValueName; `| null` → Nullable; visibility classes (incl. custom, `@invisible` → Visibility.None) → Visibility, op overrides → ParameterVisibility/ReturnTypeVisibility; `@patch` implicitOptionality → HTTPBinding.PatchImplicitOptionality; interfaces → OperationGroups (versionable); `@overload` → OverloadOf; `@sharedRoute` → SharedRoute; `@service` → Service; versioning decorators incl. `@typeChangedFrom`/`@madeOptional`/`@madeRequired` and add/remove cycles → Availability timeline (on members/variants/params too); pagination decorators incl. prev/first/last links and header continuation tokens → Pagination PropPaths (In:"header"); Azure.Core `@pollingOperation`/`@finalOperation` → LongRunning; multipart w/ parts → Content.Encoding/PartEncoding, `Http.File` → FileInfo (content-type set, contents chain, filename location); streams/SSE → StreamDetail + Variant.Event (contentType, terminal); `@error` → UsageFlags.Error; `@example`/`@opExample` → Examples (Input/Output pairs); `@pattern` message → Constraints.PatternMessage; `@mediaTypeHint` → TypeCommon.MediaTypeHint; `never` members deleted + diagnostic per §4.8; TCGC client-shaping decorators (`@clientName`, `@access`, `@usage`, `@scope`, `@override`, …) → namespaced Unmodeled (`out_of_scope`) consumed by emitter policy, never IR semantics; values/consts incl. enum-member refs → Values channel | | **Smithy 2.0** | structures → Model, mixins → Mixins (non-structure mixins flattened — spec-sanctioned); `document` → Any; unions → WireTagged Union, member `@jsonName` → Variant.WireName; enum/intEnum → Enum (open by default); `@sparse` → element Nullable; traits: constraints → Constraints, `@paginated` → Pagination (declared), `@retryable` → ErrorCase.Retryable + Throttling, `@error` fault → ErrorCase.Fault, `@readonly` → Idempotency safe, `@idempotent`/`@idempotencyToken` → Idempotency, `@sensitive` → Sensitive/Secret, `@tags` → Tags, `@clientOptional`/`@input` → Property.ClientOptional (+InputOnly), `@addedDefault` → DefaultAdded, root-shape `@default` pushed down to properties w/ provenance; `@streaming` blob → StreamDetail (+`@requiresLength` → RequiresLength); event streams → StreamDetail.Events union + Property.EventHeader/EventPayload + Initial messages; service-level errors → Service.CommonErrors; protocol traits → Service.Protocols; service `rename`/`version` → Service.Renames/Version; resources → OperationGroup + ResourceInfo (identifiers, properties, lifecycle incl. put/@noReplace, instance vs collection ops); http traits → HTTPBinding incl. `@endpoint`/`@hostLabel` → HostPrefix/host location (additive binding), `@httpPrefixHeaders`/`@httpQueryParams` → Prefix bindings, `@httpResponseCode` → Response.StatusCodeProp, `@requestCompression` → Compression, `@httpChecksumRequired` → ChecksumRequired; `@auth` order → priority-ordered Auth, `@optionalAuth` → empty option; `@jsonName` → WireName; `@mediaType` → Encoding.MediaType; xml traits → XMLHints at type and property level; `@examples` → Examples (Input/Output/Error); waiters + rules-engine traits → verbatim Unmodeled (`out_of_scope`, §15); `smithy.api#Unit` → nil payload / shared empty Model for tag-only variants; other traits → namespaced Unmodeled | diff --git a/testdata/conformance/openapi/allof-conflicting-type.golden.json b/testdata/conformance/openapi/allof-conflicting-type.golden.json index 65e7f546..6ce9cfed 100644 --- a/testdata/conformance/openapi/allof-conflicting-type.golden.json +++ b/testdata/conformance/openapi/allof-conflicting-type.golden.json @@ -189,7 +189,7 @@ { "severity": "warning", "code": "openapi/conflicting-redeclaration", - "message": "declarations of field \"clone_url\" disagree: incompatible types t/prim/url and t/prim/string; kept the first declaration (/components/schemas/Repository/allOf/0/properties/clone_url) over the redeclaration (/components/schemas/Repository/allOf/1/properties/clone_url)", + "message": "declarations of field \"clone_url\" disagree: incompatible types t/prim/url and t/prim/string (the redeclaration's type is kept verbatim under Unmodeled); kept the first declaration (/components/schemas/Repository/allOf/0/properties/clone_url) over the redeclaration (/components/schemas/Repository/allOf/1/properties/clone_url)", "provenance": { "source": 0, "pointer": "/components/schemas/Repository/allOf/1/properties/clone_url" @@ -198,7 +198,7 @@ { "severity": "warning", "code": "openapi/conflicting-redeclaration", - "message": "declarations of field \"id\" disagree: incompatible types t/prim/integer and t/prim/string; kept the first declaration (/components/schemas/Identified/allOf/0/properties/id) over the redeclaration (/components/schemas/Identified/allOf/1/properties/id)", + "message": "declarations of field \"id\" disagree: incompatible types t/prim/integer and t/prim/string (the redeclaration's type is kept verbatim under Unmodeled); kept the first declaration (/components/schemas/Identified/allOf/0/properties/id) over the redeclaration (/components/schemas/Identified/allOf/1/properties/id)", "provenance": { "source": 0, "pointer": "/components/schemas/Identified/allOf/1/properties/id"