fix: deoptimize right of for of stmt when assign in for of

[Kingwl]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Closes #4901

Description

[Kingwl]

Not sure it's the correct fix. Seems a little weird about access (and type cast) parent node in child node.

[XiSenao]

Do we need to move out of it if there are no side effects inside for...of?

const list = [ { value: 1 } ];

for (const item of list) {
    // item.value = 0;
}

if (list[0].value === 0) {
    console.log(42);
}

[lukastaegert]

for..of loops are never removed at the moment. The reason is that Rollup cannot reliably detect side effects in custom iterators. For that reason, we decided to always keep them until someone implements something better.

The solution you implemented here is not completely off, but it is not 100% straightforward. The deoptimizePath method is always called for the left side of for-of statements because different values are assigned to the variable, even if it is not a declaration.

So what you did is equivalent to always deoptmizing the left side of the statement. Which I think is fine! It is simple and fixes the bug for now. But then, it should (and can) be done directly in ForOfStatement. Just add this.right.deoptimizePath(UNKNOWN_PATH) to applyDeoptimizations in ForOfStatement. Would you be willing to apply these changes?

Also, as the test checks for a bug, you should not use a form test. It is easy to update the test in bulk and overlook if the test is correct or broken. Instead, convert it to a function test. Those tests have access to Node's assert as a global variable, so do something like this

const list = [ { value: 1 } ];

 for (const item of list) {
     item.value = 0;
 }

assert.ok(list[0].value === 0 ? true : false);

This currently becomes

assert.ok(false);

without the fix.

[lukastaegert]

I created #4949 to implement this solution.

[rollup-bot]

This issue has been resolved via #4949 as part of rollup@3.20.7. You can test it via npm install rollup.