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

.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

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)

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"}},

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")
+
+	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",
+			"/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)
+	// 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
 // 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

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