From f2c446253bf7889c52074a115032586463cbd78e Mon Sep 17 00:00:00 2001 From: Robert Bartel Date: Thu, 25 Jun 2026 15:13:14 +0000 Subject: [PATCH 1/2] Fix ExtFloat negate -0.0 canonicalization (deferred item closed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Op::Negate's ExtFloat arm pushed `ExtFloat(-n)` without folding -0.0 -> +0.0, unlike every other ExtFloat producer (Div in arithmetic.rs, NarrowFloat, the numeric builtins). ExtFloat Eq/Ord/Hash are bitwise (value.rs), so a negated zero became a distinct container key from +0.0 even though the IEEE `==` operator calls them equal — letting a Set/Map hold two "equal" elements. Repro (pre-fix): `let p = 0.0/1.0; let n = -p` then `set.from_list([p,n])` has length 2 while `p == n` is true. Post-fix the set collapses to 1. This was the lone hole in the codebase's "an ExtFloat never holds -0.0" invariant; closing it needs no design decision (the policy was already chosen everywhere else). The broader question — whether silt's Float type should expose -0.0 at all — remains intentionally deferred. Lock: tests/extfloat_negate_neg_zero_canonicalization_tests.rs - negated_extfloat_zero_is_a_single_set_key (runtime, from_list keying + ==) - inserting_both_zero_signs_dedups (runtime, BTreeSet::insert path) - execute_rs_canonicalizes_negated_extfloat_zero (source-grep guard) cargo build + full cargo test green (5405 passed, 0 failed); fmt + clippy clean. --- src/vm/execute.rs | 10 +- ..._negate_neg_zero_canonicalization_tests.rs | 139 ++++++++++++++++++ 2 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 tests/extfloat_negate_neg_zero_canonicalization_tests.rs diff --git a/src/vm/execute.rs b/src/vm/execute.rs index 3f3c747..2908a27 100644 --- a/src/vm/execute.rs +++ b/src/vm/execute.rs @@ -1368,7 +1368,15 @@ impl Vm { self.push(Value::Float(result)); } Value::ExtFloat(n) => { - self.push(Value::ExtFloat(-n)); + // Canonicalize -0.0 -> +0.0 to match every other + // ExtFloat producer (Div in arithmetic.rs, NarrowFloat, + // the numeric builtins). ExtFloat Eq/Ord/Hash are + // *bitwise* (value.rs), so a stray ExtFloat(-0.0) would + // be a distinct container key from ExtFloat(+0.0) even + // though the `==` operator (IEEE) calls them equal — + // letting a set/map hold two "equal" elements. + let result = -n; + self.push(Value::ExtFloat(if result == 0.0 { 0.0 } else { result })); } other => { return Err(VmError::new(format!( diff --git a/tests/extfloat_negate_neg_zero_canonicalization_tests.rs b/tests/extfloat_negate_neg_zero_canonicalization_tests.rs new file mode 100644 index 0000000..dcf72bd --- /dev/null +++ b/tests/extfloat_negate_neg_zero_canonicalization_tests.rs @@ -0,0 +1,139 @@ +//! Lock: negating an `ExtFloat` must canonicalize `-0.0 -> +0.0`, the same +//! as every other `ExtFloat` producer (`Div` in `src/vm/arithmetic.rs`, +//! `NarrowFloat` and the numeric builtins). +//! +//! Why it matters — silt runs two equality notions over floats: +//! - the user-facing `==` operator widens to IEEE, so `-0.0 == +0.0` is true; +//! - container keys (`Value::Set` is a `BTreeSet`, `Value::Map` a +//! `BTreeMap`) use the *bitwise* `Ord`/`Hash` in `src/value.rs`, under +//! which `-0.0` and `+0.0` are DISTINCT. +//! `-0.0` is the only finite value where the two disagree, so the codebase +//! keeps the invariant "an `ExtFloat` never holds `-0.0`" by canonicalizing at +//! every producer. `Op::Negate`'s `ExtFloat` arm (`src/vm/execute.rs`) was the +//! lone hole: `-(0.0 / 1.0)` yielded `ExtFloat(-0.0)` while `0.0 / 1.0` yields +//! `ExtFloat(+0.0)` — `==`-equal yet distinct set keys, so a set built from +//! both held TWO elements. Pre-fix this test sees length 2; post-fix, 1. +//! +//! Drives the `silt` CLI end-to-end (lexer -> parser -> typechecker -> +//! compiler -> VM) so the lock exercises the real runtime path, not a +//! typecheck-only tautology. + +use std::process::Command; + +const EXECUTE_RS: &str = include_str!("../src/vm/execute.rs"); + +fn run_silt_raw(label: &str, src: &str) -> (String, String, bool) { + let tmp = std::env::temp_dir().join(format!("silt_extfloat_negzero_{label}.silt")); + std::fs::write(&tmp, src).expect("write temp file"); + let bin = env!("CARGO_BIN_EXE_silt"); + let out = Command::new(bin) + .arg("run") + .arg(&tmp) + .output() + .expect("spawn silt run"); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); + (stdout, stderr, out.status.success()) +} + +fn run_silt_ok(label: &str, src: &str) -> String { + let (stdout, stderr, ok) = run_silt_raw(label, src); + assert!( + ok, + "silt run should succeed for {label}; stdout={stdout}, stderr={stderr}" + ); + stdout +} + +/// Behavioral lock: a negated zero and a plain zero are `==`-equal AND collapse +/// to a single set element. `0.0 / 1.0` widens `Float / Float` to `ExtFloat`, +/// and `-pos` walks `Op::Negate`'s `ExtFloat` arm at runtime (no const-folding +/// across the `let`). Both `set.from_list` keying and `==` must agree. +#[test] +fn negated_extfloat_zero_is_a_single_set_key() { + let out = run_silt_ok( + "set_collapse", + r#" +import set + +fn main() { + let pos = 0.0 / 1.0 + let neg = -pos + println(pos == neg) + let s = set.from_list([pos, neg]) + println(set.length(s)) +} +"#, + ); + let lines: Vec<&str> = out.lines().collect(); + assert!(lines.len() >= 2, "expected 2 lines; got {out:?}"); + assert_eq!( + lines[0], "true", + "`-0.0 == +0.0` must hold under the IEEE `==` operator; got {:?}", + lines[0] + ); + assert_eq!( + lines[1], "1", + "a set of `==`-equal floats must hold ONE element — the negated zero \ + leaked in as a distinct bitwise key (ExtFloat(-0.0) vs ExtFloat(+0.0)); \ + got {:?}", + lines[1] + ); +} + +/// `insert`-then-`insert` of the two zeros must likewise dedup, exercising the +/// `BTreeSet::insert` path rather than the bulk `from_list` collect. +#[test] +fn inserting_both_zero_signs_dedups() { + let out = run_silt_ok( + "set_insert", + r#" +import set + +fn main() { + let pos = 0.0 / 1.0 + let neg = -pos + let s0 = set.new() + let s1 = set.insert(s0, pos) + let s2 = set.insert(s1, neg) + println(set.length(s2)) + println(set.contains(s2, neg)) +} +"#, + ); + let lines: Vec<&str> = out.lines().collect(); + assert!(lines.len() >= 2, "expected 2 lines; got {out:?}"); + assert_eq!( + lines[0], "1", + "negated zero must dedup against +0.0 on insert" + ); + assert_eq!( + lines[1], "true", + "the negated zero must be found in the set" + ); +} + +/// Source-grep guard: the `ExtFloat` negate arm must keep canonicalizing zero. +/// A bare `Value::ExtFloat(-n)` (no `== 0.0` fold) reintroduces the bug. +#[test] +fn execute_rs_canonicalizes_negated_extfloat_zero() { + // Find the ExtFloat arm of Op::Negate and assert it guards on `== 0.0`. + let negate_idx = EXECUTE_RS + .find("Op::Negate") + .expect("Op::Negate handler present in execute.rs"); + let ext_rel = EXECUTE_RS[negate_idx..] + .find("Value::ExtFloat(n) =>") + .expect("ExtFloat arm present under Op::Negate"); + let ext_idx = negate_idx + ext_rel; + // Scan from the arm header to the next match arm (`other =>`), so the + // guard check is scoped to this arm's body regardless of comment length. + let body = &EXECUTE_RS[ext_idx..]; + let arm = &body[..body.find("other =>").unwrap_or(body.len().min(800))]; + assert!( + arm.contains("== 0.0"), + "Op::Negate's ExtFloat arm no longer canonicalizes -0.0 -> +0.0 \ + (missing the `== 0.0` guard). A raw `ExtFloat(-n)` lets a negated \ + zero become a distinct bitwise set/map key — see this test's module \ + doc." + ); +} From 5b4d27ab9abcd332587995b55f7b36d25976dce3 Mon Sep 17 00:00:00 2001 From: Robert Bartel Date: Fri, 26 Jun 2026 06:14:15 +0000 Subject: [PATCH 2/2] Fix audit findings: ExtFloat mixed-arithmetic -0.0 canonicalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BROKEN: the collapsed mixed (Float | ExtFloat, Float | ExtFloat) arithmetic arm pushed a raw `ExtFloat(a OP b)` for Add/Sub/Mul/Div/Mod without folding -0.0 -> +0.0, unlike the dedicated (Float, Float) Div arm, Op::Negate, NarrowFloat, and the numeric builtins. ExtFloat Eq/Ord/Hash are *bitwise* (value.rs), so a computed ExtFloat(-0.0) becomes a distinct Set/Map key from ExtFloat(+0.0) even though the IEEE `==` operator calls them equal — letting a container hold two "equal" elements and breaking the codebase's "an ExtFloat never holds -0.0" invariant. File: src/vm/arithmetic.rs:72-82 (binary_arithmetic mixed float arm) Test: tests/extfloat_mixed_arithmetic_neg_zero_canonicalization_tests.rs - mixed_mul_negative_zero_is_a_single_set_key (runtime: (-1.0)*(0.0/1.0) keying + ==) - mixed_div_negative_zero_dedups_on_insert (runtime: (-1.0)/(1.0/0.0) BTreeSet::insert + contains) - arithmetic_rs_canonicalizes_mixed_arm_zero (source-grep guard on the `== 0.0` fold) Repro: `let z = 0.0/1.0; let n = (-1.0)*z; let p = 1.0/1.0/1.0*0.0;` then `set.length(set.from_list([p, n]))` is 2 pre-fix while `p == n` is true; post-fix the set collapses to 1. Prior-fix-gap: f2c4462 closed the Op::Negate hole and claimed it was the "lone hole" in the invariant, but missed this computed-result arm — the same bug class, different producer. cargo build + full cargo test green (5408 passed, 0 failed); fmt + clippy (--all-features -D warnings) + check --benches all clean. --- src/vm/arithmetic.rs | 25 +++- ...thmetic_neg_zero_canonicalization_tests.rs | 139 ++++++++++++++++++ 2 files changed, 157 insertions(+), 7 deletions(-) create mode 100644 tests/extfloat_mixed_arithmetic_neg_zero_canonicalization_tests.rs diff --git a/src/vm/arithmetic.rs b/src/vm/arithmetic.rs index 2d703fa..15514be 100644 --- a/src/vm/arithmetic.rs +++ b/src/vm/arithmetic.rs @@ -71,14 +71,25 @@ impl Vm { // of which side was a plain `Float`. (Value::Float(a) | Value::ExtFloat(a), Value::Float(b) | Value::ExtFloat(b)) => { let (a, b) = (*a, *b); - match op { - Op::Add => Value::ExtFloat(a + b), - Op::Sub => Value::ExtFloat(a - b), - Op::Mul => Value::ExtFloat(a * b), - Op::Div => Value::ExtFloat(a / b), - Op::Mod => Value::ExtFloat(a % b), + let result = match op { + Op::Add => a + b, + Op::Sub => a - b, + Op::Mul => a * b, + Op::Div => a / b, + Op::Mod => a % b, _ => unreachable!(), - } + }; + // Canonicalize -0.0 -> +0.0 to uphold the "an ExtFloat never + // holds -0.0" invariant, exactly as the `(Float, Float)` Div + // arm above (and Op::Negate, NarrowFloat, the numeric builtins) + // already do. ExtFloat Eq/Ord/Hash are *bitwise* (value.rs), so + // a stray ExtFloat(-0.0) — reachable here via e.g. `(-1.0) * + // (0.0 / 1.0)` or `(-1.0) / (1.0 / 0.0)` — would be a distinct + // container key from ExtFloat(+0.0) even though the IEEE `==` + // operator calls them equal, letting a set/map hold two "equal" + // elements. The dedicated finite `(Float, Float)` arm above + // means this only runs with at least one ExtFloat operand. + Value::ExtFloat(if result == 0.0 { 0.0 } else { result }) } (Value::String(a), Value::String(b)) if op == Op::Add => { Value::String(format!("{a}{b}")) diff --git a/tests/extfloat_mixed_arithmetic_neg_zero_canonicalization_tests.rs b/tests/extfloat_mixed_arithmetic_neg_zero_canonicalization_tests.rs new file mode 100644 index 0000000..0f806ab --- /dev/null +++ b/tests/extfloat_mixed_arithmetic_neg_zero_canonicalization_tests.rs @@ -0,0 +1,139 @@ +//! Lock: the mixed `Float`/`ExtFloat` arithmetic arm in +//! `src/vm/arithmetic.rs` must canonicalize `-0.0 -> +0.0`, the same as the +//! dedicated `(Float, Float)` `Div` arm, `Op::Negate`, `NarrowFloat`, and the +//! numeric builtins. +//! +//! Why it matters — silt runs two equality notions over floats: +//! - the user-facing `==` operator widens to IEEE, so `-0.0 == +0.0` is true; +//! - container keys (`Value::Set` is a `BTreeSet`, `Value::Map` a +//! `BTreeMap`) use the *bitwise* `Ord`/`Hash` in `src/value.rs`, under +//! which `-0.0` and `+0.0` are DISTINCT. +//! `-0.0` is the only finite value where the two disagree, so the codebase +//! keeps the invariant "an `ExtFloat` never holds `-0.0`" by canonicalizing at +//! every producer. Commit f2c4462 closed the `Op::Negate` hole and claimed it +//! was the lone one — but the collapsed mixed `(Float | ExtFloat, Float | +//! ExtFloat)` arm (Add/Sub/Mul/Div/Mod) still pushed a raw `ExtFloat(a OP b)` +//! with no `== 0.0` fold. `(-1.0) * (0.0 / 1.0)` and `(-1.0) / (1.0 / 0.0)` +//! both yield `ExtFloat(-0.0)`, which is `==`-equal to `+0.0` yet a distinct +//! set key. Pre-fix the sets below hold TWO elements; post-fix, ONE. +//! +//! Drives the `silt` CLI end-to-end (lexer -> parser -> typechecker -> +//! compiler -> VM) so the lock exercises the real runtime path, not a +//! typecheck-only tautology. + +use std::process::Command; + +const ARITHMETIC_RS: &str = include_str!("../src/vm/arithmetic.rs"); + +fn run_silt_raw(label: &str, src: &str) -> (String, String, bool) { + let tmp = std::env::temp_dir().join(format!("silt_extfloat_mixed_negzero_{label}.silt")); + std::fs::write(&tmp, src).expect("write temp file"); + let bin = env!("CARGO_BIN_EXE_silt"); + let out = Command::new(bin) + .arg("run") + .arg(&tmp) + .output() + .expect("spawn silt run"); + let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); + (stdout, stderr, out.status.success()) +} + +fn run_silt_ok(label: &str, src: &str) -> String { + let (stdout, stderr, ok) = run_silt_raw(label, src); + assert!( + ok, + "silt run should succeed for {label}; stdout={stdout}, stderr={stderr}" + ); + stdout +} + +/// `Mul` in the mixed arm: `(-1.0) * (0.0 / 1.0)` produces `-0.0`. The right +/// operand `0.0 / 1.0` widens `Float / Float` to `ExtFloat(+0.0)`, so the +/// multiply walks the mixed `(Float, ExtFloat)` arm. Result must canonicalize +/// and dedup against a plain `+0.0` set key. +#[test] +fn mixed_mul_negative_zero_is_a_single_set_key() { + let out = run_silt_ok( + "mul", + r#" +import set + +fn main() { + let zero = 0.0 / 1.0 + let neg = (-1.0) * zero + let pos = 1.0 / 1.0 / 1.0 * 0.0 + println(pos == neg) + println(set.length(set.from_list([pos, neg]))) +} +"#, + ); + let lines: Vec<&str> = out.lines().collect(); + assert!(lines.len() >= 2, "expected 2 lines; got {out:?}"); + assert_eq!(lines[0], "true", "`-0.0 == +0.0` under IEEE `==`"); + assert_eq!( + lines[1], "1", + "mixed-arm Mul `-0.0` leaked in as a distinct bitwise set key \ + (ExtFloat(-0.0) vs ExtFloat(+0.0)); got {:?}", + lines[1] + ); +} + +/// `Div` in the mixed arm: `(-1.0) / (1.0 / 0.0)` = `(-1.0) / +inf` = `-0.0`. +/// The right operand is `ExtFloat(+inf)`, so the divide walks the mixed arm. +#[test] +fn mixed_div_negative_zero_dedups_on_insert() { + let out = run_silt_ok( + "div", + r#" +import set + +fn main() { + let inf = 1.0 / 0.0 + let neg = (-1.0) / inf + let pos = 1.0 / inf + let s0 = set.new() + let s1 = set.insert(s0, pos) + let s2 = set.insert(s1, neg) + println(neg == pos) + println(set.length(s2)) + println(set.contains(s2, neg)) +} +"#, + ); + let lines: Vec<&str> = out.lines().collect(); + assert!(lines.len() >= 3, "expected 3 lines; got {out:?}"); + assert_eq!(lines[0], "true", "`-0.0 == +0.0` under IEEE `==`"); + assert_eq!( + lines[1], "1", + "mixed-arm Div `-0.0` must dedup against +0.0 on insert; got {:?}", + lines[1] + ); + assert_eq!( + lines[2], "true", + "the negated zero must be found in the set" + ); +} + +/// Source-grep guard: the mixed arithmetic arm must keep canonicalizing zero. +/// A bare `Value::ExtFloat(a OP b)` (no `== 0.0` fold) reintroduces the bug. +#[test] +fn arithmetic_rs_canonicalizes_mixed_arm_zero() { + // Locate the collapsed mixed Float/ExtFloat arm and assert its body folds + // `-0.0` via the `== 0.0` guard, scoped from the arm header to the next + // arm (the `String`/`String` `Add` arm) so the check stays local. + let arm_idx = ARITHMETIC_RS + .find("Value::Float(a) | Value::ExtFloat(a), Value::Float(b) | Value::ExtFloat(b)") + .expect("mixed Float/ExtFloat arithmetic arm present in arithmetic.rs"); + let body = &ARITHMETIC_RS[arm_idx..]; + let arm = &body[..body + .find("Value::String(a), Value::String(b)") + .expect("String/String arm follows the mixed float arm")]; + assert!( + arm.contains("== 0.0"), + "the mixed Float/ExtFloat arithmetic arm no longer canonicalizes \ + -0.0 -> +0.0 (missing the `== 0.0` guard). A raw `ExtFloat(a OP b)` \ + lets a computed negative zero become a distinct bitwise set/map key \ + — see this test's module doc." + ); +}