From 8ade72ab5b0322df79329a5f54b7c7642d85969b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 11 Aug 2026 13:21:04 +0200 Subject: [PATCH] feat(scenario): let a scenario declare a component once and repeat it over data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the "parameterised templates and data iteration" gap, aimed at the failure mode the original audit named as dominant: a generator duplicating a repeated structure by hand, every copy an opportunity to drift. Ten near-identical cards used to be ten JSON subtrees written out. Now: "components": { "stat_card": { "params": {...}, "template": {...} } }, "children": [{ "for-each": [ {...}, {...}, {...} ], "template": { "use": "stat_card", "props": {...} } }] `params` reuses `config`'s exact shape, so a component parameter is a variable scoped to one instance instead of the file. Omitting `default` makes it required. The overrides key is `props`, not `config`, and that is not an inconsistency: `variables::substitute` deliberately skips any object carrying a literal `config` key, to protect the root declarations block. Reusing the name would have left every `for-each` nested inside a `use` silently unsubstituted — the failure mode this work exists to remove. **`--fix` refuses on these scenarios**, exactly as it already does for `include`. Violation paths carry resolved indices while `--fix` navigates raw JSON; an iteration over ten items shifts everything after it by nine. PR #145 established the refusal for `include` and #160 confirmed doing better is impractical. Patching the wrong node silently is the one unacceptable answer. Every failure gets a named error saying where: a cycle reports the chain rather than overflowing the stack, iterating a non-array reports what it found instead, an unknown component, a missing required parameter and an undeclared prop key all say so. A `for-each` that silently produced nothing because a key was misspelled would be the worst possible outcome here. Pass order is fixed and documented: substitution, then expansion, then `include`, per document. So a `for-each` can iterate an array that came from a `config` variable, and `components` is strictly file-local — reaching into an included file's definitions is a named error in both directions rather than an accident of scope. One correction to the delivered work: the pre-expansion unresolved-reference scan 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 would have cost more than the scan is worth, and would have undermined every real diagnostic this chantier added. That scan now skips the directive bodies, and runs again after expansion where the keys are gone and a leftover `$name` is unambiguous. Both directions are tested: a correct binding warns about nothing, a misspelled one is still caught. --- .../rustmotion-cli/src/commands/validate.rs | 114 +- .../rustmotion-cli/src/commands/validation.rs | 87 ++ crates/rustmotion-core/src/error.rs | 55 + crates/rustmotion-core/src/expand.rs | 1112 +++++++++++++++++ crates/rustmotion-core/src/lib.rs | 1 + crates/rustmotion-core/src/variables.rs | 34 +- crates/rustmotion/src/include.rs | 7 + crates/rustmotion/src/lib.rs | 1 + crates/rustmotion/src/loader.rs | 11 +- .../rustmotion/tests/templates_iteration.rs | 287 +++++ 10 files changed, 1697 insertions(+), 12 deletions(-) create mode 100644 crates/rustmotion-core/src/expand.rs create mode 100644 crates/rustmotion/tests/templates_iteration.rs diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index 62fb753d..62f8dc17 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 85610148..52d8765e 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 f6dc0bde..6f1a6100 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 00000000..497a7d66 --- /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 aa7290df..3afb31ea 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 429b08a0..230a6fcc 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 bbfd7de6..378d3e4d 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 e88ffbdd..395b89dc 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 cfd9fdf8..b7aa5c56 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 00000000..20ba4ff5 --- /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 + ); +}