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.

Sign up for GitHub

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 Enumerable.Reverse(this T[]) polyfill #75104

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Add Enumerable.Reverse(this T[]) polyfill #75104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65ecd79
Add `Enumerable.Reverse(this T[])` polyfill
jjonescz Sep 13, 2024
646c6b8
Remove the extension method in .NET 10
jjonescz Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Compilers/Core/Portable/InternalUtilities/EnumerableExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,13 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the guidelines say to define the non-extension version for binary compatibility, or does that not apply here for some reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

#else
public static IEnumerable<T> Reverse<T>(this T[] source) => Enumerable.Reverse(source);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jjonescz jjonescz Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dotnet/roslyn/blob/5a39d0ad691ee88dcf163808d9ed7a46b168a1c9/docs/contributing/Target%20Framework%20Strategy.md#pattern-for-extension-methods

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should be able to mitigate this by wrapping this in

#if !NET10_OR_GREATER

By 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't we potentially break in a torn dll scenario though? Or have we thoroughly removed any change of that at this point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we certainly had a tear like this when we had the EA dll. Have we gotten rid of all of those scenarios?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't have this type of tearing problem without the EA DLL

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good idea about adding the #if for .NET 10, I have done that. Now it matches our guidelines linked above precisely.

#endif

#if NETSTANDARD

// Copied from https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/src/System/Linq/Chunk.cs
Expand Down
2 changes: 1 addition & 1 deletion ...able/CodeRefactorings/UseRecursivePatterns/UseRecursivePatternsCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ protected override async Task FixAllAsync(

// Process all nodes to refactor in reverse to ensure nested nodes
// are processed before the outer nodes to refactor.
foreach (var originalNode in Enumerable.Reverse(nodes))
foreach (var originalNode in nodes.Reverse())
{
// Only process nodes fully within a fixAllSpan
if (!fixAllSpans.Any(fixAllSpan => fixAllSpan.Contains(originalNode.Span)))
Expand Down
Loading