Mitigate first-class Span breaks #108789

jjonescz · 2024-10-11T13:36:53Z

No description provided.

jjonescz · 2024-10-11T13:54:08Z

src/libraries/System.Collections.Immutable/tests/ImmutableStackTest.cs

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


This mitigation is only needed in runtime, user code shouldn't break (unless they write code into System namespace).

The break is because MemoryExtensions.Reverse<T>(this Span<T>) is now applicable for array.Reverse(). Because Span is better than IEnumerable by overload resolution rules, code like array.Reverse() would break when going from C# 13 to C# 14. So we added Enumerable.Reverse<T>(this T[]) overload (#107723) which is preferred in normal scenarios for arrays. But it actually doesn't have an effect here since we are in the System namespace, so System.MemoryExtensions is found and then the search stops and doesn't reach System.Linq.Enumerable.

This mitigation is only needed in runtime, user code shouldn't break (unless they write code into System namespace).

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 /vmr/src/aspnetcore/src/DataProtection/DataProtection/src/Internal/ContainerUtils.cs(106,31): error CS0023: Operator '.' cannot be applied to operand of type 'void' [/vmr/src/aspnetcore/src/DataProtection/DataProtection/src/Microsoft.AspNetCore.DataProtection.csproj::TargetFramework=netstandard2.0]

It doesn't have the new Enumerable.Reverse(T[]) overload.

jjonescz · 2024-10-11T13:59:23Z

src/libraries/System.Memory/tests/Memory/CopyTo.cs

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

jjonescz · 2024-10-11T14:01:21Z

src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs

@@ -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])!;


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 string[] to Span<string?> without nullability warnings.

jjonescz · 2024-10-11T14:05:20Z

...es/System.Runtime/tests/System.Diagnostics.Debug.Tests/System.Diagnostics.Debug.Tests.csproj

@@ -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" />


Without this the build would fail with:

error CS0656: Missing compiler required member 'System.MemoryExtensions.AsSpan'

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 string was used, but that's not available on .NET Framework. In this case the conversion is part of interpolated string conversion, so this worked previously because a different interpolated string builder overload was chosen that didn't need the string->span conversion (the compiler doesn't gate the overload resolution by whether the codegen helpers exist).

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?

Maybe it's just some weirdness of this project. It has a reference to Span via src\libraries\System.Runtime\ref\System.Runtime.cs. That contains definition of Span but not MemoryExtensions.

jjonescz · 2024-10-11T17:02:46Z

....Specification.Tests/src/Microsoft.Extensions.DependencyInjection.Specification.Tests.csproj

@@ -2,6 +2,7 @@

  <PropertyGroup>
    <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks>
+    <LangVersion>latest</LangVersion>


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 specific Reverse() usage, but in general building older TFMs with newer langversions is unsupported, so this seemed better.

What will that mean when C# 14 is latest? We'll still be building this against net8.0 at that time.

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.

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.

Makes sense, I removed this.

jaredpar · 2024-10-11T17:10:02Z

@stephentoub FYI

stephentoub · 2024-10-11T17:16:39Z

src/libraries/System.Collections.Specialized/tests/ListDictionaryTests.cs

@@ -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 =>


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?

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?

Yes, these are all in the same boat; they are all in System namespace.

stephentoub · 2024-10-11T17:17:08Z

src/libraries/System.Collections/tests/Generic/Queue/Queue.Generic.Tests.cs

@@ -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();


stephentoub · 2024-10-11T17:17:39Z

src/libraries/System.Collections/tests/Generic/Stack/Stack.Generic.Tests.cs

@@ -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());


Same for many of these... I'm not clear on why changes here are necessary

Same reason as the other ones - the System namespace problem.

stephentoub · 2024-10-11T17:20:17Z

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs

@@ -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();


@michaelgsharp, subsequently we should avoid using LINQ in these cases.

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

stephentoub · 2024-11-05T22:21:11Z

@jjonescz, why was this closed?

am11 · 2024-11-05T22:26:10Z

These changes were merged in #109405 and #109467.

jjonescz added 2 commits October 11, 2024 10:45

Update Roslyn to 4.13.0-1.24510.9

049a619

Update roslyn-analyzers

f399e33

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 11, 2024

dotnet-policy-service bot assigned jjonescz Oct 11, 2024

jjonescz added area-codeflow for labeling automated codeflow and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 11, 2024

jjonescz commented Oct 11, 2024

View reviewed changes

Fix first-class Span breaks

69f2934

jjonescz force-pushed the FirstClassSpan-released branch from 3b2bf41 to 69f2934 Compare October 11, 2024 17:00

This was referenced Oct 11, 2024

[browser] Connection refused in 127.0.0.1 #104349

Open

[browser][wbt] fails with InvalidOperationException: There is no currently active test #105315

Open

jjonescz commented Oct 11, 2024

View reviewed changes

This was referenced Oct 11, 2024

[wasm] DebuggerTests.WasmHostProvider.LaunchHostAsync #108071

Open

[wasm] DebuggerTests.Inspector.OpenSessionAsync #108072

Open

chrome-DebuggerTests timing out #108078

Open

stephentoub reviewed Oct 11, 2024

View reviewed changes

This was referenced Oct 11, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

jjonescz added 2 commits October 14, 2024 10:31

Remove LangVersion=latest

42bdcfb

Fixup more places

15c50c3

build-analysis bot mentioned this pull request Oct 14, 2024

ReadECDsaPrivateKey_BrainpoolP160r1_Pfx test failure on windows #108815

Closed

jkotas mentioned this pull request Oct 30, 2024

Update the sdk target to net10.0 dotnet/sdk#43015

Merged

This was referenced Oct 31, 2024

Avoid a LINQ usage in tensor extensions #109405

Merged

Update Roslyn to 4.13.0-1.24510.9 #109467

Merged

mgravell mentioned this pull request Nov 3, 2024

Fix compilation with latest roslyn dotnet/aspnetcore#58769

Merged

jjonescz closed this Nov 4, 2024

jjonescz deleted the FirstClassSpan-released branch November 4, 2024 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mitigate first-class Span breaks #108789

Mitigate first-class Span breaks #108789

jjonescz commented Oct 11, 2024

jjonescz Oct 11, 2024

am11 Nov 3, 2024

stephentoub Nov 3, 2024 •

edited

Loading

jjonescz Oct 11, 2024

jjonescz Oct 11, 2024

jjonescz Oct 11, 2024 •

edited

Loading

stephentoub Oct 11, 2024

jjonescz Oct 14, 2024 •

edited

Loading

jjonescz Oct 11, 2024

stephentoub Oct 11, 2024

jjonescz Oct 11, 2024 •

edited

Loading

stephentoub Oct 11, 2024

jjonescz Oct 14, 2024

jaredpar commented Oct 11, 2024

stephentoub Oct 11, 2024

stephentoub Oct 11, 2024

jjonescz Oct 11, 2024

stephentoub Oct 11, 2024

stephentoub Oct 11, 2024

jjonescz Oct 14, 2024

stephentoub Oct 11, 2024

am11 Oct 31, 2024

stephentoub Oct 31, 2024

stephentoub commented Nov 5, 2024

am11 commented Nov 5, 2024

Mitigate first-class Span breaks #108789

Mitigate first-class Span breaks #108789

Conversation

jjonescz commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Nov 5, 2024

am11 commented Nov 5, 2024

stephentoub Nov 3, 2024 •

edited

Loading

jjonescz Oct 11, 2024 •

edited

Loading

jjonescz Oct 14, 2024 •

edited

Loading

jjonescz Oct 11, 2024 •

edited

Loading