[cdytotoro-ctrl]

The generated code for 'each val in expr' evaluated expr inside the same function scope where the loop body declares 'var val' and 'var index'. Because of var hoisting, the loop variable declarations shadowed outer variables of the same name with undefined while expr was evaluated, so a nested 'each val, index in val.length > 1 ? val[1] : []' crashed with 'Cannot read property length of undefined' (Fixes #2733).

The iterable expression is now passed as an argument of the scoping IIFE, so it is evaluated in the enclosing scope (where outer 'val' and 'index' are still live) while the loop body keeps its own scoped variables. The expression is parenthesized so top-level comma expressions still work as a single argument.

Tests: new test case nested-each-shadowing reproduces the issue example; existing each/eachOf/alternate cases all pass.

[rollingversions]

There is no change log for this pull request yet.

Create a changelog

[seebaermichi]

Thanks for this! I tested it against master (c323ed3e), npm pug@3.0.4 and this branch (582ca9a2).

The fix is right. On master and 3.0.4, #2733's template throws Cannot read properties of undefined (reading 'length'), through render and through compileClient alike: the inner loop's hoisted var val is already undefined when val.length > 1 ? val[1] : [] is evaluated. On this branch both render the expected HTML, the same HTML as the template with the inner names renamed. Passing the iterable into the IIFE also fixes two related cases that throw on master:

• an inner key that shadows the outer key used in the iterable: each v, i in lists / each w, i in lists[i]
• a body variable named like the iterable: each x in items / - var items = null

Unchanged from master: each … else over an empty array and an empty object, an object iterable, the iterable is evaluated once, this in the iterable, and a block comment (each x in items /* c */ compiles to (items /* c */)). A trailing // c can't swallow the closing ));, because the lexer already rejects line comments in the each expression (Line comments not allowed in an expression), as on master.

Tests:

• Full suite: 649/651. The 2 failures are the same as on master (V8 error-message wording in error.reporting.test.js).
• run.test.js and run-es2015.test.js: 117/117. No snapshot pins the old }).call(this); shape of each.
• The new nested-each-shadowing case fails without the change in pug-code-gen (Cannot read properties of undefined (reading 'length')) and passes with it.
• Nera's test suites and site builds against this branch are green, with byte-identical output.

Two behaviour changes worth knowing, both because the iterable is now evaluated in the template function rather than inside the IIFE. Neither needs a change in my view:

• An unparenthesized top-level comma now means what the comma operator says. The description says comma expressions "still work as a single argument". They compile, but each x in items, z used to become var $$obj = items, z;, which iterated items (and quietly declared z). Now it is (items, z), which iterates z. With items: [1, 2] master renders <p>1</p><p>2</p>, this branch throws Cannot read properties of undefined (reading 'length'). Master's behaviour was accidental, and I doubt real templates rely on it.
• arguments in the iterable now refers to the template function's arguments instead of the IIFE's (empty) ones.

Out of scope here, but the same defect: each val of (visitEachOf) emits for (const val of <obj>), so each val of val.items inside each val of groups throws Cannot access 'val' before initialization on master, 3.0.4 and this branch. That could be a follow-up PR.

Rolling Versions still says there is no changelog entry. Something like this for pug-code-gen would do: "Evaluate the each iterable before the loop variables are declared, so a nested each can reuse the outer loop's variable names (#2733)".

Looks good to merge to me.