Skip to content

Mixins typing works with inference but not with explicit types #16089

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

Closed
dmitryxcom opened this issue May 25, 2017 · 7 comments
Closed

Mixins typing works with inference but not with explicit types #16089

dmitryxcom opened this issue May 25, 2017 · 7 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@dmitryxcom
Copy link

TypeScript Version: 2.3.1

Code

class Person {
  constructor(public name: string) {}
}

type Constructor<T> = new(...args: any[]) => T;

interface MixinFn<MixinInterface> {
  <T extends Constructor<{}>>(superclass: T): Constructor<MixinInterface> & T;
}

interface Tagable {
  tag: (tag: string) => void;
}


// Why doesn't this work?
type TagableMxinFn = MixinFn<Tagable>;
// Impossible to drop typing for T
// results in an error with --strict (superclass is any)
// and further error on extends saying that T should be of type a constructor
const Tagable: TagableMxinFn = <T extends Constructor<{}>>(superclass: T) => {
  return class extends superclass {
    // no typechecking of rerturn type; must use implements
  }
};

const TagablePerson = Tagable(Person);
let foo  = new TagablePerson('a');
// foo.tag(); error



// While this does
// the other way of acheiving the same typing is to remove return type
// `: Constructor(Tagable) & T`
// and specify `implements` Tagable in the class expression.
function Tagable2<T extends Constructor<{}>>(superclass: T):
    Constructor<Tagable> & T {
  return class extends superclass {
    // implements Tagable is enforced here
    // commenting out the next line results in an appropriate error
    tag(tag: string) {}
  }
}

const Tagable2Person = Tagable2(Person);
let bar = new Tagable2Person('s');
bar.name;
bar.tag('s'); // all good, autocompleted and checked

// Bonus: this is valid.
const TagableScam: TagableMxinFn = () => {};

export {};

Expected behavior:
conext: #13743 (comment)

  • function Tagable:MixinFn must resolve the type of superclass argument without explicit providing a generic typing for it.
  • function Tagable:MixinFn should be checked to be correct (Constructor & T)

Actual behavior:

  • none of these checks happen when types are explicitly specified in accordance with Mixin classes #13743
    i.e. { new(...args: any[]) => A } & { new(s: string) => B } doesn't seem to hold true.
    as far as I can tell typing a function with MixinFn does nothing.
@mhegazy
Copy link
Contributor

mhegazy commented May 25, 2017

I am not able to reproduce this locally. please provide a self contained reproduction of the issue in question.

c:\test\sandbox>type a.ts
class Person {
  constructor(public name: string) {}
}

type Constructor<T> = new(...args: any[]) => T;

interface MixinFn<MixinInterface> {
  <T extends Constructor<{}>>(superclass: T): Constructor<MixinInterface> & T;
}
interface Tagable {
  tag: (tag: string) => void;
}


// Why doesn't this work?
type TagableMxinFn = MixinFn<Tagable>;
// Impossible to drop typing for T
// results in an error with --strict (superclass is any)
// and further error on extends saying that T should be of type a constructor
const Tagable: TagableMxinFn = <T extends Constructor<{}>>(superclass: T) => {
  return class extends superclass {
    // no typechecking of rerturn type; must use implements
  }
};

const TagablePerson = Tagable(Person);
let foo  = new TagablePerson('a');
foo.tag("");



// While this does
// the other way of acheiving the same typing is to remove return type
// `: Constructor(Tagable) & T`
// and specify `implements` Tagable in the class expression.
function Tagable2<T extends Constructor<{}>>(superclass: T):
    Constructor<Tagable> & T {
  return class extends superclass {
    // implements Tagable is enforced here
    // commenting out the next line results in an appropriate error
    tag(tag: string) {}
  }
}

const Tagable2Person = Tagable2(Person);
let bar = new Tagable2Person('s');
bar.name;
bar.tag('s'); // all good, autocompleted and checked

// Bonus: this is valid.
const TagableScam: TagableMxinFn = () => {};

export {};

c:\test\sandbox>tsc --v
Version 2.3.3

c:\test\sandbox>tsc a.ts

c:\test\sandbox>echo %ERRORLEVEL%
0

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label May 25, 2017
@dmitryxcom
Copy link
Author

The point here is that const TagableScam: TagableMxinFn = () => {}; must break, and it doesn't, so you wouldn't see any errors. Im not sure how to reverse that into an error.

So should

// and further error on extends saying that T should be of type a constructor
const Tagable: TagableMxinFn = <T extends Constructor<{}>>(superclass: T) => {
  return class extends superclass {
    // no typechecking of rerturn type; must use implements
  }
};

since the return value doesn't implement Tagagble,

and once that is being checked, this should not break with --strict:

const Tagable2: MixinFn<Tagable> = (superclass)  => {
  return class extends superclass {
    // implements Tagable is enforced here
    // commenting out the next line results in an appropriate error
    tag(tag: string) {}
  }
};

@mhegazy
Copy link
Contributor

mhegazy commented May 25, 2017

i see. for the TagableScam, the issue here is before comparing a generic function the generic type parameters are erased (i.e. replaced with any), that makes the unsafe go undetected. #5616 tracks fixing this.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Jun 9, 2017
@dmitryxcom
Copy link
Author

Does it mean that what I described is supposed TS behavior (namely, inference behavior being impossible to code in types)?!
To me it doesn't look like so, and per your own guidelines:
https://github.com/Microsoft/TypeScript/wiki/FAQ#i-disagree-with-the-outcome-of-this-suggestion
I don't think it is supposed to be closed (i.e. It is a real, existing problem in the language that doesn't work the way it should).
Obviously, I'm quite disappointed with such a handling of a reported bug =(
The last action was adding a label of "needs more info", with all the info being provided. Next action is "closed". Sigh

@mhegazy
Copy link
Contributor

mhegazy commented Jun 12, 2017

Not sure i understand the issue at this point. can you provide some explanation and why not the same as . #5616.

@dmitryxcom
Copy link
Author

I misunderstood the automatically closed message. It does look like #5616 should fix it.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

2 participants