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

Respect explicitly-set undefined or null values in a frame. #481

Closed
wants to merge 1 commit into from

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Jul 21, 2015

This fixes #478, which is a nasty subtle bug that leads to very strange and difficult-to-diagnose behaviors in calling template macros with arguments whose default value is none.

The core of the problem is nunjucks' loose handling of null and undefined in symbol value lookups. Symbol values are looked up at runtime using contextOrFrameLookup. If the frame lookup returns null or undefined, contextOrFrameLookup considers that a "not found" and looks up the name in the context instead. Similarly, with stacked frames, if an undefined or null value is found for the name in one frame, it ignores that and goes up to the parent frame instead. This means that you effectively cannot have a variable explicitly set to a value of undefined or null in a scope - if a parent scope (frame or context) has some other value for that symbol name, it will be used instead.

This pull request changes that behavior to instead use hasOwnProperty to determine whether a variable is present in a given frame or not, and never go up the frame stack (or fall back to context) if the variable is present, regardless of its value.

(If #480 is accepted, this PR could be changed to still consider undefined the same as "not found", but support null. That would be enough to support variables being set to none, which is really the motivating use case here.)

@carljm carljm mentioned this pull request Jul 21, 2015
@jgerigmeyer
Copy link
Contributor

This does fix the test case described in #478, but seems to introduce new bugs of its own. Consider the following slightly-modified test case:

{% macro inner(class=none) %}
  <div class="{{ class }}">{{ caller() }}</div>
{% endmacro %}

{% macro outer(class=none) %}
  <div class="{{ class }}">
    {% call inner() %}
      {{ class }}
    {% endcall %}
  </div>
{% endmacro %}

{{ outer(class='outer') }}

Jinja2 outputs as expected:

<div class="outer">
  <div class="None">
    outer
  </div>
</div>

But nunjucks (with this PR) outputs:

<div class="outer">
  <div class="">
  </div>
</div>

@carljm
Copy link
Contributor Author

carljm commented Jul 23, 2015

Thanks for the feedback; I'll look into that.

@carljm
Copy link
Contributor Author

carljm commented Feb 17, 2016

@jgerigmeyer The "new" bug you mention in your follow-up here isn't new with this PR, it's the existing behavior of nunjucks and unrelated to this PR. It's a combination of two things: 1) in Jinja2 none renders as None, whereas in nunjucks it renders as the empty string -- not sure if I even consider this a bug that needs to be fixed, and 2) In nunjucks you can't reference vars from enclosing scopes from within a {% call %} block, whereas in Jinja2 you can -- this is I think related to #664.

@carljm
Copy link
Contributor Author

carljm commented Feb 17, 2016

I pushed a simpler fix for #478 in ba5e514. With the macro-default-arg part of the issue gone, I decided that the more JS-like behavior here would be to continue to consider a value of undefined as equivalent to a variable that is, well, undefined. So I only changed the behavior for none / null. Which makes the fix quite a bit simpler, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argument from caller macro is incorrectly passed through to callee macro?
2 participants