Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for abstract constructor types #36392

Merged
merged 9 commits into from
Jan 8, 2021

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Jan 24, 2020

This adds support for the abstract keyword on constructor types and construct signatures, allowing you to indicate the signature is abstract. This also updates the definitions for InstanceType and ConstructorParameters to use the abstract modifier:

Syntax

// constructor types
type T1 = abstract new () => any;

Semantics

  • Non-abstract constructor types are assignable to abstract constructor types if they would otherwise be assignable.
  • Abstract constructor types are not assignable to non-abstract constructor types.
  • Values with types containing abstract constructor types cannot be instantiated via new (this previously only applied to abstract classes)

Examples - Basic Usage

type Constructor<T> = new (...args: any[]) => T;
type AbstractConstructor<T> = abstract new (...args: any[]) => T;
interface Obj<T> {
  x: T;
}

declare let c: Constructor<Obj<number>>;
declare let a: AbstractConstructor<Obj<number>>;

// assignability
a = c; // ok, a non-abstract constructor type can be assigned to an abstract constructor type
c = a; // error, an abstract constructor type cannot be assigned to a non-abstract constructor type

// new expression
new c(); // ok
new a(); // error, cannot create an instance of an abstract class

// inference and conditional types
type T1 = InstanceType<typeof c>["x"]; // number
type T2 = InstanceType<typeof a>["x"]; // number

Examples - Mixins

interface Mixin { ... }
abstract class Base { ... }

declare function Mixin1<TBase extends new (...args: any[]) => {}>(base: TBase):
    TBase & (new (...args: any[]) => Mixin);

class Sub1 extends Mixin1(Base) { }
//                        ^^^^ TS2345: Argument of type 'typeof Base' is not assignable 
//                                     to parameter of type 'new (...args: any[]) => {}'.
//                                     Cannot assign an abstract constructor type to a 
//                                     non-abstract constructor type.

declare function Mixin2<TBase extends abstract new (...args: any[]) => {}>(base: TBase):
    TBase & (new (...args: any[]) => Mixin);

class Sub2 extends Mixin2(Base) { } // Ok

Fixes #26829
Fixes #35576

@rbuckton
Copy link
Member Author

CC: @weswigham, @RyanCavanaugh, @DanielRosenwasser (as suggested reviewers is not working).

@ajafff
Copy link
Contributor

ajafff commented Jan 24, 2020

Does this also support mixins with abstract classes? Could you add a test for that, too?

@rbuckton rbuckton force-pushed the abstractConstructSignatures branch from 136dbf0 to 757b70f Compare January 28, 2020 21:09
@rbuckton
Copy link
Member Author

I'm going to revert support for abstract construct signatures in favor of investigating class type expressions:

// [source.ts]
function MyMixin<TBaseClass extends abstract new (...args: any[]) => any>(
  baseClass: TBaseClass
) {
  abstract class MyMixinClass extends baseClass {
    abstract abstractMethod(): void;
  }
  return MyMixinClass;
}

// [output.d.ts]
declare function MyMixin<TBaseClass extends abstract new (...args: any[]) => any>(
  baseClass: TBaseClass
): abstract class extends TBaseClass {
  abstract abstractMethod(): void;
};

@sandersn sandersn added the Experiment A fork with an experimental idea which might not make it into master label Feb 1, 2020
@rbuckton rbuckton changed the title Add support for abstract constructor types and signatures Add support for abstract constructor types Jun 18, 2020
@rbuckton rbuckton force-pushed the abstractConstructSignatures branch from 757b70f to eb5789a Compare July 10, 2020 23:29
@rbuckton
Copy link
Member Author

Build failures seem to be due to the fact that eslint isn't handling the construct signatures having modifiers in lib/es5.d.ts so I'm reverting that part for now.

@rbuckton rbuckton force-pushed the abstractConstructSignatures branch from 0b9fd25 to 65a49a7 Compare July 13, 2020 19:59
@trusktr
Copy link
Contributor

trusktr commented Jul 14, 2020

declare function MyMixin<TBaseClass extends abstract new (...args: any[]) => any>(baseClass: TBaseClass)

If the constraint for TBaseClass is an abstract constructor, can we still base a non-abstract class for baseClass?

It won't be helpful if we're stuck with either abstract or not-abstract.

I think what people really want is that if they pass in an abstract class, then the mixin result is an abstract class (unless the mixin class implements the methods/properties), or if they pass a non-abstract class, then the mixin result is not abstract.

Otherwise the usability of mixins would be inflexible.

The following code has some type information omitted (like plain JS) so that I can clearly show the intention:

abstract class Foo {
  abstract fooMethod(): void
}

function CoolMixin(Base) {
  return class Cool extends Base {
    coolMethod() {}
  }
}

class MyClass extends Foo {
  // Required to implement the abstract fooMethod here.
  // ...
}

class MyOtherClass extends CoolMixin(Foo) {
  // Should also be required to implement the abstract fooMethod here.
  // ...
}

As a workaround, we could do the following but it is less ideal:

abstract class Foo {
  abstract fooMethod(): void
}

function CoolMixin(Base) {
  return class Cool extends Base {
    coolMethod() {}
  }
}

class MyClass extends Foo {
  // Required to implement the abstract fooMethod here.
  // ...
}

class _MyOtherClass extends Foo {
  // Required to implement the abstract fooMethod here.
}

class MyOtherClass extends CoolMixin(_MyOtherClass) {
  // ...
}

And if the mixin returns an abstract class, it should inherit abstractedness. So,

abstract class Foo {
  abstract fooMethod(): void
}

function OtherMixin(Base) {
  return abstract class Other extends Base {
    otherMethod() {}
  }
}

class MyClass extends Foo {
  // Required to implement fooMethod here.
  // ...
}

class MyOtherClass extends OtherMixin(Foo) {
  // Required to implement fooMethod and otherMethod here.
  // ...
}

class Bar {
  barMethod() {...}
}

// the previous OtherMixin application accepted an abstract class, and now this application accepts a non-abstract class:
class MyNextClass extends OtherMixin(Bar) {
  // Required to implement otherMethod here, and the barMethod is inherited.
  // ...
}

Note that last example in particular accepts both abstract and non-abstract base classes (I didn't show an example of CoolMixin accepting a non-abstract class but that should work too).

I believe that's what we should aim for in TypeScript, otherwise we're not effectively modeling plain JS in the most useful way.

Mixins are suppose to be mixed-and-matched, but if we can not work with abstract in a flexible way, we won't be able to mix-and-match mixins.

I'm merely a TypeScript end user, but I hope some solution can be found so that mixins are as convenient as they are in plain JS but with structural types in place.

# Conflicts:
#	src/compiler/parser.ts
#	src/compiler/types.ts
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 8, 2020
@rbuckton
Copy link
Member Author

rbuckton commented Sep 8, 2020

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 8, 2020

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 2e77e93. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @rbuckton, 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/84547/artifacts?artifactName=tgz&fileId=78EEF1EB3E873A239A07C29FA531FBC2DFBC4D34D336DA97287D6ED06314D6F302&fileName=/typescript-4.1.0-insiders.20200908.tgz"
    }
}

and then running npm install.

@rbuckton
Copy link
Member Author

rbuckton commented Sep 9, 2020

@trusktr: A new () => T type is assignable to an abstract new () => T type. A mixin definition that uses something like <TBase extends abstract new (...args: any[]) => {}>(base: TBase) { ... } will work for both abstract and non-abstract superclasses:

function Mixin<TBase extends abstract new (...args: any[]) => {}>(base: TBase) {
    abstract class M extends base {
    }
    return M;
}

abstract class AbstractBase {
    abstract m(): number;
}

class ConcreteBase {
    m(): number { return 1;}
}

// error: C must implement abstract method `m`
class C extends Mixin(AbstractBase) {
}

// no error
class D extends Mixin(ConcreteBase) {
}

@rbuckton
Copy link
Member Author

rbuckton commented Sep 9, 2020

Although it should probably be an error for class M extends base, since M should also be marked abstract.

@orta
Copy link
Contributor

orta commented Sep 9, 2020

@typescript-bot pack this

@rbuckton rbuckton requested a review from sheetalkamat January 5, 2021 22:20
@DanielRosenwasser
Copy link
Member

I think the one thing that I'm still a little bit surprised about is that there's no way to rewrite the following example

abstract class Foo {
  abstract fooMethod(): void
}

function extendFoo(FooCtor: abstract new () => Foo) {
  // Error! Must implement 'fooMethod'
  return class extends FooCtor {
  }
}

in such a way that Foo is declared as any other object type like an interface.

The first reason I think this is weird is because abstract doesn't get funneled around as any other modifier, and so this gets a little funky as you try to do algebra over the properties.

abstract class Foo {
  abstract fooMethod(): void
}

// Homomorphic mapped type over `Foo`
type CopyFoo = {
    [K in keyof Foo]: Foo[K]
}

function extendFoo(FooCtor: abstract new () => CopyFoo) {
  // No error now!
  return class extends FooCtor {
  }
}

The second is that in order to model this sort of pattern, you always have to declare a class, even if you won't necessarily be extending from that class (and even if nobody is going to be extending from that class). That on its own feels kind of strange in our mostly-structural type system.

But I don't really know the right way to reconcile all of this - would "fixing" these issues mean that interfaces would have to allow abstract members? Would interfaces themselves need to be declared abstract? And how would this work with class/interface merging?

@rbuckton
Copy link
Member Author

rbuckton commented Jan 5, 2021

The issue is that we don't (yet) have a way to represent this type in any other way. #41587 will eventually add syntax that would permit this in type-space:

type TFoo = typeof abstract class {
  constructor();
  abstract fooMethod(): void;
};

function extendFoo(FooCtor: TFoo) { ... }

However, typeof class doesn't solve the issue that abstract new is intended to solve. With typeof class, you still cannot infer the instance type, so there's no version of InstanceType<T> that could use typeof class on its own. However, with abstract constructor types, we can eventually rewrite InstanceType<T> as this:

type InstanceType<T extends abstract new (...args: any) => any> = T extends abstract new (...args: any) => infer R ? R : any;

With a new InstanceType, we can have a type definition that preserves abstract without introducing a class in the value-space:

type TFooInst = InstanceType<typeof abstract class {
  abstract fooMethod(): void;
}>;

In the end we will need both abstract constructor types and typeof class, but one is not dependent on the other. The main reason to ship abstract constructor types a version ahead of typeof class has to do with our dependency on typescript-eslint. Since typescript-eslint won't understand abstract new until it ships in a stable release, we need abstract new to ship one version prior to changing InstanceType to avoid lint errors in our own codebase. That way, by the time typeof class lands, we'll have a version of InstanceType that can be used in the way mentioned above.

Whether or not we eventually do something else for abstract in regards to interfaces or our type-algebra may be related to this feature, but is essentially out of scope as any decisions we make there are unlikely to impact the behavior of this feature. The only real cross-cutting concern I see would be that we might eventually loosen the restriction that classes extending from abstract new base types must themselves be declared abstract (instead basing it off of whatever new rules we come up with), and that wouldn't be a breaking change.

@rbuckton
Copy link
Member Author

rbuckton commented Jan 5, 2021

I think the one thing that I'm still a little bit surprised about is that there's no way to rewrite the following example

abstract class Foo {
  abstract fooMethod(): void
}

function extendFoo(FooCtor: abstract new () => Foo) {
  // Error! Must implement 'fooMethod'
  return class extends FooCtor {
  }
}

in such a way that Foo is declared as any other object type like an interface.

Actually, the example above isn't the problem, as FooCtor isn't generic, so you can return a non-abstract class:

function extendFoo(FooCtor: abstract new () => Foo) {
  // No error
  return class extends FooCtor {
    fooMethod(): void {} // implements it just fine.
  }
}

While this isn't necessarily sound, it aligns with other inconsistencies in the language, in that we assume Foo is the only instance type we'll create (even if you could pass an assignment compatible definition with more abstract methods). The important bit we discussed in the design meeting in November was in regards to a generic abstract constructor type. When an abstract constructor type comes from the constraint of a generic, we cannot know all of the possible permutations of that generic, therefore we must ensure the result is still marked abstract. That flows through how we handle mixins in the language, such that abstract methods in the base must eventually be implemented:

interface Foo {
  normalMethod(): void;
}
declare function FooMixin<Ctor extends abstract new(...args: any[]) => any>(base: Ctor): Ctor & (abstract new (...args: any[]) => Foo);

class Concrete {}
class SubConcrete extends MixinFoo(Concrete) {} // ok

abstract class Abstract {
  abstract fooMethod(): void;
}
class SubConcreteAbstract extends FooMixin(Abstract) {
  // I must implement 'fooMethod' because it's marked 'abstract' in the base class.
  fooMethod(): void {}
}

@rbuckton
Copy link
Member Author

rbuckton commented Jan 5, 2021

[...] Would interfaces themselves need to be declared abstract? [...]

No. We declare a class as abstract because it affects the constructor side. If we had abstract methods on interfaces, you might say the following are essentially the same:

// as a class
abstract class Foo {
  abstract fooMethod(): void;
}

// as an interface/constructor
interface Foo {
  abstract fooMethod(): void;
}
let Foo: abstract new () => Foo;

I would imagine that if we did introduce abstract in an interface method, we'd probably want to make new () => Foo an error since you cannot create an instance of an object with abstract methods, in the same way that class Foo { abstract fooMethod(): void; } is an error today.

@rbuckton
Copy link
Member Author

rbuckton commented Jan 7, 2021

I'll be pushing up a minor change to declaration emit/quick info shortly to handle inferred return types that result in an anonymous type with abstract construct signatures.

For reference, in this case:

// @target: esnext
// @declaration: true

interface Mixin {
    mixinMethod(): void;
}

function Mixin<TBase extends abstract new (...args: any[]) => any>(baseClass: TBase) {
    // must be `abstract` because we cannot know *all* of the possible abstract members that need to be
    // implemented for this to be concrete.
    abstract class MixinClass extends baseClass implements Mixin {
        mixinMethod(): void {}
        static staticMixinMethod(): void {}
    }
    return MixinClass;
}

We currently emit this:

declare function Mixin<TBase extends abstract new (...args: any[]) => any>(baseClass: TBase): {
    new (...args: any[]): { // does not preserve `abstract`
      [x: string]: any;
      mixinMethod(): void;
    };
    staticMixinMethod(): void;
}) & TBase;

When we need to emit this instead:

declare function Mixin<TBase extends abstract new (...args: any[]) => any>(baseClass: TBase): ((abstract new (...args: any[]) => {
    [x: string]: any;
    mixinMethod(): void;
}) & {
    staticMixinMethod(): void;
}) & TBase;

@rbuckton
Copy link
Member Author

rbuckton commented Jan 7, 2021

@RyanCavanaugh can you take one more look with the update for declaration emit?

@rbuckton rbuckton force-pushed the abstractConstructSignatures branch from d88027d to cfec2ca Compare January 7, 2021 22:56
@rbuckton rbuckton merged commit 0d284e6 into master Jan 8, 2021
@rbuckton rbuckton deleted the abstractConstructSignatures branch January 8, 2021 02:23
Zzzen pushed a commit to Zzzen/TypeScript that referenced this pull request Jan 16, 2021
* Add support for abstract constructor types

* Add backwards-compatible overloads for creating/updating constructor types

* Reverting use of 'abstract' in lib/es5.d.ts due to eslint issues

* Update baseline due to reverting lib

* Add error for failing to mark an mixin class as abstract

* Fix declaration/quick info for abstract construct signatures
raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Feb 26, 2021
The Model/Entity being abstract caused compilation errors as TS 4.2
reinforces abstract checks.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/
microsoft/TypeScript#36392

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Feb 27, 2021
The Model/Entity being abstract caused compilation errors as TS 4.2
reinforces abstract checks.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/
microsoft/TypeScript#36392

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
raymondfeng added a commit to loopbackio/loopback-next that referenced this pull request Feb 27, 2021
The Model/Entity being abstract caused compilation errors as TS 4.2
reinforces abstract checks.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/
microsoft/TypeScript#36392

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
@ITenthusiasm
Copy link

ITenthusiasm commented Mar 5, 2021

Sorry to ask, but has this already been released? abstract doesn't seem to be a recognized keyword in my repository.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Mar 24, 2021

This also updates the definitions for InstanceType and ConstructorParameters to use the abstract modifier

This doesn't appear to actually be present? There is no mention of ConstructorParameters in the PR diff, and both InstanceType and ConstructorParameters don't support abstract classes in the latest version of TS. You can build a custom ConstructorParameters, suggesting that this has been released.

@rbuckton
Copy link
Member Author

This also updates the definitions for InstanceType and ConstructorParameters to use the abstract modifier

This doesn't appear to actually be present? There is no mention of ConstructorParameters in the PR diff, and both InstanceType and ConstructorParameters don't support abstract classes in the latest version of TS. You can build a custom ConstructorParameters, suggesting that this has been released.

They couldn't be added at the same time due to a lag between TS releases and the TypeScript ESLint plugin. They will be added by #43380.

pktippa pushed a commit to pktippa/loopback-next that referenced this pull request May 1, 2021
The Model/Entity being abstract caused compilation errors as TS 4.2
reinforces abstract checks.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/
microsoft/TypeScript#36392

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>
@jcalz
Copy link
Contributor

jcalz commented Jan 31, 2022

I was a little surprised that you can't write { abstract new(): Foo } as a synonym for abstract new() => Foo. Not sure if that's documented anywhere, but I figured I'd mention it.

@rbuckton
Copy link
Member Author

It's hinted at a bit here: #36392 (comment), but was something we discussed in a Design Meeting. Adding abstract signatures to an interface (and possibly abstract members as well) was something we didn't want to touch on yet with this feature, as it introduces significantly more complexity. The intent was for the typeof class type to handle this, eventually.

@ntucker
Copy link

ntucker commented Feb 6, 2023

The declaration emit seems to not enforce abstract members...

from

type Constructor = abstract new (...args: any[]) => {};

function mixin<TBase extends Constructor>(Base: TBase) {
  abstract class MyClass extends Base {
    abstract abs(): void;
  }
  return MyClass;
}

I get

type Constructor = abstract new (...args: any[]) => {};
declare function mixin<TBase extends Constructor>(Base: TBase): (abstract new (...args: any[]) => {
    abs(): void;
}) & TBase;

But if I use that as an ambient declaration (imagine publishing a package based on this). Then an implementation lacking abs doesn't error:

class Broken extends mixin(class {}) {
  field = 5;
}

Playground (this obviously errors as it's not using ambient)

@lorenzodallavecchia
Copy link

I have noticed that abstract constructor types do not accept a class with a protected constructor.
See this playground.

Argument of type 'typeof A2' is not assignable to parameter of type 'abstract new (...args: any[]) => object'.
Cannot assign a 'protected' constructor type to a 'public' constructor type.(2345)

What is the reason for this behavior?
Abstract constructors cannot be called anyway, so their visibility should not matter IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master For Backlog Bug PRs that fix a backlog bug
Projects
None yet