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

Uninitialized properties should still exist #57

Closed mbrowne closed 7 years ago

mbrowne commented 7 years ago

First of all, I hope that opening an issue here is the correct starting point for suggesting a change to the proposal. @loganfsmyth (one of the Babel maintainers) suggested that I open a new issue, since my suggestion is kind of independent of this issue that I commented on. *

Currently, the proposal specifies that properties that aren't given default values should be completely ignored, i.e. this:

class Demo {
    foo;
}

translates to this:

function Demo() {
    //no mention of foo property whatsoever
}

Beyond the fact that one would expect the runtime environment to be aware of the property even if it hasn't been initialized yet, this is problematic because you can't enumerate over the property or check for its existence in any way before it is given a value:

let d = new Demo();
('d' in foo); //false

As I explained here, this makes some common and useful patterns impossible to implement using class properties.

I propose changing the spec such that the above example would transpile to this:

var Demo = function Demo() {
  ...
  this.foo = undefined;
};

Or the more spec-conformant version:

var Demo = function Demo() {
  ...
  Object.defineProperty(this, "foo", {
    enumerable: true,
    writable: true,
    value: undefined
  });
};

Logan pointed out here that this would mean that uninitialized properties in subclasses would be set to undefined even if the parent class specified a default value, for example:

class Base {
    foo = 4;
}
class Child extends Base {
    foo;
}

let c = new Child();
c.foo; //undefined

After considering it and comparing with the behavior of Java and PHP, I'm thinking that the above behavior is how it should work, even if it's initially surprising to some users. As I said in my reply to Logan:

If I'm explicitly declaring a property in a subclass, I don't think I'd want the addition of a default value for that property in the parent class to change the behavior of the child class -- especially if the parent class is maintained by a 3rd party.

However, I think this is a topic that should be discussed to weigh the pros and cons. If it's decided that uninitialized properties shouldn't overwrite the default value specified in a parent class, then the implementation could be modified accordingly, for example:

function Child() {
    Base.apply(this, arguments);
    this.foo = ('foo' in this) ? this.foo: undefined;
}

Or more succinctly:

function Child() {
    Base.apply(this, arguments);
    this.foo = this.foo;
}

(Technically this alteration would only be needed for subclasses, but perhaps it would be simpler to be consistent.)

I look forward to hearing people's thoughts on this - thank you.


* But it's still related, and I think it would be ideal for instance properties to exist on the prototype somehow (as long as non-primitive default values are defined in the constructor rather than the proprotype).

mbrowne commented 7 years ago

P.S. This is how it works in Java:

class Base {
    public int foo = 4;
}

class Child extends Base {
    public int foo;
}

class Program {
    public static void main(String[] args) {
        Child c = new Child();
        System.out.println(c.foo); //outputs "0"
    }
}
ljharb commented 7 years ago

This is already the plan of record, as I understand it.

bakkot commented 7 years ago

Strongly agree that foo; should be equivalent to foo = undefined;. I think it's currently the intent of the authors too; at least, it's what @littledan said in his slides at the January TC39 meeting.

I think it's actually a bug that it's not what's currently specified. This commit introduced the behavior you propose for static fields, but not instance fields.

--

Edit for pedantry:

Technically this alteration would only be needed for subclasses, but perhaps it would be simpler to be consistent

It's possible to detect the difference between a property which is present with a value of undefined and a missing property, so in fact even base classes and classes which extend null need this.

mbrowne commented 7 years ago

Thanks! This is good news.

It's possible to detect the difference between a property which is present with a value of undefined and a missing property, so in fact even base classes and classes which extend null need this.

It probably doesn't matter now, but to be clear, I was saying that this.foo = ('foo' in this) ? this.foo: undefined would only be needed for subclasses. Agreed, base classes would still need to set this.foo = undefined.

ljharb commented 7 years ago

Base classes still inherit from Function.prototype and Object.prototype.

mbrowne commented 7 years ago

So what's the next step; are we just waiting for the authors to confirm this change? Note that I already implemented this in Babel (pull request here) but I imagine that the spec will need to be officially changed before the pull request can be accepted.

littledan commented 7 years ago

I don't know if there's any change to make. I believe the semantics you're talking about here are already what the draft specification requires.

bakkot commented 7 years ago

@littledan I think it's missing for instance fields. Per 9.a.iii, nothing happens if initializer is empty. Should be a simple fix but I can't make it right now

mbrowne commented 7 years ago

I was going to submit a pull request for this, but it looks like it's already updated to set uninitialized properties to undefined in the master branch: https://github.com/tc39/proposal-class-public-fields/blob/master/spec/new-productions.htm. Maybe someone already changed it but didn't update the gh-pages branch so the github page is out of date? Another difference is that the source file in the master branch doesn't mention steps 3-7 here.

jeffmo commented 7 years ago

That might've been me -- I can update the gh-pages from master later today

Sent from my iPhone

On Feb 12, 2017, at 9:45 AM, Matt Browne notifications@github.com wrote:

I was going to submit a pull request for this, but it looks like it's already updated to set uninitialized properties to undefined in the master branch: https://github.com/tc39/proposal-class-public-fields/blob/master/spec/new-productions.htm. Maybe someone already changed it but didn't update the gh-pages branch so the github page is out of date? Another difference is that the source file in the master branch doesn't mention steps 3-7 here.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

jeffmo commented 7 years ago

gh-pages updated now

mbrowne commented 7 years ago

Great! Thank you.