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

[API Proposal]: Enumerable.Reverse<T>(T[]) #107723

Closed
333fred opened this issue Sep 11, 2024 · 10 comments · Fixed by #107957
Closed

[API Proposal]: Enumerable.Reverse<T>(T[]) #107723

333fred opened this issue Sep 11, 2024 · 10 comments · Fixed by #107957
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Linq in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@333fred
Copy link
Member

333fred commented Sep 11, 2024

Background and motivation

The BCL has long resisted adding new array-based enumerable implementations, and with good reason; however, C# 14 is going to introduce a new feature, first-class spans, which unfortunately changes what array.Reverse() means between void MemoryExtensions.Reverse<T>(this Span<T>) and IEnumerable<T> Enumerable.Reverse<T>(this IEnumerable<T>). Overall, the benefits of this new feature will pretty significantly benefit the BCL by reducing the number of overloads of various things that are necessary; in talking with @stephentoub, we believe that adding an array-based overload, only for this specific case, and only to ensure source-compatibility for array.Reverse(), is worth the tradeoff.

Elaborating on the break: today, array.Reverse() binds to Enumerable.Reverse<T>(IEnumerable<T>), and MemoryExtensions.Reverse(Span<T>) isn't applicable. In C# 14, this changes; both of these methods are applicable in extension form, and the Span<T> version is preferred over IEnumerable<T>. This means the behavior changes from "return a new stream, reversed from the input stream", to "in-place reverse the array, and return nothing". This is almost certainly a source-breaking change (or, for the person that ignored the result of Reverse for some reason, is a behavior break). By introducing Enumerable.Reverse<T>(T[]), that method will be the most-specific overload among the 3 applicable methods, and existing behavior will be preserved.

Important note: this does not mean that we are reopening the door for array-based Enumerable extensions overall. We are specifically looking at this method to fix a source-breaking change in C# 14. This proposal should not be used to justify further expansion of array-based Enumerable methods.

API Proposal

namespace System.Linq;

public static class Enumerable
{
    public static IEnumerable<TSource> Reverse<TSource>(this T[] source);
}

API Usage

using System;
using System.Linq;

int[] l = [1, 2, 3];

foreach (var v in l.Reverse())
    Console.WriteLine(v);

Alternative Designs

No response

Risks

This is a fairly low-risk API, so long as we don't go using it to justify further array-based Enumerable overloads.

@333fred 333fred added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 11, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Sep 12, 2024
@eiriktsarpalis eiriktsarpalis added this to the 10.0.0 milestone Sep 12, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 12, 2024
@eiriktsarpalis
Copy link
Member

Sounds reasonable. I'm guessing this is the only (Linq) location where such a break was encountered when prototyping first-class spans?

@jjonescz
Copy link
Member

jjonescz commented Sep 12, 2024

I'm guessing this is the only (Linq) location where such a break was encountered when prototyping first-class spans?

Yes.

Other notes:

An alternative would be applying OverloadResolutionPriorityAttribute(-1) to MemoryExtensions.Reverse. Then array.Reverse() would continue to bind to Enumerable.Reverse, and span.Reverse() would continue to bind to MemoryExtensions.Reverse. If we also added MemoryExtensions.ReverseInPlace or something like that, one could even call it on an array like array.ReverseInPlace().
(EDIT: This does not work because ORPA is not considered across different types.)

The same breaking change exists for people on .NET Framework / netstandard and using the System.Memory NuGet package and updating to C# 14. I'm not sure how we can tackle that. Perhaps users can define their own Reverse(this T[]) extension method to unblock themselves.

@stephentoub
Copy link
Member

stephentoub commented Sep 12, 2024

An alternative would be applying OverloadResolutionPriorityAttribute(-1) to MemoryExtensions.Reverse. Then array.Reverse() would continue to bind to Enumerable.Reverse, and span.Reverse() would continue to bind to MemoryExtensions.Reverse.

What's the downside to this? Is it just that if I have an array, calling array.Reverse() will bind to the LINQ method (which is already the case today)?

@jjonescz
Copy link
Member

What's the downside to this? Is it just that if I have an array, calling array.Reverse() will bind to the LINQ method (which is already the case today)?

Yes, I don't think there are any other downsides. I just haven't thought this workaround through more previously. The usual effect of ORPA making the overload un-callable does not apply here since both MemoryExtensions.Reverse(span) and span.Reverse() invocations should always bind to only this one method anyway (both without the feature and with the feature + the ORPA workaround).

@Joe4evr
Copy link
Contributor

Joe4evr commented Sep 12, 2024

An alternative would be applying OverloadResolutionPriorityAttribute(-1) to MemoryExtensions.Reverse.

I thought [OverloadResolutionPriority] is only considered for members declared in the same type, ergo it's ignored if the ambiguity is between extension methods declared across different classes.

@jjonescz
Copy link
Member

I thought [OverloadResolutionPriority] is only considered for members declared in the same type, ergo it's ignored if the ambiguity is between extension methods declared across different classes.

Ah, thanks, you're right, I forgot about that aspect. Disregard the ORPA alternative then.

@333fred
Copy link
Member Author

333fred commented Sep 12, 2024

@Joe4evr is correct. ORPA is not an option here.

@333fred
Copy link
Member Author

333fred commented Sep 12, 2024

Perhaps users can define their own Reverse(this T[]) extension method to unblock themselves.

Yes, that is my assumption on what they'll need to do. I've suggested to @Sergio0694 that he should add this to polysharp, and appropriately type forward in the negative case. Since this is called by user code, if the type forward isn't there, anyone attempting to support downlevel will need to be careful, or risk creating a missingmethodexception scenario.

@terrajobst
Copy link
Member

terrajobst commented Sep 17, 2024

Video

  • As a rare tie breaker this is fine.
namespace System.Linq;

public static class Enumerable
{
    public static IEnumerable<TSource> Reverse<TSource>(this TSource[] source);
}

@terrajobst terrajobst 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 Sep 17, 2024
333fred added a commit that referenced this issue Sep 17, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Sep 17, 2024
333fred added a commit that referenced this issue Sep 25, 2024
sirntar pushed a commit to sirntar/runtime that referenced this issue Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Linq in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants