Skip to content
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

Type guards in Array.prototype.filter #7657

Closed
calebegg opened this issue Mar 23, 2016 · 39 comments
Closed

Type guards in Array.prototype.filter #7657

calebegg opened this issue Mar 23, 2016 · 39 comments
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@calebegg
Copy link

TypeScript Version:

nightly (1.9.0-dev.20160323)

Code

let numbers: number[] =
    [1, '2', 3, '4'].filter(x => typeof x !== 'string');

declare function isNumber(arg: any): arg is number;
let numbers2: number[] = [1, '2', 3, '4'].filter(isNumber);

Playground

Expected behavior:
.filter with a type guard returns an array of the specified type. This would be especially useful with --strictNullChecks, so we could do something like foo.map(maybeReturnsNull).filter(x => x != null)....

Actual behavior:
.filter returns an array of the same type, and the two lets above are errors.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. In Discussion Not yet reached consensus Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Mar 23, 2016
@RyanCavanaugh
Copy link
Member

The required overload is

interface Array<T> {
    filter<U extends T>(pred: (a: T) => a is U): U[];
}

(which you can add to your codebase today to make this work)

@calebegg
Copy link
Author

Wow, I didn't realize it would be that easy. It doesn't fix the first case (presumably because the function is not getting recognized as a type guard function), but it does work with a cast:

let numbers: number[] =
    [1, '2', 3, '4']
        .filter(<(x) => x is number>(x => typeof x == 'number'));

Would inferring that x => typeof x == 'number' is a type guard be a good candidate for a separate suggestion? Or is it too specific? (I didn't see anything in the tracker already).

@DanielRosenwasser
Copy link
Member

The problem I ran into with adding the overload into lib.d.ts was that it doesn't work with Array.isArray, and I don't entirely recall why. We could investigate this a bit.

@NoelAbrahams
Copy link

Looks to be the same as #2835

@RyanCavanaugh
Copy link
Member

(context: #2835 was long before we had type guards)

@bcherny
Copy link

bcherny commented Mar 30, 2016

if someone can patch this to lib.d.ts, i could get rid of so many asserts in my code.

@loilo
Copy link

loilo commented Jun 8, 2016

Is there some place to read up about the "is" in @RyanCavanaugh's overloading code above? Couldn't find it anywhere, neither in this issue about reserved keywords nor on the pages linked from there.
Also Google wasn't helpful this time.

@RyanCavanaugh
Copy link
Member

@loilo see #1007

@loilo
Copy link

loilo commented Jun 8, 2016

Thank you!

EDIT: Okay, my bad - I've mistaken this comment in the "is-keyword" issue for a suggestion rather than an existing solution.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2016

Documentation for user defined type guards is available in the language handbook at: http://www.typescriptlang.org/docs/handbook/advanced-types.html#user-defined-type-guards

@loilo
Copy link

loilo commented Jun 9, 2016

Yes, now that I know the name of the feature I was already able to find information about it. Thanks anyway. :)

@PanayotCankov
Copy link

Wow 👍 to add @RyanCavanaugh 's suggestion in the lib.d.ts. I was having something like:

interface Node {
    astType: "node" | "rule";
}
interface Rule extends Node {
    ruleSomething: string;
}
function isRule(node: Node): node is Rule {
    return node.astType === "rule";
}
let nodes: Node[] = [];
nodes.filter(isRule).forEach(rule => console.log(rule.ruleSomething));

It is magical. ❤️

@jesseschalken
Copy link
Contributor

@calebegg You can just add :x is number to the signature rather than using a cast:

let numbers: number[] =
    [1, '2', 3, '4']
        .filter((x):x is number => typeof x == 'number');

In principle this would allow the compiler to check that x is number does in fact follow from typeof x == 'number', but I don't think it bothers checking that (TS 1.8).

@bcherny
Copy link

bcherny commented Sep 6, 2016

A few more cases (using TS2.0):

// setup
class A { type = 'A' }
class B { type = 'B' }
type C = A | B
function isA(_): _ is A { return _.type === 'A' }
const cs: C[] = [new A(), new B(), new B()]

// (1) - OK
let as = cs.filter(isA)

// (2) - Not OK - type is still C[]
let as = cs.filter(_ => isA(_))

// (3) - Not OK - type is still C[]
let bs = cs.filter(_ => !isA(_))

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Sep 6, 2016
@mhegazy mhegazy modified the milestones: Community, TypeScript 2.1 Sep 21, 2016
@OliverJAsh
Copy link
Contributor

Does this work with discriminated unions? My understanding is that, with @RyanCavanaugh's overload, you still need user defined type guards. E.g:

    const numbers: number[] = [1, '2', 3, '4']
        .filter((x):x is number => typeof x == 'number')
        .map(y => y)

Surely with TypeScript 2's discriminated union types, we can drop the type guard here somehow? I'm not sure how, though!

    const numbers: number[] = [1, '2', 3, '4']
        .filter(x => typeof x == 'number')
        .map(y => y)

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Oct 5, 2016

@RyanCavanaugh I'm also curious how this will help when you're filtering on properties, e.g.:

// https://github.com/Microsoft/TypeScript/issues/8123
// Will be fixed in TS 2.1
declare global {
    interface Array<T> {
        filter<U extends T>(pred: (a: T) => a is U): U[];
    }
}

const numbers: number[] = [1, '1']
    .filter((x): x is number => typeof x == 'number')
    .map(y => y) // y is number, good!

const numbers2: number[] = [{ foo: 1 }, { foo: '1' }]
    .filter(x => typeof x.foo == 'number')
    .map(y => y.foo) // y.foo is number | string, not good!

@Arnavion
Copy link
Contributor

Arnavion commented Oct 5, 2016

.filter(x => typeof x.foo == 'number') fails for the same reason that .filter(x => typeof x == 'number') would fail - missing return type on the predicate means it's inferred as returning Boolean and not a type guard.

You want .filter((x): x is { foo: number } => typeof x.foo == 'number')

@OliverJAsh
Copy link
Contributor

Thank you!

Any ideas where discriminated unions will/can help here, or do we have to provide user defined type guards?

@rob3c
Copy link

rob3c commented Nov 4, 2016

I'm trying to get something similar working in rxjs, and I'm not sure if it's failing because of a current TypeScript limitation, or if the rxjs type definition for filter just needs updating. Here's the rxjs definition as of [email protected]:

// definition 1
export declare function filter<T>(
    this: Observable<T>, 
    predicate: (value: T, index: number) => boolean, 
    thisArg?: any
): Observable<T>;

// definition 2
export declare function filter<T, S extends T>(
    this: Observable<T>, 
    predicate: (value: T, index: number) => value is S, 
    thisArg?: any
): Observable<S>;

Definition 2 looks like it's trying to implement the U extends T strategy mentioned above for Array<T>, but it doesn't seem to work due to a clash with definition 1.

Here's a contrived example of using a type guard with filter in addition to a regular boolean expression, but the two clashing definitions prevent both from working correctly (see comments):

interface Action { type: string; }
class FooAction implements Action { type = 'FOO'; name = 'Bob'; }

function isFooAction(action: Action): action is FooAction {
    return action && action.type === 'FOO';
}

var bob$: Observable<Action> = someActionObservable
    .filter(isFooAction) // I'd like this to cause map to see a FooAction instead of just Action below
    .map(foo => foo.name) // foo is FooAction with definition 1 commented-out, Action otherwise
    .filter(foo => foo.name === 'Bob'); // fails to compile without definition 1

Is there a magic type definition tweak I can make to avoid cluttering the method chain with unnecessary casts? It seems like there should be some way to flow the type info through method chains like this without a conflict.

UPDATE: Oops - the map and second filter calls aren't meant to be chained together, as it looks like in my example. One or the other is commented-out, depending on which definition you're testing.

UPDATE 2: Nevermind, I found a solution that only requires updating the type definitions. I'm preparing a pull request for rxjs now.

@josundt
Copy link

josundt commented Nov 13, 2016

I have tested this with TS 2.1.1.
While ReadOnlyArray.filter with type guard functions work as expected, it seems it does not work with Array.filter.

const roArr: ReadonlyArray<string | null> = ["foo", null];
const fRoArr = roArr.filter((i): i is string => typeof i === "string"); 
// -> fRoArr inferred as string[]

const arr: Array<string | null> = ["foo", null];
const fArr = arr.filter((i): i is string => typeof i === "string");
// -> fArr inferred as (string | null)[]

@Arnavion
Copy link
Contributor

@josundt See #10916

@OliverJAsh
Copy link
Contributor

How can you use type predicates to negate a type? For example:

[1, 'foo', true].filter((x): x is not number => typeof x !== 'number')

@janslow
Copy link

janslow commented Nov 25, 2016

Not ideal, but this works for primitives. Although, it might be easier to leave that unguarded.

type NotNumber = Object | string | symbol | undefined | null | boolean | Function;
function notNumber(x: any): x is NotNumber {
    return typeof x !== 'number';
}

@bcherny
Copy link

bcherny commented Dec 7, 2016

Any ideas where discriminated unions will/can help here, or do we have to provide user defined type guards?

@mhegazy @RyanCavanaugh Possible for one of you guys to chime in here? It's a lot of friction to have to (a) explicitly parametrize or (b) explicitly type guard every .filter call.

@gasi
Copy link

gasi commented Mar 8, 2017

I tried the type guard as follows with no luck:

type FooBar = Foo | Bar

interface Foo {
    type: 'Foo'
}

interface Bar {
    type: 'Bar'
}

const fooBars: Array<FooBar> = [
    { type: 'Foo' },
    { type: 'Bar' },
]

const foos: Array<Foo> = fooBars.filter(
    (item): item is Foo => item.type === 'Foo'
)

@mvestergaard
Copy link

mvestergaard commented Apr 20, 2017

Is this related to this problem? Make sure to enable strictNullChecks.

I would expect the control flow here to know that in the .map, the value is number and not undefined.
I find that this problem forces me to write much less readable code, because I have to jump through hoops to get around the type system tripping up.

If this is the same issue, is there any plans to make this scenario work in the near-ish future?

@maiermic
Copy link

This issue has been fixed by #16223 and can be closed. You can try it out in [email protected] for example.

@calebegg
Copy link
Author

@maiermic: Are you sure? I still get an error on this code:

['a', undefined].filter((e): e is string => !!e)[0].substring(0, 10);
test.ts(1,1): error TS2532: Object is possibly 'undefined'.

Playground (be sure to check "strictNullChecks" in options to see the error)

@maiermic
Copy link

@calebegg That looks like a bug to me. Your example works if you use a function declaration instead of the lambda:

['a', undefined].filter(isString)[0].substring(0, 10);

function isString(e): e is string {
    return !!e
}

@NaridaL
Copy link
Contributor

NaridaL commented Sep 18, 2017

@calebegg I opened #18562 ... in the meantime, specifying the type of e explicitly is a workaround:

['a', undefined].filter((e: any): e is string => !!e)[0].substring(0, 10);
                          ^^^^^

@calebegg
Copy link
Author

@NaridaL Interesting, thanks for identifying the issue!

@mhegazy mhegazy removed this from the Community milestone Apr 26, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests