microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.93k stars 12.47k forks source link

*explicit* type guard passed to Array.filter does not narrow type #54966

Open flying-sheep opened 1 year ago

flying-sheep commented 1 year ago

Bug Report

🔎 Search Terms

array.filter, type guard, explicit, is

🕗 Version & Regression Information

TS 3.9.7–5.1.6, target version ES2019 or later (releases earlier than 3.9.7 didn’t have Object.fromEntries)

⏯ Playground Link

Playground link with relevant code

💻 Code

interface Element { type: Elem['type'], children: Node_[] }
interface CodeBlock extends Element { type: 'cb', children: string[] }
interface Field extends Element { type: 'f', name: string }
interface FieldList extends Element { type: 'fl', children: Field[] }

type Elem = CodeBlock | Field | FieldList
type Node_ = Elem | string

function getMeta2(fieldLists: Elem) {
    return Object.fromEntries(
        fieldLists.children
            .filter((n: Node_): n is FieldList => typeof n !== 'string' && n.type === 'fl')
            .flatMap((fl) => fl.children)
            .map((f) => [f.name, f.children[0]?.toString()]),
    )
}

🙁 Actual behavior

> .flatMap((fl) => fl.children)
Property 'children' does not exist on type 'Node_'.
  Property 'children' does not exist on type 'string'.

🙂 Expected behavior

The return type of filter should be FieldList[]

nmain commented 1 year ago

Simplified repro:

declare var a: (3 | 4)[] | number[]
var b = a.filter((x): x is 4 => x === 4);
var c: 4[] = b;

The union type on a means that the right overload of filter isn't picked up. You can fix this by directly choosing that type; in your original example, that would be something like this:

function getMeta2(fieldLists: Elem) {
    var x: Node_[] = fieldLists.children;
    return Object.fromEntries(
        x
            .filter((n: Node_): n is FieldList => typeof n !== 'string' && n.type === 'fl')
            .flatMap((fl) => fl.children as Field[])
            .map((f) => [f.name, f.children[0]?.toString()]),
    )
}

I don't know if there's an actual bug here.

Maybe I’ll do it some other time if I find the time

You can just use the dropdown version select from the playground.

Andarist commented 1 year ago

As mentioned by @nmain - the right overload isn't picked up here. You can test out why it isn't picked up by creating a new method with just that overload (TS playground):

interface Element { type: Elem['type'], children: Node_[] }
interface CodeBlock extends Element { type: 'cb', children: string[] }
interface Field extends Element { type: 'f', name: string }
interface FieldList extends Element { type: 'fl', children: Field[] }

type Elem = CodeBlock | Field | FieldList
type Node_ = Elem | string

interface Array<T> {
    filter2<S extends T>(predicate: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[];
}

function getMeta2(fieldLists: Elem) {
    return Object.fromEntries(
        fieldLists.children
            // This expression is not callable.
            //   Each member of the union type '(<S extends Node_>(predicate: (value: Node_, index: number, array: Node_[]) => value is S, thisArg?: any) => S[]) | (<S extends string>(predicate: (value: string, index: number, array: string[]) => value is S, thisArg?: any) => S[]) | (<S extends Field>(predicate: (value: Field, index: number, array: Field[]) => val...' has signatures, but none of those signatures are compatible with each other.(2349)
            .filter2((n: Node_): n is FieldList => typeof n !== 'string' && n.type === 'fl')
            .flatMap((fl) => fl.children as Field[])
            .map((f) => [f.name, f.children[0]?.toString()]),
    )
}

There are some improvements coming in TS 5.2 that targets specifically cases like this (see this section of the announcement). We can try this out in the playground to see that TS 5.2 works with this filter2 (TS playground) but it still doesn't work if we switch back to the regular filter (TS playground).

It behaves like that because the new logic kicks in only if no matching signatures can be found. However, with filter, we are able to match the second overload "in the first pass" so it doesn't even try using this new logic in this case. This currently works by design - but I wonder if a case like this was considered when implementing this enhancement. cc @weswigham

flying-sheep commented 1 year ago

You can just use the dropdown version select from the playground.

done!

I don't know if there's an actual bug here.

Node_[] is identical to of string[] | Node_[] | Field[], no? every Field and every string is also a Node_. Shouldn’t this collapse?

MartinJohns commented 1 year ago

Node_[] is identical to of string[] | Node_[] | Field[], no?

It's (string | CodeBlock | Field | FieldList)[].

flying-sheep commented 1 year ago

doesn’t matter, my point is that if

const x: Node_[] = fieldLists.children
f(x)

is valid,

f(fieldLists.children)

should also be valid, no?

fatcerberus commented 1 year ago

You’ve heard of flying pigs 🐽… Now get ready for flying sheep![^1] 🐑

[^1]: I feel sorry for the guy who has to try to herd them

RyanCavanaugh commented 1 year ago

I'm not sure how to fix this without breaking an existing invariant, honestly. Touching anything around signature resolution tends to cause all kinds of problems.