From 004b7b8d1dee01de1f981d9ef91051031d441a82 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 13 Oct 2025 22:59:42 +0100 Subject: [PATCH 01/18] Support manifest macros Register user-defined Jinja macros from the manifest before rendering so they are callable during template evaluation. Add schema support for the macros list, cover parsing with rstest cases, and extend cucumber scenarios for happy and error paths. Document the loader behaviour and mark the roadmap item complete. --- docs/netsuke-design.md | 8 +++ docs/roadmap.md | 2 +- src/ast.rs | 13 +++++ src/manifest.rs | 81 +++++++++++++++++++++++++++++- src/manifest/render.rs | 1 + tests/data/jinja_macro_invalid.yml | 7 +++ tests/data/jinja_macros.yml | 11 ++++ tests/features/manifest.feature | 11 ++++ tests/manifest_jinja_tests.rs | 39 ++++++++++++++ 9 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 tests/data/jinja_macro_invalid.yml create mode 100644 tests/data/jinja_macros.yml diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 96b657f2..9acad8a4 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -539,6 +539,7 @@ use netsuke::ast::*; let ast = NetsukeManifest { netsuke_version: Version::parse("1.0.0").unwrap(), vars: HashMap::new(), + macros: vec![], rules: vec![], actions: vec![], targets: vec![Target { @@ -720,6 +721,13 @@ If a macro name matches a built-in function or filter, the macro overrides the built-in definition. This mirrors Jinja's behaviour and follows `minijinja` semantics where later definitions shadow earlier ones. +The manifest loader compiles each macro definition into an internal template +and registers a wrapper function that evaluates the macro on demand. The +wrapper constructs a fresh MiniJinja state for every invocation so macro calls +do not depend on the lifetime of the manifest parsing state. This preserves +MiniJinja's argument handling, including keyword parameters and `caller` +support, while allowing later macros to override earlier ones. + ### 4.4 Essential Custom Functions To transform `minijinja` from a general-purpose templating engine into a diff --git a/docs/roadmap.md b/docs/roadmap.md index ba6fb067..d419511d 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -107,7 +107,7 @@ configurations with variables, control flow, and custom functions. - [x] Implement the critical glob(pattern) custom function to perform file path globbing, with results sorted lexicographically. - - [ ] Support user-defined Jinja macros declared in a top-level macros list, + - [x] Support user-defined Jinja macros declared in a top-level macros list, registering them with the environment before rendering. - **Success Criterion:** diff --git a/src/ast.rs b/src/ast.rs index 816dd5d9..431fda2d 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -56,6 +56,15 @@ where /// assert_eq!(manifest.targets.len(), 1); /// # Ok(()) } /// ``` +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub struct MacroDefinition { + /// Full macro signature as accepted by `MiniJinja`. + pub signature: String, + /// Body of the macro written using YAML block style. + pub body: String, +} + #[derive(Debug, Deserialize, Serialize)] #[serde(deny_unknown_fields)] pub struct NetsukeManifest { @@ -66,6 +75,10 @@ pub struct NetsukeManifest { #[serde(default)] pub vars: Vars, + /// Optional list of user-defined Jinja macros registered before rendering. + #[serde(default)] + pub macros: Vec, + /// Named rule templates that can be referenced by targets. #[serde(default)] pub rules: Vec, diff --git a/src/manifest.rs b/src/manifest.rs index cc39b7fe..49007d1c 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -9,7 +9,10 @@ use crate::ast::NetsukeManifest; use anyhow::{Context, Result}; -use minijinja::{Environment, Error, ErrorKind, UndefinedBehavior, value::Value}; +use minijinja::{ + Environment, Error, ErrorKind, State, UndefinedBehavior, + value::{Rest, Value}, +}; use serde_yml::Value as YamlValue; use std::{fs, path::Path}; @@ -57,6 +60,80 @@ fn env_var(name: &str) -> std::result::Result { } } +fn parse_macro_name(signature: &str) -> Result { + let trimmed = signature.trim(); + let Some((name, _rest)) = trimmed.split_once('(') else { + return Err(anyhow::anyhow!( + "macro signature '{signature}' must include parameter list" + )); + }; + let name = name.trim(); + if name.is_empty() { + return Err(anyhow::anyhow!( + "macro signature '{signature}' is missing an identifier" + )); + } + Ok(name.to_string()) +} + +fn register_macro(env: &mut Environment, signature: &str, body: &str, index: usize) -> Result<()> { + let name = parse_macro_name(signature)?; + let template_name = format!("__manifest_macro_{index}_{name}"); + let template_source = format!("{{% macro {signature} %}}{body}{{% endmacro %}}",); + + env.add_template_owned(template_name.clone(), template_source) + .with_context(|| format!("compile macro '{name}'"))?; + + let macro_template = template_name.clone(); + let macro_name = name.clone(); + env.add_function( + name, + move |state: &State, args: Rest| -> Result { + let Rest(args) = args; + let template = state.env().get_template(¯o_template)?; + let macro_state = template.eval_to_state(())?; + let value = macro_state.lookup(¯o_name).ok_or_else(|| { + Error::new( + ErrorKind::InvalidOperation, + format!("macro '{macro_name}' is not defined in template '{macro_template}'"), + ) + })?; + value.call(¯o_state, &args) + }, + ); + + Ok(()) +} + +fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> { + let Some(items) = doc.get("macros") else { + return Ok(()); + }; + + let seq = items + .as_sequence() + .ok_or_else(|| anyhow::anyhow!("macros must be a sequence"))?; + for (idx, item) in seq.iter().enumerate() { + let mapping = item + .as_mapping() + .ok_or_else(|| anyhow::anyhow!("macros[{idx}] must be a mapping"))?; + let signature_key = YamlValue::String("signature".into()); + let body_key = YamlValue::String("body".into()); + let signature = mapping + .get(&signature_key) + .and_then(YamlValue::as_str) + .ok_or_else(|| anyhow::anyhow!("macros[{idx}] signature must be a string"))?; + let body = mapping + .get(&body_key) + .and_then(YamlValue::as_str) + .ok_or_else(|| anyhow::anyhow!("macros[{idx}] body must be a string"))?; + + register_macro(env, signature, body, idx) + .with_context(|| format!("register macro '{signature}'"))?; + } + Ok(()) +} + /// Parse a manifest string using Jinja for value templating. /// /// The input YAML must be valid on its own. Jinja expressions are evaluated @@ -87,6 +164,8 @@ fn from_str_named(yaml: &str, name: &str) -> Result { } } + register_manifest_macros(&doc, &mut jinja)?; + expand_foreach(&mut doc, &jinja)?; let manifest: NetsukeManifest = diff --git a/src/manifest/render.rs b/src/manifest/render.rs index a2dc76bd..616c4eb8 100644 --- a/src/manifest/render.rs +++ b/src/manifest/render.rs @@ -142,6 +142,7 @@ mod tests { Ok(NetsukeManifest { netsuke_version: Version::parse("1.0.0")?, vars: manifest_vars, + macros: Vec::new(), rules: vec![rule], actions: Vec::new(), targets: vec![target], diff --git a/tests/data/jinja_macro_invalid.yml b/tests/data/jinja_macro_invalid.yml new file mode 100644 index 00000000..09135eba --- /dev/null +++ b/tests/data/jinja_macro_invalid.yml @@ -0,0 +1,7 @@ +netsuke_version: "1.0.0" +macros: + - body: | + Missing signature +targets: + - name: oops + command: "{{ greet('world') }}" diff --git a/tests/data/jinja_macros.yml b/tests/data/jinja_macros.yml new file mode 100644 index 00000000..ff4489b2 --- /dev/null +++ b/tests/data/jinja_macros.yml @@ -0,0 +1,11 @@ +netsuke_version: "1.0.0" +macros: + - signature: "greet(name)" + body: |- + Hello {{ name }}! + - signature: "shout(text)" + body: |- + {{ text | upper }} +targets: + - name: greeting + command: "{{ shout(greet('netsuke')) }}" diff --git a/tests/features/manifest.feature b/tests/features/manifest.feature index f666440a..38172bae 100644 --- a/tests/features/manifest.feature +++ b/tests/features/manifest.feature @@ -62,12 +62,23 @@ Feature: Manifest Parsing When the manifest is checked Then the first target command is "echo world" + Scenario: Rendering manifest macros + Given the manifest file "tests/data/jinja_macros.yml" is parsed + When the manifest is checked + Then the first target command is "HELLO NETSUKE!" + Scenario: Parsing fails when an environment variable is undefined Given the environment variable "NETSUKE_UNDEFINED_ENV" is unset And the manifest file "tests/data/jinja_env_missing.yml" is parsed When the parsing result is checked Then parsing the manifest fails + Scenario: Parsing fails when a macro is missing its signature + Given the manifest file "tests/data/jinja_macro_invalid.yml" is parsed + When the parsing result is checked + Then parsing the manifest fails + And the error message contains "signature" + Scenario: Rendering Jinja conditionals in a manifest Given the manifest file "tests/data/jinja_if.yml" is parsed When the manifest is checked diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs index 58bcf784..4dfd0139 100644 --- a/tests/manifest_jinja_tests.rs +++ b/tests/manifest_jinja_tests.rs @@ -162,6 +162,45 @@ fn undefined_variable_errors() { assert!(manifest::from_str(&yaml).is_err()); } +#[rstest] +fn registers_manifest_macros() { + let yaml = manifest_yaml(concat!( + "macros:\n", + " - signature: \"greet(name)\"\n", + " body: |-\n", + " Hello {{ name }}!\n", + " - signature: \"shout(text)\"\n", + " body: |-\n", + " {{ text | upper }}\n", + "targets:\n", + " - name: greet\n", + " command: \"{{ shout(greet('world')) }}\"\n", + )); + + let manifest = manifest::from_str(&yaml).expect("parse macros"); + let target = manifest.targets.first().expect("target"); + match &target.recipe { + Recipe::Command { command } => assert_eq!(command, "HELLO WORLD!"), + other => panic!("expected command recipe, got {other:?}"), + } +} + +#[rstest] +fn manifest_macro_with_missing_signature_errors() { + let yaml = manifest_yaml(concat!( + "macros:\n", + " - body: |\n", + " hi\n", + "targets:\n", + " - name: noop\n", + " command: noop\n", + )); + + let err = manifest::from_str(&yaml).expect_err("macro signature required"); + let msg = format!("{err:?}"); + assert!(msg.contains("signature"), "error message: {msg}"); +} + #[rstest] fn syntax_error_errors() { let yaml = manifest_yaml("targets:\n - name: hello\n command: echo {{ who\n"); From 0047e580c6e054ec0da5f33cf20e468dd282ad53 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 14 Oct 2025 23:39:45 +0100 Subject: [PATCH 02/18] Refine manifest macro registration Refactor macro loading to use serde, cover edge cases with unit and behavioural tests, and extend fixtures for argument handling. --- src/manifest.rs | 164 ++++++++++++++++------ tests/ast_tests.rs | 39 +++++ tests/data/jinja_macro_arguments.yml | 23 +++ tests/data/jinja_macro_missing_parens.yml | 8 ++ tests/features/manifest.feature | 16 +++ tests/manifest_jinja_tests.rs | 59 ++++++++ tests/steps/manifest_steps.rs | 21 +++ 7 files changed, 289 insertions(+), 41 deletions(-) create mode 100644 tests/data/jinja_macro_arguments.yml create mode 100644 tests/data/jinja_macro_missing_parens.yml diff --git a/src/manifest.rs b/src/manifest.rs index 49007d1c..5a174d86 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -7,7 +7,7 @@ //! filesystem patterns during template evaluation. Both helpers fail fast when //! inputs are missing or patterns are invalid. -use crate::ast::NetsukeManifest; +use crate::ast::{MacroDefinition, NetsukeManifest}; use anyhow::{Context, Result}; use minijinja::{ Environment, Error, ErrorKind, State, UndefinedBehavior, @@ -62,6 +62,11 @@ fn env_var(name: &str) -> std::result::Result { fn parse_macro_name(signature: &str) -> Result { let trimmed = signature.trim(); + if trimmed.is_empty() { + return Err(anyhow::anyhow!( + "macro signature '{signature}' is missing an identifier" + )); + } let Some((name, _rest)) = trimmed.split_once('(') else { return Err(anyhow::anyhow!( "macro signature '{signature}' must include parameter list" @@ -84,56 +89,42 @@ fn register_macro(env: &mut Environment, signature: &str, body: &str, index: usi env.add_template_owned(template_name.clone(), template_source) .with_context(|| format!("compile macro '{name}'"))?; - let macro_template = template_name.clone(); - let macro_name = name.clone(); - env.add_function( - name, - move |state: &State, args: Rest| -> Result { - let Rest(args) = args; - let template = state.env().get_template(¯o_template)?; - let macro_state = template.eval_to_state(())?; - let value = macro_state.lookup(¯o_name).ok_or_else(|| { - Error::new( - ErrorKind::InvalidOperation, - format!("macro '{macro_name}' is not defined in template '{macro_template}'"), - ) - })?; - value.call(¯o_state, &args) - }, - ); - + env.add_function(name.clone(), make_macro_fn(template_name, name)); Ok(()) } fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> { - let Some(items) = doc.get("macros") else { + let Some(macros) = doc.get("macros").cloned() else { return Ok(()); }; - let seq = items - .as_sequence() - .ok_or_else(|| anyhow::anyhow!("macros must be a sequence"))?; - for (idx, item) in seq.iter().enumerate() { - let mapping = item - .as_mapping() - .ok_or_else(|| anyhow::anyhow!("macros[{idx}] must be a mapping"))?; - let signature_key = YamlValue::String("signature".into()); - let body_key = YamlValue::String("body".into()); - let signature = mapping - .get(&signature_key) - .and_then(YamlValue::as_str) - .ok_or_else(|| anyhow::anyhow!("macros[{idx}] signature must be a string"))?; - let body = mapping - .get(&body_key) - .and_then(YamlValue::as_str) - .ok_or_else(|| anyhow::anyhow!("macros[{idx}] body must be a string"))?; - - register_macro(env, signature, body, idx) - .with_context(|| format!("register macro '{signature}'"))?; + let defs: Vec = serde_yml::from_value(macros) + .context("macros must be a sequence of mappings with string signature/body")?; + + for (idx, def) in defs.into_iter().enumerate() { + register_macro(env, &def.signature, &def.body, idx) + .with_context(|| format!("register macro '{}'", def.signature))?; } Ok(()) } +fn make_macro_fn( + template_name: String, + macro_name: String, +) -> impl Fn(&State, Rest) -> Result { + move |state, Rest(args)| { + let template = state.env().get_template(&template_name)?; + let macro_state = template.eval_to_state(())?; + let value = macro_state.lookup(¯o_name).ok_or_else(|| { + Error::new( + ErrorKind::InvalidOperation, + format!("macro '{macro_name}' not defined in template '{template_name}'"), + ) + })?; + value.call(¯o_state, &args) + } +} + /// Parse a manifest string using Jinja for value templating. /// /// The input YAML must be valid on its own. Jinja expressions are evaluated @@ -201,4 +192,95 @@ pub fn from_path(path: impl AsRef) -> Result { } #[cfg(test)] -mod tests; +mod tests { + use super::*; + use anyhow::Result as AnyResult; + use minijinja::Environment; + use rstest::{fixture, rstest}; + use serde_yml::value::Mapping; + + fn render_with(env: &Environment, template: &str) -> AnyResult { + Ok(env.render_str(template, ())?) + } + + #[fixture] + fn strict_env() -> Environment<'static> { + let mut env = Environment::new(); + env.set_undefined_behavior(UndefinedBehavior::Strict); + env + } + + #[rstest] + #[case("greet(name)", "greet")] + #[case("shout(text='hi')", "shout")] + #[case("joiner(*items)", "joiner")] + #[case("format(name, caller=None)", "format")] + #[case("complex(value, /, *, flag=false, **kw)", "complex")] + fn parse_macro_name_extracts_identifier(#[case] signature: &str, #[case] expected: &str) { + let name = parse_macro_name(signature).expect("parse name"); + assert_eq!(name, expected); + } + + #[rstest] + #[case("greet", "include parameter list")] + #[case("(name)", "missing an identifier")] + #[case(" ", "missing an identifier")] + fn parse_macro_name_errors(#[case] signature: &str, #[case] message: &str) { + let err = parse_macro_name(signature).expect_err("should fail"); + assert!(err.to_string().contains(message), "{err}"); + } + + #[rstest] + #[case("greet()", "Hello", "{{ greet() }}", "Hello")] + #[case("echo(text='hi')", "{{ text }}", "{{ echo() }}", "hi")] + #[case( + "joiner(items)", + "{{ items | join(',') }}", + "{{ joiner(['a', 'b', 'c']) }}", + "a,b,c" + )] + #[case( + "show(name, excited=false)", + "{{ name ~ ('!' if excited else '') }}", + "{{ show('Netsuke', excited=true) }}", + "Netsuke!" + )] + fn register_macro_handles_arguments( + #[case] signature: &str, + #[case] body: &str, + #[case] template: &str, + #[case] expected: &str, + mut strict_env: Environment, + ) { + register_macro(&mut strict_env, signature, body, 0).expect("register"); + let rendered = render_with(&strict_env, template).expect("render"); + assert_eq!(rendered, expected); + } + + #[rstest] + fn register_manifest_macros_validates_shape(mut strict_env: Environment) { + let mut mapping = Mapping::new(); + mapping.insert( + YamlValue::from("macros"), + YamlValue::from(vec![YamlValue::from(42)]), + ); + let doc = YamlValue::Mapping(mapping); + let err = register_manifest_macros(&doc, &mut strict_env).expect_err("shape error"); + assert!( + err.to_string() + .contains("macros must be a sequence of mappings"), + "{err}" + ); + } + + #[rstest] + fn register_manifest_macros_supports_multiple(mut strict_env: Environment) { + let yaml = serde_yml::from_str::( + "macros:\n - signature: \"greet(name)\"\n body: |\n Hello {{ name }}\n - signature: \"shout(text)\"\n body: |\n {{ text | upper }}\n", + ) + .expect("yaml value"); + register_manifest_macros(&yaml, &mut strict_env).expect("register"); + let rendered = render_with(&strict_env, "{{ shout(greet('netsuke')) }}").expect("render"); + assert_eq!(rendered.trim(), "HELLO NETSUKE"); + } +} diff --git a/tests/ast_tests.rs b/tests/ast_tests.rs index 19ea6844..edc35953 100644 --- a/tests/ast_tests.rs +++ b/tests/ast_tests.rs @@ -198,6 +198,45 @@ fn optional_fields() { assert!(matches!(rule.deps, StringOrList::Empty)); } +#[rstest] +fn parses_macro_definitions() { + let yaml = r#" + netsuke_version: "1.0.0" + macros: + - signature: "greet(name)" + body: |- + Hello {{ name }} + targets: + - name: hello + command: "{{ greet('world') }}" + "#; + + let manifest = parse_manifest(yaml).expect("parse"); + assert_eq!(manifest.macros.len(), 1); + let macro_def = manifest.macros.first().expect("macro"); + assert_eq!(macro_def.signature, "greet(name)"); + assert!(macro_def.body.contains("Hello {{ name }}")); + + let serialised = serde_yml::to_string(&manifest.macros).expect("serialise macros"); + assert!(serialised.contains("greet(name)")); + assert!(serialised.contains("Hello {{ name }}")); +} + +#[test] +fn macro_definition_rejects_invalid_types() { + let yaml = r#" + netsuke_version: "1.0.0" + macros: + - signature: 42 + body: [] + targets: + - name: hello + command: noop + "#; + + assert!(parse_manifest(yaml).is_err()); +} + #[rstest] #[case::invalid_enum_variant( r#" diff --git a/tests/data/jinja_macro_arguments.yml b/tests/data/jinja_macro_arguments.yml new file mode 100644 index 00000000..5ab145bd --- /dev/null +++ b/tests/data/jinja_macro_arguments.yml @@ -0,0 +1,23 @@ +netsuke_version: "1.0.0" +macros: + - signature: "no_args()" + body: |- + ready + - signature: "defaulted(name='world')" + body: |- + Hi {{ name }} + - signature: "joiner(items)" + body: |- + {{ items | join(',') }} + - signature: "show(name, excited=false)" + body: |- + {{ name ~ ('!' if excited else '') }} +targets: + - name: ready + command: "{{ no_args() }}" + - name: default + command: "{{ defaulted() }}" + - name: join + command: "{{ joiner(['a', 'b', 'c']) }}" + - name: excite + command: "{{ show('Netsuke', excited=true) }}" diff --git a/tests/data/jinja_macro_missing_parens.yml b/tests/data/jinja_macro_missing_parens.yml new file mode 100644 index 00000000..0b049de3 --- /dev/null +++ b/tests/data/jinja_macro_missing_parens.yml @@ -0,0 +1,8 @@ +netsuke_version: "1.0.0" +macros: + - signature: "greet" + body: |- + hi +targets: + - name: noop + command: "noop" diff --git a/tests/features/manifest.feature b/tests/features/manifest.feature index 38172bae..60f874f8 100644 --- a/tests/features/manifest.feature +++ b/tests/features/manifest.feature @@ -67,6 +67,16 @@ Feature: Manifest Parsing When the manifest is checked Then the first target command is "HELLO NETSUKE!" + Scenario: Rendering manifest macros with varied signatures + Given the manifest file "tests/data/jinja_macro_arguments.yml" is parsed + When the manifest is checked + Then the manifest has 4 macros + And the macro 1 signature is "no_args()" + And the target 1 command is "ready" + And the target 2 command is "Hi world" + And the target 3 command is "a,b,c" + And the target 4 command is "Netsuke!" + Scenario: Parsing fails when an environment variable is undefined Given the environment variable "NETSUKE_UNDEFINED_ENV" is unset And the manifest file "tests/data/jinja_env_missing.yml" is parsed @@ -79,6 +89,12 @@ Feature: Manifest Parsing Then parsing the manifest fails And the error message contains "signature" + Scenario: Parsing fails when a macro omits parentheses + Given the manifest file "tests/data/jinja_macro_missing_parens.yml" is parsed + When the parsing result is checked + Then parsing the manifest fails + And the error message contains "parameter list" + Scenario: Rendering Jinja conditionals in a manifest Given the manifest file "tests/data/jinja_if.yml" is parsed When the manifest is checked diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs index 4dfd0139..506f2910 100644 --- a/tests/manifest_jinja_tests.rs +++ b/tests/manifest_jinja_tests.rs @@ -185,6 +185,65 @@ fn registers_manifest_macros() { } } +#[rstest] +#[case( + concat!( + "macros:\n", + " - signature: \"no_args()\"\n", + " body: |-\n", + " ready\n", + ), + "{{ no_args() }}", + "ready", +)] +#[case( + concat!( + "macros:\n", + " - signature: \"defaulted(name='world')\"\n", + " body: |-\n", + " Hi {{ name }}\n", + ), + "{{ defaulted() }}", + "Hi world", +)] +#[case( + concat!( + "macros:\n", + " - signature: \"joiner(items)\"\n", + " body: |-\n", + " {{ items | join(',') }}\n", + ), + "{{ joiner(['a', 'b', 'c']) }}", + "a,b,c", +)] +#[case( + concat!( + "macros:\n", + " - signature: \"show(name, excited=false)\"\n", + " body: |-\n", + " {{ name ~ ('!' if excited else '') }}\n", + ), + "{{ show('Netsuke', excited=true) }}", + "Netsuke!", +)] +fn registers_manifest_macro_argument_variants( + #[case] macros_block: &str, + #[case] expression: &str, + #[case] expected: &str, +) { + let command = expression.replace('"', "\\\""); + let yaml = manifest_yaml(&format!( + "{macros_block}targets:\n - name: test\n command: \"{command}\"\n", + )); + + let manifest = manifest::from_str(&yaml).expect("parse macros"); + let target = manifest.targets.first().expect("target"); + match &target.recipe { + Recipe::Command { command } => assert_eq!(command, expected), + other => panic!("expected command recipe, got {other:?}"), + } +} + #[rstest] fn manifest_macro_with_missing_signature_errors() { let yaml = manifest_yaml(concat!( diff --git a/tests/steps/manifest_steps.rs b/tests/steps/manifest_steps.rs index ca38e6ff..bfce808d 100644 --- a/tests/steps/manifest_steps.rs +++ b/tests/steps/manifest_steps.rs @@ -229,6 +229,27 @@ fn manifest_has_targets(world: &mut CliWorld, count: usize) { assert_eq!(manifest.targets.len(), count); } +#[then(expr = "the manifest has {int} macros")] +fn manifest_has_macros(world: &mut CliWorld, count: usize) { + let manifest = world.manifest.as_ref().expect("manifest"); + assert_eq!(manifest.macros.len(), count); +} + +#[then(expr = "the macro {int} signature is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] +fn macro_signature_is(world: &mut CliWorld, index: usize, signature: String) { + let manifest = world.manifest.as_ref().expect("manifest"); + let idx = index.checked_sub(1).expect("macros use 1-based index"); + let macro_def = manifest + .macros + .get(idx) + .unwrap_or_else(|| panic!("missing macro {index}")); + assert_eq!(macro_def.signature, signature); +} + #[then(expr = "the manifest has targets named {string}")] #[expect( clippy::needless_pass_by_value, From 572b85a87228653d5eb9ebbc836a8e975663aa7f Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 14 Oct 2025 23:39:50 +0100 Subject: [PATCH 03/18] Refactor register_macro to use MacroDefinition --- src/manifest.rs | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 5a174d86..bc05bbfd 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -81,10 +81,13 @@ fn parse_macro_name(signature: &str) -> Result { Ok(name.to_string()) } -fn register_macro(env: &mut Environment, signature: &str, body: &str, index: usize) -> Result<()> { - let name = parse_macro_name(signature)?; +fn register_macro(env: &mut Environment, macro_def: &MacroDefinition, index: usize) -> Result<()> { + let name = parse_macro_name(¯o_def.signature)?; let template_name = format!("__manifest_macro_{index}_{name}"); - let template_source = format!("{{% macro {signature} %}}{body}{{% endmacro %}}",); + let template_source = format!( + "{{% macro {} %}}{}{{% endmacro %}}", + macro_def.signature, macro_def.body + ); env.add_template_owned(template_name.clone(), template_source) .with_context(|| format!("compile macro '{name}'"))?; @@ -102,8 +105,13 @@ fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<() .context("macros must be a sequence of mappings with string signature/body")?; for (idx, def) in defs.into_iter().enumerate() { - register_macro(env, &def.signature, &def.body, idx) - .with_context(|| format!("register macro '{}'", def.signature))?; + let macro_def = MacroDefinition { + signature: def.signature, + body: def.body, + }; + let macro_name = macro_def.signature.clone(); + register_macro(env, ¯o_def, idx) + .with_context(|| format!("register macro '{macro_name}'"))?; } Ok(()) } @@ -252,7 +260,11 @@ mod tests { #[case] expected: &str, mut strict_env: Environment, ) { - register_macro(&mut strict_env, signature, body, 0).expect("register"); + let macro_def = MacroDefinition { + signature: signature.to_string(), + body: body.to_string(), + }; + register_macro(&mut strict_env, ¯o_def, 0).expect("register"); let rendered = render_with(&strict_env, template).expect("render"); assert_eq!(rendered, expected); } From 65e9c88e876e05832fda60f5de08f958e533b8d9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 03:11:20 +0100 Subject: [PATCH 04/18] Handle kwargs in manifest macros --- src/manifest.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index bc05bbfd..0e6d8dc7 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -11,7 +11,7 @@ use crate::ast::{MacroDefinition, NetsukeManifest}; use anyhow::{Context, Result}; use minijinja::{ Environment, Error, ErrorKind, State, UndefinedBehavior, - value::{Rest, Value}, + value::{Kwargs, Rest, Value}, }; use serde_yml::Value as YamlValue; use std::{fs, path::Path}; @@ -119,8 +119,8 @@ fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<() fn make_macro_fn( template_name: String, macro_name: String, -) -> impl Fn(&State, Rest) -> Result { - move |state, Rest(args)| { +) -> impl Fn(&State, Rest, Kwargs) -> Result { + move |state, Rest(mut args), kwargs| { let template = state.env().get_template(&template_name)?; let macro_state = template.eval_to_state(())?; let value = macro_state.lookup(¯o_name).ok_or_else(|| { @@ -129,6 +129,9 @@ fn make_macro_fn( format!("macro '{macro_name}' not defined in template '{template_name}'"), ) })?; + if kwargs.args().next().is_some() { + args.push(Value::from(kwargs)); + } value.call(¯o_state, &args) } } From b6d0de7da47997d7b369b7b28b643326a0f7e2b9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 10:16:32 +0100 Subject: [PATCH 05/18] Collect kwargs before invoking manifest macros --- src/manifest.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 0e6d8dc7..158df827 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -120,7 +120,7 @@ fn make_macro_fn( template_name: String, macro_name: String, ) -> impl Fn(&State, Rest, Kwargs) -> Result { - move |state, Rest(mut args), kwargs| { + move |state, Rest(args), kwargs| { let template = state.env().get_template(&template_name)?; let macro_state = template.eval_to_state(())?; let value = macro_state.lookup(¯o_name).ok_or_else(|| { @@ -129,10 +129,16 @@ fn make_macro_fn( format!("macro '{macro_name}' not defined in template '{template_name}'"), ) })?; - if kwargs.args().next().is_some() { - args.push(Value::from(kwargs)); - } - value.call(¯o_state, &args) + + let call_args = if kwargs.args().next().is_some() { + args.into_iter() + .chain(std::iter::once(Value::from(kwargs))) + .collect::>() + } else { + args + }; + + value.call(¯o_state, &call_args) } } From 341e1f0172e628b3b62c305279d2fd33e7ce393f Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 10:16:37 +0100 Subject: [PATCH 06/18] Avoid cloning manifest macro definitions --- src/manifest.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 158df827..90e8e4c6 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -104,14 +104,9 @@ fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<() let defs: Vec = serde_yml::from_value(macros) .context("macros must be a sequence of mappings with string signature/body")?; - for (idx, def) in defs.into_iter().enumerate() { - let macro_def = MacroDefinition { - signature: def.signature, - body: def.body, - }; - let macro_name = macro_def.signature.clone(); - register_macro(env, ¯o_def, idx) - .with_context(|| format!("register macro '{macro_name}'"))?; + for (idx, def) in defs.iter().enumerate() { + register_macro(env, def, idx) + .with_context(|| format!("register macro '{}'", def.signature))?; } Ok(()) } From 4f566348f6a1b2106c0ffea13040a5e43984187e Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 10:42:11 +0100 Subject: [PATCH 07/18] Add ManifestName wrapper for manifest parsing --- src/manifest.rs | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 90e8e4c6..437c3bd3 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -16,6 +16,27 @@ use minijinja::{ use serde_yml::Value as YamlValue; use std::{fs, path::Path}; +/// A display name for a manifest source, used in error reporting. +#[derive(Debug, Clone)] +pub struct ManifestName(String); + +impl ManifestName { + pub fn new(name: impl Into) -> Self { + Self(name.into()) + } + + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl std::fmt::Display for ManifestName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + mod diagnostics; mod expand; mod glob; @@ -145,9 +166,9 @@ fn make_macro_fn( /// # Errors /// /// Returns an error if YAML parsing or Jinja evaluation fails. -fn from_str_named(yaml: &str, name: &str) -> Result { +fn from_str_named(yaml: &str, name: &ManifestName) -> Result { let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| ManifestError::Parse { - source: map_yaml_error(e, yaml, name), + source: map_yaml_error(e, yaml, name.as_str()), })?; let mut jinja = Environment::new(); @@ -173,7 +194,7 @@ fn from_str_named(yaml: &str, name: &str) -> Result { let manifest: NetsukeManifest = serde_yml::from_value(doc).map_err(|e| ManifestError::Parse { - source: map_yaml_error(e, yaml, name), + source: map_yaml_error(e, yaml, name.as_str()), })?; render_manifest(manifest, &jinja) @@ -188,7 +209,7 @@ fn from_str_named(yaml: &str, name: &str) -> Result { /// /// Returns an error if YAML parsing or Jinja evaluation fails. pub fn from_str(yaml: &str) -> Result { - from_str_named(yaml, "Netsukefile") + from_str_named(yaml, &ManifestName::new("Netsukefile")) } /// Load a [`NetsukeManifest`] from the given file path. @@ -200,7 +221,8 @@ pub fn from_path(path: impl AsRef) -> Result { let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref) .with_context(|| format!("failed to read {}", path_ref.display()))?; - from_str_named(&data, &path_ref.display().to_string()) + let name = ManifestName::new(path_ref.display().to_string()); + from_str_named(&data, &name) } #[cfg(test)] From e1708fc272e6c7702d002ca6dc9f3e353bdd405a Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 11:01:29 +0100 Subject: [PATCH 08/18] Document manifest macro helpers --- src/manifest.rs | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/manifest.rs b/src/manifest.rs index 437c3bd3..d2f101b3 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -81,6 +81,22 @@ fn env_var(name: &str) -> std::result::Result { } } +/// Extract the macro identifier from a signature string. +/// +/// The signature must follow the form `name(params)` where `name` is a valid +/// Jinja identifier and `params` is a parameter list (possibly empty). +/// +/// # Errors +/// +/// Returns an error if the signature is empty, lacks a parameter list, or the +/// identifier before `(` is empty. +/// +/// # Examples +/// +/// ```rust,ignore +/// let name = parse_macro_name("greet(name)").expect("valid signature"); +/// assert_eq!(name, "greet"); +/// ``` fn parse_macro_name(signature: &str) -> Result { let trimmed = signature.trim(); if trimmed.is_empty() { @@ -102,6 +118,16 @@ fn parse_macro_name(signature: &str) -> Result { Ok(name.to_string()) } +/// Register a single manifest macro in the Jinja environment. +/// +/// Compiles the macro body into a template and registers a callable function +/// with the extracted macro name. The template name is synthesised using the +/// provided index to ensure uniqueness. +/// +/// # Errors +/// +/// Returns an error if the macro signature is invalid or template compilation +/// fails. fn register_macro(env: &mut Environment, macro_def: &MacroDefinition, index: usize) -> Result<()> { let name = parse_macro_name(¯o_def.signature)?; let template_name = format!("__manifest_macro_{index}_{name}"); @@ -117,6 +143,16 @@ fn register_macro(env: &mut Environment, macro_def: &MacroDefinition, index: usi Ok(()) } +/// Register all manifest macros from a YAML document. +/// +/// Expects the YAML to have a `macros` key containing a sequence of mappings, +/// each with `signature` and `body` string fields. Registers each macro in the +/// environment using [`register_macro`]. +/// +/// # Errors +/// +/// Returns an error if the YAML shape is invalid, any macro signature is +/// malformed, or template compilation fails. fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<()> { let Some(macros) = doc.get("macros").cloned() else { return Ok(()); From 3dfdc0e2993f7d58ee3c7fd0cd789c494512dc84 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 14:08:29 +0100 Subject: [PATCH 09/18] Document macro wrapper and forward kwargs --- src/manifest.rs | 58 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index d2f101b3..11116519 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -168,6 +168,42 @@ fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<() Ok(()) } +/// Create a wrapper that invokes a compiled manifest macro on demand. +/// +/// The returned closure fetches the internal template, resolves the macro, and +/// forwards the provided positional and keyword arguments to the call. +/// +/// # Examples +/// +/// ```rust,ignore +/// # use minijinja::Environment; +/// # use minijinja::value::{Kwargs, Rest, Value}; +/// # use netsuke::manifest::make_macro_fn; +/// let mut env = Environment::new(); +/// env.add_template( +/// "macro", +/// "{% macro greet(name='friend') %}hi {{ name }}{% endmacro %}", +/// ) +/// .unwrap(); +/// let wrapper = make_macro_fn("macro".into(), "greet".into()); +/// let state = env +/// .get_template("macro") +/// .unwrap() +/// .eval_to_state(()) +/// .unwrap(); +/// let kwargs = Kwargs::from_iter([ +/// (String::from("name"), Value::from("Ada")), +/// ]); +/// let output = wrapper(&state, Rest(vec![]), kwargs) +/// .unwrap() +/// .to_string(); +/// assert_eq!(output, "hi Ada"); +/// ``` +/// +/// # Errors +/// +/// The wrapper returns an error if the macro cannot be located or execution +/// fails. fn make_macro_fn( template_name: String, macro_name: String, @@ -182,16 +218,22 @@ fn make_macro_fn( ) })?; - let call_args = if kwargs.args().next().is_some() { - args.into_iter() - .chain(std::iter::once(Value::from(kwargs))) - .collect::>() - } else { - args - }; + call_macro_value(&value, ¯o_state, args, kwargs) + } +} - value.call(¯o_state, &call_args) +fn call_macro_value( + value: &Value, + state: &State, + mut args: Vec, + kwargs: Kwargs, +) -> Result { + let has_kwargs = kwargs.args().next().is_some(); + if has_kwargs { + args.push(Value::from(kwargs)); } + + value.call(state, &args) } /// Parse a manifest string using Jinja for value templating. From 562fa548771ef4dddab7339ee55abf3923f03f49 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 14:35:23 +0100 Subject: [PATCH 10/18] Preserve macro kwargs when calling wrappers --- src/manifest.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 11116519..1ee41748 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -225,15 +225,19 @@ fn make_macro_fn( fn call_macro_value( value: &Value, state: &State, - mut args: Vec, + args: Vec, kwargs: Kwargs, ) -> Result { - let has_kwargs = kwargs.args().next().is_some(); - if has_kwargs { - args.push(Value::from(kwargs)); + if kwargs.args().next().is_some() { + let mut call_args = args; + // MiniJinja encodes keyword arguments as a trailing `Kwargs` value on the + // argument slice. Push the wrapper rather than attempting to call with + // a separate parameter so the runtime extracts keywords correctly. + call_args.push(Value::from(kwargs)); + value.call(state, call_args.as_slice()) + } else { + value.call(state, args.as_slice()) } - - value.call(state, &args) } /// Parse a manifest string using Jinja for value templating. From aed58c6d0c2749ba355d9ea796f3cbe8e5a13493 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 15 Oct 2025 23:28:21 +0100 Subject: [PATCH 11/18] Forward manifest macro kwargs via helper --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- src/manifest.rs | 27 ++++++++++++++++++++------- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a8c37e89..49965b16 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1141,9 +1141,9 @@ dependencies = [ [[package]] name = "minijinja" -version = "2.11.0" +version = "2.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4e60ac08614cc09062820e51d5d94c2fce16b94ea4e5003bb81b99a95f84e876" +checksum = "a9f264d75233323f4b7d2f03aefe8a990690cdebfbfe26ea86bcbaec5e9ac990" dependencies = [ "memo-map", "self_cell", diff --git a/Cargo.toml b/Cargo.toml index 06a37b1c..174740e1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,7 @@ legacy-digests = ["sha1", "md5"] clap = { version = "4.5.0", features = ["derive"] } serde = { version = "1", features = ["derive"] } serde_yml = "0.0.12" -minijinja = { version = "2.11.0", features = ["loader"] } +minijinja = { version = "2.12.0", features = ["loader"] } cap-std = { version = "3.4.4", features = ["fs_utf8"] } camino = "1.2.0" semver = { version = "1", features = ["serde"] } diff --git a/src/manifest.rs b/src/manifest.rs index 1ee41748..1dde1f60 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -218,25 +218,38 @@ fn make_macro_fn( ) })?; - call_macro_value(&value, ¯o_state, args, kwargs) + call_macro_value(&value, ¯o_state, args.as_slice(), kwargs) } } fn call_macro_value( value: &Value, state: &State, - args: Vec, + args: &[Value], + kwargs: Kwargs, +) -> Result { + call_value_with_kwargs(value, state, args, kwargs) +} + +/// Bridge `MiniJinja`'s calling convention to the wrapper-friendly signature. +/// +/// `MiniJinja` encodes keyword arguments as a trailing [`Kwargs`] value in the +/// positional slice, so this helper rebuilds the slice when keywords are +/// provided before invoking [`Value::call`]. This keeps the wrapper logic +/// straightforward while matching the runtime's expectations. +fn call_value_with_kwargs( + value: &Value, + state: &State, + args: &[Value], kwargs: Kwargs, ) -> Result { if kwargs.args().next().is_some() { - let mut call_args = args; - // MiniJinja encodes keyword arguments as a trailing `Kwargs` value on the - // argument slice. Push the wrapper rather than attempting to call with - // a separate parameter so the runtime extracts keywords correctly. + let mut call_args = Vec::with_capacity(args.len() + 1); + call_args.extend_from_slice(args); call_args.push(Value::from(kwargs)); value.call(state, call_args.as_slice()) } else { - value.call(state, args.as_slice()) + value.call(state, args) } } From 534b23ea0f6fbfbffbbb044b6c9d2950986ba429 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 00:31:34 +0100 Subject: [PATCH 12/18] Forward manifest macro kwargs via trait --- src/manifest.rs | 132 +++---------------------- src/manifest/tests.rs | 223 ++++++++++++++---------------------------- 2 files changed, 88 insertions(+), 267 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 1dde1f60..83073b62 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -228,28 +228,23 @@ fn call_macro_value( args: &[Value], kwargs: Kwargs, ) -> Result { - call_value_with_kwargs(value, state, args, kwargs) + ValueCallExt::call(value, state, args, kwargs) } -/// Bridge `MiniJinja`'s calling convention to the wrapper-friendly signature. -/// -/// `MiniJinja` encodes keyword arguments as a trailing [`Kwargs`] value in the -/// positional slice, so this helper rebuilds the slice when keywords are -/// provided before invoking [`Value::call`]. This keeps the wrapper logic -/// straightforward while matching the runtime's expectations. -fn call_value_with_kwargs( - value: &Value, - state: &State, - args: &[Value], - kwargs: Kwargs, -) -> Result { - if kwargs.args().next().is_some() { - let mut call_args = Vec::with_capacity(args.len() + 1); - call_args.extend_from_slice(args); - call_args.push(Value::from(kwargs)); - value.call(state, call_args.as_slice()) - } else { - value.call(state, args) +trait ValueCallExt { + fn call(&self, state: &State, args: &[Value], kwargs: Kwargs) -> Result; +} + +impl ValueCallExt for Value { + fn call(&self, state: &State, args: &[Value], kwargs: Kwargs) -> Result { + if kwargs.args().next().is_some() { + let mut call_args = Vec::with_capacity(args.len() + 1); + call_args.extend_from_slice(args); + call_args.push(Self::from(kwargs)); + Self::call(self, state, call_args.as_slice()) + } else { + Self::call(self, state, args) + } } } @@ -321,99 +316,4 @@ pub fn from_path(path: impl AsRef) -> Result { } #[cfg(test)] -mod tests { - use super::*; - use anyhow::Result as AnyResult; - use minijinja::Environment; - use rstest::{fixture, rstest}; - use serde_yml::value::Mapping; - - fn render_with(env: &Environment, template: &str) -> AnyResult { - Ok(env.render_str(template, ())?) - } - - #[fixture] - fn strict_env() -> Environment<'static> { - let mut env = Environment::new(); - env.set_undefined_behavior(UndefinedBehavior::Strict); - env - } - - #[rstest] - #[case("greet(name)", "greet")] - #[case("shout(text='hi')", "shout")] - #[case("joiner(*items)", "joiner")] - #[case("format(name, caller=None)", "format")] - #[case("complex(value, /, *, flag=false, **kw)", "complex")] - fn parse_macro_name_extracts_identifier(#[case] signature: &str, #[case] expected: &str) { - let name = parse_macro_name(signature).expect("parse name"); - assert_eq!(name, expected); - } - - #[rstest] - #[case("greet", "include parameter list")] - #[case("(name)", "missing an identifier")] - #[case(" ", "missing an identifier")] - fn parse_macro_name_errors(#[case] signature: &str, #[case] message: &str) { - let err = parse_macro_name(signature).expect_err("should fail"); - assert!(err.to_string().contains(message), "{err}"); - } - - #[rstest] - #[case("greet()", "Hello", "{{ greet() }}", "Hello")] - #[case("echo(text='hi')", "{{ text }}", "{{ echo() }}", "hi")] - #[case( - "joiner(items)", - "{{ items | join(',') }}", - "{{ joiner(['a', 'b', 'c']) }}", - "a,b,c" - )] - #[case( - "show(name, excited=false)", - "{{ name ~ ('!' if excited else '') }}", - "{{ show('Netsuke', excited=true) }}", - "Netsuke!" - )] - fn register_macro_handles_arguments( - #[case] signature: &str, - #[case] body: &str, - #[case] template: &str, - #[case] expected: &str, - mut strict_env: Environment, - ) { - let macro_def = MacroDefinition { - signature: signature.to_string(), - body: body.to_string(), - }; - register_macro(&mut strict_env, ¯o_def, 0).expect("register"); - let rendered = render_with(&strict_env, template).expect("render"); - assert_eq!(rendered, expected); - } - - #[rstest] - fn register_manifest_macros_validates_shape(mut strict_env: Environment) { - let mut mapping = Mapping::new(); - mapping.insert( - YamlValue::from("macros"), - YamlValue::from(vec![YamlValue::from(42)]), - ); - let doc = YamlValue::Mapping(mapping); - let err = register_manifest_macros(&doc, &mut strict_env).expect_err("shape error"); - assert!( - err.to_string() - .contains("macros must be a sequence of mappings"), - "{err}" - ); - } - - #[rstest] - fn register_manifest_macros_supports_multiple(mut strict_env: Environment) { - let yaml = serde_yml::from_str::( - "macros:\n - signature: \"greet(name)\"\n body: |\n Hello {{ name }}\n - signature: \"shout(text)\"\n body: |\n {{ text | upper }}\n", - ) - .expect("yaml value"); - register_manifest_macros(&yaml, &mut strict_env).expect("register"); - let rendered = render_with(&strict_env, "{{ shout(greet('netsuke')) }}").expect("render"); - assert_eq!(rendered.trim(), "HELLO NETSUKE"); - } -} +mod tests; diff --git a/src/manifest/tests.rs b/src/manifest/tests.rs index df2793d3..e583b5e5 100644 --- a/src/manifest/tests.rs +++ b/src/manifest/tests.rs @@ -1,175 +1,96 @@ -use super::diagnostics::map_yaml_error; -use super::glob::{glob_paths, normalize_separators}; -use super::hints::YAML_HINTS; -use rstest::{fixture, rstest}; -use serde::de::Error as _; -use serde_yml::Error as YamlError; - -#[test] -fn yaml_error_without_location_defaults_to_first_line() { - let err = YamlError::custom("boom"); - let msg = map_yaml_error(err, "", "test").to_string(); - assert!(msg.contains("line 1, column 1"), "message: {msg}"); -} - -#[test] -fn yaml_hint_needles_are_lowercase() { - for (needle, _) in YAML_HINTS { - assert_eq!( - needle, - needle.to_lowercase(), - "needle not lower-case: {needle}" - ); - } -} +//! Tests for manifest parsing and macro registration helpers. -#[test] -fn glob_paths_invalid_pattern_sets_syntax_error() { - let err = glob_paths("[").expect_err("expected pattern error"); - assert_eq!(err.kind(), minijinja::ErrorKind::SyntaxError); -} +use super::*; +use anyhow::Result as AnyResult; +use minijinja::Environment; +use rstest::{fixture, rstest}; +use serde_yml::value::Mapping; -#[cfg(unix)] -#[rstest] -#[case("\\[")] -#[case("\\]")] -#[case("\\{")] -#[case("\\}")] -fn normalize_separators_preserves_bracket_escape_variants(#[case] pat: &str) { - assert_eq!(normalize_separators(pat), pat); +fn render_with(env: &Environment, template: &str) -> AnyResult { + Ok(env.render_str(template, ())?) } -#[cfg(unix)] -#[rstest] -#[case("a\\b\\*", "a/b\\*")] -#[case("a\\b\\?", "a/b\\?")] -#[case("config\\*.yml", "config/*.yml")] -#[case("data\\?x.csv", "data\\?x.csv")] -fn normalize_separators_preserves_wildcard_escape_variants( - #[case] pat: &str, - #[case] expected: &str, -) { - // Inputs like "config\\*.yml" are treated as Windows-style and turn the - // backslash into a separator on Unix; other cases keep '\\' to force a - // literal via bracket-class rewrite downstream. - assert_eq!(normalize_separators(pat), expected); +#[fixture] +fn strict_env() -> Environment<'static> { + let mut env = Environment::new(); + env.set_undefined_behavior(UndefinedBehavior::Strict); + env } -#[cfg(unix)] #[rstest] -#[case("assets/\\*.\\?", "assets//*.\\?")] -#[case("src/\\[a\\].c", "src/\\[a\\].c")] -#[case("build/\\{debug,release\\}/lib", "build/\\{debug,release\\}/lib")] -fn normalize_separators_preserves_specific_escape_patterns( - #[case] pat: &str, - #[case] expected: &str, -) { - // Escaped metacharacters are preserved verbatim on Unix. - assert_eq!(normalize_separators(pat), expected); -} - -#[cfg(unix)] -#[test] -fn normalize_separators_handles_mixed_slashes() { - let input = r"dir\sub/leaf"; - assert_eq!(normalize_separators(input), "dir/sub/leaf"); -} - -#[fixture] -fn tmp() -> tempfile::TempDir { - tempfile::tempdir().expect("temp dir") +#[case("greet(name)", "greet")] +#[case("shout(text='hi')", "shout")] +#[case("joiner(*items)", "joiner")] +#[case("format(name, caller=None)", "format")] +#[case("complex(value, /, *, flag=false, **kw)", "complex")] +fn parse_macro_name_extracts_identifier(#[case] signature: &str, #[case] expected: &str) { + let name = parse_macro_name(signature).expect("parse name"); + assert_eq!(name, expected); } -#[cfg(unix)] #[rstest] -#[case("\\*", "*")] -#[case("\\?", "?")] -#[case("prefix\\*suffix", "prefix*suffix")] -#[case("pre\\?post", "pre?post")] -#[case("mid\\*-", "mid*-")] -#[case("mid\\?_", "mid?_")] -fn glob_paths_treats_escaped_wildcards_as_literals( - tmp: tempfile::TempDir, - #[case] pattern_suffix: &str, - #[case] filename: &str, -) { - std::fs::write(tmp.path().join(filename), "a").expect("write file"); - std::fs::write(tmp.path().join("other"), "b").expect("write file"); - let pattern = format!("{}/{}", tmp.path().display(), pattern_suffix); - let out = glob_paths(&pattern).expect("glob ok"); - assert_eq!( - out, - vec![format!("{}/{}", tmp.path().display(), filename).replace('\\', "/"),], - ); +#[case("greet", "include parameter list")] +#[case("(name)", "missing an identifier")] +#[case(" ", "missing an identifier")] +fn parse_macro_name_errors(#[case] signature: &str, #[case] message: &str) { + let err = parse_macro_name(signature).expect_err("should fail"); + assert!(err.to_string().contains(message), "{err}"); } -#[cfg(unix)] #[rstest] +#[case("greet()", "Hello", "{{ greet() }}", "Hello")] +#[case("echo(text='hi')", "{{ text }}", "{{ echo() }}", "hi")] #[case( - vec![("sub", true), ("a.txt", false)], // (name, is_dir) - "*", - vec!["a.txt"], - "ignores directories and matches only files" -)] -#[case( - vec![ - ("dir", true), - ("dir/prefix*suffix", true), - ("dir/prefix*suffix/next", false), - ], - "dir/prefix\\*suffix/*", - vec!["dir/prefix*suffix/next"], - "treats escaped '*' as literal inside a segment across subsequent segments", + "joiner(items)", + "{{ items | join(',') }}", + "{{ joiner(['a', 'b', 'c']) }}", + "a,b,c" )] #[case( - vec![("[ab]", false), ("b", false)], - "\\[ab\\]", - vec!["[ab]"], - "respects bracket escapes - treats as literals" + "show(name, excited=false)", + "{{ name ~ ('!' if excited else '') }}", + "{{ show('Netsuke', excited=true) }}", + "Netsuke!" )] -#[case( - vec![("{debug,release}", false)], - "\\{debug,release\\}", - vec!["{debug,release}"], - "treats braces as literals when escaped" -)] -fn glob_paths_behavior_scenarios( - tmp: tempfile::TempDir, - #[case] files_to_create: Vec<(&str, bool)>, // (name, is_directory) - #[case] pattern_suffix: &str, - #[case] expected_matches: Vec<&str>, - #[case] description: &str, +fn register_macro_handles_arguments( + #[case] signature: &str, + #[case] body: &str, + #[case] template: &str, + #[case] expected: &str, + mut strict_env: Environment, ) { - for (name, is_dir) in &files_to_create { - let path = tmp.path().join(name); - if *is_dir { - std::fs::create_dir(&path).expect("create dir"); - } else { - std::fs::write(&path, "content").expect("write file"); - } - } - - let pattern = format!("{}/{}", tmp.path().display(), pattern_suffix); - let mut result = glob_paths(&pattern).expect("glob ok"); - result.sort(); - let mut expected: Vec = expected_matches - .iter() - .map(|name| format!("{}/{}", tmp.path().display(), name).replace('\\', "/")) - .collect(); - expected.sort(); - assert_eq!(result, expected, "Test case: {description}"); + let macro_def = MacroDefinition { + signature: signature.to_string(), + body: body.to_string(), + }; + register_macro(&mut strict_env, ¯o_def, 0).expect("register"); + let rendered = render_with(&strict_env, template).expect("render"); + assert_eq!(rendered, expected); } #[rstest] -fn glob_paths_returns_empty_for_no_matches(tmp: tempfile::TempDir) { - let pattern = format!("{}/missing*", tmp.path().display()); - let result = glob_paths(&pattern).expect("glob ok"); - assert!(result.is_empty()); +fn register_manifest_macros_validates_shape(mut strict_env: Environment) { + let mut mapping = Mapping::new(); + mapping.insert( + YamlValue::from("macros"), + YamlValue::from(vec![YamlValue::from(42)]), + ); + let doc = YamlValue::Mapping(mapping); + let err = register_manifest_macros(&doc, &mut strict_env).expect_err("shape error"); + assert!( + err.to_string() + .contains("macros must be a sequence of mappings"), + "{err}" + ); } -#[test] -fn glob_paths_reports_unmatched_brace() { - let err = glob_paths("foo/{bar").expect_err("expected brace error"); - assert_eq!(err.kind(), minijinja::ErrorKind::SyntaxError); - assert!(err.to_string().contains("unmatched '{'")); +#[rstest] +fn register_manifest_macros_supports_multiple(mut strict_env: Environment) { + let yaml = serde_yml::from_str::( + "macros:\n - signature: \"greet(name)\"\n body: |\n Hello {{ name }}\n - signature: \"shout(text)\"\n body: |\n {{ text | upper }}\n", + ) + .expect("yaml value"); + register_manifest_macros(&yaml, &mut strict_env).expect("register"); + let rendered = render_with(&strict_env, "{{ shout(greet('netsuke')) }}").expect("render"); + assert_eq!(rendered.trim(), "HELLO NETSUKE"); } From fc53b8ef5e53e45a1086f76c05ee6ac41582b498 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 00:59:01 +0100 Subject: [PATCH 13/18] Inline macro call helper Document the ManifestName accessors for the new public API and call the ValueCallExt helper directly from the manifest macro wrapper. --- src/manifest.rs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 83073b62..8688c2d5 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -21,11 +21,29 @@ use std::{fs, path::Path}; pub struct ManifestName(String); impl ManifestName { + /// Construct a manifest name for diagnostics. + /// + /// # Examples + /// + /// ```rust,ignore + /// use netsuke::manifest::ManifestName; + /// let name = ManifestName::new("Netsukefile"); + /// assert_eq!(name.to_string(), "Netsukefile"); + /// ``` pub fn new(name: impl Into) -> Self { Self(name.into()) } #[must_use] + /// Borrow the manifest name as a string slice. + /// + /// # Examples + /// + /// ```rust,ignore + /// use netsuke::manifest::ManifestName; + /// let name = ManifestName::new("Config"); + /// assert_eq!(name.as_str(), "Config"); + /// ``` pub fn as_str(&self) -> &str { &self.0 } @@ -218,19 +236,10 @@ fn make_macro_fn( ) })?; - call_macro_value(&value, ¯o_state, args.as_slice(), kwargs) + ::call(&value, ¯o_state, args.as_slice(), kwargs) } } -fn call_macro_value( - value: &Value, - state: &State, - args: &[Value], - kwargs: Kwargs, -) -> Result { - ValueCallExt::call(value, state, args, kwargs) -} - trait ValueCallExt { fn call(&self, state: &State, args: &[Value], kwargs: Kwargs) -> Result; } From 41ba8c846a1e1caee241d12edcae4fb1ee75a708 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 01:34:29 +0100 Subject: [PATCH 14/18] Document kwargs forwarding Clarify why manifest macro wrappers append a Kwargs value before invoking minijinja::Value::call so keyword arguments reach macro implementations. --- src/manifest.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/manifest.rs b/src/manifest.rs index 8688c2d5..c6b07070 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -247,6 +247,10 @@ trait ValueCallExt { impl ValueCallExt for Value { fn call(&self, state: &State, args: &[Value], kwargs: Kwargs) -> Result { if kwargs.args().next().is_some() { + // MiniJinja encodes keyword arguments as a trailing `Kwargs` value + // in the positional slice passed to [`Value::call`]. The API does + // not expose a dedicated parameter for kwargs, so we append the + // converted `Value` when at least one named argument is present. let mut call_args = Vec::with_capacity(args.len() + 1); call_args.extend_from_slice(args); call_args.push(Self::from(kwargs)); From 390978bc3b4a7be40dc11dfa22e44a29d2a8502e Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 09:45:48 +0100 Subject: [PATCH 15/18] Support caller kwargs in manifest macros --- src/manifest.rs | 68 +++++++++++++++++++++++++++---------------- src/manifest/tests.rs | 12 ++++++++ 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index c6b07070..b73ce200 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -10,10 +10,11 @@ use crate::ast::{MacroDefinition, NetsukeManifest}; use anyhow::{Context, Result}; use minijinja::{ - Environment, Error, ErrorKind, State, UndefinedBehavior, - value::{Kwargs, Rest, Value}, + AutoEscape, Environment, Error, ErrorKind, State, UndefinedBehavior, + value::{Kwargs, Object, Rest, Value}, }; use serde_yml::Value as YamlValue; +use std::sync::Arc; use std::{fs, path::Path}; /// A display name for a manifest source, used in error reporting. @@ -229,35 +230,52 @@ fn make_macro_fn( move |state, Rest(args), kwargs| { let template = state.env().get_template(&template_name)?; let macro_state = template.eval_to_state(())?; - let value = macro_state.lookup(¯o_name).ok_or_else(|| { - Error::new( - ErrorKind::InvalidOperation, - format!("macro '{macro_name}' not defined in template '{template_name}'"), - ) - })?; + let mut call_args = Vec::with_capacity(args.len() + 1); + call_args.extend_from_slice(&args); + let mut entries: Vec<(String, Value)> = Vec::new(); + for key in kwargs.args() { + let mut value = kwargs.peek::(key)?; + if key == "caller" { + value = Value::from_object(CallerAdapter::new(state, value)); + } + entries.push((key.to_string(), value)); + } + if !entries.is_empty() { + call_args.push(Value::from(Kwargs::from_iter(entries))); + } - ::call(&value, ¯o_state, args.as_slice(), kwargs) + let rendered = macro_state.call_macro(¯o_name, call_args.as_slice())?; + + let value = if matches!(state.auto_escape(), AutoEscape::None) { + Value::from(rendered) + } else { + Value::from_safe_string(rendered) + }; + Ok(value) } } -trait ValueCallExt { - fn call(&self, state: &State, args: &[Value], kwargs: Kwargs) -> Result; +#[derive(Debug)] +struct CallerAdapter { + caller: Value, + state: *const State<'static, 'static>, } -impl ValueCallExt for Value { - fn call(&self, state: &State, args: &[Value], kwargs: Kwargs) -> Result { - if kwargs.args().next().is_some() { - // MiniJinja encodes keyword arguments as a trailing `Kwargs` value - // in the positional slice passed to [`Value::call`]. The API does - // not expose a dedicated parameter for kwargs, so we append the - // converted `Value` when at least one named argument is present. - let mut call_args = Vec::with_capacity(args.len() + 1); - call_args.extend_from_slice(args); - call_args.push(Self::from(kwargs)); - Self::call(self, state, call_args.as_slice()) - } else { - Self::call(self, state, args) - } +impl CallerAdapter { + fn new(state: &State, caller: Value) -> Self { + #[allow(clippy::cast_ptr_alignment)] + let ptr = state as *const State<'_, '_> as *const State<'static, 'static>; + Self { caller, state: ptr } + } +} + +unsafe impl Send for CallerAdapter {} +unsafe impl Sync for CallerAdapter {} + +impl Object for CallerAdapter { + fn call(self: &Arc, _state: &State, args: &[Value]) -> Result { + let state = unsafe { &*self.state.cast::>() }; + self.caller.call(state, args) } } diff --git a/src/manifest/tests.rs b/src/manifest/tests.rs index e583b5e5..b57d99cf 100644 --- a/src/manifest/tests.rs +++ b/src/manifest/tests.rs @@ -68,6 +68,18 @@ fn register_macro_handles_arguments( assert_eq!(rendered, expected); } +#[rstest] +fn register_macro_forwards_caller(mut strict_env: Environment) { + let macro_def = MacroDefinition { + signature: "wrap(prefix, caller)".to_string(), + body: "{{ prefix }}{{ caller() }}".to_string(), + }; + register_macro(&mut strict_env, ¯o_def, 0).expect("register"); + let rendered = + render_with(&strict_env, "{% call wrap('Hi ') %}World{% endcall %}").expect("render"); + assert_eq!(rendered.trim(), "Hi World"); +} + #[rstest] fn register_manifest_macros_validates_shape(mut strict_env: Environment) { let mut mapping = Mapping::new(); From 62909e3984426e4711c2bbe294ccd41cb788874b Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 12:33:08 +0100 Subject: [PATCH 16/18] Add ManifestName AsRef and document serde_yml risk --- docs/netsuke-design.md | 5 +++++ src/manifest.rs | 18 +++++++++++------- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 9acad8a4..91498a9b 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -417,6 +417,11 @@ unnecessary project risk. `serde_yml` is mature, widely adopted, and battle-tested, making it the prudent choice for production-quality software. +**Maintenance risk.** `serde_yml` is archived upstream and carries unsoundness +advisories. Netsuke relies on it today, but we will investigate a maintained +successor such as `serde_yaml_ng`. A follow-up ADR will outline the migration +plan and compatibility testing. + ### 3.2 Core Data Structures (`ast.rs`) The Rust structs that `serde_yml` will deserialise into form the Abstract diff --git a/src/manifest.rs b/src/manifest.rs index b73ce200..585a9b2e 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -14,8 +14,7 @@ use minijinja::{ value::{Kwargs, Object, Rest, Value}, }; use serde_yml::Value as YamlValue; -use std::sync::Arc; -use std::{fs, path::Path}; +use std::{fs, path::Path, ptr, sync::Arc}; /// A display name for a manifest source, used in error reporting. #[derive(Debug, Clone)] @@ -56,6 +55,12 @@ impl std::fmt::Display for ManifestName { } } +impl AsRef for ManifestName { + fn as_ref(&self) -> &str { + self.0.as_str() + } +} + mod diagnostics; mod expand; mod glob; @@ -263,8 +268,7 @@ struct CallerAdapter { impl CallerAdapter { fn new(state: &State, caller: Value) -> Self { - #[allow(clippy::cast_ptr_alignment)] - let ptr = state as *const State<'_, '_> as *const State<'static, 'static>; + let ptr = ptr::from_ref(state).cast::>(); Self { caller, state: ptr } } } @@ -274,7 +278,7 @@ unsafe impl Sync for CallerAdapter {} impl Object for CallerAdapter { fn call(self: &Arc, _state: &State, args: &[Value]) -> Result { - let state = unsafe { &*self.state.cast::>() }; + let state = unsafe { &*self.state }; self.caller.call(state, args) } } @@ -289,7 +293,7 @@ impl Object for CallerAdapter { /// Returns an error if YAML parsing or Jinja evaluation fails. fn from_str_named(yaml: &str, name: &ManifestName) -> Result { let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| ManifestError::Parse { - source: map_yaml_error(e, yaml, name.as_str()), + source: map_yaml_error(e, yaml, name.as_ref()), })?; let mut jinja = Environment::new(); @@ -315,7 +319,7 @@ fn from_str_named(yaml: &str, name: &ManifestName) -> Result { let manifest: NetsukeManifest = serde_yml::from_value(doc).map_err(|e| ManifestError::Parse { - source: map_yaml_error(e, yaml, name.as_str()), + source: map_yaml_error(e, yaml, name.as_ref()), })?; render_manifest(manifest, &jinja) From e6816603bba798a9f5a0bd7fd3fecf80ff74256a Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 18:21:32 +0100 Subject: [PATCH 17/18] Document caller adapter safety contract - explain the raw pointer invariants around CallerAdapter so review safety concerns are addressed - capture follow-up ADR and migration spike tasks for replacing serde_yml as requested during review --- docs/netsuke-design.md | 7 +++++++ src/manifest.rs | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 91498a9b..0b81be6f 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -422,6 +422,13 @@ advisories. Netsuke relies on it today, but we will investigate a maintained successor such as `serde_yaml_ng`. A follow-up ADR will outline the migration plan and compatibility testing. +Follow-up actions: + +- Draft an ADR comparing `serde_yml` with candidate replacements and recording + the migration decision. +- Schedule a migration spike to prototype deserialisation with the preferred + crate and capture compatibility notes. + ### 3.2 Core Data Structures (`ast.rs`) The Rust structs that `serde_yml` will deserialise into form the Abstract diff --git a/src/manifest.rs b/src/manifest.rs index 585a9b2e..7f802f1f 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -260,6 +260,26 @@ fn make_macro_fn( } } +/// Adapter to preserve the outer template state for caller block invocation. +/// +/// `MiniJinja` executes the wrapper closure with a synthetic [`State`] that +/// differs from the one active when the manifest macro was called. Caller +/// blocks, however, must run within the original context to access the +/// manifest's globals and captured variables. This adapter stores a raw +/// pointer to that state so the macro can invoke the block later. +/// +/// # Safety +/// +/// The raw pointer is only valid while the outer [`State`] is alive. Safety +/// hinges on: +/// +/// - the macro invocation remaining synchronous (no `async` suspension) +/// - the original state outliving every [`CallerAdapter`] invocation +/// - the adapter not moving across threads despite the `Send`/`Sync` impls +/// +/// The unsynchronised `Send` and `Sync` impls mirror `MiniJinja`'s built-in +/// macro objects. They rely on the engine executing caller blocks on the same +/// thread that created the adapter, which matches the runtime's behaviour. #[derive(Debug)] struct CallerAdapter { caller: Value, From 8d39b4cd55887242ec5c164e43f0d4af72bdfa06 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 16 Oct 2025 19:07:42 +0100 Subject: [PATCH 18/18] Forward manifest macro kwargs --- docs/roadmap.md | 7 +++++ src/manifest.rs | 66 ++++++++++++++++++++++++++++++++++++++----- src/manifest/tests.rs | 11 ++++++++ 3 files changed, 77 insertions(+), 7 deletions(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index d419511d..9efa60c4 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -117,6 +117,13 @@ configurations with variables, control flow, and custom functions. custom macros, and the `glob()` function to discover and operate on source files. +- [ ] **YAML Parser Migration:** + + - [ ] Draft an ADR evaluating maintained replacements for `serde_yml` + (for example `serde_yaml_ng`) and record the migration decision. + - [ ] Run a migration spike with the preferred crate, exercising the manifest + fixtures to capture compatibility notes and required mitigations. + ## Phase 3: The "Friendly" Polish 🛡️ Objective: To implement the advanced features that deliver a superior, secure, diff --git a/src/manifest.rs b/src/manifest.rs index 7f802f1f..192509c6 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -192,6 +192,48 @@ fn register_manifest_macros(doc: &YamlValue, env: &mut Environment) -> Result<() Ok(()) } +/// Invoke a `MiniJinja` value with optional keyword arguments. +/// +/// `MiniJinja` encodes keyword arguments by appending a [`Kwargs`] value to the +/// positional slice. This helper hides that convention so callers can pass the +/// keyword collection explicitly. +/// +/// # Examples +/// +/// ```rust,ignore +/// use minijinja::{Environment, value::{Kwargs, Value}}; +/// use netsuke::manifest::call_macro_value; +/// +/// let mut env = Environment::new(); +/// env.add_template( +/// "macro", +/// "{% macro greet(name='friend') %}hi {{ name }}{% endmacro %}", +/// ) +/// .unwrap(); +/// let template = env.get_template("macro").unwrap(); +/// let state = template.eval_to_state(()).unwrap(); +/// let value = state.lookup("greet").unwrap(); +/// let kwargs = Kwargs::from_iter([(String::from("name"), Value::from("Ada"))]); +/// let rendered = call_macro_value(&state, &value, &[], Some(kwargs)).unwrap(); +/// assert_eq!(rendered.to_string(), "hi Ada"); +/// ``` +fn call_macro_value( + state: &State, + macro_value: &Value, + positional: &[Value], + kwargs: Option, +) -> Result { + kwargs.map_or_else( + || macro_value.call(state, positional), + |kwargs| { + let mut call_args = Vec::with_capacity(positional.len() + 1); + call_args.extend_from_slice(positional); + call_args.push(Value::from(kwargs)); + macro_value.call(state, call_args.as_slice()) + }, + ) +} + /// Create a wrapper that invokes a compiled manifest macro on demand. /// /// The returned closure fetches the internal template, resolves the macro, and @@ -235,8 +277,16 @@ fn make_macro_fn( move |state, Rest(args), kwargs| { let template = state.env().get_template(&template_name)?; let macro_state = template.eval_to_state(())?; - let mut call_args = Vec::with_capacity(args.len() + 1); - call_args.extend_from_slice(&args); + let macro_value = macro_state.lookup(¯o_name).ok_or_else(|| { + Error::new( + ErrorKind::InvalidOperation, + format!("macro '{macro_name}' not defined in '{template_name}'"), + ) + })?; + + // MiniJinja requires keyword arguments to be appended as a trailing + // `Kwargs` value within the positional slice. Build that value lazily so + // we avoid allocating when no keywords were supplied. let mut entries: Vec<(String, Value)> = Vec::new(); for key in kwargs.args() { let mut value = kwargs.peek::(key)?; @@ -245,12 +295,14 @@ fn make_macro_fn( } entries.push((key.to_string(), value)); } - if !entries.is_empty() { - call_args.push(Value::from(Kwargs::from_iter(entries))); - } - - let rendered = macro_state.call_macro(¯o_name, call_args.as_slice())?; + let maybe_kwargs = if entries.is_empty() { + None + } else { + Some(entries.into_iter().collect::()) + }; + let rendered_value = call_macro_value(¯o_state, ¯o_value, &args, maybe_kwargs)?; + let rendered: String = rendered_value.into(); let value = if matches!(state.auto_escape(), AutoEscape::None) { Value::from(rendered) } else { diff --git a/src/manifest/tests.rs b/src/manifest/tests.rs index b57d99cf..546447a9 100644 --- a/src/manifest/tests.rs +++ b/src/manifest/tests.rs @@ -68,6 +68,17 @@ fn register_macro_handles_arguments( assert_eq!(rendered, expected); } +#[rstest] +fn register_macro_supports_keyword_invocation(mut strict_env: Environment) { + let macro_def = MacroDefinition { + signature: "salute(name='friend')".to_string(), + body: "Hello {{ name }}".to_string(), + }; + register_macro(&mut strict_env, ¯o_def, 0).expect("register"); + let rendered = render_with(&strict_env, "{{ salute(name='Ada') }}").expect("render"); + assert_eq!(rendered.trim(), "Hello Ada"); +} + #[rstest] fn register_macro_forwards_caller(mut strict_env: Environment) { let macro_def = MacroDefinition {