Repository: djc/askama (rust)

Issue:
Allow to have named arguments in macro
It'd pretty nice to be able to have named arguments. For example:

```
{% macro menu_link(href, text, target) %}
...
{% endmacro menu_link %}
```

Then to be able to call it like this:

```
{% menu_link(href="/something", text="link", target="_blank") %}
```

It makes the code much better when reading it.

As a first step, only allowing it for macros would be nice, but potentially extending it to filters would be nice too.

Pull request description:
Allow to pass named arguments to macro calls
Fixes #885.

Based on https://github.com/djc/askama/pull/909 (so only the last 3 commits are relevant to this PR).

I made a very permissive implementation which allows to mix named and non-named arguments. The only thing we need to check outside of the parser is that the named arguments actually have an equivalent in the called macro. For the rest, everything is checked in the parser directly, which allows nicer error messages.

Patch:
diff --git a/askama_derive/src/generator.rs b/askama_derive/src/generator.rs
index c6c5d10..65136c6 100644
--- a/askama_derive/src/generator.rs
+++ b/askama_derive/src/generator.rs
@@ -727,7 +727,50 @@ impl<'a> Generator<'a> {
                 args.len()
             )));
         }
-        for (expr, arg) in std::iter::zip(args, &def.args) {
+        let mut named_arguments = HashMap::new();
+        // Since named arguments can only be passed last, we only need to check if the last argument
+        // is a named one.
+        if let Some(Expr::NamedArgument(_, _)) = args.last() {
+            // First we check that all named arguments actually exist in the called item.
+            for arg in args.iter().rev() {
+                let Expr::NamedArgument(arg_name, _) = arg else {
+                    break;
+                };
+                if !def.args.iter().any(|arg| arg == arg_name) {
+                    return Err(CompileError::from(format!(
+                        "no argument named `{arg_name}` in macro {name:?}"
+                    )));
+                }
+                named_arguments.insert(arg_name, arg);
+            }
+        }
+
+        // Handling both named and unnamed arguments requires to be careful of the named arguments
+        // order. To do so, we iterate through the macro defined arguments and then check if we have
+        // a named argument with this name:
+        //
+        // * If there is one, we add it and move to the next argument.
+        // * If there isn't one, then we pick the next argument (we can do it without checking
+        //   anything since named arguments are always last).
+        let mut allow_positional = true;
+        for (index, arg) in def.args.iter().enumerate() {
+            let expr = match named_arguments.get(&arg) {
+                Some(expr) => {
+                    allow_positional = false;
+                    expr
+                }
+                None => {
+                    if !allow_positional {
+                        // If there is already at least one named argument, then it's not allowed
+                        // to use unnamed ones at this point anymore.
+                        return Err(CompileError::from(format!(
+                            "cannot have unnamed argument (`{arg}`) after named argument in macro \
+                             {name:?}"
+                        )));
+                    }
+                    &args[index]
+                }
+            };
             match expr {
                 // If `expr` is already a form of variable then
                 // don't reintroduce a new variable. This is
@@ -1104,6 +1147,7 @@ impl<'a> Generator<'a> {
             Expr::RustMacro(ref path, args) => self.visit_rust_macro(buf, path, args),
             Expr::Try(ref expr) => self.visit_try(buf, expr.as_ref())?,
             Expr::Tuple(ref exprs) => self.visit_tuple(buf, exprs)?,
+            Expr::NamedArgument(_, ref expr) => self.visit_named_argument(buf, expr)?,
         })
     }
 
@@ -1504,6 +1548,15 @@ impl<'a> Generator<'a> {
         Ok(DisplayWrap::Unwrapped)
     }
 
+    fn visit_named_argument(
+        &mut self,
+        buf: &mut Buffer,
+        expr: &Expr<'_>,
+    ) -> Result<DisplayWrap, CompileError> {
+        self.visit_expr(buf, expr)?;
+        Ok(DisplayWrap::Unwrapped)
+    }
+
     fn visit_array(
         &mut self,
         buf: &mut Buffer,
@@ -1923,6 +1976,7 @@ pub(crate) fn is_cacheable(expr: &Expr<'_>) -> bool {
         }
         Expr::Group(arg) => is_cacheable(arg),
         Expr::Tuple(args) => args.iter().all(is_cacheable),
+        Expr::NamedArgument(_, expr) => is_cacheable(expr),
         // We have too little information to tell if the expression is pure:
         Expr::Call(_, _) => false,
         Expr::RustMacro(_, _) => false,
diff --git a/askama_parser/src/expr.rs b/askama_parser/src/expr.rs
index 76691be..2928b57 100644
--- a/askama_parser/src/expr.rs
+++ b/askama_parser/src/expr.rs
@@ -1,3 +1,5 @@
+use std::borrow::Cow;
+use std::collections::HashSet;
 use std::str;
 
 use nom::branch::alt;
@@ -12,7 +14,7 @@ use nom::sequence::{pair, preceded, terminated, tuple};
 use super::{
     char_lit, identifier, not_ws, num_lit, path_or_identifier, str_lit, ws, Level, PathOrIdentifier,
 };
-use crate::ParseResult;
+use crate::{ErrorContext, ParseResult};
 
 macro_rules! expr_prec_layer {
     ( $name:ident, $inner:ident, $op:expr ) => {
@@ -61,6 +63,7 @@ pub enum Expr<'a> {
     Attr(Box<Expr<'a>>, &'a str),
     Index(Box<Expr<'a>>, Box<Expr<'a>>),
     Filter(&'a str, Vec<Expr<'a>>),
+    NamedArgument(&'a str, Box<Expr<'a>>),
     Unary(&'a str, Box<Expr<'a>>),
     BinOp(&'a str, Box<Expr<'a>>, Box<Expr<'a>>),
     Range(&'a str, Option<Box<Expr<'a>>>, Option<Box<Expr<'a>>>),
@@ -72,17 +75,83 @@ pub enum Expr<'a> {
 }
 
 impl<'a> Expr<'a> {
-    pub(super) fn arguments(i: &'a str, level: Level) -> ParseResult<'a, Vec<Self>> {
+    pub(super) fn arguments(
+        i: &'a str,
+        level: Level,
+        is_template_macro: bool,
+    ) -> ParseResult<'a, Vec<Self>> {
         let (_, level) = level.nest(i)?;
+        let mut named_arguments = HashSet::new();
+        let start = i;
+
         preceded(
             ws(char('(')),
             cut(terminated(
-                separated_list0(char(','), ws(move |i| Self::parse(i, level))),
+                separated_list0(
+                    char(','),
+                    ws(move |i| {
+                        // Needed to prevent borrowing it twice between this closure and the one
+                        // calling `Self::named_arguments`.
+                        let named_arguments = &mut named_arguments;
+                        let has_named_arguments = !named_arguments.is_empty();
+
+                        let (i, expr) = alt((
+                            move |i| {
+                                Self::named_argument(
+                                    i,
+                                    level,
+                                    named_arguments,
+                                    start,
+                                    is_template_macro,
+                                )
+                            },
+                            move |i| Self::parse(i, level),
+                        ))(i)?;
+                        if has_named_arguments && !matches!(expr, Self::NamedArgument(_, _)) {
+                            Err(nom::Err::Failure(ErrorContext {
+                                input: start,
+                                message: Some(Cow::Borrowed(
+                                    "named arguments must always be passed last",
+                                )),
+                            }))
+                        } else {
+                            Ok((i, expr))
+                        }
+                    }),
+                ),
                 char(')'),
             )),
         )(i)
     }
 
+    fn named_argument(
+        i: &'a str,
+        level: Level,
+        named_arguments: &mut HashSet<&'a str>,
+        start: &'a str,
+        is_template_macro: bool,
+    ) -> ParseResult<'a, Self> {
+        if !is_template_macro {
+            // If this is not a template macro, we don't want to parse named arguments so
+            // we instead return an error which will allow to continue the parsing.
+            return Err(nom::Err::Error(error_position!(i, ErrorKind::Alt)));
+        }
+
+        let (_, level) = level.nest(i)?;
+        let (i, (argument, _, value)) =
+            tuple((identifier, ws(char('=')), move |i| Self::parse(i, level)))(i)?;
+        if named_arguments.insert(argument) {
+            Ok((i, Self::NamedArgument(argument, Box::new(value))))
+        } else {
+            Err(nom::Err::Failure(ErrorContext {
+                input: start,
+                message: Some(Cow::Owned(format!(
+                    "named argument `{argument}` was passed more than once"
+                ))),
+            }))
+        }
+    }
+
     pub(super) fn parse(i: &'a str, level: Level) -> ParseResult<'a, Self> {
         let (_, level) = level.nest(i)?;
         let range_right = move |i| {
@@ -122,7 +191,7 @@ impl<'a> Expr<'a> {
             let (i, (_, fname, args)) = tuple((
                 char('|'),
                 ws(identifier),
-                opt(|i| Expr::arguments(i, level)),
+                opt(|i| Expr::arguments(i, level, false)),
             ))(i)?;
             Ok((i, (fname, args)))
         }
@@ -354,7 +423,7 @@ impl<'a> Suffix<'a> {
 
     fn call(i: &'a str, level: Level) -> ParseResult<'a, Self> {
         let (_, level) = level.nest(i)?;
-        map(move |i| Expr::arguments(i, level), Self::Call)(i)
+        map(move |i| Expr::arguments(i, level, false), Self::Call)(i)
     }
 
     fn r#try(i: &'a str) -> ParseResult<'a, Self> {
diff --git a/askama_parser/src/node.rs b/askama_parser/src/node.rs
index ba117c5..d11895b 100644
--- a/askama_parser/src/node.rs
+++ b/askama_parser/src/node.rs
@@ -577,7 +577,7 @@ impl<'a> Call<'a> {
             cut(tuple((
                 opt(tuple((ws(identifier), ws(tag("::"))))),
                 ws(identifier),
-                opt(ws(|nested| Expr::arguments(nested, s.level.get()))),
+                opt(ws(|nested| Expr::arguments(nested, s.level.get(), true))),
                 opt(Whitespace::parse),
             ))),
         ));
diff --git a/book/src/template_syntax.md b/book/src/template_syntax.md
index a794d3f..4bd9894 100644
--- a/book/src/template_syntax.md
+++ b/book/src/template_syntax.md
@@ -564,7 +564,7 @@ You can define macros within your template by using `{% macro name(args) %}`, en
 
 You can then call it with `{% call name(args) %}`:
 
-```
+```jinja
 {% macro heading(arg) %}
 
 <h1>{{arg}}</h1>
@@ -576,7 +576,7 @@ You can then call it with `{% call name(args) %}`:
 
 You can place macros in a separate file and use them in your templates by using `{% import %}`:
 
-```
+```jinja
 {%- import "macro.html" as scope -%}
 
 {% call scope::heading(s) %}
@@ -584,6 +584,51 @@ You can place macros in a separate file and use them in your templates by using
 
 You can optionally specify the name of the macro in `endmacro`:
 
-```html
+```jinja
 {% macro heading(arg) %}<p>{{arg}}</p>{% endmacro heading %}
 ```
+
+You can also specify arguments by their name (as defined in the macro):
+
+```jinja
+{% macro heading(arg, bold) %}
+
+<h1>{{arg}} <b>{{bold}}</b></h1>
+
+{% endmacro %}
+
+{% call heading(bold="something", arg="title") %}
+```
+
+You can use whitespace characters around `=`:
+
+```jinja
+{% call heading(bold = "something", arg = "title") %}
+```
+
+You can mix named and non-named arguments when calling a macro:
+
+```
+{% call heading("title", bold="something") %}
+```
+
+However please note than named arguments must always come **last**.
+
+Another thing to note, if a named argument is referring to an argument that would
+be used for a non-named argument, it will error:
+
+```jinja
+{% macro heading(arg1, arg2, arg3, arg4) %}
+{% endmacro %}
+
+{% call heading("something", "b", arg4="ah", arg2="title") %}
+```
+
+In here it's invalid because `arg2` is the second argument and would be used by
+`"b"`. So either you replace `"b"` with `arg3="b"` or you pass `"title"` before:
+
+```jinja
+{% call heading("something", arg3="b", arg4="ah", arg2="title") %}
+{# Equivalent of: #}
+{% call heading("something", "title", "b", arg4="ah") %}
+```


Test patch:
diff --git a/testing/tests/macro.rs b/testing/tests/macro.rs
index faada37..0088f36 100644
--- a/testing/tests/macro.rs
+++ b/testing/tests/macro.rs
@@ -95,3 +95,31 @@ fn test_macro_self_arg() {
     let t = MacroSelfArgTemplate { s: "foo" };
     assert_eq!(t.render().unwrap(), "foo");
 }
+
+#[derive(Template)]
+#[template(
+    source = "{%- macro thrice(param1, param2) -%}
+{{ param1 }} {{ param2 }}
+{% endmacro -%}
+
+{%- call thrice(param1=2, param2=3) -%}
+{%- call thrice(param2=3, param1=2) -%}
+{%- call thrice(3, param2=2) -%}
+",
+    ext = "html"
+)]
+struct MacroNamedArg;
+
+#[test]
+// We check that it's always the correct values passed to the
+// expected argument.
+fn test_named_argument() {
+    assert_eq!(
+        MacroNamedArg.render().unwrap(),
+        "\
+2 3
+2 3
+3 2
+"
+    );
+}
diff --git a/testing/tests/ui/macro_named_argument.rs b/testing/tests/ui/macro_named_argument.rs
new file mode 100644
index 0000000..fe8a194
--- /dev/null
+++ b/testing/tests/ui/macro_named_argument.rs
@@ -0,0 +1,45 @@
+use askama::Template;
+
+#[derive(Template)]
+#[template(source = "{%- macro thrice(param1, param2) -%}
+{{ param1 }} {{ param2 }}
+{%- endmacro -%}
+
+{%- call thrice(param1=2, param3=3) -%}", ext = "html")]
+struct InvalidNamedArg;
+
+#[derive(Template)]
+#[template(source = "{%- macro thrice(param1, param2) -%}
+{{ param1 }} {{ param2 }}
+{%- endmacro -%}
+
+{%- call thrice(param1=2, param1=3) -%}", ext = "html")]
+struct InvalidNamedArg2;
+
+// Ensures that filters can't have named arguments.
+#[derive(Template)]
+#[template(source = "{%- macro thrice(param1, param2) -%}
+{{ param1 }} {{ param2 }}
+{%- endmacro -%}
+
+{%- call thrice(3, param1=2) | filter(param1=12) -%}", ext = "html")]
+struct InvalidNamedArg3;
+
+// Ensures that named arguments can only be passed last.
+#[derive(Template)]
+#[template(source = "{%- macro thrice(param1, param2) -%}
+{{ param1 }} {{ param2 }}
+{%- endmacro -%}
+{%- call thrice(param1=2, 3) -%}", ext = "html")]
+struct InvalidNamedArg4;
+
+// Ensures that named arguments can't be used for arguments before them.
+#[derive(Template)]
+#[template(source = "{%- macro thrice(param1, param2) -%}
+{{ param1 }} {{ param2 }}
+{%- endmacro -%}
+{%- call thrice(3, param1=2) -%}", ext = "html")]
+struct InvalidNamedArg5;
+
+fn main() {
+}
diff --git a/testing/tests/ui/macro_named_argument.stderr b/testing/tests/ui/macro_named_argument.stderr
new file mode 100644
index 0000000..b346f2d
--- /dev/null
+++ b/testing/tests/ui/macro_named_argument.stderr
@@ -0,0 +1,44 @@
+error: no argument named `param3` in macro "thrice"
+ --> tests/ui/macro_named_argument.rs:3:10
+  |
+3 | #[derive(Template)]
+  |          ^^^^^^^^
+  |
+  = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: named argument `param1` was passed more than once
+       problems parsing template source at row 5, column 15 near:
+       "(param1=2, param1=3) -%}"
+  --> tests/ui/macro_named_argument.rs:11:10
+   |
+11 | #[derive(Template)]
+   |          ^^^^^^^^
+   |
+   = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: problems parsing template source at row 5, column 29 near:
+       "| filter(param1=12) -%}"
+  --> tests/ui/macro_named_argument.rs:20:10
+   |
+20 | #[derive(Template)]
+   |          ^^^^^^^^
+   |
+   = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: named arguments must always be passed last
+       problems parsing template source at row 4, column 15 near:
+       "(param1=2, 3) -%}"
+  --> tests/ui/macro_named_argument.rs:29:10
+   |
+29 | #[derive(Template)]
+   |          ^^^^^^^^
+   |
+   = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: cannot have unnamed argument (`param2`) after named argument in macro "thrice"
+  --> tests/ui/macro_named_argument.rs:37:10
+   |
+37 | #[derive(Template)]
+   |          ^^^^^^^^
+   |
+   = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)