Skip to content

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

Closed

Description

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

Activity

  1. changed the title [-]Some regression with union of classes[/-] [+]Some regression with union of classes in TS4.2[/+] on Apr 21, 2021
  2. RyanCavanaugh commented on Apr 21, 2021

    @RyanCavanaugh
    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.

  3. RyanCavanaugh commented on Apr 21, 2021

    @RyanCavanaugh
    Member

    cc Anders Hejlsberg (@ahejlsberg) for possible interest

  4. dartess commented on Jul 2, 2021

    @dartess
    Author

    Wesley Wigham (@weswigham) hi! thank you for your work!

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

    The short version of the code from Ryan Cavanaugh (@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'.
    
  5. 9 remaining items

  6. added
    BugA bug in TypeScript
    and removed
    RescheduledThis issue was previously scheduled to an earlier milestone
    on Feb 15, 2023
  7. RyanCavanaugh commented on Feb 15, 2023

    @RyanCavanaugh
    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.

    Mateusz Burzyński (@Andarist) this seems very much up your alley in terms of investigations?

  8. Andarist commented on Feb 15, 2023

    @Andarist
    Contributor

    A man nodding and 'Indeed, indeed' caption

  9. RyanCavanaugh commented on Feb 15, 2023

    @RyanCavanaugh
    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.

  10. added
    Design LimitationConstraints of the existing architecture prevent this from being fixed
    and removed
    BugA bug in TypeScript
    on Feb 15, 2023
  11. Andarist commented on Feb 15, 2023

    @Andarist
    Contributor

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

  12. RyanCavanaugh commented on Feb 15, 2023

    @RyanCavanaugh
    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Design LimitationConstraints of the existing architecture prevent this from being fixed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions