From 9c45a2275b51ac5500d0f188c103e9db8e601767 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 10 Nov 2024 13:16:48 +0100 Subject: [PATCH 1/6] Added argument splatting support --- minijinja/src/compiler/ast.rs | 43 ++--- minijinja/src/compiler/codegen.rs | 159 +++++++++++------- minijinja/src/compiler/instructions.rs | 6 + minijinja/src/compiler/meta.rs | 29 +++- minijinja/src/compiler/parser.rs | 66 +++++--- minijinja/src/vm/context.rs | 5 + minijinja/src/vm/mod.rs | 61 ++++++- minijinja/tests/inputs/calls-and-literals.txt | 15 ++ .../test_parser__parser@call.txt.snap | 108 ++++++------ .../test_parser__parser@filter.txt.snap | 96 ++++++----- .../test_parser__parser@filter_block.txt.snap | 16 +- ..._templates__vm@calls-and-literals.txt.snap | 18 ++ .../test_templates__vm@debug.txt.snap | 1 + minijinja/tests/test_templates.rs | 5 +- 14 files changed, 396 insertions(+), 232 deletions(-) create mode 100644 minijinja/tests/inputs/calls-and-literals.txt create mode 100644 minijinja/tests/snapshots/test_templates__vm@calls-and-literals.txt.snap diff --git a/minijinja/src/compiler/ast.rs b/minijinja/src/compiler/ast.rs index dd4b58fe..8f78ce25 100644 --- a/minijinja/src/compiler/ast.rs +++ b/minijinja/src/compiler/ast.rs @@ -145,7 +145,6 @@ pub enum Expr<'a> { Call(Spanned>), List(Spanned>), Map(Spanned>), - Kwargs(Spanned>), } #[cfg(feature = "internal_debug")] @@ -165,7 +164,6 @@ impl<'a> fmt::Debug for Expr<'a> { Expr::Call(s) => fmt::Debug::fmt(s, f), Expr::List(s) => fmt::Debug::fmt(s, f), Expr::Map(s) => fmt::Debug::fmt(s, f), - Expr::Kwargs(s) => fmt::Debug::fmt(s, f), } } } @@ -186,7 +184,6 @@ impl<'a> Expr<'a> { Expr::Map(_) => "map literal", Expr::Test(_) => "test expression", Expr::Filter(_) => "filter expression", - Expr::Kwargs(_) => "keyword arguments", } } } @@ -444,7 +441,7 @@ pub struct IfExpr<'a> { pub struct Filter<'a> { pub name: &'a str, pub expr: Option>, - pub args: Vec>, + pub args: Vec>, } /// A test expression. @@ -453,7 +450,7 @@ pub struct Filter<'a> { pub struct Test<'a> { pub name: &'a str, pub expr: Expr<'a>, - pub args: Vec>, + pub args: Vec>, } /// An attribute lookup expression. @@ -477,7 +474,17 @@ pub struct GetItem<'a> { #[cfg_attr(feature = "unstable_machinery_serde", derive(serde::Serialize))] pub struct Call<'a> { pub expr: Expr<'a>, - pub args: Vec>, + pub args: Vec>, +} + +/// A call argument helper +#[cfg_attr(feature = "internal_debug", derive(Debug))] +#[cfg_attr(feature = "unstable_machinery_serde", derive(serde::Serialize))] +pub enum CallArg<'a> { + Pos(Expr<'a>), + Kwarg(&'a str, Expr<'a>), + Splat(Expr<'a>), + KwargsSplat(Expr<'a>), } /// Creates a list of values. @@ -503,30 +510,6 @@ impl<'a> List<'a> { } } -/// Creates a map of kwargs -#[cfg_attr(feature = "internal_debug", derive(Debug))] -#[cfg_attr(feature = "unstable_machinery_serde", derive(serde::Serialize))] -pub struct Kwargs<'a> { - pub pairs: Vec<(&'a str, Expr<'a>)>, -} - -impl<'a> Kwargs<'a> { - pub fn as_const(&self) -> Option { - if !self.pairs.iter().all(|x| matches!(x.1, Expr::Const(_))) { - return None; - } - - let mut rv = value_map_with_capacity(self.pairs.len()); - for (key, value) in &self.pairs { - if let Expr::Const(value) = value { - rv.insert(Value::from(*key), value.value.clone()); - } - } - - Some(crate::value::Kwargs::wrap(rv)) - } -} - /// Creates a map of values. #[cfg_attr(feature = "internal_debug", derive(Debug))] #[cfg_attr(feature = "unstable_machinery_serde", derive(serde::Serialize))] diff --git a/minijinja/src/compiler/codegen.rs b/minijinja/src/compiler/codegen.rs index f5550d97..5338507b 100644 --- a/minijinja/src/compiler/codegen.rs +++ b/minijinja/src/compiler/codegen.rs @@ -7,7 +7,7 @@ use crate::compiler::instructions::{ use crate::compiler::tokens::Span; use crate::output::CaptureMode; use crate::value::ops::neg; -use crate::value::Value; +use crate::value::{Kwargs, Value, ValueMap}; #[cfg(test)] use similar_asserts::assert_eq; @@ -503,7 +503,7 @@ impl<'source> CodeGenerator<'source> { self.add_with_span(Instruction::FastSuper, call.span()); return; } else if name == "loop" && call.args.len() == 1 { - self.compile_expr(&call.args[0]); + self.compile_call_args(std::slice::from_ref(&call.args[0]), None); self.add(Instruction::FastRecurse); return; } @@ -660,21 +660,17 @@ impl<'source> CodeGenerator<'source> { if let Some(ref expr) = f.expr { self.compile_expr(expr); } - for arg in &f.args { - self.compile_expr(arg); - } + let arg_count = self.compile_call_args(&f.args, None); let local_id = get_local_id(&mut self.filter_local_ids, f.name); - self.add(Instruction::ApplyFilter(f.name, f.args.len() + 1, local_id)); + self.add(Instruction::ApplyFilter(f.name, arg_count + 1, local_id)); self.pop_span(); } ast::Expr::Test(f) => { self.push_span(f.span()); self.compile_expr(&f.expr); - for arg in &f.args { - self.compile_expr(arg); - } + let arg_count = self.compile_call_args(&f.args, None); let local_id = get_local_id(&mut self.test_local_ids, f.name); - self.add(Instruction::PerformTest(f.name, f.args.len() + 1, local_id)); + self.add(Instruction::PerformTest(f.name, arg_count + 1, local_id)); self.pop_span(); } ast::Expr::GetAttr(g) => { @@ -717,18 +713,6 @@ impl<'source> CodeGenerator<'source> { self.add(Instruction::BuildMap(m.keys.len())); } } - ast::Expr::Kwargs(m) => { - if let Some(val) = m.as_const() { - self.add(Instruction::LoadConst(val)); - } else { - self.set_line_from_span(m.span()); - for (key, value) in &m.pairs { - self.add(Instruction::LoadConst(Value::from(*key))); - self.compile_expr(value); - } - self.add(Instruction::BuildKwargs(m.pairs.len())); - } - } } } @@ -765,57 +749,110 @@ impl<'source> CodeGenerator<'source> { fn compile_call_args( &mut self, - args: &[ast::Expr<'source>], + args: &[ast::CallArg<'source>], caller: Option<&Caller<'source>>, ) -> usize { - match caller { - // we can conditionally compile the caller part here since this will - // nicely call through for non macro builds - #[cfg(feature = "macros")] - Some(caller) => self.compile_call_args_with_caller(args, caller), - _ => { - for arg in args { - self.compile_expr(arg); + let mut pending_args = 0; + let mut num_args_batches = 0; + let mut has_kwargs = caller.is_some(); + let mut static_kwargs = caller.is_none(); + + for arg in args { + match arg { + ast::CallArg::Pos(expr) => { + self.compile_expr(expr); + pending_args += 1; + } + ast::CallArg::Splat(expr) => { + if pending_args > 0 { + self.add(Instruction::BuildList(Some(pending_args))); + pending_args = 0; + num_args_batches += 1; + } + self.compile_expr(expr); + num_args_batches += 1; + } + ast::CallArg::Kwarg(_, expr) => { + if !matches!(expr, ast::Expr::Const(_)) { + static_kwargs = false; + } + has_kwargs = true; + } + ast::CallArg::KwargsSplat(_) => { + static_kwargs = false; + has_kwargs = true; } - args.len() } } - } - #[cfg(feature = "macros")] - fn compile_call_args_with_caller( - &mut self, - args: &[ast::Expr<'source>], - caller: &Caller<'source>, - ) -> usize { - let mut injected_caller = false; + if has_kwargs { + let mut pending_kwargs = 0; + let mut num_kwargs_batches = 0; + let mut collected_kwargs = ValueMap::new(); + for arg in args { + match arg { + ast::CallArg::Kwarg(key, value) => { + if static_kwargs { + if let ast::Expr::Const(c) = value { + collected_kwargs.insert(Value::from(*key), c.value.clone()); + } else { + unreachable!(); + } + } else { + self.add(Instruction::LoadConst(Value::from(*key))); + self.compile_expr(value); + pending_kwargs += 1; + } + } + ast::CallArg::KwargsSplat(expr) => { + if pending_kwargs > 0 { + self.add(Instruction::BuildKwargs(pending_kwargs)); + num_kwargs_batches += 1; + pending_kwargs = 0; + } + self.compile_expr(expr); + num_kwargs_batches += 1; + } + ast::CallArg::Pos(_) | ast::CallArg::Splat(_) => {} + } + } - // try to add the caller to already existing keyword arguments. - for arg in args { - if let ast::Expr::Kwargs(ref m) = arg { - self.set_line_from_span(m.span()); - for (key, value) in &m.pairs { - self.add(Instruction::LoadConst(Value::from(*key))); - self.compile_expr(value); - } - self.add(Instruction::LoadConst(Value::from("caller"))); - self.compile_macro_expression(caller); - self.add(Instruction::BuildKwargs(m.pairs.len() + 1)); - injected_caller = true; + if !collected_kwargs.is_empty() { + self.add(Instruction::LoadConst(Kwargs::wrap(collected_kwargs))); } else { - self.compile_expr(arg); + // The conditions above guarantee that if we collect static kwargs + // we cannot enter this block (single kwargs batch, no caller). + + #[cfg(feature = "macros")] + { + if let Some(caller) = caller { + self.add(Instruction::LoadConst(Value::from("caller"))); + self.compile_macro_expression(caller); + pending_kwargs += 1 + } + } + if num_kwargs_batches > 0 { + if pending_kwargs > 0 { + self.add(Instruction::BuildKwargs(pending_kwargs)); + num_kwargs_batches += 1; + } + self.add(Instruction::MergeKwargs(num_kwargs_batches)); + } else { + self.add(Instruction::BuildKwargs(pending_kwargs)); + } } + pending_args += 1; } - // if there are no keyword args so far, create a new kwargs object - // and add caller to that. - if !injected_caller { - self.add(Instruction::LoadConst(Value::from("caller"))); - self.compile_macro_expression(caller); - self.add(Instruction::BuildKwargs(1)); - args.len() + 1 + if num_args_batches > 0 { + if pending_args > 0 { + self.add(Instruction::BuildList(Some(pending_args))); + num_args_batches += 1; + } + self.add(Instruction::UnpackLists(num_args_batches)); + !0 } else { - args.len() + pending_args } } diff --git a/minijinja/src/compiler/instructions.rs b/minijinja/src/compiler/instructions.rs index 56dc1a7a..6b7e280e 100644 --- a/minijinja/src/compiler/instructions.rs +++ b/minijinja/src/compiler/instructions.rs @@ -60,12 +60,18 @@ pub enum Instruction<'source> { /// Builds a kwargs map of the last n pairs on the stack. BuildKwargs(usize), + /// Merges N kwargs maps on the list into one. + MergeKwargs(usize), + /// Builds a list of the last n pairs on the stack. BuildList(Option), /// Unpacks a list into N stack items. UnpackList(usize), + /// Unpacks N lists onto the stack and pushes the number of items there were unpacked. + UnpackLists(usize), + /// Add the top two values Add, diff --git a/minijinja/src/compiler/meta.rs b/minijinja/src/compiler/meta.rs index 2f5915aa..e10c6ee0 100644 --- a/minijinja/src/compiler/meta.rs +++ b/minijinja/src/compiler/meta.rs @@ -81,6 +81,15 @@ fn tracker_visit_macro<'a>(m: &ast::Macro<'a>, state: &mut AssignmentTracker<'a> m.body.iter().for_each(|node| track_walk(node, state)); } +fn tracker_visit_callarg<'a>(callarg: &ast::CallArg<'a>, state: &mut AssignmentTracker<'a>) { + match callarg { + ast::CallArg::Pos(expr) + | ast::CallArg::Kwarg(_, expr) + | ast::CallArg::Splat(expr) + | ast::CallArg::KwargsSplat(expr) => tracker_visit_expr(expr, state), + } +} + fn tracker_visit_expr<'a>(expr: &ast::Expr<'a>, state: &mut AssignmentTracker<'a>) { match expr { ast::Expr::Var(var) => { @@ -108,11 +117,15 @@ fn tracker_visit_expr<'a>(expr: &ast::Expr<'a>, state: &mut AssignmentTracker<'a } ast::Expr::Filter(expr) => { tracker_visit_expr_opt(&expr.expr, state); - expr.args.iter().for_each(|x| tracker_visit_expr(x, state)); + expr.args + .iter() + .for_each(|x| tracker_visit_callarg(x, state)); } ast::Expr::Test(expr) => { tracker_visit_expr(&expr.expr, state); - expr.args.iter().for_each(|x| tracker_visit_expr(x, state)); + expr.args + .iter() + .for_each(|x| tracker_visit_callarg(x, state)); } ast::Expr::GetAttr(expr) => { // if we are tracking nested, we check if we have a chain of attribute @@ -157,17 +170,15 @@ fn tracker_visit_expr<'a>(expr: &ast::Expr<'a>, state: &mut AssignmentTracker<'a } ast::Expr::Call(expr) => { tracker_visit_expr(&expr.expr, state); - expr.args.iter().for_each(|x| tracker_visit_expr(x, state)); + expr.args + .iter() + .for_each(|x| tracker_visit_callarg(x, state)); } ast::Expr::List(expr) => expr.items.iter().for_each(|x| tracker_visit_expr(x, state)), ast::Expr::Map(expr) => expr.keys.iter().zip(expr.values.iter()).for_each(|(k, v)| { tracker_visit_expr(k, state); tracker_visit_expr(v, state); }), - ast::Expr::Kwargs(expr) => expr - .pairs - .iter() - .for_each(|(_, v)| tracker_visit_expr(v, state)), } } @@ -265,7 +276,7 @@ fn track_walk<'a>(node: &ast::Stmt<'a>, state: &mut AssignmentTracker<'a>) { stmt.call .args .iter() - .for_each(|x| tracker_visit_expr(x, state)); + .for_each(|x| tracker_visit_callarg(x, state)); tracker_visit_macro(&stmt.macro_decl, state); } #[cfg(feature = "loop_controls")] @@ -275,7 +286,7 @@ fn track_walk<'a>(node: &ast::Stmt<'a>, state: &mut AssignmentTracker<'a>) { stmt.call .args .iter() - .for_each(|x| tracker_visit_expr(x, state)); + .for_each(|x| tracker_visit_callarg(x, state)); } } } diff --git a/minijinja/src/compiler/parser.rs b/minijinja/src/compiler/parser.rs index ae9a06e0..efcd055b 100644 --- a/minijinja/src/compiler/parser.rs +++ b/minijinja/src/compiler/parser.rs @@ -523,7 +523,7 @@ impl<'a> Parser<'a> { let span = self.stream.current_span(); let mut expr = ok!(self.parse_unary_only()); expr = ok!(self.parse_postfix(expr, span)); - vec![expr] + vec![ast::CallArg::Pos(expr)] } else { Vec::new() }; @@ -547,50 +547,70 @@ impl<'a> Parser<'a> { Ok(expr) } - fn parse_args(&mut self) -> Result>, Error> { + fn parse_args(&mut self) -> Result>, Error> { let mut args = Vec::new(); let mut first_span = None; - let mut kwargs = Vec::new(); + let mut has_kwargs = false; + + enum ArgType { + Regular, + Splat, + KwargsSplat, + } expect_token!(self, Token::ParenOpen, "`(`"); loop { if skip_token!(self, Token::ParenClose) { break; } - if !args.is_empty() || !kwargs.is_empty() { + if !args.is_empty() || has_kwargs { expect_token!(self, Token::Comma, "`,`"); if skip_token!(self, Token::ParenClose) { break; } } + + let arg_type = if skip_token!(self, Token::Pow) { + ArgType::KwargsSplat + } else if skip_token!(self, Token::Mul) { + ArgType::Splat + } else { + ArgType::Regular + }; + let expr = ok!(self.parse_expr()); - // keyword argument - match expr { - ast::Expr::Var(ref var) if skip_token!(self, Token::Assign) => { - if first_span.is_none() { - first_span = Some(var.span()); + match arg_type { + ArgType::Regular => { + // keyword argument + match expr { + ast::Expr::Var(ref var) if skip_token!(self, Token::Assign) => { + if first_span.is_none() { + first_span = Some(var.span()); + } + has_kwargs = true; + args.push(ast::CallArg::Kwarg(var.id, ok!(self.parse_expr_noif()))); + } + _ if has_kwargs => { + return Err(syntax_error(Cow::Borrowed( + "non-keyword arg after keyword arg", + ))); + } + _ => { + args.push(ast::CallArg::Pos(expr)); + } } - kwargs.push((var.id, ok!(self.parse_expr_noif()))); } - _ if !kwargs.is_empty() => { - return Err(syntax_error(Cow::Borrowed( - "non-keyword arg after keyword arg", - ))); + ArgType::Splat => { + args.push(ast::CallArg::Splat(expr)); } - _ => { - args.push(expr); + ArgType::KwargsSplat => { + args.push(ast::CallArg::KwargsSplat(expr)); + has_kwargs = true; } } } - if !kwargs.is_empty() { - args.push(ast::Expr::Kwargs(ast::Spanned::new( - ast::Kwargs { pairs: kwargs }, - self.stream.expand_span(first_span.unwrap()), - ))); - }; - Ok(args) } diff --git a/minijinja/src/vm/context.rs b/minijinja/src/vm/context.rs index 5902b75b..5b9009f8 100644 --- a/minijinja/src/vm/context.rs +++ b/minijinja/src/vm/context.rs @@ -110,6 +110,11 @@ impl Stack { } pub fn slice_top(&mut self, n: usize) -> &[Value] { + let n = if n == !0 { + self.pop().as_usize().unwrap() + } else { + n + }; &self.values[self.values.len() - n..] } diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index d6776e07..4876e0d7 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -11,7 +11,7 @@ use crate::error::{Error, ErrorKind}; use crate::output::{CaptureMode, Output}; use crate::utils::{untrusted_size_hint, AutoEscape, UndefinedBehavior}; use crate::value::namespace_object::Namespace; -use crate::value::{ops, value_map_with_capacity, value_optimization, Kwargs, Value}; +use crate::value::{ops, value_map_with_capacity, value_optimization, Kwargs, Value, ValueMap}; use crate::vm::context::{Frame, LoopState, Stack}; use crate::vm::loop_object::Loop; use crate::vm::state::BlockStack; @@ -367,22 +367,48 @@ impl<'env> Vm<'env> { } Instruction::BuildMap(pair_count) => { let mut map = value_map_with_capacity(*pair_count); + stack.reverse_top(*pair_count * 2); for _ in 0..*pair_count { - let value = stack.pop(); let key = stack.pop(); + let value = stack.pop(); map.insert(key, value); } stack.push(Value::from_object(map)) } Instruction::BuildKwargs(pair_count) => { let mut map = value_map_with_capacity(*pair_count); + stack.reverse_top(*pair_count * 2); for _ in 0..*pair_count { - let value = stack.pop(); let key = stack.pop(); + let value = stack.pop(); map.insert(key, value); } stack.push(Kwargs::wrap(map)) } + Instruction::MergeKwargs(count) => { + let mut kwargs_sources = Vec::new(); + for _ in 0..*count { + kwargs_sources.push(stack.pop()); + } + kwargs_sources.reverse(); + let values: &[Value] = &kwargs_sources; + let mut rv = ValueMap::new(); + for value in values { + let iter = ctx_ok!(value + .as_object() + .and_then(|x| x.try_iter_pairs()) + .ok_or_else(|| { + Error::new( + ErrorKind::InvalidOperation, + "attempted to construct keyword arguments from non object", + ) + })); + for (key, value) in iter { + rv.insert(key, value); + } + } + stack.push(Kwargs::wrap(rv)); + } Instruction::BuildList(n) => { let count = n.unwrap_or_else(|| stack.pop().try_into().unwrap()); let mut v = Vec::with_capacity(untrusted_size_hint(count)); @@ -395,6 +421,20 @@ impl<'env> Vm<'env> { Instruction::UnpackList(count) => { ctx_ok!(self.unpack_list(&mut stack, *count)); } + Instruction::UnpackLists(count) => { + let mut lists = Vec::new(); + for _ in 0..*count { + lists.push(stack.pop()); + } + let mut len = 0; + for list in lists.into_iter().rev() { + for item in ctx_ok!(list.try_iter()) { + stack.push(item); + len += 1; + } + } + stack.push(Value::from(len)); + } Instruction::Add => func_binop!(add), Instruction::Sub => func_binop!(sub), Instruction::Mul => func_binop!(mul), @@ -542,8 +582,9 @@ impl<'env> Vm<'env> { ) })); let args = stack.slice_top(*arg_count); + let arg_count = args.len(); a = ctx_ok!(filter.apply_to(state, args)); - stack.drop_top(*arg_count); + stack.drop_top(arg_count); stack.push(a); } Instruction::PerformTest(name, arg_count, local_id) => { @@ -554,8 +595,9 @@ impl<'env> Vm<'env> { Error::new(ErrorKind::UnknownTest, format!("test {name} is unknown")) })); let args = stack.slice_top(*arg_count); + let arg_count = args.len(); let rv = ctx_ok!(test.perform(state, args)); - stack.drop_top(*arg_count); + stack.drop_top(arg_count); stack.push(Value::from(rv)); } Instruction::CallFunction(name, arg_count) => { @@ -580,8 +622,9 @@ impl<'env> Vm<'env> { recurse_loop!(true); } else if let Some(func) = state.lookup(name) { let args = stack.slice_top(*arg_count); + let arg_count = args.len(); a = ctx_ok!(func.call(state, args)); - stack.drop_top(*arg_count); + stack.drop_top(arg_count); stack.push(a); } else { bail!(Error::new( @@ -592,14 +635,16 @@ impl<'env> Vm<'env> { } Instruction::CallMethod(name, arg_count) => { let args = stack.slice_top(*arg_count); + let arg_count = args.len(); a = ctx_ok!(args[0].call_method(state, name, &args[1..])); - stack.drop_top(*arg_count); + stack.drop_top(arg_count); stack.push(a); } Instruction::CallObject(arg_count) => { let args = stack.slice_top(*arg_count); + let arg_count = args.len(); a = ctx_ok!(args[0].call(state, &args[1..])); - stack.drop_top(*arg_count); + stack.drop_top(arg_count); stack.push(a); } Instruction::DupTop => { diff --git a/minijinja/tests/inputs/calls-and-literals.txt b/minijinja/tests/inputs/calls-and-literals.txt new file mode 100644 index 00000000..354628bd --- /dev/null +++ b/minijinja/tests/inputs/calls-and-literals.txt @@ -0,0 +1,15 @@ +{ + "one": 1, + "two": 2, + "nine": 9 +} +--- +{{ get_args(1, 2, 3, 4, 5, 6, 7, 8, 9) }} +{{ get_args(1, 2, 3, *[4, 5, 6], 7, 8, 9) }} +{{ get_args(one, 2, 3, 4, 5, 6, 7, 8, 9) }} +{{ get_args(1, 2, 3, *[4, 5, 6], 7, 8, nine) }} +{{ get_args(1, 2, one=1, two=two) }} +{{ get_args(1, 2, one=1, **dict(two=two)) }} +{{ get_args(1, *[2], one=1, **dict(two=two)) }} +{{ get_args(1, 2, *[3], *[4], **{8: 8, 9: 9})}} +{{ get_args(1, 2, *[3], *[4], **{8: 8, nine: nine})}} \ No newline at end of file diff --git a/minijinja/tests/snapshots/test_parser__parser@call.txt.snap b/minijinja/tests/snapshots/test_parser__parser@call.txt.snap index 6b8429f7..9cbdd376 100644 --- a/minijinja/tests/snapshots/test_parser__parser@call.txt.snap +++ b/minijinja/tests/snapshots/test_parser__parser@call.txt.snap @@ -26,12 +26,16 @@ Ok( name: "cycle", } @ 2:3-2:13, args: [ - Const { - value: 1, - } @ 2:14-2:15, - Const { - value: 2, - } @ 2:17-2:18, + Pos( + Const { + value: 1, + } @ 2:14-2:15, + ), + Pos( + Const { + value: 2, + } @ 2:17-2:18, + ), ], } @ 2:7-2:19, } @ 2:0-2:19, @@ -58,28 +62,28 @@ Ok( id: "foo", } @ 4:3-4:6, args: [ - Const { - value: 1, - } @ 4:7-4:8, - Const { - value: 2, - } @ 4:10-4:11, - Kwargs { - pairs: [ - ( - "a", - Const { - value: 3, - } @ 4:15-4:16, - ), - ( - "b", - Const { - value: 4, - } @ 4:20-4:21, - ), - ], - } @ 4:13-4:22, + Pos( + Const { + value: 1, + } @ 4:7-4:8, + ), + Pos( + Const { + value: 2, + } @ 4:10-4:11, + ), + Kwarg( + "a", + Const { + value: 3, + } @ 4:15-4:16, + ), + Kwarg( + "b", + Const { + value: 4, + } @ 4:20-4:21, + ), ], } @ 4:3-4:22, } @ 4:0-4:22, @@ -92,12 +96,16 @@ Ok( id: "trailing", } @ 5:3-5:11, args: [ - Const { - value: 1, - } @ 5:12-5:13, - Const { - value: 2, - } @ 5:15-5:16, + Pos( + Const { + value: 1, + } @ 5:12-5:13, + ), + Pos( + Const { + value: 2, + } @ 5:15-5:16, + ), ], } @ 5:3-5:18, } @ 5:0-5:18, @@ -110,22 +118,22 @@ Ok( id: "trailing_kwarg", } @ 6:3-6:17, args: [ - Const { - value: 1, - } @ 6:18-6:19, - Const { - value: 2, - } @ 6:21-6:22, - Kwargs { - pairs: [ - ( - "a", - Const { - value: 3, - } @ 6:26-6:27, - ), - ], - } @ 6:24-6:29, + Pos( + Const { + value: 1, + } @ 6:18-6:19, + ), + Pos( + Const { + value: 2, + } @ 6:21-6:22, + ), + Kwarg( + "a", + Const { + value: 3, + } @ 6:26-6:27, + ), ], } @ 6:3-6:29, } @ 6:0-6:29, diff --git a/minijinja/tests/snapshots/test_parser__parser@filter.txt.snap b/minijinja/tests/snapshots/test_parser__parser@filter.txt.snap index d61a733f..57425ebb 100644 --- a/minijinja/tests/snapshots/test_parser__parser@filter.txt.snap +++ b/minijinja/tests/snapshots/test_parser__parser@filter.txt.snap @@ -18,12 +18,16 @@ Ok( } @ 1:3-1:6, ), args: [ - Const { - value: 1, - } @ 1:11-1:12, - Const { - value: 2, - } @ 1:14-1:15, + Pos( + Const { + value: 1, + } @ 1:11-1:12, + ), + Pos( + Const { + value: 2, + } @ 1:14-1:15, + ), ], } @ 1:7-1:16, ), @@ -42,12 +46,16 @@ Ok( } @ 2:3-2:6, ), args: [ - Const { - value: 1, - } @ 2:11-2:12, - Const { - value: 2, - } @ 2:14-2:15, + Pos( + Const { + value: 1, + } @ 2:11-2:12, + ), + Pos( + Const { + value: 2, + } @ 2:14-2:15, + ), ], } @ 2:7-2:17, } @ 2:0-2:17, @@ -63,22 +71,22 @@ Ok( } @ 3:3-3:6, ), args: [ - Const { - value: 1, - } @ 3:11-3:12, - Const { - value: 2, - } @ 3:14-3:15, - Kwargs { - pairs: [ - ( - "a", - Const { - value: 1, - } @ 3:19-3:20, - ), - ], - } @ 3:17-3:21, + Pos( + Const { + value: 1, + } @ 3:11-3:12, + ), + Pos( + Const { + value: 2, + } @ 3:14-3:15, + ), + Kwarg( + "a", + Const { + value: 1, + } @ 3:19-3:20, + ), ], } @ 3:7-3:21, } @ 3:0-3:21, @@ -94,22 +102,22 @@ Ok( } @ 4:3-4:6, ), args: [ - Const { - value: 1, - } @ 4:11-4:12, - Const { - value: 2, - } @ 4:14-4:15, - Kwargs { - pairs: [ - ( - "a", - Const { - value: 1, - } @ 4:19-4:20, - ), - ], - } @ 4:17-4:22, + Pos( + Const { + value: 1, + } @ 4:11-4:12, + ), + Pos( + Const { + value: 2, + } @ 4:14-4:15, + ), + Kwarg( + "a", + Const { + value: 1, + } @ 4:19-4:20, + ), ], } @ 4:7-4:22, } @ 4:0-4:22, diff --git a/minijinja/tests/snapshots/test_parser__parser@filter_block.txt.snap b/minijinja/tests/snapshots/test_parser__parser@filter_block.txt.snap index 293a4b27..ed3d557d 100644 --- a/minijinja/tests/snapshots/test_parser__parser@filter_block.txt.snap +++ b/minijinja/tests/snapshots/test_parser__parser@filter_block.txt.snap @@ -20,12 +20,16 @@ Ok( } @ 1:10-1:13, ), args: [ - Const { - value: 1, - } @ 1:18-1:19, - Const { - value: 2, - } @ 1:21-1:22, + Pos( + Const { + value: 1, + } @ 1:18-1:19, + ), + Pos( + Const { + value: 2, + } @ 1:21-1:22, + ), ], } @ 1:14-1:23, ), diff --git a/minijinja/tests/snapshots/test_templates__vm@calls-and-literals.txt.snap b/minijinja/tests/snapshots/test_templates__vm@calls-and-literals.txt.snap new file mode 100644 index 00000000..260995d1 --- /dev/null +++ b/minijinja/tests/snapshots/test_templates__vm@calls-and-literals.txt.snap @@ -0,0 +1,18 @@ +--- +source: minijinja/tests/test_templates.rs +description: "{{ get_args(1, 2, 3, 4, 5, 6, 7, 8, 9) }}\n{{ get_args(1, 2, 3, *[4, 5, 6], 7, 8, 9) }}\n{{ get_args(one, 2, 3, 4, 5, 6, 7, 8, 9) }}\n{{ get_args(1, 2, 3, *[4, 5, 6], 7, 8, nine) }}\n{{ get_args(1, 2, one=1, two=two) }}\n{{ get_args(1, 2, one=1, **dict(two=two)) }}\n{{ get_args(1, *[2], one=1, **dict(two=two)) }}\n{{ get_args(1, 2, *[3], *[4], **{8: 8, 9: 9})}}\n{{ get_args(1, 2, *[3], *[4], **{8: 8, nine: nine})}}" +info: + one: 1 + two: 2 + nine: 9 +input_file: minijinja/tests/inputs/calls-and-literals.txt +--- +[1, 2, 3, 4, 5, 6, 7, 8, 9] +[1, 2, 3, 4, 5, 6, 7, 8, 9] +[1, 2, 3, 4, 5, 6, 7, 8, 9] +[1, 2, 3, 4, 5, 6, 7, 8, 9] +[1, 2, {"one": 1, "two": 2}] +[1, 2, {"one": 1, "two": 2}] +[1, 2, {"one": 1, "two": 2}] +[1, 2, 3, 4, {8: 8, 9: 9}] +[1, 2, 3, 4, {8: 8, 9: 9}] diff --git a/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap b/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap index 007fdcb0..c5b7b045 100644 --- a/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap +++ b/minijinja/tests/snapshots/test_templates__vm@debug.txt.snap @@ -26,6 +26,7 @@ State { globals: { "debug": minijinja::functions::builtins::debug, "dict": minijinja::functions::builtins::dict, + "get_args": test_templates::test_vm::{{closure}}::{{closure}}, "namespace": minijinja::functions::builtins::namespace, "range": minijinja::functions::builtins::range, }, diff --git a/minijinja/tests/test_templates.rs b/minijinja/tests/test_templates.rs index f3fe5d71..22c7fdd4 100644 --- a/minijinja/tests/test_templates.rs +++ b/minijinja/tests/test_templates.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use std::{env, fs}; use insta::assert_snapshot; -use minijinja::value::{Enumerator, Object, ObjectRepr, Value}; +use minijinja::value::{Enumerator, Object, ObjectRepr, Rest, Value}; use minijinja::{context, render, Environment, Error, ErrorKind, State}; use similar_asserts::assert_eq; @@ -36,6 +36,9 @@ fn test_vm() { let contents = std::fs::read_to_string(path).unwrap(); let mut iter = contents.splitn(2, "\n---\n"); let mut env = Environment::new(); + env.add_function("get_args", |args: Rest| -> Value { + Value::from(args.0) + }); let ctx: Value = serde_json::from_str(iter.next().unwrap()).unwrap(); for (path, source) in &refs { From eab7520da4b7d9e39ec7496d1fa44631fb350d36 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 10 Nov 2024 13:23:27 +0100 Subject: [PATCH 2/6] Link changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e6f9317..6546004b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ All notable changes to MiniJinja are documented here. - `minijinja-cli` now does not convert INI files to lowercase anymore. This was an unintended behavior. #633 - Moved up MSRV to 1.63.0 due to indexmap. #635 +- Added argument splatting support (`*args` for variable args and `**kwargs` + for keyword arguments) and fixed a bug where sometimes maps and keyword + arguments were created in inverse order. #642 ## 2.4.0 From c2ec7f6feb8864e22022db0e9e41e844802346f2 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 10 Nov 2024 13:51:07 +0100 Subject: [PATCH 3/6] Added more tests and assert object on kwargs splat --- minijinja/src/vm/mod.rs | 11 ++++++++-- .../tests/inputs/err_bad_args_unpack.txt | 3 +++ .../tests/inputs/err_bad_kwargs_unpack.txt | 3 +++ ...templates__vm@err_bad_args_unpack.txt.snap | 22 +++++++++++++++++++ ...mplates__vm@err_bad_kwargs_unpack.txt.snap | 22 +++++++++++++++++++ 5 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 minijinja/tests/inputs/err_bad_args_unpack.txt create mode 100644 minijinja/tests/inputs/err_bad_kwargs_unpack.txt create mode 100644 minijinja/tests/snapshots/test_templates__vm@err_bad_args_unpack.txt.snap create mode 100644 minijinja/tests/snapshots/test_templates__vm@err_bad_kwargs_unpack.txt.snap diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index 4876e0d7..911b14ee 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -11,7 +11,9 @@ use crate::error::{Error, ErrorKind}; use crate::output::{CaptureMode, Output}; use crate::utils::{untrusted_size_hint, AutoEscape, UndefinedBehavior}; use crate::value::namespace_object::Namespace; -use crate::value::{ops, value_map_with_capacity, value_optimization, Kwargs, Value, ValueMap}; +use crate::value::{ + ops, value_map_with_capacity, value_optimization, Kwargs, ObjectRepr, Value, ValueMap, +}; use crate::vm::context::{Frame, LoopState, Stack}; use crate::vm::loop_object::Loop; use crate::vm::state::BlockStack; @@ -394,13 +396,18 @@ impl<'env> Vm<'env> { let values: &[Value] = &kwargs_sources; let mut rv = ValueMap::new(); for value in values { + ctx_ok!(self.env.undefined_behavior().assert_iterable(value)); let iter = ctx_ok!(value .as_object() + .filter(|x| x.repr() == ObjectRepr::Map) .and_then(|x| x.try_iter_pairs()) .ok_or_else(|| { Error::new( ErrorKind::InvalidOperation, - "attempted to construct keyword arguments from non object", + format!( + "attempted to apply keyword arguments from non map (got {})", + value.kind() + ), ) })); for (key, value) in iter { diff --git a/minijinja/tests/inputs/err_bad_args_unpack.txt b/minijinja/tests/inputs/err_bad_args_unpack.txt new file mode 100644 index 00000000..496abcb0 --- /dev/null +++ b/minijinja/tests/inputs/err_bad_args_unpack.txt @@ -0,0 +1,3 @@ +{} +--- +{{ get_args(*true) }} diff --git a/minijinja/tests/inputs/err_bad_kwargs_unpack.txt b/minijinja/tests/inputs/err_bad_kwargs_unpack.txt new file mode 100644 index 00000000..5efa1050 --- /dev/null +++ b/minijinja/tests/inputs/err_bad_kwargs_unpack.txt @@ -0,0 +1,3 @@ +{} +--- +{{ get_args(**[1, 2, 3]) }} diff --git a/minijinja/tests/snapshots/test_templates__vm@err_bad_args_unpack.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_bad_args_unpack.txt.snap new file mode 100644 index 00000000..16ec4c58 --- /dev/null +++ b/minijinja/tests/snapshots/test_templates__vm@err_bad_args_unpack.txt.snap @@ -0,0 +1,22 @@ +--- +source: minijinja/tests/test_templates.rs +description: "{{ get_args(*true) }}" +info: {} +input_file: minijinja/tests/inputs/err_bad_args_unpack.txt +--- +!!!ERROR!!! + +Error { + kind: InvalidOperation, + detail: "bool is not iterable", + name: "err_bad_args_unpack.txt", + line: 1, +} + +invalid operation: bool is not iterable (in err_bad_args_unpack.txt:1) +--------------------------- err_bad_args_unpack.txt --------------------------- + 1 > {{ get_args(*true) }} + i ^^^^^^^^^^^^^^^ invalid operation +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +No referenced variables +------------------------------------------------------------------------------- diff --git a/minijinja/tests/snapshots/test_templates__vm@err_bad_kwargs_unpack.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_bad_kwargs_unpack.txt.snap new file mode 100644 index 00000000..16e6a910 --- /dev/null +++ b/minijinja/tests/snapshots/test_templates__vm@err_bad_kwargs_unpack.txt.snap @@ -0,0 +1,22 @@ +--- +source: minijinja/tests/test_templates.rs +description: "{{ get_args(**[1, 2, 3]) }}" +info: {} +input_file: minijinja/tests/inputs/err_bad_kwargs_unpack.txt +--- +!!!ERROR!!! + +Error { + kind: InvalidOperation, + detail: "attempted to apply keyword arguments from non map (got sequence)", + name: "err_bad_kwargs_unpack.txt", + line: 1, +} + +invalid operation: attempted to apply keyword arguments from non map (got sequence) (in err_bad_kwargs_unpack.txt:1) +-------------------------- err_bad_kwargs_unpack.txt -------------------------- + 1 > {{ get_args(**[1, 2, 3]) }} + i ^^^^^^^^^^^^^^^^^^^^^ invalid operation +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +No referenced variables +------------------------------------------------------------------------------- From d0336e1b8167c9682d31c5747aa0931eb89dbbf2 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 10 Nov 2024 13:52:50 +0100 Subject: [PATCH 4/6] Rename internal enum methods for clarity --- minijinja/src/compiler/ast.rs | 4 ++-- minijinja/src/compiler/codegen.rs | 8 ++++---- minijinja/src/compiler/meta.rs | 4 ++-- minijinja/src/compiler/parser.rs | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/minijinja/src/compiler/ast.rs b/minijinja/src/compiler/ast.rs index 8f78ce25..38a36c5a 100644 --- a/minijinja/src/compiler/ast.rs +++ b/minijinja/src/compiler/ast.rs @@ -483,8 +483,8 @@ pub struct Call<'a> { pub enum CallArg<'a> { Pos(Expr<'a>), Kwarg(&'a str, Expr<'a>), - Splat(Expr<'a>), - KwargsSplat(Expr<'a>), + PosSplat(Expr<'a>), + KwargSplat(Expr<'a>), } /// Creates a list of values. diff --git a/minijinja/src/compiler/codegen.rs b/minijinja/src/compiler/codegen.rs index 5338507b..df5cfb12 100644 --- a/minijinja/src/compiler/codegen.rs +++ b/minijinja/src/compiler/codegen.rs @@ -763,7 +763,7 @@ impl<'source> CodeGenerator<'source> { self.compile_expr(expr); pending_args += 1; } - ast::CallArg::Splat(expr) => { + ast::CallArg::PosSplat(expr) => { if pending_args > 0 { self.add(Instruction::BuildList(Some(pending_args))); pending_args = 0; @@ -778,7 +778,7 @@ impl<'source> CodeGenerator<'source> { } has_kwargs = true; } - ast::CallArg::KwargsSplat(_) => { + ast::CallArg::KwargSplat(_) => { static_kwargs = false; has_kwargs = true; } @@ -804,7 +804,7 @@ impl<'source> CodeGenerator<'source> { pending_kwargs += 1; } } - ast::CallArg::KwargsSplat(expr) => { + ast::CallArg::KwargSplat(expr) => { if pending_kwargs > 0 { self.add(Instruction::BuildKwargs(pending_kwargs)); num_kwargs_batches += 1; @@ -813,7 +813,7 @@ impl<'source> CodeGenerator<'source> { self.compile_expr(expr); num_kwargs_batches += 1; } - ast::CallArg::Pos(_) | ast::CallArg::Splat(_) => {} + ast::CallArg::Pos(_) | ast::CallArg::PosSplat(_) => {} } } diff --git a/minijinja/src/compiler/meta.rs b/minijinja/src/compiler/meta.rs index e10c6ee0..25661593 100644 --- a/minijinja/src/compiler/meta.rs +++ b/minijinja/src/compiler/meta.rs @@ -85,8 +85,8 @@ fn tracker_visit_callarg<'a>(callarg: &ast::CallArg<'a>, state: &mut AssignmentT match callarg { ast::CallArg::Pos(expr) | ast::CallArg::Kwarg(_, expr) - | ast::CallArg::Splat(expr) - | ast::CallArg::KwargsSplat(expr) => tracker_visit_expr(expr, state), + | ast::CallArg::PosSplat(expr) + | ast::CallArg::KwargSplat(expr) => tracker_visit_expr(expr, state), } } diff --git a/minijinja/src/compiler/parser.rs b/minijinja/src/compiler/parser.rs index efcd055b..7529ac72 100644 --- a/minijinja/src/compiler/parser.rs +++ b/minijinja/src/compiler/parser.rs @@ -602,10 +602,10 @@ impl<'a> Parser<'a> { } } ArgType::Splat => { - args.push(ast::CallArg::Splat(expr)); + args.push(ast::CallArg::PosSplat(expr)); } ArgType::KwargsSplat => { - args.push(ast::CallArg::KwargsSplat(expr)); + args.push(ast::CallArg::KwargSplat(expr)); has_kwargs = true; } } From 35ff4d4ca263bcfae14e8d1bc760d3aa7747d5de Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 10 Nov 2024 14:29:01 +0100 Subject: [PATCH 5/6] Enforce upper limit on args and make instructions clearer --- minijinja/src/compiler/codegen.rs | 30 +++++++++++++------------ minijinja/src/compiler/instructions.rs | 10 ++++----- minijinja/src/compiler/parser.rs | 7 ++++++ minijinja/src/vm/context.rs | 9 ++++---- minijinja/src/vm/mod.rs | 31 +++++++++++++------------- minijinja/tests/test_vm.rs | 2 +- 6 files changed, 49 insertions(+), 40 deletions(-) diff --git a/minijinja/src/compiler/codegen.rs b/minijinja/src/compiler/codegen.rs index df5cfb12..455e2566 100644 --- a/minijinja/src/compiler/codegen.rs +++ b/minijinja/src/compiler/codegen.rs @@ -503,7 +503,7 @@ impl<'source> CodeGenerator<'source> { self.add_with_span(Instruction::FastSuper, call.span()); return; } else if name == "loop" && call.args.len() == 1 { - self.compile_call_args(std::slice::from_ref(&call.args[0]), None); + self.compile_call_args(std::slice::from_ref(&call.args[0]), 0, None); self.add(Instruction::FastRecurse); return; } @@ -660,17 +660,17 @@ impl<'source> CodeGenerator<'source> { if let Some(ref expr) = f.expr { self.compile_expr(expr); } - let arg_count = self.compile_call_args(&f.args, None); + let arg_count = self.compile_call_args(&f.args, 1, None); let local_id = get_local_id(&mut self.filter_local_ids, f.name); - self.add(Instruction::ApplyFilter(f.name, arg_count + 1, local_id)); + self.add(Instruction::ApplyFilter(f.name, arg_count, local_id)); self.pop_span(); } ast::Expr::Test(f) => { self.push_span(f.span()); self.compile_expr(&f.expr); - let arg_count = self.compile_call_args(&f.args, None); + let arg_count = self.compile_call_args(&f.args, 1, None); let local_id = get_local_id(&mut self.test_local_ids, f.name); - self.add(Instruction::PerformTest(f.name, arg_count + 1, local_id)); + self.add(Instruction::PerformTest(f.name, arg_count, local_id)); self.pop_span(); } ast::Expr::GetAttr(g) => { @@ -724,7 +724,7 @@ impl<'source> CodeGenerator<'source> { self.push_span(c.span()); match c.identify_call() { ast::CallType::Function(name) => { - let arg_count = self.compile_call_args(&c.args, caller); + let arg_count = self.compile_call_args(&c.args, 0, caller); self.add(Instruction::CallFunction(name, arg_count)); } #[cfg(feature = "multi_template")] @@ -735,13 +735,13 @@ impl<'source> CodeGenerator<'source> { } ast::CallType::Method(expr, name) => { self.compile_expr(expr); - let arg_count = self.compile_call_args(&c.args, caller); - self.add(Instruction::CallMethod(name, arg_count + 1)); + let arg_count = self.compile_call_args(&c.args, 1, caller); + self.add(Instruction::CallMethod(name, arg_count)); } ast::CallType::Object(expr) => { self.compile_expr(expr); - let arg_count = self.compile_call_args(&c.args, caller); - self.add(Instruction::CallObject(arg_count + 1)); + let arg_count = self.compile_call_args(&c.args, 1, caller); + self.add(Instruction::CallObject(arg_count)); } }; self.pop_span(); @@ -750,9 +750,10 @@ impl<'source> CodeGenerator<'source> { fn compile_call_args( &mut self, args: &[ast::CallArg<'source>], + extra_args: usize, caller: Option<&Caller<'source>>, - ) -> usize { - let mut pending_args = 0; + ) -> Option { + let mut pending_args = extra_args; let mut num_args_batches = 0; let mut has_kwargs = caller.is_some(); let mut static_kwargs = caller.is_none(); @@ -850,9 +851,10 @@ impl<'source> CodeGenerator<'source> { num_args_batches += 1; } self.add(Instruction::UnpackLists(num_args_batches)); - !0 + None } else { - pending_args + assert!(pending_args as u16 as usize == pending_args); + Some(pending_args as u16) } } diff --git a/minijinja/src/compiler/instructions.rs b/minijinja/src/compiler/instructions.rs index 6b7e280e..dd7fb288 100644 --- a/minijinja/src/compiler/instructions.rs +++ b/minijinja/src/compiler/instructions.rs @@ -128,10 +128,10 @@ pub enum Instruction<'source> { In, /// Apply a filter. - ApplyFilter(&'source str, usize, LocalId), + ApplyFilter(&'source str, Option, LocalId), /// Perform a filter. - PerformTest(&'source str, usize, LocalId), + PerformTest(&'source str, Option, LocalId), /// Emit the stack top as output Emit, @@ -181,13 +181,13 @@ pub enum Instruction<'source> { EndCapture, /// Calls a global function - CallFunction(&'source str, usize), + CallFunction(&'source str, Option), /// Calls a method - CallMethod(&'source str, usize), + CallMethod(&'source str, Option), /// Calls an object - CallObject(usize), + CallObject(Option), /// Duplicates the top item DupTop, diff --git a/minijinja/src/compiler/parser.rs b/minijinja/src/compiler/parser.rs index 7529ac72..9228e688 100644 --- a/minijinja/src/compiler/parser.rs +++ b/minijinja/src/compiler/parser.rs @@ -609,6 +609,13 @@ impl<'a> Parser<'a> { has_kwargs = true; } } + + // Set an arbitrary limit of max function parameters. This is done + // in parts because the opcodes can only express 2**16 as argument + // count. + if args.len() > 2000 { + syntax_error!("Too many aguments in function call") + } } Ok(args) diff --git a/minijinja/src/vm/context.rs b/minijinja/src/vm/context.rs index 5b9009f8..b0f9577a 100644 --- a/minijinja/src/vm/context.rs +++ b/minijinja/src/vm/context.rs @@ -109,11 +109,10 @@ impl Stack { self.values[start..].reverse(); } - pub fn slice_top(&mut self, n: usize) -> &[Value] { - let n = if n == !0 { - self.pop().as_usize().unwrap() - } else { - n + pub fn get_call_args(&mut self, n: Option) -> &[Value] { + let n = match n { + Some(n) => n as usize, + None => self.pop().as_usize().unwrap(), }; &self.values[self.values.len() - n..] } diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index 911b14ee..cdccfc3a 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -588,7 +588,7 @@ impl<'env> Vm<'env> { format!("filter {name} is unknown"), ) })); - let args = stack.slice_top(*arg_count); + let args = stack.get_call_args(*arg_count); let arg_count = args.len(); a = ctx_ok!(filter.apply_to(state, args)); stack.drop_top(arg_count); @@ -601,54 +601,55 @@ impl<'env> Vm<'env> { .ok_or_else(|| { Error::new(ErrorKind::UnknownTest, format!("test {name} is unknown")) })); - let args = stack.slice_top(*arg_count); + let args = stack.get_call_args(*arg_count); let arg_count = args.len(); let rv = ctx_ok!(test.perform(state, args)); stack.drop_top(arg_count); stack.push(Value::from(rv)); } Instruction::CallFunction(name, arg_count) => { + let args = stack.get_call_args(*arg_count); // super is a special function reserved for super-ing into blocks. - if *name == "super" { - if *arg_count != 0 { + let rv = if *name == "super" { + if !args.is_empty() { bail!(Error::new( ErrorKind::InvalidOperation, "super() takes no arguments", )); } - stack.push(ctx_ok!(self.perform_super(state, out, true))); + ctx_ok!(self.perform_super(state, out, true)) // loop is a special name which when called recurses the current loop. } else if *name == "loop" { - if *arg_count != 1 { + if args.len() != 1 { bail!(Error::new( ErrorKind::InvalidOperation, - format!("loop() takes one argument, got {}", *arg_count) + "loop() takes one argument" )); } // leave the one argument on the stack for the recursion + // this internall calls continue. recurse_loop!(true); } else if let Some(func) = state.lookup(name) { - let args = stack.slice_top(*arg_count); - let arg_count = args.len(); - a = ctx_ok!(func.call(state, args)); - stack.drop_top(arg_count); - stack.push(a); + ctx_ok!(func.call(state, args)) } else { bail!(Error::new( ErrorKind::UnknownFunction, format!("{name} is unknown"), )); - } + }; + let arg_count = args.len(); + stack.drop_top(arg_count); + stack.push(rv); } Instruction::CallMethod(name, arg_count) => { - let args = stack.slice_top(*arg_count); + let args = stack.get_call_args(*arg_count); let arg_count = args.len(); a = ctx_ok!(args[0].call_method(state, name, &args[1..])); stack.drop_top(arg_count); stack.push(a); } Instruction::CallObject(arg_count) => { - let args = stack.slice_top(*arg_count); + let args = stack.get_call_args(*arg_count); let arg_count = args.len(); a = ctx_ok!(args[0].call(state, &args[1..])); stack.drop_top(arg_count); diff --git a/minijinja/tests/test_vm.rs b/minijinja/tests/test_vm.rs index f13ebac2..19d6cdb5 100644 --- a/minijinja/tests/test_vm.rs +++ b/minijinja/tests/test_vm.rs @@ -307,7 +307,7 @@ fn test_call_object() { 42 + a }))); c.add(Instruction::LoadConst(Value::from(23i32))); - c.add(Instruction::CallObject(2)); + c.add(Instruction::CallObject(Some(2))); c.add(Instruction::Emit); let output = simple_eval(&c.finish().0, ()).unwrap(); From 6658c458e40edaaa7f2810ab056b4681079feee8 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 10 Nov 2024 14:37:08 +0100 Subject: [PATCH 6/6] Fix comment --- minijinja/src/vm/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/minijinja/src/vm/mod.rs b/minijinja/src/vm/mod.rs index cdccfc3a..4119b3ff 100644 --- a/minijinja/src/vm/mod.rs +++ b/minijinja/src/vm/mod.rs @@ -626,8 +626,8 @@ impl<'env> Vm<'env> { "loop() takes one argument" )); } - // leave the one argument on the stack for the recursion - // this internall calls continue. + // leave the one argument on the stack for the recursion. The + // recurse_loop! macro itself will perform a jump and not return here. recurse_loop!(true); } else if let Some(func) = state.lookup(name) { ctx_ok!(func.call(state, args))