diff --git a/docs/MATERIALIZATION-SPEC.md b/docs/MATERIALIZATION-SPEC.md index 254e19e..2811996 100644 --- a/docs/MATERIALIZATION-SPEC.md +++ b/docs/MATERIALIZATION-SPEC.md @@ -295,8 +295,8 @@ for both languages from this point forward. had not yet caught in code: `saphyr` (this engine's YAML parser) silently demoted an integer literal beyond `i64` range to a lossy `f64` before canonicalization ever saw it — fixed in `reader.rs`, - not papered over here, via a new `Value::BigInt(String)` carrying - the literal's exact digits. Not yet wired into a pipeline stage — + not papered over here, via a new `Value::BigInt` carrying the + literal's exact digits (a validated `BigIntValue` newtype since #53). Not yet wired into a pipeline stage — `Parse`/`Normalize`/`Resolve`/`Validate`, which would produce a materialized tree to canonicalize, still don't exist — so this is a standalone primitive, correct and tested in isolation, not yet diff --git a/engine/src/canonical.rs b/engine/src/canonical.rs index e13b006..1fb9e44 100644 --- a/engine/src/canonical.rs +++ b/engine/src/canonical.rs @@ -6,6 +6,7 @@ //! session, not assumed from documentation. Number canonicalization (A3) //! already has a proven Rust peer in the `cic-canonical` crate (both its //! `canonical_float` and `canonical_integer`); the logic is ported here +//! (`canonical_integer` as [`crate::reader::BigIntValue::canonical`]) //! rather than taken as a dependency, so this crate's canonicalization //! stays self-contained and this engine does not acquire a cross-repository //! build dependency on `CIC-Relay`. @@ -42,12 +43,9 @@ use sha2::{Digest as _, Sha256}; /// is reachable from authored YAML's own `.nan`/`.inf` tags, not only from /// constructing a [`Value`] directly. /// -/// Returns `E_MALFORMED_VALUE` for the two shapes only a hand-built -/// [`Value`] can have (the reader never produces them): a `Value::BigInt` -/// whose text is not an optional sign followed by ASCII digits -/// (`R-CANON-BIGINT-DIGITS`), and a `Map` holding the same key more than -/// once (`R-CANON-UNIQUE-KEYS`). Neither panics and neither is written out -/// as if it were valid. +/// A `Value` cannot hold a non-digit `BigInt` or a repeated map key (see +/// [`crate::reader::BigIntValue`] and [`crate::reader::Map`]), so the only +/// failure is the non-finite float above. pub fn to_canonical_json(v: &Value) -> Result> { let mut out = Vec::new(); write_value(&mut out, v, "$")?; @@ -78,27 +76,12 @@ fn write_value(out: &mut Vec, v: &Value, path: &str) -> Result<()> { Value::Int(i) => out.extend_from_slice(i.to_string().as_bytes()), // A3's big.Int-equivalent case (review-caught on PR #19: `reader.rs` // used to silently lose this precision to saphyr's own i64/f64 - // fallback -- see `Value::BigInt`'s own doc comment). `reader.rs` - // only guarantees the text is a validated digit run; normalization - // -- sign fold, leading-zero strip, `-0` -> `0` -- happens here, - // ported from `cic-canonical`'s own already-proven - // `canonical_integer`, the same function Go's `number.go` and this - // crate's number canonicalization both already trust. `Value` is - // public, so a caller can hand in a `BigInt` the reader would never - // build: that is an error, not a panic (Go's `CanonicalInteger` - // checks the same way). - Value::BigInt(s) => match canonical_integer(s) { - Some(n) => out.extend_from_slice(n.as_bytes()), - None => { - return Err(Error::new( - code::MALFORMED_VALUE, - "R-CANON-BIGINT-DIGITS", - Stage::Canonicalize, - path, - format!("{path} is a BigInt whose text is not a signed digit run: {s:?}"), - )); - } - }, + // fallback -- see `Value::BigInt`'s own doc comment). The text is a + // digit run by construction (`BigIntValue::try_new`); normalization + // -- sign fold, leading-zero strip, `-0` -> `0` -- is + // `BigIntValue::canonical`, the same rule Go's `number.go` applies via + // `big.Int.SetString(s, 10).String()`. + Value::BigInt(n) => out.extend_from_slice(n.canonical().as_bytes()), Value::Float(f) => { if !f.is_finite() { return Err(Error::new( @@ -121,32 +104,6 @@ fn write_value(out: &mut Vec, v: &Value, path: &str) -> Result<()> { Ok(()) } -/// A3, ported from `cic-canonical`'s own `canonical_integer` (not imported -/// as a dependency — see module docs): sign folded, leading zeros stripped, -/// `-0`/`+0` -> `0` — the same normalization Go's `number.go` applies via -/// `big.Int.SetString(s, 10).String()`, verified empirically against it in -/// this session (`"000000123"` -> `"123"`, `"-00000123"` -> `"-123"`, -/// `"-0"`/`"+0"` -> `"0"`). Returns `None` only if `s` is not actually a -/// validated digit run — [`crate::reader::Value::BigInt`]'s own invariant -/// guarantees this never happens for a value this crate constructed itself. -pub(crate) fn canonical_integer(s: &str) -> Option { - let (neg, digits) = match s.strip_prefix('-') { - Some(rest) => (true, rest), - None => (false, s.strip_prefix('+').unwrap_or(s)), - }; - if digits.is_empty() || !digits.bytes().all(|b| b.is_ascii_digit()) { - return None; - } - let magnitude = digits.trim_start_matches('0'); - Some(if magnitude.is_empty() { - "0".to_string() // all zeros, including "-0"/"+0" - } else if neg { - format!("-{magnitude}") - } else { - magnitude.to_string() - }) -} - /// A3, ported from `cic-canonical`'s own `canonical_float` (not imported as a /// dependency — see module docs): shortest round-trip plain-decimal form, /// `-0.0` folded to `0`. Rust's `f64` `Display` is shortest round-trip and @@ -177,23 +134,11 @@ fn write_array(out: &mut Vec, items: &[Value], path: &str) -> Result<()> { fn write_map(out: &mut Vec, m: &Map, path: &str) -> Result<()> { // Raw UTF-8 byte order -- `str`'s `Ord` is already byte-wise comparison, // matching Go's `sort.Strings` (A2) with no custom comparator needed. - // Sorting the entries (not just the keys) puts any duplicate next to its - // twin, so it is caught here instead of being written twice with the - // first value both times, which is what a key-list + `get` walk did. - let mut entries: Vec<&(String, Value)> = m.0.iter().collect(); - entries.sort_by(|a, b| a.0.cmp(&b.0)); - if let Some(pair) = entries.windows(2).find(|w| w[0].0 == w[1].0) { - let k = &pair[0].0; - return Err(Error::new( - code::MALFORMED_VALUE, - "R-CANON-UNIQUE-KEYS", - Stage::Canonicalize, - format!("{path}.{k}"), - format!("{path} holds the key {k:?} more than once; canonical JSON objects have unique keys"), - )); - } + // Keys are unique by `Map`'s construction, so the order is total. + let mut entries: Vec<(&str, &Value)> = m.iter().collect(); + entries.sort_unstable_by(|a, b| a.0.cmp(b.0)); out.push(b'{'); - for (i, (k, v)) in entries.into_iter().map(|e| (&e.0, &e.1)).enumerate() { + for (i, (k, v)) in entries.into_iter().enumerate() { if i > 0 { out.push(b','); } @@ -249,7 +194,7 @@ fn write_json_string(out: &mut Vec, s: &str) { #[cfg(test)] mod tests { use super::*; - use crate::reader::Map; + use crate::reader::{BigIntValue, Map}; fn json(v: &Value) -> String { String::from_utf8(to_canonical_json(v).expect("value is finite")) @@ -474,7 +419,7 @@ mod tests { ]; for (literal, want) in cases { assert_eq!( - json(&Value::BigInt((*literal).to_string())), + json(&Value::BigInt(BigIntValue::try_new(*literal).unwrap())), *want, "literal {literal:?}" ); diff --git a/engine/src/collection.rs b/engine/src/collection.rs index b4f032e..d4e8dfc 100644 --- a/engine/src/collection.rs +++ b/engine/src/collection.rs @@ -39,7 +39,6 @@ //! floats), kept here only as a defensive, documented exception for a //! `Value` this function might be handed before that stage runs. -use crate::canonical::canonical_integer; use crate::reader::Value; /// Ported from `collection.go`'s `CollectionTopology`. @@ -124,11 +123,9 @@ impl Collection { } /// `fmt.Sprintf("%v", ...)`, ported in full (see this module's own doc -/// comment for the correction history). `None` for a non-finite float, and -/// for the two shapes only a hand-built `Value` can have (a `BigInt` that is -/// not a signed digit run, a `Map` with a repeated key) -- every other -/// `Value` variant, including nested `Seq`/`Map`, has a defined, -/// Go-matching display form. `pub(crate)`: the not-yet-built +/// comment for the correction history). `None` only for a non-finite +/// float -- every other `Value` variant, including nested `Seq`/`Map`, has +/// a defined, Go-matching display form. `pub(crate)`: the not-yet-built /// object-level walker's `resolvePath`-equivalent needs the exact same /// formatting for its own `{key=val}` path-segment matching (`conformance. /// go`'s `resolvePath` uses the identical `fmt.Sprintf("%v", em[key])`), @@ -138,7 +135,7 @@ pub(crate) fn go_display(v: &Value) -> Option { Value::Null => Some("".to_string()), Value::Bool(b) => Some(if *b { "true" } else { "false" }.to_string()), Value::Int(i) => Some(i.to_string()), - Value::BigInt(s) => canonical_integer(s), + Value::BigInt(n) => Some(n.canonical()), Value::Float(f) => go_float_display(*f), Value::Str(s) => Some(s.clone()), Value::Seq(items) => { @@ -151,15 +148,11 @@ pub(crate) fn go_display(v: &Value) -> Option { Value::Map(m) => { // Go's `fmt` package has sorted map keys for deterministic // `%v` output since Go 1.12 (verified empirically) -- A2's - // byte-wise sort, reused, not a second rule. A repeated key - // (impossible in a Go map) has no display form. - let mut entries: Vec<&(String, Value)> = m.0.iter().collect(); - entries.sort_by(|a, b| a.0.cmp(&b.0)); - if entries.windows(2).any(|w| w[0].0 == w[1].0) { - return None; - } + // byte-wise sort, reused, not a second rule. + let mut entries: Vec<(&str, &Value)> = m.iter().collect(); + entries.sort_unstable_by(|a, b| a.0.cmp(b.0)); let mut parts = Vec::with_capacity(entries.len()); - for (k, val) in entries.into_iter().map(|e| (&e.0, &e.1)) { + for (k, val) in entries { parts.push(format!("{k}:{}", go_display(val)?)); } Some(format!("map[{}]", parts.join(" "))) diff --git a/engine/src/conformance.rs b/engine/src/conformance.rs index 51ce153..da5ed7d 100644 --- a/engine/src/conformance.rs +++ b/engine/src/conformance.rs @@ -34,7 +34,7 @@ //! verified against real Go output before being encoded — see that //! function's own doc comment for what each round found and fixed. -use crate::canonical::{canonical_float, canonical_integer, to_canonical_json}; +use crate::canonical::{canonical_float, to_canonical_json}; use crate::reader::Value; use num_bigint::BigInt; use num_rational::BigRational; @@ -192,15 +192,10 @@ fn compare_exact(intent: &Value, observed: &Value) -> (bool, bool) { if i_num != o_num { return (false, false); // number vs non-number -> not comparable } - // A hand-built `BigInt` that is not a digit run has no canonical - // form: not comparable, rather than a panic or a guess. - return match ( - canonical_number_string(intent), - canonical_number_string(observed), - ) { - (Some(a), Some(b)) => (a == b, true), - _ => (false, false), - }; + return ( + canonical_number_string(intent) == canonical_number_string(observed), + true, + ); } if json_kind(intent) != json_kind(observed) { return (false, false); @@ -211,14 +206,12 @@ fn compare_exact(intent: &Value, observed: &Value) -> (bool, bool) { /// Canonical decimal form of a numeric `Value`, reusing section A's own /// number canonicalization (`canonical.rs`) rather than a second /// implementation — the same digits `to_canonical_json` would write for -/// this value, without re-serializing the whole tree. `None` only for a -/// `BigInt` whose text is not a signed digit run, which the reader never -/// builds but a caller of the public `Value` can. -fn canonical_number_string(v: &Value) -> Option { +/// this value, without re-serializing the whole tree. +fn canonical_number_string(v: &Value) -> String { match v { - Value::Int(i) => Some(i.to_string()), - Value::BigInt(s) => canonical_integer(s), - Value::Float(f) => Some(canonical_float(*f)), + Value::Int(i) => i.to_string(), + Value::BigInt(n) => n.canonical(), + Value::Float(f) => canonical_float(*f), _ => unreachable!("canonical_number_string is only called where is_numeric_value(v)"), } } @@ -243,7 +236,9 @@ fn canonical_equal(a: &Value, b: &Value) -> bool { fn as_rational(v: &Value) -> Option { match v { Value::Int(i) => Some(BigRational::from_integer(BigInt::from(*i))), - Value::BigInt(s) => BigInt::from_str(s).ok().map(BigRational::from_integer), + Value::BigInt(n) => BigInt::from_str(n.as_str()) + .ok() + .map(BigRational::from_integer), Value::Float(f) => BigRational::from_f64(*f), Value::Str(s) => rat_from_string(s), Value::Null | Value::Bool(_) | Value::Seq(_) | Value::Map(_) => None, diff --git a/engine/src/digest_projection.rs b/engine/src/digest_projection.rs index bf1705d..1407e13 100644 --- a/engine/src/digest_projection.rs +++ b/engine/src/digest_projection.rs @@ -250,7 +250,7 @@ mod tests { }; let mut fields = BTreeMap::new(); let mut keys = BTreeSet::new(); - for (k, val) in m.0 { + for (k, val) in m { keys.insert(k.clone()); let evidence = evidence_of(&k, val); fields.insert( diff --git a/engine/src/error.rs b/engine/src/error.rs index 2390b0d..2763e8d 100644 --- a/engine/src/error.rs +++ b/engine/src/error.rs @@ -124,13 +124,14 @@ pub mod code { /// Raised at `Stage::Materialize`. pub const INCOMPLETE_OBJECT: &str = "E_INCOMPLETE_OBJECT"; - /// A [`crate::reader::Value`] built by hand (not by the reader) breaks an - /// invariant the reader guarantees: a `BigInt` that is not a signed digit - /// run, or a `Map` holding the same key twice. Raised by `canonical` - /// (`R-CANON-BIGINT-DIGITS`, `R-CANON-UNIQUE-KEYS`) instead of panicking - /// or writing invalid canonical JSON. Not expressible as a YAML vector -- - /// the reader cannot produce either shape -- so the raisers are covered - /// by unit tests in `canonical.rs`. + /// A [`crate::reader::Value`] part built by hand that breaks an invariant + /// the reader guarantees: a `BigInt` that is not a signed digit run + /// (`R-VALUE-BIGINT-DIGITS`, raised by `BigIntValue::try_new`) or a `Map` + /// holding the same key twice (`R-VALUE-UNIQUE-KEYS`, raised by + /// `Map::from_pairs`). These are constructor errors: the types have no + /// other way in, so the canonicalizer and comparators never see either + /// shape. Covered by unit tests in `engine/tests/hand_built_values.rs` + /// (not a YAML vector -- the reader cannot produce them). pub const MALFORMED_VALUE: &str = "E_MALFORMED_VALUE"; } diff --git a/engine/src/plan.rs b/engine/src/plan.rs index c290da7..cb01248 100644 --- a/engine/src/plan.rs +++ b/engine/src/plan.rs @@ -506,7 +506,7 @@ mod tests { }; let mut fields = BTreeMap::new(); let mut keys = BTreeSet::new(); - for (k, val) in m.0 { + for (k, val) in m { keys.insert(k.clone()); let evidence = evidence_of(&k, val); fields.insert( @@ -704,11 +704,7 @@ mod tests { let Value::Map(m) = &mut observed else { unreachable!() }; - let nics_idx = - m.0.iter() - .position(|(k, _)| k == "network_interfaces") - .unwrap(); - let Value::Seq(nics) = &mut m.0[nics_idx].1 else { + let Some(Value::Seq(nics)) = m.get_mut("network_interfaces") else { unreachable!() }; nics[1] = nic("nic-1", "prod-subnet-WRONG", &[]); @@ -736,8 +732,7 @@ mod tests { let Value::Map(m) = &mut observed else { unreachable!() }; - let idx = m.0.iter().position(|(k, _)| k == "memory_gb").unwrap(); - m.0[idx].1 = Value::Str("large".into()); + m.insert("memory_gb", Value::Str("large".into())); let obs = oci_observation(); let verdict = evaluate( diff --git a/engine/src/reader.rs b/engine/src/reader.rs index b7ec4ee..34c13c1 100644 --- a/engine/src/reader.rs +++ b/engine/src/reader.rs @@ -88,35 +88,159 @@ pub enum Value { /// Rust, parsing the raw scalar text directly (via `saphyr`'s own /// `early_parse(false)` loader mode) rather than trusting `saphyr`'s /// default eager resolution to i64/f64. - BigInt(String), + BigInt(BigIntValue), Float(f64), Str(String), Seq(Vec), Map(Map), } +/// An integer literal that does not fit in `i64`, as its validated text: an +/// optional `+`/`-` followed by one or more ASCII digits, nothing else. +/// +/// The field is private and [`BigIntValue::try_new`] is the only way in +/// (besides the reader, which goes through it), so every `BigIntValue` that +/// exists satisfies the invariant the canonicalizer relies on -- it is not a +/// convention a caller can break. Sign folding, leading-zero stripping and +/// `-0` -> `0` still happen at canonicalization ([`BigIntValue::canonical`]). +/// +/// The value cannot be built around the check: +/// +/// ```compile_fail +/// use cic_primitive_engine::reader::{BigIntValue, Value}; +/// let _ = Value::BigInt(BigIntValue("not-a-number".to_string())); +/// ``` +/// +/// ``` +/// use cic_primitive_engine::reader::{BigIntValue, Value}; +/// assert!(BigIntValue::try_new("not-a-number").is_err()); +/// let big = BigIntValue::try_new("9223372036854775808").unwrap(); +/// let _ = Value::BigInt(big); +/// ``` +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct BigIntValue(String); + +impl BigIntValue { + /// # Errors + /// `E_MALFORMED_VALUE` (`R-VALUE-BIGINT-DIGITS`) unless `s` is an + /// optional sign followed by one or more ASCII digits. + pub fn try_new(s: impl Into) -> Result { + let s = s.into(); + let digits = s.strip_prefix(['+', '-']).unwrap_or(&s); + if digits.is_empty() || !digits.bytes().all(|b| b.is_ascii_digit()) { + return Err(Error::new( + code::MALFORMED_VALUE, + "R-VALUE-BIGINT-DIGITS", + Stage::Read, + "$", + format!("{s:?} is not an optional sign followed by ASCII digits"), + )); + } + Ok(Self(s)) + } + + /// The validated text, exactly as given (not yet normalized). + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } + + /// A3's normal form, ported from `cic-canonical`'s own `canonical_integer` + /// (not imported as a dependency): sign folded, leading zeros stripped, + /// `-0`/`+0` -> `0` -- the same normalization Go's `number.go` applies + /// via `big.Int.SetString(s, 10).String()`, verified empirically against + /// it (`"000000123"` -> `"123"`, `"-00000123"` -> `"-123"`, `"-0"`/`"+0"` + /// -> `"0"`). Infallible because the text is a digit run by construction. + #[must_use] + pub fn canonical(&self) -> String { + let (neg, digits) = match self.0.strip_prefix('-') { + Some(rest) => (true, rest), + None => (false, self.0.strip_prefix('+').unwrap_or(&self.0)), + }; + let magnitude = digits.trim_start_matches('0'); + if magnitude.is_empty() { + "0".to_string() // all zeros, including "-0"/"+0" + } else if neg { + format!("-{magnitude}") + } else { + magnitude.to_string() + } + } +} + +/// An insertion-ordered string-keyed mapping whose keys are unique. +/// +/// The storage is private: a `Map` is built by the reader, by +/// [`Map::from_pairs`] (which refuses a repeated key) or by [`Map::insert`] +/// (which replaces), so no `Map` outside this crate can hold the same key +/// twice -- the canonicalizer and the comparators rely on that. +/// +/// ```compile_fail +/// use cic_primitive_engine::reader::{Map, Value}; +/// let _ = Map(vec![("x".to_string(), Value::Int(1)), ("x".to_string(), Value::Int(2))]); +/// ``` +/// +/// ``` +/// use cic_primitive_engine::reader::{Map, Value}; +/// let pairs = vec![("x".to_string(), Value::Int(1)), ("x".to_string(), Value::Int(2))]; +/// assert!(Map::from_pairs(pairs).is_err()); +/// ``` #[derive(Debug, Clone, Default, PartialEq)] -pub struct Map(pub Vec<(String, Value)>); +pub struct Map(Vec<(String, Value)>); impl Map { + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Build a map from pairs, keeping their order. + /// + /// # Errors + /// `E_MALFORMED_VALUE` (`R-VALUE-UNIQUE-KEYS`) if a key occurs twice. + pub fn from_pairs(pairs: impl IntoIterator) -> Result { + let pairs: Vec<(String, Value)> = pairs.into_iter().collect(); + let mut seen = HashSet::with_capacity(pairs.len()); + for (k, _) in &pairs { + if !seen.insert(k.as_str()) { + return Err(Error::new( + code::MALFORMED_VALUE, + "R-VALUE-UNIQUE-KEYS", + Stage::Read, + format!("$.{k}"), + format!("the key {k:?} occurs more than once"), + )); + } + } + Ok(Self(pairs)) + } + #[must_use] pub fn get(&self, k: &str) -> Option<&Value> { self.0.iter().find(|(key, _)| key == k).map(|(_, v)| v) } #[must_use] + pub fn get_mut(&mut self, k: &str) -> Option<&mut Value> { + self.0.iter_mut().find(|(key, _)| key == k).map(|(_, v)| v) + } + #[must_use] pub fn contains(&self, k: &str) -> bool { self.get(k).is_some() } - /// Append without looking for an existing key. + /// Append a key the caller knows is new, without the replace-or-append + /// search `insert` does. /// - /// `insert` searches the whole vector on every call, so building an n-key - /// mapping with it costs `n(n-1)/2` comparisons — the second quadratic in - /// this file, and the one that remained after the duplicate-key scan - /// stopped being the first. The parser can append safely because - /// `scan_input` has already refused duplicates, so there is nothing to - /// overwrite. - pub fn push(&mut self, k: impl Into, v: Value) { - self.0.push((k.into(), v)); + /// Crate-private: "known to be new" is the caller's claim, and it is + /// checked here in every build profile (`assert!`, not `debug_assert!`), + /// so a wrong claim stops at the call instead of leaving a `Map` with a + /// repeated key. The check is linear per call, which is why the reader -- + /// the one caller that can see arbitrarily many untrusted keys -- builds + /// through [`Map::from_pairs`] instead. Remaining callers add a handful of + /// literal keys. + pub(crate) fn push(&mut self, k: impl Into, v: Value) { + let k = k.into(); + assert!(!self.contains(&k), "Map::push with an existing key {k:?}"); + self.0.push((k, v)); } pub fn insert(&mut self, k: impl Into, v: Value) { @@ -127,6 +251,10 @@ impl Map { self.0.push((k, v)); } } + /// The entries in insertion order. + pub fn iter(&self) -> impl Iterator { + self.0.iter().map(|(k, v)| (k.as_str(), v)) + } #[must_use] pub fn keys(&self) -> Vec<&str> { self.0.iter().map(|(k, _)| k.as_str()).collect() @@ -141,6 +269,14 @@ impl Map { } } +impl IntoIterator for Map { + type Item = (String, Value); + type IntoIter = std::vec::IntoIter<(String, Value)>; + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} + impl Value { #[must_use] pub fn as_map(&self) -> Option<&Map> { @@ -170,7 +306,7 @@ impl Value { Value::Null => "null".into(), Value::Bool(b) => b.to_string(), Value::Int(i) => i.to_string(), - Value::BigInt(s) => s.clone(), + Value::BigInt(n) => n.as_str().to_string(), Value::Float(f) => f.to_string(), Value::Str(s) => s.clone(), Value::Seq(_) => "".into(), @@ -387,7 +523,7 @@ fn convert(y: &Yaml, stage: Stage, path: &str, what: &str) -> Result { .collect::>>()?, )), Yaml::Mapping(m) => { - let mut out = Map::default(); + let mut pairs: Vec<(String, Value)> = Vec::with_capacity(m.len()); for (k, v) in m { // Non-string keys are refused rather than stringified. YAML // allows `1:` and `true:`, and coercing them would let two @@ -416,9 +552,25 @@ fn convert(y: &Yaml, stage: Stage, path: &str, what: &str) -> Result { format!("{what} has a non-string mapping key; node names are strings"), )); }; - out.push(key, convert(v, stage, path, what)?); + pairs.push((key, convert(v, stage, path, what)?)); } - Ok(Value::Map(out)) + // `scan_input` has already refused every spelling of a repeated + // key (plain/quoted/escaped/flow/explicit/tagged), and saphyr's own + // mapping type cannot hold one, so this check does not fire today. + // It is here so the uniqueness of a `Map` the reader hands out + // rests on the type's own constructor in every build profile, not + // on that scan plus a debug-only assertion. Linear, unlike a + // per-key `contains`. + let map = Map::from_pairs(pairs).map_err(|e| { + Error::new( + code::MALFORMED_DOCUMENT, + "R-READ-DUPLICATE-KEY", + stage, + path, + format!("{what}: {}", e.detail), + ) + })?; + Ok(Value::Map(map)) } // Aliases are already refused at the event-scan stage. A tagged // mapping/sequence (a custom, non-core-schema tag — `Yaml::Tagged`) @@ -501,7 +653,7 @@ fn resolve_scalar( if let Some(digits) = base10_integer_literal(text) { return Ok(match digits.parse::() { Ok(i) => Value::Int(i), - Err(_) => Value::BigInt(digits.to_string()), + Err(_) => Value::BigInt(BigIntValue::try_new(digits)?), }); } } @@ -558,11 +710,11 @@ mod tests { ("9223372036854775807", Value::Int(9_223_372_036_854_775_807)), ( "9223372036854775808", - Value::BigInt("9223372036854775808".to_string()), + Value::BigInt(BigIntValue::try_new("9223372036854775808").unwrap()), ), ( "9223372036854775809", - Value::BigInt("9223372036854775809".to_string()), + Value::BigInt(BigIntValue::try_new("9223372036854775809").unwrap()), ), ( "-9223372036854775808", @@ -570,7 +722,7 @@ mod tests { ), ( "-9223372036854775809", - Value::BigInt("-9223372036854775809".to_string()), + Value::BigInt(BigIntValue::try_new("-9223372036854775809").unwrap()), ), ("9007199254740992", Value::Int(9_007_199_254_740_992)), ("9007199254740993", Value::Int(9_007_199_254_740_993)), @@ -580,7 +732,9 @@ mod tests { ("-0", Value::Int(0)), ( "1234567890123456789012345678901234567890", - Value::BigInt("1234567890123456789012345678901234567890".to_string()), + Value::BigInt( + BigIntValue::try_new("1234567890123456789012345678901234567890").unwrap(), + ), ), ]; for (literal, want) in cases { @@ -623,7 +777,10 @@ mod tests { fn explicitly_tagged_int_also_keeps_exact_precision() { let doc = parse_doc("v: !!int 9223372036854775808"); let got = doc.as_map().and_then(|m| m.get("v")).expect("key `v`"); - assert_eq!(got, &Value::BigInt("9223372036854775808".to_string())); + assert_eq!( + got, + &Value::BigInt(BigIntValue::try_new("9223372036854775808").unwrap()) + ); let doc = parse_doc("v: !!int 123"); let got = doc.as_map().and_then(|m| m.get("v")).expect("key `v`"); @@ -645,6 +802,14 @@ mod tests { // The existing conformance/reader/ vectors only check accept/reject, // not the resulting tree -- re-verified here that ordinary documents + #[test] + #[should_panic(expected = "Map::push with an existing key")] + fn push_refuses_a_repeated_key_in_every_profile() { + let mut m = Map::default(); + m.push("a", Value::Int(1)); + m.push("a", Value::Int(2)); + } + // still parse to the exact same Value shape as before this rewrite. #[test] fn ordinary_documents_unaffected() { diff --git a/engine/tests/hand_built_values.rs b/engine/tests/hand_built_values.rs deleted file mode 100644 index ffa4be9..0000000 --- a/engine/tests/hand_built_values.rs +++ /dev/null @@ -1,180 +0,0 @@ -//! `Value` is public, so a caller can build shapes the reader never produces. -//! Nothing reachable through the public API may panic on them or write bytes -//! that are not valid canonical JSON (#52). -//! -//! These cannot be conformance-corpus vectors: the corpus is YAML in, and the -//! reader cannot produce either shape. - -use cic_primitive_engine::reader::{Map, Value}; -use cic_primitive_engine::{ - classify_field_value, code, compare, digest, to_canonical_json, Collection, CollectionTopology, - CompareType, Coverage, FieldVerdict, Stage, -}; - -fn big(s: &str) -> Value { - Value::BigInt(s.to_string()) -} - -fn dup_map() -> Value { - Value::Map(Map(vec![ - ("x".into(), Value::Int(1)), - ("x".into(), Value::Int(2)), - ])) -} - -/// Not a signed digit run: each of these used to hit `expect(...)`. -const BAD_BIGINTS: &[&str] = &[ - "", - "+", - "-", - "not-a-number", - "12 3", - "1e5", - "1.5", - "0x10", - "--1", - "٣", -]; - -#[test] -fn malformed_bigint_is_an_error_not_a_panic() { - for s in BAD_BIGINTS { - let err = to_canonical_json(&big(s)).expect_err(s); - assert_eq!(err.code, code::MALFORMED_VALUE, "{s:?}"); - assert_eq!(err.rule, "R-CANON-BIGINT-DIGITS", "{s:?}"); - assert_eq!(err.stage, Stage::Canonicalize, "{s:?}"); - assert_eq!(err.path, "$", "{s:?}"); - } -} - -#[test] -fn malformed_bigint_error_carries_the_nested_path() { - let v = Value::Map(Map(vec![( - "a".into(), - Value::Seq(vec![Value::Int(1), big("nope")]), - )])); - let err = to_canonical_json(&v).expect_err("nested"); - assert_eq!(err.code, code::MALFORMED_VALUE); - assert!(err.path.starts_with("$.a"), "path was {}", err.path); -} - -#[test] -fn well_formed_bigints_still_canonicalize() { - for (input, want) in [ - ("9223372036854775808", "9223372036854775808"), - ("-9223372036854775809", "-9223372036854775809"), - ("+000123", "123"), - ("-0", "0"), - ] { - let got = to_canonical_json(&big(input)).expect(input); - assert_eq!(String::from_utf8(got).unwrap(), want, "{input}"); - } -} - -#[test] -fn duplicate_key_is_an_error_not_invalid_json() { - let err = to_canonical_json(&dup_map()).expect_err("duplicate key"); - assert_eq!(err.code, code::MALFORMED_VALUE); - assert_eq!(err.rule, "R-CANON-UNIQUE-KEYS"); - assert_eq!(err.stage, Stage::Canonicalize); - assert_eq!(err.path, "$.x"); -} - -#[test] -fn duplicate_key_is_found_wherever_it_sits() { - // Not adjacent in insertion order, and nested inside a sequence. - let inner = Value::Map(Map(vec![ - ("a".into(), Value::Int(1)), - ("b".into(), Value::Int(2)), - ("a".into(), Value::Int(3)), - ])); - let v = Value::Map(Map(vec![( - "k".into(), - Value::Seq(vec![Value::Null, inner]), - )])); - let err = to_canonical_json(&v).expect_err("nested duplicate"); - assert_eq!(err.rule, "R-CANON-UNIQUE-KEYS"); - assert!(err.path.ends_with(".a"), "path was {}", err.path); -} - -#[test] -fn unique_keys_are_still_sorted_bytewise() { - let v = Value::Map(Map(vec![ - ("b".into(), Value::Int(2)), - ("a".into(), Value::Int(1)), - ("B".into(), Value::Int(0)), - ])); - let got = String::from_utf8(to_canonical_json(&v).unwrap()).unwrap(); - assert_eq!(got, r#"{"B":0,"a":1,"b":2}"#); -} - -#[test] -fn digest_input_can_only_come_from_valid_canonical_json() { - // `digest` takes bytes, so the guard is that no bytes are produced at all. - assert!(to_canonical_json(&dup_map()).is_err()); - assert!(to_canonical_json(&big("x")).is_err()); - assert!(digest(b"{}").starts_with("sha256:")); -} - -#[test] -fn compare_does_not_panic_on_a_malformed_bigint() { - let good = Value::Int(5); - for s in BAD_BIGINTS { - let bad = big(s); - // Exact: a malformed number has no canonical form -> not comparable. - assert_eq!( - compare(&bad, &good, CompareType::Exact), - (false, false), - "{s:?}" - ); - assert_eq!( - compare(&good, &bad, CompareType::Exact), - (false, false), - "{s:?}" - ); - assert_eq!( - compare(&bad, &bad, CompareType::Exact), - (false, false), - "{s:?}" - ); - // Numeric already failed closed (BigInt::from_str). - assert_eq!( - compare(&bad, &good, CompareType::Numeric), - (false, false), - "{s:?}" - ); - } -} - -#[test] -fn classification_does_not_panic_on_hand_built_values() { - let v = classify_field_value( - Coverage::Observed, - true, - &big("nope"), - &Value::Int(1), - CompareType::Exact, - ); - assert_eq!(v, FieldVerdict::NotComparable); - // Two equal duplicate-key maps are not "conformant": they have no canonical form. - let v = classify_field_value( - Coverage::Observed, - true, - &dup_map(), - &dup_map(), - CompareType::Exact, - ); - assert_ne!(v, FieldVerdict::Conformant); -} - -#[test] -fn set_element_key_does_not_panic_on_hand_built_values() { - let set = Collection { - topology: CollectionTopology::Set, - keys: vec![], - }; - // No display form -> the empty identity, the same fallback a non-finite float gets. - assert_eq!(set.element_key(&big("nope")), ""); - assert_eq!(set.element_key(&dup_map()), ""); - assert_eq!(set.element_key(&Value::Seq(vec![dup_map()])), ""); -} diff --git a/engine/tests/value_invariants.rs b/engine/tests/value_invariants.rs new file mode 100644 index 0000000..387ee87 --- /dev/null +++ b/engine/tests/value_invariants.rs @@ -0,0 +1,149 @@ +//! `Value` is public, so what a caller can build is part of the engine's +//! contract. The shapes the reader never produces -- a `BigInt` that is not a +//! digit run, a `Map` with a repeated key -- are refused by the constructors +//! (#53; #52 had made the canonicalizer fail closed on them first). Everything +//! below goes through the public API only. +//! +//! "Cannot be built around the constructor" is pinned by the `compile_fail` +//! doctests on `BigIntValue` and `Map` in `reader.rs`. None of this can be a +//! conformance-corpus vector: the corpus is YAML in, and the reader cannot +//! produce these shapes. + +use cic_primitive_engine::reader::{self, BigIntValue, Map, Value}; +use cic_primitive_engine::{ + classify_field_value, code, compare, to_canonical_json, Collection, CollectionTopology, + CompareType, Coverage, FieldVerdict, Stage, +}; + +fn big(s: &str) -> Value { + Value::BigInt(BigIntValue::try_new(s).expect(s)) +} + +fn pairs(kv: &[(&str, i64)]) -> Vec<(String, Value)> { + kv.iter() + .map(|(k, v)| ((*k).to_string(), Value::Int(*v))) + .collect() +} + +fn canon(v: &Value) -> String { + String::from_utf8(to_canonical_json(v).expect("canonical")).unwrap() +} + +/// Not a signed digit run: each of these used to reach an `expect(...)`. +const BAD_BIGINTS: &[&str] = &[ + "", + "+", + "-", + "not-a-number", + "12 3", + "1e5", + "1.5", + "0x10", + "--1", + "+-1", + "٣", + "1\n", +]; + +#[test] +fn bigint_constructor_rejects_anything_but_a_signed_digit_run() { + for s in BAD_BIGINTS { + let err = BigIntValue::try_new(*s).expect_err(s); + assert_eq!(err.code, code::MALFORMED_VALUE, "{s:?}"); + assert_eq!(err.rule, "R-VALUE-BIGINT-DIGITS", "{s:?}"); + assert_eq!(err.stage, Stage::Read, "{s:?}"); + } +} + +#[test] +fn bigint_constructor_accepts_and_normalizes_digit_runs() { + for (input, want) in [ + ("9223372036854775808", "9223372036854775808"), + ("-9223372036854775809", "-9223372036854775809"), + ("+000123", "123"), + ("-000123", "-123"), + ("-0", "0"), + ("+0", "0"), + ("0000", "0"), + ] { + let n = BigIntValue::try_new(input).expect(input); + assert_eq!(n.as_str(), input, "as_str keeps the text as given"); + assert_eq!(n.canonical(), want, "{input}"); + assert_eq!(canon(&Value::BigInt(n)), want, "{input}"); + } +} + +#[test] +fn from_pairs_rejects_a_repeated_key_wherever_it_sits() { + for kv in [ + &[("x", 1), ("x", 2)][..], + &[("a", 1), ("b", 2), ("a", 3)][..], // not adjacent in insertion order + &[("a", 1), ("b", 2), ("c", 3), ("c", 4), ("b", 5)][..], + ] { + let err = Map::from_pairs(pairs(kv)).expect_err("duplicate"); + assert_eq!(err.code, code::MALFORMED_VALUE); + assert_eq!(err.rule, "R-VALUE-UNIQUE-KEYS"); + assert_eq!(err.stage, Stage::Read); + assert!(err.path.starts_with("$."), "path was {}", err.path); + } +} + +#[test] +fn from_pairs_keeps_unique_keys_in_insertion_order() { + let m = Map::from_pairs(pairs(&[("b", 2), ("a", 1), ("B", 0)])).unwrap(); + assert_eq!(m.keys(), ["b", "a", "B"]); + assert_eq!(m.len(), 3); + // Canonical output is sorted bytewise regardless of insertion order. + assert_eq!(canon(&Value::Map(m)), r#"{"B":0,"a":1,"b":2}"#); + assert!(Map::from_pairs(Vec::new()).unwrap().is_empty()); +} + +#[test] +fn insert_replaces_so_uniqueness_survives_mutation() { + let mut m = Map::new(); + m.insert("x", Value::Int(1)); + m.insert("x", Value::Int(2)); + m.insert("y", Value::Int(3)); + assert_eq!(m.len(), 2); + assert_eq!(canon(&Value::Map(m)), r#"{"x":2,"y":3}"#); +} + +#[test] +fn reader_output_satisfies_both_invariants() { + let doc = "big: 9223372036854775808\nneg: -00000000000000000000000000000000000001\nn:\n a: 1\n b: 2\n"; + let v = reader::parse(doc.as_bytes(), Stage::Read, "$", "test").expect("parse"); + assert_eq!( + canon(&v), + r#"{"big":9223372036854775808,"n":{"a":1,"b":2},"neg":-1}"# + ); + // A duplicate key never reaches the type: the reader refuses the document. + let dup = reader::parse(b"x: 1\nx: 2\n", Stage::Read, "$", "test").expect_err("duplicate"); + assert_eq!(dup.code, code::MALFORMED_DOCUMENT); +} + +#[test] +fn comparison_and_classification_handle_every_constructible_bigint() { + let five = Value::Int(5); + let huge = big("9223372036854775808"); + assert_eq!(compare(&huge, &huge, CompareType::Exact), (true, true)); + assert_eq!( + compare(&big("+0009223372036854775808"), &huge, CompareType::Exact), + (true, true) + ); + assert_eq!(compare(&huge, &five, CompareType::Exact), (false, true)); + assert_eq!(compare(&huge, &five, CompareType::Numeric), (false, true)); + assert_eq!( + classify_field_value(Coverage::Observed, true, &huge, &huge, CompareType::Exact), + FieldVerdict::Conformant + ); +} + +#[test] +fn set_element_key_uses_the_canonical_bigint_form() { + let set = Collection { + topology: CollectionTopology::Set, + keys: vec![], + }; + assert_eq!(set.element_key(&big("+0001")), "1"); + assert_eq!(set.element_key(&big("-0")), "0"); +}