Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix:added check for forXstatement pattern #11703

Merged
merged 6 commits into from Jun 12, 2020

Conversation

wlawt
Copy link
Contributor

@wlawt wlawt commented Jun 10, 2020

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
Copy link

codesandbox-ci bot commented Jun 10, 2020

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
Copy link
Collaborator

babel-bot commented Jun 10, 2020

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

@nicolo-ribaudo
Copy link
Member

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

@existentialism existentialism added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 10, 2020
@@ -337,6 +337,7 @@ const handle = {
// [MEMBER = _VALUE] = ARR -> [_destructureSet(MEMBER) = _VALUE] = ARR
// [...MEMBER] -> [..._destructureSet(MEMBER)]
if (
parentPath.isForXStatement() ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@@ -337,6 +337,8 @@ const handle = {
// [MEMBER = _VALUE] = ARR -> [_destructureSet(MEMBER) = _VALUE] = ARR
// [...MEMBER] -> [..._destructureSet(MEMBER)]
if (
(parentPath.isForXStatement() &&
parentPath.parentPath.isAssignmentPattern({ left: node })) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);
  }
}

class D {
#arr;
f() {
for (const el of this.#arr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be reversed.

Please also add a test for for-in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure, will remember for next time!

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you @wlawt!

wlawt and others added 2 commits June 11, 2020 15:44
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
kaicataldo
kaicataldo previously approved these changes Jun 11, 2020
@kaicataldo
Copy link
Member

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

@kaicataldo kaicataldo dismissed their stale review June 11, 2020 20:31

Failing tests

@wlawt
Copy link
Contributor Author

wlawt commented Jun 11, 2020

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
Copy link
Contributor

JLHwung commented Jun 11, 2020

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

@existentialism existentialism merged commit 183acba into babel:master Jun 12, 2020
@existentialism
Copy link
Member

@wlawt great work! thank you!

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Sep 12, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Class Fields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private Name in the left ForOfStatement is not transformed
8 participants