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 all 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
54 changes: 50 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 @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -486,18 +487,63 @@ 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: We cannot use well-known members because per the spec
// the Span types involved in the Span conversions can be any that match the type name.
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

Debug.Assert(type.IsSpan() || type.IsReadOnlySpan());

return TryFindSingleMember(type, WellKnownMemberNames.ImplicitConversionName,
static (method) => method is
{
ParameterCount: 1,
Arity: 0,
IsStatic: true,
DeclaredAccessibility: Accessibility.Public,
Parameters: [{ Type: ArrayTypeSymbol { IsSZArray: true, ElementType: TypeParameterSymbol } }]
});
}

private static MethodSymbol? TryFindSingleMember(TypeSymbol type, string name, Func<MethodSymbol, bool> predicate)
{
var members = type.GetMembers(name);
MethodSymbol? result = null;
foreach (var member in members)
{
if (member is MethodSymbol method && predicate(method))
{
if (result is not null)
{
// Ambiguous member found.
return null;
}

result = method;
}
}

return result;
}

private static void CheckInlineArrayTypeIsSupported(SyntaxNode syntax, TypeSymbol inlineArrayType, TypeSymbol elementType, BindingDiagnosticBag diagnostics)
Expand Down
59 changes: 31 additions & 28 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 @@ -3929,10 +3932,18 @@ private static bool IsIntegerTypeSupportingPointerConversions(TypeSymbol type)
}

#nullable enable
private bool IsFeatureFirstClassSpanEnabled
{
get
{
// Note: when Compilation is null, we assume latest LangVersion.
return Compilation?.IsFeatureEnabled(MessageID.IDS_FeatureFirstClassSpan) != false;
}
}

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)
if (!IsFeatureFirstClassSpanEnabled)
{
return false;
}
Expand All @@ -3941,14 +3952,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 +3986,7 @@ 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)
if (!IsFeatureFirstClassSpanEnabled)
{
return false;
}
Expand All @@ -3985,8 +3995,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 @@ -3998,23 +4007,17 @@ private bool HasExplicitSpanConversion(TypeSymbol? source, TypeSymbol destinatio

private bool IgnoreUserDefinedSpanConversions(TypeSymbol? source, TypeSymbol? target)
{
if (source is null || target is null)
{
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));
return source is not null && target is not null &&
IsFeatureFirstClassSpanEnabled &&
(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