"Convert forEach to for-of" breaks with Maps (refactoring does not examine type of operand)

[six-transit]

Describe the bug

The "convert forEach to for-of" appears to act blindly on a pattern-matching basis, but not all .forEach() methods are created equal! For example, Maps have a forEach method with a different callback signature. Refactoring map.forEach(value => { ... to for(const value of map) { ... will break code!

How to reproduce

With this code:

const animalPowerLevels = new Map([["dog", 5000], ["cat", 5000.01]]);
animalPowerLevels.forEach(powerLevel => { console.log(Math.floor(powerLevel)) });

Perform the "convert forEach to for-of" action on animalPowerLevels.forEach. The resulting code will look like this:

for (const powerLevel of animalPowerLevels) { console.log(Math.floor(powerLevel)) }

...which in TypeScript results in the following error:

Argument of type '[string, number]' is not assignable to parameter of type 'number'. ts(2345)

This happens since for-of for Maps iterates over key-value pairs.

Expected behavior

"Convert forEach to for-of" should take the type of the operand into account, where possible. For the above example, these would be acceptable results:

for (const [, powerLevel] of animalPowerLevels) { console.log(Math.floor(powerLevel)) }

for (const powerLevel of animalPowerLevels.values()) { console.log(Math.floor(powerLevel)) }

With the latter being preferred in the case where only the value was being used (ie. Map.forEach with a single-argument callback).

In the case where Map.forEach is called with a two-argument callback, like this:

animalPowerLevels.forEach((powerLevel, animalName) => { console.log(Math.floor(powerLevel)) });

The expected result would be this:

for (const [animalName, powerLevel] of animalPowerLevels) { console.log(Math.floor(powerLevel)) }

Additional information

• Version of the extension impacted: v6.9.1

I haven't checked, but there may be other built-ins with forEach methods that will result in broken code if this refactoring is applied to them.

Also, arguably this refactoring should not work based on simple pattern matching in the first place. Anything that's not a built-in but that has a forEach method should probably not be refactorable with this action -- the action should only work for built-ins. Even assuming other objects with forEach methods have [@@iterator] defined and are iterable with for-of, there's no guarantee the semantics would be the same -- the forEach callback arg and the iterable values could be totally unrelated for userland code.

[nicoespeon]

Thank you for the very detailed report, that will be super helpful!

The refactoring is using the AST to detect the forEach. An Array's forEach isn't represented with a special node though, so I understand the current implementation…

But it seems to me that it's lacking some validation rules. In particular, we should check that the MemberExpression object has the Array type… which may be tricky to determine. We probably need to rely on TS to do so, so we can tell if something has a built-in forEach method or not.

abracadabra/src/refactorings/convert-for-each-to-for-of/convert-for-each-to-for-of.ts

Lines 93 to 96 in 5846910

	if (!t.isExpressionStatement(path.parent)) return;
	if (!t.isMemberExpression(path.node.callee)) return;
	if (!t.isIdentifier(path.node.callee.property, { name: "forEach" }))
	return;

It's a non-trivial bug to solve. Thanks for reporting. I will give this a try 👍

[nicoespeon]

For the record: I spent a bit of time on this issue. What we need to do is to detect the type of the callee object to determine if it's an Array, a Map, or something else.

We have a TypeChecker that could do that. But it's quite experimental and, to be frank, quite slow. If we start using it, then we will have the same issue as #416 and we will need to remove the Quick Fix for this refactoring. Also, that won't even be the panacea because the current TypeChecker can't infer types that are coming from other files 😬

I think we should have a better TypeChecker first. Then, we will be able to do this.