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

Unconstrained generics are incorrectly assignable to _any_ "Partial" type #30634

Open
weswigham opened this issue Mar 28, 2019 · 3 comments
Open
Labels
Bug A bug in TypeScript
Milestone

Comments

@weswigham
Copy link
Member

weswigham commented Mar 28, 2019

TypeScript Version: 3.4.0-dev.201xxxxx

Code

declare let x: Partial<HTMLElement>;

function f<T>(y: T) {
    x = y;
}

f({ innerHtml: Symbol() }); // this is blatantly incompatible with `HTMLElement`

if (x.innerHTML) {
    x.innerHTML.toLowerCase(); // We just set it to a `symbol` and not a `string`, this will error at runtime
}

Expected behavior:
An error on x = y.

Actual behavior:
No error.

Playground Link

The root cause is this relationship in structuredTypeRelatedTo added way back in this:

if (relation !== subtypeRelation && isPartialMappedType(target) && isEmptyObjectType(source)) {
    return Ternary.True;
}

This is unsound when source is the empty type resulting from the constraint of an unconstrained generic type (which, if changed to unknown, catches this issue!). It's unsound when it comes from a constraint at all, actually. Fixing this will break react-redux, whose recursive Shared type (which we have in our test suite) actually only checks because of this unsoundness.

@RyanCavanaugh
Copy link
Member

Not a regression

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Mar 28, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Mar 28, 2019
@weswigham
Copy link
Member Author

As I said in the OP, the change that introduced the bug was #26517 from what I can tell, so we've had it long enough that we have people unintentionally relying on it. react-redux's very terrible no good Shared type:

export type Shared<
    InjectedProps,
    DecorationTargetProps extends Shared<InjectedProps, DecorationTargetProps>
    > = {
        [P in Extract<keyof InjectedProps, keyof DecorationTargetProps>]?: InjectedProps[P] extends DecorationTargetProps[P] ? DecorationTargetProps[P] : never;
    };

only actually typechecks in most cases because Shared is partial and most uses involve unconstrained type parameters. "no constraint" is defaulted to the empty object, which is then assignable to any partial type... like Shared. This is the only thing allowing most uses of this highly suspect pattern to pass the typechecker. Specifically this one is core - GetProps<C> is unconstrained - we know absolutely nothing about it (it's an infer type parameter or never), so how could is possibly satisfy the Shared<InjectedProps, DecorationTargetProps> constraint? It shouldn't be able to, yet this bug is why it does~

weswigham added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Mar 29, 2019
weswigham added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Mar 29, 2019
weswigham added a commit to weswigham/react-dnd that referenced this issue Mar 30, 2019
Fixing microsoft/TypeScript#30634 will make this constraint fail to check for most users - this type seems to have been copied from (to?) `react-redux`, which has taken in a similar change on DT:

DefinitelyTyped/DefinitelyTyped#34335
DefinitelyTyped/DefinitelyTyped#34339
darthtrevino pushed a commit to react-dnd/react-dnd that referenced this issue Mar 31, 2019
Fixing microsoft/TypeScript#30634 will make this constraint fail to check for most users - this type seems to have been copied from (to?) `react-redux`, which has taken in a similar change on DT:

DefinitelyTyped/DefinitelyTyped#34335
DefinitelyTyped/DefinitelyTyped#34339
alesn pushed a commit to alesn/DefinitelyTyped that referenced this issue Apr 23, 2019
darthtrevino pushed a commit to react-dnd/react-dnd that referenced this issue Feb 3, 2022
Fixing microsoft/TypeScript#30634 will make this constraint fail to check for most users - this type seems to have been copied from (to?) `react-redux`, which has taken in a similar change on DT:

DefinitelyTyped/DefinitelyTyped#34335
DefinitelyTyped/DefinitelyTyped#34339
@jakebailey
Copy link
Member

This one was fixed in #49119, explicitly via the first bullet in the PR description:

An unconstrained type parameter is no longer assignable to {}.

But, could probably use an explicit test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants