diff --git a/Cargo.lock b/Cargo.lock index 38649068..cd90b9e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2570,6 +2570,7 @@ dependencies = [ "mockable", "netsuke", "ninja_env", + "proptest", "sha2", "tempfile", "thiserror 1.0.69", diff --git a/src/ir/cmd_interpolate.rs b/src/ir/cmd_interpolate.rs index edd91e90..4d4e4e18 100644 --- a/src/ir/cmd_interpolate.rs +++ b/src/ir/cmd_interpolate.rs @@ -11,6 +11,9 @@ use shell_quote::{QuoteRefExt, Sh}; use super::IrGenError; +pub(crate) const INS_TOKEN: &str = "__NETSUKE_INS_PLACEHOLDER__"; +pub(crate) const OUTS_TOKEN: &str = "__NETSUKE_OUTS_PLACEHOLDER__"; + /// Returns `true` when the command contains an odd number of backticks. /// /// # Examples @@ -148,8 +151,8 @@ fn find_substitution<'a>( }) })) .or_else(|| { - try_match_token(chars, pos, "__NETSUKE_INS_PLACEHOLDER__", ins) - .or_else(|| try_match_token(chars, pos, "__NETSUKE_OUTS_PLACEHOLDER__", outs)) + try_match_token(chars, pos, INS_TOKEN, ins) + .or_else(|| try_match_token(chars, pos, OUTS_TOKEN, outs)) }) } @@ -207,6 +210,10 @@ fn substitute(template: &str, ins: &[String], outs: &[String]) -> String { out } +#[cfg(test)] +#[path = "cmd_interpolate_property_tests.rs"] +mod property_tests; + #[cfg(test)] mod tests { use super::*; @@ -258,7 +265,7 @@ mod tests { #[test] fn interpolate_command_replaces_template_placeholders() { let command = interpolate_command( - "__NETSUKE_INS_PLACEHOLDER__ $out __NETSUKE_OUTS_PLACEHOLDER__", + &format!("{INS_TOKEN} $out {OUTS_TOKEN}"), &[Utf8PathBuf::from("in")], &[Utf8PathBuf::from("out")], ) diff --git a/src/ir/cmd_interpolate_property_tests.rs b/src/ir/cmd_interpolate_property_tests.rs new file mode 100644 index 00000000..bd840e2a --- /dev/null +++ b/src/ir/cmd_interpolate_property_tests.rs @@ -0,0 +1,62 @@ +//! Property tests for command interpolation token boundaries. +//! +//! These properties ensure `interpolate_command` replaces placeholders outside +//! backticks with quoted paths, preserves `$in`, `$out`, and long placeholder +//! tokens verbatim inside backtick-delimited regions, and rejects unbalanced +//! backtick input as an invalid command. + +use proptest::prelude::*; +use test_support::ninja_gen::paths_strategy; + +use super::{INS_TOKEN, IrGenError, OUTS_TOKEN, interpolate_command}; + +fn safe_text_strategy() -> impl Strategy { + // Empty fragments are intentional: surrounding command text may be absent, + // and trimming whitespace-only generated text exercises that boundary. + "[a-zA-Z0-9_./ -]{0,24}".prop_map(|text| text.trim().to_owned()) +} + +proptest! { + #[test] + fn dollar_tokens_inside_backticks_are_preserved(prefix in safe_text_strategy(), suffix in safe_text_strategy(), inputs in paths_strategy("in", 1..10), outputs in paths_strategy("out", 1..10)) { + let template = format!("echo {prefix} `printf '$in $out'` {suffix}"); + let command = interpolate_command(&template, &inputs, &outputs).expect("balanced command should interpolate"); + + prop_assert!(command.contains("`printf '$in $out'`")); + } + + #[test] + fn long_placeholders_outside_backticks_are_replaced(inputs in paths_strategy("in", 1..10), outputs in paths_strategy("out", 1..10)) { + let command = interpolate_command( + &format!("echo {INS_TOKEN} then {OUTS_TOKEN}"), + &inputs, + &outputs, + ).expect("command should interpolate"); + + prop_assert!(!command.contains(INS_TOKEN)); + prop_assert!(!command.contains(OUTS_TOKEN)); + for input in inputs { + prop_assert!(command.contains(input.as_str())); + } + for output in outputs { + prop_assert!(command.contains(output.as_str())); + } + } + + #[test] + fn tokens_inside_backticks_are_preserved_verbatim(token in prop::sample::select(vec!["$in", "$out", INS_TOKEN, OUTS_TOKEN]), inputs in paths_strategy("in", 1..10), outputs in paths_strategy("out", 1..10)) { + let template = format!("echo `{token}`"); + let command = interpolate_command(&template, &inputs, &outputs).expect("balanced command should interpolate"); + + prop_assert_eq!(command, template); + } + + #[test] + fn unbalanced_backticks_are_rejected(prefix in safe_text_strategy(), suffix in safe_text_strategy(), inputs in paths_strategy("in", 1..10), outputs in paths_strategy("out", 1..10)) { + let template = format!("echo {prefix} ` $in {suffix}"); + let err = interpolate_command(&template, &inputs, &outputs).expect_err("unbalanced backticks should fail"); + + let is_invalid_command = matches!(err, IrGenError::InvalidCommand { .. }); + prop_assert!(is_invalid_command); + } +} diff --git a/src/ir/cycle.rs b/src/ir/cycle.rs index 2a3c6ca4..dedc9775 100644 --- a/src/ir/cycle.rs +++ b/src/ir/cycle.rs @@ -164,6 +164,10 @@ fn canonicalize_cycle(mut cycle: Vec) -> Vec { cycle } +#[cfg(test)] +#[path = "cycle_property_tests.rs"] +mod property_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/src/ir/cycle_property_tests.rs b/src/ir/cycle_property_tests.rs new file mode 100644 index 00000000..554bfe84 --- /dev/null +++ b/src/ir/cycle_property_tests.rs @@ -0,0 +1,214 @@ +//! Property tests for IR cycle detection invariants. +//! +//! These properties map directly to the detector contract: `dag_has_no_cycle` +//! rejects false positives, `back_edge_produces_cycle` covers explicit and +//! implicit dependency traversal, `order_only_back_edge_has_no_cycle` proves +//! order-only dependencies stay out of traversal, `missing_dependencies...` +//! checks missing-edge reporting, and `dag_results_are_stable...` guards +//! determinism across `HashMap` insertion order. + +use std::collections::{HashMap, HashSet}; + +use camino::Utf8PathBuf; +use proptest::prelude::*; + +use super::{BuildEdge, analyse}; + +fn node(index: usize) -> Utf8PathBuf { + Utf8PathBuf::from(format!("n{index}")) +} + +fn build_edge(output: Utf8PathBuf, deps: Vec) -> BuildEdge { + BuildEdge { + action_id: "id".into(), + inputs: deps, + implicit_deps: Vec::new(), + explicit_outputs: vec![output], + implicit_outputs: Vec::new(), + order_only_deps: Vec::new(), + phony: false, + always: false, + } +} + +fn push_dependency(edge: &mut BuildEdge, dep: Utf8PathBuf, is_implicit: bool) { + if is_implicit { + edge.implicit_deps.push(dep); + } else { + edge.inputs.push(dep); + } +} + +fn dag_from_edges( + node_count: usize, + edges: &[(usize, usize, bool)], +) -> HashMap { + let mut graph = HashMap::new(); + for index in 0..node_count { + let output = node(index); + let mut edge = build_edge(output.clone(), Vec::new()); + for &(from, to, is_implicit) in edges { + if from == index && to < from { + push_dependency(&mut edge, node(to), is_implicit); + } + } + graph.insert(output, edge); + } + graph +} + +fn graph_strategy() -> impl Strategy> { + ( + 1usize..50, + prop::collection::vec((0usize..50, 0usize..50, any::()), 0..250), + ) + .prop_map(|(node_count, edges)| dag_from_edges(node_count, &edges)) +} + +fn cyclic_graph_strategy() +-> impl Strategy, Utf8PathBuf, Utf8PathBuf)> { + (2usize..50, any::()).prop_map(|(node_count, back_edge_is_implicit)| { + let chain_edges: Vec<_> = (1..node_count) + .map(|index| (index, index - 1, false)) + .collect(); + let mut graph = dag_from_edges(node_count, &chain_edges); + let from = node(0); + let to = node(node_count - 1); + let edge = graph.get_mut(&from).expect("generated node must exist"); + if back_edge_is_implicit { + edge.implicit_deps.push(to.clone()); + } else { + edge.inputs.push(to.clone()); + } + (graph, from, to) + }) +} + +fn order_only_back_edge_strategy() -> impl Strategy> { + (2usize..50).prop_map(|node_count| { + let chain_edges: Vec<_> = (1..node_count) + .map(|index| (index, index - 1, false)) + .collect(); + let mut graph = dag_from_edges(node_count, &chain_edges); + graph + .get_mut(&node(0)) + .expect("generated node must exist") + .order_only_deps + .push(node(node_count - 1)); + graph + }) +} + +fn missing_graph_strategy() -> impl Strategy> { + ( + graph_strategy(), + prop::collection::vec((0usize..50, 0usize..20, any::()), 1..50), + ) + .prop_map(|(mut graph, missing_edges)| { + let node_count = graph.len(); + for (from, missing_index, is_implicit) in missing_edges { + let bounded_from = from.min(node_count.saturating_sub(1)); + let Some(edge) = graph.get_mut(&node(bounded_from)) else { + continue; + }; + let missing = Utf8PathBuf::from(format!("missing-{missing_index}")); + push_dependency(edge, missing, is_implicit); + } + graph + }) +} + +// Deliberately accepts short, duplicate, or out-of-range order vectors. Chosen +// entries are inserted first, then `or_insert` fills any gaps so the final map +// always contains every original target while still varying insertion order. +fn rebuild_in_order( + graph: &HashMap, + order: &[usize], +) -> HashMap { + let mut entries: Vec<_> = graph + .iter() + .map(|(key, edge)| (key.clone(), edge.clone())) + .collect(); + entries.sort_by(|left, right| left.0.cmp(&right.0)); + let mut rebuilt = HashMap::new(); + for index in order { + let bounded_index = (*index).min(entries.len().saturating_sub(1)); + let Some((key, edge)) = entries.get(bounded_index) else { + continue; + }; + rebuilt.insert(key.clone(), edge.clone()); + } + for (key, edge) in entries { + rebuilt.entry(key).or_insert(edge); + } + rebuilt +} + +fn sorted_missing(report: &super::CycleDetectionReport) -> Vec<(Utf8PathBuf, Utf8PathBuf)> { + let mut missing = report.missing_dependencies.clone(); + missing.sort(); + missing +} + +fn injected_missing_deps(graph: &HashMap) -> HashSet { + graph + .values() + .flat_map(|edge| edge.inputs.iter().chain(&edge.implicit_deps)) + .filter(|dep| dep.as_str().starts_with("missing-")) + .cloned() + .collect() +} + +proptest! { + #[test] + fn dag_has_no_cycle(graph in graph_strategy()) { + prop_assert!(analyse(&graph).cycle.is_none()); + } + + #[test] + fn back_edge_produces_cycle((graph, from, to) in cyclic_graph_strategy()) { + let cycle = analyse(&graph).cycle.expect("back-edge should produce cycle"); + let cycle_edges: HashSet<_> = cycle + .windows(2) + .filter_map(|pair| { + let [left, right] = pair else { + return None; + }; + Some((left.clone(), right.clone())) + }) + .collect(); + prop_assert!(cycle_edges.contains(&(from, to))); + } + + #[test] + fn order_only_back_edge_has_no_cycle(graph in order_only_back_edge_strategy()) { + prop_assert!(analyse(&graph).cycle.is_none()); + } + + #[test] + fn missing_dependencies_are_absent_targets(graph in missing_graph_strategy()) { + let injected_missing = injected_missing_deps(&graph); + let report = analyse(&graph); + let reported_missing: HashSet<_> = report + .missing_dependencies + .iter() + .map(|(_, dep)| dep.clone()) + .collect(); + + for dep in injected_missing { + prop_assert!(reported_missing.contains(&dep)); + } + for (_, dep) in report.missing_dependencies { + prop_assert!(!graph.contains_key(&dep)); + } + } + + #[test] + fn dag_results_are_stable_across_insertion_orders(graph in missing_graph_strategy(), order in prop::collection::vec(0usize..50, 0..100)) { + let baseline = analyse(&graph); + let reordered = rebuild_in_order(&graph, &order); + let reordered_report = analyse(&reordered); + prop_assert_eq!(&baseline.cycle, &reordered_report.cycle); + prop_assert_eq!(sorted_missing(&baseline), sorted_missing(&reordered_report)); + } +} diff --git a/src/ir/mod.rs b/src/ir/mod.rs index c3e5a06b..6ad5d43e 100644 --- a/src/ir/mod.rs +++ b/src/ir/mod.rs @@ -29,4 +29,5 @@ mod cycle; mod from_manifest; mod graph; +pub(crate) use cmd_interpolate::{INS_TOKEN, OUTS_TOKEN}; pub use graph::{Action, BuildEdge, BuildGraph, IrGenError}; diff --git a/src/manifest/render.rs b/src/manifest/render.rs index d5e0d055..7e951106 100644 --- a/src/manifest/render.rs +++ b/src/manifest/render.rs @@ -7,6 +7,7 @@ //! so that [`crate::ir::cmd_interpolate`] can substitute them later. use super::ManifestValue; use crate::ast::{NetsukeManifest, Recipe, StringOrList, Target, Vars}; +use crate::ir::{INS_TOKEN, OUTS_TOKEN}; use anyhow::{Context, Result}; use minijinja::Environment; @@ -121,10 +122,10 @@ fn render_recipe_str_with( let mut recipe_ctx = ctx.clone(); recipe_ctx .entry("ins".into()) - .or_insert_with(|| ManifestValue::String("__NETSUKE_INS_PLACEHOLDER__".into())); + .or_insert_with(|| ManifestValue::String(INS_TOKEN.into())); recipe_ctx .entry("outs".into()) - .or_insert_with(|| ManifestValue::String("__NETSUKE_OUTS_PLACEHOLDER__".into())); + .or_insert_with(|| ManifestValue::String(OUTS_TOKEN.into())); render_str_with(env, tpl, &recipe_ctx, what) } diff --git a/src/ninja_gen.rs b/src/ninja_gen.rs index aaf62fe5..b68741cc 100644 --- a/src/ninja_gen.rs +++ b/src/ninja_gen.rs @@ -303,7 +303,9 @@ impl Display for DisplayEdge<'_> { writeln!(f) } } - +#[cfg(test)] +#[path = "ninja_gen_property_tests.rs"] +mod property_tests; #[cfg(test)] mod tests { use super::*; diff --git a/src/ninja_gen_property_tests.rs b/src/ninja_gen_property_tests.rs new file mode 100644 index 00000000..2e0a527d --- /dev/null +++ b/src/ninja_gen_property_tests.rs @@ -0,0 +1,106 @@ +//! Property tests for Ninja build edge separator formatting. +//! +//! `implicit_deps_separator_precedes_order_only_separator` verifies the +//! dependency-side ` | ` appears after explicit inputs and before ` || `. +//! `implicit_deps_separator_is_absent_when_empty` verifies the dependency-side +//! implicit separator is omitted when `implicit_deps` is empty. + +use camino::Utf8PathBuf; +use proptest::prelude::*; +use test_support::ninja_gen::paths_strategy; + +use super::DisplayEdge; +use crate::ir::BuildEdge; + +fn path_strategy(prefix: &'static str) -> impl Strategy { + (0usize..100).prop_map(move |index| Utf8PathBuf::from(format!("{prefix}{index}"))) +} + +fn edge_strategy() -> impl Strategy { + edge_strategy_with_ranges(0..5, 0..5, 0..5) +} + +fn edge_strategy_with_ranges( + input_range: std::ops::Range, + implicit_range: std::ops::Range, + order_only_range: std::ops::Range, +) -> impl Strategy { + ( + "[a-z][a-z0-9_]{0,8}", + paths_strategy("in", input_range), + prop::collection::vec(path_strategy("out"), 1..5), + paths_strategy("iout", 0..5), + paths_strategy("imp", implicit_range), + paths_strategy("order", order_only_range), + ) + .prop_map( + |( + action_id, + inputs, + explicit_outputs, + implicit_outputs, + implicit_deps, + order_only_deps, + )| { + BuildEdge { + action_id, + inputs, + implicit_deps, + explicit_outputs, + implicit_outputs, + order_only_deps, + phony: false, + always: false, + } + }, + ) +} + +fn format_edge(edge: &BuildEdge) -> String { + DisplayEdge { + edge, + action_restat: false, + } + .to_string() +} + +fn build_line(formatted: &str) -> &str { + formatted.lines().next().expect("build line") +} + +fn dependency_side(line: &str) -> &str { + line.split_once(": ") + .map(|(_, deps)| deps) + .expect("build line should contain rule separator") +} + +fn bare_pipe_position(line: &str) -> Option { + line.match_indices(" | ").map(|(index, _)| index).next() +} + +proptest! { + #[test] + fn implicit_deps_separator_precedes_order_only_separator(edge in edge_strategy_with_ranges(1..5, 1..5, 1..5)) { + let formatted = format_edge(&edge); + let line = build_line(&formatted); + let deps = dependency_side(line); + let implicit_pos = bare_pipe_position(deps).expect("implicit separator should be emitted"); + let order_pos = deps.find(" || ").expect("order-only separator should be emitted"); + let (before_implicit, _) = deps.split_at(implicit_pos); + let first_input = edge.inputs.first().expect("input should exist"); + + prop_assert!(before_implicit.contains(first_input.as_str())); + prop_assert!(implicit_pos < order_pos); + } + + #[test] + fn implicit_deps_separator_is_absent_when_empty(mut edge in edge_strategy()) { + edge.implicit_deps.clear(); + + let formatted = format_edge(&edge); + let line = build_line(&formatted); + let deps = dependency_side(line); + + prop_assert!(bare_pipe_position(deps).is_none()); + } +} diff --git a/test_support/Cargo.toml b/test_support/Cargo.toml index a058f654..437e3a30 100644 --- a/test_support/Cargo.toml +++ b/test_support/Cargo.toml @@ -10,6 +10,7 @@ tempfile = "3.8.0" mockable = { version = "0.3", features = ["mock"] } ninja_env = { path = "../ninja_env" } camino = "1.2.0" +proptest = "1.11.0" cap-std = { version = "3.4.4", features = ["fs_utf8"] } sha2 = { version = "=0.10.8", default-features = false, features = ["std"] } anyhow = "1" diff --git a/test_support/src/ninja_gen.rs b/test_support/src/ninja_gen.rs index 64382d63..8dfd81d7 100644 --- a/test_support/src/ninja_gen.rs +++ b/test_support/src/ninja_gen.rs @@ -44,6 +44,8 @@ use camino::Utf8PathBuf; use netsuke::ir::{Action, BuildEdge}; +use proptest::prelude::*; +use std::ops::Range; use tempfile::TempDir; use crate::ninja; @@ -77,3 +79,15 @@ pub struct NinjaIntegrationCase { pub fn ninja_integration_setup() -> Option { ninja::ninja_integration_workspace().ok() } + +fn path_strategy(prefix: &'static str) -> impl Strategy { + (0usize..100).prop_map(move |index| Utf8PathBuf::from(format!("{prefix}{index}"))) +} + +/// Generate UTF-8 paths with the supplied prefix and vector size range. +pub fn paths_strategy( + prefix: &'static str, + size_range: Range, +) -> impl Strategy> { + prop::collection::vec(path_strategy(prefix), size_range) +}