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

2.9 regression when assigning a variable inside a loop #641

Closed
ThiefMaster opened this issue Jan 7, 2017 · 65 comments

Comments

@ThiefMaster
Copy link
Member

commented Jan 7, 2017

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 mitsuhiko self-assigned this Jan 7, 2017

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

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

@alex

This comment has been minimized.

Copy link

commented Jan 7, 2017

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

@ThiefMaster

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

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

@ThiefMaster

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2017

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

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

The unbound local error should be resolved on master.

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 8, 2017

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

mitsuhiko added a commit that referenced this issue Jan 8, 2017

Implement consistent scoping for sets in loops
While technically this applies to any scope and not just for loops
it comes up most commonly in the context of for loops.  This now
defines the behavior for scoping in a way that is consistent but
different than it was in the past.  There is an ongoing conversation
if we should keep it that way or not.

References #641

mitsuhiko added a commit that referenced this issue Jan 8, 2017

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 8, 2017

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.

@mitsuhiko mitsuhiko closed this Jan 8, 2017

foosel added a commit to foosel/OctoPrint that referenced this issue Jan 9, 2017

Pinning Jinja to <2.9 for now
Variables defined in an outer scope can no longer be set from an inner scope (see
pallets/jinja#641). Regardless of whether that is right or wrong, we can't control if
people are using such constructs in their plugins, which versions of Jinja >= 2.9 would
now break out of the blue, regardless of OctoPrint version. That is unacceptable sadly
and requires pinning for now, until plugin authors have had a chance to adapt
accordingly.

Also see #1697.
@MinchinWeb

This comment has been minimized.

Copy link

commented Jan 12, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

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 %}

@mitsuhiko mitsuhiko reopened this Jan 12, 2017

@ThiefMaster

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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

@ThiefMaster

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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

@mitsuhiko

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

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

This comment has been minimized.

Copy link

commented Jan 25, 2017

@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

This comment has been minimized.

Copy link

commented Jan 27, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

Use loop.index instead.

@ThiefMaster

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

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

This comment has been minimized.

Copy link
Member

commented Jan 31, 2017

Yeah that sounds like an idea.

ThiefMaster added a commit to ThiefMaster/jinja that referenced this issue Feb 1, 2017

@davidism

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

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

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

@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

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

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 added a commit to ThiefMaster/jinja that referenced this issue Feb 1, 2017

@ThiefMaster

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

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

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

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...

ThiefMaster added a commit to ThiefMaster/jinja that referenced this issue Feb 2, 2017

rcaelers added a commit to rcaelers/workrave that referenced this issue Feb 7, 2017

Revert to Ninja 2.8 because of regression.
See pallets/jinja#641

Signed-off-by: Rob Caelers <rob.caelers@gmail.com>
@Da-Juan

This comment has been minimized.

Copy link

commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

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.

@pallets pallets locked and limited conversation to collaborators Apr 5, 2017

@davidism davidism closed this Jul 5, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
You can’t perform that action at this time.