Add span type inference #74646

jjonescz · 2024-08-05T11:50:15Z

Test plan: #73445
Corresponding speclet update: dotnet/csharplang#8333

jjonescz · 2024-08-05T12:20:15Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

            // var d1 = a.M;
-            Diagnostic(ErrorCode.ERR_CannotInferDelegateType, "a.M").WithLocation(4, 10),
+            Diagnostic(ErrorCode.ERR_MethDelegateMismatch, "M").WithArguments("M", "System.Func<int, int>").WithLocation(4, 12),


Note that type inference successfully infers the delegate type to be Func<int, int> but then binding fails (because the span conversion doesn't exist for extension method receiver). I'm not sure if this is a problem, I think this could happen for some existing scenarios as well.

jjonescz · 2024-08-08T05:40:43Z

@cston @333fred for reviews, thanks

333fred

Mostly LGTM. Couple of minor comments.

333fred · 2024-08-08T17:21:34Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

+
+        private static TypeWithAnnotations GetSpanElementType(TypeSymbol type)
+        {
+            return ((NamedTypeSymbol)type).TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];


Consider assert that type is Span or ROS. #Resolved

333fred · 2024-08-08T17:45:37Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+        var comp = CreateCompilationWithSpanAndMemoryExtensions(source, parseOptions: TestOptions.Regular13);
+        CompileAndVerify(comp, expectedOutput: "1").VerifyDiagnostics();
+
+        var expectedOutput = ExecutionConditionUtil.IsCoreClr ? "3" : "2";


What exception is being thrown on CoreCLR? I assume something about variance? Consider leaving a comment. #Resolved

Consider reporting the exception type rather than "3", in this test and the next test.

333fred · 2024-08-08T18:05:33Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+            using System;
+            class C
+            {
+                void M(ReadOnlySpan<string> a, object b) => M1(a, b);


Let's test a scenario like this with a non-variant non-array conversion, like ROS<int> and object/vice versa; I expect the former to fail, and the latter to succeed. #Resolved

cston · 2024-08-10T17:57:00Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+
+        comp = CreateCompilationWithSpanAndMemoryExtensions(source);
+        CompileAndVerify(comp, expectedOutput: expectedOutput).VerifyDiagnostics();
+    }


}

Are we testing similar cases to these new tests, but with IEnumerable<T> and ReadOnlySpan<T>? #Closed

cston · 2024-08-10T17:59:23Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+            // (4,43): error CS0411: The type arguments for method 'C.M1<T>(Span<T[]>, T)' cannot be inferred from the usage. Try specifying the type arguments explicitly.
+            //     void M(Span<string[]> a, object b) => M1(a, b);
+            Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "M1").WithArguments("C.M1<T>(System.Span<T[]>, T)").WithLocation(4, 43));
+    }


}

Consider testing with void M2(Span<object[]> a, string b) => M1(a, b) as well. #Closed

cston · 2024-08-10T18:02:28Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+            """;
+        var comp = CreateCompilationWithSpanAndMemoryExtensions(source).VerifyDiagnostics();
+        AssertEx.Equal("C.M1<System.Object>", DisplayInvokedMethodTypeArguments(comp));
+    }


}

Please test:

void M2(int[] a, object b) => M1(a, b); void M3(object[] a, int b) => M1(a, b); ``` #Closed

cston · 2024-08-10T18:08:41Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+            // (4,36): error CS0411: The type arguments for method 'C.M1<T>(Span<T>)' cannot be inferred from the usage. Try specifying the type arguments explicitly.
+            //     void M(ReadOnlySpan<int> a) => M1(a);
+            Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "M1").WithArguments("C.M1<T>(System.Span<T>)").WithLocation(4, 36));
+    }


}

Are we testing inferring ReadOnlySpan<T> from call with Span<U>?

void M(Span<string> a, object b) => M1(a, b); void M1<T>(ReadOnlySpan<T> a, T b) { } ``` #Resolved

cston · 2024-08-10T18:28:26Z

...ortable/CodeRefactorings/UseRecursivePatterns/UseRecursivePatternsCodeRefactoringProvider.cs

@@ -600,7 +600,7 @@ static bool CanConvertToSubpattern(IdentifierNameSyntax name, SemanticModel mode

        // Process all nodes to refactor in reverse to ensure nested nodes
        // are processed before the outer nodes to refactor.
-        foreach (var originalNode in nodes.Reverse())
+        foreach (var originalNode in Enumerable.Reverse(nodes))


Enumerable.Reverse(nodes)

Why was this change needed? Are we now binding nodes.Reverse() to MemoryExtensions.Reverse<T>(Span<T>)? And if so, is that an issue? (It looks like the array that is created from var nodes = ... ToArray(); should not include array variance.) #Resolved

Are we now binding nodes.Reverse() to MemoryExtensions.Reverse<T>(Span<T>)?

Yes.

And if so, is that an issue?

Yes, because MemoryExtensions.Reverse returns void.

cston · 2024-08-10T18:38:24Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

@@ -105,14 +105,10 @@ private static string Concat(string container, string name)
                }
            }
            """;
-        internal const string s_collectionExtensionsWithSpan = s_collectionExtensions +
+        internal const string s_collectionExtensionsWithReadOnlySpan = s_collectionExtensions +


s_collectionExtensionsWithReadOnlySpan

Alternatively, could we leave the name unchanged, and simply remove the Report<T>(this in Span<T>) overload? Would that result in many additional test changes? #Closed

Not many additional changes, I can do that.

cston · 2024-08-10T21:29:24Z

src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs

+            // (4,36): error CS0411: The type arguments for method 'C.M1<T>(Span<T>)' cannot be inferred from the usage. Try specifying the type arguments explicitly.
+            //     void M(ReadOnlySpan<int> a) => M1(a);
+            Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "M1").WithArguments("C.M1<T>(System.Span<T>)").WithLocation(4, 36));
+    }


}

Consider testing variance when the array and span are the type arguments. For instance, with the following, for the calls with I<object[]> and IOut<object[]> I'd expect type inference to succeed but the subsequent argument conversion to fail, and for the call with IIn<object[]>, I'd expect type inference to fail.

interface I<T> where T : allows ref struct { } class A { static void M(I<object[]> a) => M1(a); static void M1<T>(I<ReadOnlySpan<T>> x) { } } interface IOut<out T> where T : allows ref struct { } class B { static void M(IOut<object[]> a) => M1(a); static void M1<T>(IOut<ReadOnlySpan<T>> x) { } } interface IIn<in T> where T : allows ref struct { } class C { static void M(IIn<object[]> a) => M1(a); static void M1<T>(IIn<ReadOnlySpan<T>> x) { } } ``` #Closed

jjonescz added the Feature - First-class Span Types label Aug 5, 2024

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 5, 2024

jjonescz commented Aug 5, 2024

View reviewed changes

jjonescz force-pushed the FirstClassSpan-14-TypeInference branch from f408180 to 733cb63 Compare August 5, 2024 14:47

Add span type inference

ede4c07

jjonescz force-pushed the FirstClassSpan-14-TypeInference branch from 733cb63 to ede4c07 Compare August 5, 2024 15:02

dotnet-policy-service bot added the VSCode label Aug 5, 2024

jjonescz removed the VSCode label Aug 5, 2024

build-analysis bot mentioned this pull request Aug 5, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jjonescz marked this pull request as ready for review August 5, 2024 17:03

jjonescz requested review from a team as code owners August 5, 2024 17:03

jjonescz requested review from 333fred and cston August 5, 2024 17:03

333fred reviewed Aug 8, 2024

View reviewed changes

cston reviewed Aug 10, 2024

View reviewed changes

jjonescz added 2 commits August 12, 2024 13:21

Simplify collection expression test helpers

f192ca6

Improve tests

611329d

dotnet-policy-service bot added the VSCode label Aug 12, 2024

jjonescz removed the VSCode label Aug 12, 2024

cston approved these changes Aug 12, 2024

View reviewed changes

333fred approved these changes Aug 12, 2024

View reviewed changes

jjonescz merged commit dd8f133 into dotnet:features/FirstClassSpan Aug 13, 2024
28 checks passed

jjonescz deleted the FirstClassSpan-14-TypeInference branch August 13, 2024 07:48

This was referenced Oct 22, 2024

[Automated] PRs inserted in VS build main-35421.103 #75574

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35424.209 #75628

Closed

dotnet-bot mentioned this pull request Oct 29, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35428.160 #75657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add span type inference #74646

Add span type inference #74646

jjonescz commented Aug 5, 2024

jjonescz Aug 5, 2024

jjonescz commented Aug 8, 2024

333fred left a comment

333fred Aug 8, 2024 •

edited by jjonescz

Loading

333fred Aug 8, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited

Loading

333fred Aug 8, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited by jjonescz

Loading

jjonescz Aug 12, 2024

cston Aug 10, 2024 •

edited

Loading

jjonescz Aug 12, 2024

cston Aug 10, 2024 •

edited

Loading

Add span type inference #74646

Add span type inference #74646

Conversation

jjonescz commented Aug 5, 2024

jjonescz Aug 5, 2024

Choose a reason for hiding this comment

jjonescz commented Aug 8, 2024

333fred left a comment

Choose a reason for hiding this comment

333fred Aug 8, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

333fred Aug 8, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

333fred Aug 8, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Aug 12, 2024

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

jjonescz Aug 12, 2024

Choose a reason for hiding this comment

cston Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

333fred Aug 8, 2024 •

edited by jjonescz

Loading

333fred Aug 8, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited

Loading

333fred Aug 8, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited by jjonescz

Loading

cston Aug 10, 2024 •

edited

Loading

cston Aug 10, 2024 •

edited

Loading