Nunjucks: Treating "for" loops as a separate scope does not behave reasonably
This code:
{% set lastDate = '' %}
{% for item in items %}
{{ renderEvent(event, item, lastDate) }}
{% set lastDate = item.date %}
{% endfor %}
Does not behave as a programmer accustomed to [fill in pretty much any language here] would expect.
The renderEvent macro receives '' on every iteration, because it sees the lastDate variable declared in the outer scope.
The second set statement creates a separate lastDate variable in an inner scope but it is too late for renderEvent to see it.
If we get rid of the first "set" entirely, the code starts working; renderEvent passes undefined on the first pass, and the value of the second "set" on each pass thereafter.
This works, but it requires us to lean heavily on nunjuck's tolerance for passing undefined variables, and if we needed to initialize to something other than "nothing" we'd be out of luck.
IMHO a for loop should never create a new variable scope in a language that has no distinction between declaring a variable ("var foo") and updating it.
boutell
All 38 comments
It looks to me like this is an issue with the implementation of set rather than for.
mattbasta
on 9 Dec 2013
Possibly. The documentation does say that "for" creates a new scope.
boutell
on 9 Dec 2013
"If set is used at the top-level, it changes the value of the global template context. If used inside scoped blocks, like for, include, and others, it only modifies the current scope."
boutell
on 9 Dec 2013
Yeah, it seems that set isn't seeing the outer definition for some reason. If I have time today I'll look and see what's happening.
mattbasta
on 9 Dec 2013
Thanks, much appreciated!
Now I hate to say this, but...
I do wonder what the value of having "for" create a new scope is, if it'll
just write to outer scopes anyway if they already contain the variable.
That seems to be the same thing as not having "for" create a scope at all.
Which I think would be a sensible change.
But I thought it should be considered carefully (:
On Mon, Dec 9, 2013 at 2:32 PM, Matt Basta [email protected] wrote:
Yeah, it seems that set isn't seeing the outer definition for some
reason. If I have time today I'll look and see what's happening.—
Reply to this email directly or view it on GitHubhttps://github.com/jlongster/nunjucks/issues/166#issuecomment-30165097
.
Tom Boutell
P'unk Avenue
215 755 1330
punkave.com
window.punkave.com
boutell
on 9 Dec 2013
On closer inspection, it seems this might be happening because the compiler decides to create a second variable to store the value from the second set in. It seems like it's caused by this line:
https://github.com/jlongster/nunjucks/blob/master/src/compiler.js#L501
That frame.get should probably be a frame.lookup. Other places in the compiler use lookup instead in similar contexts:
https://github.com/jlongster/nunjucks/blob/master/src/compiler.js#L312
That should be all that's needed here. There's another potential issue, though, in that anything that does a frame lookup to retrieve the value is going to get the outer frame's value...though I'm not sure I can think of anything that would trigger that behavior. I imagine it's probably possible (or the code to update the frame probably wouldn't exist), but it's a bit late for me to investigate one way or the other right now.
mattbasta
on 10 Dec 2013
Right... I think @jlongster might want to chime in because it seems to be a
language design decision.
On Tue, Dec 10, 2013 at 2:49 AM, Matt Basta [email protected]:
On closer inspection, it seems this might be happening because the
compiler decides to create a second variable to store the value from the
second set in. It seems like it's caused by this line:https://github.com/jlongster/nunjucks/blob/master/src/compiler.js#L501
That frame.get should probably be a frame.lookup. Other places in the
compiler use lookup instead in similar contexts:https://github.com/jlongster/nunjucks/blob/master/src/compiler.js#L312
That should be all that's needed here. There's another potential issue,
though, in that anything that does a frame lookup to retrieve the value is
going to get the outer frame's value...though I'm not sure I can think of
anything that would trigger that behavior. I imagine it's probably possible
(or the code to update the frame probably wouldn't exist), but it's a bit
late for me to investigate one way or the other right now.—
Reply to this email directly or view it on GitHubhttps://github.com/jlongster/nunjucks/issues/166#issuecomment-30205547
.
Tom Boutell
P'unk Avenue
215 755 1330
punkave.com
window.punkave.com
boutell
on 10 Dec 2013
I'm at a work week right now so pretty busy but I promise to look at this later this week.
jlongster
on 11 Dec 2013
Of course thanks
On Dec 11, 2013 5:16 PM, "James Long" [email protected] wrote:
I'm at a work week right now so pretty busy but I promise to look at this
later this week.—
Reply to this email directly or view it on GitHubhttps://github.com/jlongster/nunjucks/issues/166#issuecomment-30370008
.
boutell
on 12 Dec 2013
This also acts strange:
{% if override %}
{% set value = override %}
{% endif %}
{{ value }}
If there is value, but no override, it outputs nothing.
This works correctly:
{% set value = value %}
{% if override %}
{% set value = override %}
{% endif %}
{{ value }}
So it works like var hoisting in javascript. It shouldn't.
sunflowerdeath
on 18 Feb 2014
@sunflowerdeath Thanks. Gonna catch up on a lot of nunjucks bugs this week so I'll take a look.
jlongster
on 18 Feb 2014
This is still causing some serious confusion around here (:
boutell
on 4 Apr 2014
This is our number one nunjucks trouble spot, causing a lot of convoluted code to be written. Others have opened various duplicates of the issue.
If you are too busy can you give any hints as to where I might look?
boutell
on 7 Jul 2014
I have not been good at keeping up with nunjucks issues recently. Thanks for bringing this up again. I _promise_ you that tomorrow, July 8th, I will take a close look at this and either fix it or post a detailed description on how we can work on it. There's a bunch of issues I need to look at, and I'll start with this one.
jlongster
on 7 Jul 2014
Thank you very much!
On Mon, Jul 7, 2014 at 4:54 PM, James Long [email protected] wrote:
I have not been good at keeping up with nunjucks issues recently. Thanks
for bringing this up again. I _promise_ you that tomorrow, July 8th, I
will take a close look at this and either fix it or post a detailed
description on how we can work on it. There's a bunch of issues I need to
look at, and I'll start with this one.—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-48239438.
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 7 Jul 2014
The issue is directly related to how the compiler attempts to optimize lookups. The compiler keeps track of scoping at compile-time, and if it already knows the location of a variable, instead of emitting a contextOrFrameLookup it just directly inserts the runtime variable that's already declared (usually a temporary variable like t_1).
The problem was that compileSet aggressively added the left-hand side var name into the scope, so subsequent lookups of that variable would statically get resolved to a single run-time var, not a contextorFrameLookup.
Obviously, that is very broken. In @sunflowerdeath's example that if code would basically turn into:
if(expr) {
var t_1 = override;
}
output(t_1);
It badly renames lookups that shouldn't be. In the for loop example, it initially creates a temporary t_1 variable and set its to the empty string, and the compiler renames all references to lastDate to t_1. But the set inside the for loop only updates lastDate in the current frame!
I also tweaked it to use frame.lookup instead of frame.get. I don't think it was very related, but that could fix edge cases in nested for loops.
jlongster
on 9 Jul 2014
for loops in nunjucks produce a new scope because all the variables like loop.index are inserted and only available there. It fixes the problem that JS has where for(var i=0;;){} introduces i into the outer scope. nunjucks tends to use let semantics more than var (well, it's sort of mix)
It's a bit Pythonic, really. If you use set to change a variable that was set in an outer scope, it will correctly change it in that scope. Otherwise, set only introduces the variable in the current scope.
We could just have set update variable in the global scope always. I don't know if there's a good enough argument for that though. Now that I've fixed this, everything should work nicely, with the additional benefit that scoping will help you avoid certain errors.
If you wanted a global variable, you could just do {% set foo = null %} at the global scope or something. A little verbose, but you can see how vars are used explicitly.
This is just a templating language though, so if enough people thought set should operate on global scope by default, we can discuss it in another issue.
jlongster
on 9 Jul 2014
Actually, I was wrong. I thought a frame.set call at run-time walked up each scope and set a variable if it existed in a parent scope. But it just sets it on the current frame, so any set with only be for the current scope.
Before, it kind of worked to set across scopes because the aggressive optimizer inlined the lookups and it worked because the initial var was hoisted (even if in a for). Now that we fixed that, it won't work. I think I'm going to change frame.set to walk up the parent scopes and look for an existing variable, and set it at that scope.
jlongster
on 9 Jul 2014
Thanks for working on it.
Hmm, so should I test with what you've committed or do you think it's still
a bit busted at this point?
On Tue, Jul 8, 2014 at 11:50 PM, James Long [email protected]
wrote:
Actually, I was wrong. I thought a frame.set call at run-time walked up
each scope and set a variable if it existed in a parent scope. But it just
sets it on the current frame, so any set with only be for the current
scope.Before, it kind of worked to set across scopes because the aggressive
optimizer inlined the lookups and it worked because the initial var was
hoisted (even if in a for). Now that we fixed that, it won't work. I
think I'm going to change frame.set to walk up the parent scopes and look
for an existing variable, and set it at that scope.—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-48427143.
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 9 Jul 2014
You can try it if you want; I think for most cases it'll work fine. I'm going to change the set resolving right now though, so you can wait a little bit too.
jlongster
on 9 Jul 2014
@boutell ok, fixed in master. Can you try it with all scenarios you can think of and let me know if it behaves as you expect? It's basically like Python now. A set will only modify it's current frame. However, if the variable has been previously set before in an outer scope, it will resolve to that scope and change that variable.
jlongster
on 9 Jul 2014
I think changing for loops to not create a new frame is too big of a breaking change. It's a pretty basic semantic and I wouldn't be surprised if several templates broke if we changed that. With the new set behavior, though, I think everything works rather intuitively.
jlongster
on 9 Jul 2014
So practically speaking, what a developer needs to know is that this will
work:
{%set somethingWasCool = false %}
{% for x in y %}
{% if isCool(x) %}
{% set somethingWasCool = true %}
{% endif %}
{{ x }}
{%endfor %}
{% if somethingWasCool %}
Something was cool!
{% endif %}
... Will work, but this will NOT work:
{% for x in y %}
{% if isCool(x) %}
{% set somethingWasCool = true %}
{% endif %}
{{ x }}
{%endfor %}
{% if somethingWasCool %}
Something was cool!
{% endif %}
Because if the variable is not already set it is created only in the scope
of the for loop.
Correct?
On Wed, Jul 9, 2014 at 10:44 AM, James Long [email protected]
wrote:
I think changing for loops to not create a new frame is too big of a
breaking change. It's a pretty basic semantic and I wouldn't be surprised
if several templates broke if we changed that. With the new set behavior,
though, I think everything works rather intuitively.—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-48480423.
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 9 Jul 2014
Yep, that is correct. All of that should behave how you think.
jlongster
on 9 Jul 2014
OK. Unfotunately with the previous bug the opposite was true, so we wrote
code like:
{%- for field in fields -%}
{%- if (interesting(field)) -%}
{% if notFirst %}, {% endif %}
{% set notFirst = true %}
{{ field.name }}
{%- endif -%}
{%- endfor -%}.
And that worked! Now it won't.
So we will need to change all of these occurrences when you npm publish
this fix. But, it's worth it to have the more sensible behavior, since a
single scope per macro doesn't seem to be an option.
So if you could kindly give me a heads up when you npm publish (:
On Wed, Jul 9, 2014 at 10:53 AM, James Long [email protected]
wrote:
Yep, that is correct. All of that should behave how you think.
—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-48483269.
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 9 Jul 2014
I'm not sure I see how that breaks; you're just using notFirst within the same for loop. The for loop create a new frame, but that frame is re-used across loops, so that code should work I think?
What do you mean by "single scope per macro"? When a macro is called, a new frame is created for the macro too.
jlongster
on 9 Jul 2014
I think you're probably right on that one.
On Wed, Jul 9, 2014 at 12:15 PM, James Long [email protected]
wrote:
I'm not sure I see how that breaks; you're just using notFirst within the
same for loop. The for loop create a new frame, but that frame is re-used
across loops, so that code should work I think?What do you mean by "single scope per macro"? When a macro is called, a
new frame is created for the macro too.—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-48497130.
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 10 Jul 2014
In the case we are facing here...
{% for image in images %}
{% if thumbs.selected == image.id %}
{% set highlight = image.url %}
{% endif %}
<img src="image.thumbUrl" />
{% endfor %}
<img src="{{ highlight }}" />
In this case we need to, using the loop, identify the selected thumb to show it afterwards in a bigger size.
But it does not work, because the highlight variable is gone!
I know that the set token creates the new variable inside the for scope, but how can we get that to work? I see the issue is closed, but couldn't figure out if this is a bug, or if there is a way to do it, but it is not documented anywhere!
felipenmoura
on 4 Dec 2014
@felipenmoura from my understanding in the comments here you define the variable outside the loop so it knows to edit the 'global' var rather than create one local to the loop... like so:
{% set highlight = '' %}
{% for image in images %}
{% if thumbs.selected == image.id %}
{% set highlight = image.url %}
{% endif %}
<img src="image.thumbUrl" />
{% endfor %}
<img src="{{ highlight }}" />
garygreen
on 5 Dec 2014
We tried that, we too thought what would be the behaviour, but this did not work, either!
When the {% set highlight = image.url %} happens inside the loop, it ignores the "highlight" variable from outside! And it was "killed" in the end of each loop, becoming "" after the endfor statement!
The version we are using here is 1.0.4.
felipenmoura
on 9 Dec 2014
I still don't think creating new scopes inside "for" loops makes sense in a
language without explicit declarations. It's just not possible to correctly
infer what is supposed to happen.
On Tue, Dec 9, 2014 at 3:56 PM, Felipe Nascimento de Moura <
[email protected]> wrote:
We tried that, we too thought what would be the behaviour, but this did
not work, either!
When the {% set highlight = image.url %} happens inside the loop, it
ignores the "highlight" variable from outside! And it was "killed" in the
end of each loop, becoming "" after the endfor statement!
The version we are using here is 1.0.4.—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-66356099.
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 9 Dec 2014
yes, I agree!
That's what I am talking about! It looks to be happening now!
I want it to use the variable from outside the loop, and change it, but it is _not_ doing that, instead, is setting the value only inside the loop, reseting it to the previous value, after the loop ends! :(
felipenmoura
on 15 Dec 2014
Sorry to necro this, but this does not seem resolved. Behaviour is still as @felipenmoura described, and modifications to a variable taking place inside a for loop are not accessible from outside it, even if the variable was originally declared outside the for loop.
tortilaman
on 1 Jan 2018
Tortilaman, are you using nunjucks with Apostrophe? What versions of
Apostrophe and of Nunjucks are you winding up with?
On Mon, Jan 1, 2018 at 5:07 AM, tortilaman notifications@github.com wrote:
Sorry to necro this, but this does not seem resolved. Behaviour is still
as @felipenmoura https://github.com/felipenmoura described, and
modifications to a variable taking place inside a for loop are not
accessible from outside it, even if the variable was originally declared
outside the for loop.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-354645931,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAB9fdNxfofcQGfimZFpHOn4VC1s3_ENks5tGK5QgaJpZM4BS4XW
.
--
THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 2 Jan 2018
I am using Apostrophe. On my most recent install, NPM grabbed:
Apostrophe 2.4.4
Nunjucks 2.5.2
In Package.json it's just calling for "apostrophe": "^2.0.0", as is the default with the CLI I think.
tortilaman
on 8 Jan 2018
OK. I ask because Nunjucks is up to 3.x, and Apostrophe's dependency was
not bumped because we haven't had time to examine if there are any bc
breaks. You might want to test this with a standalone, non-Apostrophe
program that uses Nunjucks 3.x to see if the bug exists that version of
Nunjucks.
On Mon, Jan 8, 2018 at 4:18 PM, tortilaman notifications@github.com wrote:
I am using Apostrophe. On my most recent install, NPM grabbed:
Apostrophe 2.4.4
Nunjucks 2.5.2—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-356099626,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAB9fcgr0OO0cNrwdWWiojuXAy_5CJFpks5tIoY6gaJpZM4BS4XW
.
--
THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 8 Jan 2018
Also thinking of using an npm-shrinkwrap.json to update nunjucks if the issue is resolved.
Thanks for the info, I'll see what happens.
tortilaman
on 9 Jan 2018
Sounds good. If you can reproduce the issue with 3.x it should stay here in
the nunjucks github, otherwise it needs to come over to the apostrophe
github and leave the poor nunjucks team in peace (:
On Mon, Jan 8, 2018 at 7:04 PM, tortilaman notifications@github.com wrote:
Also thinking of using an npm-shrinkwrap.json to update nunjucks if the
issue is resolved.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mozilla/nunjucks/issues/166#issuecomment-356136572,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAB9ffPWwtNCe3N3wxPwTmNNiDqNw2Mbks5tIq0PgaJpZM4BS4XW
.
--
THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT
P'UNK AVENUE | (215) 755-1330 | punkave.com
boutell
on 9 Jan 2018
Related issues
mintyPT
·
4Comments
gaboesquivel
·
4Comments
wslx520
·
4Comments
thisKai
·
3Comments
ricordisamoa
·
5Comments
Most helpful comment
I still don't think creating new scopes inside "for" loops makes sense in a
language without explicit declarations. It's just not possible to correctly
infer what is supposed to happen.
On Tue, Dec 9, 2014 at 3:56 PM, Felipe Nascimento de Moura <
[email protected]> wrote:
*THOMAS BOUTELL, *DEV & OPS
P'UNK AVENUE | (215) 755-1330 | punkave.com