Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macro in child template isn't found inside of block #136

Closed
SergioBenitez opened this issue Nov 7, 2022 · 5 comments · Fixed by #140
Closed

Macro in child template isn't found inside of block #136

SergioBenitez opened this issue Nov 7, 2022 · 5 comments · Fixed by #140
Labels
bug Something isn't working

Comments

@SergioBenitez
Copy link
Contributor

SergioBenitez commented Nov 7, 2022

Consider the following (also here):

{% extends template %}
{% macro foo() %}inside foo{% endmacro %}
{% block title %}{{ foo() }}{% endblock %}
{% block body %}new body{% endblock %}

This gives the following error:

Error {
    kind: UnknownFunction,
    detail: "foo is unknown",
    name: "macro-extends.txt",
    line: 3,
}

unknown function: foo is unknown (in macro-extends.txt:3)
------------------------------ macro-extends.txt ------------------------------
   1 | {% extends template %}
   2 | {% macro foo() %}inside foo{% endmacro %}
   3 > {% block title %}{{ foo() }}{% endblock %}
     i                     ^^^^^ unknown function
   4 | {% block body %}new body{% endblock %}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
No referenced variables
-------------------------------------------------------------------------------

Removing the extends results in the template compiling without error.

I considered whether this was intended behavior, but the syntax docs make no mention of blocks having some newly defined scope. And if they do, then how would I go about invoking foo(), defined in a child template, inside of a block in said child?

@mitsuhiko mitsuhiko added the bug Something isn't working label Nov 7, 2022
@SergioBenitez
Copy link
Contributor Author

It seems that if I instead define the macro inside of the parent template, things again work as expected. Is this the intention? jinja2 doesn't work this way, and the docs don't mention any particular deviation with regard to this issue.

@mitsuhiko
Copy link
Owner

This is indeed a bug. The behavior in Jinja2 is preferrable. For context for the above template this is what the Jinja2 semantics are (dump from the compiler output):

def root(context, missing=missing, environment=environment):
    resolve = context.resolve_or_missing
    undefined = environment.undefined
    concat = environment.concat
    cond_expr_undefined = Undefined
    if 0: yield None
    parent_template = None
    l_0_template = resolve('template')
    l_0_foo = missing
    pass
    parent_template = environment.get_template((undefined(name='template') if l_0_template is missing else l_0_template), None)
    for name, parent_block in parent_template.blocks.items():
        context.blocks.setdefault(name, []).append(parent_block)
    def macro():
        t_1 = []
        pass
        t_1.append(
            'inside foo',
        )
        return concat(t_1)
    context.exported_vars.add('foo')
    context.vars['foo'] = l_0_foo = Macro(environment, macro, 'foo', (), False, False, False, context.eval_ctx.autoescape)
    yield from parent_template.root_render_func(context)

def block_title(context, missing=missing, environment=environment):
    resolve = context.resolve_or_missing
    undefined = environment.undefined
    concat = environment.concat
    cond_expr_undefined = Undefined
    if 0: yield None
    _block_vars = {}
    l_0_foo = resolve('foo')
    pass
    yield str(context.call((undefined(name='foo') if l_0_foo is missing else l_0_foo), _block_vars=_block_vars))

def block_body(context, missing=missing, environment=environment):
    resolve = context.resolve_or_missing
    undefined = environment.undefined
    concat = environment.concat
    cond_expr_undefined = Undefined
    if 0: yield None
    _block_vars = {}
    pass
    yield 'new body'

blocks = {'title': block_title, 'body': block_body}
debug_info = '1=14&2=17&3=28&4=39'

Jinja2 basically executes the parent top level code even after {% extends %}. I assume that in MiniJinja the example would still work if the macro is declared before {% extends %} but I did not try.

@mitsuhiko
Copy link
Owner

mitsuhiko commented Nov 7, 2022

Memo to myself, I confirmed that a macro declared before exists shows up:

{%- macro foo() %}inside foo{% endmacro %}
{%- extends template %}
{%- block title %}{{ foo() }}{% endblock %}
{%- block body %}new body{% endblock %}

I think fundamentally the logic in Jinja2 in Python is implemented in a pretty abstruse way and I'm not sure I want to fully replicate it, but I do think that macros belong after {% extends %} so I want to find a good evaluation model where some logic can be found that lets them be placed there.

  • The execution model of MiniJinja is basically that after {% extends %} the blocks are loaded, and the root render func is now invoked.
  • In case of Jinja2 different logic applies depending on {% extends %} is in a conditional block or not. If it's known that in all cases something is extended, then statically all output is hidden but code is still executed until in the end the root render func is invoked. If the extending was conditional, then all output functions are checked if an extend has taken place yet.

In MiniJinja extending can also be conditional but so far at least it has immediately meant that once the block is encountered, the rest of the template is not executed.

MiniJinja case is best understood this way:

Block: "body"
     0: EmitRaw("new body")
Block: "title"
     0: CallFunction("foo", 0)
     1: Emit
Block: "<root>"
     0: Lookup("template")
     1: LoadBlocks   # <- this ends execution. The execution continues in the root of the parent

     # all the code from here is unreachable
     2: EmitRaw("\n")
     3: Jump(6)
     4: EmitRaw("inside foo")
     5: Return
     6: BuildMap(0)
     7: LoadConst([])
     8: BuildMacro("foo", 4, false)
     9: StoreLocal("foo")
    10: EmitRaw("\n")
    11: CallBlock("title")
    12: EmitRaw("\n")
    13: CallBlock("body")

@mitsuhiko
Copy link
Owner

mitsuhiko commented Nov 8, 2022

I think there are two interesting options available. One is to follow the Jinja2 model directly where the code until the end of the template after the {% extends %} tag is executed with output disabled. This means that in terms of variable declarations first the child template runs, then the parent template runs and the blocks pull in the variables from the context.

The second option would be for blocks to basically get a closure. That way they see their own declarations first.

Templates

Take the following templates:

layout.html:

{% set var = "from index" %}
{% block a %}{{ var }}{% endblock %}
{% block b %}{% endblock %}

child.html:

{% extends "layout.html" %}
{% set var = "from child" %}
{% block b %}{{ var }}{% endblock %}

Output

In Jinja2 the output of the above thing would be

from index
from index

If we were to implement blocks to have closures, the output instead would be

from index
from child

The question here is if it's better to stick to Jinja2 compatibility or if it would be more logical to implement closures here. Counter argument to the closures is that some changes from the parent template at render time are usually expected to be visible:

{% for item in navigation %}
  {% block nav %}{{ item }}{% endblock %}
{% endfor %}

If the child template used closures, then item would not be visible. At the very least it would not be allowed to be defined so that it functions.

@mitsuhiko
Copy link
Owner

I think I am leaning towards not doing closures now and sticking with the Jinja2 execution model, as much as I dislike it. The logistics around capturing a closure are surprisingly complex and the benefits are not clear to me. So I would say for now I consider this a bugfix and will catch up to the Jinja2 execution model with a note to revisit this if closure capturing would be an interesting change.

MiniJinja already departs from some Jinja2 mechanics that were not great, but in this case I'm not convinced that closures are the correct solution here, given the clear user expectation that the block caller's context is implicitly passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants