tc39 / proposal-decorators

Decorators for ES6 classes
https://arai-a.github.io/ecma262-compare/?pr=2417
2.74k stars 106 forks source link

Creating a new private name #243

Closed tjcrowder closed 4 years ago

tjcrowder commented 5 years ago

Decorators will often need to create private names, such as the @observed decorator in METAPROGRAMMING.md. With the current proposed spec text, there's no defined way to do that; instead, you have to jump through hoops and use a side effect. From METAPROGRAMMING.md:

// There is no built-in PrivateName constructor, but a new private name can
// be constructed by extracting it from a throwaway class
function PrivateName() {
  let name;
  function extract({key}) { name = key; }
  class Throwaway { @extract #_; }
  return name;
}

This is a standard operation. It should have an exposed, official way to do it.

littledan commented 5 years ago

Cc @caridy who encouraged this limitation

littledan commented 5 years ago

Actually this definition needs to be updated to match the one in friend.js

tjcrowder commented 5 years ago

Just as a consumer of this proposal, I strongly believe requiring a workaround like this is a blocker. Given the strong motivator to provide a clean way to achieve a common use case, I'd be keen to know what the counter-motivator is.

The workaround is not just awkward, but easy to get wrong. Consumers will have to learn it, like a mantra (or of course, copy/paste). My understanding from a private discussion with Dan is that the old one in METAPROGRAMMING.md (it may be updated by the time you're reading this, I've done a PR):

// DOESN'T WORK ANYMORE
function PrivateName() {
  let name;
  function extract({key}) { name = key; }
  class Throwaway { @extract #_; }
  return name;
}

...doesn't work (anymore) because it attempts to reuse the private name (using it in Throwaway, then in whatever you were really going to use it for), which is why the one in friend.js has to be used instead:

// Works because the decorator throws, so `Throwaway` doesn't actually use it
function PrivateName() {
  function extract({key}) { throw key; }
  try { class Throwaway { @extract #_; } }
  catch (name) { return name; }
}

Rather than have that copied-and-pasted into every project doing this common use case, it should be provided by the standard library.

nicolo-ribaudo commented 5 years ago

IIRC, there was some discussion about private #x; declarations to create new private names.

tjcrowder commented 5 years ago

@nicolo-ribaudo - Interesting. I'm not bothered how we do it, though given that the spec is written in terms of Private Name objects that already have a constructor, prototype, etc., exposing the constructor seems the simplest option.

littledan commented 5 years ago

The idea is, there are several possible follow-on proposals that can provide this functionality, and it's accessible now through a three-liner, so we can give those follow-on proposals time to be carefully considered.

tjcrowder commented 5 years ago

I think the time to carefully consider them is now, but obviously that's just one view amongst many. I hope this gets discussed today or tomorrow. It seems like something could be resolved with the people in the room (but I've never been in that room). It would be a real shame if the five-liner were necessary.

littledan commented 5 years ago

Well, a previous draft "just worked" for this, where you could new the PrivateName constructor (though it's a bit hard to get ahold of). Removing this functionality was feedback from the room. It would be simple to re-add it, but it means not following the advice this feedback.

tjcrowder commented 5 years ago

@littledan - It's pushing back against that feedback, months later, in the face of consumer feedback on the proposal and the evolution of the proposal in the meantime. That seems reasonable to me. But you're the man on the spot, not me; I lack your experience of the situation, knowledge of the people, and experience of this process in general. I respect that knowledge and experience, and your judgement.

I'd ask that you raise it, and if it doesn't get resolved, that a solid reason for leaving this kind of gap in the spec be provided. (For me, it's very unsatisfying to say "because we might want to solve it differently in the future." If that's the reason, that's the reason, but it leaves things incomplete in my view.) I'd also suggest that if this is left open, a note in the spec would be needed showing the correct workaround, as it's a bit arcane and earlier versions will no longer work.

If you don't feel raising it is appropriate, that's obviously your call. I recognize my very, very minor role here. But I would already have left it if I didn't believe that leaving this gap is a genuine problem.

littledan commented 5 years ago

Well, I've tried to contact the person who raised the concern. We'll see. I have to say, your intuition matches mine.

littledan commented 5 years ago

OK, @tjcrowder and I talked about this further offline. I didn't understand it from the above thread, but @tjcrowder was picturing that we'd add PrivateName to the global scope, or maybe as a property of Object; I'm not really comfortable with those possibilities, as I am worried it would lead to misunderstandings and misuse. We've discussed this extensively in this repository and TC39 in the past.

Let's pursue exposing a constructor for PrivateName objects as a follow-on proposal, together with a "decorator standard library", and for now, publish a library for the three lines it takes to make new private names with current decorators.

Lodin commented 5 years ago

@littledan, is there any possible threats if it is not a constructor, but a function that returns PrivateName object? Like a function from polyfill, but native.

tjcrowder commented 5 years ago

@Lodin - Dan just kindly told me in a private conversation why adding a global for this is problematic: If it's a global, can it be overwritten? Even if not overwritten, it could be shadowed. Now, privacy is broken. Hence the suggestion about syntax for it that @nicolo-ribaudo mentioned.

So the history (which he again was kind enough to share as I hadn't found it on my own) is that there was a lot of back-and-forth on this: How to do it, real reasons to be concerned about the obvious thing (new PrivateName), and no consensus about what to do instead. Meanwhile, we have a workaround that isn't subject to those problems, and an otherwise-ready proposal.

For me, being able to do this is part-and-parcel, but now I understand A) Why it's not as simple as it seems and in particular why new PrivateName is potentially problematic, and B) How we got here. (And (A) lets me start thinking how to solve it at some stage... :-) ) So we live with the workaround for now.

@littledan - Please don't hesitate to correct anything I got wrong above. And thanks as ever for your patience.

tjcrowder commented 5 years ago

And to be clear: Now that I understand those things, I understand why it makes sense to move forward with the proposal as it is now rather than waiting. And the world wants decorators to move to Stage 3!

Lodin commented 5 years ago

@tjcrowder Oh I see, didn't thought about shadowing at all. Well, I don't see it as a blocking issue as well, since there is polyfill solution.

BTW, couldn't Standard Library solve these issues? Since it is based on imports, it becomes hard to overwrite it. Not sure about shadowing though.

tjcrowder commented 5 years ago

@Lodin - For me, it should be intrinsic because private names are intrinsic. But that's a conversation for another day... :-)

littledan commented 5 years ago

@Lodin Part of the issue is, how do you get to this function reliably? PrivateName takes pains to freeze itself, but it induces a bit more hesitation to make a frozen property of the global object, even if you can watch out for lexical shadowing locally.

I agree that the standard library gives a good way forward on this issue!

caridy commented 5 years ago

@tjcrowder we have discussed this extensively, and yes, the committee is open for a generalization of the private fields/symbols beyond classes. I think that, by now, everyone in the thread is aware of the high-level constrains of exposing this as part of the decorator API. Hopefully we can get the ball rolling on that as a followup proposal, and considering that we have multiple overlapping discussions about this topic, I'm confident that we will have something that fulfill your particular use-case, which is basically having a simple way to define a instance level storage mechanism for a field decorated as @observed. The current solution seems to be fine (the one described in METAPROGRAMMING.md), considering that the decorator's consumer doesn't have to know about it, just the decorator author. Additionally you can always use a WeakMap for the same purpose.

jridgewell commented 5 years ago

Due to the super-return-override trick, PrivateName is essentially reifiable today (test it in Chrome Canary). You can even implement a WeakMap with it!

function PrivateName() {
  class Super {
    constructor(object) {
      return object;
    }
  }

  class State extends Super {
    #data;
    constructor(object) {
      super(object);
    }

    static has(object) {
      try {
        object.#data;
        return true;
      } catch {
        return false;
      }
    }

    static init(object, value) {
      new State(object);
      object.#data = value;
    }

    static get(object) {
      return object.#data;
    }

    static set(object, value) {
      return object.#data = value;
    }
  }

  return {
    __proto__: null,
    has: State.has,
    init: State.init,
    get: State.get,
    set: State.set,
  };
}
`WeakMap` via `PrivateName` syntax ```js function PrivateName() { class Super { constructor(object) { return object; } } class State extends Super { #data; #deleted = false; constructor(object) { super(object); } static delete(object) { try { object.#data = undefined; return object.#deleted = true; } catch { return false; } } static has(object) { try { return !object.#deleted; } catch { return false; } } static init(object, value) { try { new State(object); } catch { object.#deleted = false; } object.#data = value; return this; } static get(object) { if (object.#deleted) { throw new TypeError('PrivateName not initialized on object'); } return object.#data; } static set(object, value) { if (object.#deleted) { throw new TypeError('PrivateName not initialized on object'); } object.#data = value; return this; } } return { __proto__: null, delete: State.delete, has: State.has, init: State.init, get: State.get, set: State.set, }; } ```
ljharb commented 5 years ago

@jridgewell i tried it in canary 74 and got a syntax error on the private field; is there a flag to enable?

jridgewell commented 5 years ago

Oh, yes. The “Experimental JavaScript” flag, I forget its explicit name.

mathiasbynens commented 5 years ago

The flag is --harmony-private-fields:

Or, through the Chrome flags UI, you can just enable all --harmony features:

rdking commented 5 years ago

How do the class methods know what the sigil name of the private field is when a decorator uses the workaround approach to create a private field in a class?

I'm asking because it seems like, with the workaround approach, the sigil name will always be #_, which would be problematic if more than 1 decorator used the same approach, or 1 decorator created multiple private names. They'd all be accessible as this.#_. So how would the engine know how to resolve that?

ljharb commented 5 years ago

The names aren't strings, they're lexical; so just like Symbol('a') and Symbol('a') are different property keys, two different #_ would also be unique, despite having the same string representation.

rdking commented 5 years ago

I understand that, and I'm saying that the fact that they have "the same string representation" might be an issue. Here's an absurd example of what I mean:

//A class decorator that adds 1 private field and 1 private method
function AddFeature(desc) {
  function PrivateName() {
    function extract({key}) { throw key; }
    try { class Throwaway { @extract #_; } }
    catch (name) { return name; }
  }

  let retval = Object.assign({}, desc);
  retval.extras = [];

  retval.extras.push({
    key: PrivateName(), 
    kind: "field",
    placement: "own",
    initialize: ()=>42,
    descriptor: { writable: true }
  });
  retval.extras.push({
    key: PrivateName(),
    kind: "method"
    placement: "own",
    descriptor: {},
    method: function() {
      //How do I reference the private field above?
    }
  });
  return retval;
}

@AddFeature
class Example {
  doSomething() {
    //How do I reference the private method or field supplied by @AddFeature?
  }
}

If something in the mechanics of the internal PrivateName is somehow aware of the lexical name used when the PrivateName instances were created, then this should fail due to a name conflict between 2 PrivateNames. Sure, the private name instances are different, but unless the string you type to access them (the sigil name or friendly name) is different, then there's no consistent way for the 2 new private fields to be individually referenced. Unless I'm mistaken, a new instance of Example will have 2 private elements with the sigil name #_.

tjcrowder commented 5 years ago

The description of the private name is completely irrelevant. Those two #_ private names are separate because they were created in separate scopes.

How do I reference the private field above?

You'd remember the private name returned by your first call to PrivateName() and use it in the method.

const fieldName = PrivateName();
retval.extras.push({
  key: fieldName, 
  kind: "field",
  placement: "own",
  initialize: ()=>42,
  descriptor: { writable: true }
});
retval.extras.push({
  key: PrivateName(),
  kind: "method"
  placement: "own",
  descriptor: {},
  method: function() {
    const value = fieldName.get(this);
  }
});

How do I reference the private method or field supplied by @AddFeature?

You can't, in doSomething's code in Example. Which makes sense, because Example doesn't know it's being decorated and doesn't know anything about the decorations being done.

You could modify your decorator to wrap doSomething. The wrapper could then access the private field and method (if you remembered both their names).

Having said all that, as I understand it userland decorators are in jeopardy because of performance concerns from JavaScript engine implementers expressed at the January TC39 meeting (when decorators didn't progress to Stage 3). If they happen, they may change markedly before they do.

rdking commented 5 years ago

Ouch. So adding new private fields does not inject a lexical name into the class scope?

tjcrowder commented 5 years ago

No, lexical scoping remains...lexical. :-)

rdking commented 5 years ago

Also, if user-land decorators become impossible, then doesn't that destroy the whole point of the proposal? Sure, new features and capabilities will still be able to be provided by engine implementors, but that only increases the burden on them as the desired features will vary from one project to the next. There's no reasonable way for engine providers to predict all the ways users will want to use decorators.

If users will not be able to write their own decorators, then what of all the compromises and controversial trade-offs that have gone into other proposals with the promise of a work-around via decorators? This is especially disappointing considering my views regarding class-fields. I've almost finished a work-around for the issues I have with it, including the syntax. However, it all becomes cumbersome again if decorators don't supply names for private fields to the lexical scope of the class.

No, lexical scoping remains...lexical. :-)

I was under the impression that decorators operated by interfering with the parsing of the class definition, in effect modifying the lexical scope of the class definition. If that's not happening, then how exactly can a decorator remove a private field? Sure, the slot/Weakmap can be reclaimed, but the private name would remain in the lexical scope as an artifact. Wouldn't that trigger an unexpected error?

tjcrowder commented 5 years ago

(Remember I'm not TC39 or even a major contributor to proposals.) There's talk of a standard (built-in) decorators library as a possible next step with all the obvious ones (bound, observable, etc.), with maybe userland after. I for one would be extremely unhappy if userland decorators don't happen or are substantially delayed. But it hasn't been that long since the meeting and I really hope a cogent, thorough writeup of the actual performance concerns is forthcoming, so we can all understand what the trade-off being made is.

I was under the impression that decorators operated by interfering with the parsing of the class definition, in effect modifying the lexical scope of the class definition.

Nope. :-) Roughly speaking (see the spec text for details), the class definition is parsed into element descriptors describing each element (method, field) of the class. Decorators can then modify those descriptors, remove them, add new ones, etc. Then the class is instantiated from the updated element descriptors, and hooks run (and a placement="static", kind="hook" hook with a replace can completely replace the class with something else).

But again, see the spec text for details.

If that's not happening, then how exactly can a decorator remove a private field?

Removing a private field would be an extremely uncommon decorator action. But it would do it by removing the descriptor for the field. It would then also have to replace any code in the class that tried to use the private field, since of course that code would fail because the object wouldn't have the necessary entry in [[PrivateFieldValues]].

nicolo-ribaudo commented 5 years ago
class Cl1 {
  @remove #foo;

  test() {
    this.#foo;
  }
}

class Cl2 {
  #foo;

  test(externalObject) {
    externalObject.#foo;
  }
}

These two .test() methods would throw the same error when called.

rdking commented 5 years ago

....wait, I get it. This requires a producer/consumer design.

//A class decorator that adds 1 private field and 1 private method
let { AddFeature, AddFeatureField, AddFeatureMethod } = { function() {
  function getPrivateName() {
    function extract({key}) { throw key; }
    try { class Throwaway { @extract #_; } }
    catch (name) { return name; }
  }

  return {
    AddFeature(desc) {
      let retval = Object.assign({}, desc);
      retval.elements = desc.elements.slice();
      retval.extras = [];
      let featureField = getPrivateName();
      let featureMethod = getPrivateName();

      retval.extras.push({
        key: featureField, 
        kind: "field",
        placement: "own",
        initialize: ()=>42,
        descriptor: { writable: true }
      });
      retval.extras.push({
        key: featureMethod,
        kind: "method"
        placement: "own",
        descriptor: {},
        method: function() {
          //Question: How do I reference the private field above?
          //Answer: use featureField.get(this) and featureField.set(this, value);
        }
      });

      for (let element of retval.elements) {
        if (element.descriptor.addFeatureField) {
          let desc = Object.assign(element.descriptor, {
            get() { return featureField.get(this); },
            set(v) { featureField.set(this, v); }
          });
        }
        else if (element.descriptor.addFeatureMethod) {
          let desc = Object.assign(element.descriptor, {
            get() { return featureMethod.get(this); },
            set(v) { featureMethod.set(this, v); }
          });
        }
      }

      return retval;
    },
    AddFeatureField(desc) {
      let retval = Object.assign({}, desc);
      retval.descriptor = Object.assign({}, desc.descriptor);
      retval.descriptor.addFeatureField = true;
      return retval;
    },
    AddFeatureMethod(desc) {
      let retval = Object.assign({}, desc);
      retval.descriptor = Object.assign({}, desc.descriptor);
      retval.descriptor.addFeatureMethod = true;
      return retval;
    }
  };
}

@AddFeature
class Example {
  @AddFeatureField
  #featureField;
  @AddFeatureMethod
  #featureMethod;
  doSomething() {
    //Q: How do I reference the private method or field supplied by @AddFeature?
    //A: use this.#featureField and this.#featureMethod
  }
}

Does this seem right?

tjcrowder commented 5 years ago

The descriptor object a decorator receives is a temporary object only for the call to the decorator. If you add non-standard properties to the descriptor object, they're thrown away. AddFeature wouldn't see them. You can see this in the spec in DecorateElement, where the object passed to the decorator is created specifically for that call (Step 2.e) and then the resulting object's standard properties are used to populate two new objects (Step 2.g's call to ToElementExtras), after which it's thrown away.

If the feature is going to provide something that the class's own code is supposed to use, the class own code should at least declare the private thing. For instance:

class Example {
    @addFeature
    #featureMethod() { }

    doSomething() {
        this.#featureMethod();
    }
}

...where addFeature replaces the method body of #featureMethod with the appropriate implementation. If the feature being added needs a field as well, declare the field in the class.

This is all tangential to this issue, though, so I'd say this discussion doesn't belong here. Also, since this may well change, I'm not sure going deep into it at this stage is going to be all that useful. :-) Not, at least, until there's a clearer view of what's happening next.

rdking commented 5 years ago

@tjcrowder @nicolo-ribaudo Thank you both for the help. This has proved both informational and somewhat disheartening. As for how this tangent relates to the main topic, I'm on the side that thinks decorators should have an ergonomic syntax for declaring new private elements right out of the box. However, I also believe it to be a misstep for those newly declared private elements to not be immediately available as part of the lexical scope of class. The present WeakMap-like notation is counterintuitive when dealing with something that's supposed to appear to be an internal property of this.

littledan commented 4 years ago

This proposal does not have a concept of private names, so this feature would be very much separate from decorators.

rdking commented 4 years ago

@littledan Why is it a good idea for this proposal to proceed without a concept of private names when they are now an almost irrevocable certainty in the structure of a class? If decorators are meant to manipulate the structure of a class where private names will appear, then doesn't it stand to reason that decorators need to be able to handle them?