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

Overriding with methods and other edge cases #16

Closed zenparsing closed 8 years ago

zenparsing commented 9 years ago

Under the current proposal, declared property names are [[Set]] during instance initialization. This creates some interesting and perhaps unintuitive edge-cases.

First, if the field is overridden by a method:

class X {
  a = 1;
}

class Y extends X {
  a() { }
}

typeof (new Y).a; // "number"

The definition of a in the base class gets to overwrite the definition of a in the subclass.

Second, if the field is defined as an accessor pair in the base class:

class X {
  get a() { return "abc" }
}

class Y extends X {
  a = 1;
}

(new Y); // throws an error because there is no setter for "a"

It feels like something is going on here that shouldn't. Perhaps fields should not be allowed to shadow methods and vice-versa?

zenparsing commented 9 years ago

(Edit: added the extends clause to an example above)

michaelficarra commented 9 years ago

@zenparsing I'll be changing this behaviour very soon with an update to PR #6. Declared instance properties will be defined using [[DefineProperty]] on the instance, and class methods are defined on the prototype, so there shouldn't be anything surprising after that.

zenparsing commented 9 years ago

Hey @michaelficarra !

Won't the first case still apply, though? The field declaration in the base class will end up shadowing the method declared in the subclass. I suppose it makes sense if you understand the mechanics, but the normal precedence of declarations is still inverted.

michaelficarra commented 9 years ago

I think that's perfectly expected. Methods go on the prototype and fields go on the instance, so any fields will shadow prototype members.

zenparsing commented 9 years ago

I disagree. : ) As a general rule, declarations in the base class (no matter what kind they are) shouldn't shadow declarations found in subclasses.

michaelficarra commented 9 years ago

Well in general, all declarations in classes affect the prototype. This proposal changes that, adding a declaration that affects each instance, so general rules like the one you've mentioned will necessarily be violated.

jeffmo commented 9 years ago

@zenparsing:

class Base {
  constructor() {
    this.a = 1;
  }
}

class Child extends Base {
  a() { }
}
jeffmo commented 8 years ago

I agree this could be a hazard, but I don't think it's necessarily a new one (see my latest comment). It appears to be a new variation on the normal hazards that can come with inheritance (in the absence of a type system).