ENikS/LINQ

Enumerable.SelectMany declaration needs an overload without "result" parameter.

Closed this issue · 12 comments

Right now SelectMany is declared like this:

SelectMany<S, V>(selector?: (x: T, index: number) => Iterable<S>, result?: (x: T, s: S) => V): Enumerable<V>;

The second parameter, result, is optional, but it's also the only one that would allow for inferring V. As a result, when the second parameter is omitted, V cannot be inferred and ends up as {}. For example:

var xs = Enumerable.asEnumerable([1,2]).SelectMany( i => [i,i] );

The type of xs will be inferred as Enumerable<{}>.

To fix this, an overload of SelectMany with one generic parameter needs to be explicitly present, and in order to make it distinct from the first overload, the result parameter needs to be made not optional, which would also mean that selector can't be optional either:

    SelectMany<S, V>(selector: (x: T, index: number) => Iterable<S>, result: (x: T, s: S) => V): Enumerable<V>;
    SelectMany<S>(selector?: (x: T, index: number) => Iterable<S>): Enumerable<S>;

P.S. I also don't quite get why selector was declared optional in the first place.

ENikS commented

Do you want to send a pull request?

Yes, in a few hours.

One question before I do that though: is there a specific reason for selector parameter to be optional? The implementation seems to require its presence, so it looks like omitting it would cause a crash. On the other hand, if I remove the optionality, it might break existing code (well, it was obviously already broken, but now it won't compile, too :-).

Ah, I see now: it's being stubbed with identify function at the facade level. Never mind then.

Although, come to think of it, omitting selector will cause the same type inference problem as described above. But it seems that TypeScript isn't flexible enough to fix that.

ENikS commented

If these are not optional you will not be able to do this:

var jsn = [
    [11, 21, 31],
    [12, 22, 32],
    [13, 23, 33],
    [14, 24, 34]
];

for (let num of Linq(jsn).SelectMany()) {
    Console.log(num);
}

Right, But hover on num in your code. See what type it's inferred to?

ENikS commented

I can't right now. Writing from my phone :)

Well, I'll tell you then: num in your example would be inferred to {}. This is because, in the absence of the selector parameter, TypeScript doesn't have anything from which to infer S. C# would complain in this case, but TypeScript just defaults to {}.

I don't think this can be fixed though.

ENikS commented

What if you explicitly specify it like this:

var jsn : Iterable<Iterable<Number>> = [
    [11, 21, 31],
    [12, 22, 32],
    [13, 23, 33],
    [14, 24, 34]
];

It's not T that TypeScript can't infer. T is actually perfectly known.
What TypeScript can't infer is S.

But yes, you can explicitly specify it like this:

for(let num in Linq(jsn).SelectMany<int>())

But you have to remember to do that, it would be a great pain in the butt, and it won't be the immediately obvious solution when you encounter the errors stemming from the type being {}. It always takes me a few seconds to realize that the type inference failed somewhere and go look for it.

Actually, now that I think about it, if I was designing the API, I would sacrifice brevity and some efficiency for clarity and reliability, and would have made selector non-optional. You'd still be able to do this trivial flattening, you'd just have to specify an identity lambda as argument:

for(let num in Linq(jsn).SelectMany( x => x ))
ENikS commented

I agree but it would not be worth it to break interface