tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Properties are undefined when accessed via super from parent object #266

Closed eddyloewen closed 5 years ago

eddyloewen commented 5 years ago

Is it possible to access properties (that are overwritten in a child class) via super on a parent object?

With this example:

class A {
  foo = 1;
}

class B extends A {
  foo = 2;
  constructor() {
    super();
    console.log({foo: this.foo, superFoo: super.foo})
  }
}

const b = new B();

I get the the following output:

Object {
  foo: 2,
  superFoo: undefined
}

Is this the correct and intended behaviour? As I would like to access properties via super from the parent object.

ljharb commented 5 years ago

No, the child constructor can not access the instance until the parent constructor is complete.

In this case, since both class fields define properties on the instance, the 1 is completely lost when the 2 is defined, and there’s nothing left to access anyways.

aimingoo commented 5 years ago

@ljharb

class A {
  get foo() { return 1 }
}

class B extends A {
  get foo() { return 2 }
  constructor() {
    super();
    console.log({foo: this.foo, superFoo: super.foo})
  }
}

// try
let b = new B;

The result will be { foo: 2, superFoo: 1 } for same one input console.log(... superFoo: super.foo}). why get difference value by same interface?

@eddyloewen a good negative case. ^^.

ljharb commented 5 years ago

As an accessor, sure. Not as a field/data property. But the request was to access a subclass property from a parent class.

rdking commented 5 years ago

This is one of the trade-offs that come with fields. The regular expectation of being able to access the base class via super is broken for "fields".

ljharb commented 5 years ago

It is just as true for properties defined in the constructor in es6.

rdking commented 5 years ago

And again, the difference is that properties added in the constructor in ES6 are done explicitly, with well known side effects. No matter how you cut it, until everybody is up to speed on the gotchas of "fields", there's going to be a lot of notices about unexpected behavior, or lack of expected behavior. If only all of the trade-offs were this minute.

ljharb commented 5 years ago

I don’t think the inability of superclasses to access subclass implementation details is something that will cause a lot of confusion, because i suspect trying to do that is rare.

Let’s not derail this very specific issue with general complaints about the proposal, please.

eddyloewen commented 5 years ago

@ljharb Thank you for the clarifications. So the "problem" here is rather in the ES6 class-proposal than the class-fields-proposal correct?

nicolo-ribaudo commented 5 years ago

The problem comes from the fact that super accesses the prototype of the super class (usually it is similar to this.__proto__.__proto__, except for many caveats). For this reason, super can only access things shared by all the instances of the class (i.e. methods). Field values must be independent in each instance, so they can't be defined on the prototype but must be defined on this.

mbrowne commented 5 years ago

@eddyloewen

As I would like to access properties via super from the parent object.

It sounds like you're basically trying to access metadata about the parent class. This proposal does not include full reflection capabilities for fields (that was previously proposed and rejected as too powerful and unnecessarily difficult to implement - source). I'm not sure exactly what your goal is, but I think there are probably multiple ways you could store the metadata you need (including the default value of a field) in your own custom object. With decorators, it will probably become possible to do this sort of thing with less code (e.g. a field-level @reflect decorator).

rdking commented 5 years ago

So the "problem" here is rather in the ES6 class-proposal than the class-fields-proposal correct?

No. It's jut like @nicolo-ribaudo described. Since "fields" don't exist on the prototype, super cannot reach them. They could have easily decided to put them on the prototype with no real loss. However, they seem to have prioritized avoiding the issue of objects on the prototype rather than trying to remedy it (or maybe they couldn't come up with a solution they could all agree on).

mbrowne commented 5 years ago

@rdking

They could have easily decided to put them on the prototype with no real loss.

I think you mischaracterize the tradeoffs sometimes in a way that might sound objective but isn't really. The "loss" in this case would be significant issues with backward compatibility, or the introduction of a new strict mode which would come with its own complexities. I'm not saying it's a bad idea (I might even be persuaded that it's a good one), but trying to retroactively fix something that's been an issue since day one of JS definitely comes with tradeoffs.

rdking commented 5 years ago

@mbrowne

I think you mischaracterize the tradeoffs sometimes in a way that might sound objective but isn't really.

I do not believe that to be the case. However, the same possibility occurred to me. This is why I just opened #267 . I want to see if I'm truly mischaracterizing anything here. My statement of "no real loss" is of the same nature as there being no real loss in private-fields not being compatible with Proxy. Both cases involve a problem that pre-existed this proposal. Yet one is being worked around at the cost of several trade-offs that directly affect the usability of the language in non-constructive ways, while the other is being left as is in favor of finding an all-encompassing solution later.

I'm not saying it's a bad idea (I might even be persuaded that it's a good one), but trying to retroactively fix something that's been an issue since day one of JS definitely comes with tradeoffs.

The notion of actually fixing the foot-gun directly is meant to be taken separately from the "no real loss" statement. Also, there are approaches that can fix the foot-gun without affecting legacy code. I'm not suggesting that anything be done that would affect legacy code as that's an immediate no-go.

This discussion is definitely off topic for this thread and should be taken up in #267.

rdking commented 5 years ago

@eddyloewen I'm curious. By any chance, were you expecting (or maybe hoping) that there would be C++-like behavior here?

#include <iostream>

class A {
   public:
     int value = 0;
}

class B: public A {
   public:
      int value = 42;
      B() {
         cout << "A.value = " << A::value << endl
              << "B.value = " << value << endl;
      }
}

B ex = new B();
/* Print out (unless I'm too rusty on C++)
A.value = 0
B.value = 42
*/

Were you wanting to use super like a scoping operator to get access to instance data belonging to the base class? If that's so, I'm afraid that will never work, even with ES6 classes and fields on the prototype. The problem is the difference between classical and prototype inheritance.

With classical inheritance, instantiation causes a complete instance object to be created and chained onto the instance for each ancestor. Each of these chained instances has only the members defined by its class. Since they're all instance objects, it's possible to access and manipulate inherited data without affecting others of the same class.

With prototype inheritance, the ancestors only provide the common prototype object. So there's no per-ancestor, instance-unique container to get the properties from. It isn't the case that this couldn't be done. However, if it were going to be done, TC39 would've had to take a completely different route with ES6 classes. That route would have definitely decreased their efficiency.

eddyloewen commented 5 years ago

My actual use-case that I was trying was the following:

class A {
    properties = {
        firstName: 'John',
        lastName: 'Doe',
    };
}

class B extends A {
    properties = {
        firstName: 'Jane',
        lastName: super.properties.lastName,
    };
}

I thought that this would/could work because I can do the following in JAVA

class A {
    int foo = 1;
}

class B extends A {
    int foo = 2;

    public B() {
        super();
        System.out.format("This foo: %d", this.foo);
        System.out.println();
        System.out.format("Super foo: %d", super.foo);
    }

    public static void main(String[ ] args) {
        B b = new B();
    }
}

Which will print this:

This foo: 2
Super foo: 1

I actually came to JavasScript when Classes where a thing already. I haven't been working with the prototype chain a lot. But I get the explanation from @nicolo-ribaudo.

And like @mbrowne says it will probably work with decorators later.

bakkot commented 5 years ago

In Javascript the thing you want will work if you just use this instead of super:

class A {
    properties = {
        firstName: 'John',
        lastName: 'Doe',
    };
}

class B extends A {
    properties = {
        firstName: 'Jane',
        lastName: this.properties.lastName,
    };
}
eddyloewen commented 5 years ago

@bakkot this actually works! Thanks!

But as it turns out, our use-case is even more complicated... :)

class A {
    functions = {
        callback: () => {
            console.log('a callback');
        },
    };
}

class B extends A {
    functions = {
        callback: () => {
            this.functions.callback();
            console.log('b callback');
        },
    };
}

This will result in an infinite loop...

It is possible to store the super functions as a separate value:

class A {
    functions = {
        callback: () => {
            console.log('a callback');
        },
    };
}

class B extends A {
    functions = {
        superCallback: this.functions.callback,
        callback: () => {
            this.functions.superCallback();
            console.log('b callback');
        },
    };
}

This will work, but only when nesting maximum one level. When I try this with a nother extended class it will again result in an infinite loop because the this context won't be shifted.

mbrowne commented 5 years ago

I am not a committee member, but as far as I understand it, that is the expected behavior given the semantics of this proposal. And I think it makes sense that it works this way...class fields are basically just like taking your field declarations and moving them immediately below the super() call in the constructor (except that they are defined using Object.definePropery semantics instead of a regular assignment, but that's irrelevant here).

FWIW, at first glance this design seems like it will make the code a bit hard to follow. It may seem like just a simple use of inheritance, but readers of the code have to understand the timing and exactly what's going on with this (or even if it were possible to do it with super and you have more than one level of inheritance). I'm not anti-inheritance, but I have seen that code complexity and maintenance burden increases when it's more difficult to trace where a method call will end up and when it will happen, and it's important to be especially mindful of this when using inheritance. Even if you find a workaround for your current approach, I would at least explore some alternative designs and compare them (if you haven't already). Of course, I don't know the details of your use case and what's driving this design, so this is just general advice that may or may not be fair in your case.

bakkot commented 5 years ago

@eddyloewen When you get to the point of doing something that complicated, you should probably do it in the constructor:

class A {
    functions = {
        callback: () => {
            console.log('a callback');
        },
    };
}

class B extends A {
    constructor() {
        super();
        let superCallback = this.functions.callback;
        this.functions = {
            callback: () => {
                superCallback.call(this.functions);
                console.log('b callback');
            },
        };
    }
}

You can do this with fields syntax directly, but I wouldn't really recommend it.

(Or you could use methods on the class instead, which seem like they are more appropriate for your use case. I agree with @mbrowne that it seems like this difficulty is arising because you're trying to go about things in an odd way.)

rdking commented 5 years ago

@eddyloewen Is there any particular reason for the intervening container? The direct approach works just fine.

class A {
    callback() {
        console.log('a callback');
    }
}

class B extends A {
    callback() {
        super.callback();
        console.log('b callback');
    }
}
littledan commented 5 years ago

There's a fundamental difference between Java and JavaScript, where Java's instance variables are related to where they are in the class hierarchy, and JavaScript's objects are conceptually "flat", so one instance variable simply replaces another if they have the same name. For this reason, super makes sense on methods and accessors, but not on fields/instance properties. @bakkot , @nicolo-ribaudo and others have explained how this works well above, and some practical suggestions have been provided. It sounds like the problem is solved here, so closing the issue.

eddyloewen commented 5 years ago

Thank you all for the explanations! I will try our approach again when the proposal for decorators progresses.

trusktr commented 4 years ago

I don’t think the inability of superclasses to access subclass implementation details is something that will cause a lot of confusion, because i suspect trying to do that is rare.

Here's the same problem, without the base class knowing about the subclass: https://github.com/tc39/proposal-class-fields/issues/280

(By the way, it is totally valid design for base classes that know about sub classes in various languages, not just JavaScript. It isn't as rare as you think.)

trusktr commented 4 years ago

The regular expectation of being able to access the base class via super is broken for "fields".

And even in TypeScript, where it faithfully returns the expected type instead of undefined! https://github.com/microsoft/TypeScript/issues/35314

trusktr commented 4 years ago

that is the expected behavior given the semantics of this proposal. And I think it makes sense that it works this way

I think people coming from other languages expect features of class to work with inheritance.