Fix accidental null/undefined reduction when narrowing by null/undefined equality #57692
Conversation
typescript-bot
added
the
For Uncommitted Bug
| @@ -0,0 +1,47 @@ | |||
| narrowByUndefinedEqualityGeneric1.ts(21,23): error TS2345: Argument of type 'P[K] & ({} | null)' is not assignable to parameter of type 'S[K]'. | |||
There was a problem hiding this comment.
It's a separate issue but I'm somewhat confused by what happens here in terms of this error. Is this a separate bug? 🤔
It's especially confusing since this is OK:
type AnyObject = Record<string, any>;
type State = AnyObject;
declare function hasOwnProperty<T extends AnyObject>(
object: T,
prop: PropertyKey,
): prop is keyof T;
interface Store<S = State> {
setState<K extends keyof S>(key: K, value: S[K]): void;
}
export function syncStoreProp<
S extends State,
P extends Partial<S>,
K extends keyof S,
>(store: Store<S>, props: P, key: K) {
const value = hasOwnProperty(props, key) ? props[key] : undefined;
if (!value) return;
// this is ok
store.setState(key, value);
}The difference is in the narrowed type of value - in the test case added here we end up with: P[K] & ({} | null) while the example above ends up being NonNullable<P[K]> (so basically P[K] & {}.
So the only difference is the null bit but even with that it's essentially (P[K] & null) | (P[K] & {}). We can see here that each member of this union is of type P[K] (with undefined already excluded). We can also reason that each P[K] is assignable to S[K] - as long as P[K] is not undefined - but we have already excluded that possibility.
There is also a question here if P[K] should even be allowed here since this is a valid call:
syncStoreProp({} as Store<{ a: string; b: number }>, { a: "foo" }, "b");And we don't even need the hasOwnProperty check before creating this type. TS is OK with just this:
const value = props[key]And if that wouldn't be OK then the example above (the one that narrowed the type to P[K] & {}) shouldn't typecheck either, right?
typescript-bot
added
For Milestone Bug
| @@ -26969,9 +26969,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (strictNullChecks) { | |||
| switch (facts) { | |||
| case TypeFacts.NEUndefined: | |||
| return mapType(reduced, t => hasTypeFacts(t, TypeFacts.EQUndefined) ? getIntersectionType([t, hasTypeFacts(t, TypeFacts.EQNull) && !maybeTypeOfKind(reduced, TypeFlags.Null) ? getUnionType([emptyObjectType, nullType]) : emptyObjectType]) : t); | |||
| return mapType(reduced, t => hasTypeFacts(t, TypeFacts.EQUndefined) ? getIntersectionType([t, hasTypeFacts(t, TypeFacts.EQNull) && (!maybeTypeOfKind(reduced, TypeFlags.Null) || maybeTypeOfKind(t, TypeFlags.Null)) ? getUnionType([emptyObjectType, nullType]) : emptyObjectType]) : t); | |||
There was a problem hiding this comment.
If I understand correctly @ahejlsberg's comment here then this fix should be done elsewhere. While creating this I wasn't sure if "replacing" any with unknown would always be desirable when computing the constraint - IIRC that would fix this issue too. For reference, currently any is replaced by unknown in getConstraintFromTypeParameter here:
https://github.dev/microsoft/TypeScript/blob/3fca8c842a42bb43ba5a86dcce1b7fb0eae6c7a4/src/compiler/checker.ts#L15813-L15818
|
superseded by #57724 |
fixes #57693