Support ref struct implementing interfaces in foreach and using statements
#72810
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
1 similar comment
|
|
||
| if (unwrappedCollectionExprType.IsRefLikeType || unwrappedCollectionExprType is TypeParameterSymbol { AllowByRefLike: true }) | ||
| { | ||
| builder.CollectionType = unwrappedCollectionExprType; |
There was a problem hiding this comment.
Should the spec-let update the foreach statement definition of collection type for ref struct and allows ref struct cases?
There was a problem hiding this comment.
builder.CollectionType
Should the spec-let update the definition of collection type for
ref structandallows ref structcases?
Could you elaborate please? What are you suggesting to modify and where?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Should dotnet/csharplang#8024 include a section that updates the foreach statement definition to indicate the expected collection type, etc. for
ref structcases and type parameters withallows ref struct? Currently, the existing spec section seems to indicate the collection type will beIEnumerable<T>orIEnumerable. Are those correct, given how collection type, etc. is used in theforeachexpansion 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.
| @@ -1482,6 +1484,13 @@ public BoundExpression Convert(TypeSymbol type, BoundExpression arg) | |||
| CompoundUseSiteInfo<AssemblySymbol>.Discarded; | |||
| #endif | |||
| Conversion c = Compilation.Conversions.ClassifyConversionFromExpression(arg, type, isChecked: false, ref useSiteInfo); | |||
|
|
|||
| if (allowBoxingByRefLikeTypeParametersToObject && !c.Exists && | |||
There was a problem hiding this comment.
allowBoxingByRefLikeTypeParametersToObject
When is it valid to allowing boxing?
// The code boxes type parameter that allows ref struct, however 'box' followed by 'brfalse' is
// documented as a valid sequence at https://github.com/dotnet/runtime/blob/main/docs/design/features/byreflike-generics.md#special-il-sequences
//
// IL_000b: ldloc.0
// IL_000c: box ""T""
// IL_0011: brfalse.s IL_0020
| { | ||
| LazyAsyncMethodChecks(CancellationToken.None); | ||
| Debug.Assert(state.HasComplete(CompletionPart.FinishAsyncMethodChecks)); | ||
| } |
There was a problem hiding this comment.
Was this incorrect?
It was done too early. Apparently, for no good reason. Was causing a cycle because the check needs to access constraints, which this method is responsible to figure out in case of overriding.
|
|
||
| if (unwrappedCollectionExprType.IsRefLikeType || unwrappedCollectionExprType is TypeParameterSymbol { AllowByRefLike: true }) | ||
| { | ||
| builder.CollectionType = unwrappedCollectionExprType; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This feels fragile. We change
builder.CollectionTypebut below we continue to use the oldNamedTypeSymbol collectionType = (NamedTypeSymbol)builder.CollectionType;. Perhaps this wholeifcould 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.
AlekseyTs
merged commit b0251c2
into
dotnet:features/RefStructInterfaces
Corresponding spec update - dotnet/csharplang#8024