Block scoping is too strict (stricter than Jinja) in 3.0.0.

[jgerigmeyer]

There were a number of commits included in 3.0.0 that fixed scoping issues, but now it seems that {% call %} blocks no longer have access to variables set in their parent scope. This differs from the Jinja2 implementation.

In Jinja2, the following code:

{% macro inside() %}
  <div>inside: {{ caller() }}</div>
{% endmacro %}

{% macro outside(var) %}
  <div>outside: {{ var }}</div>
  {% call inside() %}
    {{ var }}
  {% endcall %}
{% endmacro %}

{{ outside('foobar') }}

Outputs:

<div>outside: foobar</div>
<div>inside: foobar</div>

In Nunjucks 3.0.0, the output is:

<div>outside: foobar</div>
<div>inside: </div>

See http://jsfiddle.net/jgerigmeyer/mjpbm13j/

[ChristianAuth]

We got the same problem here. Not being able to access local variables inside of {% call %} blocks seems to break a lot of standard use cases.

I would strongly vote to rollback the strict scoping for {% call %} blocks.

[vecmezoni]

Introduced in 429f636

[ArmorDarks]

Seems to be related to my similar issue #912

[paulmsmith]

This should be fixed but I'd love to see the PR for adding embed functionality progressed which would mean a lot of my use of call can be dropped entirely.

[ArmorDarks]

embed isn't option for those, who works within multiple environments.

[paulmsmith]

@ArmorDarks I get that, does that mean it's dead as a potential feature for Nunjucks? I think feature parity is important, could it be done as an extension perhaps?

[ArmorDarks]

@paulmsmith Well, I'm not a maintainer, I can't tell :)

Personally I'm not very happy with embed, since it overlaps with macros. I think that instead macros should be enhanced. However, I understand needs of those, who goes from Swig to Nunjucks and needs embed, thus I don't vote against it.

I think for now it wasn't merged only because new maintainers didn't have enough time to review PR. My guess.

[ArmorDarks]

Hi guys!

Any updates on this issue?..

[jgerigmeyer]

I dug into this for awhile yesterday, but didn't get very far. Reverting the changes to compiler.js in #667 fixes this issue, but not surprisingly breaks the added tests #667 was intended to fix. The changes in #791 are also related.

I pushed an initial attempt at oddbird@104c46c in case that helps someone else. It fixes one of the test cases, but breaks some others.

[ArmorDarks]

I'm starting to doubt that it's possible to properly support all those test cases.

In JS we don't have such issue with scoping and leaking, because we can control when we want variable to "leak", and when not.

For instance, this will cause intentional leak:

let test = 1
let funcTest = () => { test = 2 }
funcTest()
console.log(test) // will yield 2

And this won't

let test = 1
let funcTest = () => { let test = 2 } // let will create new scoped variable
funcTest()
console.log(test) // will yield 1

In Jinja and Nunjucks we have only set, which always... doing something. We can't choose when we want to create variable in new scope, or change already existing global variable. This makes usage of macros quite limiting in some rare scenarios, when they should change global variable.

Maybe instead of solving this puzzle we should lift this issue to Jinja2 community? Though, Python community doesn't seem to favor JS developers much, so I guess there are very little chances that they will reevaluate concept of set, which were in Jinja done this way for years...

Well, this is only partially related to this issue, but it might make things much simpler, since it will move responsibility for scoping in hand of developer instead of compiler.

[jgerigmeyer]

I think all of these test cases are already supported by Jinja.

[ArmorDarks]

I can only guess, since I never worked with Jinja.

Though, if Jinja works exactly the way all those newer test cases describes, this already means that in Jinja it's impossible to change global variable by macro. Not big loss, though, but still this seem like an oversight in design of the language.

[ArmorDarks]

This issue seems to be related too #679 (comment)

[jgerigmeyer]

Since we have new maintainers (welcome and thanks, @fdintino!), I'd like to bump this bug again. It's a pretty major bug in v3.0.0, and I wasn't able to figure it out after digging around awhile in the code (#906 (comment)). Many thanks to anyone willing to give it a shot!

[fdintino]

I'll try to take a look at this issue over the next weekend.

[paulmsmith]

Wondering what the status of this issue is now?

[ArmorDarks]

Same thing here... This bug makes Nunjucks 3.0.0 unusable...

[fdintino]

@noahlange any chance they your refactoring work addresses this bug?

[noahlange]

@fdintino - not at the moment, sorry. 😕

I'll do some digging and see if I can track this down.

[fdintino]

I have a fix for this. I'll clean it up later today and push it out.

What threw me (and I'm guessing others as well) is that there are tests that test for the wrong behavior for {% set %}, and so fixing this bug was causing those tests to "fail." I verified the correct behavior with jinja2 and amended those tests.

[ArmorDarks]

@fdintino Nice to hear it! Can you point to that wrong test? Just curious.

[fdintino]

@ArmorDarks it was these two tests. The values returned from jinja2 are '1' and '678afterwards: 5', respectively.

[ArmorDarks]

@fdintino Hm, looking on those tests I'm starting to doubt that Jinja's behavior is correct. Shouldn't declaration of variable before scope make any changes in that scope propagate to declared in upper scope variable? In other words, current tests behavior seems to be correct.

Is behavior same in Python? I don't have any experience with Python, so I can't check myself.

[fdintino]

It's a common misconception that Jinja2 follows the conventions of Python everywhere, but that's not generally true. It supports a subset of python syntax, but template-specific constructs like blocks, macros, includes, template scoping, and the set and call tags don't have obvious analogous in Python. The Jinja2 docs are pretty explicit in stating that writes with {% set %} should not modify variables in the outer scope (with the exception of if/else tags): see the Jinja2 docs on assignments, specifically the second section titled “Scoping Behavior.” It's definitely intentional. Since nunjucks lacks a consistent specification for how scope should act (and not unrelatedly there are a lot of bugs with scope) my preference would be to defer to Jinja's behavior generally.

I will note that Twig does not isolate writes inside {% for %} blocks, so I'm open to making those an exception the same way {% if %} and {% else %} tags are. As long as the behavior is well considered.

[ArmorDarks]

Ah, now I see that Jinja2 specification about set scoping within for, thanks. Well, that's slightly strange to me, since it violates general concept of how variables scopes works in languages, which doesn't have scope-assignment operators (like var and let in JS), but, well, okey.