Repository: mozilla/nunjucks (js)

Issue:
Macro recursion issue: Variables in macros are not stored on the stack
Consider the following template that renders a list of fully qualified property names from a JSON schema:

    {% macro typeTable(node, prefix = "", name = "" %}
      {% set nodeName = prefix + ("." if prefix) + name) %}
      {% if nodeName %}
        <tr>
          <td><code>{{ nodeName | safe }}</code></td>
          <td>{{ node.type }}</td>
          <td>
            {% markdown %}{{ node.description }}{% endmarkdown %}
          </td>
        </tr>
      {% endif %}
      {% if node.type == "object" %}
        {% for propName, propSpec in node.properties %}
          {{ typeTable(propSpec, nodeName, propName) }}
        {% endfor %}
      {% endif %}
    {% endmacro %}

    <body>
      <table class='table table-bordered'>
        <thead>
          <tr>
            <th>Qualified name</th>
            <th>Type</th>
            <th>Description</th>
          </tr>
        </thead>
        <tbody>
          {{ typeTable(schema) }}
        </tbody>
      </table>
    </body>


Where `schema` is for example the following JSON schema

    {
      "$schema": "http://json-schema.org/draft-04/schema",
      "type": "object",
      "properties": {
        "id": {
          "type": "string",
          "description": "The ID of the node"
        },
        "name": {
          "type": "string",
          "description": "The name of the node"
        },
        "status": {
          "type": "object",
          "description": "The status of the node",
          "properties": {
            "vm": {
              "type": "string"
            },
            "app": {
              "type": "string"
            }
          }
        },
        "url": {
          "type": "string",
          "format": "uri",
          "description": "The URL of the node"
        }
      }
    }

The expected result is a table with the following "qualified names":

    id
    name
    status
    status.vm
    status.app
    url

However the variable `nodeName` seems to scoped per declaration (i.e. there is only one instance) and not scoped per invocation. This means that changes to `nodeName` in recursive calls of `typeTable` can be seen by the caller. This results in the following "qualified names":

    id
    id.name
    id.name.status
    id.name.status.vm
    id.name.status.vm.app
    id.name.status.vm.app.url


Pull request:
Do not resolve up when setting a variable in macro.
This PR should fix #577 and the "macro" part of issue #561.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f58f134..9f2f400 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,11 @@ Changelog
 master (unreleased)
 -------------------
 
+* Fix `{% set %}` scoping within macros.
+  Fixes [#577](https://github.com/mozilla/nunjucks/issues/577) and
+  the macro portion of [#561](https://github.com/mozilla/nunjucks/issues/561).
+  Thanks Ouyang Yadong. Merge of [#653](https://github.com/mozilla/nunjucks/pull/653).
+
 * Add support for named `endblock` (e.g. `{% endblock foo %}`). Thanks
   ricordisamoa. Merge of [#641](https://github.com/mozilla/nunjucks/pull/641).
 
diff --git a/src/compiler.js b/src/compiler.js
index 646a93f..911b13b 100644
--- a/src/compiler.js
+++ b/src/compiler.js
@@ -822,7 +822,7 @@ var Compiler = Object.extend({
             '[' + argNames.join(', ') + '], ',
             '[' + kwargNames.join(', ') + '], ',
             'function (' + realNames.join(', ') + ') {',
-            'frame = frame.push();',
+            'frame = frame.push(true);',
             'kwargs = kwargs || {};',
             'if (kwargs.hasOwnProperty("caller")) {',
             'frame.set("caller", kwargs.caller); }'
diff --git a/src/runtime.js b/src/runtime.js
index 25c8d36..47f0399 100644
--- a/src/runtime.js
+++ b/src/runtime.js
@@ -7,10 +7,11 @@ var Obj = require('./object');
 // we know how to access variables. Block tags can introduce special
 // variables, for example.
 var Frame = Obj.extend({
-    init: function(parent) {
+    init: function(parent, isolate) {
         this.variables = {};
         this.parent = parent;
         this.topLevel = false;
+        this.isolate = isolate;
     },
 
     set: function(name, val, resolveUp) {
@@ -20,7 +21,7 @@ var Frame = Obj.extend({
         var obj = this.variables;
         var frame = this;
 
-        if(resolveUp) {
+        if(resolveUp && !frame.isolate) {
             if((frame = this.resolve(parts[0]))) {
                 frame.set(name, val);
                 return;
@@ -66,8 +67,8 @@ var Frame = Obj.extend({
         return p && p.resolve(name);
     },
 
-    push: function() {
-        return new Frame(this);
+    push: function(isolate) {
+        return new Frame(this, isolate);
     },
 
     pop: function() {


Test patch:
diff --git a/tests/compiler.js b/tests/compiler.js
index 40a2e3b..45e0eef 100644
--- a/tests/compiler.js
+++ b/tests/compiler.js
@@ -485,6 +485,44 @@
                   { name: 'james' },
                   'FooInclude james');
 
+            equal('{% macro foo() %}{% set x = "foo"%}{{ x }}{% endmacro %}' +
+                  '{% set x = "bar" %}' +
+                  '{{ x }}' +
+                  '{{ foo() }}' +
+                  '{{ x }}',
+                  'barfoobar');
+
+            equal('{% macro setFoo() %}' +
+                  '{% set x = "foo" %}' +
+                  '{{ x }}' +
+                  '{% endmacro %}' +
+                  '{% macro display() %}' +
+                  '{% set x = "bar" %}' +
+                  '{{ setFoo() }}' +
+                  '{{ x }}' +
+                  '{% endmacro %}' +
+                  '{{ display() }}',
+                  'foobar');
+
+            // This test checks that the issue #577 is resolved.
+            // If the bug is not fixed, and set variables leak into the
+            // caller scope, there will be too many "foo"s here ("foofoofoo"),
+            // because each recursive call will append a "foo" to the
+            // variable x in its caller's scope, instead of just its own.
+            equal('{% macro foo(topLevel, prefix="") %}' +
+                  '{% if topLevel %}' +
+                    '{% set x = "" %}' +
+                    '{% for i in [1,2] %}' +
+                    '{{ foo(false, x) }}' +
+                    '{% endfor %}' +
+                  '{% else %}' +
+                    '{% set x = prefix + "foo" %}' +
+                    '{{ x }}' +
+                  '{% endif %}' +
+                  '{% endmacro %}' +
+                  '{{ foo(true) }}',
+                  'foofoo');
+
             finish(done);
         });