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

Allow for specify return value on System.Linq.Enumerable.*OrDefault methods #20064

Closed
Keboo opened this issue Jan 31, 2017 · 35 comments · Fixed by #48886
Closed

Allow for specify return value on System.Linq.Enumerable.*OrDefault methods #20064

Keboo opened this issue Jan 31, 2017 · 35 comments · Fixed by #48886
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Linq help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@Keboo
Copy link
Member

Keboo commented Jan 31, 2017

I would like to propose adding overloads to the System.Linq.Enumerable.*OrDefault and System.Linq.Queryable.*OrDefault methods for specifying the value to be returned in the default case. This is the value that would be returned instead of default(TSource) when there are no items in the enumeration

There are times when the default value of a given type may be a valid value from the enumeration. If the default value for the enumeration type is valid value, there is not a nice way to determine if the returned values was because the enumeration was empty or if that value was in the enumeration.

Proposed API change

namespace System.Linq
{
    public static class Enumerable
    {
        public static TSource SingleOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
        public static TSource SingleOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);

        public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
        public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);

        public static TSource LastOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
        public static TSource LastOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);
    }

    public static class Queryable
    {
        public static TSource SingleOrDefault<TSource>(this IQueryable<TSource> source, TSource defaultValue);
        public static TSource SingleOrDefault<TSource>(this IQueryable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);

        public static TSource FirstOrDefault<TSource>(this IQueryable<TSource> source, TSource defaultValue);
        public static TSource FirstOrDefault<TSource>(this IQueryable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);

        public static TSource LastOrDefault<TSource>(this IQueryable<TSource> source, TSource defaultValue);
        public static TSource LastOrDefault<TSource>(this IQueryable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);
    }
}

Updates

  • Switch from using default parameters to method overloads
  • Added IQueryable methods as well
@jnm2
Copy link
Contributor

jnm2 commented Jan 31, 2017

This would be an alternative to having to switch from SingleOrDefault(predicate) to DefaultIfEmpty(x).Single(predicate).

@karelz
Copy link
Member

karelz commented Feb 1, 2017

You are adding new methods (we can't change existing ones).
I think the = default(TSource) will cause compiler ambiguity with the existing methods - we need to drop the default value IMO (please check first if I am not mistaken).

@JonHanna
Copy link
Contributor

JonHanna commented Feb 1, 2017

It wouldn't cause ambiguity at the compiler level, but the default would never be used because the existing methods would always beat it in the overload resolution, so it's pointless. It could certainly cause ambiguity at the human understanding level.

If we had this (I've no strong opinion either way on that) we should also have Queryable equivalents.

@jnm2
Copy link
Contributor

jnm2 commented Feb 1, 2017

I believe the way these conversations usually go is that you can't use optional parameters without breaking binary back compat. http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx

@JonHanna
Copy link
Contributor

JonHanna commented Feb 1, 2017

I think it would be safe in that regard, given the types involved, (though it could break someone reflecting and using the count of parameters to find which overload they wanted), but I wouldn't bet on that, and it could still mess with some future change, and in any case they're still pointless defaults as they'll never be used.

@Keboo
Copy link
Member Author

Keboo commented Feb 1, 2017

@karelz and @JonHanna I updated the issue description based on your comments (I think I did it correctly; first time filing an issue in this repo).

@karelz
Copy link
Member

karelz commented Feb 1, 2017

@Keboo all good so far, we will let you know if you're not following "the rules" ;-)

FYI: Next steps in API review process are:

  1. Feedback from community on API addition
  2. Area owners support the API addition as good & valuable and tag it as 'api-ready-for-review'
  3. API review group reviews the API and tags it as 'api-approved' (or sends it back to 'api-needs-work' with comments

Don't expect super-fast turnaround. New APIs need bake time and some thinking through. The process should not be rushed even for obvious things.

If you are eager to contribute to CoreFX, I would recommend to grab some 'up for grabs' issues (which are not API proposals) in the meantime - the most valuable/impactful things are:

  1. 2.0.0 (next release) up for grabs issues
  2. wishlist issues
  3. simple issues for newbie contributors (not applied too much yet - we just started experimenting with it recently)

@karelz
Copy link
Member

karelz commented Feb 1, 2017

@jnm2 AFAIK the proposal only adds APIs, it of course can't remove any ;-)

@jnm2
Copy link
Contributor

jnm2 commented Feb 1, 2017

@karelz Ah, misunderstood the purpose of the original proposal. Latest revision makes sense.

@jamesqo
Copy link
Contributor

jamesqo commented Feb 1, 2017

Would it be better to have a selector run for the default value instead of a concrete value?

public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource> selectorIfEmpty);

I wonder if we could also pass in more information to the SingleOrDefault selector, like whether the enumerable was empty or had more than 1 element.

@Keboo
Copy link
Member Author

Keboo commented Feb 2, 2017

@jamesqo I am not sure the selector adds much value here. Can you elaborate on when you would find it useful?
As for the SingleOrDefault case, it sounds like you are expecting this selector to get invoked in both when the enumeration is empty and when it contains multiple items. Wouldn't this be a change in the behavior of SingleOrDefault?

@MarkMichaelis
Copy link

This makes sense to me... good suggestion @Keboo.

@jamesqo
Copy link
Contributor

jamesqo commented Feb 5, 2017

@Keboo Sorry for the late reply.

I am not sure the selector adds much value here. Can you elaborate on when you would find it useful?

I was thinking it could be used to lazily load a value if there was no item matching the predicate. Looking back though, I agree it may not make much sense, mainly because Linq is used with pure functions so a argument-less function would go against the grain.

@karelz Everyone seems to think this is a good idea, so maybe it can be marked ready-for-review. (I have another idea for these methods which I plan to open a proposal for later, but I won't block this on it.)

@karelz
Copy link
Member

karelz commented Feb 5, 2017

@jamesqo area experts are supposed to make the first-level API approval, by marking the API as such: @VSadov @OmarTawfik can you please check if the API proposal is in line with Linq direction?

@OmarTawfik
Copy link
Contributor

LGTM. @VSadov?
I think it is useful addition to the API.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Sep 2, 2020
@eiriktsarpalis
Copy link
Member

This is a valuable addition, LGTM as proposed. @adamsitnik any objections to marking this for api review?

@adamsitnik
Copy link
Member

@eiriktsarpalis I agree that the problem exists and it should be addressed. Example:

IEnumerable<int> integers = SomehowGetACollectionOfIntegers();
int item = integers.FirstOrDefault(); 

if (item == 0) // was the first value a `0` or there was no values at all?

However, I am not sure if passing a default value to the method is the best solution as it forces the user to perform one extra comparison. It sounds easy, but:

  • if the given type does not implement IEqutable<T> interface, then the extra check might introduce another set of problems that are related to the usage of object.Equals(object):
    • reference comparison for reference types and value comparison for value types (it's often surprising for new C# devs)
    • boxing of value types and virtual method calls (performance)
  • in a generic context, the user can not use the == operator (usability)
  • if we add T : IEqutable<T> generic constraint then it limits the usage for cases where given type implementation can not be changed (usability)

nit: Perhaps Given would be a better name than Default?

- TSource SingleOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
+ TSource SingleOrGiven<TSource>(this IEnumerable<TSource> source, TSource given);

Alternatives are:

  • Try pattern which does not fit LINQ very well (not intuitive, harder to chain multiple method calls)
bool TryFirst<TSource>(this IEnumerable<TSource> source,  out TSource found);

and could also be unclear what false means in case of methods like TrySingle where false could mean that there was more than one value or no values at all.

  • returning tuple (bool found, TSource result). Personally, this is my favorite. But it would most probably require a new method name and I don't have a good proposal
IEnumerable<int> integers = SomehowGetACollectionOfIntegers();
var item = integers.FirstOrDefault(); // compiler  error: did you mean (int) or (bool, int)

@eiriktsarpalis what do you think? I would like to be well prepared for the API review.

@eiriktsarpalis
Copy link
Member

I am not sure if passing a default value to the method is the best solution as it forces the user to perform one extra comparison.

I am not sure why you would need any form of comparison here. For example, a naive implementation of FirstOfDefault would be the following:

public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue)
{
    foreach (TSource element in source)
    {
        return element;
    }

    return defaultValue;
}

@adamsitnik
Copy link
Member

I am not sure why you would need any form of comparison here

You are right. I've missunderstood the idea. It's "give me X or use this" and most probably no ifs after that.

@eiriktsarpalis what about the name? do you like the idea of changing Default to Given?

@eiriktsarpalis
Copy link
Member

do you like the idea of changing Default to Given?

Probably, Default means a very specific thing which does not hold in the proposed overloads. Probably something to bring up during API review.

@adamsitnik adamsitnik added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 20, 2020
@adamsitnik
Copy link
Member

@eiriktsarpalis good, then I've changed the label from api-needs-work to api-ready-for-review

@Clockwork-Muse
Copy link
Contributor

  • Try pattern which does not fit LINQ very well (not intuitive, harder to chain multiple method calls)

All of the proposed methods end the chain.

@adamsitnik
Copy link
Member

@Foxtrek64 I agree with you, OrElse is a better suffix than OrGiven. Let's make it part of the official proposal.

@terrajobst what is the recommended way of updating the proposal? Should I edit the issue description or post an updated comment?

@bartonjs
Copy link
Member

bartonjs commented Nov 10, 2020

Video

Looks good as proposed, but there was a concern raised that the Queryable methods might have an impact on Linq-to-SQL, which would be good to verify (@ajcvickers do you have insight or contacts here?).

namespace System.Linq
{
    public static partial class Enumerable
    {
        public static TSource SingleOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
        public static TSource SingleOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);

        public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
        public static TSource FirstOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);

        public static TSource LastOrDefault<TSource>(this IEnumerable<TSource> source, TSource defaultValue);
        public static TSource LastOrDefault<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate, TSource defaultValue);
    }

    public static class Queryable
    {
        public static TSource SingleOrDefault<TSource>(this IQueryable<TSource> source, TSource defaultValue);
        public static TSource SingleOrDefault<TSource>(this IQueryable<TSource> source, Expression<Func<TSource, bool>> predicate, TSource defaultValue);

        public static TSource FirstOrDefault<TSource>(this IQueryable<TSource> source, TSource defaultValue);
        public static TSource FirstOrDefault<TSource>(this IQueryable<TSource> source, Expression<Func<TSource, bool>> predicate, TSource defaultValue);

        public static TSource LastOrDefault<TSource>(this IQueryable<TSource> source, TSource defaultValue);
        public static TSource LastOrDefault<TSource>(this IQueryable<TSource> source, Expression<Func<TSource, bool>> predicate, TSource defaultValue);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 10, 2020
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Nov 11, 2020
@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 11, 2020

@bartonjs This should've been caught (much) earlier, but there's one tiny change that needs to happen to these additions: The overloads in Queryable that take in Func<TSource, bool> predicate needs to be Expression<Func<TSource, bool>> predicate, otherwise Query-providers can't do their magic.

@bartonjs
Copy link
Member

@Joe4evr Good catch. Since they were approved as "add TSource defaultValue to existing overloads" I've gone ahead and fixed the approved shape to be sensible.

@Foxtrek64
Copy link
Contributor

Foxtrek64 commented Nov 13, 2020

nit: Perhaps Given would be a better name than Default?

To add my $0.02, I had suggested Else in #33443 for similar reasons you state. However, the OrElse paradigm has a history in other languages and is what I would expect for these endpoints. FirstOrGiven seems rather alien in that sense.

Ultimately I feel either would be fine, but I definitely agree with @eiriktsarpalis then they say Default has a very specific meaning and should probably be avoided here. Here's my original justification:

I had first proposed an overload for FirstOrDefault() which allows you to specify the "default" value it returns, however tannergooding suggested it might be better to create an entirely new method on the grounds that the signatures may be too similar. I decided on the OrElse suffix because it has a history in other languages. This gives it the advantage of being well-known. It should pair rather nicely with its existing OrDefault counterpart, on the grounds of them sharing similar yet distinct functions.

@bartonjs I'm not sure if it's too late for this seeing as it's already been through review, but I would like to voice a strong disapproval for the MethodOrDefault() overload/signature in these methods for the reasons above. I'm not sure how the review process works here, but if it's at all possible I would like to see MethodOrElse() be the preferred signature.

If this change is approved, I would be happy to volunteer to implement this.

@bartonjs
Copy link
Member

Since this enhancement is "I want the existing *OrDefault method, but to change what it thinks the default is", it seems like a natural overload to me, rather than having SingleOrDefault() and SingleOrElse(TSource elseValue).

Additionally, .NET has had the "OrDefault" suffix for longer than C# has had the default keyword, so there's precedent for that name (OK, largely from Enumerable and Queryable, but also Nullable and dictionaries). And "OrElse" sounds pretty aggressive to me... "give me the first item, or else!" 😄.

I definitely don't see changing these members to OrElse passing review, and am skeptical that we'd use that suffix on new functionality on Enumerable or Queryable... or even entirely new types.

@eiriktsarpalis
Copy link
Member

If this change is approved, I would be happy to volunteer to implement this.

Thanks for volunteering @Foxtrek64, I have assigned the issue to you.

@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 13, 2020

@Foxtrek64 For reference, a similar method that has existed for ages in .NET is Nullable<T>.GetValueOrDefault(T defaultValue).

@GeraudFabien
Copy link

GeraudFabien commented Nov 13, 2020

You could use the operator "??" wich is great because :

  • More readable when used to it
  • More flexible (Allow throw, other linq, ...)
  • More performent since it doesn't need to allocate the default
  • Doesn't need new method and use the current logic
  • Work with Queryable

For any case where you have a struc like dictionary, int, datetime i use two trick depending of the case

  • When you want a nullable property in struct like KeyValuePair i just do '?.Key ?? throw new XXX;'.
  • Datetime and int without rule i juste move the rule in a where 'Where(x => x == z).OffType<datetime?>().FirstOrDefault()'. This way at most only one is encapsulate in Nullable<>

I used to implement this overload for one reason. When you read "or default" i expect my first param to be default. With "??" the default is after the predicate wich can be long in some case and become hard to read.

const string MostPopularName = "Dotnet";
string foo = nameRows.FirstOrDefault(MostPopularName, nameRow =>
                        nameRow.State == MyEnumOfState.MyState &&
                        !bannedWords.Contains(nameRow) &&
                        someOtherRules);
const string MostPopularName = "Dotnet";
string foo = nameRows.FirstOrDefault(nameRow =>
                        nameRow.State == MyEnumOfState.MyState &&
                        !bannedWords.Contains(nameRow) &&
                        someOtherRules, 
                        MostPopularName);
const string MostPopularName = "Dotnet";
string foo = nameRows.FirstOrDefault(nameRow =>
                        nameRow.State == MyEnumOfState.MyState &&
                        !bannedWords.Contains(nameRow.Name) &&
                        someOtherRules)
                     ?.Name ?? MostPopularName;

@Foxtrek64
Copy link
Contributor

A few questions-

System.Collections.Immutable has its own LINQ library. Should I extend this, or any additional libraries, in this change?

Should we add ElementAtOrDefault<TSource>(this IEnumerable<TSource> source, int index, TSource other); to this change? It was not included in the original scope but I feel it is likely a logical addition.

@KieranDevvs
Copy link

You could use the operator "??" wich is great because :

  • More readable when used to it
  • More flexible (Allow throw, other linq, ...)
  • More performent since it doesn't need to allocate the default
  • Doesn't need new method and use the current logic
  • Work with Queryable

For any case where you have a struc like dictionary, int, datetime i use two trick depending of the case

  • When you want a nullable property in struct like KeyValuePair i just do '?.Key ?? throw new XXX;'.
  • Datetime and int without rule i juste move the rule in a where 'Where(x => x == z).OffType<datetime?>().FirstOrDefault()'. This way at most only one is encapsulate in Nullable<>

I used to implement this overload for one reason. When you read "or default" i expect my first param to be default. With "??" the default is after the predicate wich can be long in some case and become hard to read.

const string MostPopularName = "Dotnet";
string foo = nameRows.FirstOrDefault(MostPopularName, nameRow =>
                        nameRow.State == MyEnumOfState.MyState &&
                        !bannedWords.Contains(nameRow) &&
                        someOtherRules);
const string MostPopularName = "Dotnet";
string foo = nameRows.FirstOrDefault(nameRow =>
                        nameRow.State == MyEnumOfState.MyState &&
                        !bannedWords.Contains(nameRow) &&
                        someOtherRules, 
                        MostPopularName);
const string MostPopularName = "Dotnet";
string foo = nameRows.FirstOrDefault(nameRow =>
                        nameRow.State == MyEnumOfState.MyState &&
                        !bannedWords.Contains(nameRow.Name) &&
                        someOtherRules)
                     ?.Name ?? MostPopularName;

That wouldn't work, here's why:
Take this example:

var source = new int[] {1, 2, 3};

// Compiler error, cant null coalesce non nullable type.
int item = source.FirstOrDefault() ?? 5;

In this scenario, you would have to change the collection data type to allow nullable values, and in that case, you don't know if null is a valid value from the query, or if its not found anything.

For example:

var source = new int?[] {null, 1, 2, 3};
int item = source.FirstOrDefault() ?? 5;

If the source first item is null, FirstOrDefault returns the item (null) and then the coalesce returns 5.
If the source is empty, FirstOrDefault returns default(T) of int? which is null, and the coalesce returns 5.
In other words, this isn't an alternative solution to the problem because there is no way to determine between default(T) and a valid value.

@eiriktsarpalis
Copy link
Member

Hi @Foxtrek64, per your question you should just stick to the approved APIs for now. Consider creating a new issue if you feel that it could also be added to immutable collections.

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 6.0.0 Jan 28, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 28, 2021
eiriktsarpalis pushed a commit to Foxtrek64/runtime that referenced this issue Mar 17, 2021
eiriktsarpalis added a commit that referenced this issue Mar 18, 2021
…ethods (#48886)

* Fix #20064

* Add API to ref assembly

* Make overloads with defaultValue not nullable

* Add unit tests, simplify implementation

* Add LastOrDefault tests

* Add Queryable tests

* Additional tests. Reformatting TryGet Methods.

* Update src/libraries/System.Linq.Queryable/tests/FirstOrDefaultTests.cs

* Apply suggestions from code review

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

* Fix ref methods

* Further adjust nullability

* Fix more nullables

* fix failing tests

* Restore coding style

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 18, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Linq help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.