Handle breaking change in class property runtime behavior

[RyanCavanaugh]

From https://github.com/Microsoft/TypeScript/issues/12212#issuecomment-428342448 @joeywatts:

class A {
    property = 'some string';
}
class B extends A {
    property;
}
const instanceB = new B();
// "some string" or "undefined" ?
console.log(instanceB.property);

It appears from the current definition at https://github.com/tc39/proposal-class-fields that this code will print undefined.

If the proposal advances further with these semantics, we'll need to figure out a mitigation strategy, preferably sooner rather than later.

One option is to make the code as written a type error for ~2 versions and force people to write an explicit = undefined initializer, then remove the error.

Edit [@sandersn]:

Here is the current plan, together with what has happened so far.

TypeScript 3.6

• [x] #32787: allow accessors like declare class C { get p(): number; } (and set as well)
• [x] No changes to .js emit yet
• [x] No changes to .d.ts emit yet

TypeScript 3.7

• [x] #33470: emit get/set accessors in declaration files
• [x] #33401: accessors may not override properties and vice versa.
• [x] #33423: uninitialised properties may not override properties.
  • Introduce new syntax, class C extends B { declare x: number }
  • Introduce a codefix that inserts declare where needed.
  • This declaration is erased, like the original declaration in old versions of TS
• [x] #33509 Introduce a new flag, useDefineForClassFields.
  • When "useDefineForClassFields": false:
  • Initialized fields in class bodies are emitted as constructor assignments (even when targeting ESNext).
  • Uninitialized fields in class bodies are erased.
  • The preceding errors are silenced.
  • When "useDefineForClassFields": true:
  • Fields in class bodies are emitted as-is for ESNext.
  • Fields in class bodies are emitted as Object.defineProperty assignments in the constructor otherwise.
  • In 3.7, "useDefineForClassFields" defaults to false
  • Declaration emit is the same regardless of the flag's value.

TypeScript 3.8

(or, the first version after class fields reaches Stage 4)

• [ ] "useDefineForClassFields" defaults to true when targetting ESNext.
• [ ] issue new errors as suggestions with codefixes, even when "useDefineForClassFields": false.

[littledan]

I am happy to see TS looking into this important compatibility issue. This plan sounds good to me.

Another potential incompatibility is that TC39's fields are defined with Object.defineProperty, not =, so setters defined in a superclass will not run.

[RyanCavanaugh]

@littledan this is a bit of a compat nightmare for us and it'd be great to understand why initializers are optional in the current proposal. Why would someone write an initializer-free non-private declaration in the first place? Staking out an own property with the implicit value undefined seems like an odd bit of sugar to have when it creates a footgun for derived classes as a trade-off.

[bakkot]

@RyanCavanaugh:

class Foo {
  val;
  constructor({ val }) {
    this.val = val;
  }
}

I expect to see a lot of code like the above. It's really nice to be able to see the shape of the class without having to look in the constructor, but sometimes the value which goes into the field depends on arguments to the constructor.

[RyanCavanaugh]

Why is that kind of construct even desirable without static (or even dynamic!) enforcement? It's just a comment written in a different syntax - destined to be out of date and out of sync with what actually happens in the constructor

[ljharb]

fwiw static enforcement of that pattern is trivially possible with eslint, with or without an explicit initializer.

[bakkot]

No, it's meaningfully different: because it uses [[DefineOwnProperty]] semantics, it guarantees the property is an own data property, rather than risking triggering setters on a superclass.

You can of course put a Object.defineProperty in the constuctor, but that's... not great.

---

I recall we discussed the possibility of requiring initializers at some point, though I can't dig that up just now.

[RyanCavanaugh]

rather than risking triggering setters on a superclass

I'm sure you guys have discussed this to death already, but having a body-level declaration be subtly different from a constructor assignment (i.e. it only matters when there's a superclass setter) is only going to increase developer confusion.

Now when I look at code with an uninitialized declaration, I have to nervously wonder whether it's there for documentation purposes, or to overwrite a property slot because of the risk of triggering a base class setter (isn't not triggering it the risk? Why is the base class setter there in the first place) ?

[littledan]

We did actually discuss it to death in TC39... the classic Set vs Define debate (see summary by @bakkot; even though we've already discussed it a bunch, I'm really happy to hear your thoughts on the issue). Compatibility and constructor correspondence were raised, but the committee was ultimately convinced to take the Define and implicit = undefined path, partly due to evidence from @jeffmo that these semantics were not very difficult to migrate to in Facebook . The hope was that the Define semantics would actually be easier to understand locally, as you don't have to think about whether the superclass has a setter since you'll always get that property defined.

[jeffmo]

It's true that when I looked at this before it wouldn't have been difficult to migrate things at Facebook (IIRC very few places would have had issues in the first place), but it's also worth noting that usage of setters is pretty rare in JS written at FB (as a matter of both tooling support and style).

Where they are used I was able to flag them for manual inspection -- but it may also be possible to do something more heuristically smart than just manual inspection for codebases that use them more frequently.

[Jessidhia]

This has been a problem with babel's typescript support as well, because we need to know how should we treat field type annotations without an initialization.

Without --strictPropertyInitialization, how should this code be stripped of annotations:

interface Bar {}

class Foo {
  bar: Bar = {}
}

class Baz extends Foo {
  bar: Bar
}

If we only strip the : Bar from the second class, we're actually overriding its value with undefined in babel (as long as it's not being transformed with spec: false). We'd have to strip the entire of bar: Bar to not break at runtime but that'd be a possible break if bar is not actually an inherited field ('bar' in new Baz() would fail for example).

[wycats]

I don't think there's much we can do about the strict breakage, but I'm concerned about the fact that this makes it impossible to re-declare existing fields on subclasses.

One suggestion: use declare to mark a field as "type only"

class N {
  node: Node
}

class E {
  declare node: Element
}

This would have the same behavior as today's redeclaration, while the field syntax would have the standards-compliant behavior.

[RyanCavanaugh]

We'd have to strip the entire of bar: Bar to not break at runtime

This was raised in the office and I strongly opposed it - TS has never used a type annotation to change runtime behavior and I would not want to start now.

use declare to mark a field as "type only"

I like this quite a bit

as you don't have to think about whether the superclass has a setter since you'll always get that property defined.

Every time I hear this it sounds like an extremely solid argument against the behavior as proposed. If your base class has a setter, it almost certainly wrote it as a setter for a reason, and the base class is going to get super confused when it doesn't see its own setter behavior firing. e.g.

class Base {
  // This is a nice property that normalizes null/undef to 0!
  get value { return this._value }
  set value(v) { this._value = v ? v : 0; }

  func(x) {
    this.value = x;
    // Can't happen because the setter fixes it
    Debug.assert(this.value !== undefined);
    console.log(this.value + 10);
  }
}

class Derived extends Base {
  // FYI guys, value comes from the Base class, no worries
  value;
}

const d = new Derived();
d.func(undefined); // oops!

You're asking the derived class author to understand whether or not the base class property is a setter in order to understand whether or not it's safe to use a property declaration in the derived class, whereas previously this was a distinction that was rather hard to notice without a direct call to defineProperty. People will mess this up, and important setter side effects or validations are going to get unintentionally clobbered by people who think it's safe to move a this.p = e assignment in the derived constructor body into the class body because it looks neater.

If your derived class is intentionally stomping on its base class setters, I question whether it's even plausible that it's a correctly substitutable instance of its base class. Breaking your base class should be difficult, not easy.

[mheiber]

FYI, we have some work in progress to move class fields transformation from ts.ts to esnext.ts: https://github.com/bloomberg/TypeScript/pull/10.

[Jamesernator]

@RyanCavanaugh I think the main confusion for me that class fields don't trigger setters is mostly because of the use of the = token rather than some alternative.

Personally I would've preferred foo := 12 or some other differing syntax to say, hey this is a declaration not a property assignment. But it is in popular use already thanks to Babel, and Chrome is planning on shipping it soon so we're probably stuck with it.

Getting past the use of = I think it makes sense as it's a lot like an object literal e.g.:

const base = {
  set someValue(value) {
    console.log("triggered a setter!")
  }
}

// No setter triggered
const derived = {
  __proto__: base,
  someValue: 12
}

// No setter triggered
derived.someValue = 12

const derived2 = { __proto__: base }

// setter triggered
derived2.someValue = 12

class Derived extends Base {
  // analagous to someValue: 12 in an object literal
  someValue = 12
}

[RyanCavanaugh]

Note: nothing from this list has happened yet

---

Proposal

• TypeScript 3.4
  • No changes to .js emit yet
  • It is now legal to write declare class C { get p(): number; }
    • Same semantics as p: number;, except for caveats below
  • Introduce new flag, --legacyClassFieldEmit. When specified:
    • Initialized fields in class bodies are always emitted as constructor assignments (even when targeting ESNext), and uninitialized fields in class bodies are always erased
    • Does not affect declaration emit
  • Introduce new syntax, class A { declare x: number; }
    • This form is always erased
    • These fields are subject to strictClassInitialization checking; declare x!: number is legal.
  • If --legacyClassFieldEmit is not specified:
    • It is an error to declare a property (initialized or not) if the base class defines that property as a getter/setter pair
    • TBD: property as a getter/setter pair if the base class defines that property as a non-getter/setter pair
• TypeScript 3.5
  • No changes to .js emit yet
  • Declaration emit now emits getter/setter pairs in classes
• TypeScript 3.6
  • Default downlevel emit changes to defineProperty semantics

[littledan]

This design sounds good to me.

What do you mean by, "Declaration emit now emits getter/setter pairs in classes"? A define-style field in a superclass would shadow a getter/setter pair.

[RyanCavanaugh]

What do you mean by, "Declaration emit now emits getter/setter pairs in classes"?

Today if you build this class

class A {
  get p() { return 0; }
  set p(n) { }
}

the declaration emit is

class A {
  p: number;
}

which would prevent us from detecting the derived class accidently clobbering the field.

[littledan]

Thanks for explaining!

[DanielRosenwasser]

Something that @littledan brought up last night: the --experimentalDecorators flag currently expects set semantics, not define semantics. We'll likely have to maintain the old emit with this flag turned on.

P.S.: Keywords for me in the future: set define class fields instance fields properties defineproperty defineproperties

[mhofman]

* TBD: property as a getter/setter pair if the base class defines that property as a non-getter/setter pair

IMO this case is even more problematic than the other way around as it the new behavior would override the accessors:

class A {
    property = 'some string';
}
class B extends A {
    get property() {
        return 'other string';
    }

    set property(value) {
        // readonly now!
    }
}

// TypeScript Currently
const instanceB = new B();
console.log(instanceB.property); // "other string"
instanceB.property = "override";
console.log(instanceB.property); // "other string"

// Class fields behavior
const instanceB = new B();
console.log(instanceB.property); // "some string"
instanceB.property = "override";
console.log(instanceB.property); // "override"

[trusktr]

Will this feature be disabled by default, so that [[Set]] semantics are default? It's going to introduce red squiggly lines into projects if the class fields suddenly are not allowed due to super classes having accessors. 3.6 isn't a major version bump, you know.

People will accidentally shoot their legs off with [[Define]] gun shells. :)

For example, I made an oil painting of a gruesome premonition:

class Animal {
    #sound = 'rustling noise in the bushes'

    get sound() { return this.#sound }
    set sound(val) {
      this.#sound = val;
      /* some important code here, perhaps tracking known sounds, etc */
    }

    makeSound() {
        console.log(this.#sound)
    }
}

const a = new Animal
a.makeSound() // 3.14

class Lion extends Animal {
    sound = 'RAWR!'
}

const lion = new Lion
lion.makeSound() // BUG! Expected "RAWR!" but got "rustling noise in the bushes"

It was a bloody mess.

[ljharb]

How is that possible - using the private field syntax (which doesn’t work in TS yet) with Set semantic expectations?

[robpalme]

@trusktr your code is defining the Lion declaratively - and therefore we do not expect side-effects. The fix is to make it imperative which will invoke the Animal base class accessor.

class Lion extends Animal {
    constructor() { this.sound = 'RAWR!' }
}

[hax]

How is that possible @ljharb

This is not important, you can just replace # to _ (yes, _ is just the old #, if reverse the slogan)

[hax]

The fix is to make it imperative which will invoke the Animal base class accessor.

@robpalme Yes this is the "fix" if follow the [[Define]] semantic. Unfortunately, such fix is not very obvious to most JS/TS programmers, and many see your "correct code" and want to refactor it to the wrong form because they are educated by React. 🙃

There are also many cases, you have no way to "fix". For example: Base class originally use traditional imperative form, subclass override it to accessors. Then one day the author of base class want to adopt class fields because it's a new feature and should be wonderful! Remember the author of subclass can not "fix" the decision of base class.

[bigopon]

@trusktr just another example that i just ran into, related to [[Define]], and maybe someone already mentioned it: href is defined on the instance: It will be a massive mess regardless TS/JS

---

What it really is expected to be: trigger the setter

[littledan]

@bigopon Yes, these are the semantics that TC39 chose deliberately in 2016 in collaboration with the TypeScript team.

[trusktr]

Please, many of us beg, don't make [[Define]] the default.

[trusktr]

(We understand inheritance, and we understand side-effects from superclass accessors.)

[trusktr]

This is really bothersome. I use frameworks/libraries on my base classes which use accessors to make cool things happen with my subclasses.

If I change to using class fields (in JS environments that currently support it, or in TypeScript 3.6 if it will be the case), then everything just breaks.

It's a mess.

---

As a professional engineer needing to ship resilient code, I may have to choose not to use class fields in their current form. I'm sure a lint plugin will come out for it.

[trusktr]

Please don't break people's code when they update to TypeScript 3.6.

At the very least, having an option like classFieldDefine, even if defaulting to true, would be awesome. I would certainly set it to false.

[rbuckton]

I still have concerns about the [[Define]] semantics, especially this case:

class Base {
  x = 1;
}

class Derived extends Base {
  get x() { return 2; }
  set x(value) { console.log(`x was set to ${value}`); }
}

const obj = new Derived(); // nothing printed
console.log(obj.x); // 1

The x field on Base replaces the x accessor from Derived, which breaks inheritance and may be confusing for users.

Given the time remaining for 3.6, I'm moving this to 3.7.

[Jessidhia]

The same issue would exist but the other way around with [[Set]], though. 🤔

[Jamesernator]

I've generally found that depending on subclass setters/getters just tends to make the base class much harder to refactor so // nothing printed doesn't really bother me.

The other case of console.log(obj.x); // 1 is a bit more concerning, but I don't really see any way around these sort've problems as it's basically the same as this sort've problem (that will happen regardless of [[Set]] vs [[Define]] semantics):

class Base {
  method = () => {
    return 12;   
  }
}

class Derived extends Base {
  method() {
    return 7;
  }
}

new Derived().method(); // 12

[Jamesernator]

Mind you I still think fields should be declared with := rather than = to make it clear class Point { x := 10 } is more like o = { x: 10 } rather than o.x = 10, but I think it's probably too late to change this.

[hax]

I believe it never be late to stop a wrong decision and I believe class fields (many issues, especially the footgun of [[Define]] own property semantic) is the worst one in JS history after ES4.

[ljharb]

Of course it can be too late; typeof null can never change. Similarly, class fields are shipped in over 50% of web browsers, so the time to make any changes has long passed.

[hax]

@ljharb First I don't agree solely use shipping data as the threshold, second it can never beyond 50% of web browsers in REAL MARKET. All china mainstream browsers (cover 95%+ share of China market) do not land class fields (even public fields) up to now.

[RyanCavanaugh]

Stage 3: Indicate that further refinement will require feedback from implementations and users

¯\_(ツ)_/¯

[ljharb]

Indeed; but that feedback still can’t break the web - so if anyone is relying on Define on the web, it can’t change.

[RyanCavanaugh]

What's an example of a refinement that wouldn't break the web? Why does Stage 3 exist at all? Should we just move things to Stage 4 as soon as Chrome puts it out there unflagged?

[bterlson]

@ljharb FWIW I don't think it's useful to suggest no changes are possible on a stage 3 proposal until the feedback is collected and presented. It may be practically true, but it's better to be receptive to user/implementer feedback during stage 3 until TC39 has explored the options before them in light of the feedback given.

[ljharb]

Fair enough.

[mbrowne]

The TypeScript team can proceed however they like of course, but IMO it would be worth going ahead and beginning work on updating the semantics of TS class properties to match the new ES spec. The PR could be left open since TC39 could still receive new and significant feedback as class fields are used in the real world. But I think the odds are extremely low that the real-world issues with the Object.defineProperty behavior would be so significant and common that they would justify changing the semantics at this point. I'm concerned about compatibility issues between JS and TS here, especially now that many people are compiling TS with Babel.

BTW, as someone who's not a fan of the Object.defineProperty semantics, I wish people on the TS team had argued against it more earlier in the process, especially those who are on the TC39 committee (although perhaps there were mixed opinions among the TS team). But there were two valid sides to that debate, and at some point we have to accept the spec and move on. Otherwise the differences between TS and JS will probably cause far more issues than would ever have been directly caused by the defineProperty semantics in the first place.

[hax]

I wish people on the TS team had argued against it more earlier in the process

The best time to fix a issue was early stage. The second best time is now.

[sandersn]

After we noticed problems with backward compatibility of .d.ts files in Azure SDK, we decided to change .d.ts emit when --useDefineForClassFields is off. Instead of emitting accessors, which only versions of Typescript will not parse in .d.ts, the compiler will emit a property with a leading comment. So this class:

class B {
  get p() { }
  set p(value) { }
  get q() { }
  set r(value) { }
}

Would emit

declare class B {
  /** @get @set */ p: any
  /** @get */ q: any
  /** @set */ r: any
}

Typescript 3.7 and above will look for these comments on property declarations and treat them as accessor declarations instead.

[trotyl]

I still have concerns about the [[Define]] semantics, especially this case:

class Base {
  x = 1;
}

class Derived extends Base {
  get x() { return 2; }
  set x(value) { console.log(`x was set to ${value}`); }
}

const obj = new Derived(); // nothing printed
console.log(obj.x); // 1

The x field on Base replaces the x accessor from Derived, which breaks inheritance and may be confusing for users.

@rbuckton Using [[Set]] won't really fix inheritance, but only make the breaking happens to some other people. Say that I need to use throwing getter as abstract property, like:

class Base {
  get brand(): string {
    throw new Error(`Brand must be defined`)
  }
}

class Derived extends Base {
  brand = 'derived'
}

const d = new Derived()
console.log(d.brand)

The above works well with [[Define]], but throwing for [[Set]]. You may argue that TypeScript has abstract modifier to better handle this, but abstract cannot be combined with static, for example:

class Base {
  static get brand(): string {
    throw new Error(`Brand must be defined`)
  }
}

class Derived extends Base {
  static brand = 'derived'
}

console.log(Derived.brand)

This issue already happens in TypeScript today and break my use case. I'm not going to say [[Define]] is better than [[Set]], in fact, both of them cause footguns and neither is better than another.

[Jessidhia]

That was my point with https://github.com/microsoft/TypeScript/issues/27644#issuecomment-518543395

[mbrowne]

@trotyl I disagree. When comparing options, we should consider the frequency and severity of the issues, and I think it's a real stretch to say both options are equally good/bad. The use cases I've seen where [[Set]] is problematic are mostly issues of semantic purity or fairly minor issues that would occur pretty rarely. Your example is the first I've seen that I would agree is a pretty significant footgun when using [[Set]]. But the foot-guns with [[Define]] are generally more significant and seem more likely to occur, at least based on what I've seen in online discussions about this issue.

So that's my two cents, but I'm not sure why we're still discussing this at all—the ship has sailed. I agree with those who say that stage 3 shouldn't completely close the window to further changes (e.g. changing the syntax to := instead of =), but I think the bar for justifying such changes should be very high—for example, if the semantics of the current proposal were causing great confusion to thousands of developers on a daily basis after this feature shipped in browsers. Since the Set/Define issue only affects this edge case of inherited fields and setters, I just don't see that happening. I think at this point, far more confusion would be caused by changing the default semantics of a feature that's already shipped than the relatively rare instances where the Set/Define distinction actually matters.

[trotyl]

@mbrowne

When comparing options, we should consider the frequency and severity of the issues, and I think it's a real stretch to say both options are equally good/bad. The use cases I've seen where [[Set]] is problematic are mostly issues of semantic purity or fairly minor issues that would occur pretty rarely. Your example is the first I've seen that I would agree is a pretty significant footgun when using [[Set]]. But the foot-guns with [[Define]] are generally more significant and seem more likely to occur, at least based on what I've seen in online discussions about this issue.

I see your points, but have got different conclusions.

Almost every "problematic" examples with [[Define]] I saw are violating OO practices, more specifically, overriding a base class member without knowing it intended to be virtual/abstract.

Even the example provided by @rbuckton, from a reviewer perspective, I'd say it's intended to break the application. Even with the x in Base class being an accessor property, overriding it without calling super getter/setter would always result to unexpected behavior, as the base accessor may contain critical logic for base class to work. Doing this kind of overriding one should at least add following comment to base class:

class Base {
  // IMPORTANT NOTE!!! 
  // DO NOT EVER CHANGE THIS TO ACCESSOR, IT WON'T GET CALLED
  x = 1;
}

class Derived extends Base {
  get x() { return 2; }
  set x(value) { console.log(`x was set to ${value}`); }
}

I don't believe this is how class inheritance should be, let alone the base class may not be in the same project.

Regarding controlling of override ability, OO language can be divided into two categories, explicit-virtual and explicit final, the latter in practice has been considered a design failure, where in any Java code style you should always write the final manually, and newly created JVM languages like Kotlin has go to the opposite design.

From the lengthy history of OO language, overriding a base class member without knowing it can be overridden is not something feasible, the whole application could break one day or another.

Unfortunately JavaScript is even worse than explicit-final languages, which cannot prevent overriding at all (in common declarative class code). If possible, I'd definitely like JavaScript class member to have explicit-virtual semantics, or at least runtime detection like https://github.com/tc39/proposal-class-fields/issues/176#issuecomment-538104298.