Add Enumerable.Reverse(this T[]) polyfill
#75104
Conversation
| @@ -879,6 +879,9 @@ public static bool SequenceEqual<T>(this IEnumerable<T>? first, IEnumerable<T>? | |||
| } | |||
| } | |||
|
|
|||
| // https://github.com/dotnet/runtime/issues/107723 | |||
| public static IEnumerable<T> Reverse<T>(this T[] source) => Enumerable.Reverse(source); | |||
There was a problem hiding this comment.
It's probably not correct to put this here. @jaredpar to confirm, but I'm concerned that we'll end up breaking ourselves when the bcl adds this.
There was a problem hiding this comment.
I think this is fine. Problems could arise if we wrap this in #ifs incorrectly (so when bcl adds this we need to be careful making this conditional).
There was a problem hiding this comment.
I think that we should be able to mitigate this by wrapping this in
#if !NET10_OR_GREATERBy the time that we actually start targeting net10.0 this API is going to be there so that won't cause a break on us. We generally move to the new TFM rather late in the release and think that we can expect this to be added early given it's tied to first class span support which is going in early.
There was a problem hiding this comment.
Won't we potentially break in a torn dll scenario though? Or have we thoroughly removed any change of that at this point?
There was a problem hiding this comment.
What teared scenario are you thinking of? This is just another API inside the compiler. The only impact that can happen is weirdness at compile time. The #if will ensure that net9.0 or earlier we have the method. If we get deployed into net10.0 or higher then there is just another polyfill API in our binary. In the case the #if is false then we're in net10.0 where the API is guaranteed to be available.
There was a problem hiding this comment.
Well, we certainly had a tear like this when we had the EA dll. Have we gotten rid of all of those scenarios?
There was a problem hiding this comment.
Shouldn't have this type of tearing problem without the EA DLL
There was a problem hiding this comment.
Hm, good idea about adding the #if for .NET 10, I have done that. Now it matches our guidelines linked above precisely.
Keeping it as non-extension for binary compatibility.
| @@ -880,7 +880,11 @@ public static bool SequenceEqual<T>(this IEnumerable<T>? first, IEnumerable<T>? | |||
| } | |||
|
|
|||
| // https://github.com/dotnet/runtime/issues/107723 | |||
| #if NET10_0_OR_GREATER | |||
| public static IEnumerable<T> Reverse<T>(T[] source) => Enumerable.Reverse(source); | |||
There was a problem hiding this comment.
in net10.0 though won't we get this method for free? Basically it would be coming from the BCL so we wouldn't need it at all.
There was a problem hiding this comment.
Yes, but the guidelines say to define the non-extension version for binary compatibility, or does that not apply here for some reason?
There was a problem hiding this comment.
Yes but what idiot wrote those guidelines? ;)
I looked back over the guidelines and I'm trying to decide why I added that particular section. I know the reason for creating the non-extension version was to avoid compile time ambiguities. I can't remember exactly why I didn't say "just remove it entirely". I'm guessing it was because there were cases where it was used by full name ...
Anyways, wrote that for a reason so lets follow it 😄
To avoid needing to fixup more locations when merging main into the feature branch. See also #75077 (comment).
Test plan: #73445