tc39 / proposal-class-public-fields

Stage 2 proposal for public class fields in ECMAScript
https://tc39.github.io/proposal-class-public-fields/
487 stars 25 forks source link

What should `this` be bound to in initializers? #34

Open littledan opened 8 years ago

littledan commented 8 years ago

Options:

Any thoughts? @michaelficarra @erights @domenic @jeffmo @zenparsing @allenwb others

We have discussed this a bit, but I didn't see an open bug.

zenparsing commented 8 years ago

I prefer option 2 (undefined or TDZ). Rationale:

jeffmo commented 8 years ago

Important also to distinguish "instance" vs "static" fields:

I believe it is both useful and important that this in instance fields should refer to the instance, and this in static fields should refer to the class object.

For instance fields: It is often useful to use methods on the prototype chain to calculate the initialization value of a field, so having access to this is important in these cases.

zenparsing commented 8 years ago

so having access to this is important in these cases.

My point of view, from the private field side of things, is that the initializers are only there as sugar for when you are initializing to some obvious initial value. For instance:

class C {
    #list = [];
    constructor() { }
}

I've always just imaged that anything more complicated (including dereferencing this properties) should just go in the constructor, where all of the other complex initialization logic resides.

ljharb commented 8 years ago

Option 1. Option 3 is unintuitive and confusing, and Option 2 cripples the usefulness of the proposal.

One concrete use case: a React "class" component, with a getInitialState prototype method, with state = this.getInitialState() (that way anything else that resets the state can re-call the getInitialState method to get a fresh state object).

jeffmo commented 8 years ago

Binding this to the instance would necessitate a novel scope, where this is not lexically bound to the containing scope, but arguments is. In general, I would prefer not to introduce any additional new scoping rules for this.

It is not possible to reason about the value of this anywhere in JS without contextual information. It may be a lexical binding, but it is expectedly parameterized on context. With the context of a property initializer, it's clear to most that I've polled that it is intuitive to expect this to refer to the instance in an instance field property.

Binding this will strongly encourage developers to write methods as arrow functions installed per-instance. I don't think we want to pave that path.

I don't agree here. When users wish to have memoized/bound methods, the pattern that is already used is this.foo = this.foo.bind(this);. I don't see any harm in this as long as it is done with consideration. The truth of property initializers is, of course, that the expression is evaluated on each instantiation.

All that said, there is a clear pattern that benefits from creating per-instance function properties (the most common use-case for this is setting method-listeners as event handlers). I think if we want to omit this capability on the grounds that it isn't useful or reasonable, we need to show that this use-case isn't useful or reasonable.

Additionally I would be interested to hear how difficult it would be for VMs to optimize the un-mutated form of a function-property? @littledan, thoughts here?

littledan commented 8 years ago

cc @verwaest

jeffmo commented 8 years ago

I've always just imaged that anything more complicated (including dereferencing this properties) should just go in the constructor, where all of the other complex initialization logic resides.

My only concern with this perspective is that it downplays the value of a pattern that many users do find useful. I think it's a reasonable perspective as a style for some cohort of users, but I don't think we have shown sufficient grounds to ban it at the language level when it is clearly useful for some valid patterns.

appsforartists commented 8 years ago

If you're not careful with the first option, you end up with this sharp edge:

class Thing {
  _something = this.makeSomething();

  makeSomething() {
    // constructor hasn't run here yet
  }
}

There's definitely value in allowing a property's value to be the result of an expression (for instance, it allows you to call other object's constructors rather than only primitives). However, an author may reasonably assume that a class's methods will be run after the constructor. If we're not thoughtful here, we may break that assumption. (The Babel implementation currently does.)

Would it be possible to parse property definitions in source order, blacklisting method calls?

Being able to say


class Thing {
  _thing = new Rx.Subject();
  _otherThing = this._thing.map(…);
}

is really nice, but


class Thing {
  _thing = this.makeThing();
}

seems like it shouldn't work.

littledan commented 8 years ago

@appsforartists I don't see why we should blacklist method calls from initializers when we don't blacklist them from within constructors currently.

appsforartists commented 8 years ago

Maybe my concerns are off-base. Allowing methods in property initializers feels like a foot-gun.

To be fair, this isn't a bug I've personally encountered, just one I got curious about exploring.

littledan commented 8 years ago

Why is it a different amount of footgun than allowing method calls from within the constructor?

bakkot commented 8 years ago

@appsforartists, I feel like it would be more surprising if other properties were visible but methods were not. Since this issue only applies within a single class (i.e. since the super constructor will have run by the time you could call a method from the superclass), I don't think allowing method access is too risky.

appsforartists commented 8 years ago

That case wasn't one that came to mind when I raised the concern.

This is potentially more foot-gunny, since it expands the scope of places where a method may be called before the constructor has completed (and indeed is the only place where a method may be called before the constructor has even been called). Still, I understand the objection to my concern.

littledan commented 8 years ago

I think the only reasonable way to prohibit method calling is to prohibit access to this entirely.

ljharb commented 8 years ago

Some methods don't need the constructor to have been called; some methods can be called as long as some things have been initialized on the instance. The same hazard exists with:

constructor(x) {
  this.foo(x);
  this.bar = {};
}
foo(x) {
  this.bar.baz = x;
}

If your own implementation has ordering dependencies with the constructor, than your instance properties will require the same ordering dependencies. This doesn't seem to me to be a footgun as much as a natural consequence of writing code that is brittle in that way.

bakkot commented 8 years ago

As I understand it, discussion today seemed to settle on treating initializers as the body of an anonymous method (static method for static fields) for the purposes of this, super, new.target, and arguments.

erights commented 8 years ago

yes. One invoked with no arguments, so arguments would have a zero length.

On Thu, Jul 28, 2016 at 8:23 AM, Kevin Gibbons notifications@github.com wrote:

As I understand it, discussion today seemed to settle on treating initializers as the body of an anonymous method (static method for static fields) for the purposes of this, super, new.target, and arguments.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeffmo/es-class-public-fields/issues/34#issuecomment-235809579, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQtzFJAyiifygdmSk4U3cCpyYK4XSB0ks5qaEragaJpZM4H_UlZ .

Cheers, --MarkM

ljharb commented 8 years ago

Unfortunately I missed the rationale for that, but it seems unfortunate that if I move my constructor code using arguments to an initializer, it would silently work differently - I feel like the early error would be much more useful.

erights commented 8 years ago

I do agree. Please reraise this specific issue.

On Thu, Jul 28, 2016 at 5:32 PM, Jordan Harband notifications@github.com wrote:

Unfortunately I missed the rationale for that, but it seems unfortunate that if I move my constructor code using arguments to an initializer, it would silently work differently - I feel like the early error would be much more useful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jeffmo/es-class-public-fields/issues/34#issuecomment-235931984, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQtzE3tlwE15xeUDhsf_rB5_2ZtkRdqks5qaMuTgaJpZM4H_UlZ .

Cheers, --MarkM

littledan commented 8 years ago

We have a choice to make: Should we have special handling of arguments, as in the current spec text, or should we make it analogous to something pre-existing, which would have an empty arguments? Either is technically feasible, both from a spec text and implementation perspective. How should we weigh these alternatives?

Overall, I think of arguments as an "old, bad style" feature, and I think users are recognizing this too, using rest parameters instead. Maybe they'll switch to ES2015 features in refactorings before they switch to more advanced class features that come in later versions of ES, and this question won't become an issue for them.

jeffmo commented 8 years ago

cc @allenwb who was in favor of initializer bodies having the semantic intuition of a method body

ljharb commented 8 years ago

I think that it is critical to match most users' mental model so that in the following example:

class Foo {
  foo = bar; // 1a
  [baz] = quux; // 2a

  constructor() {
    this.foo = bar; // 1b
    this[baz] = quux; // 2b
  }
}

that - separate from named constructor arguments, which we've all agreed multiple times only make sense when lexically available - that lines 1a/1b, and 2a/2b, for any given LHS value (foo or baz), and any given RHS value (bar or quux), either behave exactly the same in both positions, or, have an error (early or otherwise) in one or both positions. The same should be true even if we come up with an alternative syntax for = because of a different mental model we want to respect (declarative vs imperative, for example).

We can separately decide what semantics we want (set vs define, for example) and what syntax we want, but I think it's a massive refactoring hazard if I can move an initializing line out of a constructor, remove the "this" or "this.", and have it silently behave differently. The same should hold if I move an instance property initializer line into a constructor. I think that allowing arguments inside the class body to mean anything other than an error, or "the constructor's arguments object" (which we don't want, for a number of reasons), would be a huge mistake.

jeffmo commented 8 years ago

@ljharb: It's worth mentioning that your example does behave the same. It's only the [relatively rare] case where the user uses arguments (and not the less-deprecated rest-arg) that it would not. I realize you used bar and quux as higher-level placeholders, but I want to point out the importance of considering relevancy of the specific case.

As much as it's important to consider the holistic/general composability of stuff like this, it's also important to consider the likelihood of encountering various incongruencies. Additionally, as mentioned in the discussion yesterday: There are many views of "consistency" that can often conflict. In this case, one view of consistency is to say that initializer bodies are consistent with method bodies. Another view of consistency is to say that initializer bodies are consistent with behavior of the same logic in a constructor.

As for where I stand: I don't feel super strongly here, but at the moment I find the "consistency across class members even in rare cases" perspective a little more compelling than the case for "consistency with constructor initializers even in rare cases" (where "rare cases" is referring to the use of new.target/arguments in initializers).

The same should hold if I move an instance property initializer line into a constructor.

Even today, any time you move an arguments or new.target expression from one position to another you risk heavy contextual mis-match -- so I don't think it would be too surprising to expect this (even if done by mistake). There's really no good reason to use arguments or new.target inside an initializer expression though, so again I think it's reasonable to expect the initializer -> constructor refactoring of arguments/new.target to be extremely rare. The opposite direction seems more plausible (but also flawed WRT the expectation of the behavior of the expressions after factoring out of a contextual function body).

ljharb commented 8 years ago

@jeffmo You're right that it's a specific case - but it's an easy one to fix. That's why i think arguments MUST be an early error in a class body, which we all agreed to in May.

That there's no good reason to use either of those in initializer position is exactly why they should be forbidden by the grammar.

jeffmo commented 8 years ago

@ljharb: If we were to go back to early error, what would you say these expressions should do within direct eval?

ljharb commented 8 years ago

I'd assume a runtime error, although it'd be great if we could forbid direct eval in there too :-)

jeffmo commented 8 years ago

Unfortunately detecting direct eval statically is undecidable, so there's not much we can do there.

We could make it a runtime error only for direct eval, but we do have some precedent for removing static errors when there's a chance that static analysis can't entirely cover the validation (duplicate object properties in strict mode comes to mind).

jeffmo commented 8 years ago

I guess it's also worth pointing out that the current spec text (i.e. not updated since the latest committee discussion) bans arguments in a not-so-precise way -- which is that it bans any identifiers named arguments. The false positive here (albeit potentially rare) is:

let arguments = 42;
class C { p = arguments; }
ljharb commented 8 years ago

Is arguments a valid identifier at the top level of modules?

bakkot commented 8 years ago

No, but classes can be used in scripts. (Or rather, it's a valid identifier, but you can't bind to it a la let arguments.)

ljharb commented 8 years ago

I tried in the console and it wasn't valid, but I guess that's probably due to the console being implicitly wrapped in a function. I think that's still a fine way to ban arguments, tho.

appsforartists commented 8 years ago

I'd expect instance properties to have access to things outside the scope of the class (e.g. other variables in the same module and globals) and to this (which includes access to any other instance properties defined earlier in the same class). Treating them as if they're in the constructor (e.g. giving them access to arguments) feels contrary to my expectation of how blocks/scopes work.

FWIW, if you paste this in Chrome's console today, you can see that constructors have access to arguments:

class Thing { constructor() { console.log(arguments)}}
new Thing(1)
// [q]
jeffmo commented 8 years ago

Just to clarify: The discussion from yesterday does not specify that arguments should be the same arguments from the constructor -- only that it essentially always be an empty Arguments object.

Of course that decision is being called into question here -- which is fine -- but I don't think there are any options on the table that would make initializers share an arguments object with the constructor.

appsforartists commented 8 years ago

Ahh - I misunderstood.

Is there a reason you'd want to allow access to arguments at all? Seems a bit nonsensical to have it resolve to anything (other than undefined) outside the scope of a function.

bakkot commented 8 years ago

You might be inside the scope of a function:

function f() {
  class A {
    x = arguments[0];
  }
}

It would be odd for it to resolve to f's arguments (especially given that it does not see f's this), so it requires some sort of special treatment.

littledan commented 8 years ago

I wrote spec text what we discussed at TC39 in https://github.com/jeffmo/es-class-public-fields/pull/43

aluanhaddad commented 8 years ago

TypeScript has had this feature for quite a long time (for instance properties), and I think it works well. Basically, you get access to this only as an RValue and have no access to arguments whatsoever. I do not think anyone has requested access to arguments in this context. It is still dangerous because it allows initializers to access methods and unitialized properties, but it is rarely a problem in practice because, if you think of the class body as the expansion of the constructor into the enclosing scope and the declarations as assignments, the required initialization order is obvious.

jakwuh commented 8 years ago

I am pretty new to TC39 specs style so can't get on my own if instance property initializers are executed before/after super() call.

I think there is a clean thought: if they are executed before super() then even if we allow to call methods on a class being constructed from within property initializers then this keyword should be in TDZ, as it done currently in class constructor, otherwise there is an inconsistency here. But considering it works this way the next question I would ask myself: why this is in TDZ in class method, but not in TDZ in property initializers? Again, inconsistence and this explains wrong usage of this in property initializers. There are a lot of other patterns to implent the same functionality: make your method static and call it from property initializers, or create a function outside a class and then call it and so on.

And finally, if property initializers are executing after super() keyword, then I can't understand the confusion of using this as far as we used to use it in a class constructor after super() call.

jeffmo commented 8 years ago

@jakwuh: To clarify: Instance field initializers execute after the call to super() because the this object doesn't exist before then. this gets allocated during the super() call.