-
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
Mitigate first-class Span breaks #108789
Mitigate first-class Span breaks #108789
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -305,7 +305,7 @@ public void PeekRef_Empty() | |
protected override IEnumerable<T> GetEnumerableOf<T>(params T[] contents) | ||
{ | ||
ImmutableStack<T> stack = ImmutableStack<T>.Empty; | ||
foreach (T value in contents.Reverse()) | ||
foreach (T value in Enumerable.Reverse(contents)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This mitigation is only needed in runtime, user code shouldn't break (unless they write code into System namespace). The break is because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
BTW this is not using System namespace https://github.com/dotnet/aspnetcore/blob/c9df8c1550a941f104db3b442a13587a2fcb901c/src/DataProtection/DataProtection/src/Internal/ContainerUtils.cs#L106 and yet we are getting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't have the new Enumerable.Reverse(T[]) overload. |
||
{ | ||
stack = stack.Push(value); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,7 +137,7 @@ public static void Set_CustomComparerTest() | |
|
||
int visited = s_dictionaryData.Length; | ||
|
||
Assert.All(s_dictionaryData.Reverse(), element => | ||
Assert.All(Enumerable.Reverse(s_dictionaryData), element => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this one need to change? We're only building here for netcoreappcurrent, which should be able to see the array-based overload, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, are these all in the same boat as your comment on the immutable tests, that they're not seeing the new Enumerable overload because the tests themselves are in the system namespace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, these are all in the same boat; they are all in System namespace. |
||
{ | ||
string newValue = "new" + element.Value; | ||
Assert.True(ld.Contains(element.Key)); | ||
|
@@ -154,7 +154,7 @@ public static void Remove_CustomComparerTest() | |
ObservableStringComparer comparer = new ObservableStringComparer(); | ||
ListDictionary ld = Fill(new ListDictionary(comparer), s_dictionaryData); | ||
|
||
Assert.All(s_dictionaryData.Reverse(), element => | ||
Assert.All(Enumerable.Reverse(s_dictionaryData), element => | ||
{ | ||
int originalSize = ld.Count; | ||
Assert.True(ld.Contains(element.Key)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,7 +181,6 @@ public void Queue_Generic_ToArray_NonWrappedQueue(int count) | |
Queue<T> collection = new Queue<T>(count + 1); | ||
AddToCollection(collection, count); | ||
T[] elements = collection.ToArray(); | ||
elements.Reverse(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :) |
||
Assert.True(Enumerable.SequenceEqual(elements, collection.ToArray<T>())); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,7 @@ public void Stack_Generic_Constructor_IEnumerable(EnumerableType enumerableType, | |
_ = numberOfMatchingElements; | ||
IEnumerable<T> enumerable = CreateEnumerable(enumerableType, null, enumerableLength, 0, numberOfDuplicateElements); | ||
Stack<T> stack = new Stack<T>(enumerable); | ||
Assert.Equal(enumerable.ToArray().Reverse(), stack.ToArray()); | ||
Assert.Equal(Enumerable.Reverse(enumerable.ToArray()), stack.ToArray()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same for many of these... I'm not clear on why changes here are necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason as the other ones - the System namespace problem. |
||
} | ||
|
||
[Fact] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,7 @@ public static void TryCopyToArraySegmentImplicit() | |
Memory<int> srcMemory = src; | ||
bool success = srcMemory.TryCopyTo(segment); | ||
Assert.True(success); | ||
Assert.Equal(src, segment); | ||
Assert.Equal(src.AsSpan(), segment); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some ambiguities in xunit that should be resolved by updating (xunit/xunit#3021) but that seemed like a complex task (to update xunit in dotnet/runtime) |
||
} | ||
|
||
[Fact] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1165,7 +1165,7 @@ internal static void GetStoreValuesAsStringOrStringArray(HeaderDescriptor descri | |
else | ||
{ | ||
Debug.Assert(length > 1, "The header should have been removed when it became empty"); | ||
values = multiValue = new string[length]; | ||
values = (multiValue = new string[length])!; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nullability changed a bit where previously user-defined conversions between Spans and arrays were involved and now the built-in implicit span conversions are used. The rules should be actually "better", for example this code was assigning |
||
} | ||
|
||
int currentIndex = 0; | ||
|
@@ -1205,8 +1205,8 @@ internal static int GetStoreValuesIntoStringArray(HeaderDescriptor descriptor, o | |
} | ||
|
||
int currentIndex = 0; | ||
ReadStoreValues<object?>(values, info.ParsedAndInvalidValues, descriptor.Parser, ref currentIndex); | ||
ReadStoreValues<string?>(values, info.RawValue, null, ref currentIndex); | ||
ReadStoreValues<object?>(values!, info.ParsedAndInvalidValues, descriptor.Parser, ref currentIndex); | ||
ReadStoreValues<string?>(values!, info.RawValue, null, ref currentIndex); | ||
Debug.Assert(currentIndex == length); | ||
|
||
return length; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2658,7 +2658,7 @@ public static Tensor<T> PermuteDimensions<T>(this Tensor<T> tensor, params ReadO | |
|
||
if (dimensions.IsEmpty) | ||
{ | ||
lengths = tensor._lengths.Reverse().ToArray(); | ||
lengths = Enumerable.Reverse(tensor._lengths).ToArray(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @michaelgsharp, subsequently we should avoid using LINQ in these cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This instance of LINQ usage was addressed in the changes from #109405, as it was hindering the source-build codeflow. Perhaps we should consider removing LINQ usage entirely from tensor computation (to ensure more efficient and optimized code). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
permutation = Enumerable.Range(0, tensor.Rank).Reverse().ToArray(); | ||
} | ||
else | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
<ProjectReference Include="$(CoreLibProject)" /> | ||
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime\src\System.Runtime.csproj" /> | ||
<ProjectReference Include="$(LibrariesProjectRoot)System.Collections\src\System.Collections.csproj" /> | ||
<ProjectReference Include="$(LibrariesProjectRoot)System.Memory\src\System.Memory.csproj" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this the build would fail with:
That's because that helper is now used for codegen of the built-in span conversions from string to ReadOnlySpan. Previously the user-defined conversion on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not understanding this one. For .NET Framework, Span itself is part of the System.Memory package. How could you be in a situation where you're using Span on netfx but don't have the System.Memory reference in your transitive closure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's just some weirdness of this project. It has a reference to Span via |
||
<ProjectReference Include="$(LibrariesProjectRoot)System.Threading\src\System.Threading.csproj" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project was building against net8.0, net9.0 with C# 14 (preview). That means it gets the first-class Span feature but not the
Enumerable.Reverse(this T[])
API. I changed it to build with langversion=latest instead of preview. I could alternatively change the specificReverse()
usage, but in general building older TFMs with newer langversions is unsupported, so this seemed better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will that mean when C# 14 is latest? We'll still be building this against net8.0 at that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But when you are targeting net8.0, then you are getting LangVersion=13 as latest, I think. But yeah, there was just one break here I think, so I could just explicitly use
Enumerable.Range
as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the case.
latest
does not take TFM into account, e.g. if I create a .NET Framework 4.8 app, it defaults to using C# 7.3, but if I change LangVer to latest, I get the most recent version my compiler supports.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I removed this.