Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Destructuring instances with private fields #4

Closed
jeffmo opened this issue Jun 7, 2017 · 21 comments
Closed

Destructuring instances with private fields #4

jeffmo opened this issue Jun 7, 2017 · 21 comments

Comments

@jeffmo
Copy link
Member

jeffmo commented Jun 7, 2017

It just occurred to me that, due to the nature of private-field names, destructuring private fields off an instance into a local binding is not possible with the spec as-is.

I think this could be fixed with a simple tweak to destructuring grammar, though:

const {#pField: pField} = this;

would roughly desguar to

const pField = this.#pField;

Should we include this destructuring grammar in this proposal? Or defer it to a separate proposal?

(I would be in favor of including it if it's as non-controversial as I imagine, defer if it's at all controversial)

@jeffmo jeffmo changed the title Destructuring private fields Destructuring instances with private fields Jun 7, 2017
@kentcdodds
Copy link
Member

I would be in favor of including it if it's as non-controversial as I imagine, defer if it's at all controversial

I agree. FWIW, I have no problem with what you've proposed 👍

@littledan
Copy link
Member

Well, this makes sense, but at the same time it sort of subsumes a possible semantics for private names in object literals. If we allow {#foo: 1, bar() { return #foo } } on the right-hand side, then it's strange if this destructuring case does not introduce a new binding for #pField on the right hand side (which would be impossible to satisfy).

@kentcdodds
Copy link
Member

Good point. I'm not certain the use case for destructuring private fields is super necessary due to the shorthand anyway. Another reason I see people destructuring is to alias things to avoid variable name conflicts, but that's less of a problem with private fields...

@jeffmo
Copy link
Member Author

jeffmo commented Jun 8, 2017

I'm not sure I follow:

then it's strange if this destructuring case does not introduce a new binding for #pField on the right hand side (which would be impossible to satisfy)

Did you mean "not introduce a new binding for #foo on the left hand side"?

I think I may be missing something, but AFAICT this works with future support for privates in objects too...

const obj = {#p: 42, bar() {
  const {#p: p} = obj;
  return p;
}};

(

To be clear, I am not suggesting the following destructuring syntax since it would be problematic: const {#p} = obj;. IOW: PrivateName is only permitted in a destructuring pattern if it is followed by a : to rename.

(In theory we could remove this restriction for expression-patterns where there may actually be a private binding already -- but I'd be fine with staying strict there since that seems like a pretty obscure scenario and remaining congruent between expression and binding forms is nice)

@littledan
Copy link
Member

I meant on the right-hand side: {#foo: bar} in expression position would introduce a new identifier #foo whose access is lexically scoped to inside the curly brackets.

What you're saying seems possible, but it seems strange that in an object literal as an expression, a new unique binding is introduced, but as destructuring it refers to an existing binding. But maybe this is obvious when you use it, and I'm just being a spec pedant.

@jeffmo
Copy link
Member Author

jeffmo commented Jun 8, 2017

After a bit of conversation with @littledan I'm convinced that this could be confusing enough that it warrants further discussion and we should leave it to a separate follow-up proposal if ever it comes into demand.

The example he posed that convinced me was:

class X {
  #a;
  foo() {
    let {#a: a} = {#a: b};
  }
}

There's nothing technically wrong with this example, but it may be confusing enough that someone could mistakenly expect that the a binding here would be initialized with the value of b (whereas in reality this code would throw because, in this hypothetical private-object-fields scenario, #a would not be visible outside the curly braces because it is lexically private to the interior of that object).

@cybairfly
Copy link

Any further thoughts? I think the ability to destructure private fields should be possible and consistent with non-private fields:

#foo whose access is lexically scoped to inside the curly brackets
Makes sense - private member should be limited to its own closure @littledan

and therefore this also makes complete sense and should not work @jeffmo

#a would not be visible outside the curly braces because it is lexically private

All in all, to me it looks like a reasonable thing to expect from a publicly inaccessible member and the hash sign makes it quite clear that this is expected. I think this would be a welcome feature and consistent with destructuring of public fields with known and expected limitations for private ones. For your consideration:

class Thing {
    #a = 1;
    b = 2;
    c = 3;
    d = 4;
    #z = null;

    start = () => {
        this.doStuff(this);
    }

    doStuff = ({c, d, b} = this) => {
        console.log({b, d, c});
        this.doThing(this);
    }

    doThing = ({d, c, b} = this) => {
        console.log({b, c, d});
        this.doMore(this);
    }

    noCanDo = ({b, d, c, #a, #z} = this) => {
        console.log({d, c, e, #a, #z}); // won't work
        console.log({d, c, e, a: this.#a, z: this.#z});
    }
}

@ljharb
Copy link
Member

ljharb commented Feb 21, 2021

Of course it won’t work, because you can’t declare a variable using # syntax.

@cybairfly
Copy link

Well, even with the private fields aliased, that still works better than the alternative @ljharb

    noCanDo = ({b, d, c, #a: a, #z: z} = this) => {
        console.log({d, c, e, a, z}); // would work
        console.log({d, c, e, a: this.#a, z: this.#z});
    }

@benlesh
Copy link

benlesh commented Sep 3, 2021

@littledan I just ran into this (using it in production because I'm psycho) and thought it was curious. Relevant Twitter thread here

I had a few thoughts about this (I'm neither for or against it), and I didn't see them in this thread:

On an elementary level, it seems like it would be safe to allow destructure from this.

class Person {
  #income = 100;
  #ssn = '123-45-6789';

  fileTaxes() {
    const { #income: income, #ssn: ssn } = this;
    irs.shenanigans(income, ssn);
  }
}

If it was allowed one unintended consequence of this would be the fact that destructuring can be done with a ... spread, which implies iteration, which private fields should not be iterable (to my understanding). So that may get confusing.

class Person {
  #income = 100;
  #ssn = '123-45-6789';

  fileTaxes() {
    const { ...what } = this;
    irs.shenanigans(what['#income'], what['#ssn']); // This doesn't seem right.
  }
}

Maybe we could try to allow only named destructures of private fields, but that might confuse people as well (although less often).

class Person {
  #income = 100;
  #ssn = '123-45-6789';

  fileTaxes() {
    const { #income: income, #ssn: ssn } = this; // okay
    irs.shenanigans(income, ssn);
  }
  
  recordPrivateData() {
     const { ...allPersonalData } = this;
     record(allPersonalData); // #ssn and #income are missing!
  }
}

But this isn't entirely "new" behavior, because people can already destructure non-enumerable properties by name, but not by ... spread. So it's probably totally okay

var obj = { prop1: 'one' };
Object.defineProperty(obj, 'prop2', { value: 'two', enumerable: false });

var { prop2 } = obj;
console.log(prop2); // "two";
var { ...copy } = obj;
console.log(copy);  // { prop1: 'one' }

I guess I'm indifferent about whether or not this ends up supported. However, I will say that it makes dealing with #field over private _field in TypeScript mildly annoying/boilerplatey.

@bakkot
Copy link
Contributor

bakkot commented Sep 3, 2021

And this isn't entirely "new" behavior, because people can already destructure non-enumerable properties I think?

Yup:

let o = Object.create(Object.prototype, { 'x': { value: 0, enumerable: true }, 'y': { value: 1, enumerable: false } });
console.log({...o}); // { x: 0 }
let { x: a, y: b } = o;
console.log(a, b); // 0, 1

I think allowing let { #x: x } = this without changing the semantics of {...this} would work fine, and if it's something which is coming up, we should do it.

(I do want to say that I'm opposed to allowing a bare let { #x } = this to mean let x = this.#x; that's just too confusing.)

@jridgewell
Copy link
Member

let { #x: x } = this

I think this syntax is good, and we should add it. This mirrors stringified destructuring let { "foo-bar": fooBar } = this. Same way you have to assign the stringified field to a local identifier, you'll need to assign the private field to a local identifier, let { #x } = this is just a syntax error.

@benlesh
Copy link

benlesh commented Sep 3, 2021

FWIW: I think that the real catch here is that it should only be allowed from this or identifiers pointing to this. Examples above like this should not work, IMO:

class X {
  #a;
  
  foo() {
    let {#a: a} = {#a: b};
      // ^ --- SyntaxError here I hope
  }
  
  bar() {
     let { #a: a } = this; // okay
  }
  
  baz() {
     let { ...copy } = this;
     copy.#a; // SyntaxError  copy isn't an instance of the enclosing class.
  }
}

@littledan maybe this issue could be reopened to track the discussion? Or at least so it stays in memory if there's a follow up proposal. Again, I don't have strong feelings about whether this should land, but it seems like it's a possibility?

@jridgewell
Copy link
Member

Accessing a private field is done at runtime, not syntax time. So you'd be able to destructure from random objects, but it would throw if the field isn't installed on the RHS object.

@bakkot
Copy link
Contributor

bakkot commented Sep 3, 2021

@benlesh I don't think it makes sense to restrict what can go on the RHS, any more than it does to restrict what you can put before .#x - since you can write o.#x, you should be able to write ({ #x: a} = o) as well. Whether or not either throws at runtime depends on whether o has the #x field when that code runs.

That said,

  foo() {
    let {#a: a} = {#a: b};
  }

would still be a SyntaxError, but on the RHS rather than the LHS.

@benlesh
Copy link

benlesh commented Sep 4, 2021

I don't see your point. I guess I don't agree at all. If you had an identifier that pointed to an object that had a private variable, and it wasn't a reference to the class you were in, it wouldn't work. Which seems reasonable to me. It has little to do with the right hand side and has more to do with what you're accessing.

@benlesh
Copy link

benlesh commented Sep 4, 2021

Oh well. It seems decided. As I stated earlier I really don't care if this gets added. Haha. Although I really do think it could be added and it's a bit of a shame that it doesn't work given all the effort that went into destructuring work and how popular it is to destructure "private" fields off of classes and TypeScript.

@jridgewell
Copy link
Member

I've created https://github.com/jridgewell/proposal-destructuring-private to track this.

@rdking
Copy link

rdking commented Sep 7, 2021

@benlesh Just how I see it but:

foo() {
    let {#a: a} = {#a: b};
}

is the same as

foo() {
    let a = {#a: b}.#a;
}

This would fail resolving {#a: b} because it's an object literal, and private fields aren't available for defining objects. That's why @bakkot said it would be a RHS SyntaxError.

@benlesh
Copy link

benlesh commented Sep 8, 2021

I feel like the examples being pointed out are false equivalencies. Everyone is talking about object literals when I'm actually talking about this within a class context. I completely agree that object literals should not work like that, however that's not at all what I'm talking about and it's a distraction from what I'm talking about.

@bakkot
Copy link
Contributor

bakkot commented Sep 8, 2021

@benlesh Your example had an object literal in it: let {#a: a} = {#a: b}; is code you wrote. The RHS of that line of code is an object literal. That's why we're talking about it.

I think we're mostly on the same page: let { #a: a } = this should work, and let { #a: a } = notAnInstance should be an error. The only quibble is that you suggested the error should be a SyntaxError, which implies it would happen at parse time, which doesn't really make sense - the parser doesn't know that notAnInstance is not an instance. Instead, we're saying that it should be a ReferenceError at runtime, just like notAnInstance.#a is today.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants