-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Proposal (& implementation): LINQ APIs to enable C# 8.0 index and range for IEnumerable<T> #28776
Comments
This looks interesting to me and has been on my list of things to look into for Ix and async LINQ for From a technical point of view, here are my 0.02 cents:
|
In light of symmetry, don't forget PLINQ (where operators must be implemented by the library, and not with add-ons). |
@bartdesmet Thank you for your insights. I hope these LINQ operators can be provided in corefx. Since index/range are language level features, it is nice to have them work with not only array, but also any type that implements |
@Clockwork-Muse I looked at PLINQ before I post this proposal. |
@Dixin - |
@joshfree @tarekgh Thank you for labeling this issue. Since it is now in the Future Milestone (issues not triaged to a planned release. Work in this milestone may happen, or may not). Does it mean there is not much chance of actions for the next release? Please suggest what should I do next. |
We'll look at the details to ensure the proposed APIs is reasonable and then we'll have the issue reviewed by the design committee. expect will get traction on this when we ship 3.0. if you didn't see any activities by then, please ping us. Usually we'll get into that in right time. Thanks for the proposal. |
Thank you @tarekgh. |
My concern with this approach is that it seems to assign arbitrary meaning to "from end" indices simply to reconcile them with IEnumerable. While throwing is probaby the better approach, it can still be violating user expectations, particularly if the type implementing IEnumerable is a list: var l = new List<int>() { 1, 2, 3 };
l[^1]; // 3
l.ElementAt(^1); // throws ArgumentOutOfRangeException It could be argued that a type test could be used and adapt meaning if the underlying type is As such, I would recommend against accepting this proposal. We should simply acknowledge that indices and ranges are intended for list-like types, and IEnumerable is much more general-purpose than that. |
@eiriktsarpalis, thank you for your review and feedback. Actually in my proposed implementation, the meaning of "from end" is consistent with existing APIs. For the proposed static method
I agree this particular method should only allow "from start", and disallow "from end". For the other method
You can also take a look at the unit tests of the proposed implementation.
You mentioned
That's why I proposed these APIs. They would enable Index and Range for so many types in .NET and for LINQ queries. If they keep consistency with existing APIs, they can provide good convenience. Thank you @eiriktsarpalis so much again for your time. Please let me know what you think. |
Thanks for the clarification @Dixin, I mistook your original comment as specifying the "from end" behaviour for all proposed methods and not just The Going back to I tend to favour Also, @Dixin, it looks most of the |
Note that ranges only support non-negative numbers, since they are ranges of indices rather than a general-purpose range construct. |
@eiriktsarpalis, thank you for your reply.
That is by design.
I totally realize |
Technically this equivalent proposal only requires O(5) space, at most. There's no reason a from-end |
I'm not too sure about that. It seems to me that the particular design of |
@eiriktsarpalis I updated the proposal contents including all links. Thank you. |
@Clockwork-Muse Yes, the proposed implementation only requires O(5) space, at most. Here is the link to the source. |
It's Θ(k) for |
I am not sure either, and I don't have a preference here, just trying to put everything on the plate for discussion. @eiriktsarpalis please let me know if you want me to remove it from the proposal or make other changes. |
At this point I'm fairly convinced that we should hold off from adding the |
…At, keep the original behavior.
…At, keep the original behavior.
…At, keep the original behavior.
… for ElementAt, ElementAtOrDefault.
* Implement #28776: Implement LINQ APIs for index and range. * Implement #28776: Implement LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. Code review update. * Implement #28776: LINQ APIs for index and range. Code review update. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. Code review update. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. * Implement #28776: LINQ APIs for index and range. Update ElementAt, keep the original behavior. * Implement #28776: LINQ APIs for index and range. Update ElementAt, keep the original behavior. * Implement #28776: LINQ APIs for index and range. Update ElementAt, keep the original behavior. * Implement #28776: LINQ APIs for index and range. Add unit tests for ElementAt, ElementAtOrDefault. * Implement #28776: LINQ APIs for index and range. Update unit tests. * Implement #28776: LINQ APIs for index and range. Update unit tests. * Implement #28776: LINQ APIs for index and range. Update unit tests. * Implement #28776: LINQ APIs for index and range. Update for merge. * Implement #28776: LINQ APIs for index and range. Update unit tests.
C# 8.0 introduces index and range features for array and countable types. It is natural and convenient to generally support index and range for all IEnumerable types and LINQ queries. I searched the API review notes, didn't find such APIs, so propose them here.
Problem
Index/range are language level features. Currently, they
RuntimeHelpers.GetSubArray
, etc.)IEnumerable<T>
sequence, or LINQ query.Rationale and usage
The goals of these LINQ APIs are:
This enables index and range language features to work with any type that implements
IEnumerable<T>
, and LINQ queries.LINQ already has
ElementAt(int)
andElementAtOrDefault(int)
operators. It would be natural to have aSystem.Index
overload asElementAt(Index)
andElementAtOrDefault(Index)
, as well asSlice(Range)
, so that LINQ can seamlessly work with C# 8.0:With these APIs, the C#
countable[Index]
andcountable[Range]
syntax are enabled for sequences & LINQ queries asenumerable.ElementAt(Index)
andenumerable.Slice(Range)
.Proposed APIs
For LINQ to Objects:
For remote LINQ:
API review feedback: @eiriktsarpalis:
Implementation details (and pull request)
The API review process says PR should not be submitted before the API proposal is approved. So currently I implemented these APIs separately Dixin/Linq.IndexRange:
Enumerable
:ElementAt(Index)
andElementAtOrDefault(Index)
Slice(Range)
Queryable
:ElementAt(Index)
,ElementAtOrDefault(Index)
,Slice(Range)
.Please see the unit tests about how they work.
These proposed APIs can be used by adding NuGet package
Linq.IndexRange
:If this proposal is doable, I can submit a PR quickly.
Open questions
Slice(Range)
vsElementsIn(Range)
as nameShould
Slice
be calledElementsIn(Range)
? Currently I implemented both.ElementsIn(Range)
keeps the naming consistency with originalElementAt(Index)
. Might it be natural for existing LINQ users?Slice
is consistent with the countable types, which requires aSlice
method to support range. I preferSlice
for this reason.API review feedback: Rename to
Take(Range)
.@eiriktsarpalis:
Out of bound handling
If Index is out of the boundaries, for
ElementAt(Index)
, there are 2 options:IndexOutOfRangeException
ArgumentOutOfRangeException
. My current implementation goes this way, to keepElementAt(Index)
consistent with orginalElementAt(int)
.If Range goes off the boundaries of source sequence, for
Slice(Range)
, there are 2 options:array[Range]
, throwArgumentOutOfRangeException
. I implementedElementsIn(Range)
following this way. See unit tests ofElementsIn
.Skip
/Take
/SkipLast
/TakeLast
, do not throw any exception. I implementedSlice
following this way. See unit test ofSlice
.API review feedback: @eiriktsarpalis:
ElementAt(Index)
andQueryable
As @bartdesmet mentioned in the comments, LINQ providers may have issues when they see
ElementAt
having anIndex
argument, etc. Should we have a new name for the operator instead of overload? For example,At(Index)
orIndex(Index)
?Feedback
My original proposal includes other methods like
Enumerable.Range(Range)
andAsEnumerable(this Range)
. After discussion with @eiriktsarpalis, we are keeping onlyElementAt(Index)
andSlice(Range)
. The original proposal can be found in the history or this link.I had an email discussion with @MadsTorgersen, merging his feedback here:
API review feedback
@eiriktsarpalis:
The text was updated successfully, but these errors were encountered: