Switch implicit span to conversion from type #74232

jjonescz · 2024-07-02T08:14:34Z

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

333fred · 2024-07-02T17:02:43Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

-            // PROTOTYPE: Is it fine that this conversion does not exists when Compilation is null?
-            if (Compilation?.IsFeatureEnabled(MessageID.IDS_FeatureFirstClassSpan) != true)
+            // Note: when Compilation is null, we assume latest LangVersion.
+            if (Compilation?.IsFeatureEnabled(MessageID.IDS_FeatureFirstClassSpan) == false)


A little worried about this. Do we have an understanding of when Compilation is null, such that we can test those scenarios with lower language versions? Will they get different behavior, or new language version errors? #Resolved

So far I've been able to hit only one scenario with Compilation being null - from IMethodSymbol.ReduceExtensionMethod public API - see e.g., test Conversion_Array_Span_ExtensionMethodReceiver_Implicit_Reduced_01. And the documentation says we should consider the latest language version in that case (it talks about constraints but it seems consistent to do for other features as well):

/// <param name="compilation">The compilation in which constraints should be checked. /// Should not be null, but if it is null we treat constraints as we would in the latest /// language version.</param>

Maybe other scenarios (not just reduced extension methods) should behave similarly? But there might be no more scenarios, I have investigated callsites for a while and haven't found any that I could hit.

Will they get different behavior, or new language version errors?

If they end up in the binder, they will get a language version error. Otherwise, they will get different behavior - like the ReduceExtensionMethod example mentioned above - the call will consider the implicit span conversion even in older lang version.

333fred · 2024-07-02T17:08:46Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

                    // PROTOTYPE: Check runtime APIs used for other span conversions once they are implemented.
-                    if (destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions))
+                    // NOTE: Span conversions do not use well-known types because they are "conversions from type" and hence don't have access to Compilation.


Not quite how I'd word this comment; the lack of a Compilation in ConversionsBase is just an implementation detail. The reason is that we explicitly specified that we match (ReadOnly)Span by type name, so we need to use that same (ReadOnly)Span to look up APIs in. #Resolved

333fred · 2024-07-02T17:10:58Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

                    }
                }
            }
        }

+        internal static MethodSymbol? TryFindImplicitOperatorFromArray(TypeSymbol type)


It may be worth extracting this as a TryFindSingleMember method that takes a filter delegate, since I assume we'll need the same logic for looking up the ReadOnlySpan->ReadOnlySpan converter method. Up to you though. #Resolved

333fred · 2024-07-02T17:13:11Z

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

-        comp.VerifyDiagnostics(
-            // (2,15): error CS0656: Missing compiler required member 'System.Span`1.op_Implicit'
+    [Fact]
+    public void Conversion_Array_Span_Implicit_DifferentOperator()


Perhaps a version where the operator is explicit? #Resolved

333fred · 2024-07-02T17:15:06Z

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

+    }
+
+    [Fact]
+    public void Conversion_Array_Span_Implicit_UnrecognizedModreq()


Let's add a test with an unrecognized modopt as well. #Resolved

jjonescz · 2024-07-03T08:48:44Z

@cston for the second review, thanks

jjonescz · 2024-07-08T10:02:32Z

@cston @333fred for reviews, thanks

cston · 2024-07-08T21:10:03Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

+        }
+
+        internal static MethodSymbol? TryFindImplicitOperatorFromArray(TypeSymbol type)
+        {


{

Consider adding Debug.Assert(type.IsSpan() || type.IsReadOnlySpan()); #Closed

cston · 2024-07-08T21:13:29Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

+                } method ? method : null);
+        }
+
+        private static T? TryFindSingleMember<T>(TypeSymbol type, string name, Func<Symbol, T?> predicate) where T : class


Is the type parameter necessary, or could this be simplified to TryFindSingleMethod with MethodSymbol directly? #Closed

cston · 2024-07-08T21:36:45Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

-                (ignoreUserDefinedSpanConversionsInOneDirection(Compilation, source, target) ||
-                ignoreUserDefinedSpanConversionsInOneDirection(Compilation, target, source));
+            // Note: when Compilation is null, we assume latest LangVersion.
+            return Compilation?.IsFeatureEnabled(MessageID.IDS_FeatureFirstClassSpan) != false &&


This check is repeated several times, with the same comment. Consider creating a private IsFirstClassSpansEnabled property so this is captured in one location. #Closed

cston · 2024-07-08T21:40:33Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs


                            rewrittenOperand = _factory.Convert(method.ParameterTypesWithAnnotations[0].Type, rewrittenOperand);

-                            if (member == WellKnownMember.System_ReadOnlySpan_T__op_Implicit_Array)
+                            if (_compilation.IsReadOnlySpanType(spanType))


_compilation.IsReadOnlySpanType(spanType)

This uses WellKnownMember.System_ReadOnlySpan_T rather than checking by fully-qualified type name. Is that correct? #Resolved

That was my intent - I even have a test for it: Conversion_Array_Span_Implicit_ConstantData_NotWellKnownSpan. The emit of constant data for ReadOnlySpan (i.e., emit of BoundReadOnlySpanFromArray) expects the well-known span type and I don't think changing that is worth the effort.

cston · 2024-07-08T22:59:19Z

    CreateCompilationWithSpan(source, parseOptions: TestOptions.Regular12).VerifyDiagnostics(

[This comment is not specific to this PR, so it can be addressed separately.]

CreateCompilationWithSpan() is always using the local type definitions for Span<T> and ReadOnlySpan<T>. On .NET core, we should compile against the BCL implementation instead, to ensure we're testing against shipping APIs. Consider using CreateCompilationWithSpanAndMemoryExtensions() which covers both cases. #Pending

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:58 in 4161bd1. [](commit_id = 4161bd1, deletion_comment = False)

jjonescz · 2024-07-09T11:57:18Z

    CreateCompilationWithSpan(source, parseOptions: TestOptions.Regular12).VerifyDiagnostics(

Sounds good, thanks! Let me do that after I merge main into the feature branch so I get #74281 in.

In reply to: 2215489010

Refers to: src/Compilers/CSharp/Test/Emit3/FirstClassSpanTests.cs:58 in 4161bd1. [](commit_id = 4161bd1, deletion_comment = False)

jjonescz · 2024-07-10T06:48:52Z

@333fred for another look, thanks

jjonescz added 2 commits July 2, 2024 10:10

Switch implicit span to conversion from type

45d6ee4

Revert bootstrap build break mitigation

6f87b19

jjonescz added the Feature - First-class Span Types label Jul 2, 2024

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

jjonescz marked this pull request as ready for review July 2, 2024 10:22

jjonescz requested review from a team as code owners July 2, 2024 10:22

jjonescz requested review from 333fred and cston July 2, 2024 10:23

build-analysis bot mentioned this pull request Jul 2, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

333fred reviewed Jul 2, 2024

View reviewed changes

Improve code and tests

4161bd1

jjonescz requested a review from 333fred July 3, 2024 08:48

cston reviewed Jul 8, 2024

View reviewed changes

Simplify code

caa2777

jjonescz requested a review from cston July 9, 2024 12:01

cston approved these changes Jul 9, 2024

View reviewed changes

333fred approved these changes Jul 10, 2024

View reviewed changes

jjonescz merged commit 7997421 into dotnet:features/FirstClassSpan Jul 11, 2024
28 checks passed

jjonescz deleted the FirstClassSpan-08-ConversionFromType branch July 11, 2024 08:44

jjonescz mentioned this pull request Jul 15, 2024

Improve span conversion tests #74387

Merged

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

Switch implicit span to conversion from type #74232

Switch implicit span to conversion from type #74232

jjonescz commented Jul 2, 2024

333fred Jul 2, 2024 •

edited by jjonescz

Loading

jjonescz Jul 3, 2024 •

edited

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

jjonescz commented Jul 3, 2024

jjonescz commented Jul 8, 2024

cston Jul 8, 2024 •

edited

Loading

cston Jul 8, 2024 •

edited

Loading

cston Jul 8, 2024 •

edited

Loading

cston Jul 8, 2024 •

edited by jjonescz

Loading

jjonescz Jul 9, 2024

cston commented Jul 8, 2024 •

edited by jjonescz

Loading

jjonescz commented Jul 9, 2024

jjonescz commented Jul 10, 2024

Switch implicit span to conversion from type #74232

Switch implicit span to conversion from type #74232

Conversation

jjonescz commented Jul 2, 2024

333fred Jul 2, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

333fred Jul 2, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

333fred Jul 2, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

333fred Jul 2, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

333fred Jul 2, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz commented Jul 3, 2024

jjonescz commented Jul 8, 2024

cston Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

cston Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

cston Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

cston Jul 8, 2024 • edited by jjonescz Loading

Choose a reason for hiding this comment

jjonescz Jul 9, 2024

Choose a reason for hiding this comment

cston commented Jul 8, 2024 • edited by jjonescz Loading

jjonescz commented Jul 9, 2024

jjonescz commented Jul 10, 2024

333fred Jul 2, 2024 •

edited by jjonescz

Loading

jjonescz Jul 3, 2024 •

edited

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

333fred Jul 2, 2024 •

edited by jjonescz

Loading

cston Jul 8, 2024 •

edited

Loading

cston Jul 8, 2024 •

edited

Loading

cston Jul 8, 2024 •

edited

Loading

cston Jul 8, 2024 •

edited by jjonescz

Loading

cston commented Jul 8, 2024 •

edited by jjonescz

Loading