djc / askama

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

Scope changes inside `block`, preventing to call macros which should be in scope #989

Closed GuillaumeGomez closed 4 months ago

GuillaumeGomez commented 5 months ago

The bug can be reproduced with this code:

File macro.html:

{% macro what(a, b) %}
    {{ a }} = {{ b }}
{% endmacro %}

File base.html:

{% import "macro.html" as bla %}

{% block header %}{% endblock %}

File main.rs:

use askama::Template;

#[derive(Template)]
#[template(source = r#"{% extends "base.html" %}

{% block header %}
{% call bla::what(1, 2) %}
{% endblock %}"#, ext = "txt")]
struct F;

fn main() {
    assert_eq!(F.render().unwrap(), r#"Hello You"#);
}

This code will fail with this error:

error: no import found for scope "bla"
 --> src/main.rs:4:10
  |
4 | #[derive(Template)]
  |          ^^^^^^^^
  |
  = note: this error originates in the derive macro `Template` (in Nightly builds, run with -Z macro-backtrace for more info)
djc commented 5 months ago

So you're expecting that macro scopes imported in the base are available in the child template?

Seems kinda reasonable, let's see how much complexity in the generator it requires?

On the other hand, it doesn't seem too crazy to make imports file-scoped...

djc commented 5 months ago

So this is supported by Tera, right? What about Jinja? I'm still on the fence about whether this is a good idea -- file-scoped imports actually seem pretty reasonable to me, similar to how things work in Rust.

And, this is actually easy to work around, right?

GuillaumeGomez commented 5 months ago

No it's not unfortunately, especially when extends comes in. It means you have to re-import everything you need inside your block, which is pretty bad, especially when the information is already around.

As for original jinja support, I tested this code and it works:

foo.py:

from jinja2 import Environment, PackageLoader, select_autoescape
env = Environment(
    loader=PackageLoader("foo"),
    autoescape=select_autoescape()
)

template = env.get_template("mytemplate.html")
template.render(the="variables", go="here")

mytemplate.html:

{% extends "extend_and_import.html" %}
{%- import "macro.html" as m2 -%}

{%- macro another(param) -%}
--> {{ param }}
{%- endmacro -%}

{% block header -%}
{{ m1.twice(1) }}
{{ m2.twice(2) }}
{{ another(3) }}
{%- endblock -%}

extend_and_import.html:

{% import "macro.html" as m1 %}

{% block header %}{% endblock %}

macro.html:

{%- macro twice(param) -%}
{{param}} {{param}}
{% endmacro %}

(you'll note that scopes can be accessed with . and not ::, funny discovery)

GuillaumeGomez commented 5 months ago

The whole explanation is here. And I also need to not allow access to variables which are not defined in the block in my PR (currently it does allow it, which is invalid based on the description).

djc commented 5 months ago

I wasn't suggesting per-block imports but per-file imports. It feels to me like macros and inheritance should operate independently.

GuillaumeGomez commented 5 months ago

Apparently jinja2 is fine with this? We're supposed to inherit whatever the "parents" imported.

GuillaumeGomez commented 5 months ago

Anyway, updated #991 to prevent inheriting variables.