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

Should static fields be created per subclass, or just in the superclass? #2

Closed
littledan opened this issue Dec 13, 2017 · 12 comments
Closed

Comments

@littledan
Copy link
Member

Continuation of tc39/proposal-class-fields#43 (comment) . To summarize previous discussion in that thread and at TC39:

@bakkot has argued that it would be more intuitive and consistent with instance fields to have static fields created on all objects, not just the superclass, with a number of desirable properties:

  • Avoids confusing situations of inherited data properties
  • Avoids the issue of private static fields and methods causing TypeErrors when accessed on subclasses
  • Could be considered more analogous to instance fields (which are also created multiple times)

@jridgewell has mentioned that he has some issue threads which show the per-subclass semantics to be better; I'm not sure where these are, and it would be great to have this data.

@ljharb, @zkat and others have brought up that the superclass-only semantics are common in the way people write code directly: Defining a class or constructor function and setting properties on it directly. Allowing syntax for this would give a little bit more freedom to class authors--it would clearly make static fields in the "good path" that the blessed class syntax supports, and allow freely declaring the static field anywhere the author wants inside the class.

@rbuckton said that static fields existing just as properties in the classes where they are declared is the current TypeScript semantics for some time, and programmers have not reported issues with it.

@littledan's position: We should just create one static field in the class where it's defined. In conjunction with not pursuing static private fields or methods, I think this should work well.

Making a field per subclass seems to me like encoding an idiom that seems rather odd to me: subclassing as a sort of stateful factory. Using in classes in this sort of stateful way seems like something we should allow (it's already possible, if you ask subclass-ers to call a static method to initialize the subclass), but not necessarily encourage with built-in language features.

I'm happy to be convinced otherwise. In particular, if anyone has concrete cases that are not well-served by TypeScript's static field semantics, I'd be really interested to hear them.

@bakkot
Copy link

bakkot commented Dec 13, 2017

Re: @ljharb and @zkat's point: there's an important difference between the current semantics and the "old way", which is that one did not normally make the superclass itself the prototype of the subclass.

That is to say, while people may have written

function Base(){}
Base.prop = 0;

function Derived(){}
Derived.prototype.__proto__ = Base.prototype;

or similar idioms, basically no one was including Derived.__proto__ = Base, as far as I can tell. So the issue with prototypical inheritance of data fields would not have come up.

(Does anyone know why ES6 classes include that? It seems weird to me. I guess it's consistent with what other languages with the class keyword do.)


Re: @rbuckton's point: TypeScript actually does copy properties to the subclass in environments which don't support modifying prototypes, so if people haven't complained about their semantics, that suggests putting properties on subclasses would work (or no one cares if their code breaks in older environments, I guess).


Anyway: I am currently leaning towards not pursuing static private at all, in which case I would also be in favor of the current semantics for static public. While I don't like the semantics for inheriting static public fields, I don't think they're quite worth changing on their own; it would only be worthwhile if we wanted to use the same mechanism to make static private to work with subclasses.

But I'd also like to hear from Backbone or any other projects which ran into the prototype-placed data field problem.

@jridgewell
Copy link
Member

jridgewell commented Dec 13, 2017

Sorry, I'm really terrible with follow through. 😳

I spent about an hour Monday finding bugs in Backbone's issue tracker, and came up with the following (I know there are many more, this is just what I found):

Some background: All of these bugs have to do with Backbone's encouraged inheritance pattern. Defining a subclass with Sub = Base.extend(protoProps) places the properties on the Sub.prototype just like with ES6 class and instance fields.

However, the defaults field (and events, routes, etc) is really nothing more than a static field that we decided to place on the prototype to make class declarations prettier. It is exactly equivalent to React's .propTypes, they just happen to put it directly on the constructor.

Anyways, it is very unusual for someone to directly modify this.defaults, since that doesn't reflect anything useful on the model (the model's attributes are stored in this.attributes). Instead, they would use this.get('some_defaulted_field').nested = 1, and end up modifying the nested property for every instance (including sub instances) of Base.


So these aren't perfect issues, because even if every subclass re-ran the "static" initializer, we'd still have bugs where we copy the static property into this.attributes and that gets modified. But I do have a an actual use that my coworkers and I wrote into production:

const Cacheable = Backbone.Model.extend({
  constructor(attributes) {
    if (attributes && attributes.id !== undefined) {
      const id = attributes.id;
      const cached = this.constructor._cache[id];
      if (cached) {
        return cached;
      }

      this.constructor._cache[id] = this;
    }
    
    Backbone.Model.apply(this, arguments);
  }
}, {
  _cache: {},
});

const Sub = Cacheable.extend({});

Here, we're trying to define a cachable base class that will allow us to share instances with the same id. We're using Backbone's .extend(protoProps, staticProps) again, this time passing in a staticProps arguments, which attaches fields to the constructor just like static fields do.

But, there's a bug. Because the Sub._cache === Cacheable._cache, we end up getting other subclass's instances returned, where we expected Sub._cache !== Sub2._cache. So to fix it, we had to enforce (with a runtime check, because this was 2014 and we couldn't lint yet) that every subclass had to define _cache: {}, which was private data that only Cacheable should have to know about. (Did you know I was going to tie in private static fields, too? 😉)


So I think it's worthwhile to at least consider re-running the initializers on every subclass. As far as I know (I'm sure there's an example), this covers all the current library code use cases. And if you really want to share the exact reference, you could define an accessor/copy-initializer:

class Base {
  static share = {};
  static accessor_share = 0;
}

class Sub = {
  static share = Base.share;
  static get accessor_share() {
    return Base.accessor_share;
  }
}

@littledan
Copy link
Member Author

Does anyone know why ES6 classes include that? It seems weird to me. I guess it's consistent with what other languages with the class keyword do.

I don't know all the history here (I heard rumors of some meeting outside of TC39 at the end of 2014 where everything changed), but some of the ES2015 standard library, e.g., the way @@species works, depends on constructors forming a parallel inheritance chain.

TypeScript actually does copy properties to the subclass in environments which don't support modifying prototypes, so if people haven't complained about their semantics, that suggests putting properties on subclasses would work

It's pretty remarkable that TS has such a big variance in semantics and it ends up working out for their users! I believe @rbuckton also mentioned at TC39 that TS used to do this all the time, until they upgraded to ES2015 class semantics. Anyway, I agree that this is good evidence that code won't be broken by that alternate semantics.

@allenwb
Copy link
Member

allenwb commented Dec 13, 2017

(Does anyone know why ES6 classes include that? It seems weird to me. I guess it's consistent with what other languages with the class keyword do.)

It is consistent with what other dynamically typed languages with classes have done. The utility of such methods has been demonstrated in those languages.

Note that the word "static" is a misnomer as there is nothing static about such properties. They are simply normal (ie dynamically modifiable) properties of the constructor function defined by a class definition. Better terminology would be "class property" or "constructor property". During development of ES6 this naming concern was discuss and alternative keywords were briefly considered. But the consensus was that the Java/C++ precedent for this use of the "static" keyword was too strong to deviate from.

One important use case is to define static methods (often factories) that are directly usable (via inheritance) on the subclass. Such methods will typically invoke (possibly indirectly through other static methods) a possibly subclass constructor using code like new this().

As a specific example consider somebody defining a new kind of collection class where they want to have from and of methods similar to Arrays. the definitions of those methods might look like:

static of(...elements) {
   return this.from(elements)
}

static from(iterable) {
   let coll = new this();
   for (const el of iterable) coll.append(el);
   return coll;
}

@littledan
Copy link
Member Author

It is consistent with what other dynamically typed languages with classes have done. The utility of such methods has been demonstrated in those languages.

Do you have any examples on hand?

@allenwb
Copy link
Member

allenwb commented Dec 13, 2017

see above example of of and from.

Another common use case is for a base constructor makes use of(eg, invokes) a over-rideable static method that is provided to allow subclasses to inject behavior into the base constructor.

These are common idioms in dynamically typed class based languages (ie, not C++/Java/C#) All discussed during ES6 should in notes including (I believe) some presentations.

@allenwb
Copy link
Member

allenwb commented Dec 13, 2017

I could (probably have) separately explain why private fields are a completely different beast from properties and hence the same idioms don't really work for "static" private fields.

Public static fields (ie data properties of the constructor) aren't a problem as they are just ordinary properties.

@littledan
Copy link
Member Author

littledan commented Dec 13, 2017

Thanks for these cases; this is really useful. Sorry for being unclear, but I was also interested in examples of programming languages which use these semantics. Do you know where to find this in the notes?

@allenwb
Copy link
Member

allenwb commented Dec 13, 2017

examples of programming languages which use these semantics

Smalltalk, Ruby, Python

Do you know where to find this in the notes?

Probably sometime in 2012±1.
I recall at least one discussion where I showed examples like the above probably more. Depends upon how good the note takers were back them.

@littledan
Copy link
Member Author

@jridgewell Thanks for these references. It was interesting reading about Backbone and how users have been encountering things with models.

Some background that I just learned from these bugs and the Backbone source: Backbone's simple class system has convenient syntax for prototype properties which may be functions or data. To access these, there's frequent use of Underscore.js's _.result, which will call a method if it is a function or return the data if it is data. Backbone.Model augments this with a couple ways to have data properties for instances: there is a defaults method/property that you can set up, and you can also pass in an options bag. These together will be accessible with the get or set methods, where set fires events as appropriate (these get and set methods are sort of the core of how you communicate between the View and the Model, I gather). Please correct me if any of that is wrong.

The problem that a lot of programmers in these bugs run into is, if you provide an object rather than a function as your default value in a Backbone.Model object, then each time the model is instantiated, its attributes will point to the same values. For example, if one of them is an empty array, you probably actually wanted a different array to come with each instance of the model, rather than the shared one! A solution, as @jridgewell helpfully told people on a few of these bugs, is to make default a method instead.

I can see that there's been a detailed investigation into using ES6 classes that @jridgewell has been involved in, but I'm not sure exactly what were your thoughts on subclassing Models. I don't know nearly as much as you do about the topic, but I had a couple of vague ideas as a first impression:

  • Use a static field for the defaults (Is this related to what you're thinking about, @jridgewell ?). I'm not sure how this could work. Even if static field semantics were to re-evaluate on subclassing, how would you make sure to do the re-evaluation on multiple instances of the same subclass?
  • Use a static method for the defaults. This is kind of analogous to the current solution of advising users to pass a default function, but maybe it's not ergonomic enough (any other downsides?). if you want to enforce method-ness only when it's inherited by an ES6 class, you can check !(new.target === Model || new.target === undefined) (though I guess this wouldn't work well if user code is transpiled and Backbone.js is not).
  • Use decorated field declarations for attributes. The @observed decorator sample in the decorators proposal seems pretty similar: If you use a decorated instance field declaration, the decorator could pass off the name and initialization thunk into some metadata that the Backbone.Model constructor could use in place of defaults.

Was there some other pattern you were imagining to make this work?

@littledan
Copy link
Member Author

Sorry, @jridgewell, I read your previous post incompletely. About the cache use case: Would it work to use a factory pattern, like this:

function makeCache() {
  let cache = {};
  return Backbone.Model.extend({
    constructor(attributes) {
      if (attributes && attributes.id !== undefined) {
        const id = attributes.id;
        const cached = cache[id];
        if (cached) {
          return cached;
        }

        cache[id] = this;
      }
    
      Backbone.Model.apply(this, arguments);
    }
  });
}

const Sub = makeCache();

@littledan
Copy link
Member Author

It seems like we are fairly settled on static fields being initialized only a single time at this point.

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

4 participants