destructuring private fields with array pattern / object pattern

[tanhauhau]

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]

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

[babel-bot]

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

[jridgewell]

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

[jridgewell]

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]

[tanhauhau]

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 ?

[jridgewell]

Yup, you're correct in both cases.

[tanhauhau]

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

[jridgewell]

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

[nicolo-ribaudo]

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]

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]

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

[tanhauhau]

{ 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]

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]

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]

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]

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]

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]

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]

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]

Good catch, you're right!

[tanhauhau]

cool let me work on it!

[tanhauhau]

@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]

@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;
  }
}

2. 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]

Where does this extra semicolon come from?

[tanhauhau]

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

[tanhauhau]

removed extra comma

[jridgewell]

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.

[nicolo-ribaudo]

Do we also need to check for AssignmentPatterns?

[this.#foo = 2] = []

[tanhauhau]

checked and added more test case

[jridgewell]

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.

[tanhauhau]

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 ?

[nicolo-ribaudo]

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

[tanhauhau]

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

[nicolo-ribaudo]

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

[nicolo-ribaudo]

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

[jridgewell]

Man, some of these syntaxes are crazy.

[jridgewell]

How did CallExpression get into this?

[tanhauhau]

@jridgewell

[_destructureSet(MEMBER) = _VALUE] = ARR

[jridgewell]

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

[tanhauhau]

Yes you are right. my bad. fixed it

[jridgewell]

LGTM