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

TS is overly picky when declaring a class constructor type #29707

Open
bcherny opened this issue Feb 3, 2019 · 8 comments
Open

TS is overly picky when declaring a class constructor type #29707

bcherny opened this issue Feb 3, 2019 · 8 comments
Labels
Bug A bug in TypeScript Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Milestone

Comments

@bcherny
Copy link

bcherny commented Feb 3, 2019

TypeScript Version: 3.4.0-dev.20190202

Search Terms: class, extend, constructor, any[]

Code

type ClassConstructor = new(...args: any[]) => {}

function mixin<C extends ClassConstructor>(Class: C) {
  return class extends Class {}
}

Expected behavior:

I should be able to replace ...args: any[] with ...args: unknown[], or any other signature.

Actual behavior:

Error: Type 'C' is not a constructor function type. [2507]

Playground Link: https://www.typescriptlang.org/play/index.html#src=type%20ClassConstructor%20%3D%20new(...args%3A%20unknown%5B%5D)%20%3D%3E%20%7B%7D%0D%0A%0D%0Afunction%20mixin%3CC%20extends%20ClassConstructor%3E(Class%3A%20C)%20%7B%0D%0A%20%20return%20class%20extends%20Class%20%7B%7D%0D%0A%7D

@dragomirtitian
Copy link
Contributor

In the pull request the rest parameter of any[] is required:

In the following, the term mixin constructor type refers to a type that has a single construct signature with a single rest argument of type any[] and an object-like return type.

Not saying it should be but this is the expected behavior, unknown along came a long time after this PR, maybe the rules can be changed.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Feb 4, 2019
@RyanCavanaugh
Copy link
Member

Agree, unknown[] should be an acceptable substitute. Accepting PRs for an easy fix.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Good First Issue Well scoped, documented and has the green light labels Feb 4, 2019
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Feb 4, 2019
@dragomirtitian
Copy link
Contributor

@RyanCavanaugh So the issue seems ridiculously easy to fix (and I have the code ready), I was just wondering how to go about the tests. Should I add a new one or change one to include a version with unknown[] (this seems like a good candidate conformance/classes/mixinClassesAnnotated.ts)

@RyanCavanaugh
Copy link
Member

@dragomirtitian no preference; whichever's easiest for you

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Feb 4, 2019

@RyanCavanaugh I added a fix, just one problem (which unfortunately probably makes my fix almost useless).

The constraint new(...args: unknown[]) => {} works well as long as strictFunctionTypes are not turned on. When that flag is on this code becomes an error

class Base {
    constructor(public x: number, public y: number) {}
}
let c : new (...args: unknown[]) => {} = Base // error under strictFunctionTypes: true
new c("") // runtime type mismatch 

This means that you can't really call the mixin with anything but an empty constructor or one that has unknown arguments.

@RyanCavanaugh
Copy link
Member

That's why it's a constraint (not a concrete type) though, right?

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Feb 5, 2019

@RyanCavanaugh I can look into that, but this problem seems much bigger than mixins and would have implications broadly. Rest parameters of type unknown[] are generally not compatible with arbitrary parameter types even if we are talking about a constraint. This currently fails under strictFunctionTypes

function test<T extends (...a: unknown[]) => unknown>(fn:T) : T{
      fn(""); // this passes "" can be assigned to unknown 
      return fn;
}
test(function (a: number) {   // error here under strictFunctionTypes
})

I'm not 100% sure it is necessarily a great idea to make the code above error free . It seems to be weaker as far as type safety goes. As we see in the code above the callback passed in accepts a number but it's called inside test with a string. I think the current behavior is better, don't let a function with a number parameter be assigned to a function with an unknown parameter, the implementing function can't handle unknown.

While this is no worse than if we use any[], unknown[] would give a false sense of type-safety when no safety actually exists. Except for obeying the no-any tslint rule not sure why unknown is better in this instance. At least any would function as a '💀💀 careful unsafe 💀💀' warning, at least that's the way I see any :)

@jszopi
Copy link

jszopi commented Jul 23, 2022

The problem isn't specific to constructors or rest parameters, it's just that function types are contravariant in their parameter types. You only run into this inverted subtyping relation when you, for example, pass a function as a parameter to another function, as you do with mixins.

unknown is one of the widest types, and so will be the most restrictive when used as a function parameter type. For the function type to be as wide as possible, the parameter type must be as narrow as possible. any works because, while it's the widest type, it's also one of the narrowest. The counterintuitive conclusion is that the type of ...args should be never[].

I ran into this problem today when I needed to describe a type that would fit the right operand of instanceof. Since the code had no business in actually calling the constructor, never[] did the trick. For mixins, however, it seems like they need to take the constructor in, as well as return an interface based off it. With my limited TS experience, I don't see any other way of achieving this than with any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@bcherny @RyanCavanaugh @jszopi @dragomirtitian and others