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

T.constructor should be of type T #3841

Open
LPGhatguy opened this issue Jul 13, 2015 · 92 comments
Open

T.constructor should be of type T #3841

LPGhatguy opened this issue Jul 13, 2015 · 92 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@LPGhatguy
Copy link
Contributor

Given

class Example {
}

The current type of Example.constructor is Function, but I feel that it should be typeof Example instead. The use case for this is as follows:

I'd like to reference the current value of an overridden static property on the current class.

In TypeScript v1.5-beta, doing this requires:

class Example {
    static someProperty = "Hello, world!";

    constructor() {
        // Output overloaded value of someProperty, if it is overloaded.
        console.log(
            (<typeof Example>this.constructor).someProperty
        );
    }
}

class SubExample {
    static someProperty = "Overloaded! Hello world!";

    someMethod() {
        console.log(
            (<typeof SubExample>this.constructor).someProperty
        );
    }
}

After this proposal, the above block could be shortened to:

class Example {
    static someProperty = "Hello, world!";

    constructor() {
        // Output overloaded value of someProperty, if it is overloaded.
        console.log(
            this.constructor.someProperty
        );
    }
}

class SubExample {
    static someProperty = "Overloaded! Hello world!";

    someMethod() {
        console.log(
            this.constructor.someProperty
        );
    }
}

This removes a cast to the current class.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jul 13, 2015
@kitsonk
Copy link
Contributor

kitsonk commented Sep 29, 2015

Also, for clarity, as discussed in #4356 it is logical based on the ES specification to strongly type .constructor property of an instance and the following is essentially equivalent valid ways of creating instances:

class Foo {
    foo() { console.log('bar'); }
}

let foo1 = new Foo();

let foo2 = new foo1.constructor();

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". and removed In Discussion Not yet reached consensus labels Oct 5, 2015
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Oct 5, 2015
@RyanCavanaugh
Copy link
Member

Accepting PRs for this. Anyone interested? 😄

@weswigham
Copy link
Member

I spoke with @ahejlsberg about this - we could type constructor fairly well just with a change to our lib.d.ts now that we have this types (in theory):

interface Constructor<T> {
  new (...args: any[]): T;
  prototype: T;
}

interface Object {
    constructor: Constructor<this>;
}

But! There are two issues - we don't instantiate this types on apparent type members (as in, anything on object) correctly right now, and there would be a performance impact in making every object have a this typed member (it's constructor member). Additionally, this method doesn't capture the arguments of the constructor of the class (or its static members), so it could stand to be improved.

@RyanCavanaugh
Copy link
Member

Additionally, this method doesn't capture the arguments of the constructor of the class (or its static members),

This was a dealbreaker for us since it wouldn't even address the problem in the OP. Wiring it up in the compiler the same way we do prototype seems necessary.

@jbondc
Copy link
Contributor

jbondc commented Oct 20, 2015

Can take a look, does

  1. Removing from lib.d.ts:
interface Object {
    constructor: Function;
  1. inferring/adding a "constructor" property/symbol with a type to the classType seem like the right thing to do?

@jbondc
Copy link
Contributor

jbondc commented Oct 20, 2015

Nevermind, it's a bit more involved then I thought. Attempt here in case helpful:
master...jbondc:this.constructor

Think proper way involves changing ConstructorKeyword in getSymbolAtLocation()

@Arnavion
Copy link
Contributor

Arnavion commented Dec 6, 2015

As I mentioned in #5933, will this not be a breaking change for assigning object literals to class types?

class Foo {
    constructor(public prop: string) { }
}

var foo: Foo = { prop: "5" };

This is currently allowed, but with this issue fixed it should produce an error that the literal is missing the constructor property? If it didn't produce an error, then it would be allowed to call statics on Foo such as foo.constructor.bar() which would break if foo.constructor wasn't actually Foo.

@LPGhatguy
Copy link
Contributor Author

Would this be possible to implement solely with the polymorphic this type that's a work in progress? Assuming typeof mechanics would work there as well, could the signature for Object.prototype.constructor be set to:

interface Object {
    constructor: typeof this;
}

@ahejlsberg
Copy link
Member

The general problem we face here is that lots of valid JavaScript code has derived constructors that aren't proper subtypes of their base constructors. In other words, the derived constructors "violate" the substitution principle on the static side of the class. For example:

class Base {
    constructor() { }  // No parameters
}

class Derived {
    constructor(x: number, y: number) { }  // x and y are required parameters
}

var factory: typeof Base = Derived;  // Error, Derived constructor signature incompatible
var x = new factory();

If we were to add a strongly typed constructor property to every class, then it would become an error to declare the Derived class above (we'd get the same error for the constructor property that we get for the factory variable above). Effectively we'd require all derived classes to have constructors that are compatible with (i.e. substitutable for) the base constructor. That's simply not feasible, not to mention that it would be a massive breaking change.

In cases where you do want substitutability you can manually declare a "constructor" property. There's an example here. I'm not sure we can do much better than that.

@pocesar
Copy link

pocesar commented Jan 18, 2016

Well, since the constructor, per spec (and after typescript compilation) is always the defined in the prototype, I don't see why it shouldn't be strongly typed. If people are hacking their prototypes manually, let them typecast their code. this.constructor should be the class definition, and super.constructor the immediate super prototype definition, and so on.

Since non-typescript modules usually have their manual typings (through DefinitelyTyped), it's safe to rewrite their constructor there as needed (in case the module author did something silly like):

function Foo(){
}
Foo.prototype = {
   constructor: Foo,
   someMethod: function() {
   }
}

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus and removed Help Wanted You can do this Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". labels Jan 19, 2016
@RyanCavanaugh RyanCavanaugh removed this from the Community milestone Jan 19, 2016
@RyanCavanaugh
Copy link
Member

I don't understand what's going on in this thread. Need to take it back to the design meeting.

@TechQuery
Copy link

If someone wants to make it work with prettier (which removes the quotes), wrapping around [] does the trick:

class C {
    ['constructor']: typeof C;
}

As the new confused proposal "class fields as define properties" of ES2022 becomes TypeScript default behavior, we must write as following to avoid access-blocking to the prototype:

class C {
    declare ['constructor']: typeof C;
}

@hinell
Copy link

hinell commented Mar 29, 2023

@TechQuery This issue didn't age well as I see. Now even more crowded, completely unnecessary keywords are needed. What a joke. This is going to break backwards compatibility again. This is crazy. I think TS needs to be replaced by more robust and sensible language.

@datvm
Copy link

datvm commented Jun 5, 2023

Any workaround for annonymous class without manually giving it a name?

new class {

    static #init = false;

    constructor() {
        (this.constructor as ???).#init = true; // What to put in here?
    }

}();

@ljharb
Copy link
Contributor

ljharb commented Jun 5, 2023

the workaround is to give it a name, since anonymous things hurt debugging anyways.

@hinell
Copy link

hinell commented Jun 6, 2023

The workaround is to drop TypeScript altogehter. We need a better language. Read:

@ShaMan123
Copy link

ShaMan123 commented Jun 6, 2023

That should be a great concern and alarming to the TS team.
As well as this issue and the need to do horrible workarounds to do things that are considered the core scope of TS.
TS is there to make coding easier, not harder.
All the workarounds in this thread are too ugly, forgive me if that offends anyone.

@pinko-fowle
Copy link

(this.constructor as MyClass).myStaticMethod() is the workaround I've been using. Quite dislike it.

Here's an example use case:

var A = class {
        static answer(): any { return 42 };
        lifeUniverseEverything() { return (this.constructor as typeof A).answer() } // WORKAROUND HERE
}
var B = class extends A {
        static answer(){ return "6*9" }
}
console.log(`the answer to life the universe & everything is... ${(new B).lifeUniverseEverything()}`)
// => the answer to life the universe & everything is... 6*9

Being able to have virtual static methods is a super-powerful & super-useful capabillity! Wish it was better supported. Thanks, happy hacking folks.

@clshortfuse
Copy link

clshortfuse commented Sep 9, 2023

@pinko-fowle Note that doing so regenerates the entire class, with the equivalent of

{
  new (...args: any[]): {[K in key of CLASS]: CLASS[K]},
  [K in key of typeof CLASS]: typeof CLASS[K],
} & typeof CLASS;

That makes a really gigantic file when you generate types since it's duplicating every single key. The more you extend the worse it gets. Something like HTMLElement has 100s of properties.

@trusktr
Copy link
Contributor

trusktr commented Sep 21, 2023

@Fryuni I can't get your example to work.

playground

Screenshot 2023-09-20 at 9 51 25 PM

@Fryuni
Copy link

Fryuni commented Sep 21, 2023

@trusktr, there are three problems:

The first one is somewhat silly, but you need the export {} somewhere at the top level of the file for the declare global to work correctly. Don't ask me why, I never figured that one out.

Second, my solution cannot infer the type; just validate it. demonstration.

The last one I mentioned in this comment, when the class has no private instance members, any other class with a subset of the instance members will be assignable to it. It worked for us because all our classes do have private members. demonstration

@clshortfuse
Copy link

@trusktr If you're okay with not using native syntax for JS extends you can do this:

class TSConstructorFix {
  /**
   * @type {{
   * <T1 extends typeof TSConstructorFix, T2 extends T1, T3 extends (Base:T1) => T2>
   * (this: T1,customExtender: T3):
   *  ReturnType<T3>
   *      & {
   *          new(): {
   *              static(): {[K in keyof ReturnType<T3>]: ReturnType<T3>[K]}
   *          } & {
   *            [K in keyof InstanceType<ReturnType<T3>>]: InstanceType<ReturnType<T3>>[K]}
   *          }
   *      }
   * }}
   */
  static extend(customExtender) {
    // @ts-expect-error Can't cast T
    return customExtender(this);
  }

  static() {
    return this.constructor;
  }
}

const Test = TSConstructorFix.extend((Base) => class extends Base {
  static foo = 123;

  go() { return 0; }
});

const t = new Test();
t.static().foo;
t.go();

Playground link

That's basically a fork of how I've workaround this issue. I'm kinda used to that more declarative syntax now, personally.

Base class

Extension Example

@clshortfuse
Copy link

clshortfuse commented Sep 21, 2023

So this works...

class TSConstructorFix {
  /**
   * @type {{
   * <T1 extends typeof TSConstructorFix, T2 extends T1, T3 extends (Base:T1) => T2>
   * (this: T1,customExtender: T3):
   *  ReturnType<T3>
   *      & {
   *          new(...args: any[]): {
   *              constructor: {[K in keyof ReturnType<T3>]: ReturnType<T3>[K]}
   *          } & {
   *            [K in keyof InstanceType<ReturnType<T3>>]: InstanceType<ReturnType<T3>>[K]}
   *          }
   *      }
   * }}
   */
  static extend(customExtender) {
    // @ts-expect-error Can't cast T
    return customExtender(this);
  }
}
- class Test {
+ class Test extends TSConstructorFix.extend((Base) => class extends Base {
  static foo = 123;

  go() { return 0; }
- }
+ }) {}
const t = new Test();
t.constructor.foo += 4;
t.go();

If you're willing to add extends TSConstructorFix.extend((Base) => class extends Base after every class name and ]) { at the end (maybe a Regex). There's more probably some more coaxing of ConstructorParameters<T> left, but that's the main idea.

@trusktr
Copy link
Contributor

trusktr commented Sep 22, 2023

@clshortfuse that'd be a fairly invasive change I think. Better to just const ctor = this.constructor as Foo in some few spots I think.

@Fryuni Thanks, got it! Btw, export {} not needed if you tack the export onto one of your actual classes (that's what I had in my example).

@teadrinker2015
Copy link

the workaround is to give it a name, since anonymous things hurt debugging anyways.

Considering calling your parents and your friends in their full names. It's more explicit.

@jwbth
Copy link

jwbth commented Sep 29, 2024

As the new confused proposal "class fields as define properties" of ES2022 becomes TypeScript default behavior, we must write as following to avoid access-blocking to the prototype:

class C {
    declare ['constructor']: typeof C;
}

Factors than neccesiate using declare include:

  • having target set to es2020 or higher in tsconfig.json;
  • having useDefineForClassFields set to true (e.g. this is done in the default configuration for Vue).

Setting useDefineForClassFields to false will make the code work without declare. Can't wouch it won't break anything else. See https://www.typescriptlang.org/docs/handbook/2/classes.html#type-only-field-declarations for details.

DavisVaughan added a commit to posit-dev/positron that referenced this issue Sep 30, 2024
…eam specialization (#4848)

Addresses #4519

In `addActivityItemStream()`, either a `ActivityItemOutputStream` or
`ActivityItemErrorStream` always goes in, but it was possible for a
"generic" `ActivityItemStream` to come out here when we make one from
the `remainderText` after the last newline:


https://github.com/posit-dev/positron/blob/e51332d5baa59b72f5e8b28861bbb751f25452ea/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts#L118-L136

That generic `ActivityItemStream` would get set here:

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L110-L116

And then pushed onto `this._activityItems`

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L138-L139

The problem is that our Console code doesn't know how to deal with a
generic `ActivityItemStream`. It must be a specialized
`ActivityItemOutputStream` or `ActivityItemErrorStream`!

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/contrib/positronConsole/browser/components/runtimeActivity.tsx#L46-L49

If a generic `ActivityItemStream` slipped through, the output would
never appear in the Console, making it look "dropped", as reported in
#4519.

---

I like how we have a generic `ActivityItemStream` class that we
specialize for the output/error types, so I've fixed this bug by using
[polymorphic
`this`](https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types)
to allow `addActivityItemStream()` to take and return objects of the
correct specialized type.

Ideally we'd simply be able to use `new this.constructor()` in place of
`new ActivityItemStream()` to use the polymorphic constructor when we
need to create new objects, but due to some [well known
weirdness](microsoft/TypeScript#3841) in
typescript, you have to manually cast it to the constructor type, so
`newActivityItemStream()` wraps that up.

I've also made the `ActivityItemStream` constructor `protected` to avoid
us using it by accident in the codebase. We should _always_ be creating
`ActivityItemOutputStream` or `ActivityItemErrorStream` instead.

Before 😢 



https://github.com/user-attachments/assets/b1854d9e-94eb-4952-aead-bebcf5c5d6a2



After 🥳 


https://github.com/user-attachments/assets/4a76974c-6862-4e84-a296-620717ddb8da
@thodnev
Copy link

thodnev commented Oct 16, 2024

Could someone please add it to TS Milestones for one of the upcoming versions

The issue is open for 9 years. And it deserves being addressed. Brief Stackoverflow review shows devs even started using ClassName.method notation instead of this.constructor.method thus bringing Divergent Change code smell into their projects to simply satisfy the typing

The solution above by @jwbth seems the most clear and concise.
It is just one line of code at the top. Not a big deal.

But it still has to be added and class name still has to be provided in two distinct places. If someone accidentally forgets to change one of them while changing the other, this can lead to a very pleasant debugging experience.

isabelizimm pushed a commit to posit-dev/positron that referenced this issue Oct 16, 2024
…eam specialization (#4848)

Addresses #4519

In `addActivityItemStream()`, either a `ActivityItemOutputStream` or
`ActivityItemErrorStream` always goes in, but it was possible for a
"generic" `ActivityItemStream` to come out here when we make one from
the `remainderText` after the last newline:


https://github.com/posit-dev/positron/blob/e51332d5baa59b72f5e8b28861bbb751f25452ea/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts#L118-L136

That generic `ActivityItemStream` would get set here:

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L110-L116

And then pushed onto `this._activityItems`

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L138-L139

The problem is that our Console code doesn't know how to deal with a
generic `ActivityItemStream`. It must be a specialized
`ActivityItemOutputStream` or `ActivityItemErrorStream`!

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/contrib/positronConsole/browser/components/runtimeActivity.tsx#L46-L49

If a generic `ActivityItemStream` slipped through, the output would
never appear in the Console, making it look "dropped", as reported in
#4519.

---

I like how we have a generic `ActivityItemStream` class that we
specialize for the output/error types, so I've fixed this bug by using
[polymorphic
`this`](https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types)
to allow `addActivityItemStream()` to take and return objects of the
correct specialized type.

Ideally we'd simply be able to use `new this.constructor()` in place of
`new ActivityItemStream()` to use the polymorphic constructor when we
need to create new objects, but due to some [well known
weirdness](microsoft/TypeScript#3841) in
typescript, you have to manually cast it to the constructor type, so
`newActivityItemStream()` wraps that up.

I've also made the `ActivityItemStream` constructor `protected` to avoid
us using it by accident in the codebase. We should _always_ be creating
`ActivityItemOutputStream` or `ActivityItemErrorStream` instead.

Before 😢 



https://github.com/user-attachments/assets/b1854d9e-94eb-4952-aead-bebcf5c5d6a2



After 🥳 


https://github.com/user-attachments/assets/4a76974c-6862-4e84-a296-620717ddb8da
isabelizimm pushed a commit to posit-dev/positron that referenced this issue Oct 16, 2024
…eam specialization (#4848)

Addresses #4519

In `addActivityItemStream()`, either a `ActivityItemOutputStream` or
`ActivityItemErrorStream` always goes in, but it was possible for a
"generic" `ActivityItemStream` to come out here when we make one from
the `remainderText` after the last newline:


https://github.com/posit-dev/positron/blob/e51332d5baa59b72f5e8b28861bbb751f25452ea/src/vs/workbench/services/positronConsole/browser/classes/activityItemStream.ts#L118-L136

That generic `ActivityItemStream` would get set here:

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L110-L116

And then pushed onto `this._activityItems`

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/services/positronConsole/browser/classes/runtimeItemActivity.ts#L138-L139

The problem is that our Console code doesn't know how to deal with a
generic `ActivityItemStream`. It must be a specialized
`ActivityItemOutputStream` or `ActivityItemErrorStream`!

https://github.com/posit-dev/positron/blob/c9504d307c686ce2deefdc6887ad2f7b044dd2e9/src/vs/workbench/contrib/positronConsole/browser/components/runtimeActivity.tsx#L46-L49

If a generic `ActivityItemStream` slipped through, the output would
never appear in the Console, making it look "dropped", as reported in
#4519.

---

I like how we have a generic `ActivityItemStream` class that we
specialize for the output/error types, so I've fixed this bug by using
[polymorphic
`this`](https://www.typescriptlang.org/docs/handbook/advanced-types.html#polymorphic-this-types)
to allow `addActivityItemStream()` to take and return objects of the
correct specialized type.

Ideally we'd simply be able to use `new this.constructor()` in place of
`new ActivityItemStream()` to use the polymorphic constructor when we
need to create new objects, but due to some [well known
weirdness](microsoft/TypeScript#3841) in
typescript, you have to manually cast it to the constructor type, so
`newActivityItemStream()` wraps that up.

I've also made the `ActivityItemStream` constructor `protected` to avoid
us using it by accident in the codebase. We should _always_ be creating
`ActivityItemOutputStream` or `ActivityItemErrorStream` instead.

Before 😢 



https://github.com/user-attachments/assets/b1854d9e-94eb-4952-aead-bebcf5c5d6a2



After 🥳 


https://github.com/user-attachments/assets/4a76974c-6862-4e84-a296-620717ddb8da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests