From dbda38d80ea9a09de5bf95a957e0683e1914883d Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Sat, 9 May 2026 10:21:02 +0200 Subject: [PATCH] internal/cuetxtar: tighten cmpStruct/cmpConjunction against silent passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related cmp* loosenesses let assertions silently pass when they shouldn't. Tighten both, in the same commit, so the auto-promote of @test(eq:todo) cannot retire a todo whose actual value no longer matches the asserted shape. (1) cmpStruct: leaf-error rejection. When the expected was a struct with no embedded _|_ but the actual value was a leaf *adt.Bottom (BaseValue=*adt.Bottom, no surviving arcs), Fields() yielded nothing on the Bottom and the field iteration loop reported no unmatched expected fields. The assertion succeeded even though the value did not match the asserted shape at all — `objects: {}` "matched" against an incomplete-error value where `objects` was undefined. Reject when val.Core().V.DerefValue() has BaseValue=*adt.Bottom AND no child arcs. A struct that retains its arcs but carries an errored descendant still falls through to the field loop, which finds the error at the child path. The lenient hasEmbedBottom path remains the explicit way to assert "value is an error" — write {_|_, ...} in the expected. (2) cmpConjunction: exact conjunct count. cmpConjunction accepted `len(args) == len(astParts)-1` on the rationale that a "validator was consumed during evaluation" — for example, an assertion of `struct.MaxFields(2) & {}` would silently pass against a value that evaluated to plain `{}`. The asserted validator was never verified to have actually applied, the rendered output of such values lost the validator entirely, and the eq:todo auto-promote could retire todos whose actual value no longer reflected the asserted shape. Require equal conjunct count and remove the consumed-validator fallback. If a test wants to assert the presence of a validator, the value must preserve it; otherwise the expected expression should be simplified to just the residual. Adds two regression tests: - testdata/inline/eq_leaf_error_rejected.txtar: leaf-Bottom rejection, opt-in via field-level `_|_ @test(err, …)`, and errored-struct-with-arcs falls through. - testdata/inline/eq_conjunct_count.txtar: validator-collapses rejection, plus a positive control with bound conjunctions (`>=0 & <=10`) that survive evaluation. Updates cue/testdata/eval/issue3801.txtar — the `eq:todo` body asserted a residual struct against a leaf incomplete error and was silently accepted before the leaf-Bottom rejection landed. Signed-off-by: Marcel van Lohuizen Change-Id: Ib9de6b4c52865c117a2f29cb12df65e596584db0 Reviewed-on: https://cue.gerrithub.io/c/cue-lang/cue/+/1236924 Unity-Result: CUE porcuepine Reviewed-by: Daniel Martí TryBot-Result: CUEcueckoo --- cue/testdata/eval/issue3801.txtar | 2 +- internal/cuetxtar/astcmp.go | 23 +++-- internal/cuetxtar/testdata/inline/basic.txtar | 4 +- .../testdata/inline/eq_conjunct_count.txtar | 42 ++++++++ .../inline/eq_leaf_error_rejected.txtar | 96 +++++++++++++++++++ 5 files changed, 157 insertions(+), 10 deletions(-) create mode 100644 internal/cuetxtar/testdata/inline/eq_conjunct_count.txtar create mode 100644 internal/cuetxtar/testdata/inline/eq_leaf_error_rejected.txtar diff --git a/cue/testdata/eval/issue3801.txtar b/cue/testdata/eval/issue3801.txtar index f93c3803c..6b405dca4 100644 --- a/cue/testdata/eval/issue3801.txtar +++ b/cue/testdata/eval/issue3801.txtar @@ -112,7 +112,7 @@ issue3801: let: full: { #JSONOp: {op: "add", path: string, value: _} | {op: "remove", path: string} #Main: { namespace: string - output: #Patches & {(namespace): patch} + output: _|_ @test(err, code=incomplete) } out: { ns1: [{op: "add", path: "/metadata", value: "foo"}] diff --git a/internal/cuetxtar/astcmp.go b/internal/cuetxtar/astcmp.go index 801310e4d..8424960ab 100644 --- a/internal/cuetxtar/astcmp.go +++ b/internal/cuetxtar/astcmp.go @@ -412,6 +412,21 @@ func (c *cmpCtx) cmpStruct(path cue.Path, s *ast.StructLit, val cue.Value) error return pathErr(path, "value has embedded %v but expected struct has no embedded expression", scalar) } + // Reject when val itself is a leaf error (BaseValue=*adt.Bottom + // with no surviving child arcs). An expected struct without an + // embedded _|_ is asserting a struct shape; matching it against + // a leaf-erroneous value would silently pass when the expected + // field set is empty (Fields() yields nothing on a Bottom). A + // struct-shaped value that carries errored descendants but + // retains its arcs falls through here — the field loop below + // finds the error at the child path. The lenient hasEmbedBottom + // path is the explicit way to opt into accepting an error here. + if vx := val.Core().V; vx != nil { + deref := vx.DerefValue() + if b, ok := deref.BaseValue.(*adt.Bottom); ok && len(deref.Arcs) == 0 { + return pathErr(path, "value is an error (%v) but expected struct has no embedded _|_; use {_|_, ...} to assert error", b.Err) + } + } } // Compare regular fields (including definitions, optional, required, hidden). @@ -897,7 +912,7 @@ func (c *cmpCtx) cmpConjunction(path cue.Path, e *ast.BinaryExpr, val cue.Value) // We could be comparing a struct and a struct and a validator. return pathErr(path, "expected conjunction (&), got %v", op) } - if len(args) != len(astParts) && len(args) != len(astParts)-1 { + if len(args) != len(astParts) { return pathErr(path, "conjunction: expected %d conjunct(s), got %d", len(astParts), len(args)) } @@ -922,12 +937,6 @@ func (c *cmpCtx) cmpConjunction(path cue.Path, e *ast.BinaryExpr, val cue.Value) } } if !found { - // If the value has one fewer conjunct than expected, a validator - // was consumed during evaluation. Accept the unmatched non-struct - // conjunct — it documents the constraint that was applied. - if len(args) < len(astParts) { - continue - } return pathErr(path, "%s: no matching conjunct expr %s", pos(ae), toStr(ae)) } } diff --git a/internal/cuetxtar/testdata/inline/basic.txtar b/internal/cuetxtar/testdata/inline/basic.txtar index 5fe0b7a23..8a16bd4d1 100644 --- a/internal/cuetxtar/testdata/inline/basic.txtar +++ b/internal/cuetxtar/testdata/inline/basic.txtar @@ -24,7 +24,7 @@ kindIntFail: int @test(kind=string) closedStruct: close({a: 1}) @test(closed) // eq: builtin validator call expression -maxFields: struct.MaxFields(2) & {} @test(eq, struct.MaxFields(2) & {}) +minFields: struct.MinFields(0) & {} @test(eq, {}) // eq: selector reference (math.Pi) piField: math.Pi @test(eq, math.Pi) @@ -60,7 +60,7 @@ kindIntFail: int @test(kind=string) closedStruct: close({a: 1}) @test(closed) // eq: builtin validator call expression -maxFields: struct.MaxFields(2) & {} @test(eq, struct.MaxFields(2) & {}) +minFields: struct.MinFields(0) & {} @test(eq, {}) // eq: selector reference (math.Pi) piField: math.Pi @test(eq, math.Pi) diff --git a/internal/cuetxtar/testdata/inline/eq_conjunct_count.txtar b/internal/cuetxtar/testdata/inline/eq_conjunct_count.txtar new file mode 100644 index 000000000..94ee19cd3 --- /dev/null +++ b/internal/cuetxtar/testdata/inline/eq_conjunct_count.txtar @@ -0,0 +1,42 @@ +# Tests cmpConjunction's strict conjunct-count requirement. +# +# When the expected expression in @test(eq, ...) is a conjunction (a & b), +# the actual value must contain the same number of conjuncts. Previously +# the comparator accepted len(args) == len(astParts)-1 on the rationale +# that a "validator was consumed during evaluation" — e.g. an assertion +# of `struct.MaxFields(2) & {}` would silently pass against a value that +# evaluated down to plain `{}`. The asserted validator was never +# verified to have actually applied, the rendered output of such values +# lost the validator entirely, and the eq:todo auto-promote could +# silently retire todos whose actual value no longer matched. +# +# Cases: +# - case1: rejection — `MaxFields(2) & {}` (2 conjuncts) against a +# value that reduced to `{}` (1 conjunct). +# - case2: positive control — same shape on both sides. +-- in/test.cue -- +import "struct" + +// CASE 1: validator collapses during eval. Expected has 2 conjuncts; +// actual has 1. cmpConjunction must reject. +case1: {a: struct.MinFields(0) & {}}.a @test(eq, struct.MinFields(0) & {}) + +// CASE 2: positive control — equal conjunct counts. Bound conjunctions +// survive evaluation (`>=0 & <=10` does not reduce to a single value), +// so the comparator sees 2 conjuncts on both sides. +case2: >=0 & <=10 @test(eq, >=0 & <=10) +-- out/run/errors.txt -- +path case1: conjunction: expected 2 conjunct(s), got 1 +-- out/status.txt -- +update: identical to input +-- out/force/test.cue -- +import "struct" + +// CASE 1: validator collapses during eval. Expected has 2 conjuncts; +// actual has 1. cmpConjunction must reject. +case1: {a: struct.MinFields(0) & {}}.a @test(eq, {}) + +// CASE 2: positive control — equal conjunct counts. Bound conjunctions +// survive evaluation (`>=0 & <=10` does not reduce to a single value), +// so the comparator sees 2 conjuncts on both sides. +case2: >=0 & <=10 @test(eq, >=0 & <=10) diff --git a/internal/cuetxtar/testdata/inline/eq_leaf_error_rejected.txtar b/internal/cuetxtar/testdata/inline/eq_leaf_error_rejected.txtar new file mode 100644 index 000000000..9b9e093d9 --- /dev/null +++ b/internal/cuetxtar/testdata/inline/eq_leaf_error_rejected.txtar @@ -0,0 +1,96 @@ +# Tests cmpStruct's leaf-error rejection: an expected struct without an +# embedded _|_, matched against a value whose BaseValue is *adt.Bottom +# with no surviving child arcs, must fail rather than silently pass via +# the empty-Fields() iteration. +# +# Three cases: +# - case1: rejection — `bad: {}` against `bad: 1 & 2` (leaf Bottom). +# The runner emits the new error message; out/run/errors.txt +# asserts that. +# - case2: explicit opt-in — `bad: _|_ @test(err, …)` against the +# same leaf Bottom. Passes via the field-level errCheck path. +# - case3: errored-struct-with-arcs — wrapper has BaseValue=Bottom +# but retains its child arcs; the field loop descends and finds +# the error at the child path. No top-level rejection fires. +-- in/test.cue -- +case1: { + bad: 1 & 2 +} @test(eq, { + bad: {} +}) + +case2: { + bad: 1 & 2 +} @test(eq, { + bad: _|_ @test(err, code=eval, contains="conflicting values 2 and 1") +}) + +case3: { + ok: 1 + bad: 1 & 2 +} @test(eq, { + ok: 1 + bad: _|_ @test(err, code=eval, contains="conflicting values 2 and 1") +}) +-- out/run/errors.txt -- +path case1: bad: value is an error (case1.bad: conflicting values 2 and 1) but expected struct has no embedded _|_; use {_|_, ...} to assert error +-- out/status.txt -- +update: output passes run +-- out/update/out/errors.txt -- +[eval] case1.bad: conflicting values 2 and 1: + ./test.cue:2:7 + ./test.cue:2:11 +[eval] case2.bad: conflicting values 2 and 1: + ./test.cue:8:7 + ./test.cue:8:11 +[eval] case3.bad: conflicting values 2 and 1: + ./test.cue:15:7 + ./test.cue:15:11 +-- out/update/test.cue -- +case1: { + bad: 1 & 2 +} @test(eq, { + bad: {} +}) + +case2: { + bad: 1 & 2 +} @test(eq, { + bad: _|_ @test(err, code=eval, contains="conflicting values 2 and 1") +}) + +case3: { + ok: 1 + bad: 1 & 2 +} @test(eq, { + ok: 1 + bad: _|_ @test(err, code=eval, contains="conflicting values 2 and 1") +}) +-- out/force/test.cue -- +case1: { + bad: 1 & 2 +} @test(err) + +case2: { + bad: 1 & 2 +} @test(eq, { + bad: _|_ @test(err, code=eval, contains="conflicting values 2 and 1") +}) + +case3: { + ok: 1 + bad: 1 & 2 +} @test(eq, { + ok: 1 + bad: _|_ @test(err, code=eval, contains="conflicting values 2 and 1") +}) +-- out/force/out/errors.txt -- +[eval] case1.bad: conflicting values 2 and 1: + ./test.cue:2:7 + ./test.cue:2:11 +[eval] case2.bad: conflicting values 2 and 1: + ./test.cue:8:7 + ./test.cue:8:11 +[eval] case3.bad: conflicting values 2 and 1: + ./test.cue:15:7 + ./test.cue:15:11 -- 2.51.2