Incorrect ES5 code emitted for closures in loops when noCheck is enabled

[PhoebeSzmucer]

🔎 Search Terms

ES5, noCheck, loop, closures, transpilation, transpileModule

🕗 Version & Regression Information

This broke in either #57934 or #58364

⏯ Playground Link

https://www.typescriptlang.org/dev/bug-workbench/?target=1&noCheck=true#code/PTAEAEDsHsGEAsCmBjA1gLlAFwE4FdEAoECLAQxwHNEtNEBnAVmLHHvmgHcBRAWwEsshQsmiR6WUADNxoALygA2gF0A3ISnQcoABSjxkgG5kANgVDQpSgIwAaUACZ7AZnsAWZQEpQAb0KgA6XEAOgAHPHYdHW85AD5dH1BjM0RQAF9PT3U0kTEJUBwGPBNJBRl6YN4yUJ0ZeXiZaKzc8WgTRGCTaEodQvpirE8gA

💻 Code

// @noCheck: true
// @target: es5
// @showEmit

const fns = [];
for (const value of [1, 2, 3]) {
    fns.push(() => ({ value }));
}
const result = fns.map(fn => fn());
console.log(result)

🙁 Actual behavior

The following incorrect code is produced:

"use strict";
var fns = [];
for (var _i = 0, _a = [1, 2, 3]; _i < _a.length; _i++) {
    var value = _a[_i];
    fns.push(function () { return ({ value: value }); });
}
var result = fns.map(function (fn) { return fn(); });
console.log(result);

Which evaluates to this incorrect value: [{ value: 3 }, { value: 3 }, { value: 3 }]

🙂 Expected behavior

The following should be produced:

"use strict";
var fns = [];
var _loop_1 = function (value) {
    fns.push(function () { return ({ value: value }); });
};
for (var _i = 0, _a = [1, 2, 3]; _i < _a.length; _i++) {
    var value = _a[_i];
    _loop_1(value);
}
var result = fns.map(function (fn) { return fn(); });
console.log(result);

Which would evaluate to: [{ value: 1 }, { value: 2 }, { value: 3 }]

Additional information about the issue

transpileModule(ts, { compilerOptions: { target: ScriptTarget.ES5 } }) started producing incorrect code in TypeScript 5.5, since I believe noCheck: true is the default there.

[ericanderson]

@weswigham / @DanielRosenwasser any thoughts on this?

[jakebailey]

I was looking at this last night; this seems like a flag case missed in #58364 (but I didn't spend enough time on it to actually figure out where or fix anything).

[jakebailey]

Doing more debugging with this test:

// @target: es5

const fns = [];
for (const value of [1, 2, 3]) {
    fns.push(() => ({ value }));
}
const result = fns.map(fn => fn());
console.log(result)

Shows that shouldConvertBodyOfIterationStatement returns true with checking, but false without checking.

(No need to set noCheck or vary it in tests, all tests verify that checking does not change emit, we just didn't have a test for this case.)

[jakebailey]

This seems like something specifically to do with it being { value }. Swap the code to () => value and it works. My money is on something with the symbol we pass into checkIdentifierCalculateNodeCheckFlags for the node, though copying the exact code at the other call site for that does not change things.

[weswigham]

checkIdentifierCalculateNodeCheckFlags is supposed to drop into checkNestedBlockScopedBinding, which should in turn call getEnclosingIterationStatement to get the containing loop and set LoopWithCapturedBlockScopedBinding on it, thus enabling the transform. If the symbol is missing declarations/is the unknown symbol, it definitely would not do so, since it walks up to the iteration statement from the symbol's value declaration and not the usage, though that's a bit strange - in the non-noCheck case, we get the symbol with getResolvedSymbol(node) directly on the identifier, while in the noCheck case we're using getSymbolAtLocation within checkSingleIdentifier - these functions must behave differently on binding pattern identifiers?

[PhoebeSzmucer]

Thank you for a very quick fix!