Skip to content
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.

Sign up for GitHub

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

Switch implicit span to conversion from type #74232

Merged
merged 4 commits into from
Jul 11, 2024
Merged

Switch implicit span to conversion from type #74232

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
45d6ee4
Switch implicit span to conversion from type
jjonescz Jun 27, 2024
6f87b19
Revert bootstrap build break mitigation
jjonescz Jun 27, 2024
4161bd1
Improve code and tests
jjonescz Jul 3, 2024
caa2777
Simplify code
jjonescz Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,20 +486,56 @@ void checkConstraintLanguageVersionAndRuntimeSupportForConversion(SyntaxNode syn
}
else if (conversion.IsSpan)
{
Debug.Assert(destination.OriginalDefinition.IsSpan() || destination.OriginalDefinition.IsReadOnlySpan());

CheckFeatureAvailability(syntax, MessageID.IDS_FeatureFirstClassSpan, diagnostics);

// 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.
Copy link
Member

@333fred 333fred Jul 2, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (TryFindImplicitOperatorFromArray(destination.OriginalDefinition) is { } method)
{
_ = GetWellKnownTypeMember(WellKnownMember.System_ReadOnlySpan_T__op_Implicit_Array, diagnostics, syntax: syntax);
diagnostics.ReportUseSite(method, syntax);
}
else
{
Debug.Assert(destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions));
_ = GetWellKnownTypeMember(WellKnownMember.System_Span_T__op_Implicit_Array, diagnostics, syntax: syntax);
Error(diagnostics,
ErrorCode.ERR_MissingPredefinedMember,
syntax,
destination.OriginalDefinition,
WellKnownMemberNames.ImplicitConversionName);
}
}
}
}

internal static MethodSymbol? TryFindImplicitOperatorFromArray(TypeSymbol type)
Copy link
Member

@333fred 333fred Jul 2, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
Copy link
Member

@cston cston Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{

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

var members = type.GetMembers(WellKnownMemberNames.ImplicitConversionName);
MethodSymbol? result = null;
foreach (var member in members)
{
if (member is MethodSymbol
{
ParameterCount: 1,
Arity: 0,
IsStatic: true,
DeclaredAccessibility: Accessibility.Public,
Parameters: [{ Type: ArrayTypeSymbol { IsSZArray: true, ElementType: TypeParameterSymbol } }]
} method)
{
if (result is not null)
{
// Ambiguous method found.
return null;
}

result = method;
}
}

return result;
}

private static void CheckInlineArrayTypeIsSupported(SyntaxNode syntax, TypeSymbol inlineArrayType, TypeSymbol elementType, BindingDiagnosticBag diagnostics)
{
if (elementType.IsPointerOrFunctionPointer() || elementType.IsRestrictedType())
Expand Down
47 changes: 24 additions & 23 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,6 @@ private static bool IsStandardImplicitConversionFromExpression(ConversionKind ki
case ConversionKind.StackAllocToPointerType:
case ConversionKind.StackAllocToSpanType:
case ConversionKind.InlineArray:
case ConversionKind.ImplicitSpan:
return true;
default:
return false;
Expand All @@ -624,6 +623,7 @@ private static bool IsStandardImplicitConversionFromType(ConversionKind kind)
case ConversionKind.ImplicitPointer:
case ConversionKind.ImplicitPointerToVoid:
case ConversionKind.ImplicitTuple:
case ConversionKind.ImplicitSpan:
return true;
default:
return false;
Expand Down Expand Up @@ -743,6 +743,11 @@ Conversion classifyConversion(TypeSymbol source, TypeSymbol destination, ref Com
return tupleConversion;
}

if (HasImplicitSpanConversion(source, destination, ref useSiteInfo))
{
return Conversion.ImplicitSpan;
}

return Conversion.NoConversion;
}
}
Expand Down Expand Up @@ -912,6 +917,10 @@ private Conversion DeriveStandardExplicitFromOppositeStandardImplicitConversion(

break;

case ConversionKind.ImplicitSpan:
impliedExplicitConversion = Conversion.NoConversion;
break;

default:
throw ExceptionUtilities.UnexpectedValue(oppositeConversion.Kind);
}
Expand Down Expand Up @@ -1021,11 +1030,6 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi
return Conversion.ImplicitDynamic;
}

if (HasImplicitSpanConversion(source, destination, ref useSiteInfo))
{
return Conversion.ImplicitSpan;
}

// The following conversions only exist for certain form of expressions,
// if we have no expression none if them is applicable.
if (sourceExpression == null)
Expand Down Expand Up @@ -1909,8 +1913,7 @@ public Conversion ConvertExtensionMethodThisArg(TypeSymbol parameterType, TypeSy
public Conversion ClassifyImplicitExtensionMethodThisArgConversion(BoundExpression sourceExpressionOpt, TypeSymbol sourceType, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
Debug.Assert(sourceExpressionOpt is null || Compilation is not null);
// PROTOTYPE: Revert `TypeSymbol.Equals(...)` to `(object)sourceExpressionOpt.Type == sourceType` when implicit span is a "conversion from type".
Debug.Assert(sourceExpressionOpt == null || TypeSymbol.Equals(sourceExpressionOpt.Type, sourceType, TypeCompareKind.ConsiderEverything));
Debug.Assert(sourceExpressionOpt == null || (object)sourceExpressionOpt.Type == sourceType);
Debug.Assert((object)destination != null);

if ((object)sourceType != null)
Expand Down Expand Up @@ -3931,8 +3934,8 @@ private static bool IsIntegerTypeSupportingPointerConversions(TypeSymbol type)
#nullable enable
private bool HasImplicitSpanConversion(TypeSymbol? source, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
// 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)
Copy link
Member

@333fred 333fred Jul 2, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jjonescz jjonescz Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return false;
}
Expand All @@ -3941,14 +3944,14 @@ private bool HasImplicitSpanConversion(TypeSymbol? source, TypeSymbol destinatio
if (source is ArrayTypeSymbol { IsSZArray: true, ElementTypeWithAnnotations: { } elementType })
{
// SPEC: ...to `System.Span<Ei>`.
if (destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions))
if (destination.OriginalDefinition.IsSpan())
{
var spanElementType = ((NamedTypeSymbol)destination).TypeArgumentsWithDefinitionUseSiteDiagnostics(ref useSiteInfo)[0];
return hasIdentityConversion(elementType, spanElementType);
}

// SPEC: ...to `System.ReadOnlySpan<Ui>`, provided that `Ei` is covariance-convertible to `Ui`.
if (destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions))
if (destination.OriginalDefinition.IsReadOnlySpan())
{
var spanElementType = ((NamedTypeSymbol)destination).TypeArgumentsWithDefinitionUseSiteDiagnostics(ref useSiteInfo)[0];
return hasCovariantConversion(elementType, spanElementType, ref useSiteInfo);
Expand All @@ -3975,8 +3978,8 @@ bool hasIdentityConversion(TypeWithAnnotations source, TypeWithAnnotations desti
/// </remarks>
private bool HasExplicitSpanConversion(TypeSymbol? source, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
// 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)
{
return false;
}
Expand All @@ -3985,8 +3988,7 @@ private bool HasExplicitSpanConversion(TypeSymbol? source, TypeSymbol destinatio
// to `System.Span<Ui>` or `System.ReadOnlySpan<Ui>`
// provided an explicit reference conversion exists from `Ti` to `Ui`.
if (source is ArrayTypeSymbol { IsSZArray: true, ElementTypeWithAnnotations: { } elementType } &&
(destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions) ||
destination.OriginalDefinition.Equals(Compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)))
(destination.OriginalDefinition.IsSpan() || destination.OriginalDefinition.IsReadOnlySpan()))
{
var spanElementType = ((NamedTypeSymbol)destination).TypeArgumentsWithDefinitionUseSiteDiagnostics(ref useSiteInfo)[0];
return HasIdentityOrReferenceConversion(elementType.Type, spanElementType.Type, ref useSiteInfo) &&
Expand All @@ -4003,18 +4005,17 @@ private bool IgnoreUserDefinedSpanConversions(TypeSymbol? source, TypeSymbol? ta
return false;
}

// PROTOTYPE: Is it fine that this check is not performed when Compilation is null?
return Compilation?.IsFeatureEnabled(MessageID.IDS_FeatureFirstClassSpan) == true &&
(ignoreUserDefinedSpanConversionsInOneDirection(Compilation, source, target) ||
ignoreUserDefinedSpanConversionsInOneDirection(Compilation, target, source));
// Note: when Compilation is null, we assume latest LangVersion.
return Compilation?.IsFeatureEnabled(MessageID.IDS_FeatureFirstClassSpan) != false &&
Copy link
Member

@cston cston Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(ignoreUserDefinedSpanConversionsInOneDirection(source, target) ||
ignoreUserDefinedSpanConversionsInOneDirection(target, source));

static bool ignoreUserDefinedSpanConversionsInOneDirection(CSharpCompilation compilation, TypeSymbol a, TypeSymbol b)
static bool ignoreUserDefinedSpanConversionsInOneDirection(TypeSymbol a, TypeSymbol b)
{
// SPEC: User-defined conversions are not considered when converting between
// SPEC: - any single-dimensional `array_type` and `System.Span<T>`/`System.ReadOnlySpan<T>`
if (a is ArrayTypeSymbol { IsSZArray: true } &&
(b.OriginalDefinition.Equals(compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions) ||
b.OriginalDefinition.Equals(compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)))
(b.OriginalDefinition.IsSpan() || b.OriginalDefinition.IsReadOnlySpan()))
{
return true;
}
Expand Down
9 changes: 3 additions & 6 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7909,10 +7909,10 @@ private Conversion GenerateConversionForConditionalOperator(BoundExpression sour
return conversion;
}

private Conversion GenerateConversion(Conversions conversions, BoundExpression? sourceExpression, TypeSymbol? sourceType, TypeSymbol destinationType, bool fromExplicitCast, bool extensionMethodThisArgument, bool isChecked, bool forceFromExpression = false)
private Conversion GenerateConversion(Conversions conversions, BoundExpression? sourceExpression, TypeSymbol? sourceType, TypeSymbol destinationType, bool fromExplicitCast, bool extensionMethodThisArgument, bool isChecked)
{
var discardedUseSiteInfo = CompoundUseSiteInfo<AssemblySymbol>.Discarded;
bool useExpression = forceFromExpression || sourceType is null || UseExpressionForConversion(sourceExpression);
bool useExpression = sourceType is null || UseExpressionForConversion(sourceExpression);
if (extensionMethodThisArgument)
{
return conversions.ClassifyImplicitExtensionMethodThisArgConversion(
Expand Down Expand Up @@ -8903,10 +8903,7 @@ private TypeWithState VisitConversion(
if (checkConversion)
{
var previousKind = conversion.Kind;
conversion = GenerateConversion(_conversions, conversionOperand, operandType.Type, targetType, fromExplicitCast, extensionMethodThisArgument, isChecked: conversionOpt?.Checked ?? false,
// Span conversion is "from expression".
// PROTOTYPE: Should it be "from type" instead?
forceFromExpression: true);
conversion = GenerateConversion(_conversions, conversionOperand, operandType.Type, targetType, fromExplicitCast, extensionMethodThisArgument, isChecked: conversionOpt?.Checked ?? false);
Debug.Assert(!conversion.Exists || conversion.Kind == previousKind);
canConvertNestedNullability = conversion.Exists;
}
Expand Down
17 changes: 3 additions & 14 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,28 +619,17 @@ private BoundExpression MakeConversionNodeCore(
{
var spanType = (NamedTypeSymbol)rewrittenType;

WellKnownMember member;
if (spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions))
{
member = WellKnownMember.System_ReadOnlySpan_T__op_Implicit_Array;
}
else
{
Debug.Assert(spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.AllIgnoreOptions));
member = WellKnownMember.System_Span_T__op_Implicit_Array;
}

if (!TryGetWellKnownTypeMember(rewrittenOperand.Syntax, member, out MethodSymbol? symbol))
if (Binder.TryFindImplicitOperatorFromArray(spanType.OriginalDefinition) is not { } methodDefinition)
{
throw ExceptionUtilities.Unreachable();
}
else
{
MethodSymbol method = symbol.AsMember(spanType);
MethodSymbol method = methodDefinition.AsMember(spanType);

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

if (member == WellKnownMember.System_ReadOnlySpan_T__op_Implicit_Array)
if (_compilation.IsReadOnlySpanType(spanType))
Copy link
Member

@cston cston Jul 8, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_compilation.IsReadOnlySpanType(spanType)

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return new BoundReadOnlySpanFromArray(syntax, rewrittenOperand, method, spanType) { WasCompilerGenerated = true };
}
Expand Down
22 changes: 22 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,6 +1338,28 @@ internal static bool IsValidV6SwitchGoverningType(this TypeSymbol type, bool isT
return false;
}

internal static bool IsSpan(this TypeSymbol type)
{
return type is NamedTypeSymbol
{
Name: "Span",
Arity: 1,
ContainingType: null,
ContainingNamespace: { Name: nameof(System), ContainingNamespace.IsGlobalNamespace: true },
};
}

internal static bool IsReadOnlySpan(this TypeSymbol type)
{
return type is NamedTypeSymbol
{
Name: "ReadOnlySpan",
Arity: 1,
ContainingType: null,
ContainingNamespace: { Name: nameof(System), ContainingNamespace.IsGlobalNamespace: true },
};
}

internal static bool IsSpanChar(this TypeSymbol type)
{
return type is NamedTypeSymbol
Expand Down
Loading
Loading