Repository: mitsuhiko/minijinja (rust)

Issue:
Lexer syntax errors point to the wrong span
## Description

When working on #443 I noticed that syntax errors in the lexer point to wrong span

## Reproduction steps

Try to parse the following template:

```{{ 10_ }}```

Additional helpful information:

- Version of minijinja: 1.0.15

## What did you expect

```
syntax error: '_' may not occur at end of number (in err_bad_trailing_underscore.txt:1)
----------------------- err_bad_trailing_underscore.txt -----------------------
   1 > {{ 10_ }}
     i      ^ syntax error
```

Instead I got

```
syntax error: '_' may not occur at end of number (in err_bad_trailing_underscore.txt:1)
----------------------- err_bad_trailing_underscore.txt -----------------------
   1 > {{ 10_ }}
     i ^^ syntax error
```

Pull request:
Fixed incorrect lexer position reports
This fixes the logic in the lexer and parser so that on lexing errors the right information.  The way this is correctly implemented is by making the lexer aware of the filename so that it can already report errors correctly.  The error attachment logic in the parser is also slightly changed now.

It's a bit unfortunate that this breaks the unstable machinery API but I'm not sure if there is a particularly nice way to avoid that.  At least this makes it slightly less weird.  Now at least the lexer and parser are at least in the position of reporting the correct error locations on their own.

Fixes #445 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7ffa889..af0dbe0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,8 @@ All notable changes to MiniJinja are documented here.
   (`1 is sameas 1`).  #626
 - Added error context for strict undefined errors during template
   rendering.  #627
+- Syntax errors caused by the lexer now include the correct position of
+  the error.  #630
 
 ## 2.4.0
 
diff --git a/minijinja/src/compiler/lexer.rs b/minijinja/src/compiler/lexer.rs
index a06f64f..0fc5890 100644
--- a/minijinja/src/compiler/lexer.rs
+++ b/minijinja/src/compiler/lexer.rs
@@ -18,6 +18,7 @@ pub struct WhitespaceConfig {
 pub struct Tokenizer<'s> {
     stack: Vec<LexerState>,
     source: &'s str,
+    filename: &'s str,
     current_line: u32,
     current_col: u32,
     current_offset: usize,
@@ -287,6 +288,7 @@ impl<'s> Tokenizer<'s> {
     /// Creates a new tokenizer.
     pub fn new(
         input: &'s str,
+        filename: &'s str,
         in_expr: bool,
         syntax_config: SyntaxConfig,
         whitespace_config: WhitespaceConfig,
@@ -302,6 +304,7 @@ impl<'s> Tokenizer<'s> {
         }
         Tokenizer {
             source,
+            filename,
             stack: vec![if in_expr {
                 LexerState::Variable
             } else {
@@ -319,6 +322,11 @@ impl<'s> Tokenizer<'s> {
         }
     }
 
+    /// Returns the current filename.
+    pub fn filename(&self) -> &str {
+        self.filename
+    }
+
     /// Produces the next token from the tokenizer.
     pub fn next_token(&mut self) -> Result<Option<(Token<'s>, Span)>, Error> {
         loop {
@@ -398,7 +406,14 @@ impl<'s> Tokenizer<'s> {
 
     #[inline]
     fn syntax_error(&mut self, msg: &'static str) -> Error {
-        Error::new(ErrorKind::SyntaxError, msg)
+        let mut span = self.span(self.loc());
+        if span.start_col == span.end_col {
+            span.end_col += 1;
+            span.end_offset += 1;
+        }
+        let mut err = Error::new(ErrorKind::SyntaxError, msg);
+        err.set_filename_and_span(self.filename, span);
+        err
     }
 
     fn eat_number(&mut self) -> Result<(Token<'s>, Span), Error> {
@@ -469,7 +484,7 @@ impl<'s> Tokenizer<'s> {
             } else {
                 u128::from_str_radix(&num, radix)
                     .map(Token::Int128)
-                    .map_err(|_| self.syntax_error("invalid integer"))
+                    .map_err(|_| self.syntax_error("invalid integer (too large)"))
             }),
             self.span(old_loc),
         ))
@@ -509,6 +524,7 @@ impl<'s> Tokenizer<'s> {
             })
             .count();
         if escaped || self.rest_bytes().get(str_len + 1) != Some(&delim) {
+            self.advance(str_len + 1);
             return Err(self.syntax_error("unexpected end of string"));
         }
         let s = self.advance(str_len + 2);
@@ -638,6 +654,7 @@ impl<'s> Tokenizer<'s> {
                     self.handle_tail_ws(ws);
                     Ok(ControlFlow::Continue(()))
                 } else {
+                    self.advance(self.rest_bytes().len());
                     Err(self.syntax_error("unexpected end of comment"))
                 }
             }
@@ -726,6 +743,7 @@ impl<'s> Tokenizer<'s> {
                 return Ok(ControlFlow::Break((Token::TemplateData(result), span)));
             }
         }
+        self.advance(self.rest_bytes().len());
         Err(self.syntax_error("unexpected end of raw block"))
     }
 
@@ -896,7 +914,8 @@ pub fn tokenize(
 ) -> impl Iterator<Item = Result<(Token<'_>, Span), Error>> {
     // This function is unused in minijinja itself, it's only used in tests and in the
     // unstable machinery as a convenient alternative to the tokenizer.
-    let mut tokenizer = Tokenizer::new(input, in_expr, syntax_config, whitespace_config);
+    let mut tokenizer =
+        Tokenizer::new(input, "<string>", in_expr, syntax_config, whitespace_config);
     std::iter::from_fn(move || tokenizer.next_token().transpose())
 }
 
diff --git a/minijinja/src/compiler/parser.rs b/minijinja/src/compiler/parser.rs
index 10a2446..ae9a06e 100644
--- a/minijinja/src/compiler/parser.rs
+++ b/minijinja/src/compiler/parser.rs
@@ -103,11 +103,13 @@ impl<'a> TokenStream<'a> {
     /// Tokenize a template
     pub fn new(
         source: &'a str,
+        filename: &'a str,
         in_expr: bool,
         syntax_config: SyntaxConfig,
         whitespace_config: WhitespaceConfig,
     ) -> TokenStream<'a> {
-        let mut tokenizer = Tokenizer::new(source, in_expr, syntax_config, whitespace_config);
+        let mut tokenizer =
+            Tokenizer::new(source, filename, in_expr, syntax_config, whitespace_config);
         let current = tokenizer.next_token().transpose();
         TokenStream {
             tokenizer,
@@ -229,14 +231,22 @@ macro_rules! with_recursion_guard {
 }
 
 impl<'a> Parser<'a> {
+    /// Creates a new parser.
+    ///
+    /// `in_expr` is necessary to parse within an expression context.  Otherwise
+    /// the parser starts out in template context.  This means that when
+    /// [`parse`](Self::parse) is to be called, the `in_expr` argument must be
+    /// `false` and for [`parse_standalone_expr`](Self::parse_standalone_expr)
+    /// it must be `true`.
     pub fn new(
         source: &'a str,
+        filename: &'a str,
         in_expr: bool,
         syntax_config: SyntaxConfig,
         whitespace_config: WhitespaceConfig,
     ) -> Parser<'a> {
         Parser {
-            stream: TokenStream::new(source, in_expr, syntax_config, whitespace_config),
+            stream: TokenStream::new(source, filename, in_expr, syntax_config, whitespace_config),
             in_macro: false,
             in_loop: false,
             blocks: BTreeSet::new(),
@@ -244,6 +254,37 @@ impl<'a> Parser<'a> {
         }
     }
 
+    /// Parses a template.
+    pub fn parse(&mut self) -> Result<ast::Stmt<'a>, Error> {
+        let span = self.stream.last_span();
+        self.subparse(&|_| false)
+            .map(|children| {
+                ast::Stmt::Template(Spanned::new(
+                    ast::Template { children },
+                    self.stream.expand_span(span),
+                ))
+            })
+            .map_err(|err| self.attach_location_to_error(err))
+    }
+
+    /// Parses an expression and asserts that there is no more input after it.
+    pub fn parse_standalone_expr(&mut self) -> Result<ast::Expr<'a>, Error> {
+        self.parse_expr()
+            .and_then(|result| {
+                if ok!(self.stream.next()).is_some() {
+                    syntax_error!("unexpected input after expression")
+                } else {
+                    Ok(result)
+                }
+            })
+            .map_err(|err| self.attach_location_to_error(err))
+    }
+
+    /// Returns the current filename.
+    pub fn filename(&self) -> &str {
+        self.stream.tokenizer.filename()
+    }
+
     fn parse_ifexpr(&mut self) -> Result<ast::Expr<'a>, Error> {
         let mut span = self.stream.last_span();
         let mut expr = ok!(self.parse_or());
@@ -668,11 +709,11 @@ impl<'a> Parser<'a> {
         Ok(expr)
     }
 
-    pub fn parse_expr(&mut self) -> Result<ast::Expr<'a>, Error> {
+    fn parse_expr(&mut self) -> Result<ast::Expr<'a>, Error> {
         with_recursion_guard!(self, self.parse_ifexpr())
     }
 
-    pub fn parse_expr_noif(&mut self) -> Result<ast::Expr<'a>, Error> {
+    fn parse_expr_noif(&mut self) -> Result<ast::Expr<'a>, Error> {
         self.parse_or()
     }
 
@@ -1175,49 +1216,33 @@ impl<'a> Parser<'a> {
         Ok(rv)
     }
 
-    pub fn parse(&mut self) -> Result<ast::Stmt<'a>, Error> {
-        let span = self.stream.last_span();
-        Ok(ast::Stmt::Template(Spanned::new(
-            ast::Template {
-                children: ok!(self.subparse(&|_| false)),
-            },
-            self.stream.expand_span(span),
-        )))
+    #[inline]
+    fn attach_location_to_error(&mut self, mut err: Error) -> Error {
+        if err.line().is_none() {
+            err.set_filename_and_span(self.filename(), self.stream.last_span())
+        }
+        err
     }
 }
 
 /// Parses a template.
 pub fn parse<'source>(
     source: &'source str,
-    filename: &str,
+    filename: &'source str,
     syntax_config: SyntaxConfig,
     whitespace_config: WhitespaceConfig,
 ) -> Result<ast::Stmt<'source>, Error> {
-    let mut parser = Parser::new(source, false, syntax_config, whitespace_config);
-    parser.parse().map_err(|mut err| {
-        if err.line().is_none() {
-            err.set_filename_and_span(filename, parser.stream.last_span())
-        }
-        err
-    })
+    Parser::new(source, filename, false, syntax_config, whitespace_config).parse()
 }
 
-/// Parses an expression
+/// Parses a standalone expression.
 pub fn parse_expr(source: &str) -> Result<ast::Expr<'_>, Error> {
-    let mut parser = Parser::new(source, true, Default::default(), Default::default());
-    parser
-        .parse_expr()
-        .and_then(|result| {
-            if ok!(parser.stream.next()).is_some() {
-                syntax_error!("unexpected input after expression")
-            } else {
-                Ok(result)
-            }
-        })
-        .map_err(|mut err| {
-            if err.line().is_none() {
-                err.set_filename_and_span("<expression>", parser.stream.last_span())
-            }
-            err
-        })
+    Parser::new(
+        source,
+        "<expression>",
+        true,
+        Default::default(),
+        Default::default(),
+    )
+    .parse_standalone_expr()
 }


Test patch:
diff --git a/minijinja/tests/inputs/err_partial_float.txt b/minijinja/tests/inputs/err_partial_float.txt
new file mode 100644
index 0000000..0ece609
--- /dev/null
+++ b/minijinja/tests/inputs/err_partial_float.txt
@@ -0,0 +1,3 @@
+{}
+---
+{{ 42e+ }}
diff --git a/minijinja/tests/inputs/err_raw_block_missing_end.txt b/minijinja/tests/inputs/err_raw_block_missing_end.txt
new file mode 100644
index 0000000..78d0795
--- /dev/null
+++ b/minijinja/tests/inputs/err_raw_block_missing_end.txt
@@ -0,0 +1,3 @@
+{}
+---
+{% raw %}this raw block is missing an end tag
diff --git a/minijinja/tests/inputs/err_too_large_integer.txt b/minijinja/tests/inputs/err_too_large_integer.txt
new file mode 100644
index 0000000..84381d7
--- /dev/null
+++ b/minijinja/tests/inputs/err_too_large_integer.txt
@@ -0,0 +1,3 @@
+{}
+---
+{{ 10234237582375823572357238572385723582375827582375238572385723857238572385723523758 }}
diff --git a/minijinja/tests/inputs/err_unexpected_character.txt b/minijinja/tests/inputs/err_unexpected_character.txt
new file mode 100644
index 0000000..7780d32
--- /dev/null
+++ b/minijinja/tests/inputs/err_unexpected_character.txt
@@ -0,0 +1,3 @@
+{}
+---
+{{ foo @ }}
diff --git a/minijinja/tests/inputs/err_unterminated_string.txt b/minijinja/tests/inputs/err_unterminated_string.txt
new file mode 100644
index 0000000..639d75e
--- /dev/null
+++ b/minijinja/tests/inputs/err_unterminated_string.txt
@@ -0,0 +1,3 @@
+{}
+---
+{{ "this string is missing a closing quote
diff --git a/minijinja/tests/snapshots/test_templates__vm@err_bad_trailing_underscore.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_bad_trailing_underscore.txt.snap
index 32e3e71..c9d9c68 100644
--- a/minijinja/tests/snapshots/test_templates__vm@err_bad_trailing_underscore.txt.snap
+++ b/minijinja/tests/snapshots/test_templates__vm@err_bad_trailing_underscore.txt.snap
@@ -16,7 +16,7 @@ Error {
 syntax error: '_' may not occur at end of number (in err_bad_trailing_underscore.txt:1)
 ----------------------- err_bad_trailing_underscore.txt -----------------------
    1 > {{ 10_ }}
-     i ^^ syntax error
+     i       ^ syntax error
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 No referenced variables
 -------------------------------------------------------------------------------
diff --git a/minijinja/tests/snapshots/test_templates__vm@err_partial_float.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_partial_float.txt.snap
new file mode 100644
index 0000000..9c4c726
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@err_partial_float.txt.snap
@@ -0,0 +1,22 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{{ 42e+ }}"
+info: {}
+input_file: minijinja/tests/inputs/err_partial_float.txt
+---
+!!!SYNTAX ERROR!!!
+
+Error {
+    kind: SyntaxError,
+    detail: "invalid float",
+    name: "err_partial_float.txt",
+    line: 1,
+}
+
+syntax error: invalid float (in err_partial_float.txt:1)
+---------------------------- err_partial_float.txt ----------------------------
+   1 > {{ 42e+ }}
+     i        ^ syntax error
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+No referenced variables
+-------------------------------------------------------------------------------
diff --git a/minijinja/tests/snapshots/test_templates__vm@err_raw_block_missing_end.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_raw_block_missing_end.txt.snap
new file mode 100644
index 0000000..00e8db6
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@err_raw_block_missing_end.txt.snap
@@ -0,0 +1,22 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{% raw %}this raw block is missing an end tag"
+info: {}
+input_file: minijinja/tests/inputs/err_raw_block_missing_end.txt
+---
+!!!SYNTAX ERROR!!!
+
+Error {
+    kind: SyntaxError,
+    detail: "unexpected end of raw block",
+    name: "err_raw_block_missing_end.txt",
+    line: 1,
+}
+
+syntax error: unexpected end of raw block (in err_raw_block_missing_end.txt:1)
+------------------------ err_raw_block_missing_end.txt ------------------------
+   1 > {% raw %}this raw block is missing an end tag
+     i                                              ^ syntax error
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+No referenced variables
+-------------------------------------------------------------------------------
diff --git a/minijinja/tests/snapshots/test_templates__vm@err_too_large_integer.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_too_large_integer.txt.snap
new file mode 100644
index 0000000..9df9e05
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@err_too_large_integer.txt.snap
@@ -0,0 +1,22 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{{ 10234237582375823572357238572385723582375827582375238572385723857238572385723523758 }}"
+info: {}
+input_file: minijinja/tests/inputs/err_too_large_integer.txt
+---
+!!!SYNTAX ERROR!!!
+
+Error {
+    kind: SyntaxError,
+    detail: "invalid integer (too large)",
+    name: "err_too_large_integer.txt",
+    line: 1,
+}
+
+syntax error: invalid integer (too large) (in err_too_large_integer.txt:1)
+-------------------------- err_too_large_integer.txt --------------------------
+   1 > {{ 10234237582375823572357238572385723582375827582375238572385723857238572385723523758 }}
+     i                                                                                       ^ syntax error
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+No referenced variables
+-------------------------------------------------------------------------------
diff --git a/minijinja/tests/snapshots/test_templates__vm@err_unexpected_character.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_unexpected_character.txt.snap
new file mode 100644
index 0000000..f1ed446
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@err_unexpected_character.txt.snap
@@ -0,0 +1,22 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{{ foo @ }}"
+info: {}
+input_file: minijinja/tests/inputs/err_unexpected_character.txt
+---
+!!!SYNTAX ERROR!!!
+
+Error {
+    kind: SyntaxError,
+    detail: "unexpected character",
+    name: "err_unexpected_character.txt",
+    line: 1,
+}
+
+syntax error: unexpected character (in err_unexpected_character.txt:1)
+------------------------ err_unexpected_character.txt -------------------------
+   1 > {{ foo @ }}
+     i        ^ syntax error
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+No referenced variables
+-------------------------------------------------------------------------------
diff --git a/minijinja/tests/snapshots/test_templates__vm@err_unterminated_string.txt.snap b/minijinja/tests/snapshots/test_templates__vm@err_unterminated_string.txt.snap
new file mode 100644
index 0000000..7f22b1b
--- /dev/null
+++ b/minijinja/tests/snapshots/test_templates__vm@err_unterminated_string.txt.snap
@@ -0,0 +1,22 @@
+---
+source: minijinja/tests/test_templates.rs
+description: "{{ \"this string is missing a closing quote"
+info: {}
+input_file: minijinja/tests/inputs/err_unterminated_string.txt
+---
+!!!SYNTAX ERROR!!!
+
+Error {
+    kind: SyntaxError,
+    detail: "unexpected end of string",
+    name: "err_unterminated_string.txt",
+    line: 1,
+}
+
+syntax error: unexpected end of string (in err_unterminated_string.txt:1)
+------------------------- err_unterminated_string.txt -------------------------
+   1 > {{ "this string is missing a closing quote
+     i                                           ^ syntax error
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+No referenced variables
+-------------------------------------------------------------------------------