diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index 62fb753..62f8dc1 100644 --- a/crates/rustmotion-cli/src/commands/validate.rs +++ b/crates/rustmotion-cli/src/commands/validate.rs @@ -30,18 +30,21 @@ fn announced_duration(scenario: &ResolvedScenario) -> f64 { /// Why `--fix` must not write over this input. /// /// `--fix` serialises `LoadedScenario::raw`, which is the document *after* -/// variable substitution and `include` resolution — not the document on disk. For -/// a plain JSON scenario the two coincide and writing back is faithful. For -/// anything templated they do not, and the write silently replaces the source -/// with its own expansion: the `config` block and every `$var` disappear, includes -/// get inlined into the parent, and an HTML input is replaced by JSON outright. +/// variable substitution, `for-each`/`use` expansion, and `include` resolution +/// — not the document on disk. For a plain JSON scenario the two coincide and +/// writing back is faithful. For anything templated they do not, and the write +/// silently replaces the source with its own expansion: the `config` block and +/// every `$var` disappear, includes get inlined into the parent, `for-each`/ +/// `use` get inlined into their repeated/instantiated output, and an HTML +/// input is replaced by JSON outright. /// -/// One rule covers all three: only write back a source `--fix` can reproduce. +/// One rule covers all four: only write back a source `--fix` can reproduce. #[derive(Debug, PartialEq, Eq)] enum FixRefusal { HtmlSource, Templated, UsesInclude, + UsesTemplateDirectives, } impl FixRefusal { @@ -63,6 +66,13 @@ impl FixRefusal { resolved tree — inlining the included files into the parent and patching by a \ path that no longer means the same node. Fix the included file directly." ), + Self::UsesTemplateDirectives => format!( + "--fix cannot rewrite {p}: it uses `for-each`/`use` (or declares `components`), \ + and the fixer would write back the expanded tree — inlining every repeated \ + instance and patching by a path that no longer means the same source node, \ + exactly like `include`. Fix the `components` definition or the `for-each` \ + template directly." + ), } } } @@ -85,6 +95,16 @@ fn refuse_fix(input: &Path, raw_source: &str) -> Option { if raw_source.contains("\"include\"") { return Some(FixRefusal::UsesInclude); } + // Same conservative, raw-substring detection as `UsesInclude` above (not + // a full walk of the tree): `components`/`for-each`/`use` can appear at + // any depth, and `--fix` must refuse before it ever gets far enough to + // find out whether they're actually reachable. + if source.get("components").is_some() + || raw_source.contains("\"for-each\"") + || raw_source.contains("\"use\"") + { + return Some(FixRefusal::UsesTemplateDirectives); + } None } @@ -583,7 +603,7 @@ mod tests { } /// `--fix` writes back the *resolved* tree. Anything the resolution erased is - /// erased on disk too, so these three inputs must be refused rather than + /// erased on disk too, so these inputs must be refused rather than /// silently rewritten. mod fix_refusals { use super::super::{refuse_fix, FixRefusal}; @@ -637,6 +657,32 @@ mod tests { ); } + #[test] + fn a_scenario_using_for_each_is_refused() { + // No `$` anywhere in this fixture on purpose — proves the + // detection is driven by the `for-each` marker itself, not by + // piggybacking on the pre-existing `$`-content check. + let with_for_each = r##"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"duration":1.0,"children":[ + {"for-each":[1,2],"template":{"type":"text","content":"static"}} + ]}]}"##; + assert_eq!( + refuse_fix(Path::new("s.json"), with_for_each), + Some(FixRefusal::UsesTemplateDirectives) + ); + } + + #[test] + fn a_scenario_declaring_components_is_refused_even_with_no_use_site_yet() { + let with_components = r##"{"video":{"width":320,"height":240,"fps":30}, + "components":{"card":{"params":{},"template":{"type":"text","content":"hi"}}}, + "scenes":[{"duration":1.0,"children":[]}]}"##; + assert_eq!( + refuse_fix(Path::new("s.json"), with_components), + Some(FixRefusal::UsesTemplateDirectives) + ); + } + #[test] fn every_refusal_names_the_file_and_says_what_to_do_instead() { let p = Path::new("scenes/hero.json"); @@ -644,6 +690,7 @@ mod tests { FixRefusal::HtmlSource, FixRefusal::Templated, FixRefusal::UsesInclude, + FixRefusal::UsesTemplateDirectives, ] { let msg = r.explain(p); assert!(msg.contains("scenes/hero.json"), "{msg}"); @@ -788,5 +835,58 @@ mod tests { ); assert_eq!(part_after, part, "included file must be untouched too"); } + + /// Same failure mode as `include`, for the sibling mechanism: `for-each` + /// expanding to more than one node shifts every later `children[N]` + /// index, so a path-based `--fix` patch would land on the wrong + /// (or a nonexistent) sibling if it were allowed to write back the + /// expanded tree. It must be refused outright instead. + #[test] + fn cmd_validate_fix_refuses_to_overwrite_a_scenario_using_for_each_and_leaves_the_file_untouched( + ) { + let path = std::env::temp_dir().join(format!( + "rm_validate_fix_for_each_{}.json", + std::process::id() + )); + let original = r##"{ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [ + { "label": "short" }, + { "label": "this string is too long to fit in its card" } + ], + "template": { + "type": "card", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "200px", "background": "#222244" }, + "children": [{ + "type": "text", + "content": "$label", + "style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" } + }] + } + }] + }] + }"##; + std::fs::write(&path, original).expect("write fixture"); + + let result = cmd_validate(&path, None, /*fix=*/ true, false, false, false, None); + + let after = std::fs::read_to_string(&path).expect("read back fixture"); + std::fs::remove_file(&path).ok(); + + assert!( + result.is_err(), + "--fix on a for-each-using scenario with a real violation must be refused" + ); + assert_eq!( + after, original, + "the file must be byte-identical after a refused --fix — the two `for-each` \ + iterations expand into two card siblings, so a path-based patch would not even \ + land on the right one" + ); + } } } diff --git a/crates/rustmotion-cli/src/commands/validation.rs b/crates/rustmotion-cli/src/commands/validation.rs index 8561014..52d8765 100644 --- a/crates/rustmotion-cli/src/commands/validation.rs +++ b/crates/rustmotion-cli/src/commands/validation.rs @@ -12,6 +12,7 @@ use rustmotion::engine; use rustmotion::error::{Result, RustmotionError}; +use rustmotion::expand; use rustmotion::include::{self, IncludeSource}; use rustmotion::schema::{ResolvedScenario, Scenario}; use rustmotion::variables; @@ -170,6 +171,14 @@ pub fn load_with_vars( .map(|p| p.display().to_string()) .unwrap_or_else(|| "".to_string()); variables::apply_variables(&mut json_value, overrides, &label)?; + // Expand `for-each`/`use` (and consume `components`) *before* `raw` is + // captured below, so `LoadedScenario::raw` — what geometry checks walk + // and what `--fix` would serialise — is already the expanded tree. This + // is the same reason `include::resolve_includes` runs before this + // function returns: a validator that reasons about the pre-expansion + // document would be validating something other than what actually + // renders. + expand::expand_directives(&mut json_value, &label)?; let scenario: Scenario = serde_json::from_value(json_value.clone())?; let resolved = include::resolve_includes(scenario, &include_source)?; @@ -512,6 +521,84 @@ pub fn warn_strict_attrs_is_now_default() { ); } +/// Proves `validate` reasons about the *expanded* tree, not the +/// pre-expansion `for-each`/`use` directives — the same requirement the +/// workstream brief states for `include`-produced scenes ("le validateur +/// doit voir l'arbre expansé"). If `load_with_vars` only expanded directives +/// for rendering but validated the raw, unexpanded document, a geometry +/// violation baked into one of several `for-each`-generated items would be +/// invisible: the un-expanded document has no `text`/`card` components at +/// all at that position, only a directive object geometry checks don't know +/// how to measure. +#[cfg(test)] +mod expanded_tree_is_what_gets_validated { + use super::*; + + #[test] + fn a_geometry_violation_inside_a_for_each_generated_item_is_detected() { + // Two iterations: the first is short and fits, the second is a + // narrow-card/nowrap-text combination guaranteed to overflow — the + // same violation shape `NARROW_CARD_JSON` uses elsewhere in this + // crate's tests. + let json = serde_json::json!({ + "video": { "width": 1920, "height": 1080 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [ + { "label": "ok" }, + { "label": "this string is far too long to fit in this narrow card" } + ], + "template": { + "type": "card", + "x": 100, "y": 100, + "style": { "width": "200px", "height": "200px", "background": "#222244" }, + "children": [{ + "type": "text", + "content": "$label", + "style": { "color": "#ffffff", "font-size": "96px", "white-space": "nowrap" } + }] + } + }] + }] + }) + .to_string(); + + let loaded = load(ValidationSource::Inline(&json)).expect("scenario loads"); + + // The raw tree `--fix` would act on must already be expanded: no + // `for-each` directive marker survives, and there are 2 concrete + // children where the source only wrote 1 directive. + let raw_children = loaded.raw["scenes"][0]["children"].as_array().unwrap(); + assert_eq!( + raw_children.len(), + 2, + "loaded.raw must hold the 2 expanded cards, not the 1 for-each directive" + ); + assert!( + raw_children.iter().all(|c| c.get("for-each").is_none()), + "no for-each directive marker must survive into loaded.raw: {raw_children:?}" + ); + + let report = run_checks(&loaded, false); + assert_eq!( + report.geom_violations.len(), + 1, + "exactly one of the two for-each-generated cards overflows; a validator that only \ + saw the pre-expansion directive could not have found this at all: {:?}", + report.geom_violations + ); + // The violation's path must point at the *second* expanded card + // (children[1]), proving the geometry walker is indexing into the + // expanded array, not some placeholder. + assert!( + report.geom_violations[0].path.contains("children[1]"), + "expected the violation to be attributed to the second expanded card: {}", + report.geom_violations[0].path + ); + } +} + #[cfg(test)] mod check_crf_tests { use super::check_crf; diff --git a/crates/rustmotion-core/src/error.rs b/crates/rustmotion-core/src/error.rs index f6dc0bd..6f1a610 100644 --- a/crates/rustmotion-core/src/error.rs +++ b/crates/rustmotion-core/src/error.rs @@ -148,6 +148,61 @@ pub enum RustmotionError { #[error("Cannot interpolate non-string variable '${name}' into string in '{path}'")] VariableInterpolationTypeError { name: String, path: String }, + // --- Templates: `components` / `use` / `for-each` (see rustmotion_core::expand) --- + #[error("'components' at '{path}' must be an object mapping names to definitions")] + ComponentsBlockNotObject { path: String }, + + #[error("Component definition '{name}' at '{path}' is invalid: {reason}")] + ComponentDefinitionInvalid { + name: String, + path: String, + reason: String, + }, + + #[error("'use' directive at '{path}' is invalid: {reason}")] + UseDirectiveInvalid { path: String, reason: String }, + + #[error( + "Unknown component '{name}' referenced via 'use' at '{path}' — no such name in this \ + file's 'components' block" + )] + UnknownComponent { name: String, path: String }, + + #[error( + "Missing required parameter '{param}' for component '{component}' at '{path}' — it has \ + no default and was not supplied via 'props'" + )] + ComponentParamMissing { + component: String, + param: String, + path: String, + }, + + #[error( + "Unknown parameter '{param}' passed to component '{component}' at '{path}' — not \ + declared in its 'params'" + )] + UnknownComponentParam { + component: String, + param: String, + path: String, + }, + + #[error("Component instantiation cycle at '{path}': {chain}")] + ComponentCycle { chain: String, path: String }, + + #[error("'for-each' directive at '{path}' is invalid: {reason}")] + ForEachDirectiveInvalid { path: String, reason: String }, + + #[error("'for-each' at '{path}' must resolve to an array; found {found}")] + ForEachNotArray { path: String, found: String }, + + #[error( + "Template/component expansion depth limit ({limit}) exceeded at '{path}' — likely a \ + runaway nested 'use'/'for-each' template" + )] + ExpansionDepthExceeded { limit: u32, path: String }, + // --- Encoding --- #[error("No frames to render (total duration is 0)")] NoFrames, diff --git a/crates/rustmotion-core/src/expand.rs b/crates/rustmotion-core/src/expand.rs new file mode 100644 index 0000000..497a7d6 --- /dev/null +++ b/crates/rustmotion-core/src/expand.rs @@ -0,0 +1,1112 @@ +//! Data-driven repetition and reusable component templates. +//! +//! This is the answer to the dominant failure mode the original audit named: +//! an LLM asked for "ten identical cards, different data" hand-writes ten +//! JSON subtrees, and every copy is a chance to diverge (a forgotten color, +//! a stray `font-size`, a `position` that doesn't match its siblings). Two +//! directives close that gap, both usable inside any `children` array — +//! exactly where a component would go: +//! +//! - **`for-each`** repeats a `template` subtree once per element of an +//! array, binding the current element's fields (plus `$index`) into it. +//! - **`use`** instantiates a named, reusable subtree declared once in a +//! top-level `components` block, with `props` overrides — a factored-out +//! component definition, the same relationship `include` has to a whole +//! scenario file, but *within* one file and *without* the I/O. +//! +//! ## Why this lives in `rustmotion-core`, not `rustmotion` +//! +//! `include.rs` needs file/network I/O (`std::fs`, `ureq`), so it lives in +//! the `rustmotion` crate. This module is pure `serde_json::Value` rewriting +//! — no I/O, same as `variables.rs` — so it lives next to it here. +//! +//! ## Syntax, and why it looks like `include`/`config` rather than a third +//! dialect +//! +//! ```json +//! { +//! "components": { +//! "stat_card": { +//! "params": { +//! "label": { "type": "string" }, +//! "value": { "type": "number", "default": 0 }, +//! "color": { "type": "string", "default": "#6366F1" } +//! }, +//! "template": { +//! "type": "card", +//! "style": { "width": "300px", "background": "$color" }, +//! "children": [ +//! { "type": "text", "content": "$label" }, +//! { "type": "counter", "value": "$value" } +//! ] +//! } +//! } +//! }, +//! "scenes": [{ +//! "duration": 3.0, +//! "children": [ +//! { +//! "for-each": "$rows", +//! "template": { "use": "stat_card", "props": { "label": "$label", "value": "$value" } } +//! } +//! ] +//! }] +//! } +//! ``` +//! +//! `components[name].params` is deliberately the exact same shape as the +//! scenario-level `config` block (`{"type": ..., "default": ..., "description": ...}`, +//! see [`crate::schema::VariableDefinition`]) — a param is a variable scoped +//! to one component instead of the whole file. `use` + its overrides field +//! mirrors `IncludeDirective { include, config }` (a name plus overrides) — +//! *except* the overrides field is called **`props`**, not `config`. That is +//! a deliberate, load-bearing difference, not inconsistency: [`substitute`] +//! (shared with `variables.rs`) skips recursing into any object key literally +//! named `"config"`, so that the scenario-level `config` *declarations* block +//! (whose `default` values must stay literal, see +//! `variables::test_config_key_not_substituted`) is never accidentally +//! rewritten by whole-document substitution. Reusing that same key name for +//! `use`'s overrides would make a `for-each` binding (`$label`) placed inside +//! a *nested* `use`'s overrides silently never substitute — exactly the kind +//! of silent failure this workstream exists to remove. `props` sidesteps the +//! collision entirely while keeping the rest of the shape familiar. +//! +//! For `for-each`, each array element's own fields are bound directly (flat, +//! not `$item.label`): the codebase's existing `$name` substitution has no +//! dotted-path support (see `variables::parse_single_var_ref`), so an element +//! `{"label": "Revenue", "value": 120}` exposes `$label` and `$value` +//! straight into the template, exactly like a `config` default would. The +//! whole element is *also* bound to `$item` (for forwarding it wholesale, +//! e.g. into a nested `use`'s `props` via `{"$var": "item"}`), and the +//! 0-based position is bound to `$index`. Explicit data always wins: if an +//! element's own field is named `index` or `item`, that value is kept and the +//! built-in is not inserted over it. +//! +//! ## Pass ordering (load-bearing, tested in +//! `rustmotion/tests/templates_iteration.rs`) +//! +//! Every call site runs `expand_directives` immediately *after* +//! `variables::apply_variables` and *before* `Scenario` is deserialized — +//! same document, same pass boundary `include` sits on the other side of. +//! Concretely, per document (root scenario file, and independently for each +//! file pulled in by `include`, since `components` is file-local — see +//! below): +//! +//! 1. Parse JSON. +//! 2. `variables::apply_variables` — resolves the file's own `config`/`$var`. +//! 3. **`expand::expand_directives`** (this module) — resolves `for-each`/ +//! `use` using the now-literal document, then removes `components`. +//! 4. Deserialize into `Scenario`. +//! 5. `include::resolve_includes` — splices in child files (each of which +//! already went through steps 1-4 independently inside +//! `include::fetch_and_resolve`). +//! +//! Two consequences fall out of running expansion strictly after variable +//! substitution and strictly per-document: +//! +//! - **You *can* iterate over an array that came from a variable.** +//! `"for-each": "$rows"` is, by the time this module sees it, no longer a +//! `$`-string — step 2 already replaced it with the literal array (if +//! `rows` is a declared `config` variable of array type). `for-each` itself +//! never has to know variables exist. +//! - **You *cannot* instantiate a component defined in an included file** — +//! not from the *parent's* `use` sites, anyway. `components` is scoped to +//! the document it is declared in, the same way `config` is: each document +//! gets its own `apply_variables` + `expand_directives` pass over its own +//! text before it is ever spliced into anything else. A `use` inside a +//! file that *includes* another file cannot see the includee's +//! `components`, and a `use` inside the includee cannot see the includer's. +//! This is a deliberate simplicity choice (no cross-file component +//! registry, no import syntax to design and version) — see the module test +//! `use_cannot_reach_a_component_defined_in_a_sibling_included_file` in +//! `rustmotion/tests/templates_iteration.rs` for the resulting diagnostic. +//! +//! ## The index-shift trap (already drew blood once — see PR #145 / #160) +//! +//! `include` has the exact same shape of bug this module could reintroduce: +//! a directive that expands to a scene count other than 1 shifts every +//! later `views[V].scenes[S]` index, and `--fix` patches the raw JSON by +//! that same indexed path. `for-each` is strictly worse on this axis — ten +//! elements shift nine siblings, not (at most) a handful. This module does +//! **not** try to solve that by tracking pre/post-expansion index maps: it +//! solves it the way `include` already does, by removing the temptation. +//! `expand_directives` runs *before* `Scenario` is deserialized, so +//! `LoadedScenario::raw` (what `--fix` would serialize) is *already* the +//! expanded tree by the time `commands/validate.rs` sees it — same as +//! `include`'s resolved scenes are already spliced into `raw` by the time +//! `--fix` runs. `commands/validate.rs::refuse_fix` is extended with a +//! `UsesTemplateDirectives` case, detected the same (raw-substring, +//! conservative-by-design) way `UsesInclude` already is, so `--fix` refuses +//! outright rather than writing the expansion back over the author's +//! `for-each`/`use`/`components` source. + +use std::collections::HashMap; + +use serde::Deserialize; +use serde_json::Value; + +use crate::error::{Result, RustmotionError}; +use crate::schema::VariableType; +use crate::variables::substitute; + +/// Defense-in-depth ceiling on nested `use`/`for-each` expansion. True +/// self-reference cycles are caught immediately by the name stack in +/// [`resolve_entry`] and never reach this; this only guards against +/// legitimately deep (non-cyclic) nesting run away, mirroring +/// `include::MAX_INCLUDE_DEPTH`'s role for the sibling mechanism. +const MAX_EXPANSION_DEPTH: u32 = 64; + +/// One entry of the top-level `components` map: a named, parameterised +/// subtree. `params` reuses the exact shape of the scenario-level `config` +/// block, except a param's `default` is optional — omitting it makes the +/// parameter *required*, which `config` variables cannot express (every +/// `config` variable must have a default, since it is meant to render +/// standalone with no overrides at all; a component parameter has no such +/// obligation — an icon component's `icon` name, for instance, has no +/// sensible default). +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +struct ComponentDefinition { + #[serde(default)] + params: HashMap, + /// The subtree to instantiate: a single component object, or an array of + /// sibling component objects (a fragment spliced in place). + template: Value, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +struct ComponentParam { + #[serde(rename = "type")] + #[allow(dead_code)] + // documentation/schema parity with `config`; not cross-checked against `default`'s actual JSON type (same as `VariableDefinition::var_type` today) + param_type: VariableType, + #[serde(default)] + default: Option, + #[serde(default)] + #[allow(dead_code)] + description: Option, +} + +/// `{"use": "name", "props": {...}}` — instantiate a `components` entry. +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct UseDirective { + #[serde(rename = "use")] + use_name: String, + #[serde(default)] + props: HashMap, +} + +/// `{"for-each": [...], "template": {...}}` — repeat `template` once per +/// element of the (already variable-substituted) array. +#[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] +struct ForEachDirective { + #[serde(rename = "for-each")] + for_each: Value, + template: Value, +} + +fn is_for_each(v: &Value) -> bool { + matches!(v, Value::Object(m) if m.contains_key("for-each")) +} + +fn is_use(v: &Value) -> bool { + matches!(v, Value::Object(m) if m.contains_key("use")) +} + +/// Expand every `for-each`/`use` directive found in any `children` array +/// anywhere in `value`, and consume the top-level `components` block (like +/// `variables::apply_variables` consumes `config`, it is removed so it never +/// reaches `Scenario`'s `deny_unknown_fields`). Call this once per document, +/// immediately after `variables::apply_variables` and before deserializing +/// into `Scenario` — see the module doc for why that ordering is load-bearing. +/// +/// `file_label` is the same kind of label `apply_variables` takes (a file +/// path, ``, or ``) — used only for error messages, alongside a +/// structural location built while walking (e.g. `scenes[2].children[1]`), +/// so a diagnostic names *where in the source* the offending directive is, +/// not just which file. +pub fn expand_directives(value: &mut Value, file_label: &str) -> Result<()> { + let defs = extract_component_definitions(value, file_label)?; + + let Value::Object(root) = value else { + return Ok(()); + }; + root.remove("components"); + + if let Some(Value::Array(scenes)) = root.remove("scenes") { + let mut out = Vec::with_capacity(scenes.len()); + for (i, mut scene) in scenes.into_iter().enumerate() { + let scene_path = format!("scenes[{i}]"); + let mut stack = Vec::new(); + walk_children(&mut scene, &defs, file_label, &scene_path, &mut stack, 0)?; + out.push(scene); + } + root.insert("scenes".to_string(), Value::Array(out)); + } + + if let Some(Value::Array(views)) = root.remove("composition") { + let mut out_views = Vec::with_capacity(views.len()); + for (vi, mut view) in views.into_iter().enumerate() { + if let Value::Object(vmap) = &mut view { + if let Some(Value::Array(scenes)) = vmap.remove("scenes") { + let mut out = Vec::with_capacity(scenes.len()); + for (si, mut scene) in scenes.into_iter().enumerate() { + let scene_path = format!("composition[{vi}].scenes[{si}]"); + let mut stack = Vec::new(); + walk_children(&mut scene, &defs, file_label, &scene_path, &mut stack, 0)?; + out.push(scene); + } + vmap.insert("scenes".to_string(), Value::Array(out)); + } + } + out_views.push(view); + } + root.insert("composition".to_string(), Value::Array(out_views)); + } + + warn_unresolved_after_expansion(value, file_label); + Ok(()) +} + +/// Report `$name`s that survived both variable substitution and directive +/// expansion. +/// +/// `variables::apply_variables` runs its own scan, but *before* this pass and +/// skipping `template`/`props`/`components` — every `$name` in there is a +/// binding this function is about to resolve, and reporting them would emit a +/// warning per binding on every correct scenario. Those keys are consumed by +/// the time we get here, so scanning the expanded document sees only genuine +/// leftovers: a `$typo` in a template that matched no data field, or a `$` in +/// ordinary content. +/// +/// A warning rather than an error, matching what `apply_variables` decided for +/// the same diagnostic: a literal `$` in a price or a shell path is legitimate +/// content and must not fail a render. +fn warn_unresolved_after_expansion(value: &Value, file_label: &str) { + for name in crate::variables::find_unresolved(value) { + eprintln!( + "Warning: {}", + crate::error::RustmotionError::UnresolvedVariable { + name, + path: file_label.to_string(), + } + ); + } +} + +fn extract_component_definitions( + value: &Value, + file_label: &str, +) -> Result> { + let Value::Object(root) = value else { + return Ok(HashMap::new()); + }; + match root.get("components") { + None => Ok(HashMap::new()), + Some(Value::Object(defs_map)) => { + let mut out = HashMap::with_capacity(defs_map.len()); + for (name, def_val) in defs_map { + let def: ComponentDefinition = + serde_json::from_value(def_val.clone()).map_err(|e| { + RustmotionError::ComponentDefinitionInvalid { + name: name.clone(), + path: file_label.to_string(), + reason: e.to_string(), + } + })?; + out.insert(name.clone(), def); + } + Ok(out) + } + Some(_) => Err(RustmotionError::ComponentsBlockNotObject { + path: file_label.to_string(), + }), + } +} + +/// Find the `children` array on `value` (if any), expand every entry in it +/// (concrete entries pass through unchanged but are still recursed into, so +/// nested containers get their own `children` expanded too), then recurse +/// into every other field generically — a `for-each`/`use` can appear +/// anywhere a `children` array can, at any nesting depth. +fn walk_children( + value: &mut Value, + defs: &HashMap, + file_label: &str, + location: &str, + stack: &mut Vec, + depth: u32, +) -> Result<()> { + match value { + Value::Object(map) => { + if matches!(map.get("children"), Some(Value::Array(_))) { + if let Some(Value::Array(arr)) = map.remove("children") { + let mut expanded = Vec::with_capacity(arr.len()); + for (i, entry) in arr.into_iter().enumerate() { + let entry_loc = format!("{location}.children[{i}]"); + expanded.extend(resolve_entry( + entry, defs, file_label, &entry_loc, stack, depth, + )?); + } + map.insert("children".to_string(), Value::Array(expanded)); + } + } + for (k, v) in map.iter_mut() { + if k == "children" { + continue; // already fully expanded above + } + walk_children(v, defs, file_label, location, stack, depth)?; + } + } + Value::Array(arr) => { + for v in arr.iter_mut() { + walk_children(v, defs, file_label, location, stack, depth)?; + } + } + _ => {} + } + Ok(()) +} + +/// Resolve one `children` array entry into zero or more concrete entries. +/// A plain component entry resolves to exactly itself (after recursing into +/// its own `children`, if it has one). A `for-each`/`use` directive resolves +/// to the nodes it produces — which are, in turn, run back through this same +/// function, so a `for-each` template that is itself a `use`, or a `use` +/// whose template is itself a `for-each`, composes without special-casing. +/// +/// A bare JSON array (a `for-each`/`use` template written as a *fragment* — +/// several sibling nodes instead of one) is flattened here too, generically, +/// rather than only where `use` happens to produce one: both directives' +/// `template` accept either shape, and this is the single place that +/// splices a fragment's elements into the parent `children` array instead of +/// nesting a raw `[...]` inside it (which downstream `Component` +/// deserialization has no concept of). +fn resolve_entry( + entry: Value, + defs: &HashMap, + file_label: &str, + location: &str, + stack: &mut Vec, + depth: u32, +) -> Result> { + if depth > MAX_EXPANSION_DEPTH { + return Err(RustmotionError::ExpansionDepthExceeded { + limit: MAX_EXPANSION_DEPTH, + path: format!("{file_label}: {location}"), + }); + } + + if let Value::Array(fragment) = entry { + let mut out = Vec::with_capacity(fragment.len()); + for (i, n) in fragment.into_iter().enumerate() { + let frag_loc = format!("{location}[{i}]"); + out.extend(resolve_entry( + n, + defs, + file_label, + &frag_loc, + stack, + depth + 1, + )?); + } + return Ok(out); + } + + if is_for_each(&entry) { + let produced = expand_for_each_directive(entry, file_label, location)?; + let mut out = Vec::with_capacity(produced.len()); + for (i, node) in produced.into_iter().enumerate() { + let iter_loc = format!("{location}[{i}]"); + out.extend(resolve_entry( + node, + defs, + file_label, + &iter_loc, + stack, + depth + 1, + )?); + } + return Ok(out); + } + + if is_use(&entry) { + let (name, node) = expand_use_directive(entry, defs, file_label, location)?; + if stack.contains(&name) { + let mut chain = stack.clone(); + chain.push(name); + return Err(RustmotionError::ComponentCycle { + chain: chain.join(" -> "), + path: format!("{file_label}: {location}"), + }); + } + stack.push(name); + let result = resolve_entry(node, defs, file_label, location, stack, depth + 1); + stack.pop(); + return result; + } + + let mut node = entry; + walk_children(&mut node, defs, file_label, location, stack, depth)?; + Ok(vec![node]) +} + +fn expand_for_each_directive(entry: Value, file_label: &str, location: &str) -> Result> { + let directive: ForEachDirective = + serde_json::from_value(entry).map_err(|e| RustmotionError::ForEachDirectiveInvalid { + path: format!("{file_label}: {location}"), + reason: e.to_string(), + })?; + + let items = match &directive.for_each { + Value::Array(items) => items.clone(), + other => { + return Err(RustmotionError::ForEachNotArray { + path: format!("{file_label}: {location}"), + found: describe_value(other), + }) + } + }; + + let mut out = Vec::with_capacity(items.len()); + for (idx, element) in items.into_iter().enumerate() { + let mut bindings: HashMap = HashMap::new(); + if let Value::Object(obj) = &element { + for (k, v) in obj { + bindings.insert(k.clone(), v.clone()); + } + } + // Explicit data wins: only fill these in if the element didn't + // already define a field with that name. + bindings + .entry("index".to_string()) + .or_insert_with(|| Value::from(idx)); + bindings + .entry("item".to_string()) + .or_insert_with(|| element.clone()); + + let mut node = directive.template.clone(); + substitute(&mut node, &bindings, file_label)?; + out.push(node); + } + Ok(out) +} + +fn expand_use_directive( + entry: Value, + defs: &HashMap, + file_label: &str, + location: &str, +) -> Result<(String, Value)> { + let directive: UseDirective = + serde_json::from_value(entry).map_err(|e| RustmotionError::UseDirectiveInvalid { + path: format!("{file_label}: {location}"), + reason: e.to_string(), + })?; + + let def = defs + .get(&directive.use_name) + .ok_or_else(|| RustmotionError::UnknownComponent { + name: directive.use_name.clone(), + path: format!("{file_label}: {location}"), + })?; + + for key in directive.props.keys() { + if !def.params.contains_key(key) { + return Err(RustmotionError::UnknownComponentParam { + component: directive.use_name.clone(), + param: key.clone(), + path: format!("{file_label}: {location}"), + }); + } + } + + let mut bindings: HashMap = HashMap::with_capacity(def.params.len()); + for (pname, pdef) in &def.params { + match directive.props.get(pname) { + Some(v) => { + bindings.insert(pname.clone(), v.clone()); + } + None => match &pdef.default { + Some(d) => { + bindings.insert(pname.clone(), d.clone()); + } + None => { + return Err(RustmotionError::ComponentParamMissing { + component: directive.use_name.clone(), + param: pname.clone(), + path: format!("{file_label}: {location}"), + }) + } + }, + } + } + + let mut node = def.template.clone(); + substitute(&mut node, &bindings, file_label)?; + Ok((directive.use_name.clone(), node)) +} + +fn describe_value(v: &Value) -> String { + match v { + Value::Null => "null".to_string(), + Value::Bool(b) => format!("boolean ({b})"), + Value::Number(n) => format!("number ({n})"), + Value::String(s) => { + let preview: String = s.chars().take(40).collect(); + let ellipsis = if s.chars().count() > 40 { "…" } else { "" }; + format!( + "string (\"{preview}{ellipsis}\"){}", + if s.starts_with('$') { + " — looks like an unresolved/undeclared variable reference" + } else { + "" + } + ) + } + Value::Object(_) => "object".to_string(), + Value::Array(_) => "array".to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn expand(mut value: Value) -> Result { + expand_directives(&mut value, "test.json")?; + Ok(value) + } + + // ---- unresolved-reference scanning across the two passes ---- + + /// `apply_variables` scans for leftover `$name`s before this pass runs. + /// Left unguarded it reported every template binding as a typo — six + /// warnings on the canonical example, each accusing the author of a + /// mistake they had not made. Warnings that are reliably wrong teach the + /// reader to ignore warnings, which costs more than the scan is worth. + #[test] + fn template_bindings_are_not_reported_as_unresolved_before_expansion() { + let doc = json!({ + "components": { + "card": { + "params": { "label": { "type": "string" } }, + "template": { "type": "text", "content": "$label" } + } + }, + "scenes": [{ "duration": 1.0, "children": [{ + "for-each": [{ "label": "one" }], + "template": { "use": "card", "props": { "label": "$label" } } + }]}] + }); + assert!( + crate::variables::find_unresolved(&doc).is_empty(), + "bindings inside components/template/props belong to expansion, \ + not to the pre-expansion scan: {:?}", + crate::variables::find_unresolved(&doc) + ); + } + + /// The other half: skipping those keys must not turn a false positive + /// into a false negative. Once expansion has consumed them, a `$name` + /// that matched no data field is a genuine leftover and is visible to the + /// very same scan. + #[test] + fn a_typo_inside_a_template_is_still_found_after_expansion() { + let expanded = expand(json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ "duration": 1.0, "children": [{ + "for-each": [{ "label": "one" }], + "template": { "type": "text", "content": "$labl" } + }]}] + })) + .expect("a typo is a warning, not a hard error"); + assert_eq!( + crate::variables::find_unresolved(&expanded), + vec!["labl".to_string()], + "the leftover must be visible once template/props are gone" + ); + } + + /// And the correct spelling leaves nothing behind, so the scan above is + /// discriminating rather than merely quiet. + #[test] + fn a_correct_binding_leaves_nothing_unresolved_after_expansion() { + let expanded = expand(json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ "duration": 1.0, "children": [{ + "for-each": [{ "label": "one" }], + "template": { "type": "text", "content": "$label" } + }]}] + })) + .expect("expands"); + assert!(crate::variables::find_unresolved(&expanded).is_empty()); + } + + // ---- for-each ---- + + #[test] + fn for_each_repeats_template_once_per_element_binding_its_fields() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [ + { "label": "Revenue", "value": 120 }, + { "label": "Users", "value": 340 } + ], + "template": { "type": "text", "content": "$label: $value" } + }] + }] + }); + let out = expand(doc).unwrap(); + let children = out["scenes"][0]["children"].as_array().unwrap(); + assert_eq!(children.len(), 2); + assert_eq!(children[0]["content"], json!("Revenue: 120")); + assert_eq!(children[1]["content"], json!("Users: 340")); + } + + #[test] + fn for_each_binds_index_and_whole_item() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": ["a", "b", "c"], + "template": { "type": "text", "content": "$index:$item" } + }] + }] + }); + let out = expand(doc).unwrap(); + let children = out["scenes"][0]["children"].as_array().unwrap(); + assert_eq!(children.len(), 3); + assert_eq!(children[0]["content"], json!("0:a")); + assert_eq!(children[1]["content"], json!("1:b")); + assert_eq!(children[2]["content"], json!("2:c")); + } + + #[test] + fn for_each_lets_explicit_item_fields_win_over_built_in_index() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [{ "index": "custom", "label": "x" }], + "template": { "type": "text", "content": "$index" } + }] + }] + }); + let out = expand(doc).unwrap(); + assert_eq!(out["scenes"][0]["children"][0]["content"], json!("custom")); + } + + #[test] + fn for_each_over_empty_array_produces_nothing_and_is_not_an_error() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [], + "template": { "type": "text", "content": "unused" } + }] + }] + }); + let out = expand(doc).unwrap(); + assert_eq!(out["scenes"][0]["children"], json!([])); + } + + #[test] + fn for_each_source_that_is_not_an_array_is_a_named_error_not_a_silent_empty_result() { + // The exact silent failure mode the brief calls out: a for-each + // source key typo'd or referencing an undeclared variable leaves a + // literal, non-array `$...` string here — this must be a hard error + // naming where, not a quietly empty `children`. + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": "$itms", + "template": { "type": "text", "content": "$label" } + }] + }] + }); + let err = expand(doc).expect_err("non-array for-each source must fail loudly"); + assert!( + matches!(err, RustmotionError::ForEachNotArray { .. }), + "{err}" + ); + let msg = err.to_string(); + assert!(msg.contains("scenes[0].children[0]"), "{msg}"); + assert!(msg.contains("unresolved"), "{msg}"); + } + + #[test] + fn for_each_missing_template_is_a_named_error() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ "for-each": [1, 2, 3] }] + }] + }); + let err = expand(doc).expect_err("missing template must fail"); + assert!( + matches!(err, RustmotionError::ForEachDirectiveInvalid { .. }), + "{err}" + ); + } + + #[test] + fn for_each_with_a_fragment_template_splices_every_sibling_in_place_not_a_nested_array() { + // Each iteration's `template` is an *array* of two sibling nodes + // (an icon + a label), not a single object — both must end up as + // direct, flat siblings in the surrounding `children` array; a + // nested `[...]` there would not deserialize as a component. + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [ { "label": "A" }, { "label": "B" } ], + "template": [ + { "type": "icon", "icon": "lucide:dot" }, + { "type": "text", "content": "$label" } + ] + }] + }] + }); + let out = expand(doc).unwrap(); + let children = out["scenes"][0]["children"].as_array().unwrap(); + assert_eq!( + children.len(), + 4, + "2 iterations x 2 fragment nodes = 4 flat siblings, got: {children:#?}" + ); + assert!(children.iter().all(|c| c.is_object()), "{children:#?}"); + assert_eq!(children[0]["type"], json!("icon")); + assert_eq!(children[1]["content"], json!("A")); + assert_eq!(children[2]["type"], json!("icon")); + assert_eq!(children[3]["content"], json!("B")); + } + + // ---- use / components ---- + + fn doc_with_stat_card(props: Value) -> Value { + json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "stat_card": { + "params": { + "label": { "type": "string" }, + "value": { "type": "number", "default": 0 }, + "color": { "type": "string", "default": "#6366F1" } + }, + "template": { + "type": "card", + "style": { "background": "$color" }, + "children": [ + { "type": "text", "content": "$label" }, + { "type": "counter", "value": "$value" } + ] + } + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "stat_card", "props": props }] + }] + }) + } + + #[test] + fn use_instantiates_a_component_with_props_overriding_defaults() { + let out = expand(doc_with_stat_card( + json!({ "label": "Revenue", "value": 42 }), + )) + .unwrap(); + let card = &out["scenes"][0]["children"][0]; + assert_eq!(card["type"], json!("card")); + assert_eq!(card["style"]["background"], json!("#6366F1")); + assert_eq!(card["children"][0]["content"], json!("Revenue")); + assert_eq!(card["children"][1]["value"], json!(42)); + } + + #[test] + fn use_falls_back_to_param_default_when_not_overridden() { + let out = expand(doc_with_stat_card(json!({ "label": "Users" }))).unwrap(); + assert_eq!( + out["scenes"][0]["children"][0]["children"][1]["value"], + json!(0) + ); + } + + #[test] + fn components_block_does_not_survive_expansion() { + let out = expand(doc_with_stat_card(json!({ "label": "x" }))).unwrap(); + assert!(out.get("components").is_none()); + } + + #[test] + fn use_of_unknown_component_is_a_named_error() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "does_not_exist", "props": {} }] + }] + }); + let err = expand(doc).expect_err("unknown component must fail"); + match &err { + RustmotionError::UnknownComponent { name, path } => { + assert_eq!(name, "does_not_exist"); + assert!(path.contains("scenes[0].children[0]"), "{path}"); + } + other => panic!("expected UnknownComponent, got {other}"), + } + } + + #[test] + fn use_missing_a_required_parameter_is_a_named_error() { + // `label` has no default in `doc_with_stat_card` — omitting it must + // fail, not silently render an empty/placeholder value. + let out = expand(doc_with_stat_card(json!({}))); + let err = out.expect_err("missing required param must fail"); + match &err { + RustmotionError::ComponentParamMissing { + component, param, .. + } => { + assert_eq!(component, "stat_card"); + assert_eq!(param, "label"); + } + other => panic!("expected ComponentParamMissing, got {other}"), + } + } + + #[test] + fn use_with_an_undeclared_prop_key_is_a_named_error() { + let out = expand(doc_with_stat_card( + json!({ "label": "x", "labell": "typo" }), + )); + let err = out.expect_err("typo'd prop key must fail"); + match &err { + RustmotionError::UnknownComponentParam { param, .. } => assert_eq!(param, "labell"), + other => panic!("expected UnknownComponentParam, got {other}"), + } + } + + #[test] + fn use_of_a_component_that_uses_itself_is_a_named_cycle_not_a_stack_overflow() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "recursive": { + "params": {}, + "template": { "type": "card", "children": [ { "use": "recursive", "props": {} } ] } + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "recursive", "props": {} }] + }] + }); + let err = expand(doc).expect_err("self-referencing component must fail"); + match &err { + RustmotionError::ComponentCycle { chain, .. } => { + assert!(chain.contains("recursive"), "{chain}"); + } + other => panic!("expected ComponentCycle, got {other}"), + } + } + + #[test] + fn indirect_two_hop_cycle_is_also_a_named_cycle() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "a": { "params": {}, "template": { "type": "card", "children": [ { "use": "b", "props": {} } ] } }, + "b": { "params": {}, "template": { "type": "card", "children": [ { "use": "a", "props": {} } ] } } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "a", "props": {} }] + }] + }); + let err = expand(doc).expect_err("indirect cycle must fail"); + match &err { + RustmotionError::ComponentCycle { chain, .. } => { + assert!(chain.contains('a') && chain.contains('b'), "{chain}"); + } + other => panic!("expected ComponentCycle, got {other}"), + } + } + + #[test] + fn use_with_a_fragment_template_splices_every_sibling_in_place() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "icon_label": { + "params": { "label": { "type": "string" } }, + "template": [ + { "type": "icon", "icon": "lucide:dot" }, + { "type": "text", "content": "$label" } + ] + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "icon_label", "props": { "label": "hi" } }] + }] + }); + let out = expand(doc).unwrap(); + let children = out["scenes"][0]["children"].as_array().unwrap(); + assert_eq!(children.len(), 2, "{children:#?}"); + assert_eq!(children[0]["type"], json!("icon")); + assert_eq!(children[1]["content"], json!("hi")); + } + + // ---- composition of the two directives ---- + + #[test] + fn for_each_template_can_be_a_use_directive() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "row": { + "params": { "label": { "type": "string" } }, + "template": { "type": "text", "content": "$label" } + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [ { "label": "A" }, { "label": "B" } ], + "template": { "use": "row", "props": { "label": "$label" } } + }] + }] + }); + let out = expand(doc).unwrap(); + let children = out["scenes"][0]["children"].as_array().unwrap(); + assert_eq!(children.len(), 2); + assert_eq!(children[0]["content"], json!("A")); + assert_eq!(children[1]["content"], json!("B")); + } + + #[test] + fn use_template_can_contain_a_nested_for_each() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "list_card": { + "params": { "items": { "type": "array" } }, + "template": { + "type": "card", + "children": [{ + "for-each": "$items", + "template": { "type": "text", "content": "$item" } + }] + } + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "list_card", "props": { "items": ["x", "y", "z"] } }] + }] + }); + let out = expand(doc).unwrap(); + let inner = out["scenes"][0]["children"][0]["children"] + .as_array() + .unwrap(); + assert_eq!(inner.len(), 3); + assert_eq!(inner[2]["content"], json!("z")); + } + + #[test] + fn nested_children_containers_are_expanded_recursively() { + let doc = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "type": "card", + "children": [{ + "for-each": [{ "v": 1 }, { "v": 2 }], + "template": { "type": "text", "content": "$v" } + }] + }] + }] + }); + let out = expand(doc).unwrap(); + let inner = out["scenes"][0]["children"][0]["children"] + .as_array() + .unwrap(); + assert_eq!(inner.len(), 2); + assert_eq!(inner[0]["content"], json!(1)); + assert_eq!(inner[1]["content"], json!(2)); + } + + // ---- the tree-identity proof, at the JSON-value level ---- + + #[test] + fn for_each_authored_tree_is_identical_to_the_hand_written_equivalent() { + let generated = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": [ + { "label": "Revenue", "value": 120 }, + { "label": "Users", "value": 340 }, + { "label": "Growth", "value": 8 } + ], + "template": { + "type": "card", + "style": { "width": "200px" }, + "children": [ + { "type": "text", "content": "$label" }, + { "type": "counter", "value": "$value" } + ] + } + }] + }] + }); + + let hand_written = json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [ + { "type": "card", "style": { "width": "200px" }, "children": [ + { "type": "text", "content": "Revenue" }, + { "type": "counter", "value": 120 } + ]}, + { "type": "card", "style": { "width": "200px" }, "children": [ + { "type": "text", "content": "Users" }, + { "type": "counter", "value": 340 } + ]}, + { "type": "card", "style": { "width": "200px" }, "children": [ + { "type": "text", "content": "Growth" }, + { "type": "counter", "value": 8 } + ]} + ] + }] + }); + + let expanded = expand(generated).unwrap(); + assert_eq!( + expanded, hand_written, + "the for-each-authored tree must be byte-for-byte identical (as JSON values) to the \ + hand-written equivalent — this is the only proof that factoring changes nothing about \ + what gets rendered" + ); + } +} diff --git a/crates/rustmotion-core/src/lib.rs b/crates/rustmotion-core/src/lib.rs index aa7290d..3afb31e 100644 --- a/crates/rustmotion-core/src/lib.rs +++ b/crates/rustmotion-core/src/lib.rs @@ -3,6 +3,7 @@ pub mod error; pub mod macros; pub mod css; pub mod engine; +pub mod expand; pub mod schema; pub mod traits; pub mod variables; diff --git a/crates/rustmotion-core/src/variables.rs b/crates/rustmotion-core/src/variables.rs index 429b08a..230a6fc 100644 --- a/crates/rustmotion-core/src/variables.rs +++ b/crates/rustmotion-core/src/variables.rs @@ -37,7 +37,20 @@ fn merge_variables( } /// Recursively substitute variable references in a JSON value tree. -fn substitute(value: &mut Value, vars: &HashMap, path: &str) -> Result<()> { +/// +/// `pub(crate)` (not private) so `crate::expand` can reuse the exact same +/// `$name` / `{"$var": "name"}` / interpolation semantics for component-param +/// and `for-each` item/index bindings, rather than re-implementing a second, +/// subtly-different substitution pass. Same reason `"config"` is skipped here +/// (see the loop below): a component-template clone can itself contain a +/// nested `use`'s `props` block — deliberately *not* named `config`, so this +/// skip does not swallow it (see `expand.rs` module doc for why `props` was +/// chosen over `config` for that field). +pub(crate) fn substitute( + value: &mut Value, + vars: &HashMap, + path: &str, +) -> Result<()> { match value { Value::String(s) => { // Check for exact match "$name" (whole-string replacement, preserves type) @@ -206,7 +219,24 @@ fn find_unresolved_recursive(value: &Value, out: &mut Vec) { } } for (key, v) in map { - if key != "config" { + // `config` holds the declarations themselves, never references. + // + // `template` / `props` / `components` hold the bodies of the + // template directives, whose `$name`s are bound by + // `expand::expand_directives` — which runs *after* this pass. + // Scanning them here reports every correct binding as an + // unresolved typo: the canonical `for-each` example emits six + // such warnings, each accusing the author of a mistake they + // did not make. Warnings that are reliably wrong teach the + // reader to ignore warnings, which would cost more than the + // scan is worth. + // + // Nothing is lost: `expand_directives` re-runs this same scan + // once expansion is done and these keys no longer exist, so a + // genuine typo inside a template is still reported — with the + // benefit of naming it after substitution, where the leftover + // is unambiguous. + if !matches!(key.as_str(), "config" | "template" | "props" | "components") { find_unresolved_recursive(v, out); } } diff --git a/crates/rustmotion/src/include.rs b/crates/rustmotion/src/include.rs index bbfd7de..378d3e4 100644 --- a/crates/rustmotion/src/include.rs +++ b/crates/rustmotion/src/include.rs @@ -148,6 +148,13 @@ fn fetch_and_resolve( directive.config.as_ref(), &directive.include, )?; + // `components` (and any `for-each`/`use` inside this file's own scenes) + // is scoped to this document: expanded here, per included file, using + // ONLY this file's own `components` block — never the parent's, and + // never visible to the parent's own `use` sites. See + // `rustmotion_core::expand`'s module doc for why that scoping was + // chosen over a cross-file component registry. + crate::expand::expand_directives(&mut json_value, &directive.include)?; let child_scenario: Scenario = serde_json::from_value(json_value).map_err(RustmotionError::from)?; diff --git a/crates/rustmotion/src/lib.rs b/crates/rustmotion/src/lib.rs index e88ffbd..395b89d 100644 --- a/crates/rustmotion/src/lib.rs +++ b/crates/rustmotion/src/lib.rs @@ -1,6 +1,7 @@ // Re-export core and components for downstream users pub use rustmotion_core as core; pub use rustmotion_core::error; +pub use rustmotion_core::expand; pub use rustmotion_core::schema; pub use rustmotion_core::traits; pub use rustmotion_core::variables; diff --git a/crates/rustmotion/src/loader.rs b/crates/rustmotion/src/loader.rs index cfd9fdf..b7aa5c5 100644 --- a/crates/rustmotion/src/loader.rs +++ b/crates/rustmotion/src/loader.rs @@ -1,6 +1,6 @@ use crate::error::{Result, RustmotionError}; use crate::schema::{ResolvedScenario, Scenario}; -use crate::{include, variables}; +use crate::{expand, include, variables}; use std::path::PathBuf; pub fn load_scenario(input: &PathBuf) -> Result { @@ -21,7 +21,9 @@ pub fn load_scenario_with_vars( let mut json_value: serde_json::Value = serde_json::from_str(&json_str).map_err(RustmotionError::from)?; - variables::apply_variables(&mut json_value, overrides, &input.display().to_string())?; + let label = input.display().to_string(); + variables::apply_variables(&mut json_value, overrides, &label)?; + expand::expand_directives(&mut json_value, &label)?; let scenario: Scenario = serde_json::from_value(json_value).map_err(RustmotionError::from)?; include::resolve_includes(scenario, &include::IncludeSource::File(input.clone())) @@ -47,6 +49,7 @@ pub fn load_scenario_from_source_with_vars( let mut json_value: serde_json::Value = serde_json::from_str(json_str).map_err(RustmotionError::from)?; variables::apply_variables(&mut json_value, overrides, "")?; + expand::expand_directives(&mut json_value, "")?; let scenario: Scenario = serde_json::from_value(json_value).map_err(RustmotionError::from)?; include::resolve_includes(scenario, &include::IncludeSource::Inline) @@ -94,7 +97,9 @@ pub fn load_scenario_from_html_with_vars( // Variable substitution happens post-transpilation so $name in HTML text // content is resolved. HTML has no config block, so undeclared overrides // are applied as raw value substitutions (no-config path in apply_variables). - variables::apply_variables(&mut value, overrides, &input.display().to_string())?; + let label = input.display().to_string(); + variables::apply_variables(&mut value, overrides, &label)?; + expand::expand_directives(&mut value, &label)?; let scenario: Scenario = serde_json::from_value(value).map_err(RustmotionError::from)?; include::resolve_includes(scenario, &include::IncludeSource::File(input.clone())) } diff --git a/crates/rustmotion/tests/templates_iteration.rs b/crates/rustmotion/tests/templates_iteration.rs new file mode 100644 index 0000000..20ba4ff --- /dev/null +++ b/crates/rustmotion/tests/templates_iteration.rs @@ -0,0 +1,287 @@ +//! End-to-end tests for `for-each`/`use`/`components` through the *real* +//! load pipeline (`rustmotion::loader`), not just `rustmotion_core::expand` +//! in isolation. These pin down the two ordering questions the workstream +//! brief calls out explicitly, and the one proof that matters most: a +//! `for-each`-authored scenario must resolve to *exactly* the same tree as +//! the hand-written equivalent. + +use rustmotion::loader::load_scenario_from_source; + +fn load(json: &serde_json::Value) -> rustmotion::schema::ResolvedScenario { + load_scenario_from_source(None, Some(&json.to_string())).expect("scenario loads") +} + +/// Pass ordering, part 1: `variables::apply_variables` runs before +/// `expand::expand_directives`, so a `for-each` source that is a bare `$var` +/// reference to a `config`-declared array variable is already the literal +/// array by the time `for-each` looks at it. +#[test] +fn for_each_can_iterate_over_an_array_that_came_from_a_config_variable() { + let json = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "config": { + "rows": { + "type": "array", + "default": [ + { "label": "Revenue", "value": 120 }, + { "label": "Users", "value": 340 } + ] + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": "$rows", + "template": { "type": "text", "content": "$label: $value" } + }] + }] + }); + let resolved = load(&json); + let children = &resolved.views[0].scenes[0].children; + assert_eq!( + children.len(), + 2, + "the $rows variable must resolve to its 2-element default before for-each consumes it" + ); + assert_eq!(children[0]["content"], serde_json::json!("Revenue: 120")); + assert_eq!(children[1]["content"], serde_json::json!("Users: 340")); +} + +/// Pass ordering, part 1 also covers an explicit `--var` override: the +/// override replaces the default *before* substitution, so `for-each` still +/// only ever sees a literal array. +#[test] +fn for_each_source_variable_can_be_overridden_at_load_time() { + let json = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "config": { + "rows": { "type": "array", "default": [ { "label": "placeholder" } ] } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ + "for-each": "$rows", + "template": { "type": "text", "content": "$label" } + }] + }] + }) + .to_string(); + + let mut overrides = std::collections::HashMap::new(); + overrides.insert( + "rows".to_string(), + serde_json::json!([{ "label": "A" }, { "label": "B" }, { "label": "C" }]), + ); + let resolved = rustmotion::loader::load_scenario_from_source_with_vars( + None, + Some(&json), + Some(&overrides), + ) + .expect("scenario loads with override"); + assert_eq!(resolved.views[0].scenes[0].children.len(), 3); +} + +/// Pass ordering, part 2: `components` is scoped to the document it is +/// declared in. A `use` site in the *parent* scenario cannot reach a +/// component defined only inside a file it `include`s — parent-level +/// expansion runs before the child file is even fetched, so there is no +/// document in which both are visible at once. +#[test] +fn use_cannot_reach_a_component_defined_only_in_an_included_file() { + let dir = std::env::temp_dir().join(format!( + "rm_templates_iteration_cross_file_{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let child_path = dir.join("child.json"); + let parent_path = dir.join("parent.json"); + + let child = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "widget": { + "params": {}, + "template": { "type": "text", "content": "from child" } + } + }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "widget", "props": {} }] + }] + }); + let parent = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [ + { "include": "child.json" }, + { + "duration": 1.0, + "children": [{ "use": "widget", "props": {} }] + } + ] + }); + std::fs::write(&child_path, child.to_string()).unwrap(); + std::fs::write(&parent_path, parent.to_string()).unwrap(); + + // The child, loaded on its own, resolves fine — its `use` site sees its + // own `components`. + let child_resolved = rustmotion::loader::load_scenario_with_vars(&child_path, None) + .expect("child resolves its own component"); + assert_eq!( + child_resolved.views[0].scenes[0].children[0]["content"], + serde_json::json!("from child") + ); + + // The parent does not: its own `use` site (in its second, non-included + // scene) cannot see the child's `components` — named error, not a silent + // no-op or a wrong-scope success. + let err = rustmotion::loader::load_scenario_with_vars(&parent_path, None) + .expect_err("parent's use site must not resolve a component defined only in the child"); + let msg = err.to_string(); + assert!( + msg.contains("widget"), + "error must name the component: {msg}" + ); + assert!( + matches!( + err, + rustmotion::error::RustmotionError::UnknownComponent { .. } + ), + "expected UnknownComponent, got: {err}" + ); + + std::fs::remove_dir_all(&dir).ok(); +} + +/// The reverse direction of the same scoping rule: a component defined in +/// the *parent* is likewise invisible to a `use` site inside an included +/// file — each document's `components` block only ever sees that document's +/// own `use` sites. +#[test] +fn use_inside_an_included_file_cannot_reach_a_component_defined_only_in_the_parent() { + let dir = std::env::temp_dir().join(format!( + "rm_templates_iteration_cross_file_reverse_{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let child_path = dir.join("child.json"); + let parent_path = dir.join("parent.json"); + + let child = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ + "duration": 1.0, + "children": [{ "use": "widget", "props": {} }] + }] + }); + let parent = serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "components": { + "widget": { + "params": {}, + "template": { "type": "text", "content": "from parent" } + } + }, + "scenes": [ { "include": "child.json" } ] + }); + std::fs::write(&child_path, child.to_string()).unwrap(); + std::fs::write(&parent_path, parent.to_string()).unwrap(); + + let err = rustmotion::loader::load_scenario_with_vars(&parent_path, None) + .expect_err("child's use site must not resolve a component defined only in the parent"); + assert!( + matches!( + err, + rustmotion::error::RustmotionError::UnknownComponent { .. } + ), + "expected UnknownComponent, got: {err}" + ); + + std::fs::remove_dir_all(&dir).ok(); +} + +/// The test that matters most: a `for-each`-authored scenario and the +/// equivalent hand-written scenario must resolve to *identical* trees. This +/// is the only proof that factoring a repeated structure into `for-each` +/// changes nothing about what actually renders. +#[test] +fn for_each_authored_scenario_resolves_identically_to_the_hand_written_equivalent() { + let generated = serde_json::json!({ + "video": { "width": 1080, "height": 1920, "fps": 30 }, + "components": { + "stat_card": { + "params": { + "label": { "type": "string" }, + "value": { "type": "number", "default": 0 }, + "color": { "type": "string", "default": "#6366F1" } + }, + "template": { + "type": "card", + "style": { "width": "300px", "height": "160px", "background": "$color" }, + "children": [ + { "type": "text", "content": "$label", "style": { "color": "#ffffff" } }, + { "type": "counter", "value": "$value" } + ] + } + } + }, + "scenes": [{ + "duration": 3.0, + "children": [{ + "for-each": [ + { "label": "Revenue", "value": 1250, "color": "#22C55E" }, + { "label": "Users", "value": 340, "color": "#3B82F6" }, + { "label": "Growth", "value": 8, "color": "#F59E0B" } + ], + "template": { "use": "stat_card", "props": { "label": "$label", "value": "$value", "color": "$color" } } + }] + }] + }); + + let hand_written = serde_json::json!({ + "video": { "width": 1080, "height": 1920, "fps": 30 }, + "scenes": [{ + "duration": 3.0, + "children": [ + { + "type": "card", + "style": { "width": "300px", "height": "160px", "background": "#22C55E" }, + "children": [ + { "type": "text", "content": "Revenue", "style": { "color": "#ffffff" } }, + { "type": "counter", "value": 1250 } + ] + }, + { + "type": "card", + "style": { "width": "300px", "height": "160px", "background": "#3B82F6" }, + "children": [ + { "type": "text", "content": "Users", "style": { "color": "#ffffff" } }, + { "type": "counter", "value": 340 } + ] + }, + { + "type": "card", + "style": { "width": "300px", "height": "160px", "background": "#F59E0B" }, + "children": [ + { "type": "text", "content": "Growth", "style": { "color": "#ffffff" } }, + { "type": "counter", "value": 8 } + ] + } + ] + }] + }); + + let resolved_generated = load(&generated); + let resolved_hand_written = load(&hand_written); + + assert_eq!( + resolved_generated.views[0].scenes[0].children, + resolved_hand_written.views[0].scenes[0].children, + "for-each + use must resolve to exactly the same children tree as the hand-written scenario" + ); + // Sanity: also compare video/duration so the whole ResolvedScenario, not + // just the children array, lines up. + assert_eq!( + resolved_generated.views[0].scenes[0].duration, + resolved_hand_written.views[0].scenes[0].duration + ); +}