Refactor Jinja macro helpers into dedicated module#212
Conversation
Reviewer's GuideThis refactor moves the manifest-specific Jinja macro helpers out of the main manifest module into a dedicated Class diagram for the new jinja_macros moduleclassDiagram
class CallerAdapter {
+Value caller
+*const State<'static, 'static> state
+new(state: &State, caller: Value)
}
CallerAdapter --|> Object
class MacroDefinition
class Environment
class State
class Value
class Kwargs
class Error
class ErrorKind
class jinja_macros {
+parse_macro_name(signature: &str) Result<String>
+register_macro(env: &mut Environment, macro_def: &MacroDefinition, index: usize) Result<()>
+register_manifest_macros(doc: &YamlValue, env: &mut Environment) Result<()>
+call_macro_value(state: &State, macro_value: &Value, positional: &[Value], kwargs: Option<Kwargs>) Result<Value, Error>
+make_macro_fn(template_name: String, macro_name: String) Fn(&State, Rest<Value>, Kwargs) -> Result<Value, Error>
}
jinja_macros ..> MacroDefinition
jinja_macros ..> Environment
jinja_macros ..> State
jinja_macros ..> Value
jinja_macros ..> Kwargs
jinja_macros ..> Error
jinja_macros ..> ErrorKind
jinja_macros ..> CallerAdapter
Class diagram for manifest module after refactorclassDiagram
class manifest {
<<module>>
+uses jinja_macros for macro helpers
}
manifest ..> jinja_macros
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbitRelease Notes
WalkthroughExtract Jinja macro handling from Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Manifest loader
participant Doc as Manifest YAML
participant Env as Minijinja Environment
participant Cache as MacroCache / Templates
participant Caller as Caller code
Note over Loader,Doc: Registration phase
Loader->>Doc: read `macros` sequence
Loader->>Env: register_manifest_macros(doc, env)
Env->>Cache: compile macro body -> register template "__manifest_macro_{i}_{name}"
Env-->>Loader: expose callable fn for macro name
Note over Caller,Env: Invocation phase
Caller->>Env: call macro function(name)(positional, kwargs)
Env->>Cache: locate MacroInstance (state + compiled macro)
Env->>Cache: if caller-block -> use CallerAdapter to attach outer state
Cache->>Env: execute macro with State -> return Value/String
Env-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/manifest/jinja_macros.rs:256` </location>
<code_context>
-
-impl CallerAdapter {
- fn new(state: &State, caller: Value) -> Self {
- let ptr = ptr::from_ref(state).cast::<State<'static, 'static>>();
- Self { caller, state: ptr }
- }
</code_context>
<issue_to_address>
**issue (bug_risk):** Pointer casting in `CallerAdapter::new` is potentially unsafe.
This cast depends on lifetime assumptions that may not always hold. Explore safer alternatives or add runtime checks to detect incorrect usage.
</issue_to_address>
### Comment 2
<location> `src/manifest/jinja_macros.rs:251-252` </location>
<code_context>
- }
-}
-
-unsafe impl Send for CallerAdapter {}
-unsafe impl Sync for CallerAdapter {}
-
-impl Object for CallerAdapter {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Unsynchronized `Send`/`Sync` implementations may introduce subtle concurrency risks.
Consider adding module-level documentation outlining the thread safety risks and usage restrictions to help future maintainers avoid concurrency bugs.
```suggestion
/// # Thread Safety Notice
///
/// `CallerAdapter` is marked as `Send` and `Sync` via unsafe implementations.
/// This is necessary for integration with certain concurrency primitives, but
/// it is not inherently thread-safe. The underlying pointer to `State` is
/// shared across threads without synchronization, which may lead to subtle
/// concurrency bugs if the referenced `State` is mutated or not truly static.
///
/// ## Usage Restrictions
/// - Only use `CallerAdapter` in contexts where the referenced `State` is
/// guaranteed to be immutable or `'static`.
/// - Avoid mutating the underlying `State` from multiple threads.
/// - Future maintainers should carefully audit any changes to ensure thread
/// safety is preserved.
///
/// Failure to follow these guidelines may result in undefined behavior or
/// difficult-to-debug concurrency issues.
state: *const State<'static, 'static>,
}
```
</issue_to_address>
### Comment 3
<location> `src/manifest/jinja_macros.rs:265-268` </location>
<code_context>
-unsafe impl Sync for CallerAdapter {}
-
-impl Object for CallerAdapter {
- fn call(self: &Arc<Self>, _state: &State, args: &[Value]) -> Result<Value, Error> {
- let state = unsafe { &*self.state };
- self.caller.call(state, args)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unsafe dereference of raw pointer in `CallerAdapter::call`.
Consider adding runtime assertions or debug checks to help detect invalid pointer usage during development.
```suggestion
fn call(self: &Arc<Self>, _state: &State, args: &[Value]) -> Result<Value, Error> {
debug_assert!(!self.state.is_null(), "CallerAdapter: state pointer is null before dereference");
let state = unsafe { &*self.state };
self.caller.call(state, args)
}
```
</issue_to_address>
### Comment 4
<location> `src/manifest/jinja_macros.rs:186` </location>
<code_context>
-///
-/// The wrapper returns an error if the macro cannot be located or execution
-/// fails.
-fn make_macro_fn(
- template_name: String,
- macro_name: String,
</code_context>
<issue_to_address>
**issue (complexity):** Consider capturing the compiled macro value during registration to avoid repeated template lookups and state evaluations on each invocation.
```markdown
You can avoid the per-call `get_template` / `eval_to_state` / `lookup` dance by capturing the compiled `Template` (or even the `Value` for your macro) in the closure at registration time. This flattens out one level of indirection, removes the string lookup, and makes your closure much simpler:
1. In `register_macro`, grab and clone the `Template` and the macro `Value` once:
```rust
pub(crate) 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 tpl_source = format!("{{% macro {} %}}{}{{% endmacro %}}",
macro_def.signature, macro_def.body);
env.add_template_owned(template_name.clone(), tpl_source)
.with_context(|| format!("compile macro '{name}'"))?;
// fetch and clone the template handle
let tpl = env.get_template(&template_name)?
.clone();
// evaluate once to grab the macro Value
let init_state = tpl.eval_to_state(())?;
let macro_obj = init_state
.lookup(&name)
.ok_or_else(|| anyhow::anyhow!("macro '{name}' not found"))?
.clone();
env.add_function(name.clone(), make_macro_fn(macro_obj));
Ok(())
}
```
2. Change `make_macro_fn` to accept the pre-built `Value` and drop all of the get/eval/lookup internals:
```rust
fn make_macro_fn(
macro_obj: Value,
) -> impl Fn(&State, Rest<Value>, Kwargs) -> Result<Value, Error> + Clone {
move |state, Rest(args), kwargs| {
// rebuild caller‐safe kwargs as before…
let mut entries = Vec::new();
for key in kwargs.args() {
let mut val = kwargs.peek::<Value>(key)?;
if key == "caller" {
val = Value::from_object(CallerAdapter::new(state, val));
}
entries.push((key.to_string(), val));
}
let call_kwargs = if entries.is_empty() {
None
} else {
Some(entries.into_iter().collect())
};
// now just call the pre‐built macro_obj
let rendered_val = call_macro_value(state, ¯o_obj, &args, call_kwargs)?;
let s: String = rendered_val.into();
let out = if matches!(state.auto_escape(), AutoEscape::None) {
Value::from(s)
} else {
Value::from_safe_string(s)
};
Ok(out)
}
}
```
This removes the extra template lookup and state‐eval on each invocation, flattens the nesting, and keeps all functionality intact.
</issue_to_address>
### Comment 5
<location> `src/manifest/jinja_macros.rs:133` </location>
<code_context>
-/// 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,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new macro registration and invocation helpers.
The new module `jinja_macros.rs` introduces several functions for macro registration and invocation, but there are no corresponding behavioural or unit tests added in this pull request. Ensure that both types of tests are present to verify correct behaviour and catch regressions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/manifest.rs(3 hunks)src/manifest/jinja_macros.rs(1 hunks)src/manifest/tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every Rust module must begin with a module-level comment using //! explaining the module's purpose and utility
Document public APIs with Rustdoc comments (///) so documentation can be generated with cargo doc
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be denied; do not silence lints except as a last resort
Lint suppressions must be tightly scoped and include a clear reason; prefer #[expect(...)] over #[allow(...)]
Place function attributes after doc comments
Do not use return in single-line functions
Prefer single-line function definitions where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helpers adhering to separation of concerns and CQRS
Where a function has too many parameters, group related parameters into meaningfully named structs
When returning large error data, consider using Arc to reduce data moved
Prefer .expect() over .unwrap() on Results/Options
Use concat!() to combine long string literals instead of escaping newlines with backslashes
Use NewTypes to model domain values; prefer explicit tuple structs; leverage newt-hype and the-newtype crates as described; provide appropriate From/AsRef/TryFrom implementations and avoid orphan-rule-violating impls
Keep each Rust code file under 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Use functions and composition to avoid repetition; extract reusable logic; prefer declarative constructs (iterators/generators/comprehensions) when readable
Small, single-responsibility functions; obey command/query segregation
Name things precisely; booleans should use is/has/should prefixes
Comment why, not what; explain assumptions, e...
Files:
src/manifest/tests.rssrc/manifest/jinja_macros.rssrc/manifest.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/manifest/tests.rssrc/manifest/jinja_macros.rssrc/manifest.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use en-GB-oxendict spelling and grammar in comments and documentation (exceptions allowed for external API names)
Files:
src/manifest/tests.rssrc/manifest/jinja_macros.rssrc/manifest.rs
**/*{test,tests}.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write unit and behavioural tests for new functionality and run them before and after changes
Files:
src/manifest/tests.rs
🧬 Code graph analysis (2)
src/manifest/tests.rs (1)
src/manifest/jinja_macros.rs (3)
parse_macro_name(33-52)register_macro(64-81)register_manifest_macros(93-106)
src/manifest.rs (1)
src/manifest/jinja_macros.rs (1)
register_manifest_macros(93-106)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, arm64, macos-aarch64)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, x86_64, macos-x86_64)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64)
- GitHub Check: Sourcery review
- GitHub Check: netsukefile (stable)
- GitHub Check: netsukefile (1.89.0)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (10)
src/manifest.rs (4)
10-14: LGTM! Clean import simplification.The refactored imports correctly reflect the reduced responsibility of this module. Removing
Arc,ptr, and the unused MiniJinja components (AutoEscape,State,Kwargs,Object,Rest) keeps the surface minimal.
65-65: LGTM! Module organization follows Rust conventions.Declaring
jinja_macrosas a submodule correctly encapsulates the macro handling logic.
74-74: LGTM! Appropriate delegation to new module.Importing
register_manifest_macrosfrom the new module maintains a clean API boundary whilst hiding implementation details.
138-138: LGTM! Macro registration preserved.The call to
register_manifest_macrosmaintains the existing behaviour whilst delegating to the new module.src/manifest/jinja_macros.rs (5)
1-7: LGTM! Clear module documentation.The module-level doc comment concisely explains the purpose and utility of the macro integration helpers.
Based on coding guidelines.
33-52: LGTM! Robust signature parsing.The function correctly validates signature shape and extracts the macro identifier. Error messages are clear and actionable.
64-81: LGTM! Macro registration logic is sound.The function compiles the macro body into a uniquely named template and registers a callable wrapper. Error context is appropriate.
133-148: LGTM! Kwargs handling is correct.The function correctly appends
Kwargsas a trailing positional value when present, matching MiniJinja's calling convention.
228-269: Unsafe code is justified; MiniJinja execution model confirms safety assumptions.Verification confirms CallerAdapter's safety holds under MiniJinja 2.12.0:
- Synchronous execution: MiniJinja guarantees macro/caller-block invocation runs synchronously as part of template rendering; no async suspension occurs.
- State lifetime: The outer
Statereference (captured at line 207) outlives everyCallerAdapterinvocation because caller blocks execute immediately within the same render context.- Single-threaded: MiniJinja does not perform cross-thread migration during render; each rendering executes on one thread.
The pattern mirrors MiniJinja's built-in macro objects. Safety documentation (lines 236-247) accurately reflects the runtime's behaviour.
src/manifest/tests.rs (1)
3-5: LGTM! Test imports updated correctly.The imports correctly reference the new
jinja_macrosmodule and pullMacroDefinitionfromcrate::ast. Tests exercise the refactored helpers without requiring changes to test logic.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? |
|
@coderabbitai please create a new GitHub issue for the following: Since each macro invocation builds a fresh State and re‐evaluates the template, you may want to benchmark hot code paths and consider caching the compiled state if performance becomes an issue. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 44 @leynos: I've created issue #213 for the performance optimisation suggestion regarding macro state caching and benchmarking: #213 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8213 🏁 Script executed: Length of output: 1398 🏁 Script executed: Length of output: 2326 @leynos: Most of the comments have been addressed, but four items remain unresolved: ✅ Resolved (9 items)
❌ Not Resolved (4 items)
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Tests for manifest parsing and macro registration helpers.
use super::jinja_macros::{❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7094 @leynos: The duplication stems from both tests following an identical four-step pattern. The cleanest fix is to consolidate them into the existing parameterised test Suggested fixAdd the two test cases to the existing parameterised test: #[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!"
)]
#[case("salute(name='friend')", "Hello {{ name }}", "{{ salute(name='Ada') }}", "Hello Ada")]
#[case("wrap(prefix, caller)", "{{ prefix }}{{ caller() }}", "{% call wrap('Hi ') %}World{% endcall %}", "Hi World")]
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.trim(), expected);
}Then delete the two standalone test functions AI agent prompt |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Has the following now been resolved? Comment on file //! Tests for manifest parsing and macro registration helpers.
use super::jinja_macros::{❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 253 @leynos: Yes, the code duplication issue has been resolved ✅ The script confirms:
The refactoring eliminates the structural duplication whilst preserving complete test coverage for keyword invocation and caller forwarding. 🐰 |
|
@coderabbitai pause |
leynos
force-pushed
the
codex/refactor-macro-code-into-jinja_macros-module
branch
from
8433cf2 to
018b676
Compare
✅ Actions performedReviews paused. |
1 participant
Summary
manifest/jinja_macros.rsmanifest.rsto consume the new module while keeping the parsing API unchangedTesting
https://chatgpt.com/codex/tasks/task_e_68f143fa8cc88322ad09678b2727e6ef
Summary by Sourcery
Extract Jinja macro helper functions from manifest.rs into a dedicated jinja_macros.rs module and update imports to maintain the existing manifest API
Enhancements:
Tests: