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

destructuring private fields with array pattern / object pattern #10017

Conversation

@tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented May 23, 2019

Q                       A
Fixed Issues? Fixes #9310
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT
@tanhauhau
Copy link
Member Author

@tanhauhau tanhauhau commented May 23, 2019

I guess I need to make sure it works on loose mode as well.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented May 23, 2019

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

@@ -100,6 +100,26 @@ const handle = {
return;
}

// { KEY: MEMBER } = OBJ -> { KEY: _ref } = OBJ; _set(MEMBER, _ref);
// [MEMBER] = ARR -> [_ref] = ARR; _set(MEMBER, _ref);
if (

This comment has been minimized.

@jridgewell

jridgewell May 23, 2019
Member

If I remember correctly, this will affect super.foo expressions, too. Can you add a test for assigning [super.foo] = array, too?

scope.push({ id: ref });

const assignmentParent = parentPath.getStatementParent();
assignmentParent.insertAfter(

This comment has been minimized.

@jridgewell

jridgewell May 23, 2019
Member

I'm not sure we can insert after the statement, because later patterns can default to the property we're assigning:

[
  this.#foo,
  x = this.#foo
] = [0];

Should have x === 0. The only way I can see this working in 100% of cases is if we use a special setter property:

// input
[this.#foo, x = this.#foo] = [0];

// output
const _this = this;
const internal = { set foo(v) { _this.#foo = v } };

[internal.foo, x = this.#foo] = [0]

This comment has been minimized.

@tanhauhau

tanhauhau May 24, 2019
Author Member

so the order matters for array pattern right?
eg:

[this.#foo, x = this.#foo] = [0]

x === 0
vs

this.#foo = 1;
[x = this.#foo, this.#foo] = [0];

x === 1 ?

how about object destructuring?

this.#foo = 2;
{
   x = this.#foo,
   y: this.#foo,
   z = this.#foo,
} = { y: 1 }

so x === 2 and z === 1 ?

This comment has been minimized.

@jridgewell

jridgewell May 24, 2019
Member

Yup, you're correct in both cases.

This comment has been minimized.

@tanhauhau

tanhauhau May 24, 2019
Author Member

would it be better if we defined const internal = { set foo(v) { _this.#foo = v } }; at the constructor if we know there will be a destructuring, and reuse it throughout the scope of the class?

or else we will be keep creating internal every time we see a destructuring pattern + default

This comment has been minimized.

@jridgewell

jridgewell May 24, 2019
Member

Yah, I would only create the object when we hit a destructure.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 24, 2019

Depending on the order things are evaluated (need to check), we might be able to out things in the next property's key:

{ x: this.#foo, y = this.#foo } = {}

// ->

{ x = tmp, [this.#foo = tmp, "y"] = this.#foo } = {}

Also, when we are destructing to member expressions instead of plain identifiers, this might work:

[ this.#foo, foo.bar = this.#foo ] = [];

// ->

[ tmp, (this.#foo = tmp, foo).bar ] = []

I'm not sure about those optimizations, but I'd like to avoid using the setter when possible.

@jridgewell
Copy link
Member

@jridgewell jridgewell commented May 24, 2019

I'm not sure I understand your input/output. Where does the array destructure come from in { x = tmp, [this.#foo = tmp, "y"] = this.#foo } = {}?

There can also be indirection between a default value and the private through a getter:

class Ex {
  get() {
    return this.#foo;
  },

  destructure(arr) {
    [ this.#foo, bar = this.get() ] = arr;
  }
}

Or maybe the private is a setter that mutates some other state.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 24, 2019

It's a computed key.
I have a more efficient idea in mind anyway, I will post it soon

@tanhauhau
Copy link
Member Author

@tanhauhau tanhauhau commented May 24, 2019

{ x: tmp, [this.#foo = tmp, "y"] = this.#foo } = {}

wow, @nicolo-ribaudo this is cool, never thought of able to use comma operator this way.

@tanhauhau
Copy link
Member Author

@tanhauhau tanhauhau commented May 24, 2019

There can also be indirection between a default value and the private through a getter:

so you are saying, if there's a default value, you can't eliminate the possibility of reading from the updated private variable right

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 24, 2019

Currently this code:

class A {
  #a;
  #b() {}
  get #c() {}
  set #d(a) {}
  
  test() {
    this.#a = 1;
    this.#b = 2;
    this.#c = 3;
    this.#d = 4;
  }
}

Is transpiled to this:

class A {
  constructor() {
    _d.set(this, { set: _set_d });
    _c.set(this, { get: _get_c });
    _b.add(this);
    _a.set( writable: true, value: void });
  }

  test() {
    _classPrivateFieldSet(this, _a, 1);
    _classPrivateMethodSet();
    _classPrivateMethodSet();
    _classPrivateFieldSet(this, _d, 4);
  }

}

When using destruturing:

class A {
  #a;
  #b() {}
  get #c() {}
  set #d(a) {}
  
  test() {
    [this.#a, this.#b, this.#c, this.#d] = [1, 2, 3, 4];
  }
}

It can be transpiled like this:

class A {
  constructor() {
    _d.set(this, { set: _set_d });
    _c.set(this, { get: _get_c });
    _b.add(this);
    _a.set( writable: true, value: void });
  }

  test() {
    [
      _classPrivateFieldDestrSet(this, _a).value,
      _classPrivateMethodSet(),
      _classPrivateMethodSet(),
      _classPrivateFieldDestrSet(this, _d).value,
     ] = [1, 2, 3, 4];
  }

}

Where the _classPrivateFieldDestrSet helper is defined like this:

export default function _classPrivateFieldDestrSet(receiver, privateMap) {
  if (!privateMap.has(receiver)) {
    throw new TypeError("attempted to set private field on non-instance");
  }
  var descriptor = privateMap.get(receiver);

  if ("value" in descriptor) return descriptor;
  if (!("__destrObj" in descriptor)) {
    descriptor.__destrObj = Object.defineProperty({}, "value", descriptor);
  }
  return descriptor.__destrObj;
}

PS. My previous example was wrong; it should have been this:

{ x: tmp, [(this.#foo = tmp, "y")]: y = this.#foo } = {}
@jridgewell
Copy link
Member

@jridgewell jridgewell commented May 24, 2019

Ha, I couldn't parse the computed key when it was inline with the other keys... 😅 That's an obscure desugaring, but I guess it'll work for object patterns.

But array destructures can't use computed keys. I don't see a way to make this work in every case unless we use the setter objects. And if we have to maintain two ways to do it, why not just choose works-with-everything approach? Hopefully this will be rare enough to not matter (I didn't even realize you could destructure to a MemberExpression before this PR).

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 24, 2019

Yeah, in the last comment I posted another example which doesn't rely on computed keys (since I'm not even sure about the order in which they are evaluated).
The advantage of the helper I proposed is that instead of creating an intermediate object with a setter when destructuring to private fields, it reuses the object descriptor we already have. In the other case (setters), it creates an object reusing the descriptor we already have, and it creates it only once per key/instance pair.

@jridgewell
Copy link
Member

@jridgewell jridgewell commented May 24, 2019

Sweet, just read the other comment. Let's use the private map's descriptor.

Though, I don't understand the __destrObj property in the helper file.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 24, 2019

Though, I don't understand the __destrObj property in the helper file.

In case of a setter, the decriptor looks like this:

const desc = {
  get: _fn_get,
  set: _fn_set,
}

We can't just return the descriptor, because assigning to .value (or .set) wouldn't invoke the setter. For this reason, we need to actually create an object with a setter:

const objWithSetter = {
  value: // This has the "descriptor" descriptor.
}

I stored it in desc.__destrObj only to avoid recreating it every time the helper is called on a given key (which probably would be called once for every invokaton of the class method which does the destructuring). Anyway, if you think that it just adds confusion this optimization can probably be removed, since destructuring to a private getter is a really rare edge case.

@jridgewell
Copy link
Member

@jridgewell jridgewell commented May 24, 2019

Ooo, clever. But the receiver won't be correct during the set, we would have to bind it. Or we can just create the reusable internal object without reusing the descriptor.

The helper approach is definitely simpler, I like it the most.

export default function _classPrivateFieldDestructureSet(receiver, privateMap) {
  if (!privateMap.has(receiver)) {
    throw new TypeError("attempted to set private field on non-instance");
  }
  var descriptor = privateMap.get(receiver);

  if ("value" in descriptor) return descriptor;
  
  if (!("__destrObj" in descriptor)) {
    descriptor.__destrObj = {
      set value(v) {
        descriptor.set.call(receiver, v)
      },
    };
  }
  return descriptor.__destrObj;
}
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 24, 2019

Good catch, you're right!

@tanhauhau
Copy link
Member Author

@tanhauhau tanhauhau commented May 25, 2019

cool let me work on it!

@tanhauhau
Copy link
Member Author

@tanhauhau tanhauhau commented May 25, 2019

@nicolo-ribaudo i noticed that the define descriptor could be non writeable, but is there a test case I can write that can test this?

@tanhauhau tanhauhau force-pushed the tanhauhau:tanhauhau/fix/private-property-object-pattern branch from b85bf22 to 29b6f54 May 26, 2019
@tanhauhau
Copy link
Member Author

@tanhauhau tanhauhau commented May 26, 2019

@jridgewell Ok, I've read through the set helper for assigning super fields, I am not sure I should implement another helper that is similar to _classPrivateFieldDestructureSet above, because:

  1. I would have somewhat duplicate logic in both helpers.set and helpers.destructureSet, and would need to retest all the edge cases
  2. I am not entirely sure where to keep the __destrObj in this case, I certainly can't modify the descriptor object returned from Object.getOwnPropertyDescriptor.

So, I am thinking maybe I can

  1. call setter right after destructuring if there's no getter within the ArrayPattern statement:
class A {
   foo() {
       ([super.bar] = [1]);
   }
}

// transpiled to

class A {
  foo() {
      ([_ref] = [1]);
      super.bar = _ref;
  }
}
  1. use a temp destructObject if there's a getter:
class A {
   foo() {
       ([super.bar, b = super.bar] = [1, 2]);
   }
}

// transpiled to

class A {
   foo() {
       const _this = this;
       const temp = {
           set value(_val) {
                babelHelpers.set(
                     babelHelpers.getPrototypeOf(_obj), 
                     "bar",
                     _val,
                     _this,
                     true
                )
           }
       };
       ([temp.value, b = super.bar] = [1, 2]);
   }
}

what do you think?

@nicolo-ribaudo

This comment has been minimized.

value: void 0
});
babelHelpers.classPrivateFieldLooseBase(this, _client)[_client] = 1;
;

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo May 26, 2019
Member

Where does this extra semicolon come from?

This comment has been minimized.

@tanhauhau

tanhauhau May 27, 2019
Author Member

oh, it's due to an extra comma in the input file

This comment has been minimized.

@tanhauhau

tanhauhau Jun 2, 2019
Author Member

removed extra comma

@jridgewell
Copy link
Member

@jridgewell jridgewell commented May 26, 2019

We’ll have to discuss how this interacts with super properties, for now it’d be fine to just throw an error in the transformer.

@tanhauhau tanhauhau force-pushed the tanhauhau:tanhauhau/fix/private-property-object-pattern branch from 4648b65 to 692eee3 May 27, 2019
(parentPath.isObjectProperty({ value: node }) &&
parentPath.parentPath.isObjectPattern()) ||
parentPath.isArrayPattern() ||
parentPath.isRestElement()

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo May 29, 2019
Member

Do we also need to check for AssignmentPatterns?

[this.#foo = 2] = []

This comment has been minimized.

@tanhauhau

tanhauhau Jun 2, 2019
Author Member

checked and added more test case

// { KEY: MEMBER = _VALUE } = OBJ
(parentPath.isAssignmentPattern({ left: node }) &&
parentPath.parentPath.isObjectProperty({ value: parent }) &&
parentPath.parentPath.parentPath.isObjectPattern()) ||

This comment has been minimized.

@jridgewell

jridgewell Jun 4, 2019
Member

Nit: I think we can actually join a few of the conditions here. parentPath.isAssignmentPattern() is sufficient to check for both array pattern and object pattern, we don't need to consult the grand parent.

This comment has been minimized.

@tanhauhau

tanhauhau Jul 13, 2019
Author Member

i guess so, it's just to make my intentions clearer, not sure will assignment pattern be a child of not ust ObjectPattern and ArrayPattern ?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 13, 2019
Member

We can move these checks to be assertions: (t.assertObjectProperty(parentPath.parentPath, { value: parent })

This comment has been minimized.

@tanhauhau

tanhauhau Jul 14, 2019
Author Member

@nicolo-ribaudo what is the rationale behind using assertions instead of is?

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 14, 2019
Member

Just that with an assertion is clear that if this is an assignment expression, the parent will always be an object/array destructing

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 14, 2019
Member

Oh ok nevermind, we can't assert two different parent types.

(parentPath.isAssignmentPattern({ left: node }) &&
parentPath.parentPath.isArrayPattern()) ||
// {...MEMBER}
// [...MEMBER]

This comment has been minimized.

@jridgewell

jridgewell Jun 4, 2019
Member

Man, some of these syntaxes are crazy.

"ObjectPattern",
"ArrayPattern",
"MemberExpression",
"CallExpression",

This comment has been minimized.

@jridgewell

jridgewell Jul 14, 2019
Member

How did CallExpression get into this?

This comment has been minimized.

@tanhauhau

tanhauhau Jul 14, 2019
Author Member

@jridgewell

[_destructureSet(MEMBER) = _VALUE] = ARR

This comment has been minimized.

@jridgewell

jridgewell Jul 14, 2019
Member

That's invalid, isn't it? Chrome throws a syntax error.

This comment has been minimized.

@tanhauhau

tanhauhau Jul 14, 2019
Author Member

Yes you are right. my bad. fixed it

Copy link
Member

@jridgewell jridgewell left a comment

LGTM

@nicolo-ribaudo nicolo-ribaudo merged commit d3fe22f into babel:master Jul 14, 2019
4 checks passed
4 checks passed
@babel-bot
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
@codecov
codecov/project 87.47% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
AdamRamberg added a commit to AdamRamberg/babel that referenced this pull request Jul 17, 2019
…l#10017)

* destructuring private fields with array pattern / object pattern

* wip: new test cases

* destrucuring and rest for private properties

* test case for loose private-loose

* add transform-desturcturing for exec

* update test case

* remove getPrototypeOf imports from get and set

* wip: destructure super assignment

* throw "Destructuring to a super field is not supported yet."

* fix tests and fix assignment pattern

* remove CallExpression from AssignmentPattern
@lock lock bot added the outdated label Oct 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants