fix:added check for forXstatement pattern

[wlawt]

Q	A
Fixed Issues?	Fixes #11272
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

Issue Ref: Private Name in the left ForOfStatement is not transformed, #11272

The problem was that the private label #a runs before the for-of transform, and that has been fixed by adding a pattern check at the start of the if statement. Test cases included the original one from #11272 and the following:

Input

class Rainbow {
  tColors = ['Red', 'Orange', 'Yellow', 'Green', 'Blue', 'Indigo', 'Violet'];
  #color;

  PrintColors() {
    for (this.#color of this.tColors) {
      console.log(this.#color)
    }
  }
}

Output

var _color = new WeakMap();

var Rainbow = /*#__PURE__*/function () {
  "use strict";

  function Rainbow() {
    babelHelpers.classCallCheck(this, Rainbow);
    babelHelpers.defineProperty(this, "tColors", ['Red', 'Orange', 'Yellow', 'Green', 'Blue', 'Indigo', 'Violet']);

    _color.set(this, {
      writable: true,
      value: void 0
    });
  }

  babelHelpers.createClass(Rainbow, [{
    key: "PrintColors",
    value: function PrintColors() {
      for (babelHelpers.classPrivateFieldDestructureSet(this, _color).value of this.tColors) {
        console.log(babelHelpers.classPrivateFieldGet(this, _color));
      }
    }
  }]);
  return Rainbow;
}();

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d07e916:

Sandbox	Source
trusting-snowflake-igbsi	Configuration
priceless-fire-zm5yv	Configuration

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/23819/

[nicolo-ribaudo]

In the example it should be this.#color and not this.color, right?

[JLHwung]

We should also check if node is the left of the ForXStatement. Consider this case

for (const el of this.#arr);

this.#x should not be transformed into _destructureSet(this, x).

[wlawt]

alright, just to clarify its 1st checking if its a for-of statement then checking if the next node is left of the for-of ?

[JLHwung]

If a path is a ForXStatement, how can its parentPath be an AssignmentPattern? Note that the children of an Pattern is always an expression. A statement can not be an expression.

Here is an AST example of for (o.p of arr);

// for (o.p of arr);
{
  "type": "ForOfStatement",
  "await": false,
  "left": {
    "type": "MemberExpression",
    "object": { "type": "Identifier", "name": "o" },
    "property": { "type": "Identifier", "name": "p" }
   },
  "right": { "type": "Identifier", "name": "arr" }
}

The parentPath here refers to the NodePath of ForOfStatement. NodePath is a wrapper of the AST node that provides parent association with other NodePath. In this PR We would like to make sure that member, which refers to MemberExpression here, is indeed the left of the underlying AST node of parentPath.

So we can use

parentPath.isForXStatement() && parentPath.node.left === node

The query function isForXStatement accepts an additional parameter as query, so the code above can be simplified as

parentPath.isForXStatement({ left: node })

[wlawt]

ok, when I run the test I seem to get the same results, which shouldn't happen?

input

class D {
  #arr;
  f() {
    for (const el of this.#arr);
  }
}

output

var _arr = new WeakMap();

var D = /*#__PURE__*/function () {
  "use strict";

  function D() {
    babelHelpers.classCallCheck(this, D);

    _arr.set(this, {
      writable: true,
      value: void 0
    });
  }

  babelHelpers.createClass(D, [{
    key: "f",
    value: function f() {
      for (var el of babelHelpers.classPrivateFieldGet(this, _arr)) {
        ;
      }
    }
  }]);
  return D;
}();

[JLHwung]

The output is expected. Can you add a test case from the original issue?

class D {
  #arr;
  f() {
    for (const this.#arr of [1, 2]);
  }
}

[jridgewell]

These should be reversed.

Please also add a test for for-in.

[JLHwung]

It is a regression check on the first draft implementation: https://github.com/babel/babel/pull/11703/files/d4c0a5e8b75306f918cf2a2307e4f65130f9ea05#diff-decaf745088978f783d453709a8a8479

[jridgewell]

Ooo, ok. So we need 2 new test cases (for-of and for-in) with it appearing in the left.

[hzoo]

👍 can just do it in the next line, or leave a comment that it shouldn't be transformed

[hzoo]

personally I think just doing them all in the same class is fine

for (const el of this.#arr);
for (this.#arr of [1, 2]);
for (this.#arr in [1,2,3]);

the empty arr seems to be the same case?

and I would rename 1-helpermemberexpressionfunction to something like for-of-member-expression or something

[wlawt]

for sure, will remember for next time!

[JLHwung]

Awesome! Thank you @wlawt!

[kaicataldo]

Looks like there are failing tests - do you mind fixing those up?

Failing tests

[wlawt]

Looks like there are failing tests - do you mind fixing those up?

@existentialism said it had to do with an upstream dep breaking the tests, not sure how I'd go about those

[JLHwung]

CI failure is unrelated and will be fixed by #11709.

[existentialism]

@wlawt great work! thank you!