Skip to content

null gets accidentally eliminated when narrowing by undefined's equality #57693

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

Closed
Andarist opened this issue Mar 8, 2024 · 8 comments Β· Fixed by #57724
Closed

null gets accidentally eliminated when narrowing by undefined's equality #57693

Andarist opened this issue Mar 8, 2024 · 8 comments Β· Fixed by #57724
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@Andarist
Copy link
Contributor

Andarist commented Mar 8, 2024

πŸ”Ž Search Terms

null undefined narrow narrowing reduction equality narrowby

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried*

*it worked differently in 5.1-5.3 because those had a bug in them ( #57202 )

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.5.0-dev.20240308#code/C4TwDgpgBAggdiA8gIwFYQMbCgXigJUwHsAnAEwB4BnYEgSzgHMAaKAQwQD4BuAKFEhQAysDbBoeeEjSZgfXmUwAbNiWgAzAK5wsdInCgALNlUQB3OAAUSRSCVAUAKlAgAPcXDJVYCFOiycABS8UFBEMlgAXFCOzCFQYDZg0da2EPYgANIQIHEAlNGJtlB03gDWOUTqMfIM4iTqbBjQIqQQFEK4wqLinFAA3vFUEMAiYu2ZLu4QnuWV1UJBFSDRmawAbmxKmhDRQgDamQC6BVDrRHRkfAC+vLxuYKTYWjrAegZUIDqtaqlgFPFOm4PF5uuM4qFLFMQd5LKo3lsOpwIVBJsCZqDllVhHEgjQ2ntgG0kawimAqClWMtVnkBvEMPoaGctjsusZTBY-ulQIEyVQqTlaQB+BJJKj7ZZHKDRbSKdQMCBXO6hOjVQKbbYSHB4WUQeVwRW0tTATQkOB8UIAektUGAhlKLhINhIVCGRLUADphqMehBAssNiyIHl5FabXaHVRDERNEoyHAAOTYNjqdSyW2GaACaAcEDRsysMwQBPrHNKNRsMggKAYTMYCpkdiMNgMJl26BUNgAWyz9qY8VVUHVQdw2qguv1hqgxtN5vi1qgVE0ZEUcGASmrdGwS4wzUVrtC+M93rG4n9OUDmpDvFuQA

πŸ’» Code

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 === undefined) return;
  // this errors
  store.setState(key, value);

  // this shouldn't affect the type anyhow, we've already checked against the same thing
  if (value === undefined) return;
  // suddently it succeeds
  store.setState(key, value);
}

πŸ™ Actual behavior

one of the calls errorrs while the other one doesn't

πŸ™‚ Expected behavior

i'd expect consistent behavior at both call sites

Additional information about the issue

originally reported by @diegohaz here: https://twitter.com/diegohaz/status/1765674095293747628

@ahejlsberg
Copy link
Member

Simpler repro:

function f1<T extends Record<string, any>, K extends keyof T>(x: T[K] | undefined) {
    if (x === undefined) return;
    x;  // T[K] & ({} | null)
    if (x === undefined) return;
    x;  // T[K] & {}
}

@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Mar 8, 2024
@ahejlsberg
Copy link
Member

In addition to the odd narrowing behavior demonstrated by the OP and the simplified repro above, we also have inconsistencies in type relationships involving intersections that strip undefined, null, or both:

function f2<T, K extends keyof T>(t: T[K], p1: Partial<T>[K] & {}, p2: Partial<T>[K] & ({} | null)) {
    t = p1;
    t = p2;  // Unexpected error
}

The issue here is missing normalization of intersections in certain cases. Specifically, we only normalize intersections with {}, not intersections with null or undefined.

@Andarist
Copy link
Contributor Author

@ahejlsberg that feels like an answer to this comment, right? I mean, this extra observed problem would fix what has been described there.

Do you plan to work on fixing this? Or should I try to extend my PR with a fix for this?

@ahejlsberg
Copy link
Member

Furthermore, using Record<string, any> as a constraint exhibits some odd behavior:

function f3<T, P extends Record<string, any>>(t: T[keyof T], p1: P[keyof P], p2: P[keyof P] & {}) {
    t = p1;  // Error as expected
    t = p2;  // Missing error
}

function f4<T, P extends Record<string, unknown>>(t: T[keyof T], p1: P[keyof P], p2: P[keyof P] & {}) {
    t = p1;  // Error as expected
    t = p2;  // Error as expected
}

This has to do with the constraint of P[keyof P] being any which effectively circumvents type checking in some cases. In general, it seems suspicious for any instantiable type to have a constraint of any, and we do indeed ignore such constraints in places. Just not everywhere.

@ahejlsberg
Copy link
Member

@ahejlsberg that feels like an answer to #57692 (comment), right? I mean, this extra observed problem would fix what has been described there.

Given type parameters S and K extends keyof S, I would expect Partial<S>[K] & ({} | null) to be assignable to S[K]. However, when P extends Partial<S>, I think expecting P[K] & ({} | null) to be assignable to S[K] is getting speculative.

Do you plan to work on fixing this? Or should I try to extend my PR with a fix for this?

I will be putting up a PR that fixes the issues demonstrated by f1 and f2 above.

@Andarist
Copy link
Contributor Author

Andarist commented Mar 11, 2024

However, when P extends Partial<S>, I think expecting P[K] & ({} | null) to be assignable to S[K] is getting speculative.

I can see that but it also means that the P[K] & {} being assignable to S[K] (something that is allowed today) is already speculative too, right?

@ahejlsberg
Copy link
Member

I can see that but it also means that the P[K] & {} being assignable to S[K] (something that is allowed today) is already speculative too, right?

Well, it's allowed only because of the oddity of Record<string, any> constraints (see my comment).

@Andarist
Copy link
Contributor Author

It has been called an oddity but I'm not sure if I should interpret this as "odd but working as intended" or "odd and it could be fixed" πŸ˜… In general, I interpret your comments about this here as the latter but I'm not 100% sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
4 participants