2.9 regression when assigning a variable inside a loop

[ThiefMaster]

2.9:

>>> jinja2.Template('{% set a = -1 %}{% for x in range(5) %}[{{ a }}:{% set a = x %}{{ a }}] {% endfor %}{{ a }}').render()
u'[:0] [:1] [:2] [:3] [:4] -1'

2.8:

>>> jinja2.Template('{% set a = -1 %}{% for x in range(5) %}[{{ a }}:{% set a = x %}{{ a }}] {% endfor %}{{ a }}').render()
u'[-1:0] [0:1] [1:2] [2:3] [3:4] -1'

---

Originally reported on IRC:

A change in jinja2 scoping appears to affect me, and I'm unsure of the correct fix. Specifically the problem is the assignment of year here: https://github.com/kennethlove/alex-gaynor-blog-design/blob/551172/templates/archive.html#L13-L24

[mitsuhiko]

While the current behavior is incorrect, the old behavior is definitely incorrect as well. The {% set %} tag was never intended to override outer scopes. I'm surprised it did in some cases. Not sure what to do here.

[mitsuhiko]

The behavior I would assume were correct is an output like [-1:0] [-1:1] [-1:2] [-1:3] [-1:4] -1.

[alex]

In this specific case I resolved it by rewriting it to use groupby.

[ThiefMaster]

I have the feeling this is a somewhat common use case - also stuff like {% set found = true %} in a loop and then checking it afterwards. It's surely likely to break things for people when this stops working...

[mitsuhiko]

I'm shocked this worked before. Did this always work?

[ThiefMaster]

apparently yes:

>>> import jinja2
>>> jinja2.__version__
'2.0'
>>> jinja2.Template('{% for x in range(5) %}[{{ a }}:{% set a = x %}{{ a }}] {% endfor %}{{ a }}').render()
u'[:0] [0:1] [1:2] [2:3] [3:4] '

[mitsuhiko]

Great. Because the issue here is that this is absolutely not sound. Which variable is it supposed to override? What if there is a function scope in between. eg: a macro or something like that. I have no idea how to support this now.

[ThiefMaster]

hmm maybe similar to python's scoping assuming no nonlocal was used)? ie don't allow overriding something defined in an outer scope (ie if there's a macro inbetween) but do allow overriding it otherise?

[mitsuhiko]

Apparently before this was caught down with a template assertion error:

>>> Template('{% set x = 0 %}{% for y in [1, 2, 3] recursive %}{{ x }}{% set x = y %}{% endfor %}').render()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
jinja2.exceptions.TemplateAssertionError: It's not possible to set and access variables derived from an outer scope! (affects: x (line 1)

[ThiefMaster]

the behavior seems to be different with/without recursive (2.8.1 gives me an UnboundLocalError error with recursive and '012' without)

[mitsuhiko]

The unbound local error should be resolved on master.

[mitsuhiko]

I'm not sure how to progress here. I really dislike that it was apparently possible to do this sort of thing.

[mitsuhiko]

With the last changes I'm going to close this as "works as intended". If there is further fallout from this we can investigate alternatives again.

[MinchinWeb]

Got sent to this issue (see #656) after this change blew up my blog template on upgrading from v.2.8.1 to v2.9.4.

I was using it keep track if various pieces of data were changing between loop iteration. I was able to fix it because I wrote the original template code (see MinchinWeb/seafoam@8eb7608 and MinchinWeb/seafoam@89d555d), but I doubt I would have been able to otherwise. The new code is harder to follow as the comparisons are now done in-line. For example, my old code (v.2.8.1):

{%- set last_day = None -%}

{% for article in dates %}
    {# ... #}
    <div class="archives-date">
        {%- if last_day != article.date.day %}
            {{ article.date | strftime('%a %-d') }}
        {% else -%}
            &mdash;
        {%- endif -%}
    </div>
    {%- set last_day = article.date.day %}
{% endfor %}

and the new code, with in-line comparisons (v2.9.4):

{% for article in dates %}
    {# ... #}
    <div class="archives-date">
        {%- if ((article.date.day == dates[loop.index0 - 1].date.day) and
                (article.date.month == dates[loop.index0 - 1].date.month) and 
                (article.date.year == dates[loop.index0 - 1].date.year)) %}
                    &mdash;
        {% else -%}
                    {{ article.date | strftime('%a %-d') }}
        {%- endif -%}
    </div>
{% endfor %}

So I just wanted to say that the "feature" (or "hack", if you prefer) is used and is already missed.

If the scoping issues are too complex to figure out sensibly at the moment, could something (at a minimum) be added to the changelog so it bites less people unaware?

[mitsuhiko]

I was not aware this is so widely abused :-/ Annoyingly there is really no way to make this work in any reliable way. However I wonder if we can isolate the common use cases here and introduce a nicer api.

In particular maybe we want to add something like this (loop.changed_from_last):

{% for article in dates %}
    <div class="archives-date">
        {%- if loop.changed_from_last(article.date.day) %}
            {{ article.date | strftime('%a %-d') }}
        {% else -%}
            &mdash;
        {%- endif -%}
    </div>
{% endfor %}

[ThiefMaster]

changed_from_last sounds good - at least functionality-wise, the name itself is a bit awkward IMO. Maybe just changed would be clear enough?

I guess the first element would always be considered "changed" no matter what it is? If that behavior is not OK for someone they could always add a not loop.first check anyway.

[davidism]

Maybe previous_context just holds the entire previous context, rather than trying to decide what the users will do with it.

[ThiefMaster]

just previous or prev? "context" sounds rather confusing in this context (no pun intended)

.....and now I can already imagine someone asking for a next :/

[davidism]

Or maybe an explicit statement for writing outside scope: set_outer or something.

[mitsuhiko]

i was thinking this:

class LoopContextBase(object):

    def __init__(self, recurse=None, depth0=0):
        ...
        self._last_iteration = missing

    def changed(self, *value):
        if self._last_iteration != value:
            self._last_iteration = value
            return True
        return False

[mitsuhiko]

@davidism we cannot do set_outer without breaking the entire scoping system. This would majorly screw up the entire thing.

[davidism]

Yeah, figured that would be the case. I'm anticipating "what if I want to know if the current value is greater than the previous", or something similar. But I like the changed method too.

[pujan14]

@davidism I played around with loops in new jinja 2.9*. with 2.9* even when I create/define a new variable inside a loop it gets cleared/deleted at end of every iteration.
Here is an example.

{% for name in names %}
{% if loop.first %}
{% set list = "" %}
{% endif %}
{% if name.first is defined and name.last is defined and not name.disabled %}
{% set list = list + name.first|string + "-" + name.last|string %}
{% if loop.last %}
{% for item in list.split(' ')|unique %}
{{ item }}
{% endfor %}
{% else %}
{% set list = list + " " %}{% endif %}
{% endif %}
{% endfor %}

This might not be the best way to do this but here from my understanding I am not breaking any scoping rules.

[Carlos-Descalzi]

Had this issue in 2.8 and superior

Here goes a test case:

import unittest
from jinja2 import Template

TEMPLATE1 = """{% set a = 1 %}{% for i in items %}{{a}},{% set a = a + 1 %}{% endfor %}"""

class TestTemplate(unittest.TestCase):

  def test_increment(self):
    items = xrange(1,10)
    expected='%s,' % ','.join([str(i) for i in items])
    t = Template(TEMPLATE1)
    result = t.render(items=items)
    self.assertEqual(expected,result)

unittest.main()

[ThiefMaster]

Use loop.index instead.

[ThiefMaster]

I think providing access to the previous loop value through an attribute of the loop object is the only good solution for this. I just discovered this snippet in our project which couldn't be solved by just checking if the last object is different than the current one and groupby also doesn't work there since it's more than just a trivial item/attribute access to get the key:

{% set previous_date = none %}
{% for item in entries -%}
    {% set date = item.start_dt.astimezone(tz_object).date() %}
    {% if previous_date and previous_date != date -%}
        ...
    {% endif %}
    {% set previous_date = date %}
{%- endfor %}

[mitsuhiko]

Yeah that sounds like an idea.

[davidism]

What about adding set(key, value) and get(key) methods to the loop object? Then people can store whatever they want across the loop.

[ThiefMaster]

Had the same idea, but then couldn't find any non-ugly cases where this would be needed. And I could already see someone asking for setdefault, pop and other dict-like methods.

[mitsuhiko]

@davidism @ThiefMaster i was thinking of just having a storage object available. Like this:

{% set ns = namespace() %}
{% set ns.value = 42 %}
...{% set ns.value = 23 %}

Obviously set can't set attributes at the moment but this could easily be extended. Since no reassignment to the namespace itself happens this is quite safe to implement.

[davidism]

Seems fine to me, it's the same solution as nonlocal for Py 2. Alternatively, automatically set up loop.ns, although that wouldn't be available outside the loop.

[ThiefMaster]

What I dislike with the namespace is that it'll feel very tempting to do {% set obj.attr = 42 %} with obj being something that is not a namespace() - something that I think shouldn't work.

Otherwise it looks like an interesting idea, even though I think previtem / nextitem / changed() cover "simple" cases nicely without the "noise" of having to define a new object in the template.

[mitsuhiko]

@ThiefMaster it would not work. The way I would see this working is that attribute assignments go through a callback on the environment which would only permit modifications to namespace objects.

[ThiefMaster]

ok, so at least no risk of templates causing unexpected side-effects on objects passed to them.

still a bit wary of the things people might do...

{% macro do_stuff(ns) %}
    {% set ns.foo %}bar{% endset %}
    {% set ns.bar %}foobar{% endset %}
{% endmacro %}

{% set ns = namespace() %}
{{ do_stuff(ns) }}

Actually, I think a new block like {% namespace ns %} would be better to define one than a callable - a variable named namespace doesn't sound like something very unlikely to be passed to a template, and while it would probably simply prevent you from using the namespace feature in that template (just like shadowing a builtin in Python) it feels a bit dirty...

[Da-Juan]

Do you have a workaround for this issue or do we have to wait for previtem/nextitem in 2.9.6?
Some of my saltstack templates are broken now.

[davidism]

As has been demonstrated to varying degrees above, you might not even need to do what you're doing. Otherwise, yes, you need to wait if you want to use 2.9. It was never supported before, it just happened to work.

[davidism]

We will not be reverting to the old behavior. While it worked in simple cases, it was not correct and was never documented as supported. While I understand that it is a breaking change, it occurred in a feature release (second number change) which is how we've always managed these changes. Pin the version until a fix is released if you need to keep relying on the old behavior.

Locking this because everything that needs to be said has been said. See #676 and #684 for the fixes that are currently being considered.