Closed eps1lon closed 7 months ago
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.
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.
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
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?
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
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.
@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)?
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)
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.
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
π Search Terms
NonNullable undefined
π Version & Regression Information
β― 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
π 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