From 03459c53bff754889c1885438eb11ad3541cf1e2 Mon Sep 17 00:00:00 2001 From: aetheron06 Date: Mon, 28 Sep 2026 08:27:50 +0100 Subject: [PATCH 1/2] feat: address requested issues - closes #703, closes #704, closes #708, closes #709 Draft implementation for four cargo-budget-report issues. This commit has NOT been built, tested, linted or formatted. - #703 validate.rs: reuse a single Levenshtein DP row instead of the full (n+1)x(m+1) matrix and drop the two Vec copies per call, prune candidates whose length difference already exceeds the typo threshold before running the DP, replace the per-name linear scan of available functions with an O(1) set lookup, build the available-functions hint lazily on the first missing name, and deserialize budget.toml once instead of twice per call. Adds 10 edge-case tests. - #704 contract_exports.rs: 12 new unit tests covering runtime-symbol classification, filtering of runtime and memory exports, stable export ordering, a module with no export section, invalid wasm, diagnostic wording and ExportScan value semantics. - #708 fixture.rs: 21 new unit tests covering save/load round trips for every JSON shape, overwrite, idempotent reload, unknown top-level fields, unicode keys and values, and every read/parse/version-mismatch/write error path. - #709 json_output.rs: module, type and function documentation for the JSON envelope, a worked example document, inline rationale for the borrowed snapshots and the unreachable expect, and 5 new tests pinning the envelope shape. --- cargo-budget-report/src/contract_exports.rs | 193 +++++++++++ cargo-budget-report/src/fixture.rs | 294 +++++++++++++++++ cargo-budget-report/src/json_output.rs | 134 +++++++- cargo-budget-report/src/validate.rs | 340 ++++++++++++++++---- 4 files changed, 902 insertions(+), 59 deletions(-) diff --git a/cargo-budget-report/src/contract_exports.rs b/cargo-budget-report/src/contract_exports.rs index 2707568c..01980e37 100644 --- a/cargo-budget-report/src/contract_exports.rs +++ b/cargo-budget-report/src/contract_exports.rs @@ -234,4 +234,197 @@ mod tests { assert!(msg.contains("shared-types")); assert!(msg.contains("cdylib")); } + + // ── runtime-symbol classification ───────────────────────────────── + + #[test] + fn runtime_symbols_are_recognised_by_name() { + for name in [ + "memory", + "__heap_base", + "__data_end", + "__stack_pointer", + "__rust_alloc", + "_start", + "_", + "__other_compiler_symbol", + ] { + assert!(is_runtime_symbol(name), "expected runtime symbol: {name}"); + } + + for name in [ + "swap", + "deposit", + "memory_usage", + "data_end", + "Memory", + "noop", + ] { + assert!(!is_runtime_symbol(name), "expected contract export: {name}"); + } + } + + // ── export scanning ─────────────────────────────────────────────── + + #[test] + fn runtime_symbols_are_filtered_out_of_the_function_list() { + let wasm = wasm_with_exports(&["__data_end", "swap", "_start"], true); + match scan_wasm_exports(&wasm).unwrap() { + ExportScan::Functions(fns) => { + assert_eq!( + fns.into_iter().collect::>(), + vec!["swap"], + "neither the toolchain symbols nor the `memory` export count" + ); + } + other => panic!("expected Functions, got {other:?}"), + } + } + + #[test] + fn function_exports_are_returned_in_a_stable_sorted_order() { + let names: Vec = (0..10).map(|i| format!("f{i}")).collect(); + let wasm = { + let borrowed: Vec<&str> = names.iter().map(String::as_str).collect(); + wasm_with_exports(&borrowed, false) + }; + + match scan_wasm_exports(&wasm).unwrap() { + ExportScan::Functions(fns) => { + let collected: Vec = fns.into_iter().collect(); + assert_eq!(collected, names, "every export is kept, in sorted order"); + } + other => panic!("expected Functions, got {other:?}"), + } + } + + #[test] + fn contract_names_containing_underscores_are_kept() { + let wasm = wasm_with_exports(&["transfer_from", "burn_from_account"], false); + match scan_wasm_exports(&wasm).unwrap() { + ExportScan::Functions(fns) => { + assert_eq!( + fns.into_iter().collect::>(), + vec!["burn_from_account", "transfer_from"], + "only a leading underscore marks a toolchain symbol" + ); + } + other => panic!("expected Functions, got {other:?}"), + } + } + + /// A minimal module with no export section at all — the shape a plain + /// `rlib` build leaves behind. + fn wasm_without_export_section() -> Vec { + let mut module = Vec::new(); + module.extend_from_slice(b"\0asm"); + module.extend_from_slice(&1u32.to_le_bytes()); + section(&mut module, 1, &[0x01, 0x60, 0x00, 0x00]); + section(&mut module, 3, &[0x01, 0x00]); + section(&mut module, 10, &[0x01, 0x02, 0x00, 0x0b]); + module + } + + #[test] + fn a_module_without_an_export_section_reports_no_function_exports() { + let wasm = wasm_without_export_section(); + assert_eq!( + scan_wasm_exports(&wasm).unwrap(), + ExportScan::NoFunctionExports + ); + assert!( + ExportScan::NoFunctionExports.diagnostic("pkg").is_some(), + "a module with no exports must still produce a diagnostic" + ); + } + + #[test] + fn invalid_wasm_is_reported_as_an_error_not_a_scan_result() { + assert!( + scan_wasm_exports(b"not a wasm module").is_err(), + "garbage bytes must not be classified" + ); + assert!( + scan_wasm_exports(b"\0asm").is_err(), + "a truncated header is an error" + ); + } + + // ── diagnostics ─────────────────────────────────────────────────── + + #[test] + fn runtime_symbol_list_is_sorted_for_stable_messages() { + let wasm = wasm_with_exports(&["_start", "memory", "__data_end"], false); + match scan_wasm_exports(&wasm).unwrap() { + ExportScan::OnlyRuntimeSymbols { found } => { + assert_eq!( + found, + vec![ + "__data_end".to_string(), + "_start".to_string(), + "memory".to_string() + ], + "the list is sorted so the message is stable across builds" + ); + } + other => panic!("expected OnlyRuntimeSymbols, got {other:?}"), + } + } + + #[test] + fn runtime_symbols_diagnostic_lists_every_name() { + let scan = ExportScan::OnlyRuntimeSymbols { + found: vec!["__heap_base".into(), "_start".into()], + }; + let message = scan.diagnostic("my-contract").unwrap(); + assert!(message.contains("my-contract"), "got: {message}"); + assert!( + message.contains("Exports found: __heap_base, _start"), + "got: {message}" + ); + assert!(message.contains("#[contractimpl]"), "got: {message}"); + } + + #[test] + fn runtime_symbols_diagnostic_handles_an_empty_list() { + let scan = ExportScan::OnlyRuntimeSymbols { found: Vec::new() }; + let message = scan.diagnostic("my-contract").unwrap(); + assert!(message.contains("Package 'my-contract'"), "got: {message}"); + assert!(message.contains("Exports found:"), "got: {message}"); + } + + #[test] + fn no_function_exports_diagnostic_names_the_missing_attributes() { + let message = ExportScan::NoFunctionExports.diagnostic("my-contract").unwrap(); + assert!(message.contains("my-contract"), "got: {message}"); + assert!(message.contains("plain library"), "got: {message}"); + assert!(message.contains("#[contract]"), "got: {message}"); + assert!(message.contains("crate-type"), "got: {message}"); + } + + #[test] + fn cdylib_message_offers_the_rlib_alternative() { + let message = not_a_cdylib_message("shared-types"); + assert!(message.contains("crate-type"), "got: {message}"); + assert!(message.contains("rlib"), "got: {message}"); + assert!(message.contains("Skipping"), "got: {message}"); + } + + #[test] + fn export_scan_clones_and_compares_by_value() { + let fns: BTreeSet = ["swap".to_string()].into_iter().collect(); + let scan = ExportScan::Functions(fns.clone()); + + assert_eq!(scan, ExportScan::Functions(fns)); + assert_eq!(scan.clone(), scan); + assert_ne!(scan, ExportScan::NoFunctionExports); + assert_ne!( + ExportScan::NoFunctionExports, + ExportScan::OnlyRuntimeSymbols { found: Vec::new() } + ); + assert!( + format!("{scan:?}").contains("Functions"), + "the scan result stays debuggable for callers that log it" + ); + } } diff --git a/cargo-budget-report/src/fixture.rs b/cargo-budget-report/src/fixture.rs index b9aa82d6..6042aa83 100644 --- a/cargo-budget-report/src/fixture.rs +++ b/cargo-budget-report/src/fixture.rs @@ -50,3 +50,297 @@ impl FixtureFile { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + /// Write `body` to `dir/fixture.json` and load it back, so the read, + /// deserialize and version-check paths are all exercised from raw text. + fn load_body(dir: &tempfile::TempDir, body: &str) -> Result { + let path = dir.path().join("fixture.json"); + std::fs::write(&path, body).expect("failed to write test fixture"); + FixtureFile::load(&path) + } + + /// One entry per JSON shape a recorded response can take, keyed the way + /// the transport helpers build replay keys. + fn sample_fixture() -> FixtureFile { + FixtureFile { + fixture_version: FIXTURE_VERSION, + entries: HashMap::from([ + ( + "invoke:abc".to_string(), + json!({ "status": "SUCCESS", "fuel_consumed": 42 }), + ), + ("simulate:def".to_string(), json!([1, 2, 3])), + ("deploy:ghi".to_string(), json!("some-contract-id")), + ("invoke:jkl".to_string(), json!(null)), + ]), + } + } + + #[test] + fn new_file_starts_empty_at_the_current_version() { + let fixture = FixtureFile::new(); + assert_eq!(fixture.fixture_version, FIXTURE_VERSION); + assert!(fixture.entries.is_empty()); + } + + #[test] + fn on_disk_version_is_pinned() { + // Bumping `FIXTURE_VERSION` invalidates every recorded fixture, so it + // is a deliberate decision that should update this assertion too. + assert_eq!(FIXTURE_VERSION, 1); + } + + #[test] + fn save_then_load_round_trips_every_entry_shape() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("fixture.json"); + let original = sample_fixture(); + + original.save(&path).unwrap(); + let loaded = FixtureFile::load(&path).unwrap(); + + assert_eq!(loaded.fixture_version, original.fixture_version); + assert_eq!(loaded.entries, original.entries); + assert!(loaded.entries["invoke:jkl"].is_null()); + } + + #[test] + fn save_writes_pretty_json_under_the_documented_keys() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("fixture.json"); + sample_fixture().save(&path).unwrap(); + + let text = std::fs::read_to_string(&path).unwrap(); + assert!(text.contains('\n'), "expected pretty-printed JSON"); + let value: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(value["fixture_version"], FIXTURE_VERSION); + assert_eq!(value["entries"]["simulate:def"], json!([1, 2, 3])); + assert_eq!( + value.as_object().unwrap().len(), + 2, + "only the two documented fields are written" + ); + } + + #[test] + fn save_overwrites_an_existing_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("fixture.json"); + std::fs::write(&path, r#"{"fixture_version": 1, "entries": {}}"#).unwrap(); + + let replacement = FixtureFile { + fixture_version: FIXTURE_VERSION, + entries: HashMap::from([("invoke:abc".to_string(), json!({ "status": "SUCCESS" }))]), + }; + replacement.save(&path).unwrap(); + + let loaded = FixtureFile::load(&path).unwrap(); + assert_eq!(loaded.entries, replacement.entries); + assert_eq!(loaded.entries.len(), 1, "the previous contents are gone"); + } + + #[test] + fn load_then_save_is_idempotent() { + let dir = tempfile::tempdir().unwrap(); + let first = dir.path().join("first.json"); + let second = dir.path().join("second.json"); + + sample_fixture().save(&first).unwrap(); + FixtureFile::load(&first).unwrap().save(&second).unwrap(); + + assert_eq!( + FixtureFile::load(&first).unwrap().entries, + FixtureFile::load(&second).unwrap().entries + ); + } + + #[test] + fn load_accepts_an_empty_entries_map() { + let dir = tempfile::tempdir().unwrap(); + let fixture = load_body(&dir, r#"{"fixture_version": 1, "entries": {}}"#).unwrap(); + assert!(fixture.entries.is_empty()); + } + + #[test] + fn load_accepts_entries_of_any_json_shape() { + let dir = tempfile::tempdir().unwrap(); + let fixture = load_body( + &dir, + r#"{ + "fixture_version": 1, + "entries": { + "invoke:abc": { "status": "SUCCESS" }, + "deploy:def": [1, 2, 3], + "simulate:ghi": "0a0b0c", + "invoke:jkl": null + } + }"#, + ) + .unwrap(); + + assert_eq!(fixture.fixture_version, FIXTURE_VERSION); + assert_eq!(fixture.entries.len(), 4); + assert_eq!(fixture.entries["invoke:abc"]["status"], "SUCCESS"); + assert_eq!(fixture.entries["deploy:def"], json!([1, 2, 3])); + assert_eq!(fixture.entries["simulate:ghi"], json!("0a0b0c")); + assert!(fixture.entries["invoke:jkl"].is_null()); + } + + #[test] + fn load_ignores_unknown_top_level_fields() { + // Forward compatibility: a fixture written by a newer tool that adds + // metadata must still replay rather than fail to parse. + let dir = tempfile::tempdir().unwrap(); + let fixture = load_body( + &dir, + r#"{"fixture_version": 1, "entries": {}, "recorded_at": "2026-01-01"}"#, + ) + .unwrap(); + assert!(fixture.entries.is_empty()); + } + + #[test] + fn round_trip_preserves_unicode_keys_and_values() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("fixture.json"); + let original = FixtureFile { + fixture_version: FIXTURE_VERSION, + entries: HashMap::from([( + "invoke:ünïcodé".to_string(), + json!({ "memo": "héllo — wörld" }), + )]), + }; + + original.save(&path).unwrap(); + let loaded = FixtureFile::load(&path).unwrap(); + assert_eq!(loaded.entries, original.entries); + } + + // ── error paths ─────────────────────────────────────────────────── + + #[test] + fn load_reports_a_missing_file_with_its_path() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("absent.json"); + let err = FixtureFile::load(&path).unwrap_err(); + let message = err.to_string(); + assert!( + message.contains("Failed to read fixture file"), + "got: {err:#}" + ); + assert!(message.contains("absent.json"), "got: {message}"); + } + + #[test] + fn load_rejects_a_directory() { + let dir = tempfile::tempdir().unwrap(); + // Reading a directory as a string is an I/O failure, not a parse one. + let err = FixtureFile::load(dir.path()).unwrap_err(); + assert!( + err.to_string().contains("Failed to read fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_invalid_json() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, "not json at all").unwrap_err(); + assert!( + err.to_string().contains("Failed to parse fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_an_empty_file() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, "").unwrap_err(); + assert!( + err.to_string().contains("Failed to parse fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_a_json_array_root() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, "[]").unwrap_err(); + assert!( + err.to_string().contains("Failed to parse fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_a_non_numeric_version() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, r#"{"fixture_version": "1", "entries": {}}"#).unwrap_err(); + assert!( + err.to_string().contains("Failed to parse fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_entries_that_are_not_a_map() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, r#"{"fixture_version": 1, "entries": [1, 2]}"#).unwrap_err(); + assert!( + err.to_string().contains("Failed to parse fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_a_missing_entries_field() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, r#"{"fixture_version": 1}"#).unwrap_err(); + assert!( + err.to_string().contains("Failed to parse fixture file"), + "got: {err:#}" + ); + } + + #[test] + fn load_rejects_a_future_fixture_version() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, r#"{"fixture_version": 2, "entries": {}}"#).unwrap_err(); + let message = err.to_string(); + assert!(message.contains("Fixture version mismatch"), "got: {err:#}"); + assert!( + message.contains(&format!("expected {FIXTURE_VERSION} got 2")), + "got: {message}" + ); + } + + #[test] + fn load_rejects_a_stale_fixture_version() { + let dir = tempfile::tempdir().unwrap(); + let err = load_body(&dir, r#"{"fixture_version": 0, "entries": {}}"#).unwrap_err(); + assert!( + err.to_string().contains("Fixture version mismatch"), + "got: {err:#}" + ); + } + + #[test] + fn save_reports_a_path_that_cannot_be_written() { + let dir = tempfile::tempdir().unwrap(); + // The parent directory does not exist, so the write must fail with the + // path named rather than panicking. + let path = dir.path().join("missing-dir").join("fixture.json"); + let err = FixtureFile::new().save(&path).unwrap_err(); + let message = err.to_string(); + assert!( + message.contains("Failed to write fixture file"), + "got: {err:#}" + ); + assert!(message.contains("fixture.json"), "got: {message}"); + } +} diff --git a/cargo-budget-report/src/json_output.rs b/cargo-budget-report/src/json_output.rs index 67ef23a8..1bd30811 100644 --- a/cargo-budget-report/src/json_output.rs +++ b/cargo-budget-report/src/json_output.rs @@ -1,25 +1,81 @@ +//! Versioned JSON envelope for `cargo budget-report --json`. +//! +//! `--json` is the machine-readable interface, so its document shape is a +//! compatibility surface: CI scripts and downstream tools parse it. Rather +//! than emitting a bare array of snapshots, the output is wrapped in an +//! envelope carrying [`SCHEMA_VERSION`], which lets a consumer check the +//! version once and reject (or adapt to) a document it does not understand — +//! instead of failing later, somewhere deep in its own parsing, on a field +//! that moved. +//! +//! The wrapper is deliberately additive. Every snapshot inside `snapshots` is +//! the same [`crate::CostReport`] row the table, CSV and Markdown renderers +//! print, so nothing about a snapshot has to be re-learned for JSON output. +//! `json_output::render_json` is the single place that produces the envelope; +//! the versioning policy itself is documented in `docs/src/reference.md`. + use serde::Serialize; /// Current JSON schema version for budget report output. /// /// Increment this value when the JSON structure changes in a way that /// requires consumers to update their parsing logic (see -/// `docs/src/reference.md` for the full versioning policy). +/// `docs/src/reference.md` for the full versioning policy). Removing a field, +/// renaming one, changing a field's type, or changing the meaning of an +/// existing value all qualify. *Adding* a field does not: a consumer that +/// ignores unknown fields keeps working, which is why the envelope starts at +/// `1` rather than tracking every additive change. pub(crate) const SCHEMA_VERSION: u32 = 1; /// Top-level wrapper emitted by `cargo budget-report --json`. /// /// Every JSON document produced by the budget report is an object with a -/// `schema_version` integer and a `snapshots` array. The individual +/// `schema_version` integer and a `snapshots` array. The individual /// snapshot objects are unchanged from the pre-versioning format. +/// +/// ```json +/// { +/// "schema_version": 1, +/// "snapshots": [ +/// { +/// "package": "amm-pool-contract", +/// "function": "do_expensive_work", +/// "metric": "CPU Instructions", +/// "value": 2770850, +/// "limit": 3500000, +/// "pass": true +/// } +/// ] +/// } +/// ``` +/// +/// The `limit` and `pass` fields are present in `--check` mode only, and +/// `Option` fields are omitted entirely when unset (see [`crate::CostReport`]). #[derive(Serialize)] pub(crate) struct BudgetReportJson<'a> { + /// Schema version of the document being emitted; always + /// [`SCHEMA_VERSION`] for as long as this type is the only writer. schema_version: u32, + /// The report rows, borrowed rather than owned: rendering only serializes + /// them, so cloning each row into the envelope would allocate for nothing. snapshots: &'a [crate::CostReport], } /// Wrap the given report rows in the versioned JSON envelope. +/// +/// Returns pretty-printed JSON (two-space indent) because the output is read +/// by a human in CI logs at least as often as it is piped into a parser. +/// +/// # Panics +/// +/// The `expect` below is unreachable in practice: every field of the envelope +/// is a `u32`, a slice of [`crate::CostReport`], or — inside `CostReport` — a +/// `String`, a `&'static str` or an `Option` of a primitive, none of which can +/// fail to serialize. Returning a `Result` would push an error onto callers +/// that have no way to act on it, so the invariant is documented instead. pub(crate) fn render_json(reports: &[crate::CostReport]) -> String { + // The envelope borrows `reports` for the duration of the call; the only + // allocation here is the output `String` itself. let wrapper = BudgetReportJson { schema_version: SCHEMA_VERSION, snapshots: reports, @@ -29,9 +85,25 @@ pub(crate) fn render_json(reports: &[crate::CostReport]) -> String { #[cfg(test)] mod tests { + //! The envelope is the compatibility surface for `--json` consumers, so + //! these tests pin its shape: which top-level keys exist, how many + //! snapshots it carries, and which snapshot fields are omitted when unset. use super::*; use crate::CostReport; + /// A fully populated row — the shape `--check` emits for a metric that has + /// both a measured value and a configured limit. + fn report(package: &str, function: &str) -> CostReport { + CostReport { + package: package.to_string(), + function: function.to_string(), + metric: "CPU Instructions", + value: Some(12345), + limit: Some(20000), + pass: Some(true), + } + } + #[test] fn budget_report_json_contains_schema_version() { let reports = vec![CostReport { @@ -55,4 +127,62 @@ mod tests { fn schema_version_matches_documented_current_version() { assert_eq!(SCHEMA_VERSION, 1); } + + #[test] + fn empty_report_list_renders_an_empty_snapshots_array() { + // A workspace where nothing was measured still has to produce a + // document a consumer can parse. + let json_str = render_json(&[]); + let json: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(json["schema_version"], SCHEMA_VERSION); + assert_eq!(json["snapshots"].as_array().map(Vec::len), Some(0)); + } + + #[test] + fn envelope_carries_only_the_schema_version_and_snapshots() { + // Adding a top-level field is a schema change, so it should show up as + // a deliberate edit to this assertion rather than as a side effect. + let json_str = render_json(&[report("pkg", "fn")]); + let json: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + let keys: Vec<&str> = json + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + assert_eq!(keys, vec!["schema_version", "snapshots"]); + } + + #[test] + fn row_order_is_preserved() { + let reports = vec![report("pkg-b", "second"), report("pkg-a", "first")]; + let json: serde_json::Value = serde_json::from_str(&render_json(&reports)).unwrap(); + assert_eq!(json["snapshots"][0]["package"], "pkg-b"); + assert_eq!(json["snapshots"][1]["package"], "pkg-a"); + } + + #[test] + fn unset_option_fields_are_omitted_from_a_snapshot() { + // Without `--check` there is no limit to report; the key is skipped + // rather than emitted as `null`. + let reports = vec![CostReport { + package: "pkg".to_string(), + function: "fn".to_string(), + metric: "Memory Bytes", + value: Some(7), + limit: None, + pass: None, + }]; + let json: serde_json::Value = serde_json::from_str(&render_json(&reports)).unwrap(); + let snapshot = json["snapshots"][0].as_object().unwrap(); + assert!(!snapshot.contains_key("limit"), "got: {snapshot:?}"); + assert!(!snapshot.contains_key("pass"), "got: {snapshot:?}"); + } + + #[test] + fn output_is_pretty_printed_multi_line_json() { + let json_str = render_json(&[report("pkg", "fn")]); + assert!(json_str.contains('\n'), "expected a pretty-printed document"); + assert!(json_str.starts_with("{\n"), "got: {json_str}"); + } } diff --git a/cargo-budget-report/src/validate.rs b/cargo-budget-report/src/validate.rs index 9f0bbd40..75add47a 100644 --- a/cargo-budget-report/src/validate.rs +++ b/cargo-budget-report/src/validate.rs @@ -1,5 +1,6 @@ use crate::BudgetToml; use anyhow::{Context, Result}; +use std::collections::HashSet; use std::io::Write; use std::process::{Command, Stdio}; @@ -56,6 +57,8 @@ pub fn decode_with_cli(xdr_b64: &str) -> Result { .context("failed to spawn stellar xdr decode")?; { + // Scoped so the mutable borrow of `child.stdin` ends here: the handle + // is released before `wait_with_output` takes the child by value. let stdin = child.stdin.as_mut().context("failed to open stdin")?; stdin .write_all(xdr_b64.as_bytes()) @@ -270,23 +273,39 @@ pub(crate) fn validate_budget_toml( // functions to compare against (e.g. nothing was built). if !available_functions.is_empty() { if let Some(toml::Value::Table(fns)) = top.get("functions") { - let avail: Vec<&str> = available_functions.iter().map(String::as_str).collect(); - let avail_list = available_functions.join(", "); + // Membership is asked once per configured function, so a set + // replaces the previous scan of the whole export list for + // every name. The typo candidates and the rendered + // "available functions" hint are built lazily, on the first + // name that is actually missing, so a config where every + // function resolves allocates neither. + let known: HashSet<&str> = + available_functions.iter().map(String::as_str).collect(); + let mut hint: Option<(Vec<&str>, String)> = None; for name in fns.keys() { - if !available_functions.iter().any(|f| f == name) { - let suggestion = closest_match(name, &avail); - errors.push(ValidationError::new( - "budget.toml [functions]", - match suggestion { - Some(s) => format!( - "function `{name}` is configured in budget.toml but does not exist in the workspace (did you mean `{s}`?). Available functions: {avail_list}" - ), - None => format!( - "function `{name}` is configured in budget.toml but does not exist in the workspace. Available functions: {avail_list}" - ), - }, - )); + if known.contains(name.as_str()) { + continue; } + let (candidate_list, avail_list) = hint.get_or_insert_with(|| { + ( + available_functions.iter().map(String::as_str).collect(), + available_functions.join(", "), + ) + }); + let suggestion = closest_match(name, candidate_list.as_slice()); + errors.push(ValidationError::new( + "budget.toml [functions]", + match suggestion { + Some(s) => format!( + "function `{name}` is configured in budget.toml but does not exist in the workspace (did you mean `{s}`?). Available functions: {}", + avail_list.as_str() + ), + None => format!( + "function `{name}` is configured in budget.toml but does not exist in the workspace. Available functions: {}", + avail_list.as_str() + ), + }, + )); } } } @@ -297,64 +316,108 @@ pub(crate) fn validate_budget_toml( // surfaced by deserializing into the real `BudgetToml`. The top-level // struct intentionally does not deny unknown keys, so those are not // double-counted here. - if let Err(e) = toml::from_str::(content) { - let msg = e.to_string(); - let already_covered = KNOWN_TOP_LEVEL_KEYS.iter().any(|k| msg.contains(k)); - if !already_covered { - errors.push(ValidationError::new( - "budget.toml", - format!("schema error: {msg}"), - )); + // + // The text is deserialized exactly once: the successful value is handed + // back below instead of being discarded and parsed a second time, which is + // what the previous `if errors.is_empty() { toml::from_str(content) }` tail + // did on every valid file. + match toml::from_str::(content) { + Ok(config) if errors.is_empty() => Ok(config), + Ok(_) => Err(errors), + Err(e) => { + let msg = e.to_string(); + let already_covered = KNOWN_TOP_LEVEL_KEYS.iter().any(|k| msg.contains(k)); + if !already_covered { + errors.push(ValidationError::new( + "budget.toml", + format!("schema error: {msg}"), + )); + } else if errors.is_empty() { + // The message already names a known key, so it was not + // re-reported as a separate schema error, and nothing else + // rejected the document. Surface the raw deserialization + // failure anyway: a file the schema check does not cover must + // not pass silently. + errors.push(ValidationError::new("budget.toml", msg)); + } + Err(errors) } } - - if errors.is_empty() { - toml::from_str(content) - .map_err(|e| vec![ValidationError::new("budget.toml", format!("{e}"))]) - } else { - Err(errors) - } } -/// Levenshtein edit distance between two strings. -#[allow(clippy::needless_range_loop)] -fn edit_distance(a: &str, b: &str) -> usize { - let a: Vec = a.chars().collect(); - let b: Vec = b.chars().collect(); - let n = a.len(); - let m = b.len(); - let mut dp = vec![vec![0usize; m + 1]; n + 1]; - for i in 0..=n { - dp[i][0] = i; - } - for j in 0..=m { - dp[0][j] = j; - } - for i in 1..=n { - for j in 1..=m { - let cost = if a[i - 1] == b[j - 1] { 0 } else { 1 }; - dp[i][j] = (dp[i - 1][j] + 1) - .min(dp[i][j - 1] + 1) - .min(dp[i - 1][j - 1] + cost); +/// Levenshtein edit distance between two strings, computed into a +/// caller-owned scratch buffer. +/// +/// Only the previous row of the recurrence is retained (a single `Vec` +/// of `b`'s length), instead of the full `(n + 1) × (m + 1)` matrix the +/// previous implementation allocated — which matters because this runs once +/// per candidate for every name in `budget.toml` that fails to resolve. +/// Characters are pulled straight out of the `&str`s as they are visited, so +/// the two `Vec` copies per call are gone as well. +/// +/// `scratch` is taken by reference rather than allocated per call so a caller +/// that measures one name against many candidates pays for the row once; the +/// buffer is cleared and resized as needed, so it is safe to reuse for any +/// pair of inputs. +fn edit_distance_into(a: &str, b: &str, scratch: &mut Vec) -> usize { + let m = b.chars().count(); + scratch.clear(); + scratch.resize(m + 1, 0); + for (j, cell) in scratch.iter_mut().enumerate() { + // Row 0 of the recurrence: the cost of deleting every character of `b`. + *cell = j; + } + + for (i, a_char) in a.chars().enumerate() { + // `prev_diag` carries `dp[i - 1][j - 1]` into the next inner step; the + // cell it is read from still holds the previous row at this point. + let mut prev_diag = scratch[0]; + scratch[0] = i + 1; + for (j, b_char) in b.chars().enumerate() { + let above = scratch[j + 1]; + let cost = usize::from(a_char != b_char); + scratch[j + 1] = (above + 1) + .min(scratch[j] + 1) + .min(prev_diag + cost); + prev_diag = above; } } - dp[n][m] + + scratch[m] } /// Return the closest `option` to `candidate` when the edit distance is small /// enough to be a plausible typo, else `None`. +/// +/// One DP row is reused across every candidate (see [`edit_distance_into`]), +/// and only the winning option is copied into a `String`, once, at the end — +/// the previous version allocated a `String` for every candidate it examined, +/// including the ones it immediately discarded. fn closest_match(candidate: &str, options: &[&str]) -> Option { - let threshold = if candidate.chars().count() <= 4 { 1 } else { 2 }; - let mut best: Option<(usize, String)> = None; + let candidate_len = candidate.chars().count(); + let threshold = if candidate_len <= 4 { 1 } else { 2 }; + + let mut scratch: Vec = Vec::new(); + let mut best: Option<(usize, &str)> = None; for opt in options { - let d = edit_distance(candidate, opt); + // Edit distance is at least the difference in lengths, so an option + // that is already further away than `threshold` characters can never + // satisfy the final check. Skipping it here keeps the `O(n × m)` DP + // off the common "nothing is close" path; candidates above the + // threshold were rejected by the final check anyway, so the result is + // unchanged. + if opt.chars().count().abs_diff(candidate_len) > threshold { + continue; + } + let d = edit_distance_into(candidate, opt, &mut scratch); match best { - Some((bd, _)) if d >= bd => {} - _ => best = Some((d, (*opt).to_string())), + Some((best_d, _)) if d >= best_d => {} + _ => best = Some((d, opt)), } } + match best { - Some((d, s)) if d > 0 && d <= threshold && d < candidate.chars().count() => Some(s), + Some((d, s)) if d > 0 && d <= threshold && d < candidate_len => Some(s.to_string()), _ => None, } } @@ -509,6 +572,169 @@ cpu_limit = 5_000_000 Some("cpu_limit".to_string()) ); } + + // ── edit-distance / typo-suggestion edge cases ──────────────────── + + /// Single-shot wrapper over the shared-scratch DP so the assertions below + /// read like the old helper; production code reuses one buffer across + /// every candidate it measures. + fn edit_distance(a: &str, b: &str) -> usize { + let mut scratch = Vec::new(); + edit_distance_into(a, b, &mut scratch) + } + + #[test] + fn edit_distance_matches_known_values() { + assert_eq!(edit_distance("", ""), 0); + assert_eq!(edit_distance("", "abc"), 3); + assert_eq!(edit_distance("abc", ""), 3); + assert_eq!(edit_distance("kitten", "sitting"), 3); + assert_eq!(edit_distance("cpu_limit", "cpu_limit"), 0); + assert_eq!(edit_distance("cpu_lmit", "cpu_limit"), 1); + } + + #[test] + fn edit_distance_is_symmetric_over_multibyte_chars() { + assert_eq!( + edit_distance("tolernce", "tolerance"), + edit_distance("tolerance", "tolernce") + ); + // Characters are compared, not bytes: `é` is one substitution away + // from `e` even though it encodes as two bytes. + assert_eq!(edit_distance("café", "cafe"), 1); + assert_eq!(edit_distance("日本", "日本語"), 1); + } + + #[test] + fn edit_distance_reuses_a_single_dp_row() { + // One row of the recurrence, never the full (n + 1) × (m + 1) table: + // this is what keeps the typo scan allocation-light. + let mut scratch = Vec::new(); + assert_eq!(edit_distance_into("aaaaaaaa", "bbbbbbbb", &mut scratch), 8); + assert_eq!(scratch.len(), 9, "one row, sized by the second operand"); + // A buffer left over from a longer comparison still answers a shorter + // one correctly, in both directions. + assert_eq!(edit_distance_into("ab", "ab", &mut scratch), 0); + assert_eq!(scratch.len(), 3); + assert_eq!(edit_distance_into("abc", "ab", &mut scratch), 1); + assert_eq!(scratch.len(), 3); + } + + #[test] + fn closest_match_skips_candidates_of_impossible_length() { + // Edit distance is at least the length difference, so a candidate + // whose length already exceeds the threshold can never win. + assert_eq!( + closest_match("cpu_limit", &["a_much_longer_function_name"]), + None + ); + assert_eq!(closest_match("", &["anything"]), None); + } + + #[test] + fn closest_match_prefers_the_first_equally_distant_candidate() { + // Both options are one edit away; the first one examined wins, which + // is what the previous implementation did too. + assert_eq!(closest_match("aa", &["ab", "ba"]), Some("ab".to_string())); + } + + #[test] + fn closest_match_ignores_an_exact_name() { + // Distance zero is not a typo, so an exact match is never suggested + // back to the user. + assert_eq!(closest_match("tolerance", &["tolerance"]), None); + } + + #[test] + fn closest_match_picks_the_nearest_of_many_options() { + let options = ["tolerance", "network", "source", "margin", "scenarios"]; + assert_eq!(closest_match("margn", &options), Some("margin".to_string())); + assert_eq!( + closest_match("scenerios", &options), + Some("scenarios".to_string()) + ); + // Nothing is within the threshold of `src`, so no suggestion (and no + // distance computation) is produced for the long options. + assert_eq!(closest_match("src", &options), None); + } + + #[test] + fn closest_match_is_case_sensitive() { + assert_eq!( + closest_match("Tolerance", &["tolerance"]), + Some("tolerance".to_string()) + ); + } + + // ── validate_budget_toml success path and error combination ─────── + + #[test] + fn valid_config_returns_the_parsed_values() { + // The successful deserialization is handed back to the caller rather + // than discarded and parsed a second time, so the values it carries + // must be the ones from the file. + let toml = "tolerance = 0.25\n\n[functions.do_expensive_work]\ncpu_limit = 5_000_000\n"; + let parsed = validate_budget_toml(toml, &avail()).expect("expected a valid config"); + assert_eq!(parsed.tolerance, Some(0.25)); + assert!(parsed.functions.contains_key("do_expensive_work")); + } + + #[test] + fn every_configured_function_present_produces_no_errors() { + let toml = "[functions.do_expensive_work]\ncpu_limit = 1\n\n\ + [functions.require_auth_only]\ncpu_limit = 1\n"; + assert!( + validate_budget_toml(toml, &avail()).is_ok(), + "no name is missing, so no typo hint is built and no error raised" + ); + } + + #[test] + fn each_missing_function_reports_the_available_list() { + let toml = "[functions.depsoit]\ncpu_limit = 1\n\n[functions.wthdraw]\ncpu_limit = 1\n"; + let errs = validate_budget_toml(toml, &avail()).unwrap_err(); + assert_eq!(errs.len(), 2, "errors: {errs:?}"); + for err in &errs { + assert_eq!(err.location, "budget.toml [functions]"); + assert!( + err.message.contains("Available functions: do_expensive_work"), + "got: {}", + err.message + ); + } + } + + #[test] + fn top_level_type_error_is_still_reported() { + // `network` is a known key, so the deserialization failure is not + // re-reported as an extra schema error; it must still reach the + // caller rather than letting the document pass. + let errs = validate_budget_toml("network = 1\n", &avail()).unwrap_err(); + assert_eq!(errs.len(), 1, "errors: {errs:?}"); + assert!( + errs[0].message.contains("network"), + "got: {}", + errs[0].message + ); + } + + #[test] + fn unknown_key_is_reported_even_when_the_schema_check_also_fails() { + let toml = "tolernce = 0.1\n\n[functions.do_expensive_work]\ncpu_limit = \"high\"\n"; + let errs = validate_budget_toml(toml, &avail()).unwrap_err(); + assert!( + errs.iter().any(|e| e.message.contains("tolernce")), + "errors: {errs:?}" + ); + } + + #[test] + fn unparseable_toml_reports_a_single_parse_error() { + let errs = validate_budget_toml("this is not toml = =\n", &avail()).unwrap_err(); + assert_eq!(errs.len(), 1); + assert_eq!(errs[0].location, "budget.toml"); + assert!(errs[0].message.contains("could not parse TOML")); + } } #[cfg(test)] From 879cefc9ba5f34351c95ff479dbdc4d75a21d639 Mon Sep 17 00:00:00 2001 From: Allison Muyideen Opeyemi Date: Wed, 30 Sep 2026 11:02:19 +0100 Subject: [PATCH 2/2] style: apply cargo fmt to the lines this PR added Formatting only, to satisfy the repo's `cargo fmt --all -- --check` lint gate. No behaviour changed. --- cargo-budget-report/src/contract_exports.rs | 4 +++- cargo-budget-report/src/json_output.rs | 5 ++++- cargo-budget-report/src/validate.rs | 10 ++++------ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/cargo-budget-report/src/contract_exports.rs b/cargo-budget-report/src/contract_exports.rs index 01980e37..a17c0f67 100644 --- a/cargo-budget-report/src/contract_exports.rs +++ b/cargo-budget-report/src/contract_exports.rs @@ -395,7 +395,9 @@ mod tests { #[test] fn no_function_exports_diagnostic_names_the_missing_attributes() { - let message = ExportScan::NoFunctionExports.diagnostic("my-contract").unwrap(); + let message = ExportScan::NoFunctionExports + .diagnostic("my-contract") + .unwrap(); assert!(message.contains("my-contract"), "got: {message}"); assert!(message.contains("plain library"), "got: {message}"); assert!(message.contains("#[contract]"), "got: {message}"); diff --git a/cargo-budget-report/src/json_output.rs b/cargo-budget-report/src/json_output.rs index 1bd30811..d5e58971 100644 --- a/cargo-budget-report/src/json_output.rs +++ b/cargo-budget-report/src/json_output.rs @@ -182,7 +182,10 @@ mod tests { #[test] fn output_is_pretty_printed_multi_line_json() { let json_str = render_json(&[report("pkg", "fn")]); - assert!(json_str.contains('\n'), "expected a pretty-printed document"); + assert!( + json_str.contains('\n'), + "expected a pretty-printed document" + ); assert!(json_str.starts_with("{\n"), "got: {json_str}"); } } diff --git a/cargo-budget-report/src/validate.rs b/cargo-budget-report/src/validate.rs index 75add47a..8cc656f2 100644 --- a/cargo-budget-report/src/validate.rs +++ b/cargo-budget-report/src/validate.rs @@ -279,8 +279,7 @@ pub(crate) fn validate_budget_toml( // "available functions" hint are built lazily, on the first // name that is actually missing, so a config where every // function resolves allocates neither. - let known: HashSet<&str> = - available_functions.iter().map(String::as_str).collect(); + let known: HashSet<&str> = available_functions.iter().map(String::as_str).collect(); let mut hint: Option<(Vec<&str>, String)> = None; for name in fns.keys() { if known.contains(name.as_str()) { @@ -376,9 +375,7 @@ fn edit_distance_into(a: &str, b: &str, scratch: &mut Vec) -> usize { for (j, b_char) in b.chars().enumerate() { let above = scratch[j + 1]; let cost = usize::from(a_char != b_char); - scratch[j + 1] = (above + 1) - .min(scratch[j] + 1) - .min(prev_diag + cost); + scratch[j + 1] = (above + 1).min(scratch[j] + 1).min(prev_diag + cost); prev_diag = above; } } @@ -697,7 +694,8 @@ cpu_limit = 5_000_000 for err in &errs { assert_eq!(err.location, "budget.toml [functions]"); assert!( - err.message.contains("Available functions: do_expensive_work"), + err.message + .contains("Available functions: do_expensive_work"), "got: {}", err.message );