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

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

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.

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.

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

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

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

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

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 } = {}

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

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.

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.

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.

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

Good catch, you're right!

cool let me work on it!

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

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

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.