From dae23defd327ebcf3fcc08abc0cc96db9e79a9a4 Mon Sep 17 00:00:00 2001 From: Robert Bartel Date: Wed, 24 Jun 2026 07:22:57 +0000 Subject: [PATCH] =?UTF-8?q?Fix=20audit=20findings:=20round=2097=20?= =?UTF-8?q?=E2=80=94=20container-Fn=20compare=20soundness,=20when-let=20ef?= =?UTF-8?q?fects=20shadowing,=20formatter=20close-line=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BROKEN: container of function-shaped values laundered into nondeterministic pointer-address ordering File: src/typechecker/mod.rs:6102 (operand_builtin_trait_violation); src/typechecker/inference.rs:5758 (is_valid_compare_operand) Test: round97_container_fn_compare_tests Repro: `let xs = [fn(x){x}]; let ys = [fn(x){x}]; print(xs < ys)` typechecked and ran, printing a nondeterministic bool (VmClosure ordered by Arc::as_ptr under ASLR). The compare/equality gates returned true for List/Range/Tuple/Set/Map heads without recursing into element types, so the round-3 (bare Fn) and round-93 (nominal record/enum) rejections were bypassed by container wrapping. Fix routes container heads through the existing gate_field_supports_trait recursion (bottoms out at Type::Fun => false). `[1,2,3] < [4,5,6]` and tuples/sets of orderable values still accepted. BROKEN: when-let binder shadowing an imported module was not marked rebound in the effects walker File: src/typechecker/effects_infer.rs:328 (Stmt::When arm of stmt_effects) Test: round97_when_let_effects_shadow_tests Repro: a `when let Ok(other) = opt(p) else { return 0 }` binder that shadows `import other`, followed by `other.double(3)` (a fn-typed field call), resolved through the module's declared `!{}` scheme instead of conservative TOP, so an effectful field fn slipped past --strict-effects. The Stmt::When arm ignored its pattern binders — the one statement form round-94 missed (Let/match-arm/loop were fixed). Fix mirrors the Stmt::Let arm: mark_pattern_rebound(pattern) + invalidate stale aliases. `sneaky` now correctly fails --strict-effects (charged !{fs,io,net,random,time}). Regressed-from: round 94 (module-shadowing soundness, when-let was the missed form) BROKEN: trailing line comment on a collapsed call/collection's close-delimiter line was dropped by the formatter File: src/formatter.rs:2475 (expr_max_line bracketed-collection arms); src/formatter.rs:752 (compute_bracket_end_line_opts) Test: round97_formatter_close_line_comment_tests Repro: `bar(\n 1\n) -- note` (and the list/tuple/map/set/record variants) round-tripped to `bar(1)`, silently losing `-- note`: expr_max_line never counted the construct's own closing-bracket line, so the statement drain never consumed trailing_map[close_line]. Fix adds the close line for Call/List/Tuple/Map/SetLit/RecordCreate/RecordUpdate/AnonRecord (mirroring the existing Lambda/Block handling). The close-line scan is strict (compute_bracket_end_line_opts, require_open_on_start_line) so a paren-less trailing-block call (`list.map { ... }`) does not latch onto a later statement's bracket and drag intervening standalone comments in — without this guard, examples/csv_analyzer.silt and examples/diff_tool.silt were re-formatted with a comment relocated. MINOR: row-narrowing lock test claimed a compile-stage guard it never exercised File: tests/round73_row_narrowing_tests.rs:118 Test: round73_row_narrowing_tests::unannotated_row_poly_bogus_field_blocks_compile Repro: the test doc claimed to drive the full lex->parse->typecheck->compile pipeline and guard against the compiler "still hands out bytecode," but the body stopped at typechecker::check. Now also drives `silt run` on the same source and asserts non-zero exit, the field error on stderr, and empty stdout (no bytecode reaches the VM). MINOR: stale source-line citations re-aimed and their locks updated File: src/vm/tests.rs / src/vm/execute.rs / src/cli/pipeline.rs (compiler/mod.rs short-circuit + is-not-imported cites, off by one line); src/typechecker/inference.rs / src/vm/dispatch.rs (auto-derive cites shifted by this round's mod.rs growth); src/lsp/completion.rs (builtin_trait_decls cite) Test: source_line_citation_lock_tests, typechecker_citation_resolve_tests Repro: the two citation-lock tests resolve each `file.rs:N` cross-reference to the named item; the compiler/mod.rs JumpIfFalse/JumpIfTrue/unreachable/is-not-imported cites had drifted +1, and the typechecker/mod.rs register_auto_derived_impls_for / builtin_trait_decls / List+Unit cites moved with the operand_builtin_trait_violation arm added above. Re-aimed both the citing comments and the lock expectation rows to the current lines. --- src/cli/pipeline.rs | 2 +- src/formatter.rs | 87 +++++++- src/lsp/completion.rs | 2 +- src/typechecker/effects_infer.rs | 30 ++- src/typechecker/inference.rs | 9 +- src/typechecker/mod.rs | 30 +++ src/vm/dispatch.rs | 4 +- src/vm/execute.rs | 6 +- src/vm/tests.rs | 4 +- tests/round73_row_narrowing_tests.rs | 45 ++++ tests/round97_container_fn_compare_tests.rs | 211 ++++++++++++++++++ ...nd97_formatter_close_line_comment_tests.rs | 170 ++++++++++++++ .../round97_when_let_effects_shadow_tests.rs | 153 +++++++++++++ tests/source_line_citation_lock_tests.rs | 14 +- tests/typechecker_citation_resolve_tests.rs | 6 +- 15 files changed, 748 insertions(+), 25 deletions(-) create mode 100644 tests/round97_container_fn_compare_tests.rs create mode 100644 tests/round97_formatter_close_line_comment_tests.rs create mode 100644 tests/round97_when_let_effects_shadow_tests.rs diff --git a/src/cli/pipeline.rs b/src/cli/pipeline.rs index 2dd191e1..1f3d7c98 100644 --- a/src/cli/pipeline.rs +++ b/src/cli/pipeline.rs @@ -451,7 +451,7 @@ pub(crate) fn is_unknown_module_warning(err: &SourceError) -> bool { /// Returns true iff `err` is the typechecker's "module 'X' is not /// imported" error. The compiler emits the same diagnostic (with -/// identical wording, see `src/compiler/mod.rs:2561`, `:2657`, `:3476`) +/// identical wording, see `src/compiler/mod.rs:2562`, `:2658`, `:3477`) /// as a hard compile error that actually blocks bytecode emission, so /// the CLI pipeline drops the typechecker's copy to avoid rendering the /// same sentence twice. See `reportable_type_errors` for the call site. diff --git a/src/formatter.rs b/src/formatter.rs index 48dcb24d..a6808d15 100644 --- a/src/formatter.rs +++ b/src/formatter.rs @@ -750,6 +750,26 @@ enum ScanMode { /// rather than a per-line walk; behavior is intentionally divergent /// for that reason and the two scaffolds are not unified. fn compute_bracket_end_line(start_line: usize, open: char, close: char) -> usize { + compute_bracket_end_line_opts(start_line, open, close, false) +} + +/// Variant of [`compute_bracket_end_line`] that, when +/// `require_open_on_start_line` is set, returns `start_line` unchanged if +/// the `open` delimiter is not found (in code context) on `start_line` +/// itself. `expr_max_line` runs over arbitrary expressions — including +/// paren-less trailing-block calls (`list.map { ... }`) that have no `(` +/// of their own. The plain scanner would skip past such a construct and +/// latch onto a *later* statement's bracket, dragging the intervening +/// standalone comments into the wrong statement on re-layout. Requiring +/// the opener on the first scanned line confines the match to this +/// construct. All pre-existing callers pass `false` (they already pass the +/// exact line the opener lives on), preserving their behavior unchanged. +fn compute_bracket_end_line_opts( + start_line: usize, + open: char, + close: char, + require_open_on_start_line: bool, +) -> usize { FMT_STATE.with(|cell| { let borrowed = cell.borrow(); let Some(state) = borrowed.as_ref() else { @@ -900,6 +920,13 @@ fn compute_bracket_end_line(start_line: usize, open: char, close: char) -> usize } } } + // Strict mode: if the opener never appeared on the first + // scanned line, this construct has no delimiter of its own + // (e.g. a paren-less trailing-block call). Don't scan into + // later statements looking for an unrelated bracket. + if require_open_on_start_line && line_idx == start_idx && !found_open { + return start_line; + } // End-of-line fallback: a `}` that pops back into a regular // string is allowed (we're in InRegular at end of line) as // long as the bracket pair has been closed at the @@ -2475,17 +2502,45 @@ fn expr_max_line(expr: &Expr) -> usize { ListElem::Single(e) | ListElem::Spread(e) => visit(e), } } + // Include the construct's own closing-bracket line so a + // trailing comment on the close-delimiter line (e.g. + // `[\n 1\n] -- note`) — which no element occupies — is + // reachable to the outer statement drain and not orphaned in + // `trailing_map`. Mirrors the Lambda/Block handling below. + let close = compute_bracket_end_line_opts(expr.span.line, '[', ']', true); + if close > max { + max = close; + } } ExprKind::Map(pairs) => { for (k, v) in pairs { visit(k); visit(v); } + let close = compute_bracket_end_line_opts(expr.span.line, '{', '}', true); + if close > max { + max = close; + } } - ExprKind::SetLit(elems) | ExprKind::Tuple(elems) => { + ExprKind::SetLit(elems) => { for e in elems { visit(e); } + // `#[ ... ]` — the `#` is a scalar the scanner skips; the + // bracket pair is `[`/`]`. + let close = compute_bracket_end_line_opts(expr.span.line, '[', ']', true); + if close > max { + max = close; + } + } + ExprKind::Tuple(elems) => { + for e in elems { + visit(e); + } + let close = compute_bracket_end_line_opts(expr.span.line, '(', ')', true); + if close > max { + max = close; + } } ExprKind::FieldAccess(e, _) => visit(e), ExprKind::Binary(l, _, r) => { @@ -2502,6 +2557,15 @@ fn expr_max_line(expr: &Expr) -> usize { for a in args { visit(a); } + // Include the call's own `)` close line so a trailing comment + // on the close-paren line (e.g. `bar(\n 1\n) -- note`) is + // reachable to the outer statement drain rather than orphaned + // in `trailing_map`. The call's span is at the callee and the + // opening `(` lives on the same line. Mirrors Lambda/Block. + let close = compute_bracket_end_line_opts(expr.span.line, '(', ')', true); + if close > max { + max = close; + } } ExprKind::Lambda { body, .. } => { visit(body); @@ -2523,12 +2587,25 @@ fn expr_max_line(expr: &Expr) -> usize { for (_, v) in fields { visit(v); } + // Include the record's `}` close line so a trailing comment on + // the close-brace line is reachable to the outer statement + // drain. Mirrors Lambda/Block. + let close = compute_bracket_end_line_opts(expr.span.line, '{', '}', true); + if close > max { + max = close; + } } - ExprKind::RecordUpdate { expr, fields } => { - visit(expr); + ExprKind::RecordUpdate { expr: recv, fields } => { + visit(recv); for (_, v) in fields { visit(v); } + // `RecordUpdate.span` is the receiver's span; the update brace + // is the first `{` at or after that line. Reach its close `}`. + let close = compute_bracket_end_line_opts(expr.span.line, '{', '}', true); + if close > max { + max = close; + } } ExprKind::AnonRecord { spread, fields } => { if let Some(s) = spread { @@ -2537,6 +2614,10 @@ fn expr_max_line(expr: &Expr) -> usize { for (_, v) in fields { visit(v); } + let close = compute_bracket_end_line_opts(expr.span.line, '{', '}', true); + if close > max { + max = close; + } } ExprKind::Match { expr, arms } => { if let Some(e) = expr.as_deref() { diff --git a/src/lsp/completion.rs b/src/lsp/completion.rs index 64b32f1f..65c01fe1 100644 --- a/src/lsp/completion.rs +++ b/src/lsp/completion.rs @@ -415,7 +415,7 @@ fn auto_derived_methods_for(canon_name: &str) -> &'static [&'static str] { // - Tuple/Map/Set → Equal/Hash/Display only (no Compare). // // Trait method names are sourced from `builtin_trait_decls` - // (src/typechecker/mod.rs:7560): + // (src/typechecker/mod.rs:7589): // Display → display, Compare → compare, Equal → equal, Hash → hash. // // `Bytes` is a stdlib alias for `List(Int)` and thus collapses onto diff --git a/src/typechecker/effects_infer.rs b/src/typechecker/effects_infer.rs index c89ff5c4..a6051fe9 100644 --- a/src/typechecker/effects_infer.rs +++ b/src/typechecker/effects_infer.rs @@ -326,8 +326,34 @@ fn stmt_effects(stmt: &Stmt, env: &TypeEnv, aliases: &mut AliasMap) -> EffectSet val_effects } Stmt::When { - expr, else_body, .. - } => walk_expr(expr, env, aliases).union(walk_expr(else_body, env, aliases)), + pattern, + expr, + else_body, + } => { + // Round 97 BROKEN (soundness): `when let pat = expr else { .. }` + // binds `pat`'s names into the enclosing block scope, exactly + // like `Stmt::Let`. The arm previously ignored the pattern, so a + // binder that shadows a same-named imported module was still + // treated as a live module qualifier — a later `name.fn(...)` + // resolved through the MODULE's qualified scheme and charged the + // module fn's declared effects instead of conservative TOP, + // letting a real effectful field-fn slip past --strict-effects. + // Mirror the `Stmt::Let` arm: resolve any aliasing source first + // (the scrutinee sits outside the new binding's scope), then mark + // the binders rebound and invalidate stale alias entries. + let scrutinee_effects = walk_expr(expr, env, aliases); + let src = resolvable_callee_name(expr, env, aliases); + aliases.mark_pattern_rebound(pattern); + if let PatternKind::Ident(name) = &pattern.kind { + if let Some(src) = src { + let src_eff = scheme_effects_of(src, env, aliases); + aliases.insert(*name, src_eff); + } else { + aliases.remove(name); + } + } + scrutinee_effects.union(walk_expr(else_body, env, aliases)) + } Stmt::WhenBool { condition, else_body, diff --git a/src/typechecker/inference.rs b/src/typechecker/inference.rs index 5ee0de8b..34a4e981 100644 --- a/src/typechecker/inference.rs +++ b/src/typechecker/inference.rs @@ -3189,7 +3189,7 @@ impl TypeChecker { } // Primitive types — check method table for trait methods. // ExtFloat is auto-derived (see `register_auto_derived_impls_for` - // in `src/typechecker/mod.rs:7896`); Channel and Fn are not + // in `src/typechecker/mod.rs:7925`); Channel and Fn are not // auto-derived but user-defined trait impls register entries // under the canonical names "Channel" / "Fn" via // `type_name_for_impl` (see `src/typechecker/mod.rs:2045`, @@ -5755,6 +5755,13 @@ pub(super) fn is_valid_arith_operand(ty: &Type, allow_string: bool) -> bool { /// nondeterministic) is rejected there with a field-precise message. /// This free function stays stateless because every other arm is /// purely structural. +/// +/// Round 97: the same applies to CONTAINER heads. `List(_)` / `Range(_)` +/// (ordering + equality) and `Tuple`/`Map`/`Set` (equality) pass this +/// shape gate, but a container whose element / component / value type is +/// `Fn`-shaped would launder into the same Arc-pointer-address ordering +/// at runtime. `operand_builtin_trait_violation` recurses into the +/// element types (via `gate_field_supports_trait`) and rejects those. pub(super) fn is_valid_compare_operand(ty: &Type, is_equality: bool) -> bool { match ty { Type::Int diff --git a/src/typechecker/mod.rs b/src/typechecker/mod.rs index 41ed23b8..757fcb3c 100644 --- a/src/typechecker/mod.rs +++ b/src/typechecker/mod.rs @@ -6099,6 +6099,36 @@ impl TypeChecker { }) }); } + // Round 97: container HEADS (List/Range/Tuple/Map/Set) pass the + // structural shape gate in `is_valid_compare_operand`, but the + // Value-level operation recurses into element types — and the VM + // fallback for a `Fn`-shaped element is Arc-pointer-address + // ordering (ASLR-nondeterministic), exactly the bug round 3 fixed + // for bare `Fn` operands and round 93 fixed for nominal fields. + // Mirror the round-93 field walk: a container is compare/equality + // -valid IFF every element / component / value type is itself + // valid. `gate_field_supports_trait` already does this recursion + // honestly (and bottoms out at `Type::Fun(..) => false`), so we + // reuse it and surface the whole container type as the reason. + Type::List(_) | Type::Range(_) | Type::Tuple(_) | Type::Map(..) | Type::Set(_) => { + let no_rescue: std::collections::HashSet<(Symbol, Symbol)> = + std::collections::HashSet::new(); + let no_negatives = HashMap::new(); + return (!self.gate_field_supports_trait( + trait_sym, + &resolved, + &no_rescue, + &no_negatives, + 0, + )) + .then(|| { + format!( + "type '{resolved}' cannot derive '{}': element type is not {}", + resolve(trait_sym), + builtin_trait_adjective(trait_sym), + ) + }); + } _ => return None, }; let canon = canonicalize_type_name(&self.resolver, name); diff --git a/src/vm/dispatch.rs b/src/vm/dispatch.rs index 2ca9edc6..3018d2ac 100644 --- a/src/vm/dispatch.rs +++ b/src/vm/dispatch.rs @@ -365,7 +365,7 @@ impl Vm { (Value::String(a), Value::String(b)) => a.cmp(b), (Value::Bool(a), Value::Bool(b)) => a.cmp(b), // List vs List: the typechecker auto-derives Compare for - // List (see src/typechecker/mod.rs:7796), so a value of + // List (see src/typechecker/mod.rs:7825), so a value of // `List(T)` flowing through a `Compare` bound must // resolve here. Defer to the existing element-wise // ordering on `Value::cmp`, which already handles @@ -394,7 +394,7 @@ impl Vm { | (Value::Record(..), Value::Record(..)) => receiver.cmp(other), // // Unit vs Unit: typechecker auto-derives Compare for `()` - // (src/typechecker/mod.rs:7793). All units are equal. + // (src/typechecker/mod.rs:7822). All units are equal. (Value::Unit, Value::Unit) => std::cmp::Ordering::Equal, _ => { return Some(Err(VmError::new(format!( diff --git a/src/vm/execute.rs b/src/vm/execute.rs index c4f52e7c..3f3c7471 100644 --- a/src/vm/execute.rs +++ b/src/vm/execute.rs @@ -1393,9 +1393,9 @@ impl Vm { Op::And => { // Round-75 VM-2: the compiler always lowers `BinOp::And` // to a `JumpIfFalse` short-circuit (see - // `src/compiler/mod.rs:2326`); the + // `src/compiler/mod.rs:2327`); the // `BinOp::And | BinOp::Or => unreachable!()` at - // `compiler/mod.rs:2358` confirms no other emission + // `compiler/mod.rs:2359` confirms no other emission // path exists. Match the LoopSetup precedent // (execute.rs:Op::LoopSetup) and crash loudly on // accidental re-emission rather than silently @@ -1411,7 +1411,7 @@ impl Vm { } Op::Or => { // See Op::And above — same rationale. Compiler emits - // `JumpIfTrue` short-circuit at compiler/mod.rs:2337; + // `JumpIfTrue` short-circuit at compiler/mod.rs:2338; // `Op::Or` is reserved-but-never-emitted. unreachable!( "compiler always lowers BinOp::And/Or to JumpIfFalse/JumpIfTrue \ diff --git a/src/vm/tests.rs b/src/vm/tests.rs index a374f87c..ecca9900 100644 --- a/src/vm/tests.rs +++ b/src/vm/tests.rs @@ -599,7 +599,7 @@ fn test_leq_float() { /// Round-75 VM-2: Op::And dispatch is now `unreachable!()` — the /// compiler always lowers `BinOp::And` to a `JumpIfFalse` short- -/// circuit (see `compiler/mod.rs:2326`). Hand-emitting Op::And here +/// circuit (see `compiler/mod.rs:2327`). Hand-emitting Op::And here /// must crash the VM with the unreachable! panic, matching the /// LoopSetup precedent. #[test] @@ -619,7 +619,7 @@ fn test_and_op_is_unreachable() { /// Round-75 VM-2: symmetric to `test_and_op_is_unreachable` — the /// compiler always lowers `BinOp::Or` to a `JumpIfTrue` short- -/// circuit at `compiler/mod.rs:2337`, so direct emission of Op::Or +/// circuit at `compiler/mod.rs:2338`, so direct emission of Op::Or /// must crash with the unreachable! panic. #[test] #[should_panic( diff --git a/tests/round73_row_narrowing_tests.rs b/tests/round73_row_narrowing_tests.rs index eba57adb..a4866b7c 100644 --- a/tests/round73_row_narrowing_tests.rs +++ b/tests/round73_row_narrowing_tests.rs @@ -22,6 +22,25 @@ use silt::typechecker; use silt::types::Severity; +use std::process::Command; + +/// Drive the full public pipeline (lexer → parser → typechecker → +/// compiler → VM) the same way `silt run` does, by spawning the built +/// `silt` binary. Returns `(stdout, stderr, success)` so callers can +/// assert that a type-rejected program never reaches runtime. +fn run_silt_raw(label: &str, src: &str) -> (String, String, bool) { + let tmp = std::env::temp_dir().join(format!("silt_round73_row_{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 type_errors(input: &str) -> Vec { let tokens = silt::lexer::Lexer::new(input) @@ -148,4 +167,30 @@ fn main() { .any(|e| e.message.contains("zzznosuchfield")), "typecheck error must mention the bogus field 'zzznosuchfield'; got: {hard_errors:?}" ); + + // Compile-stage guard (the claim this lock makes): drive the SAME + // source through the full public pipeline `silt run` uses. The CLI + // gates on hard type errors (src/cli/pipeline.rs `compile_file_with_options`: + // it `process::exit(1)`s before handing functions to the VM), so the + // rejected program must EXIT NON-ZERO, never reach runtime, and print + // the field error to stderr. If a future regression let the compiler + // hand out bytecode despite the typecheck error, the program would run + // and crash — this assertion catches that. + let (stdout, stderr, ok) = run_silt_raw("bogus_field_blocks_compile", src); + assert!( + !ok, + "silt run must reject the bogus-field program before runtime; \ + instead it succeeded — the compiler handed out runnable bytecode. \ + stdout={stdout:?}, stderr={stderr:?}" + ); + assert!( + stderr.contains("zzznosuchfield"), + "the pipeline must surface the field error on stderr; \ + stdout={stdout:?}, stderr={stderr:?}" + ); + assert!( + stdout.is_empty(), + "a type-rejected program must never produce runtime output; \ + got stdout={stdout:?}" + ); } diff --git a/tests/round97_container_fn_compare_tests.rs b/tests/round97_container_fn_compare_tests.rs new file mode 100644 index 00000000..fe3df22a --- /dev/null +++ b/tests/round97_container_fn_compare_tests.rs @@ -0,0 +1,211 @@ +//! Round 97 regression locks — container-of-`Fn` compare/equality gate. +//! +//! FINDING (BROKEN, type-system soundness): ordering (`<` `>` `<=` `>=`) +//! and equality (`==` `!=`) on a CONTAINER of function-shaped values +//! (List/Range/Tuple/Set/Map whose element / component / value type is +//! `Fn`) was wrongly ACCEPTED by the typechecker, then at runtime +//! laundered into ASLR-nondeterministic pointer-address ordering +//! (`VmClosure` ordered by `Arc::as_ptr`, src/value.rs). +//! +//! This is the same bug-class round 3 fixed for bare `Fn` operands and +//! round 93 fixed for nominal records / enums — the container HEADS +//! (`List(_)`, `Range(_)`, `Tuple`, `Map`, `Set`) slip through the +//! structural shape gate `is_valid_compare_operand` because that gate +//! does not recurse into element types. +//! +//! FIX: `operand_builtin_trait_violation` (src/typechecker/mod.rs) now +//! recurses into container element / component / value types via the +//! round-93 `gate_field_supports_trait` walker (which bottoms out at +//! `Type::Fun(..) => false`) and rejects the operand with a message +//! mirroring the round-93 nominal shape: +//! `type 'List(Fn(...) -> ...)' cannot derive 'Compare': element +//! type is not comparable` +//! +//! Every rejection test below FAILS on the pre-fix code (the program +//! typechecked and reached the VM, ordering closures by pointer +//! address) and PASSES post-fix. The positive-control tests guard +//! against over-rejection (the gate's failure mode). + +use std::process::Command; + +use silt::typechecker; +use silt::types::Severity; + +/// Typecheck `input` in-process and return the Error-severity messages. +fn type_errors(input: &str) -> Vec { + let tokens = silt::lexer::Lexer::new(input) + .tokenize() + .expect("lexer error"); + let mut program = silt::parser::Parser::new(tokens) + .parse_program() + .expect("parse error"); + let errors = typechecker::check(&mut program); + errors + .into_iter() + .filter(|e| e.severity == Severity::Error) + .map(|e| e.message) + .collect() +} + +/// Assert that typecheck rejects `input` with a message containing all +/// of `needles`. +fn assert_rejected(label: &str, input: &str, needles: &[&str]) { + let errs = type_errors(input); + assert!( + !errs.is_empty(), + "{label}: expected a typecheck rejection, got none" + ); + for needle in needles { + assert!( + errs.iter().any(|e| e.contains(needle)), + "{label}: expected an error containing {needle:?}, got: {errs:?}" + ); + } +} + +/// Assert that typecheck accepts `input` (no Error-severity messages). +fn assert_accepted(label: &str, input: &str) { + let errs = type_errors(input); + assert!( + errs.is_empty(), + "{label}: expected clean typecheck, got: {errs:?}" + ); +} + +/// Drive the full lex → parse → typecheck → compile → VM pipeline via +/// the `silt run` CLI and return (stdout, stderr, success). +fn run_silt_raw(label: &str, src: &str) -> (String, String, bool) { + let tmp = std::env::temp_dir().join(format!("silt_round97_container_fn_{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()) +} + +// ── REJECTIONS: container of Fn under ordering / equality ────────────── + +#[test] +fn list_of_fn_ordering_is_rejected() { + // The exact repro from the finding. + let src = r#" +fn main() { + let xs = [fn(x) { x }] + let ys = [fn(x) { x }] + print(xs < ys) +} +"#; + assert_rejected( + "list_of_fn_ordering", + src, + &["cannot derive 'Compare'", "element type is not comparable"], + ); +} + +#[test] +fn list_of_fn_equality_is_rejected() { + let src = r#" +fn main() { + let xs = [fn(x) { x }] + let ys = [fn(x) { x }] + print(xs == ys) +} +"#; + assert_rejected( + "list_of_fn_equality", + src, + &["cannot derive 'Equal'", "element type is not equatable"], + ); +} + +#[test] +fn tuple_of_fn_equality_is_rejected() { + // Tuples are equality-only (no ordering), so exercise `==`. + let src = r#" +fn main() { + let a = (1, fn(x) { x }) + let b = (1, fn(x) { x }) + print(a == b) +} +"#; + assert_rejected( + "tuple_of_fn_equality", + src, + &["cannot derive 'Equal'", "element type is not equatable"], + ); +} + +#[test] +fn rejected_program_never_reaches_runtime() { + // Run the ordering repro through the real CLI: it must fail (the + // typecheck rejects it) and never print a (pointer-address) Bool. + let src = r#" +fn main() { + let xs = [fn(x) { x }] + let ys = [fn(x) { x }] + print(xs < ys) +} +"#; + let (stdout, stderr, ok) = run_silt_raw("ordering_run", src); + assert!( + !ok, + "expected `silt run` to fail at typecheck; stdout={stdout:?} stderr={stderr:?}" + ); + // No `true`/`false` ever printed: the program is rejected before the VM. + assert!( + stdout.trim().is_empty(), + "rejected program should produce no stdout, got {stdout:?}" + ); + assert!( + stderr.contains("cannot derive 'Compare'") || stderr.contains("comparable"), + "expected the precise rejection on stderr, got {stderr:?}" + ); +} + +// ── POSITIVE CONTROLS: orderable / equatable containers stay valid ───── + +#[test] +fn list_of_int_ordering_still_typechecks() { + let src = r#" +fn main() { + print([1, 2, 3] < [4, 5, 6]) +} +"#; + assert_accepted("list_of_int_ordering", src); +} + +#[test] +fn list_of_int_ordering_still_runs() { + let src = r#" +fn main() { + print([1, 2, 3] < [4, 5, 6]) +} +"#; + let (stdout, stderr, ok) = run_silt_raw("int_ordering_run", src); + assert!(ok, "positive control failed: stderr={stderr:?}"); + assert_eq!(stdout.trim(), "true", "stderr={stderr:?}"); +} + +#[test] +fn tuple_and_set_of_orderable_values_still_typecheck() { + // Tuple equality of equatable components. + let tuple_src = r#" +fn main() { + print((1, "a") == (1, "b")) +} +"#; + assert_accepted("tuple_of_eq", tuple_src); + + // Set equality of equatable elements (`#[ ]` is the set literal). + let set_src = r#" +fn main() { + print(#[1, 2, 3] == #[3, 2, 1]) +} +"#; + assert_accepted("set_of_eq", set_src); +} diff --git a/tests/round97_formatter_close_line_comment_tests.rs b/tests/round97_formatter_close_line_comment_tests.rs new file mode 100644 index 00000000..ade6d006 --- /dev/null +++ b/tests/round97_formatter_close_line_comment_tests.rs @@ -0,0 +1,170 @@ +//! Round-97 lock: the formatter must preserve a trailing `--` line +//! comment that sits on the CLOSING-delimiter line of a multi-line +//! call / collection / record that the formatter then collapses onto a +//! single line. +//! +//! Pre-round-97 behaviour (BROKEN): a `--` on the close-delimiter line — +//! a line NO element occupies — was SILENTLY DROPPED on round-trip: +//! +//! ```text +//! fn main() { +//! bar( +//! 1 +//! ) -- note <- dropped +//! } +//! ``` +//! +//! Root cause: `expr_max_line` for `Call` / `List` / `Tuple` / `Map` / +//! `SetLit` / `RecordCreate` / `RecordUpdate` / `AnonRecord` visited +//! only the callee/args/elements and never added the construct's own +//! closing-bracket line. The statement-level drain therefore computed +//! `end_line = stmt_end_line(stmt)` = the last element line and never +//! reached `take_trailing_for_line(close_line)`; `take_comments_between` +//! is exclusive of the close line, so `trailing_map[close_line]` was +//! never consumed and the comment was lost. +//! +//! Fix: mirror the existing Lambda/Block handling in `expr_max_line` — +//! add the closing-bracket line (via `compute_bracket_end_line`) to the +//! running max for the bracketed-collection arms so the statement drain +//! reaches the orphaned `trailing_map` entry. +//! +//! This is DISTINCT from the round-95 fix (`--` on an ELEMENT line such +//! as `[1, 2, 3 -- c]`); here the `--` is on the close-delimiter line. +//! These tests also guard idempotency. + +use silt::formatter; + +fn assert_idempotent(src: &str) { + let once = formatter::format(src).expect("format pass 1"); + let twice = formatter::format(&once).expect("format pass 2"); + assert_eq!( + once, twice, + "non-idempotent\n--- once ---\n{once}\n--- twice ---\n{twice}" + ); +} + +#[test] +fn round97_call_close_line_trailing_comment_preserved() { + let src = "\ +fn main() { + bar( + 1 + ) -- note +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- note"), + "call close-line trailing comment was dropped:\n{out}" + ); + assert_idempotent(src); +} + +#[test] +fn round97_list_close_line_trailing_comment_preserved() { + let src = "\ +fn main() { + let xs = [ + 1 + ] -- note +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- note"), + "list close-line trailing comment was dropped:\n{out}" + ); + assert_idempotent(src); +} + +#[test] +fn round97_record_create_close_line_trailing_comment_preserved() { + let src = "\ +type Point = { x: Int, y: Int } + +fn make() -> Point { + let p = Point { + x: 1, + y: 2 + } -- built + p +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- built"), + "record-create close-line trailing comment was dropped:\n{out}" + ); + assert_idempotent(src); +} + +#[test] +fn round97_tuple_close_line_trailing_comment_preserved() { + let src = "\ +fn main() { + let t = ( + 1, + 2 + ) -- pair +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- pair"), + "tuple close-line trailing comment was dropped:\n{out}" + ); + assert_idempotent(src); +} + +#[test] +fn round97_map_close_line_trailing_comment_preserved() { + let src = "\ +fn main() { + let m = #{ + \"a\": 1 + } -- table +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- table"), + "map close-line trailing comment was dropped:\n{out}" + ); + assert_idempotent(src); +} + +#[test] +fn round97_set_close_line_trailing_comment_preserved() { + let src = "\ +fn main() { + let s = #[ + 1 + ] -- members +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- members"), + "set close-line trailing comment was dropped:\n{out}" + ); + assert_idempotent(src); +} + +#[test] +fn round97_round95_element_line_comment_still_preserved() { + // Regression guard: the round-95 case (`--` on an ELEMENT line, not + // the close line) must still be preserved by the round-97 change. + let src = "\ +fn main() { + let xs = [ + 1, 2, 3 -- elems + ] +} +"; + let out = formatter::format(src).expect("format"); + assert!( + out.contains("-- elems"), + "round-95 element-line comment regressed:\n{out}" + ); + assert_idempotent(src); +} diff --git a/tests/round97_when_let_effects_shadow_tests.rs b/tests/round97_when_let_effects_shadow_tests.rs new file mode 100644 index 00000000..ea52c75e --- /dev/null +++ b/tests/round97_when_let_effects_shadow_tests.rs @@ -0,0 +1,153 @@ +//! Round 97 (BROKEN, soundness — module-shadowing hole in `when let`): +//! The `Stmt::When` arm of `stmt_effects` ignored the `when let` +//! pattern's binders. Unlike `Stmt::Let` (and match-arm / loop bindings, +//! all fixed in round 94), it never called `aliases.mark_pattern_rebound` +//! and never invalidated stale alias entries. +//! +//! Consequence: a `when let` binder that shadows a same-named imported +//! module was still treated as a live module qualifier. A later +//! `name.fn(...)` resolved through the MODULE's qualified scheme via +//! `resolvable_callee_name` and charged the module fn's declared effects +//! (e.g. `!{}`) instead of conservative higher-order TOP. A genuinely +//! effectful field fn could then slip past `--strict-effects`. +//! +//! Fix: mirror the `Stmt::Let` arm in `src/typechecker/effects_infer.rs` +//! — resolve the aliasing source of the scrutinee first, then +//! `mark_pattern_rebound(pattern)` and invalidate stale alias entries for +//! the binders so a later dotted call is charged TOP, not the module's +//! scheme. + +use std::path::PathBuf; +use std::process::Command; + +fn rand_u64() -> u64 { + use std::time::SystemTime; + SystemTime::now() + .duration_since(SystemTime::UNIX_EPOCH) + .unwrap() + .as_nanos() as u64 +} + +/// Create a fresh tempdir holding the supplied module files plus a +/// `main.silt` containing `main_source`. Returns the dir path. +fn setup_dir(label: &str, files: &[(&str, &str)], main_source: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "silt_r97_when_let_{label}_{}_{}", + std::process::id(), + rand_u64() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("mkdir"); + for (name, content) in files { + std::fs::write(dir.join(name), content).expect("write module"); + } + std::fs::write(dir.join("main.silt"), main_source).expect("write main"); + dir +} + +/// Run `silt main.silt` in `dir`; return (stdout, stderr, ok). +fn silt_in_dir(dir: &PathBuf, args: &[&str]) -> (String, String, bool) { + let bin = env!("CARGO_BIN_EXE_silt"); + let out = Command::new(bin) + .args(args) + .arg(dir.join("main.silt")) + .output() + .expect("spawn silt"); + ( + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + out.status.success(), + ) +} + +/// A pure-annotated module fn whose name collides with a record field. +const OTHER_MODULE: (&str, &str) = ("other.silt", "pub fn double(x: Int) -> Int !{} { x * 2 }\n"); + +/// SOUNDNESS LOCK: a dotted CALL through a `when let` binder that shadows +/// an imported module must NOT silently adopt the same-named module fn's +/// (pure) declared effects. Phase A can't see through a fn-typed FIELD, so +/// the conservative answer is TOP — the unannotated fn fails +/// --strict-effects instead of letting a potentially effectful field fn +/// slip through as "pure like the module fn". +#[test] +fn strict_effects_when_let_shadowed_dotted_call_is_conservative_not_module_pure() { + let dir = setup_dir( + "when_let_shadowed_conservative", + &[OTHER_MODULE], + r#" +import other +type P { double: Fn(Int) -> Int } +fn opt(p: P) -> Result(P, String) { Ok(p) } +fn sneaky(p: P) -> Int { + when let Ok(other) = opt(p) else { return 0 } + other.double(3) +} +fn main() !{io} { println(sneaky(P { double: fn(x) { x + 1 } })) } +"#, + ); + let (stdout, stderr, ok) = silt_in_dir(&dir, &["check", "--strict-effects"]); + assert!( + !ok, + "shadowed `when let` dotted call must be charged conservatively (TOP), \ + not the module fn's !{{}}; stdout={stdout}, stderr={stderr}" + ); + let combined = format!("{stdout}\n{stderr}"); + assert!( + combined.contains("'sneaky'"), + "the conservative charge should land on 'sneaky'; got {combined}" + ); +} + +/// CONTROL: an UNSHADOWED `when let` followed by a real pure-annotated +/// module call must still pass --strict-effects. The fix must not start +/// over-charging an honest module call just because it appears after a +/// `when let` binding. Here `m` is the `when let` binder and `other` is +/// still the live module — its `!{}` scheme is correctly honored. +#[test] +fn strict_effects_when_let_unshadowed_module_call_still_pure() { + let dir = setup_dir( + "when_let_unshadowed_pure", + &[OTHER_MODULE], + r#" +import other +fn opt() -> Result(Int, String) { Ok(7) } +fn fine() -> Int { + when let Ok(m) = opt() else { return 0 } + m + other.double(3) +} +fn main() !{io} { println(fine()) } +"#, + ); + let (stdout, stderr, ok) = silt_in_dir(&dir, &["check", "--strict-effects"]); + assert!( + ok, + "unshadowed pure module call after a `when let` must pass --strict-effects; \ + stdout={stdout}, stderr={stderr}" + ); +} + +/// CONTROL: the `when let` binder leaves scope correctly — a binder that +/// is purely a local value (no dotted module collision) must not perturb +/// effect inference; the program runs and type/effect-checks cleanly. +#[test] +fn when_let_local_binder_runs_and_checks_clean() { + let dir = setup_dir( + "when_let_local_ok", + &[], + r#" +fn opt(n: Int) -> Result(Int, String) { Ok(n) } +fn use_it(n: Int) -> Int { + when let Ok(v) = opt(n) else { return -1 } + v + 1 +} +fn main() !{io} { println(use_it(41)) } +"#, + ); + let (stdout, stderr, ok) = silt_in_dir(&dir, &["run", "--strict-effects"]); + assert!( + ok, + "local `when let` binding must run and pass --strict-effects; \ + stdout={stdout}, stderr={stderr}" + ); + assert_eq!(stdout.trim(), "42"); +} diff --git a/tests/source_line_citation_lock_tests.rs b/tests/source_line_citation_lock_tests.rs index f9fc35c2..2de37572 100644 --- a/tests/source_line_citation_lock_tests.rs +++ b/tests/source_line_citation_lock_tests.rs @@ -32,7 +32,7 @@ const CITATIONS: &[(&str, &str, usize, &str)] = &[ ( "completion.rs -> builtin_trait_decls def", "src/typechecker/mod.rs", - 7560, + 7589, "fn builtin_trait_decls", ), // vm/runtime.rs:450 — "Rust 1.80+ thread-local env SAFETY note" @@ -53,21 +53,21 @@ const CITATIONS: &[(&str, &str, usize, &str)] = &[ ( "execute.rs/tests.rs -> And short-circuit JumpIfFalse emit", "src/compiler/mod.rs", - 2326, + 2327, "Op::JumpIfFalse", ), // vm/execute.rs:1362 / vm/tests.rs:622 — Or short-circuit JumpIfTrue ( "execute.rs/tests.rs -> Or short-circuit JumpIfTrue emit", "src/compiler/mod.rs", - 2337, + 2338, "Op::JumpIfTrue", ), // vm/execute.rs:1346 — `BinOp::And | BinOp::Or => unreachable!()` ( "execute.rs -> And/Or unreachable guard", "src/compiler/mod.rs", - 2358, + 2359, "BinOp::And | BinOp::Or => unreachable!()", ), // workspace.rs:284 — "FieldAccess.span is the receiver span" construction @@ -88,19 +88,19 @@ const CITATIONS: &[(&str, &str, usize, &str)] = &[ ( "pipeline.rs -> compiler 'is not imported' #1", "src/compiler/mod.rs", - 2561, + 2562, "is not imported", ), ( "pipeline.rs -> compiler 'is not imported' #2", "src/compiler/mod.rs", - 2657, + 2658, "is not imported", ), ( "pipeline.rs -> compiler 'is not imported' #3", "src/compiler/mod.rs", - 3476, + 3477, "is not imported", ), // compiler/mod.rs:441 — typechecker round-58 prefix-mirror logic diff --git a/tests/typechecker_citation_resolve_tests.rs b/tests/typechecker_citation_resolve_tests.rs index 9e3a3d3f..bcc1809f 100644 --- a/tests/typechecker_citation_resolve_tests.rs +++ b/tests/typechecker_citation_resolve_tests.rs @@ -76,15 +76,15 @@ fn mod_rs_citations_in_inference_and_dispatch_resolve() { // purpose — it never existed; the real mapper is `type_name_for_impl`. let expectations: &[(usize, &str)] = &[ // inference.rs primitive-dispatch comment block. - (7896, "fn register_auto_derived_impls_for"), + (7925, "fn register_auto_derived_impls_for"), (2045, r#"Type::Channel(_) => Some(intern("Channel"))"#), (2056, r#"Type::Fun(_, _) => Some(intern("Fn"))"#), // dispatch.rs compare-arm comment block. ( - 7796, + 7825, r#"register_auto_derived_impls_for(checker, &["List"]"#, ), - (7793, r#""Unit""#), + (7822, r#""Unit""#), ]; let mut expected_lines: BTreeSet = BTreeSet::new();