tc39 / proposal-static-class-features

The static parts of new class features, in a separate proposal
https://arai-a.github.io/ecma262-compare/?pr=1668
127 stars 27 forks source link

Example of the utility of static private methods #4

Closed domenic closed 5 years ago

domenic commented 6 years ago

This is loosely based on existing code in jsdom, with some additions to illustrate further points.

I am working on a class, call it JSDOM, which has some static factory methods:

export class JSDOM {
  // ... elided ...

  static fromURL(url, options) {
    if (options.referrer === undefined) {
      throw new TypeError();
    }
    if (options.url === undefined) {
      throw new TypeError();
    }
    options.url = (new URL(options.url)).href; // normalize

    // ... TODO: actual interesting stuff
  }

  static fromFile(filename, options) {
    if (options.url === undefined) {
      throw new TypeError();
    }
    options.url = (new URL(options.url)).href; // normalize

    // ... TODO: actual interesting stuff
  }
}

I notice two things:

Both of these are things that are addressed well by the extract method refactoring.

I have two choices on how to do this:

  1. Extract into actual static methods of the class.
  2. Extract into functions at module-level.

Although it is a matter of opinion, I find (1) better than (2), because it keeps the code inside the class that it's designed for, and near the methods that use it, instead of somewhere later in the file. Today, (1) has the disadvantage of polluting my public API, so I usually choose (2). But if we had static private methods, I could do (1) without that drawback.

Still, let's assume that arguments from #1 prevail and we don't get static private methods. It's not too bad:

export class JSDOM {
  // ... elided ...

  static fromURL(url, options = {}) {
    normalizeFromURLOptions(options);
    normalizeOptions(options);

    // ... TODO: actual interesting stuff
  }

  static fromFile(filename, options = {}) {
    normalizeOptions(options);

    // ... TODO: actual interesting stuff
  }
}

function normalizeFromURLOptions(options) {
  if (options.referrer === undefined) {
    throw new TypeError();
  }
}

function normalizeOptions(options) {
  if (options.url === undefined) {
    throw new TypeError();
  }
  options.url = (new URL(options.url)).href;
}

But as I continue my work on implementing these functions, the lack of static private ends up felt more deeply. I get to the following point:

export const registry = new JSDOMRegistry();

export class JSDOM {
  #createdBy;

  #registerWithRegistry() {
    // ... elided ...
  }

  async static fromURL(url, options = {}) {
    normalizeFromURLOptions(options);
    normalizeOptions(options);

    const body = await getBodyFromURL(url);
    const jsdom = new JSDOM(body, options);
    jsdom.#createdBy = "fromURL";
    jsdom.#registerWithRegistry(registry);
    return jsdom;
  }

  static fromFile(filename, options = {}) {
    normalizeOptions(options);

    const body = await getBodyFromFilename(filename);
    const jsdom = new JSDOM(body, options);
    jsdom.#createdBy = "fromFile";
    jsdom.#registerWithRegistry(registry);
    return jsdom;
  }
}

Again I notice I have some duplicated code. Here is the code I want to write to clean this up:

export const registry = new JSDOMRegistry();

export class JSDOM {
  #createdBy;

  #registerWithRegistry() {
    // ... elided ...
  }

  async static fromURL(url, options = {}) {
    normalizeFromURLOptions(options);
    normalizeOptions(options);

    const body = await getBodyFromURL(url);
    return JSDOM.#finalizeFactoryCreated(new JSDOM(body, options), "fromURL");
  }

  static fromFile(filename, options = {}) {
    normalizeOptions(options);

    const body = await getBodyFromFilename(filename);
    return JSDOM.#finalizeFactoryCreated(new JSDOM(body, options), "fromFile");
  }

  static #finalizeFactoryCreated(jsdom, factoryName) {
    jsdom.#createdBy = factoryName;
    jsdom.#registerWithRegistry(registry);
    return jsdom;
  }
}

But I can't, because static private methods don't exist. Here is the code I have to write to work around that:

export const registry = new JSDOMRegistry();

export class JSDOM {
  #createdBy;

  #registerWithRegistry() {
    // ... elided ...
  }

  async static fromURL(url, options = {}) {
    normalizeFromURLOptions(options);
    normalizeOptions(options);

    const body = await getBodyFromURL(url);
    return finalizeFactoryCreated(
      new JSDOM(body, options), "fromURL",
      (jsdom, value) => jsdom.#createdBy = value,
      (jsdom, ...args) => jsdom.#registerWithRegistry(...args)
    );
  }

  static fromFile(filename, options = {}) {
    normalizeOptions(options);

    const body = await getBodyFromFilename(filename);
    return finalizeFactoryCreated(
      new JSDOM(body, options), "fromFile",
      (jsdom, value) => jsdom.#createdBy = value,
      (jsdom, ...args) => jsdom.#registerWithRegistry(...args)
    );
  }
}

function finalizeFactoryCreated(jsdom, factoryName, createdBySetter, registerCaller) {
  createdBySetter(jsdom, factoryName);
  registerCaller(jsdom, registry);
  return jsdom;
}

This code is basically worse than the original code with duplication. The lack of static private, and the workarounds I have been forced to employ to get around it, have made persisting with duplication a better choice than attempting to apply extract method---because extract method has basically been broken.


This is why I think static private methods are quite important. Without it you cannot apply basic refactoring patterns, which users will expect to be feasible, to JS classes.

gibson042 commented 6 years ago

static private methods and fields in the forms that have been discussed are highly problematic , essentially the straw that breaks the camels back.

I agree.

private instance methods are problematic because they violate the lexically-resolved/object-resolved distinction I described. They syntactically manifested as if they were object-resolved methods when they are really lexically-resolved functions. They start us on the slippery-slope of complexity and confusion.

I strongly agree.

there are no issues with "static public fields". They are just initialized data properties of a constructor.

I wouldn't say no issues, but everyone (including me) seems to be comfortable with normal Javascript prototype behavior (including copy-on-write semantics) if static private fields are taken off the table.

No issues with private and public instance "fields".

Strange situations are certainly possible, but I agree that they don't really feel like new issues.

class A { #private = "A"; getter() { return this.#private; } }
class B { #private = "B"; getter() { return this.#private; } }
var traitor = Object.setPrototypeOf(new A(), B.prototype);
A.prototype.getter.call(traitor); // => "A"
traitor.getter(); // => TypeError

So, I'd be relatively happy if we rolled out just public/private instance fields and won't have a problem if we also included public static "fields".

I think if we include class body lexical 'function` declaration we will have covered all the key class capabilities and won't have to do additional work (except for decorators) on class definitions for a long while.

I've been persuaded. It's extremely unfortunate that the class fields proposal introduces a new "private" concept that doesn't apply to any other class feature, but not technically fatal. The table would end up like this:

function/method field
instance ES2015 [proposal-class-fields](https://github.com/tc39/proposal-class-fields)
static ES2015 [proposal-static-class-features](https://github.com/tc39/proposal-static-class-features)
private functionality "class body lexical declarations" "class body lexical declarations", also [proposal-class-fields](https://github.com/tc39/proposal-class-fields) `#private` one-off
littledan commented 6 years ago

@allenwb I agree with @bakkot's argument in https://github.com/tc39/proposal-static-class-features/issues/4#issuecomment-356198953 .

To clarify where we are in committee discussion: I brought the question about whether it's appropriate to pursue private methods given the lexically scoped function alternative possibility, to TC39 twice: in the July 2017 and September 2017 TC39 meetings. We discussed multiple alternatives, including function declarations in class bodies, private method syntax with lexical function declaration semantics, and private method syntax with private field semantics. The committee reached consensus in July on the third alternative. With this option, private methods reached Stage 3 in the September 2017 meeting. Later, in the November 2017 meeting, we confirmed that private instance methods are at Stage 3.

I believe Allen's arguments in favor of lexically scoped function declarations instead of private methods were made well-known to the committee. I tried to present them in my private methods presentations, but Allen had also made them directly to the committee in the past. So, I'm not sure what new information we have to revisit these decisions.

@gibson042 About using lexical scoping rather than # for private fields: see this FAQ entry for why we decided against that option.

littledan commented 6 years ago

The difficulty I keep having with @allenwb 's lexical scoping proposal is that it seems odd to me to have a bare function declaration or let declaration in the middle of a class body. I was discussing this with @ljharb and he had an interesting idea for what the syntax could be:

class C {
    static function x(v) { return v+y }
    static let y = 1;
    method() { return x(1); }
}
new C().method();  // => 2

What do you think? We could use a different keyword instead of static if anyone has any other ideas.

ajklein commented 6 years ago

@littledan At first glance I like this direction! And we already have precedent to prefix declarations with a keyword, for module exports.

ljharb commented 6 years ago

Note as well; my suggestion wouldn't necessarily preclude static #foo() {} or static #bar, it could complement it - the # syntax could be installed on subclasses, and the lexical ones wouldn't be. That might help accommodate the two mental models @allenwb's describing.

allenwb commented 6 years ago

We could use a different keyword instead of static if anyone has any other ideas.

class C {
    class function x(v) { return v+y }
    class let y = 1;
    method() { return x(1); }
}
new C().method();  // => 2

It explicitly says: I am defining a class (scoped) function or variable. static doesn't seem quite right as the current usage of static basically means "associated with the constructor object" and that isn't what these definitions do.

The difficulty I keep having with @allenwb 's lexical scoping proposal is that it seems odd to me to have a bare function declaration or let declaration in the middle of a class body.

This seems symmetric with the odd feeling I get when I see what looks like an assignment statement (ie, a instance field declaration with an initializer) in the middle of a class body. I suspect we would both would get over those feelings after enough usage.

One of the arguments for the keywordless field definitions was that the keyword is unnecessary noise and that developers will be annoyed by the extra typing. The same argument seems applicable here.

But, I even with a redundant keyword, I think this would be much better than what has been kicked around for private methods and static private fields.

allenwb commented 6 years ago

@ljharb

Note as well; my suggestion wouldn't necessarily preclude static #foo() {} or static #bar, it could complement it - the # syntax could be installed on subclasses, and the lexical ones wouldn't be.

Let me make a suggestion for a better way to think about static # inheritance. Unlike class instances, class constructors are singleton objects. Static fields (properties or private slots) are defined directly upon the constructor objects and constructor inheritance chains go directly from subclass constructor to superclass constructor without going through intermediate prototype objects. Constructor inheritance is pure prototypal inheritance.

There is one other context in JS where we declarative define single objects without also defining an associated prototype -- Object literals. To me, the semantics of static field definitions seem most like the semantics of object literals properties than the semantics of instance fields. So, when reasoning about what would be reasonable behavior for static # fields I always try to think about what I would expect if there was a way to defining #fields within object literals.

For example:

let obj1 = {
   #ctr: 0,
   get counter() {return this.#ctr},
   increment() {++this.#ctr}
};

let sub = {
   __proto__: obj1
};

console.log(obj1.counter); //0
sub.increment();
console.log(obj1.counter); //1

I really think we should steer away from private static fields (at least for now) but if you want think about them, think about object literal singletons.

ljharb commented 6 years ago

The semantics you describe make sense to me, ftr.

ljharb commented 6 years ago

Also i think class does make more sense as a keyword than static, for the lexical variations, because that’s the scope they have.

wycats commented 6 years ago

@littledan I really love this direction.

One thing that's been gnawing at me this entire time (and which @allenwb has repeatedly pointed out) is that all static private features are easily modellable using normal closures. And since private static methods are lexical, they're really just a convoluted way to describe a lexically scoped function.

The only reason that isn't sufficient to make that the end of the story here is that we need the functions to be on the inside of the class body in order to have access to instance state defined inside the class body.

@allenwb is correct that allowing lexically scoped functions inside the class body addresses the use cases for static private fields with reasonable ergonomics and a clear programming model. The issue with putting a bare function inside the class body (for some of us, at least) is that adding bare functions to a class could confuse people about what the semantics of a class body are.

@littledan's proposal to use a prefix modifier like static or class eliminates that concern for me (at least at first glance), which means that the use-cases for static/private would be fully covered by class functions! We might eventually want static/private for some reason, and I wouldn't want to completely foreclose the possibility, but we would have a coherent system with the addition of static/public fields + instance/private fields/methods/accessors + class functions even if we never decide to do static/private fields/methods/accessors.

TL;DR I would love to see us decouple static/private stuff from this proposal and focus our efforts on class functions for those use cases for now.

littledan commented 6 years ago

Bikeshedding: One thing I liked about @ljharb's suggestion of static is that this semantically implies that the declaration is not related to the instance (even if it's technically a mismatch since it doesn't really have to do with the constructor either). class seems a little odd to me because:

Note that we're not restricted to existing keywords here and can use basically anything as a contextual keyword (as long as we're ok with the no-newline-after limitation).

littledan commented 6 years ago

Next, in JS we have basically two kinds1 of procedure calls baked deep into the call semantics: lexically-resolved calls and object-resolved calls. As should be obvious from the names I choose, the primarily difference is how the actual invoked function is determined. But the two different call forms also have an impact on how programmers conceptualize and use procedural decomposition.

I'm not sure I agree entirely with this phrasing of the problem space. I think there are two reasons programmers may put code in a method:

The private instance methods proposal opts to use method syntax, rather than lexically scoped function syntax, to retain consistency and parallelism for the second case. Where a method uses this (even if many JS programmers find this to be a mis-feature), private method syntax facilitates easy refactoring between public and private ways to factor out behavior. These benefits are even despite the fact that there is only ever one value for the method, so it can't be said to really "dispatch".

I really think we should steer away from private static fields (at least for now) but if you want think about them, think about object literal singletons.

This seems like a really useful lens; thanks for bringing it up. The original proposal for class fields would throw a TypeError in both cases and be consistent with respect to subclassing. (I actually wrote out a sketch of what object literal semantics would be in an attempt to create consistency here; however, I went back and removed it because it seemed too limiting to have object private fields scoped only to a single literal.)

However, the current draft spec, due to inheriting private static methods and accessors, loses this property: Private static methods are inherited (copied) by extends, but not by prototype chains of instances. We could try to recreate this property with changes to Object.create, __proto__ in literals, etc, but this could get very complicated.

littledan commented 6 years ago

@bakkot and @rbuckton both separately brought up another possible syntax: using a static { } block in a class, with declarations inside that block having the semantics that they are available in the whole class body ("hoisted"):

class C {
    static {
        function x(v) { return v+y }
        let y = 1;
    }
    method() { return x(1); }
}
new C().method();  // => 2

What do you think of this? (emoji reacts welcome)

Advantages:

Disadvantages:

jridgewell commented 6 years ago

This breaks the property that curly braces always create a new lexical scope

I think this is a pretty large con. Being able to reference declarations that are defined inside an inner curly brace feels really bad.

bakkot commented 6 years ago

A bit behind here...

@allenwb

Is this conclusion data based?

Not precisely. As you know, it's quite hard to get good data here, which is why we lean on educators and other people with exposure to broad parts of the community. On this particular question, though, I think every single person I've ever talked to about it had the expectation that they'd be per-instance. Here's an example from the private fields discussion, which a lot of people apparently felt was reasonable despite there being no way it could possibly work without fundamentally changing what closures are; later someone says they tried using lexical declarations in class bodies in exactly this way (because an older JS pattern) and were disappointed when it didn't work.

It seems to me that much of the difficulty we've had with extending class declaration functionality has been dealing with expectations that programmers (and us) may carry over from other languages.

I actually don't think this is attributable to experience in other languages. One reason is that there's been a JavaScript pattern taught for many years which suggests using variable declarations in constructors to create (per instance) "private members": here's Crockford giving it, for example. And yes, that's not the situation here because the methods in a class body are shared, rather than being created every time the constructor runs, but especially given the examples I've linked I'm not convinced the instinctive mental model of lay programmers makes that distinction.

private instance methods are problematic because they violate the lexically-resolved/object-resolved distinction I described. They syntactically manifested as if they were object-resolved methods when they are really lexically-resolved functions. They start us on the slippery-slope of complexity and confusion.

I'm not sold on this particular claim. Because of the type check, the semantics are equivalent to "define immutable field on objects which are instances of this class, with a name which no other objects can share or reference", and not equivalent to "define a lexically-resolved function".

I also agree with @littledan's reading of how people tend to think about functions vs methods. As an example, if I have an object foo which is an array, and I call foo.map(something), I am 100% of the time expecting to this to be Array.prototype.map. This isn't the case for all methods - sometimes I really am intending to do dynamic dispatch - but it is for some of them.

class function

This would be confusing for me personally, but I think I like it better than static. local or inner would also do (as in "inner class"). Though I'm also ok with bare function declarations, just not bare let/const declarations.

I think of these my preference is for inner-prefixed general lexical declarations or bare function declarations.

littledan commented 6 years ago

OK, it seems like we have the shape of a possible solution here:

The next steps from here would be:

Although the above proposal is significantly different in semantics, it's an iteration on the same problem space, so I don't think it requires the process overhead of a new staged proposal.

littledan commented 6 years ago

I'm developing an alternative explainer and spec text based on lexical declarations in https://github.com/tc39/proposal-static-class-features/pull/10 .

wycats commented 6 years ago

@littledan I agree with your analysis, but especially with the point that private instance methods are still justified.

littledan commented 6 years ago

Draft spec text for adding lexically scoped declarations; see discussion of semantics in the commit message. Better explainer text coming soon. Any feedback would be very appreciated. https://github.com/tc39/proposal-static-class-features/pull/10/commits/4a2d164f5f3b58985ba40ce6405e5026b382d692

littledan commented 5 years ago

We have concluded that private static will be part of this proposal, and it has advanced to Stage 3.