Skip to content

Narrowing does not work when done via a predicate that narrows via mapped types #47283

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
devanshj opened this issue Jan 1, 2022 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@devanshj
Copy link

devanshj commented Jan 1, 2022

Bug Report

πŸ”Ž Search Terms

Type predicate, mapped types, narrowing

πŸ•— Version & Regression Information

Tried with v4.3.5

⏯ Playground Link

Playground link

πŸ’» Code

declare const areValuesDefined:
  <T>(t: T) => t is { [K in keyof T]: Exclude<T[K], undefined> }

let x = {} as { a: string | undefined } | { b: string }
if (areValuesDefined(x)) {
  x;
  // Expected: { a: string } | { b: string }
  // Actual: { b: string }
}

πŸ™ Actual behavior

The type of x, inside the if block, is { b: string }

πŸ™‚ Expected behavior

The type of x, inside the if block, should have been { a: string } | { b: string }.

Also the quickinfo of areValueDefined is inconsistent with the what actually the narrowed type is. The quickinfo shows the expected type.

Also the narrowing { b: string } is particularly weird, even if it somehow failed it should have kept the type as it is.

And the narrowing does work in this case...

declare const areValuesDefined:
  <T>(t: T) => t is { [K in keyof T]: Exclude<T[K], undefined> }

let x = {} as { a: string | undefined } | { b: string | undefined }
if (areValuesDefined(x)) {
  x;
  // Expected: { a: string } | { b: string }
  // Actual: { a: string } | { b: string }
}
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jan 3, 2022

Narrowing a union through a UDTG goes through this algorithm (roughly; this is from memory/experimentally-obtained):

  • Compute the type that's narrowed to (this step works, which you can verify by changing t is { ... to { ...
  • See if the narrowed-to type is a subtype of at least one constituent of the union
  • If yes, narrow to those constituents(s) of the union
  • Otherwise, intersect the narrowed-type type with each member of the union

I will name this behavior "filter-first"

Not using this algorithm produces counterintuitive and unhelpful results

type Cat = { meow: any };
type Dog = { woof: any };
type Chipped = { id: number };

declare function isCat(c: any): c is Cat;
declare let cd: Cat | Dog;
if (isCat(cd)) {
  cd;
// ^?
// Desired & actual cd: Cat
// Without filter-first: (Cat) | (Cat & Dog)
}
declare function isChipped(c: any): c is Chipped;
if (isChipped(cd)) {
  // Desired and actual cd: (Cat | Dog) & Chipped
  // Without filter-first: never
}

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jan 3, 2022
@devanshj
Copy link
Author

devanshj commented Jan 5, 2022

I get the need for pragmatism but I think this approach is a little sub-optimal. Instead of "filter-first"ing what we could do is intersect the type to be narrowed with the narrowed type and then pragmatically make Dog & Cat as never as there is no overlap so pragmatically the intersection truly is never. And I'm not sure why in the second chunk you say without filter-first it'd be never but with my suggestion it'd be the desired type.

(Cat | Dog) & Cat = (Cat & Cat) | (Dog & Cat) = Cat | never = Cat // desired result
(Cat | Dog) & Chipped // desired result

But maybe this will bring up other problems idk haha.

Actually I didn't have this exact issue posted, my issue was this failing test but turns out my NotAwareIntesect<A, B> was buggy which I fixed and now the test is passing.

So basically I'm fine with the current filter-filter approach haha, but I still think it might be a little sub-optimal.

@devanshj devanshj closed this as completed Jan 5, 2022
@RyanCavanaugh
Copy link
Member

then pragmatically make Dog & Cat as never as there is no overlap so pragmatically the intersection truly is never.

Intersection cannot produce never unless the set is actually provably empty, which it's not. This happens and matters.

@devanshj
Copy link
Author

devanshj commented Jan 6, 2022

Intersection cannot produce never unless the set is actually provably empty, which it's not.

Yes I'm aware hence "pragmatically" produce never only for UDTP, not everywhere ofc that would be ridiculous.

Also I realized making intersection of non-overlapping types never won't work because then (Cat | Dog) & Chipped also would become never. So we'd do this only when one of the narrowed constituents is a subtype of one of the to-narrow constituents. This similar to first-filter except the OP issue also gets fixed. Let me show you a demo...

// https://tsplay.dev/WG6x2m

type Test0 = DevanshNarrow<Cat | Dog, Cat>
// Devansh's narrow: Cat
// TypeScript's narrow: Cat

type Test1 = DevanshNarrow<Cat | Dog, Chipped>
// Devansh's narrow: (Cat | Dog) & Chipped
// TypeScript's narrow: (Cat | Dog) & Chipped

type Test2 = DevanshNarrow<
  { a: string | undefined } | { b: string },
  { a: string } | { b: string }
>
// Devansh's narrow:
// | ({ a: string | undefined } & { a: string })
// | ({ b: string } & { b: string })
//
// TypeScript's narrow:
// { b: string }

type DevanshNarrow
  < T
  , N
  , ShouldBePragmatic =
      ( N extends unknown
          ? N extends T ? true : false
          : never
      ) extends false
        ? false
        : true
  > = 
    ShouldBePragmatic extends true
      ? T extends unknown
          ? N extends unknown
              ? keyof T & keyof N extends never
                  ? never
                  : T & N
              : never
          : never
      : T & N

interface Cat { meow: any }
interface Dog { woof: any }
interface Chipped { id: number }

Do you have cases where this algorithm would produce undesired results?

I hope you see that this algorithm isn't arbitrary, narrowing by definition is intersecting narrowed with to-narrow and hence (Cat | Dog) & Cat is correct but TypeScript does some magic to make it Cat which is good magic, but what I'm saying is we can do that magic in a way that doesn't mess other cases like OP (ie Test2).

@devanshj
Copy link
Author

devanshj commented Jan 6, 2022

Also, it's important to note that the { b: string } narrowing, if not wrong, at least leads to runtime errors...

const foo = (x: { a: string | undefined } | { b: string }) => {
  if (areValuesDefined(x)) {
    console.log(x.b.toUpperCase())
  }
}

const areValuesDefined = <T>(x: T): x is { [K in keyof T]: Exclude<T[K], undefined> } =>
  Object.values(x).every(x => x !== undefined)

foo({ a: "hello" }) // TypeError: Cannot read properties of undefined (reading 'toUpperCase')

With my narrowing algorithm this would have not compiled.

@devanshj
Copy link
Author

devanshj commented Jan 8, 2022

@RyanCavanaugh should I open a new issue with feature request template if I want this to be considered or discussed? (I'm looking for cases where my algorithm would produce undesired results)

@RyanCavanaugh
Copy link
Member

@devanshj yes, new issue please. I'm not quite clear on what you're proposing.

@devanshj
Copy link
Author

Done #47389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

2 participants