Switch implicit span to conversion from type #74232
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| // 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal static MethodSymbol? TryFindImplicitOperatorFromArray(TypeSymbol type) |
There was a problem hiding this comment.
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.
| comp.VerifyDiagnostics( | ||
| // (2,15): error CS0656: Missing compiler required member 'System.Span`1.op_Implicit' | ||
| [Fact] | ||
| public void Conversion_Array_Span_Implicit_DifferentOperator() |
There was a problem hiding this comment.
Perhaps a version where the operator is explicit?
| } | ||
|
|
||
| [Fact] | ||
| public void Conversion_Array_Span_Implicit_UnrecognizedModreq() |
There was a problem hiding this comment.
Let's add a test with an unrecognized modopt as well.
|
@cston for the second review, thanks |
Test plan: #73445
Corresponding speclet update: dotnet/csharplang#8241