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

Add new overloads for Select and other LINQ methods #34553

Closed
AjayKMehta opened this issue Apr 5, 2020 · 8 comments
Closed

Add new overloads for Select and other LINQ methods #34553

AjayKMehta opened this issue Apr 5, 2020 · 8 comments

Comments

@AjayKMehta
Copy link

Currently, if you want to use a partially applied version of a function of arity greater than one (e.g. Func<T, A, R>) in a call to Select<T, R>(IEnumerable<T>, Func<T,R>) where the argument of type A is a local variable, you have 2 options, neither of which is great.

To illustrate these 2 options, consider the contrived example where you have the function defined below and want to use this in a call to Select for an IEnumerable<int>:

string MakeString(int len, bool toLower) => new string(toLower ? 'a' : 'A', len);
  1. Create a closure:
bool toLower = userInput == "Lower";
var results = Enumerable.Range(1, 10).Select(i => MakeString(i, toLower));
  1. To avoid creating a closure (and allocating), wrap the function (since C# doesn't support partial application like functional languages):
Func<int, string> MakeStringWrapper(bool toLower) => x => MakeString(x, toLower)
var results2 = Enumerable.Range(1, 10).Select(MakeStringWrapper(toLower));

This is not too bad since C# now supports local functions so you no longer have to pollute your code with private helper methods but this is still not ideal.

I propose adding an overload as an improvement over both these options:

public static IEnumerable<TResult> Select<TSource,TArg,TResult> (this IEnumerable<TSource> source, Func<TSource,TArg,TResult> selector, TArg arg1);

Simplified implementation (I get that actual implementation is more complex and specialized based on runtime types) would look like:

public static IEnumerable<TResult> Select<TSource,TArg,TResult> (this IEnumerable<TSource> source, Func<TSource,TArg,TResult> selector, TArg arg1)
{
    foreach(var item in source)
    {
        yield return selector(item, arg1);
    }
}

This is not a fringe issue as I have seen this a lot in LINQ code in the wild. Similar overloads could be added for other LINQ methods like GroupBy, OrderBy.

More overloads could possibly be added for the case where parameter order is switched for selector, i.e. Func<TArg,TSource,TResult> instead of Func<TSource,TArg,TResult> as shown above and for more arities.

The idea for this came from thinking of how adding *args and/or **kwargs to function signatures in Python addresses this situation 😀

def add(a, b):
      return a + b


def map_vals(iterable, f, *args, **kwargs):
	for item in iterable:
		yield f(item, *args, **kwargs)


list(map_vals(range(10), add, 2))
# [2, 3, 4, 5, 6, 7, 8, 9, 10, 11]

list(mapVals(range(10), add, b=-1))
# [-1, 0, 1, 2, 3, 4, 5, 6, 7, 8]
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Linq untriaged New issue has not been triaged by the area owner labels Apr 5, 2020
@ghost
Copy link

ghost commented Apr 5, 2020

Tagging @eiriktsarpalis as an area owner

@Joe4evr
Copy link
Contributor

Joe4evr commented Apr 5, 2020

This is what I like to call "the TState pattern" (since the extra type parameter is usually called TState), and I suggested the same thing on Twitter some time ago, except I phrased to be more general, as opposed to only LINQ.

@Suchiman
Copy link
Contributor

Suchiman commented Apr 6, 2020

@AjayKMehta

  1. To avoid creating a closure (and allocating), wrap the function (since C# doesn't support partial application like functional languages):
Func<int, string> MakeStringWrapper(bool toLower) => x => MakeString(x, toLower)
var results2 = Enumerable.Range(1, 10).Select(MakeStringWrapper(toLower));

This is no better than 1. in regards to allocations and worse in readability, this just moves the closure allocation to the expression x => MakeString(x, toLower) which closes over toLower which isn't an argument to that lambda but to the outer function, see sharplab.
You can also see it in VS if you hover the fat arrow:
devenv_2020-04-06_03-55-31

The only true allocation free solution is the explicit TState version you're proposing, which has prior art in

public static string Create<TState>(int length, TState state, SpanAction<char, TState> action)
but doing this on a large scale will cause so much code duplication...

@AjayKMehta
Copy link
Author

@Suchiman Yep, you are right. I somehow overlooked that. I quickly wrote the example code in LINQPad 6 (free edition) so I didn't see any warnings. Next time I'll use VS or VS Code 😄

IMHO having a few overloads would be great. Covering all the cases up to arity of 3 means 5 more overloads.

@svick
Copy link
Contributor

svick commented Apr 6, 2020

Considering that LINQ is inherently fairly allocation-heavy, would this actually significantly decrease the allocations?

@eiriktsarpalis
Copy link
Member

My feeling is that if allocations are a source of concern, then you shouldn't be using LINQ in the first place. For the particular issue you bring up, I believe it is almost always the case that you could refactor the code in a way such that only one closure is allocated, the one being passed to the Select method.

Given that we'd probably also need to support arities > 2, and the fact that it can be implemented trivially as an extension method like you demonstrated, I don't believe there would be a lot of value in adding this to the main API, though happy to be corrected.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Apr 6, 2020
@AjayKMehta
Copy link
Author

@eiriktsarpalis Yes, I could write the code so that I have only 1 closure allocated.

This is not pressing. Thanks for looking into this.

@eiriktsarpalis eiriktsarpalis added this to the Future milestone Jun 26, 2020
@adamsitnik
Copy link
Member

@AjayKMehta thank you for your proposal

I agree with all the points that @eiriktsarpalis has made and having said that I am going to close the issue.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants