Disallow property/accessor overrides

[sandersn]

This PR is part of the migration from [[Set]] semantics to [[Define]] semantics. #27644 tracks the complete migration. This is the second step.

Summary

Briefly, properties can now only override properties, and accessors can only override accessors. The tests come from the user-submitted horrorshow parade in #27644. Thanks to all who contributed there.

Exceptions

1. If the base property is abstract or in an interface, there is no error unless the abstract base property has an initialiser.
2. If the symbol has both Property and Accessor set, there is no error. (I'm not sure when this can happen, although @weswigham says he can produce a Javascript example pretty easily.)

Motivation

Accessors that override properties have always been error-prone with [[Set]] semantics, so these new errors are useful with [[Set]] or [[Define]] semantics. For example, base-class properties call derived accessors, which may be unexpected. The code below prints 2, the derived value, but it also prints set 1. That's because the base p = 1 calls derived accessor set p.

class B {
    p = 1
}
class D extends B {
    _p = 2
    get p() { return this._p }
    set p(value) { console.log('set', value); this._p = value }
}
var d = new D()
console.log(d.p)

In fact, if the derived class attempts to make p readonly by leaving off set p, the code crashes with "Cannot set property p of #<D> which has only a getter." This should always have been an error. However, because we haven’t given errors before, and because fixing the errors is likely to be difficult, we probably would not have added them otherwise, or added them in so many cases. Here are some examples of code that will be tricky to change:

class CleverBase {
  get p() { 
    // caching, etc.
  }
  set p() {
  }
}
class Simple extends CleverBase {
  p = 'just a value'
}

CleverBase is a framework class that intends to cache or transform the value provided by Simple. Simple is written by a framework user who just knows that they need to extend CleverBase and provide some configuration properties. This pattern no longer works with [[Define]] semantics. I believe the Angular 2 failure I found below is similar to this case.

class LegacyBase {
  p = 1
}
class SmartDerived extends LegacyBase {
  get() {
    // clever work on get
  }
  set(value) {
    // additional work to skip initial set from the base
    // clever work on set
  }
}

This code is the same as the first example — accessors override a property — but the intent is different: to ignore the base's property. With [[Set]] semantics, it's possible to work around the initial set from the base, but with [[Define]] semantics, the base property will override the derived accessors. Sometimes a derived accessor can be replaced with a property, but often the accessor needs to run additional code to work around base class limitations. In this case, the fix is not simple. I saw this in a couple of Microsoft apps, and in one place it had a comment "has to be a getter so overriding the base class works correctly".

To test this PR

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/43964/artifacts?artifactName=tgz&fileId=3AF21D4D4FC0F9A349D943EEA8D0975B1F7FB2E974C87B30467B3F4A1F0AFD2B02&fileName=/typescript-3.7.0-insiders.20190913.tgz"
    }
}

How to fix the errors

Property overrides accessor

class CleverBase {
  _p: unknown
  get p() {
    return _p
  }
  set p(value) {
    // cache or transform or register or ...
    _p = value
  }
}
class SimpleUser extends CleverBase {
  // base setter runs, caching happens
  p = "just fill in some property values"
}

SimpleUser will have an error on the property declaration p. The fix is to move it into the constructor as an assignment:

class SimpleUser extends CleverBase {
  constructor() {
    // base setter runs, caching happens
    this.p = "just fill in some property values"
  }
}

Since CleverBase declares p, there's no need for SimpleUser to do so.

Accessor overrides property

class LegacyBase {
  p = 1
}
class SmartDerived extends LegacyBase {
  get() p {
    // clever work on get
  }
  set(value) p {
    // additional work to skip initial set from the base
    // clever work on set
  }
}

SmartDerived will have an error on the get/set declarations for p. The fix is to move them into the constructor as an Object.defineProperty:

class SmarterDerived extends LegacyBase {
  constructor() {
    Object.defineProperty(this, "p", {
      get() {
        // clever work on get
      },
      set(value) {
        // clever work on set
      }
    })
  }
}

This doesn't have exactly the same semantics; the base never calls the derived setter for p. Most people won't handle this correctly anyway, so I suspect it's not a problem. However, if the original setter does have skip-initial-set code to work around the current weird Typescript semantics, that code will need to be removed.

Next steps

1. Also for 3.7, d.ts emit should always include accessors. If we choose not to do this for 3.7, we should still allow accessors to override properties. We might want to allow this anyway for backward compatibility. This is at Always emit accessors in .d.ts files #33470
2. Add an error when overriding properties that redeclare a base property with a subtype. Also add declare property: type syntax to work around this. This is at Disallow uninitialised property overrides #33423
3. The upcoming flag that changes class field emit to use [[Define]] semantics should disable this error.

[sandersn]

I'm curious how much real-world code actually does this...

@typescript-bot test this
@typescript-bot run dt
@typescript-bot user test this

[typescript-bot]

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at f76e01f. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at f76e01f. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @sandersn, I've started to run the extended test suite on this PR at f76e01f. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[sandersn]

Discovered in the design review:

1. should not apply to .js this-property assignments, and possibly not at all in .js.
2. should not apply when the base property/accessor comes from an interface.

[sandersn]

For (2), @rbuckton suggested a test that extends Map, which is defined as an interface.

[sandersn]

Summary: Only bogus failures in user tests. Two projects failed in DT, but since .d.ts files don't currently have accessors, we'll only see accessor-overrides-property errors from there. Admittedly, these are probably the most common.

It looks like RWC failed to run; I'll try again.

[sandersn]

@typescript-bot test this

[typescript-bot]

Heya @sandersn, I've started to run the extended test suite on this PR at f76e01f. You can monitor the build here. It should now contribute to this PR's status checks.

[sandersn]

this change makes it obvious that this line is always false and its contents are dead code. I'll remove it.

[sandersn]

Errr, instead I tried reviving it. We'll see how much code has unknowingly been missing this error.

[sandersn]

RWC: 27 breaks across 7 projects. So, lots of projects each had a few breaks, which is not great for a problem that often has no easy fix.

I'll look at the breaks next to see if I'm wrong about "no easy fix". I hope so!

[sandersn]

@typescript-bot test this
@typescript-bot run dt
@typescript-bot user test this

[typescript-bot]

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at c4f334a. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at c4f334a. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @sandersn, I've started to run the extended test suite on this PR at c4f334a. You can monitor the build here. It should now contribute to this PR's status checks.

[sandersn]

From RWC:

1. Angular 2: a class overrides an accessor pair with a property, and the derived class has a decorator which mentions the property. I have no idea what the intended semantics are here.
2. the copies of VS Code and Azure SDK that we have are so old they didn't have abstract properties.
3. Babylon: overrides a property with an accessor pair. I don't think this actually works quite the way it's intended, but probably switching the base to an accessor pair would fix things.
4. POPULAR MICROSOFT APP 1a: an accessor pair overrides a property, intending to ignore it. This doesn't work with [[Define]] semantics. Not sure what the workaround should be here.
5. POPULAR MICROSOFT APP 1b: a getter overrides a property. The getter returns a constant, so seems straightforwardly replaceable with a property. The comment above says "has to be a getter so overriding the base works correctly". Uh-oh.

A couple of failures are fixed in the latest commit, which allows anything to override properties declared in interfaces.

The worrisome problems are (1), (4) and (5). It sounds like (5) is working around [[Set]] semantics so would be hard to fix. (1) has to do with decorators in Angular. I'm sure we can come up with a workaround for (4), but it will be awfully complicated to put in a blog post.

[sandersn]

Note: I tested my fix for "properties/accessors aren't allowed to override methods", and it turns out it's only been broken from 3.0-3.6. That means there were no new RWC failures, as our RWC code predates 3.0.

[sandersn]

@typescript-bot pack this

[typescript-bot]

Heya @sandersn, I've started to run the tarball bundle task on this PR at c4f334a. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Hey @sandersn, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/43964/artifacts?artifactName=tgz&fileId=3AF21D4D4FC0F9A349D943EEA8D0975B1F7FB2E974C87B30467B3F4A1F0AFD2B02&fileName=/typescript-3.7.0-insiders.20190913.tgz"
    }
}

and then running npm install.

[sandersn]

Here is the failure from Angular:

@Component({selector: 'tree', properties: ['data']})
@View({
  directives: [StaticTreeComponent1],
  template:
      `<span> {{data.value}} <tree [data]='data.right'></tree><tree [data]='data.left'></tree></span>`
})
class StaticTreeComponent2 extends StaticTreeComponentBase {
  data: TreeNode;
}

data: TreeNode is a property, while StaticTreeComponentBase.data is an accessor pair. I think the fix here will be the upcoming declare syntax:

class StaticTreeComponent2 extends StaticTreeComponentBase {
  declare data: TreeNode;
}

[sandersn]

@rkirov @mprobst @evmar
This is going to cause a lot of new errors in 3.7 in complex or long-lived OO code. We'd appreciate it if you could try this PR out early:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/43964/artifacts?artifactName=tgz&fileId=3AF21D4D4FC0F9A349D943EEA8D0975B1F7FB2E974C87B30467B3F4A1F0AFD2B02&fileName=/typescript-3.7.0-insiders.20190913.tgz"
    }
}

If you have any feedback, please let us know. I expect we'll merge this in a week or less to let everybody on typescript@next start dealing with it.

[sandersn]

Note: The interface check is currently wrong; it needs to check the containing type of the base property, not the immediate base type.

[mprobst]

@sandersn thanks for the heads up!

I ran this against our internal code base. As expected, this breaks some code, though not terribly many; something like 0.1%. This is manageable for us (famous last words...), assuming there's a reasonably mechanical way to fix.

From sampling the results:

• we generally compile with .d.ts, i.e. each compilation unit only consists of a handful of .ts files, all dependencies come in as .d.ts. Most breakages appear to be caused by TS thinking a class overrides a property because the .d.ts file cannot express the fact. This would be a blocker for us, we need support for .d.ts
• There are a few APIs in Angular & friends that are problematic. E.g. MatFormFieldControl from the Angular Material UI components is an abstract class with a readonly errorState: boolean. Users extend the abstract class and "override" the property from the base with a get errorState. According to docs above this should be allowed (right?), but doesn't seem to work? Any idea how to debug? It's just the class that's abstract, not the property, in case that matters.

These are the most common issues. I'll need to see if I can exclude those to (possibly) find more specific issues.

[mprobst]

Ah I see, #33470 adds the .d.ts emit support.

@sandersn, do you have a branch that pulls all the related changes together?

[sandersn]

@mprobst Yes, sandersn/add-property-define-flag. I haven't created a PR or packed it for consumption yet. I'll update after I have.

[sandersn]

@mprobst #33509

[sandersn]

Also, I outlined a mechanical way to fix both of the errors introduced in this PR to the description. However, the second -- moving accessors into the constructor with a defineProperty call -- doesn't perfectly preserve semantics. It would work fine for all the examples I saw, none of which were aware of the weird edge cases of Set semantics.

[mprobst]

Thanks for that branch.

I ran this across our internal code base, overall breakages seem to be relatively rare. Maybe somewhat surprisingly, all of them are subtypes just narrowing the type of a supertype's property (and, often, declaring a @Decorator on them), i.e. TS2612. I cannot find any occurrences of TS2610 or TS2611. I'm not sure if we legitimately don't have any code like that, or if I'm not finding it due to some breakage. Given the odd semantics of property overrides, I wouldn't be surprised if engineers managed to steer clear of it.

I've sampled the cases I found, all that I inspected seem legit by the semantics explained here (i.e. I found no compiler bugs).

Regarding the possible fixes, "Property overrides accessor" checks out.

The "Accessor overrides property" fix will not work for us, because it moves the property name "p" into a string literal, which will collide with our code optimization/symbol renaming strategy. Given how rare this appears to be, I think we can just manually fix the code.

Let me know if you'd like me to look out for (other) specific patterns of expected breakage or would like feedback on something in particular.

[concavelenz]

@mprobst While Object.defineProperty has the literal to string issue, Object.defineProperties does not require that change and that is what Closure Compiler recommends for ADVANCED mode code that otherwise requires calls to Object.defineProperty.

[sandersn]

@mprobst Thanks for your analysis. That agrees exactly with what I saw from our test repos, except that a few people thought (incorrectly) that is was safe to use a get-only accessor in a subclass to make a property readonly. But this is probably rare in modern code that has the readonly modifier.

[sandersn]

This is now included in #33509. Thanks all for testing.