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

Use Symbol.hasInstance for instanceof narrowing, include constructor as part of class structural type #17360

Closed
dead-claudia opened this issue Jul 22, 2017 · 3 comments · Fixed by #55052
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@dead-claudia
Copy link

My proposal:

  • For instanceof expressions, if Symbol.hasInstance exists on the RHS, narrow the expression according to the method's return value.
  • For classes, implicitly add a type-level constructor prototype property which is typeof classConstructor, if such a property was not defined in the class body.
interface Foo { constructor: typeof Foo }
declare const Foo: {
    [Symbol.hasInstance](x: any): x is Foo
}

declare const Bar: {
    [Symbol.hasInstance](x: any): x is number
}

declare const foo: any
declare const bar: any

if (foo instanceof Foo) {
    // foo should be inferred as a Foo
}

if (bar instanceof Bar) {
    // bar should be inferred as a number
}

The rationale of this is that you can then (via compiler flag) by default define Symbol.hasInstance on constructors to type check against their return type (like for new () => T, defining [Symbol.hasInstance](x: any): x is T). This would mostly mitigate a certain unsoundness issue with classes, making the workaround a bit more difficult to do:

class Foo {
    bar: number = 1;
}

const foo1: Foo = new Foo() // Legal
const foo2: Foo = {bar: 2}  // No longer legal

// Making these illegal are out of scope of this proposal
const foo3: Foo = {
    constructor: Foo;
    bar: 2,
}
const foo4: Foo = new class {
    bar: number = 1
}

Notes:

@mhegazy mhegazy added the Needs Investigation This issue needs a team member to investigate its status. label Aug 29, 2017
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Sep 16, 2017
@goodmind
Copy link

any updates?

@weswigham weswigham added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. In Discussion Not yet reached consensus labels Nov 6, 2018
@RyanCavanaugh RyanCavanaugh removed In Discussion Not yet reached consensus labels Mar 7, 2019
@rbuckton
Copy link
Member

rbuckton commented Jun 9, 2019

For classes, implicitly add a type-level constructor prototype property which is typeof classConstructor, if such a property was not defined in the class body.

I wouldn't recommend this, as it would make it impossible for a subclass to define a constructor with arguments that differ from its base, because the construct signatures of the constructor property would no longer be assignable.

See this example in the playground for why this wouldn't work.

Aside from that, I agree that pairing instanceof and Symbol.hasInstance for type guards makes sense. It would be especially helpful for mixins:

interface NameMixin {
  name: string;
}

const kName = Symbol("kName");
const NameMixin = <TBaseClass extends Constructor>(baseClass: TBaseClass): 
  TBaseClass & (new (...args: any[]) => MyMixin) => {

  abstract class MixinClass extends baseClass implements NameMixin {
    private [kName] = "";
    get name() { return this[kName]; }
    set name(value) { this[kName] = value; }
  }

  return MixinClass;
};

NameMixin[Symbol.hasInstance] = (obj: unknown) => obj is NameMixin {
  return typeof obj === "object" && obj !== null && kName in obj;
};

class Base { }
class Sub extends NameMixin(Base) {}

const s: unknown = new Sub();
if (s instanceof NameMixin) {
 // s has type 'NameMixin'
}

@monolithed
Copy link

The same problem:

type X = string | string[];

const isArray = (type: unknown): type is string[] => {
    return Array.isArray(type);
};

if (isArray(x)) {
   // Ok
}
class isArray {
    static [Symbol.hasInstance](type: unknown): type is string[] {
        return Array.isArray(type);
    }
}


if (x instanceof isArray) {
   //   TS2345: Argument of type 'string | string[]' is not assignable to parameter of type 'string[]'.   Type 'string' is not assignable to type 'string[]'.
    }
}

It looks like a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants