djc / askama

Type-safe, compiled Jinja-like templates for Rust
Apache License 2.0
3.25k stars 213 forks source link

Fix invalid `if let` code generation #1037

Closed GuillaumeGomez closed 2 months ago

GuillaumeGomez commented 2 months ago

Fixes #1033.

We cannot test the generated code since the if-let chain feature is still unstable. So I thought about two possibilities to still test this:

Here are the cases to be checked:

{% if let Some(query) = s && !query.is_empty() %}{{query}}{% endif %}

{% if let Some(s) = s && !s.is_empty() %}{{s}}{% endif %}

{% if let Some(s) = s %}{{ s }}{% endif %}

Where s is Option<String>.

Kijewski commented 2 months ago

The change looks good to me. :+1:

Currently we test on stable and beta. Maybe we could add nightly, too, and enable the tests only on nightly?

GuillaumeGomez commented 2 months ago

After thinking some more about it, I don't think checking with nightly will be what we want: we want to check the generated code here, not that it works on nightly.

I'll open an issue to discuss about how to test generated content because I think it's a problem that we need to solve somehow.

djc commented 2 months ago

Maybe the generator should desugar let chains into something that works on stable? It's okay if the generated code isn't the prettiest.

GuillaumeGomez commented 2 months ago

I don't think we should implement our own condition lexer, we should just rely on the rust compiler to handle this part for us. For example, it becomes particularly tricky in cases like:

if let Some(x) = y || z == "x" && whatever

It requires to generate else if z == "x" and in both if branches to have also whatever.

GuillaumeGomez commented 2 months ago

I opened https://github.com/djc/askama/issues/1038 about this.

djc commented 2 months ago

(Would prefer if you don't merge non-trivial PRs without my approval.)

GuillaumeGomez commented 2 months ago

Sorry, noted for future contributions.