Better (post-parser) errors #1058

GuillaumeGomez · 2024-05-24T21:57:27Z

Fixes https://github.com/djc/askama/issues/926.

This is very big because a lot of small changes everywhere. However, the result is really great (at least for me 😺 ). Take a look at the updated ui tests to see the impact of this PR.

djc

I like the end result too, but there is a lot in the diff I don't like. Please be careful about minimizing the necessary changes and make some smaller commits.

djc · 2024-05-25T13:54:32Z

askama_derive/src/input.rs

@@ -55,18 +55,20 @@ impl TemplateInput<'_> {
                PathBuf::from(format!("{}.{}", ast.ident, ext)).into()
            }
            (&Source::Source(_), None) => {
-                return Err("must include 'ext' attribute when using 'source' attribute".into())
+                return Err(CompileError::new(


Seems to me like none of the call sites that pass None as the second argument for CompileError::new() actually need to change.

I disagree. I removed the From implementations on purpose to ensure that we don't use .into() by mistake in the future. It's less convenient but it forces to think about whether or not you can provide context on this error or not. If not, then you use None and it's done.

Side effect: it also allows us (reviewers) to spot it more easily when reviewing and to check if it could in fact provide context to the error.

This change should definitely not be in the same commit as all these other changes, because it is a noop change which makes the diff very verbose and thus hard to review. We can reintroduce it in a later commit, although I would suggest it might make more sense to attach error() methods to contexts like TemplateInput that can refer to the available context directly.

djc · 2024-05-25T13:55:52Z

askama_parser/src/node.rs

 pub struct Comment<'a> {
    pub ws: Ws,
    pub content: &'a str,
 }

+// We never want to compare with `content`.
+impl<'a> PartialEq for Comment<'a> {


This should be in a separate commit.

djc · 2024-05-25T13:57:22Z

askama_parser/src/lib.rs

@@ -155,6 +136,54 @@ impl fmt::Display for ParseError {
 pub(crate) type ParseErr<'a> = nom::Err<ErrorContext<'a>>;
 pub(crate) type ParseResult<'a, T = &'a str> = Result<(&'a str, T), ParseErr<'a>>;

+pub fn generate_row_and_column(src: &str, input: &str) -> (usize, usize, String) {


Extracting these from Ast::from_str() should be a separate commit.

Three or four element tuples should be structs with named fields.

djc · 2024-05-25T13:58:30Z

askama_parser/src/expr.rs

-                Some(Self::Call(args)) => expr = Expr::Call(expr.into(), args),
-                Some(Self::Try) => expr = Expr::Try(expr.into()),
+                Some(Self::Attr(attr)) => {
+                    expr = Expr {


Suggest you add a constructor on Expr to minimize the syntactic overhead of the wrapper here.

djc · 2024-05-25T13:59:53Z

askama_parser/src/expr.rs

-    /// This variant should never be used directly. It is created when generating filter blocks.
-    Generated(String),
+#[derive(Clone, Debug)]
+pub struct Expr<'a> {


In order to minimize the diff here, use a wrapper for the existing Expr type instead of this renaming. Suggest you might be able to reuse the same wrapper for both Node and Expr, it could potentially implement Deref.

What would you name this new wrapper type? ExprOrNode?

EDIT: Which would look like:

enum ExprOrNode<'a> { Expr(&'a str, Expr<'a>), Node(&'a str, Node<'a>), }

That would force Node::Expr to use Box to prevent infinite type size too.

More like:

struct WithSpan<'a, T> { inner: T, span: &'a str, } impl Deref for WithSpan<'a> { type Target = T; .. }

Might even be possible to use this to replace the ugly TemplateContents trait?

djc · 2024-05-25T14:00:47Z

askama_derive/src/input.rs

@@ -127,13 +132,16 @@ impl TemplateInput<'_> {
            let mut nested = vec![parsed.nodes()];
            while let Some(nodes) = nested.pop() {
                for n in nodes {
-                    let mut add_to_check = |path: Rc<Path>| -> Result<(), CompileError> {
-                        if !map.contains_key(&path) {
+                    let mut add_to_check = |new_path: Rc<Path>| -> Result<(), CompileError> {


Please avoid/isolate unrelated changes.

It's not unrelated, I use the captured parent path, but to do so I needed to rename the closure argument. I use it here:

let source = get_template_source( &new_path, Some((&path, parsed.source(), n.template_content())), )?;

djc · 2024-05-25T14:01:42Z

askama_derive/src/heritage.rs

 }

 impl Context<'_> {
+    pub(crate) fn new_empty(parsed: &Parsed) -> Context<'_> {


Let's call this empty().

…ons in `askama_parser`

GuillaumeGomez · 2024-05-27T21:03:53Z

Finally reworked the whole git history and made the suggested changes.

djc · 2024-05-30T10:04:21Z

askama_parser/src/lib.rs

@@ -155,6 +129,72 @@ impl fmt::Display for ParseError {
 pub(crate) type ParseErr<'a> = nom::Err<ErrorContext<'a>>;
 pub(crate) type ParseResult<'a, T = &'a str> = Result<(&'a str, T), ParseErr<'a>>;

+pub struct ErrorInfo {


Although the direction is good, this still has too much going on. Suggest a few steps, maybe in a separate PR?

Extract all of this code from Ast::from_str() into a ParseError::new() function

Unify the row/column display to {row}:{column} for both path and non-path errors

Extract an ErrorInfo type with an impl Display that takes care of the final formatting step, which will need to include a path: Option<Rc<Path>>

To the extent that you need what is here generate_row_and_column() and generate_error_info() later in the PR, they should perhaps be constructors on ErrorInfo.

Also please keep functions in top-down order -- maybe do more self-review to check for style issues? Reviewing your changes is taking many round trips.

Feedback is appreciated. I don't plan to send another PR for this, all changes are tied together and at this point your suggestions (even if great) are only improvements over this code. I'll implement what you suggested though.

Also please keep functions in top-down order -- maybe do more self-review to check for style issues? Reviewing your changes is taking many round trips.

Well, as you said, it's code styling, so we're entering personal preferences. I don't mind following a project code's style but if there is no test to enforce it and tell me what and where I did something wrong, then you shouldn't expect people to remember it or to implement it as such. Therefore, I don't plan to try to remember what style you want to apply until there is a check for it, I work on way too many different codebases to have motivation for this. If this is a blocker for you, then I'll just fork askama from this point and implement what I need on my side while backporting changes from your repository to mine. Just to be clear: this is not a hostile take or anything, I really love askama and really enjoy your suggestions. It's just that you have high expectations for PRs, and I don't have the tools nor the time to live up to your expectations. So I'll let it up to you. I don't mind if you send commits to my PRs to fix code formatting, this is very welcome as well.

GuillaumeGomez · 2024-06-07T20:46:26Z

I'll try to update this PR in the next days since #1061 got merged. Still didn't applied your comments too, just came back from a hackfest.

GuillaumeGomez requested review from Kijewski and djc May 24, 2024 21:57

djc reviewed May 25, 2024

View reviewed changes

GuillaumeGomez added 2 commits May 27, 2024 15:29

Add generate_row_and_column and generate_error_info public functi…

a33f77b

…ons in `askama_parser`

Create askama_parser::WithSpan type

e452b76

GuillaumeGomez force-pushed the better-errors branch from ced5d7d to 9ca3190 Compare May 27, 2024 20:52

GuillaumeGomez added 12 commits May 27, 2024 22:58

Wrap Expr into WithSpan

368fd82

Update Node variants to use WithSpan

a2c50d1

Add Node::span method

e40056e

Update askama_parser AST tests

c36eeb3

Update to new WithSpan type in askama_derive

e47b973

Add askama_derive::FileInfo type

a2881bb

Migrate config errors to new more complete CompileError version

debd9af

Add Context::generate_error method

dbfa09e

Migrate errors with file information

344b0ef

Remove From trait implementations for CompileError

326b02e

Update ui tests

a6613b6

Fix clippy lints

22d352b

GuillaumeGomez force-pushed the better-errors branch from 864cfd3 to 22d352b Compare May 27, 2024 20:59

djc reviewed May 30, 2024

View reviewed changes

djc mentioned this pull request Jun 7, 2024

Parser: improve error structure #1061

Merged

GuillaumeGomez closed this Jun 17, 2024

GuillaumeGomez deleted the better-errors branch June 17, 2024 16:32

djc mentioned this pull request Jun 20, 2024

Feedback for me #1070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better (post-parser) errors #1058

Better (post-parser) errors #1058

GuillaumeGomez commented May 24, 2024

djc left a comment

djc May 25, 2024

GuillaumeGomez May 25, 2024

djc May 25, 2024

djc May 25, 2024

djc May 25, 2024

djc May 25, 2024

djc May 25, 2024

GuillaumeGomez May 25, 2024 •

edited

Loading

GuillaumeGomez May 25, 2024

djc May 25, 2024 •

edited

Loading

djc May 25, 2024

GuillaumeGomez May 25, 2024

djc May 25, 2024

GuillaumeGomez May 25, 2024

GuillaumeGomez commented May 27, 2024

djc May 30, 2024

GuillaumeGomez May 30, 2024

GuillaumeGomez commented Jun 7, 2024

Better (post-parser) errors #1058

Better (post-parser) errors #1058

Conversation

GuillaumeGomez commented May 24, 2024

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 7, 2024

GuillaumeGomez May 25, 2024 •

edited

Loading

djc May 25, 2024 •

edited

Loading