From 4ce1fd4c60a58da6370522d0bb40bead8b133d01 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Thu, 23 Nov 2023 15:46:44 +0100
Subject: [PATCH] Only allow named arguments as last arguments

---
 askama_derive/src/generator.rs               |  7 +++++--
 askama_parser/src/expr.rs                    | 14 ++++++++++++--
 book/src/template_syntax.md                  |  6 ++++--
 testing/tests/macro.rs                       |  2 --
 testing/tests/ui/macro_named_argument.rs     | 11 ++++++++++-
 testing/tests/ui/macro_named_argument.stderr | 10 ++++++++++
 6 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/askama_derive/src/generator.rs b/askama_derive/src/generator.rs
index 2cc2f09f9..20c943966 100644
--- a/askama_derive/src/generator.rs
+++ b/askama_derive/src/generator.rs
@@ -736,9 +736,12 @@ impl<'a> Generator<'a> {
         //   are passed in the right order). To ensure we won't accidentally pass a named argument,
         //   we split named and unnamed arguments in two vectors before starting.
         let mut output_args = Vec::new();
+        // Since named arguments can only be passed last, we only need to check if the last argument
+        // is a named one.
         let args = if args
-            .iter()
-            .any(|arg| matches!(arg, Expr::NamedArgument(_, _)))
+            .last()
+            .map(|arg| matches!(arg, Expr::NamedArgument(_, _)))
+            .unwrap_or(false)
         {
             // We have named arguments, so we need to ensure that they are passed in the right
             // order.
diff --git a/askama_parser/src/expr.rs b/askama_parser/src/expr.rs
index 9ee1eb411..d8687e466 100644
--- a/askama_parser/src/expr.rs
+++ b/askama_parser/src/expr.rs
@@ -111,10 +111,20 @@ impl<'a> Expr<'a> {
         start: &'a str,
         is_template_macro: bool,
     ) -> ParseResult<'a, Self> {
-        alt((
+        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)
+        ))(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))
+        }
     }
 
     fn named_argument(
diff --git a/book/src/template_syntax.md b/book/src/template_syntax.md
index c1a9b96aa..9186d60be 100644
--- a/book/src/template_syntax.md
+++ b/book/src/template_syntax.md
@@ -609,9 +609,11 @@ You can use whitespace characters around `=`:
 You can mix named and non-named arguments when calling a macro:
 
 ```
-{% call heading(bold="something", "title") %}
+{% call heading("title", bold="something") %}
 ```
 
+However please note than named arguments must always come **last**.
+
 In this case, the named arguments will be passed to the related parameter and then the other
 arguments will be placed in what remains. Let's explain it in code:
 
@@ -619,7 +621,7 @@ arguments will be placed in what remains. Let's explain it in code:
 {% macro heading(arg1, arg2, arg3, arg4) %}
 {% endmacro %}
 
-{% call heading("something", arg2="title", arg4="ah", "b") %}
+{% call heading("something", "b", arg4="ah", arg2="title") %}
 ```
 
 First it'll be replaced like this:
diff --git a/testing/tests/macro.rs b/testing/tests/macro.rs
index 4a288350b..56b39cfd9 100644
--- a/testing/tests/macro.rs
+++ b/testing/tests/macro.rs
@@ -104,7 +104,6 @@ fn test_macro_self_arg() {
 
 {%- call thrice(param1=2, param2=3) -%}
 {%- call thrice(param2=3, param1=2) -%}
-{%- call thrice(param2=3, 2) -%}
 {%- call thrice(3, param1=2) -%}
 ",
     ext = "html"
@@ -121,7 +120,6 @@ fn test_named_argument() {
 2 3
 2 3
 2 3
-2 3
 "
     );
 }
diff --git a/testing/tests/ui/macro_named_argument.rs b/testing/tests/ui/macro_named_argument.rs
index 3376b0ee4..c67bf27c9 100644
--- a/testing/tests/ui/macro_named_argument.rs
+++ b/testing/tests/ui/macro_named_argument.rs
@@ -22,8 +22,17 @@ struct InvalidNamedArg2;
 {{ param1 }} {{ param2 }}
 {%- endmacro -%}
 
-{%- call thrice(param1=2, 3) | filter(param1=12) -%}", ext = "html")]
+{%- 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;
+
 fn main() {
 }
diff --git a/testing/tests/ui/macro_named_argument.stderr b/testing/tests/ui/macro_named_argument.stderr
index 27bcc30bb..5412d0d55 100644
--- a/testing/tests/ui/macro_named_argument.stderr
+++ b/testing/tests/ui/macro_named_argument.stderr
@@ -24,3 +24,13 @@ error: problems parsing template source at row 5, column 29 near:
    |          ^^^^^^^^
    |
    = 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 5, 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)