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(es/compat): Fix handling of private members in optional chaining pass #7610

Merged
merged 9 commits into from
Jul 28, 2023

Conversation

komyg
Copy link
Contributor

@komyg komyg commented Jun 30, 2023

Description:

Fix: #7561

Related issue (if exists):
#7561

@CLAassistant
Copy link

CLAassistant commented Jun 30, 2023

CLA assistant check
All committers have signed the CLA.

@komyg komyg force-pushed the fix/7561/optional-chaining branch from c8bcf45 to dcef8e6 Compare July 3, 2023 12:36
@komyg komyg force-pushed the fix/7561/optional-chaining branch from dcef8e6 to c4ac964 Compare July 20, 2023 14:46
test() {
(_this = this) === null || _this === void 0
? void 0
: _class_private_field_get(_this.y , _x);
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong.

class Foo {
  #x;
  test() {
    this === null || this === void 0 ? void 0 : this.y.#x;
  }
}

Please try locally using

{
  "plugins": [
    "@babel/plugin-transform-optional-chaining"
  ]
}

This is how I got the expected output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @kdy1,

On the original issue: #7561, @jridgewell says that this is the expected output:

// …
var _x = new WeakMap();
class Foo {
    test() {
        (_this = this) === null || _this === void 0
          ? void 0
          : _class_private_field_get(_this.y , _x);
    }
    // …
}

Also, isn't your output the same as mine, but without the transformation for the private field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: I thought that the main problem was that we were not transforming the private field.

Copy link
Member

Choose a reason for hiding this comment

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

No. Expected output of optional-chaining is mine. What you uploaded is output of full transforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, @kdy1, I don't think I understand your comment.

Here is the problem that I want to solve with this PR (please note that I haven't been able to actually solve it yet, this is why you only saw a test): whenever we have an optional chaining expression with a private field, the private field is not transformed (we don't replace it with: _class_private_field_get).

This is what I understood from these issues: #7561 and #7559.

Regarding your comment:

No. Expected output of optional-chaining is mine. What you uploaded is output of full transforms.

Do you mean that I've put the test in the wrong place? Since it is inside the optional chaining module, it shouldn't be testing private fields? If so, where would be the most appropriate place to put it?

Or do you mean that I am solving the wrong problem?

Copy link
Member

Choose a reason for hiding this comment

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

whenever we have an optional chaining expression with a private field, the private field is not transformed

This is wrong. Private fields are transformed by another pass.

Copy link
Member

Choose a reason for hiding this comment

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

The optional chaining pass should produce

class Foo {
  #x;
  test() {
    this === null || this === void 0 ? void 0 : this.y.#x;
  }
}

and another pass is responsible for the rest.

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, there are two problems?

One for the optional chaining and another for tge private field?

Copy link
Member

Choose a reason for hiding this comment

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

No, private field transforms are working correctly

Copy link
Contributor Author

@komyg komyg Jul 26, 2023

Choose a reason for hiding this comment

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

I've updated the test as you requested. I think it is correct now.

@komyg komyg force-pushed the fix/7561/optional-chaining branch from 796605b to 1b18e8a Compare July 26, 2023 16:57
@komyg komyg marked this pull request as ready for review July 26, 2023 16:58
@komyg komyg force-pushed the fix/7561/optional-chaining branch from 1b18e8a to aded348 Compare July 26, 2023 16:59
@komyg komyg requested a review from kdy1 July 26, 2023 17:02
@komyg
Copy link
Contributor Author

komyg commented Jul 26, 2023

Hey @kdy1, I believe I have solved the problem. Could you review it please?

@@ -303,3 +303,16 @@ fn fixture_loose(input: PathBuf) {
Default::default(),
);
}

#[testing::fixture("tests/optional-chaining-private-var/issue-7561/input.js")]
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to add a new function with testing::fixture

Copy link
Member

Choose a reason for hiding this comment

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

Just add a test file to the correct directory.

Copy link
Contributor Author

@komyg komyg Jul 27, 2023

Choose a reason for hiding this comment

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

I think I've added it to the correct folder, now. Could you check, please?

@kdy1 kdy1 changed the title Fix/7561/optional chaining fix(es/compat): Fix handling of private members in optional chaining pass Jul 27, 2023
@komyg komyg requested a review from kdy1 July 27, 2023 12:21
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Thank you, nice work!


swc-bump:

  • swc_ecma_transforms_compat

@kdy1 kdy1 enabled auto-merge (squash) July 28, 2023 03:32
@kdy1 kdy1 self-assigned this Jul 28, 2023
@kdy1 kdy1 added this to the Planned milestone Jul 28, 2023
Copy link
Collaborator

@swc-bot swc-bot left a comment

Choose a reason for hiding this comment

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

Automated review comment generated by auto-rebase script

@kdy1 kdy1 merged commit 7ba7b6e into swc-project:main Jul 28, 2023
249 checks passed
@komyg komyg deleted the fix/7561/optional-chaining branch July 28, 2023 11:48
@kdy1 kdy1 modified the milestones: Planned, v1.3.72 Jul 28, 2023
kdy1 added a commit to kdy1/swc that referenced this pull request Aug 15, 2023
@kdy1
Copy link
Member

kdy1 commented Aug 15, 2023

I'm reverting this commit with #7812

kdy1 added a commit to kdy1/swc that referenced this pull request Aug 15, 2023
kdy1 added a commit to kdy1/swc that referenced this pull request Aug 15, 2023
kdy1 added a commit that referenced this pull request Aug 15, 2023
**Related issue:**

 - Reverts #7610.
 - Closes #7798.
 - Closes #7789.
 - Opens #7561.
@swc-project swc-project locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Private property after an optional chain transforms into nonsense code
4 participants