Skip to content

Generate a TokenStream rather than a String#558

Merged
Kijewski merged 10 commits intoaskama-rs:masterfrom
GuillaumeGomez:tokenstream
Aug 6, 2025
Merged

Generate a TokenStream rather than a String#558
Kijewski merged 10 commits intoaskama-rs:masterfrom
GuillaumeGomez:tokenstream

Conversation

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator

Part of #420.

This PR allows to greatly improve compile-time errors by using Span. Even better: even a compiler error occurs, it shows which part of the jinja code generated it.

BIG limitations with this PR:

  • It doesn't support path, only src.
  • It doesn't work with sub-templates (ie includes or extends).

As a first step though, I think it's good enough.

@GuillaumeGomez GuillaumeGomez requested a review from Kijewski August 3, 2025 20:56
Comment thread askama_derive/src/generator/expr.rs Fixed
Comment thread askama_derive/src/generator/expr.rs Fixed
Comment thread askama_derive/src/generator/expr.rs Fixed
DisplayWrap::Unwrapped
}

// FIXME: This function should have a `Span`, but `cond.target` isn't `WithSpan`.

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
Comment thread askama_derive/src/generator/expr.rs Fixed
Comment thread askama_derive/src/generator/node.rs Dismissed
match self {
Self::Literal(lit) => lit.span(),
Self::Literal(lit) => Some(lit.span()),
// FIXME: How do we get the span of the included file?

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
Comment thread askama_derive/src/integration.rs Fixed
Comment thread askama_derive/src/integration.rs Fixed
Comment thread askama_derive/src/integration.rs Dismissed
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator Author

PR is now ready for review! Took me way too long to finally finish it and rebase was a nightmare. ^^'

Comment thread askama_derive/src/ascii_str.rs Dismissed
}
} else if pos != 0 {
buf.write("} else {");
// FIXME: Should have a span.

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
}
if has_cond {
let block_buf = block_buf.into_token_stream();
// FIXME Should have a span.

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@Kijewski
Copy link
Copy Markdown
Member

Kijewski commented Aug 6, 2025

I removed impl BufferArg for {String, &str, Arguments}, so spans are better kept. (I wouldn't have done it, if I had known beforehand how much work this would going to be. :) ) Implementing the PR must have been horrible!

* No need to manually escape raw identifiers
* Allow a few warnings
@GuillaumeGomez
Copy link
Copy Markdown
Collaborator Author

Yep, it was quite the nightmare. ^^'

* prefix temporaries in `{{ expr }}` with `__askama`
* replace custom `spanned!(..)` with `quote_spanned!(..)`
* replace `buf.write_tokens(quote_spanned!(..))` with `quote_into!(..)`
The invocation is not only bad for performance, but also not necessarily
correct:
* The output of `TokenStream::to_string()` is free to add spaces between
  `self` and `.`, or before `self`.
* The `.` might belong to `..` or `..=`.
}
buf.write('>');
let tmp = tmp.into_token_stream();
// FIXME: use a better span

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@Kijewski
Copy link
Copy Markdown
Member

Kijewski commented Aug 6, 2025

Sorry, my commits are terribly unorganized. :-/

I only found a possible bug with {% for _ in self..X %} (which will probably never be encountered in the wild), otherwise everything looks very good. 👍

Copy link
Copy Markdown
Member

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Could you please review my commits?

@GuillaumeGomez
Copy link
Copy Markdown
Collaborator Author

I can't approve because it's my PR, github doesn't allow it. But changes look good to me, thanks!

@Kijewski Kijewski merged commit 3a67787 into askama-rs:master Aug 6, 2025
42 checks passed
@Kijewski
Copy link
Copy Markdown
Member

Kijewski commented Aug 6, 2025

Thank you for your work, this change is great!

Hopefully someday soon #![feature(proc_macro_expand)]+#![feature(proc_macro_span)]+#![feature(proc_macro_value)] (or another proc-macro accessible include_bytes!() feature) will be stable. Then we can add spans for included files, too.

@Kijewski Kijewski mentioned this pull request Aug 4, 2025
@GuillaumeGomez GuillaumeGomez deleted the tokenstream branch August 6, 2025 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants