TypeScript 5.4: NonNullable does not always exclude undefined

[eps1lon]

🔎 Search Terms

NonNullable undefined

🕗 Version & Regression Information

• This changed between versions 5.3 and 5.4.0-dev.20231202

⏯ Playground Link

https://www.typescriptlang.org/play?strict=false&strictPropertyInitialization=false&strictBindCallApply=false&noImplicitReturns=false&ts=5.4.0-dev.20231202#code/KYDwDg9gTgLgBASwHY2FAZgQwMbDgYQgFsiIkB1KTMMNAHgAU4BeOAbwF8A+dgKDkRIAbhADWwOvwHSBAaTihUSACYBnOADkyGgK4AbPZgBGeiVJkW+l69IDa85HHEBPCOjgMAugC4P9zwogSmpwABShAHRRmFAA5r6YSM62ngCULDwiCMrpAD5wOirA6MjAynAA-HDyvkjAQmgA3OY2cBy2Lm4eni0yXAA0LVyhvdLIIuIMUBBgGphEwL6ygzKpvgz+zRy8vMioGDh4APJgMAhkmHoAYoXYZ2RTM1bS6Lf3SI9gFaFrcFnKWx2ymA2EMUDw2DIqngAHcqDQ0L5CCQyJRqLQoHQTu9LjckHdzh9pmAuDs4ei0BFxmJgKEAESvfHvT501K8ckIqBU4Q00KFYElOo5IA

💻 Code

export interface CommonWrapper<P = {}> {
  invoke<
        K extends NonNullable<
            {
                [K in keyof P]: P[K] extends ((...arg: any[]) => void) | undefined ? K : never;
            }[keyof P]
        >,
    >(
        invokePropName: K,
    ): P[K];
}

interface OptionalFunctionProp {
    functionProp?(): void;
}

declare const wrapper: CommonWrapper<OptionalFunctionProp>

wrapper.invoke("functionProp")
wrapper.invoke(undefined)

🙁 Actual behavior

No error

🙂 Expected behavior

Argument of type 'undefined' is not assignable to parameter of type '"functionProp"'.(2345)

Additional information about the issue

Breaks @types/enzyme tests: https://github.com/DefinitelyTyped/DefinitelyTyped/actions/runs/7057196205/job/19245682552?pr=67570

[Andarist]

Bisects to #56515

[Andarist]

The problem is that this PR implement this logic:

In a union containing intersections of a type variable and primitive types that fully cover the constraint of that type variable, the intersections are replaced with just the type variable. For example, given T extends "a" | "b", the union type T & "a" | T & "b" is reduced to just T.

It got implemented here:
https://github.com/microsoft/TypeScript/pull/56515/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R17407-R17414

But now... to do its thing it operates on the getBaseConstraintOfType(typeVariable). This typeVariable is { [K in keyof P]: P[K] extends ((...arg: any[]) => void) | undefined ? K : never; }[keyof P]. It gets simplified (by getSimplifiedType) to P[keyof P] extends ((...arg: any[]) => void) | undefined ? keyof P : never through the substituteIndexedMappedType.

The constraint of this conditional type is just keyof P and its base constraint is string | number | symbol (aka PropertyKey). So based on that, the implemented "elimination" logic kicks in and completely eliminates NonNullable from the original type.

The problem is that getBaseConstraintOfType doesn't take into account that P is a homomorphic type variable here. A mapped type over such type variable preserves all the modifiers. So accidentally-ish the bare call to substituteIndexedMappedType there just eliminates the whole possibility that this indexed access might return undefined at the instantiation time if the optional properties are involved.

A potential fix for this would be to include | undefined in that base constraint. I have no idea how breaking that would be for the code out there though. I might try to put up an experimental PR for this.

[Andarist]

Oh, I should have read the latest comments below this PR. It seems that @ahejlsberg already commented on this particular issue here:

The issue here really is that the constraint of Keys

should include undefined--because the type might be applied to something with optional properties. I tried implementing that, but it causes a whole slew of new errors that technically are correct, but break existing code. So, I think the cure is worse than the disease there.

So it seems that the current conclusion is that fixing this would be too breaking and that the type in enzyme should be adjusted to accommodate this recent change in TS.

[Ayc0]

I also noticed a regression with the 5.3 -> 5.4 migration in this playground

export type NonNullableKeys<T, U> = NonNullable<
    {
        [K in keyof T]: [U] extends [T[K]]
            ? T[K] extends U
                ? K
                : never
            : never;
    }[keyof T]
>;

export type Foo = {
    foo?: string;
};


type Hello = NonNullableKeys<Foo, string | undefined>
//   ^?
// type Hello = "foo" in TS 5.3.3
// type Hello = "foo" | undefined in TS 5.4.2

[Ayc0]

If this is a regression that we don't want to fix (for technical reason), should the breaking change be mentioned in the release note?

[Ayc0]

The thing that is weird is that if NonNullable is used in the definition of the type vs in the usage, it has a different behavior:

type Bar = NonNullableKeys<Foo, string | undefined>
//   ^?
// type Bar = "foo" in TS 5.3.3
// type Bar = "foo" | undefined in TS 5.4.2

type Paz = NonNullable<Bar>
//  ^?
// type Paz = "foo" in both

Playground

[Andarist]

This is the same issue - so I'd say that it works as intended as per @ahejlsberg's resolution.

The thing that is weird is that if NonNullable is used in the definition of the type vs in the usage, it has a different behavior:

This is because NonNullable gets eliminated here completely from ur NonNullableKeys - so since it's not there it also doesn't do its job.

[shicks]

@Andarist It seems to me that this issue has been closed prematurely. The underlying issue is that there's an expectation that NonNullable should behave consistently and it no longer does. If fixing & {} isn't feasible, then maybe we need to change the definition of NonNullable to something that won't be affected by this issue (i.e. Exclude<T, null|undefined> or whatever)?

[Andarist]

The current definition of NonNullable is way better for the compiler than the conditional type like Exclude. This was intentionally changed in 4.8: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-8.html#improved-intersection-reduction-union-compatibility-and-narrowing

You could try to spin up a new discussion thread about it but so far this new behavior has been classified as a design limitation more than once (IIRC)

[shicks]

I get that this has been called a design limitation, and I'm all in favor of a performant NonNullable, but as it currently stands this is a pretty huge foot-gun. If I'm a non-expert developer trying to write any sort of FilteredKeys<T> type transformer, I'm likely to try something like this:

type FilteredKeys<T> = {[K in keyof T]: SomePredicate<K, T[K]> extends true ? K : never}[keyof T];

When I see (with some surprise) that this type includes undefined in some circumstances, the first thing I'm going to try is to slap a NonNullable around it. It's going to be pretty frustrating when that doesn't actually remove undefined - how in the world am I supposed to get rid of this thing? I recognize that there's a workaround via -?, but it's very non-discoverable.

I can't imagine this is something a linter could detect? I just think there needs to be something to prevent this sort of frustration that I anticipate will recur pretty often.

[Andarist]

I dont disagree that this is a footgun - im not a fan of this. I dont call any shots here though. That’s why i've proposed creating a new discussion about it