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

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

Closed
fsoikin opened this issue Feb 27, 2017 · 12 comments · Fixed by #49
Closed

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

fsoikin opened this issue Feb 27, 2017 · 12 comments · Fixed by #49

Comments

@fsoikin
Copy link
Contributor

fsoikin commented Feb 27, 2017

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
Copy link
Owner

ENikS commented Feb 27, 2017

Do you want to send a pull request?

@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

Yes, in a few hours.

@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

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 :-).

@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

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

@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

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
Copy link
Owner

ENikS commented Feb 27, 2017

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);
}

@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

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

@ENikS
Copy link
Owner

ENikS commented Feb 27, 2017

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

@ENikS ENikS closed this as completed in #49 Feb 27, 2017
@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

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
Copy link
Owner

ENikS commented Feb 27, 2017

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]
];

@fsoikin
Copy link
Contributor Author

fsoikin commented Feb 27, 2017

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
Copy link
Owner

ENikS commented Feb 27, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants