microsoft / TypeScript

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

flatMap errors when callback returns certain union types #31033

Open noinkling opened 5 years ago

noinkling commented 5 years ago

TypeScript Version: 3.4.3

Search Terms: flat, flatmap

Code

// Map<string, number[]>
const myMap = new Map([
  ['foo', [1, 2, 3]],
  ['bar', [4, 5, 6]]
]);

// string[]
const myArray = ['foo', 'bar', 'baz'];

// (key: any) => number[] | undefined
const mapFn = key => myMap.get(key);

const flatMapped = myArray.flatMap(mapFn);

Actual behavior:

With strict null checks on:

Argument of type '(key: any) => number[] | undefined' is not assignable to parameter of type '(this: undefined, value: string, index: number, array: string[]) => number | readonly number[]'.
  Type 'number[] | undefined' is not assignable to type 'number | readonly number[]'.
    Type 'undefined' is not assignable to type 'number | readonly number[]'. ts(2345)

Turning strict null checks off prevents the error as you might expect, but that's obviously not desirable.

More generally, any callback that returns foo[] | bar (where bar is not compatible with foo) seems to run into the same issue. The Map.prototype.get example I've used here just seems like it would be a relatively common use case in particular (you'd typically follow it up by calling .filter(Boolean) or similar on the mapped result).

More code

Seeing if doing the operations separately will help:

// (number[] | undefined)[]
const mapped = myArray.map(mapFn);

// any[] !!
const flattened = mapped.flat();

Expected behavior:

flatMapped/flattened result to be inferred as (number | undefined)[].

Playground Link:

Doesn't seem to be able to load the appropriate library definition, even with /// <reference lib="es2019.array" />

Related Issues:

29604 seems to be related to the .flat() part.

noinkling commented 5 years ago

After playing around with the definition, this change using conditional types seems to make flatMap behave how I want:

 flatMap<U, This = undefined> (
-    callback: (this: This, value: T, index: number, array: T[]) => U|ReadonlyArray<U>,
+    callback: (this: This, value: T, index: number, array: T[]) => U,
     thisArg?: This
-): U[]
+): (U extends ReadonlyArray<infer V> ? V : U)[]
texastoland commented 5 years ago

It's not a bug. The signature correctly models the standard library behavior. flatMap expects a function that returns a type => U | readonly U[] (some type or an array of that type). Your function returns => undefined | number[]. undefined doesn't match with number. You need to either provide a default value (=> myMap.get(key) || [] which works like .filter(Boolean)) or explicitly ignore the null check (=> myMap.get(key)!).

noinkling commented 5 years ago

The signature correctly models the standard library behavior.

It models a certain statically-typed interpretation of the behaviour, but there's nothing in the ES spec that prevents it. All flat/flatMap cares about is whether each element is an array (in which case it unwraps it), or not (in which case it leaves it alone), it doesn't say that the elements in the arrays being unwrapped must be the same type as the non-wrapped elements at the top level.

Here's another, more contrived, example that currently errors in TS (but works fine in JS):

[1, ['foo'], 2].flatMap(item => item)
texastoland commented 5 years ago

Correct. The callback could return (?U)[] and the function ?U|U[], or they could both be any, or you could disable strict. The problem is that contrived examples would hurt type inference for well-reasoned code. Then TypeScript would be less useful in general.

you'd typically follow it up by calling .filter(Boolean)

=> myMap.get(key) || [] does this in a single pass without introducing unwanted undefineds. It's a general pattern that works for any type (monad) with that method (also called chain). There's a super helpful #typescript channel on the FP Slack

noinkling commented 5 years ago

The problem is that contrived examples would hurt type inference for well-reasoned code.

Can you show an example using my solution above where that would be the case? If there is value in being this inflexible I'd like to know where it lies.

Even so, I had thought TypeScript was more descriptive than prescriptive.

=> myMap.get(key) || [] does this in a single pass without introducing unwanted undefineds.

Right, I'm not discounting that pattern, it's just that there is also a more general case here.

texastoland commented 5 years ago

I started to write an answer, but the bug tracker seems like an inappropriate forum. It works as designed. I encourage you to ask on Stack or Slack. You can @ me on the previously mentioned Slack channel.

RyanCavanaugh commented 5 years ago

It models a certain statically-typed interpretation of the behaviour, but there's nothing in the ES spec that prevents it.

The ES spec prevents almost nothing; see https://stackoverflow.com/questions/41750390/what-does-all-legal-javascript-is-legal-typescript-mean

I think the normal use case for flatMap is producing a homogeneous array and the current definition reflects that. To that point, absent other information, the example you posted looks like it has a bug, because probably the author meant to filter out unfound values rather than dropping undefineds into the resulting array. Writing flatMap<number | undefined>( to indicate your intent seems like an increase in clarity.

noinkling commented 5 years ago

Writing flatMap<number | undefined>( to indicate your intent seems like an increase in clarity.

I agree - thanks for the tip, I didn't realize that would work.

I still think there probably exists other "valid" use cases for a heterogeneously mapped array but admit I can't think of anything particularly compelling right now, and such cases could just do the same as above.

However it does seem a little inconsistent then that plain map has no problem with any union type given your position here. Many people's mental model for flatMap is that it is simply map with the added option of producing multiple elements for a given input element. More to the point, if the normal use case for flatMap is producing a homogeneous array, then surely that applies to map (and I guess by extension, non-tuple arrays in general) too? Is that discrepancy just due to being a technical simplicity/elegance thing?