fix: macro parameter with default value overshadowed by unrelated macro

[shepherdwind]

close #774

There is only a little code to fix that issue. If their is some problem, I will fix soon. Or maybe their is some more good solution, I don't sure if this realization is good or not.

[carljm]

Hi, thanks for working on this!

I haven't looked at this as closely as you have, I'm sure, but this fix doesn't feel right to me. I wonder if the real problem is that in _compileMacro we don't keep the run-time frame and the compile-time frame in sync. We create a brand-new isolated run-time frame, but we don't do the same for the compile-time frame.

[shepherdwind]

The compile-time frame can not have the same value with run-time frame. Because the macro parameter is dynamic, the parameter will have a value only when macro calling. So, the parameter can only run on run-time. In compile-time the parameter should be a normal variable. I tried set frame variable, but I found that I can only set it as a string or some other type value. I don't find a way to set frame value as a variable. So I change the compileSymbol to make sure the parameter be a variable in compile-time.

[carljm]

Yeah, but I mean instead of frame = frame.push() maybe we should be creating a new isolated frame in compile-time, like we do for run-time?

[shepherdwind]

Oh, yes I tried that just replace frame = frame.push() to frame = new Iframe(), all tests is pass.

This solution is seems more useful, which do not need to hack the compileSymbol method.

[carljm]

this is a bit misleading, this text in fact should not be output, in this case

[shepherdwind]

this test output asset to equal foo. my last post code is

{# macro1 and macro2 definition #}
{% macro macro1() %}
{% endmacro %}

{% macro macro2(text="default") %}
{{macro1}}
{% endmacro %}

{# calling macro2 #}
{{macro2("this should be outputted") }}

in this case, output will be the macro1 source code. But this output have no problem, because {{macro1}} should output this function toString. so I delete that post, which is mistake.

After I change it to {{macro1()}}.

[carljm]

my comment here was just that using the text "this should be outputted" in your test, when that text actually shouldn't be output, is confusing :-) the test and the assert are both correct.

[carljm]

Hmm, well that's interesting. I was convinced by your last post (now deleted?) about why that wouldn't work :-) Do you understand how your second test still passes here?

[carljm]

What I mean is, after this change to an isolated frame inside the macro, I don't quite understand how macro1 is even visible within macro2 in the "should get right value when macro include macro" test. I must be missing something obvious...

[shepherdwind]

First, let us take a look how this bug happen.

When parse macro1, code like this

{% macro macro1() %}
{% endmacro %}

compiler will output code like this

// this is macro function body
var macro_t_1 = runtime.makeMacro([], [], () => ());
context.addExport("macro1");
context.setVariable("macro1", macro_t_1);

At the same time, compile-time frame will add varibale

var funcId = this._compileMacro(node, frame);

// Expose the macro to the templates
var name = node.name.value;
frame.set(name, funcId);

So, compiler continue parse, when deal with

{% macro macro2(macro1="default") %}
{{macro1}}
{% endmacro %}

When symbol macro1 appear, frame.lookup will return functionId macro_t_1 , so after compile this will output code

frame.set("macro1", kwargs.hasOwnProperty("macro1") ? kwargs["macro1"] : "default");
var t_4 = "";
_4 += runtime.suppressValue(macro_t_1, env.opts.autoescape);

First macro1 argument will add to run-time frame, but the {{macro1}} has already became symbol macro_t_1. That why this bug happen.

Now, why the second test will pass. When compileMacro compile-time frame is a new frame, so when macro2 compile, frame.lookup('macro1') will return null(now we can not lookup the parent frame), so code generate right

 var t_4 = "";
 t_4 += runtime.suppressValue((lineno = 0,
   colno = 96,
  runtime.callWrap(runtime.contextOrFrameLookup(context, frame, "macro1"), "macro1", context, [])), env.opts.autoescape);

In my first test case, code generate is just the same as the next one. Only diff difference is the local argument value set

// first test case, key is macro1
frame.set("macro1", kwargs.hasOwnProperty("macro1") ? kwargs["macro1"] : "default");
// next test case, key is text
frame.set("text", kwargs.hasOwnProperty("text") ? kwargs["text"] : "default");

So, at the first test, symbol macro1 will found as string, and the next test case, macro1 not find the current frame, will lookup from the context(context.setVariable("macro1", macro_t_1)), so return a function.

[carljm]

Thanks for the explanation! Looks great, merging.