tc39 / proposal-class-public-fields

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

interaction with super #62

Closed wmertens closed 7 years ago

wmertens commented 7 years ago

If you do this:

class B {
  foo = () => 5
}
class S extends B {
  foo() {
    return 7
  }
}

your call to (new S()).foo() will actually return 5 because the assignment in B goes on the instance, thus overriding the S class method.

This is of course as is should be per spec, but it can cause confusion. I wonder if this could be made an error? Since class B will always create the instance variable, class S should not define a class method?

bakkot commented 7 years ago

While I agree it can be confusing, I don't think this should be an error, since shapes and prototypes can change at runtime. This sort of "your code might (or might not) be confused" issues are generally dealt with by tooling, not by the language preventing you from writing that code. Besides which there's a consistency argument, since it is currently not an error to do

class B {
  constructor() {
    this.foo = () => 5;
  }
}
class S extends B {
  foo() {
    return 7;
  }
}
wmertens commented 7 years ago

Yes, but when you write it as a public field, you are adding semantic value: You are declaratively saying "foo should be bound to the instance".

If you do the binding in the constructor, it might be the same instructions to the VM, but you are being imperative, without semantic value.

So, for the declarative case, I think an error might be warranted.

On Mon, May 29, 2017 at 9:48 AM Kevin Gibbons notifications@github.com wrote:

While I agree it can be confusing, I don't think this should be an error, since shapes and prototypes can change at runtime. This sort of "your code might (or might not) be confused" issues are generally dealt with by tooling, not by the language preventing you from writing that code. Besides which there's a consistency argument, since it is currently not an error to do

class B { constructor() { this.foo = () => 5; } }class S extends B { foo() { return 7; } }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-class-public-fields/issues/62#issuecomment-304595999, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlkSCxvCFiJCtGZOnM-p7N23OZ95Aks5r-nhCgaJpZM4No84v .

bakkot commented 7 years ago

Considering that we don't even have an error for

class A {
  m(){}
  m(){}
}

I don't think the argument that this is a declarative position is sufficient to justify introducing this new error.

ljharb commented 7 years ago

The public field is, as you say, a declarative statement saying "foo should be bound to the instance". That's very explicit. Why would doing so be an error? Shadowing inherited properties with own properties is a common part of JS.

wmertens commented 7 years ago

The error is trying to override the own property set in B with the class property in S. It won't override the own property but you might think it does.

I suppose this is more in the domain of an eslint rule…

ljharb commented 7 years ago

Yes, I think that misunderstanding of how inheritance works in JS (that you can override a subclass from a superclass) is best handled by a linter and education.

wmertens commented 7 years ago

Closing; consensus: There should be an eslint rule for this. Since this is still only a proposal, this issue should somehow reopen once it is fully accepted. Let's just try to remember :)