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

Transform destructuring private #14304

Merged
merged 35 commits into from May 17, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Feb 24, 2022

Q                       A
Fixed Issues? Transforms destructuring-private, closes #13889
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link babel/website#2631
Any Dependency Changes?
License MIT

In this PR we implement destructuring-private transform. The design goal is:

  1. Transpile as little as possible
  2. Ensure the execution order

The PR is based on #14236. In #14236 we refactor destructuring-transform a bit so that some routines can be reused by the this plugin.

Technical details

The plugin is composed of two parts, index.ts and util.ts.

The first part converts destructuring in function params / catch clause / forXstatement for (const { #x: x } of arr); to variable declarations / assignment expressions. This part is largely based on current destructuring-transform. Note that unlike the general destructuring transform, we don't handle the export declarations because private destructuring must be placed inside a class while an export declaration must be in top level.

The second part tranpiles private destructuring in variable declarations / assignment expressions.

We first implement a simplified generator-based DFS traverser, which traverses given pattern and ignores expressions within computed keys and initializers. The traverser will be paused when it finds a private key, and reports an index path.

Then we split given pattern around the private keys. The reported index path is handled from left to right: Any patterns before private key is guaranteed to be private-key free and thus yielded to the caller. Any patterns after private key is uncharted territory and pushed to a stack so we will revisit later. The destructuring private key are then transformed and its value, which may be a new pattern, are also pushed to a stack. The transform ends when the stack is drained.

Examples: How we split patterns:

Object patterns

var { [f()]: a, #x: { #y: y }, [g()]: b } = this;

are split into

var { [f()]: a } = this,
{ #y: y } = this.#x,
{ [g()]: b } = this;

Object patterns with rest element

var { [f()]: a, #x: { #y: y }, [g()]: b, ...c } = this;

are split into

var m1;
var { [m1 = f()]: a } = this,
{ #y: y } = this.#x,
{ [g()]: b, ...c } = babelHelpers.objectWithoutProperties(this, [m1]);

This approach is a bit deviated from how we handle non-static property keys in the general transform: In that plugin all non-static property keys are memoised before the pattern, which is not spec-compliant because computed keys should be evaluated right after the previous pattern elements are handled. See the ordering fixtures test for more examples.

Array patterns

var [{ #x: x = f() }, y] = iterator();

are transformed into

var [m1, m2] = iterator(),
  { #x: x = f() } = m1,
  y = m2;

This approach assumes that any expressions within the array pattern, i.e.f(), does not interfere with the iterable iterator(). This assumption is generally held as array destructuring is meant to consume an iterator, and if one mutates the iterable in the middle, then one had better use a loop statement instead to convey such purposes. To be complete spec-compliant, we will have to pause the iterator step by step and interpolates transpiled destructuring codes, like @nicolo-ribaudo mentioned in a gist. We didn't implement such transform as it is bloated and the general destructuring transform has not supported this case yet.

Playground

You can test the behaviour in the REPL mentioned by babel-bot below. You have to enable the stage-2 preset, currently the REPl can not disable class properties transforms as they are also included in stage-2 preset.

@JLHwung JLHwung added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Destructuring Private Fields labels Feb 24, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 24, 2022

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

@JLHwung JLHwung force-pushed the transform-destructuring-private branch 8 times, most recently from e24d1ee to 2282605 Compare March 2, 2022 01:05
@@ -0,0 +1,4 @@
{
"plugins": ["proposal-destructuring-private"],
"presets": ["typescript"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is disabled because presets always run after plugins. The order is preferred for preset-env and stage plugins, but not for typescript preset since it should run before other es plugins.

@JLHwung JLHwung marked this pull request as ready for review March 2, 2022 15:39
@JLHwung JLHwung force-pushed the transform-destructuring-private branch 2 times, most recently from fcbf81d to 8fc3f59 Compare March 8, 2022 14:37
@nicolo-ribaudo nicolo-ribaudo added this to the v7.18.0 milestone Mar 24, 2022
Comment on lines 27 to 16
export default declare(function ({
assertVersion,
assumption,
}: {
assumption: (string) => boolean;
assertVersion: (string) => void;
}) {
assertVersion("^7.17.0");
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but we should really make declare provide these type annotations.

Comment on lines +22 to +24
function buildUndefinedNode() {
return unaryExpression("void", numericLiteral(0));
}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated from this PR - we should move this from @babel/traverse to @babel/type, since it's static.

),
};
} else {
// @ts-expect-error left must not contain RestElement if restExcludingKeys is nullish
Copy link
Member

Choose a reason for hiding this comment

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

Can it happen that restExcludingKeys is not nullish, but restExcludingKeys.length is 0?

Copy link
Contributor Author

@JLHwung JLHwung Mar 29, 2022

Choose a reason for hiding this comment

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

Oh the wording is incorrect, it should be "left must not be a RestElement" so it is okay to yield {left, right}. I have tuned the types so this comment is not required.

(() => {
var _m, _p, _p2, _m2;

_m = [C], [_p, ..._p2] = _m, _m2 = babelHelpers.classStaticPrivateFieldSpecGet(_p, C, _x), x = _m2 === void 0 ? 1 : _m2, z = _p2;
Copy link
Member

Choose a reason for hiding this comment

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

_m is not needed here, maybe we could split the assignment only if we need to reference it multiple times.

var _m, _p, _p2, _p3;

let z;
_m = [0, C], [_p, _p2, ..._p3] = _m, C.#x = _p, x = _p2.#x, z = _p3;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of extracting everything from the array pattern, we could extract things only starting from the first element containing a private destructuring:

_m = [0, C], [C.#x, _p2, ..._p3] = _m, x = _p2.#x, z = _p3;

@@ -80,7 +80,9 @@ export default declare((api, options) => {
}

statementBody.unshift(
t.expressionStatement(t.assignmentExpression("=", left, temp)),
t.expressionStatement(
t.assignmentExpression("=", left, t.cloneNode(temp)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

temp has been reused when creating node.left. I am surprised that babel-check-duplicated-nodes does not report this issue. However, it reports when I happen to delete

if (statementBody.length === 0 && path.isCompletionRecord()) {
            statementBody.unshift(
              t.expressionStatement(scope.buildUndefinedNode()),
            );
          }

@JLHwung JLHwung force-pushed the transform-destructuring-private branch 2 times, most recently from 1edb26e to ba0d01c Compare March 29, 2022 23:44
Comment on lines +143 to +160
const shouldPreserveCompletion =
(!isExpressionStatement(parent) && !isSequenceExpression(parent)) ||
path.isCompletionRecord();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we exclude ExpressionStatement and SequenceExpression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is copied from

if (path.isCompletionRecord() || !path.parentPath.isExpressionStatement()) {

NodePath#isCompletionRecord essentially checks alongside the ancestry if the path is the last item of its container. This approach works for ExpressionStatement in Block and expression in SequenceExpression.

@JLHwung JLHwung force-pushed the transform-destructuring-private branch from ba0d01c to 4ec26d4 Compare April 4, 2022 14:36
@nicolo-ribaudo
Copy link
Member

@jridgewell This PR is huge, but probably you are one of the most qualified people to review it, if you have time.

@JLHwung JLHwung force-pushed the transform-destructuring-private branch from 4ec26d4 to d79ed0f Compare April 11, 2022 20:47
@@ -0,0 +1,29 @@
var log = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is disabled because currently the plugin does not support it: in transformPrivateKeyDestructuring the sub patterns of the array pattern as LHS is evaluated after the iterable at RHS is consumed.

@JLHwung JLHwung force-pushed the transform-destructuring-private branch from d79ed0f to 63e69b3 Compare May 2, 2022 17:41
@@ -257,12 +263,16 @@ export function* privateKeyPathIterator(pattern: t.LVal) {
});
}

// t.LVal without t.RestElement
type LHS =
Copy link
Member

Choose a reason for hiding this comment

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

const assignmentPattern = program.get("body.0.expression.params.0");
assignmentPattern.scope.push({ id: t.identifier("ref") });
expect(program.toString()).toMatchInlineSnapshot(`
"var ref;
Copy link
Member

Choose a reason for hiding this comment

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

Where does this come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var ref is created by assignmentPattern.scope.push({ id: t.identifier("ref") });, this test is to show that the ref is pushed to the parent scope instead of

class C {
  m(a = f()) { var ref; }
}

// (b, p1) => {}
// transforms to
// (b, p1 = void 0) => {}
if (firstAssignmentPatternIndex >= firstPrivateIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Since this is the only real use for firstAssignmentPatternIndex, wouldn't it be clearer to check !ignoreFunctionLength && … and then do the specific assignment code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can not assign the firstAssignmentPatternIndex here because the transform will convert any params after the first private destructuring. In other words, firstAssignmentPatternIndex will always be less than firstPrivateIndex if we assign firstAssignmentPatternIndex here.

let { left, right } = item;
const searchPrivateKey = privateKeyPathIterator(left).next();
if (searchPrivateKey.done) {
if (restExcludingKeys?.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this handling something like the following? Turning { first, #priv: priv, ...rest } into { first }, priv = #priv, { …rest } would break the …rest's exclude keys. So wee need to handle the case where something has been excluded. But in the case of { #priv: priv, first, …rest }, it doesn't matter because priv = #priv, { first, …rest } still has the correct behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The growRestExcludingKeys will exclude private keys:

https://github.com/babel/babel/pull/14304/files#diff-5742fd1087b16ca97d1e7d8fbf176f27ad52d323f5e281db9bc349bd257efd26R68

But in the case of { #priv: priv, first, …rest }, it doesn't matter because priv = #priv, { first, …rest } still has the correct behavior.

Yes. The plugin should already handle this case, I will add new tests.

@nicolo-ribaudo nicolo-ribaudo force-pushed the transform-destructuring-private branch from 298fddf to e57ff74 Compare May 17, 2022 21:49
@nicolo-ribaudo nicolo-ribaudo merged commit 1daded5 into babel:main May 17, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the transform-destructuring-private branch May 17, 2022 22:03
devversion added a commit to angular/angular that referenced this pull request May 20, 2022
This commit accounts for the Babel types changes. Some properties
can now also be `undefined` so existing checks/assertions had to
be adjusted to also capture `undefined` (along with `null`).

Additionally, in preparation for a new ECMA proposal, Babel types
seem to have been updated to include private names in object property
keys. This is not necessarily the case for object expressions, but
could be for object patterns (in the future -- when implemented).

More details: babel/babel#14304 and
https://github.com/tc39/proposal-destructuring-private.
devversion added a commit to angular/angular that referenced this pull request May 20, 2022
This commit accounts for the Babel types changes. Some properties
can now also be `undefined` so existing checks/assertions had to
be adjusted to also capture `undefined` (along with `null`).

Additionally, in preparation for a new ECMA proposal, Babel types
seem to have been updated to include private names in object property
keys. This is not necessarily the case for object expressions, but
could be for object patterns (in the future -- when implemented).

More details: babel/babel#14304 and
https://github.com/tc39/proposal-destructuring-private.
alxhub pushed a commit to angular/angular that referenced this pull request May 20, 2022
This commit accounts for the Babel types changes. Some properties
can now also be `undefined` so existing checks/assertions had to
be adjusted to also capture `undefined` (along with `null`).

Additionally, in preparation for a new ECMA proposal, Babel types
seem to have been updated to include private names in object property
keys. This is not necessarily the case for object expressions, but
could be for object patterns (in the future -- when implemented).

More details: babel/babel#14304 and
https://github.com/tc39/proposal-destructuring-private.

PR Close #45967
alxhub pushed a commit to angular/angular that referenced this pull request May 20, 2022
This commit accounts for the Babel types changes. Some properties
can now also be `undefined` so existing checks/assertions had to
be adjusted to also capture `undefined` (along with `null`).

Additionally, in preparation for a new ECMA proposal, Babel types
seem to have been updated to include private names in object property
keys. This is not necessarily the case for object expressions, but
could be for object patterns (in the future -- when implemented).

More details: babel/babel#14304 and
https://github.com/tc39/proposal-destructuring-private.

PR Close #45967
@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 Aug 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022
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: New Feature 🚀 A type of pull request used for our changelog categories Spec: Destructuring Private Fields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support destructuring-private
4 participants