diff --git a/internal/harness/harness.go b/internal/harness/harness.go index fb0208c..b10aeef 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -81,7 +81,7 @@ func Check(ctx context.Context, spec string, data []byte) (res Result) { // Last, and after deterministic: this one permutes the input, so a compiler // that is not even same-input-same-output should be reported as that rather // than as order-dependence. - if detail, ok := orderInvariant(ctx, spec, data, doc); !ok { + if detail, ok := orderInvariant(ctx, spec, data); !ok { return Result{Spec: spec, Outcome: OutcomeOrderDependent, Detail: detail} } return res diff --git a/internal/harness/order.go b/internal/harness/order.go index 4fe2873..f999306 100644 --- a/internal/harness/order.go +++ b/internal/harness/order.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "sort" + "strings" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -19,8 +20,15 @@ import ( // depth. const maxReverseDepth = 512 -// orderInvariant compiles data as written and again with every mapping's entry -// order reversed, and reports what the permutation changed beyond source order. +// positionPlaceholder stands in for a provenance pointer that locates a +// construct by source position. The leading control byte is what makes it a +// placeholder rather than a value: no producer spells a pointer with one, so it +// cannot collide with a pointer that means itself. +const positionPlaceholder = "\x01position" + +// orderInvariant compiles a source twice — once with its mappings as declared, +// once with every mapping's entry order reversed — and reports what the +// permutation changed beyond source order. // // It is the general form of the two-order diff CLAUDE.md prescribes. // `deterministic`, next to it, looks like this and is a different property: it @@ -29,26 +37,44 @@ const maxReverseDepth = 512 // pointer first — the shape of the pointer collisions in #108 and #112, which // produce no diagnostic in either order and leave pass/validate clean on both. // -// Two limits belong here so the oracle is not over-trusted: +// Both arms are compiled from the encoder's output rather than one from the +// source and one from the rewrite. The encoder does not preserve every spelling +// — a flow-style implicit null comes back carrying an empty string — and +// comparing against the source would read that rewriting as a lowering that +// depends on order. Passing both sides through it leaves declaration order as +// the only difference between them, which is the question being asked. // -// Reversing a mapping is meaning-preserving only while its keys are distinct. A -// document with duplicate mapping keys resolves to the last one (#95), so -// reversing changes which declaration wins and the two compiles legitimately -// differ; such sources are excluded rather than reported. Sequences are left -// alone throughout — allOf precedence, oneOf variant order and prefixItems -// positions are all semantic, and reversing them would change the document's -// meaning rather than only its spelling. +// The limits belong here so the oracle is not over-trusted. Reversing a mapping +// is meaning-preserving only while its keys are distinct: duplicate keys resolve +// to the last declaration (#95), so reversing changes which one wins and the two +// compiles legitimately differ. Such a source is excluded rather than reported, +// as is one whose permutation no longer parses — see reverseMappings — and one +// whose re-encoding will not compile, which leaves nothing faithful to compare +// against. Sequences are left alone throughout: allOf precedence, oneOf variant +// order and prefixItems positions are all semantic, and reversing them would +// change the document's meaning rather than only its spelling. // // And it proves order-independence only for the constructs its input contains, // which is why it runs over the corpus rather than over one hand-written spec. -func orderInvariant(ctx context.Context, spec string, data []byte, doc *ir.Document) (string, bool) { +func orderInvariant(ctx context.Context, spec string, data []byte) (string, bool) { + baseline, ok := reencodeMappings(data) + if !ok { + return "", true // the source does not survive a parse and re-encode + } reversed, ok := reverseMappings(data) if !ok { - return "", true // not a document whose permutation is meaning-preserving + return "", true // its permutation would not be meaning-preserving } - if bytes.Equal(reversed, data) { + if bytes.Equal(reversed, baseline) { return "", true // nothing to permute; the oracle has no question to ask } + doc, _, err := compile(ctx, spec, baseline) + if err != nil { + return "", true // the re-encoding does not compile, so there is no baseline + } + if doc == nil { + return "", true // nor does a source the compiler declines outright + } other, otherDiags, err := compile(ctx, spec, reversed) if err != nil { return "recompile permuted: " + err.Error(), false @@ -109,16 +135,46 @@ func diffOrderInvariants(first, second *ir.Document, secondDiags []ir.Diagnostic // reverse — which is invariant #7's source ordering reaching the message rather // than a lowering that depends on order. Severity, code and pointer identify the // finding without that. +// +// A pointer spelled line:col is excluded for the same reason. Provenance.Pointer +// admits either a structural pointer or a source position, and a permutation +// moves a construct to a different line by design. Replacing it rather than +// dropping the field keeps the finding in the multiset, so a permutation that +// changes how many were reported still shows. func diagnosticSet(diags []ir.Diagnostic) []string { out := make([]string, 0, len(diags)) for _, d := range diags { + pointer := d.Provenance.Pointer + if isSourcePosition(pointer) { + pointer = positionPlaceholder + } out = append(out, fmt.Sprintf("%s\x00%s\x00%s\x00%d", - d.Severity, d.Code, d.Provenance.Pointer, d.Provenance.Source)) + d.Severity, d.Code, pointer, d.Provenance.Source)) } sort.Strings(out) return out } +// isSourcePosition reports whether a provenance pointer is a line:col position +// rather than a structural pointer. +func isSourcePosition(pointer string) bool { + line, col, ok := strings.Cut(pointer, ":") + return ok && isDigits(line) && isDigits(col) +} + +// isDigits reports whether s is a non-empty run of ASCII digits. +func isDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + // sourceOrderedCollections orders the collections a mapping's entry order // decides, so comparing two permutations of one document does not report their // own permutation. @@ -137,10 +193,32 @@ func renderExample(e ir.Example) string { return fmt.Sprintf("%s\x00%s\x00%v", e.Name, e.ExternalURL, e.Value) } +// reencodeMappings returns src parsed and re-encoded with its entry order +// intact: the same normalization reverseMappings applies, minus the permutation. +// It is what the permuted source is compared against, so a spelling the encoder +// rewrites changes both sides alike. +func reencodeMappings(src []byte) ([]byte, bool) { + var root yaml.Node + if err := yaml.Unmarshal(src, &root); err != nil { + return nil, false + } + out, err := encodeYAML(&root) + if err != nil { + return nil, false + } + return out, true +} + // reverseMappings returns src with the entry order of every YAML mapping // reversed, and ok=false for a source the rewrite cannot faithfully permute: one -// that does not parse, one that will not re-encode, or one carrying a duplicate -// mapping key, whose meaning depends on the order being changed. +// that does not parse, one that will not re-encode, one carrying a duplicate +// mapping key, whose meaning depends on the order being changed, or one whose +// permutation no longer parses. +// +// That last case is the rewrite's own doing rather than a fact about the +// compiler: reversing a mapping can carry an alias above the anchor it names, +// which YAML forbids. Re-parsing catches it without enumerating it, and covers +// any later ordering rule of the same kind. func reverseMappings(src []byte) ([]byte, bool) { var root yaml.Node if err := yaml.Unmarshal(src, &root); err != nil { @@ -153,6 +231,10 @@ func reverseMappings(src []byte) ([]byte, bool) { if err != nil { return nil, false } + var check yaml.Node + if err := yaml.Unmarshal(out, &check); err != nil { + return nil, false + } return out, true } diff --git a/internal/harness/order_test.go b/internal/harness/order_test.go index 0b61bef..8b37b13 100644 --- a/internal/harness/order_test.go +++ b/internal/harness/order_test.go @@ -1,6 +1,7 @@ package harness import ( + "bytes" "context" "errors" "os" @@ -75,14 +76,31 @@ func TestReverseMappings_Permutes(t *testing.T) { // TestReverseMappings_AliasIsNotFollowed pins that an anchored mapping is // reversed once, at its anchor, rather than once per alias pointing at it. +// +// The anchor and its use sit in a sequence, whose order the rewrite leaves +// alone, so the anchor still precedes the alias afterwards. Declared as two keys +// of one mapping they would swap, and a source whose permutation puts an alias +// above its anchor is refused outright — the case below this one. func TestReverseMappings_AliasIsNotFollowed(t *testing.T) { t.Parallel() - got, ok := reverseMappings([]byte("anchor: &a\n x: 1\n y: 2\nuse: *a\n")) + got, ok := reverseMappings([]byte("items:\n - anchor: &a\n x: 1\n y: 2\n - use: *a\n")) require.True(t, ok) - assert.Equal(t, "use: *a\nanchor: &a\n y: 2\n x: 1\n", string(got), + assert.Equal(t, "items:\n - anchor: &a\n y: 2\n x: 1\n - use: *a\n", string(got), "the anchored mapping reverses once and the alias still points at it") } +// TestReverseMappings_AliasAboveItsAnchorIsRefused pins the exclusion that +// replaced the shape the test above used to assert. Reversing the two keys puts +// the alias first, and YAML requires an anchor to be defined before it is +// referenced, so the permuted source does not parse at all. Handing that to the +// compiler reported the parse failure as order dependence. +func TestReverseMappings_AliasAboveItsAnchorIsRefused(t *testing.T) { + t.Parallel() + got, ok := reverseMappings([]byte("anchor: &a\n x: 1\n y: 2\nuse: *a\n")) + assert.False(t, ok, "a permutation that no longer parses is not a faithful one") + assert.Nil(t, got) +} + // TestReverseNode_DepthBound covers the walk's bound. No document the compiler // accepts nests this deep, so the bound exists to keep the walk terminating // rather than to reject real input. @@ -112,39 +130,90 @@ func TestOrderInvariant_UnpermutableSourcePasses(t *testing.T) { for _, tc := range []struct{ name, src string }{ {"a duplicate key excludes the source", "a: 1\na: 2\n"}, {"a source with nothing to permute", "a: 1\n"}, + {"an unparseable source excludes the baseline too", "a: [unclosed\n"}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() - detail, ok := orderInvariant(context.Background(), "spec", []byte(tc.src), &ir.Document{}) + detail, ok := orderInvariant(context.Background(), "spec", []byte(tc.src)) assert.True(t, ok, "declining to ask is not a finding") assert.Empty(t, detail) }) } } -// TestOrderInvariant_CompileFailureIsReported covers the recompile's error -// paths, which a correct compiler never produces on a source that already -// compiled once. +// TestOrderInvariant_CompileFailureIsReported covers the permuted recompile's +// error paths, which a correct compiler never produces on a source whose +// baseline already compiled. +// +// The seam answers for the baseline and fails only for the permutation. Failing +// both would exercise nothing: a baseline that will not compile means the +// re-encoding is not the source, which the oracle skips rather than reports. func TestOrderInvariant_CompileFailureIsReported(t *testing.T) { const src = "a: 1\nb: 2\n" orig := compile t.Cleanup(func() { compile = orig }) - compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { - return nil, nil, errors.New("boom") + baseline, ok := reencodeMappings([]byte(src)) + require.True(t, ok) + + onlyPermutedFails := func(fail func() (*ir.Document, []ir.Diagnostic, error)) { + compile = func(_ context.Context, _ string, data []byte) (*ir.Document, []ir.Diagnostic, error) { + if bytes.Equal(data, baseline) { + return &ir.Document{}, nil, nil + } + return fail() + } } - detail, ok := orderInvariant(context.Background(), "spec", []byte(src), &ir.Document{}) + + onlyPermutedFails(func() (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, errors.New("boom") + }) + detail, ok := orderInvariant(context.Background(), "spec", []byte(src)) assert.False(t, ok) assert.Contains(t, detail, "boom") - compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { + onlyPermutedFails(func() (*ir.Document, []ir.Diagnostic, error) { return nil, nil, nil - } - detail, ok = orderInvariant(context.Background(), "spec", []byte(src), &ir.Document{}) + }) + detail, ok = orderInvariant(context.Background(), "spec", []byte(src)) assert.False(t, ok) assert.Contains(t, detail, "no document") } +// TestOrderInvariant_UncompilableBaselineIsNotAFinding pins the skip the test +// above depends on: when the re-encoded source yields no document, the oracle has +// no faithful baseline to ask its question of, so it declines rather than +// reporting the permutation. +// +// Both ways of yielding none are driven, because they are separate branches and +// a compiler reaches them for different reasons — an I/O or programmer fault +// against a source it declines to lower at all. +func TestOrderInvariant_UncompilableBaselineIsNotAFinding(t *testing.T) { + tests := []struct { + name string + fail func() (*ir.Document, []ir.Diagnostic, error) + }{ + {"the baseline errors", func() (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, errors.New("boom") + }}, + {"the baseline compiles to no document", func() (*ir.Document, []ir.Diagnostic, error) { + return nil, nil, nil + }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + orig := compile + t.Cleanup(func() { compile = orig }) + compile = func(context.Context, string, []byte) (*ir.Document, []ir.Diagnostic, error) { + return tc.fail() + } + detail, ok := orderInvariant(context.Background(), "spec", []byte("a: 1\nb: 2\n")) + assert.True(t, ok, "no baseline is not a finding about the compiler") + assert.Empty(t, detail) + }) + } +} + // TestDiffOrderInvariants_ReportsEachChannel drives the three ways a permutation // can differ, so each message is exercised rather than only the first. func TestDiffOrderInvariants_ReportsEachChannel(t *testing.T) { @@ -214,13 +283,18 @@ func TestDiffOrderInvariants_ReorderedCollectionsAreNotAFinding(t *testing.T) { // // It asserts the oracle actually asked its question of most of the corpus, not // merely that it ran. +// +// Both arms are guarded, because either can go quiet on its own. The rewrite +// declines a source it cannot faithfully permute, and the baseline declines one +// whose re-encoding will not compile — a skip with no diagnostic behind it, so +// nothing else would notice it spreading. func TestOrderInvariant_ReachesTheCorpus(t *testing.T) { t.Parallel() const dir = "../../testdata/conformance/openapi" entries, err := os.ReadDir(dir) require.NoError(t, err) - var specs, permuted int + var specs, permuted, baselined int for _, e := range entries { if e.IsDir() || !strings.HasSuffix(e.Name(), ".yaml") { continue @@ -228,14 +302,68 @@ func TestOrderInvariant_ReachesTheCorpus(t *testing.T) { specs++ data, readErr := os.ReadFile(filepath.Join(dir, e.Name())) require.NoError(t, readErr) + + baseline, ok := reencodeMappings(data) + if !ok { + continue + } + if doc, _, compileErr := compile(t.Context(), e.Name(), baseline); compileErr == nil && doc != nil { + baselined++ + } rewritten, ok := reverseMappings(data) - if ok && string(rewritten) != string(data) { + if ok && string(rewritten) != string(baseline) { permuted++ } } require.Positive(t, specs, "found no conformance specs to permute") assert.Equal(t, specs, permuted, "every conformance spec must be permutable, or the oracle is silently skipping it") + assert.Equal(t, specs, baselined, + "every conformance spec's re-encoding must still compile, or the oracle has no baseline to compare against") +} + +// TestOrderInvariant_PermutationArtifactsAreNotFindings covers the sources whose +// permutation is not meaning-preserving for a reason reverseMappings does not +// exclude. Each compiles cleanly, and each differs between the two orders because +// of the rewrite rather than because of a lowering, so reporting one is a false +// finding about the compiler. +func TestOrderInvariant_PermutationArtifactsAreNotFindings(t *testing.T) { + t.Parallel() + tests := []struct { + name string + src string + }{ + { + // yaml.Marshal re-emits the flow mapping "{A}" — whose value is an + // implicit null — as "{A: ''}", so the second compile reads an empty + // string where the first read null. Block style keeps the null. + name: "flow-style implicit null", + src: "openapi: 3.0.0\ninfo: {title: 0, version: 0}\n" + + "components:\n schemas:\n 0:\n allOf:\n - {A}\n", + }, + { + // Reversed, the alias precedes the anchor it names, which YAML forbids: + // the permuted source does not parse at all. + name: "an alias reordered above its anchor", + src: "openapi: 3.0.0\ninfo: {title: 0, version: 0}\n0: &m\n1: *m\n", + }, + { + // Provenance.Pointer holds line:col for a reference-resolution failure, + // and a permutation moves the offending node to a different line. + name: "a diagnostic located by line and column", + src: "openapi: 3.0.0\ninfo: {title: 0, version: 0}\npaths:\n 0:\n 0:\n" + + " responses:\n 0:\n description: 0\n" + + " callbacks:\n 0:\n 0:\n description:\n", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + res := Check(context.Background(), tc.name+".yaml", []byte(tc.src)) + assert.NotEqual(t, OutcomeOrderDependent, res.Outcome, + "the permutation changed the document's meaning, so this is not a finding: %s", res.Detail) + }) + } } // yamlMapping returns a mapping node for the depth-bound test. @@ -261,6 +389,47 @@ func TestReverseMappings_EncodeFailureIsRefused(t *testing.T) { assert.Nil(t, got) } +// TestReencodeMappings_EncodeFailureIsRefused drives the defensive re-encode +// path on the baseline arm, the counterpart of the reverseMappings case above. +func TestReencodeMappings_EncodeFailureIsRefused(t *testing.T) { + orig := encodeYAML + t.Cleanup(func() { encodeYAML = orig }) + encodeYAML = func(any) ([]byte, error) { return nil, errors.New("boom") } + + got, ok := reencodeMappings([]byte("a: 1\nb: 2\n")) + assert.False(t, ok, "a source that will not re-encode is excluded, not reported") + assert.Nil(t, got) +} + +// TestIsSourcePosition_ClassifiesPointers covers the split diagnosticSet turns +// on: a source position moves with the source and must not identify a finding, +// while every other pointer spelling — a JSON pointer, an IR-space ID — is +// stable and must keep identifying one. +func TestIsSourcePosition_ClassifiesPointers(t *testing.T) { + t.Parallel() + tests := []struct { + name string + pointer string + want bool + }{ + {"a line and column", "11:21", true}, + {"the first position", "0:0", true}, + {"empty", "", false}, + {"no line", ":21", false}, + {"no column", "11:", false}, + {"a non-numeric half", "a:1", false}, + {"a third segment", "1:2:3", false}, + {"a JSON pointer", "/components/schemas/A", false}, + {"an IR-space id", "t/openapi/components/schemas/A", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, isSourcePosition(tc.pointer)) + }) + } +} + // TestCheck_OrderDependentOutcome pins that Check classifies an order-dependent // compiler as such rather than folding it into another oracle. The seam returns // a different registry for the permuted bytes, which is what a lowering that