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

Jump to bottom

Support ref struct implementing interfaces in foreach and using statements #72810

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Support ref struct implementing interfaces in foreach and using statements #72810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
870bf90
Support ref struct implementing interfaces in `foreach` and `using` s…
AlekseyTs Mar 29, 2024
4428dd6
More tests
AlekseyTs Mar 31, 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
34 changes: 17 additions & 17 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ private readonly struct MixableDestination

internal MixableDestination(ParameterSymbol parameter, BoundExpression argument)
{
Debug.Assert(parameter.RefKind.IsWritableReference() && parameter.Type.IsRefLikeType);
Debug.Assert(parameter.RefKind.IsWritableReference() && parameter.Type.IsRefLikeType); // PROTOTYPE(RefStructInterfaces): adjust?
Debug.Assert(GetParameterValEscapeLevel(parameter).HasValue);
Argument = argument;
Parameter = parameter;
Expand Down Expand Up @@ -1887,7 +1887,7 @@ bool isRefEscape
}

// check receiver if ref-like
if (receiver?.Type?.IsRefLikeType == true)
if (receiver?.Type?.IsRefLikeType == true) // PROTOTYPE(RefStructInterfaces): adjust?
{
escapeScope = Math.Max(escapeScope, GetValEscape(receiver, scopeOfTheContainingExpression));
}
Expand Down Expand Up @@ -1930,7 +1930,7 @@ private uint GetInvocationEscapeWithUpdatedRules(
// If `M()` does return ref-to-ref-struct, the *ref-safe-to-escape* is the narrowest *ref-safe-to-escape* contributed by all arguments which are ref-to-ref-struct.
//
if (!returnsRefToRefStruct
|| (param is null or { RefKind: not RefKind.None, Type.IsRefLikeType: true } && isArgumentRefEscape == isRefEscape))
|| (param is null or { RefKind: not RefKind.None, Type.IsRefLikeType: true } && isArgumentRefEscape == isRefEscape)) // PROTOTYPE(RefStructInterfaces): adjust?
{
uint argEscape = isArgumentRefEscape ?
GetRefEscape(argument, scopeOfTheContainingExpression) :
Expand Down Expand Up @@ -1959,7 +1959,7 @@ private static bool ReturnsRefToRefStruct(Symbol symbol)
PropertySymbol p => p.GetMethod,
_ => null
};
return method is { RefKind: not RefKind.None, ReturnType.IsRefLikeType: true };
return method is { RefKind: not RefKind.None, ReturnType.IsRefLikeType: true }; // PROTOTYPE(RefStructInterfaces): adjust?
}

/// <summary>
Expand Down Expand Up @@ -2054,7 +2054,7 @@ bool isRefEscape
}

// check receiver if ref-like
if (receiver?.Type?.IsRefLikeType == true)
if (receiver?.Type?.IsRefLikeType == true) // PROTOTYPE(RefStructInterfaces): adjust?
{
return CheckValEscape(receiver.Syntax, receiver, escapeFrom, escapeTo, false, diagnostics);
}
Expand Down Expand Up @@ -2100,7 +2100,7 @@ private bool CheckInvocationEscapeWithUpdatedRules(
// If `M()` does return ref-to-ref-struct, the *ref-safe-to-escape* is the narrowest *ref-safe-to-escape* contributed by all arguments which are ref-to-ref-struct.
//
if (!returnsRefToRefStruct
|| (param is null or { RefKind: not RefKind.None, Type.IsRefLikeType: true } && isArgumentRefEscape == isRefEscape))
|| (param is null or { RefKind: not RefKind.None, Type.IsRefLikeType: true } && isArgumentRefEscape == isRefEscape)) // PROTOTYPE(RefStructInterfaces): adjust?
{
bool valid = isArgumentRefEscape ?
CheckRefEscape(argument.Syntax, argument, escapeFrom, escapeTo, false, diagnostics) :
Expand Down Expand Up @@ -2222,7 +2222,7 @@ private void GetInvocationArgumentsForEscape(

static bool isMixableParameter([NotNullWhen(true)] ParameterSymbol? parameter) =>
parameter is not null &&
parameter.Type.IsRefLikeType &&
parameter.Type.IsRefLikeType && // PROTOTYPE(RefStructInterfaces): adjust?
parameter.RefKind.IsWritableReference();

static bool isMixableArgument(BoundExpression argument)
Expand Down Expand Up @@ -2346,9 +2346,9 @@ static bool hasRefLikeReturn(Symbol symbol)
return method.ContainingType.IsRefLikeType;
}

return method.ReturnType.IsRefLikeType;
return method.ReturnType.IsRefLikeType; // PROTOTYPE(RefStructInterfaces): adjust?
case PropertySymbol property:
return property.Type.IsRefLikeType;
return property.Type.IsRefLikeType; // PROTOTYPE(RefStructInterfaces): adjust?
default:
return false;
}
Expand Down Expand Up @@ -2408,15 +2408,15 @@ private void GetEscapeValuesForUpdatedRules(
escapeValues.Add(new EscapeValue(parameter: null, argument, EscapeLevel.ReturnOnly, isRefEscape: true));
}

if (argument.Type?.IsRefLikeType == true)
if (argument.Type?.IsRefLikeType == true) // PROTOTYPE(RefStructInterfaces): adjust?
{
escapeValues.Add(new EscapeValue(parameter: null, argument, EscapeLevel.CallingMethod, isRefEscape: false));
}

continue;
}

if (parameter.Type.IsRefLikeType && parameter.RefKind != RefKind.Out && GetParameterValEscapeLevel(parameter) is { } valEscapeLevel)
if (parameter.Type.IsRefLikeType && parameter.RefKind != RefKind.Out && GetParameterValEscapeLevel(parameter) is { } valEscapeLevel) // PROTOTYPE(RefStructInterfaces): adjust?
{
escapeValues.Add(new EscapeValue(parameter, argument, valEscapeLevel, isRefEscape: false));
}
Expand Down Expand Up @@ -2531,7 +2531,7 @@ private bool CheckInvocationArgMixing(

// collect all writeable ref-like arguments, including receiver
var receiverType = receiverOpt?.Type;
if (receiverType?.IsRefLikeType == true && !IsReceiverRefReadOnly(symbol))
if (receiverType?.IsRefLikeType == true && !IsReceiverRefReadOnly(symbol)) // PROTOTYPE(RefStructInterfaces): adjust?
{
escapeTo = GetValEscape(receiverOpt, scopeOfTheContainingExpression);
}
Expand Down Expand Up @@ -2562,7 +2562,7 @@ private bool CheckInvocationArgMixing(

if (refKind.IsWritableReference()
&& !argument.IsDiscardExpression()
&& argument.Type?.IsRefLikeType == true)
&& argument.Type?.IsRefLikeType == true) // PROTOTYPE(RefStructInterfaces): adjust?
{
escapeTo = Math.Min(escapeTo, GetValEscape(argument, scopeOfTheContainingExpression));
}
Expand Down Expand Up @@ -3789,7 +3789,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
}

// to have local-referring values an expression must have a ref-like type
if (expr.Type?.IsRefLikeType != true)
if (expr.Type?.IsRefLikeType != true) // PROTOTYPE(RefStructInterfaces): adjust?
{
return CallingMethodScope;
}
Expand Down Expand Up @@ -3874,7 +3874,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
var fieldAccess = (BoundFieldAccess)expr;
var fieldSymbol = fieldAccess.FieldSymbol;

if (fieldSymbol.IsStatic || !fieldSymbol.ContainingType.IsRefLikeType)
if (fieldSymbol.IsStatic || !fieldSymbol.ContainingType.IsRefLikeType) // PROTOTYPE(RefStructInterfaces): adjust?
{
// Already an error state.
return CallingMethodScope;
Expand Down Expand Up @@ -4371,7 +4371,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF
}

// to have local-referring values an expression must have a ref-like type
if (expr.Type?.IsRefLikeType != true)
if (expr.Type?.IsRefLikeType != true) // PROTOTYPE(RefStructInterfaces): adjust?
{
return true;
}
Expand Down Expand Up @@ -4474,7 +4474,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF
var fieldAccess = (BoundFieldAccess)expr;
var fieldSymbol = fieldAccess.FieldSymbol;

if (fieldSymbol.IsStatic || !fieldSymbol.ContainingType.IsRefLikeType)
if (fieldSymbol.IsStatic || !fieldSymbol.ContainingType.IsRefLikeType) // PROTOTYPE(RefStructInterfaces): adjust?
{
// Already an error state.
return true;
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2046,7 +2046,7 @@ private BoundExpression BindNonMethod(SimpleNameSyntax node, Symbol symbol, Bind
}
else
{
Debug.Assert(parameter.Type.IsRefLikeType);
Debug.Assert(parameter.Type.IsRefLikeType); // PROTOTYPE(RefStructInterfaces): adjust?
Error(diagnostics, ErrorCode.ERR_AnonDelegateCantUseRefLike, node, parameter.Name);
}
}
Expand All @@ -2069,7 +2069,7 @@ private BoundExpression BindNonMethod(SimpleNameSyntax node, Symbol symbol, Bind
}
else
{
Debug.Assert(parameter.Type.IsRefLikeType);
Debug.Assert(parameter.Type.IsRefLikeType); // PROTOTYPE(RefStructInterfaces): adjust?
Error(diagnostics, ErrorCode.ERR_UnsupportedPrimaryConstructorParameterCapturingRefLike, node, parameter.Name);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ private void CheckRestrictedTypeReceiver(BoundExpression expression, CSharpCompi
// error CS0029: Cannot implicitly convert type 'A' to 'B'

// Case 1: receiver is a restricted type, and method called is defined on a parent type
if (call.ReceiverOpt.Type.IsRestrictedType() && !TypeSymbol.Equals(call.Method.ContainingType, call.ReceiverOpt.Type, TypeCompareKind.ConsiderEverything2))
if (call.ReceiverOpt.Type.IsRestrictedType() && !call.Method.ContainingType.IsInterface && !TypeSymbol.Equals(call.Method.ContainingType, call.ReceiverOpt.Type, TypeCompareKind.ConsiderEverything2))
{
SymbolDistinguisher distinguisher = new SymbolDistinguisher(compilation, call.ReceiverOpt.Type, call.Method.ContainingType);
Error(diagnostics, ErrorCode.ERR_NoImplicitConv, call.ReceiverOpt.Syntax, distinguisher.First, distinguisher.Second);
Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,7 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
{
Debug.Assert(expr is object);
Debug.Assert(expr.Type is object);
// PROTOTYPE(RefStructInterfaces): adjust?
Debug.Assert(expr.Type.IsRefLikeType || hasAwait); // pattern dispose lookup is only valid on ref structs or asynchronous usings

var result = PerformPatternMethodLookup(expr,
Expand Down Expand Up @@ -1580,7 +1581,7 @@ private void ValidateAssignment(
leftEscape = GetValEscape(op1, _localScopeDepth);
rightEscape = GetValEscape(op2, _localScopeDepth);

Debug.Assert(leftEscape == rightEscape || op1.Type.IsRefLikeType);
Debug.Assert(leftEscape == rightEscape || op1.Type.IsRefLikeType); // PROTOTYPE(RefStructInterfaces): adjust?

// We only check if the safe-to-escape of e2 is wider than the safe-to-escape of e1 here,
// we don't check for equality. The case where the safe-to-escape of e2 is narrower than
Expand All @@ -1599,7 +1600,7 @@ private void ValidateAssignment(
}
}

if (!hasErrors && op1.Type.IsRefLikeType)
if (!hasErrors && op1.Type.IsRefLikeType) // PROTOTYPE(RefStructInterfaces): adjust?
{
var leftEscape = GetValEscape(op1, _localScopeDepth);
ValidateEscape(op2, leftEscape, isByRef: false, diagnostics);
Expand Down
51 changes: 39 additions & 12 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,12 @@ private EnumeratorResult SatisfiesIEnumerableInterfaces(SyntaxNode collectionSyn
Debug.Assert((object)builder.CollectionType != null);

NamedTypeSymbol collectionType = (NamedTypeSymbol)builder.CollectionType;

if (unwrappedCollectionExprType.IsRefLikeType || unwrappedCollectionExprType is TypeParameterSymbol { AllowByRefLike: true })
{
builder.CollectionType = unwrappedCollectionExprType;
Copy link
Member

@cston cston Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

builder.CollectionType

Should the spec-let update the foreach statement definition of collection type for ref struct and allows ref struct cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

builder.CollectionType

Should the spec-let update the definition of collection type for ref struct and allows ref struct cases?

Could you elaborate please? What are you suggesting to modify and where?

Copy link
Member

Choose a reason for hiding this comment

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

Should dotnet/csharplang#8024 include a section that updates the foreach statement definition to indicate the expected collection type, etc. for ref struct cases and type parameters with allows ref struct? Currently, the existing spec section seems to indicate the collection type will be IEnumerable<T> or IEnumerable. Are those correct, given how collection type, etc. is used in the foreach expansion and perhaps elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should dotnet/csharplang#8024 include a section that updates the foreach statement definition to indicate the expected collection type, etc. for ref struct cases and type parameters with allows ref struct? Currently, the existing spec section seems to indicate the collection type will be IEnumerable<T> or IEnumerable. Are those correct, given how collection type, etc. is used in the foreach expansion and perhaps elsewhere?

I am satisfied with how dotnet/csharplang#8024 specifies the expected behavior. I will mention that foreach statement will have to be adjusted accordingly. However, I prefer leaving this exercise for later and quite possibly for the one changing the official spec.

Copy link
Member

Choose a reason for hiding this comment

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

This feels fragile. We change builder.CollectionType but below we continue to use the old NamedTypeSymbol collectionType = (NamedTypeSymbol)builder.CollectionType;. Perhaps this whole if could be at the end of the method instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels fragile. We change builder.CollectionType but below we continue to use the old NamedTypeSymbol collectionType = (NamedTypeSymbol)builder.CollectionType;. Perhaps this whole if could be at the end of the method instead?

I do not find this too fragile. Also, there are early returns from the if below. Therefore, I prefer to leave the code as is.

}

if (collectionType.IsGenericType)
{
// If the type is generic, we have to search for the methods
Expand Down Expand Up @@ -1188,14 +1194,12 @@ private void GetDisposalInfoForEnumerator(SyntaxNode syntax, ref ForEachEnumerat
// is potentially disposable.

TypeSymbol enumeratorType = builder.GetEnumeratorInfo.Method.ReturnType;
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);

MethodSymbol patternDisposeMethod = null;
if (enumeratorType.IsRefLikeType || isAsync)
{
// we throw away any binding diagnostics, and assume it's not disposable if we encounter errors
var receiver = new BoundDisposableValuePlaceholder(syntax, enumeratorType);
patternDisposeMethod = TryFindDisposePatternMethod(receiver, syntax, isAsync, BindingDiagnosticBag.Discarded, out bool expanded);
MethodSymbol patternDisposeMethod = TryFindDisposePatternMethod(receiver, syntax, isAsync, BindingDiagnosticBag.Discarded, out bool expanded);
if (patternDisposeMethod is object)
{
Debug.Assert(!patternDisposeMethod.IsExtensionMethod);
Expand Down Expand Up @@ -1226,22 +1230,44 @@ private void GetDisposalInfoForEnumerator(SyntaxNode syntax, ref ForEachEnumerat
// We already checked feature availability for async scenarios
CheckFeatureAvailability(expr.Syntax, MessageID.IDS_FeatureDisposalPattern, diagnostics);
}

return;
}
}

if (!enumeratorType.IsRefLikeType && patternDisposeMethod is null)
if (implementsInterface(enumeratorType, isAsync, diagnostics))
{
// If it wasn't pattern-disposable, see if it's directly convertable to IDisposable
// For async foreach, we don't do the runtime check in unsealed case
if ((!enumeratorType.IsSealed && !isAsync) ||
this.Conversions.ClassifyImplicitConversionFromType(enumeratorType,
isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable),
ref useSiteInfo).IsImplicit)
builder.NeedsDisposal = true;
return;
}

if (!enumeratorType.IsSealed && !isAsync) // For async foreach, we don't do the runtime check in unsealed case
{
Debug.Assert(!enumeratorType.IsRefLikeType); // Ref like types are supposed to be structs, therefore, sealed.

if (enumeratorType is TypeParameterSymbol { AllowByRefLike: true })
{
Error(diagnostics, ErrorCode.ERR_BadAllowByRefLikeEnumerator, expr.Syntax, enumeratorType);
}
else
{
builder.NeedsDisposal = true;
}
}

bool implementsInterface(TypeSymbol enumeratorType, bool isAsync, BindingDiagnosticBag diagnostics)
{
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);

NamedTypeSymbol targetInterface = isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable);

bool result = this.Conversions.HasImplicitConversionToOrImplementsVarianceCompatibleInterface(enumeratorType,
targetInterface,
ref useSiteInfo);

diagnostics.Add(syntax, useSiteInfo);

return result;
}
}

Expand Down Expand Up @@ -1736,8 +1762,9 @@ private bool AllInterfacesContainsIEnumerable(
var implementedNonGeneric = this.Compilation.GetSpecialType(SpecialType.System_Collections_IEnumerable);
if ((object)implementedNonGeneric != null)
{
var conversion = this.Conversions.ClassifyImplicitConversionFromType(type, implementedNonGeneric, ref useSiteInfo);
if (conversion.IsImplicit)
var implements = this.Conversions.HasImplicitConversionToOrImplementsVarianceCompatibleInterface(type, implementedNonGeneric, ref useSiteInfo);

if (implements)
{
implementedIEnumerable = implementedNonGeneric;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ private void RemoveLocalScopes(LocalSymbol local)
Debug.Assert(localSymbol.RefKind == RefKind.None ||
refEscapeScope >= GetRefEscape(initializer, _localScopeDepth));

if (node.DeclaredTypeOpt?.Type.IsRefLikeType == true)
if (node.DeclaredTypeOpt?.Type.IsRefLikeType == true) // PROTOTYPE(RefStructInterfaces): adjust?
{
ValidateEscape(initializer, valEscapeScope, isByRef: false, _diagnostics);
}
Expand Down Expand Up @@ -563,7 +563,7 @@ private void RemoveLocalScopes(LocalSymbol local)

static uint getDeclarationValEscape(BoundTypeExpression typeExpression, uint valEscape)
{
return typeExpression.Type.IsRefLikeType ? valEscape : CallingMethodScope;
return typeExpression.Type.IsRefLikeType ? valEscape : CallingMethodScope; // PROTOTYPE(RefStructInterfaces): adjust?
}
}

Expand All @@ -588,7 +588,7 @@ static uint getPositionalValEscape(Symbol? symbol, uint valEscape)
{
return symbol is null
? valEscape
: symbol.GetTypeOrReturnType().IsRefLikeType() ? valEscape : CallingMethodScope;
: symbol.GetTypeOrReturnType().IsRefLikeType() ? valEscape : CallingMethodScope; // PROTOTYPE(RefStructInterfaces): adjust?
}
}

Expand All @@ -601,7 +601,7 @@ static uint getMemberValEscape(BoundPropertySubpatternMember? member, uint valEs
{
if (member is null) return valEscape;
valEscape = getMemberValEscape(member.Receiver, valEscape);
return member.Type.IsRefLikeType ? valEscape : CallingMethodScope;
return member.Type.IsRefLikeType ? valEscape : CallingMethodScope; // PROTOTYPE(RefStructInterfaces): adjust?
}
}

Expand Down
Loading