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

Some regression with union of classes in TS4.2 #43753

Closed
dartess opened this issue Apr 20, 2021 · 8 comments
Closed

Some regression with union of classes in TS4.2 #43753

dartess opened this issue Apr 20, 2021 · 8 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@dartess
Copy link

dartess commented Apr 20, 2021

Bug Report

I apologize for the name of issue, I was not able to correctly formulate the problem.

🔎 Search Terms

4.2, regression, class, extends, union

🕗 Version & Regression Information

This changed between versions 4.1.5 and 4.2.2
(if we take dev then between versions 4.2.0-dev.20210209 and 4.2.0-insiders.20210210)

⏯ Playground Link

Playground link with relevant code

💻 Code

I apologize again, I have shortened the example as much as I can.

/** some declared types */

declare abstract class GlobalController<P, S, SS> {
    isLocal: false;
    abstract getProps(storeState: SS, props: P, state: S): void;
}

declare abstract class LocalController<P, S> {
    isLocal: true;
    abstract getProps(props: P, state: S): void;
}

interface GlobalControllerClass<P, S, SS> {
    new(): GlobalController<P, S, SS>;
}

interface LocalControllerClass<P, S> {
    new(): LocalController<P, S>;
}

type ControllerClass<P, S, SS = {}> = GlobalControllerClass<P, S, SS> | LocalControllerClass<P, S>;

declare const createEnhancer: <P, S, SS = void>(Wrapper: ControllerClass<P, S, SS>) => any;


/** usage */

type ControllerState = {
    bar: any
};

type ControllerProps = {
    baz: any
};


/** fall since TS4.2 (4.1- works fine) */

class FailWithTS42 extends LocalController<ControllerProps, ControllerState> {
    getProps(props: ControllerProps, state: ControllerState) {}
}

createEnhancer(FailWithTS42);

in the sandbox above the same code plus some workarounds that might suggest something

🙁 Actual behavior

createEnhancer(FailWithTS42) in the last line shows the error:

Argument of type 'typeof FailWithTS42' is not assignable to parameter of type 'ControllerClass<ControllerState, ControllerState, ControllerProps>'.
  Type 'typeof FailWithTS42' is not assignable to type 'GlobalControllerClass<ControllerState, ControllerState, ControllerProps>'.
    Construct signature return types 'FailWithTS42' and 'GlobalController<ControllerState, ControllerState, ControllerProps>' are incompatible.
      The types of 'isLocal' are incompatible between these types.
        Type 'true' is not assignable to type 'false'.

🙂 Expected behavior

I can call createEnhancer with class FailWithTS42

@dartess dartess changed the title Some regression with union of classes Some regression with union of classes in TS4.2 Apr 21, 2021
@RyanCavanaugh
Copy link
Member

Here's a reduced version

declare class LocalController<T, U> {
    isLocal: true;
    getProps(a: T, b: U): void;
}
declare class GlobalController<T, U> {
    isLocal: false;
    // N.B.: Parameter type order is reversed
    getProps(a: U, b: T): void;
}

class FailWithTS42 extends LocalController<{ foo: any }, { bar: any }> { }
const p = new FailWithTS42();

// Fails
declare function createEnhancer1<T, U>(Wrapper: GlobalController<T, U> | LocalController<T, U>): void;
createEnhancer1(p);
// OK; this is the desired inference
createEnhancer1<{ foo: any }, { bar: any }>(p);


// OK
declare function createEnhancer2<T, U>(Wrapper: LocalController<T, U>): void;
createEnhancer2(p);

The TL:DR is that we're getting tripped up here and collecting inferences from GlobalController from the a and b parameters. What should be happening is that we'd detect that LocalController & GlobalController is never so there shouldn't be any inference candidates collected from the latter type.

I don't know why this ever worked.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Apr 21, 2021
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.4.0 milestone Apr 21, 2021
@RyanCavanaugh
Copy link
Member

cc @ahejlsberg for possible interest

@dartess
Copy link
Author

dartess commented Jul 2, 2021

@weswigham hi! thank you for your work!

i tried version npm:@typescript-deploys/[email protected].

The short version of the code from @RyanCavanaugh now really works without errors. But the original code still doesn't work. I tried to shorten my code a little more so that the error persists:

declare class GlobalController<P, S> {
    isLocal: false;
    getProps(state: S, props: P): void;
}

declare class LocalController<P, S> {
    isLocal: true;
    getProps(props: P, state: S): void;
}

interface GlobalControllerClass<P, S> {
    new(): GlobalController<P, S>;
}

interface LocalControllerClass<P, S> {
    new(): LocalController<P, S>;
}

declare function createEnhancer<P, S>(Wrapper: GlobalControllerClass<P, S> | LocalControllerClass<P, S>): void;

class FailWithTS42 extends LocalController<{ baz: any }, { bar: any }> {}

createEnhancer(FailWithTS42); // fall since TS4.2 (4.1- works fine)
TS2345: Argument of type 'typeof FailWithTS42' is not assignable to parameter of type 'GlobalControllerClass<{ bar: any; }, { baz: any; }> | LocalControllerClass<{ bar: any; }, { baz: any; }>'.
  Type 'typeof FailWithTS42' is not assignable to type 'GlobalControllerClass<{ bar: any; }, { baz: any; }>'.
    Construct signature return types 'FailWithTS42' and 'GlobalController<{ bar: any; }, { baz: any; }>' are incompatible.
      The types of 'isLocal' are incompatible between these types.
        Type 'true' is not assignable to type 'false'.

@andrewbranch andrewbranch added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 30, 2021
@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Help Wanted You can do this and removed Needs Investigation This issue needs a team member to investigate its status. Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone labels Feb 15, 2023
@RyanCavanaugh
Copy link
Member

Looking at this again, something is just going wrong in a bad way, I don't think this is some "subtle" thing without a direct fix, unless I'm really missing something. Trimming the repro down more and poking at the adjacent cases which do work:

declare function callme<T, U>(Wrapper: NotLocal<T, U> | Local<T, U>): void;

interface Local<T, U> {
    isLocal: true;
    a: T;
    b: U;
}
interface NotLocal<T, U> {
    isLocal: false;
    // N.B.: Order of types is reversed here
    a: U;
    b: T;
}

// No example should error.

// Works
declare const direct_decl: Local<string, number>;
callme(direct_decl);

// Works
type LocalStringNumberAlias = Local<string, number>;
declare const type_alias: LocalStringNumberAlias;
callme(type_alias);

// Fails
interface LocalStringNumberInterface extends Local<string, number> { }
declare const interface_decl: LocalStringNumberInterface;
callme(interface_decl);

That said we haven't seen other reports of this so I think this needs to be deprioritized in terms of scheduled work.

@Andarist this seems very much up your alley in terms of investigations?

@Andarist
Copy link
Contributor

A man nodding and 'Indeed, indeed' caption

@RyanCavanaugh
Copy link
Member

Me last night, going to bed right after posting that comment
image

I take back 90% of it; the problem is obviously that we're doing X<T> | Y <-> X<U> candidate collection via type identity in the first two cases (which works fine), but structural inference in the last case (which doesn't check discriminants and thus fails). This is indeed going to be difficult.

@RyanCavanaugh RyanCavanaugh added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Bug A bug in TypeScript Help Wanted You can do this labels Feb 15, 2023
@RyanCavanaugh RyanCavanaugh removed this from the Backlog milestone Feb 15, 2023
@Andarist
Copy link
Contributor

is there a specific reason why structural inference doesn't check discriminates or why this worked pre-4.2? 🤔

@RyanCavanaugh
Copy link
Member

I suspect this only worked by accident in 4.1-.

As for why to not check for discriminants - 40% perf hit to material-ui (I remember this one)

#43853 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
6 participants