Warn if collection expression to ref struct type incurs a heap allocation #70264
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
RikkiGibson
force-pushed
the
warn-ref-struct-alloc
branch
from
551a908 to
3ca6fef
Compare
| @@ -98,16 +98,25 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr | |||
| collectionTypeKind == CollectionExpressionTypeKind.Span ? WellKnownType.System_Span_T : WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)); | |||
| Debug.Assert(elementType.Equals(spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0], TypeCompareKind.AllIgnoreOptions)); | |||
|
|
|||
| if (elements.Length == 0) | |||
There was a problem hiding this comment.
I felt it was worth basing this PR on #70260 because the presence of the optimization affects whether we want to give this warning.
| { | ||
| diagnostics.Add(node.HasSpreadElements(out _, out _) | ||
| ? ErrorCode.WRN_CollectionExpressionRefStructSpreadMayAllocate | ||
| : ErrorCode.WRN_CollectionExpressionRefStructMayAllocate, |
There was a problem hiding this comment.
Looks like if a user gets the former warning, then removes spreads they might still get the latter warning - isn't that confusing?
There was a problem hiding this comment.
It could def be inconvenient to address one warning and then have another warning pop up. However, I think giving a more precise answer here could require us to more deeply analyze the collection-expr, e.g. if the spread element were removed, would we be able to use the ROS optimization based on the type and other elements? Would we be able to use InlineArray?
The main goal of having multiple diagnostic IDs is to allow end users to choose a different policy for "allocating because of spreads" versus "allocating due to lack of runtime support". I think for the audience we expect to be consuming this warning, the current behavior is adequate.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
| comp.VerifyEmitDiagnostics( | ||
| // 0.cs(12,60): warning CS9208: Collection expression of type 'MyCollection<T>' may incur unexpected heap allocations. Consider explicitly creating an array, then converting to 'MyCollection<T>' to make the allocation explicit. |
There was a problem hiding this comment.
There was a problem hiding this comment.
Since the user-defined ref structs are always exposing some Create(ROS<...>) method, we know the ref struct can always be created from an array. You're right that MyCollectionBuilder.Create((T[])[x, y, z]) can be used in this case. I could include the symbol in the diagnostic message for reference if you think that would be helpful.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
Outdated
Show resolved
Hide resolved
RikkiGibson
force-pushed
the
warn-ref-struct-alloc
branch
from
c9e4d3f to
b6a45c0
Compare
|
Rebasing to clean up the diff with main |
ghost
added this to the
|
Commit message isn't perfectly accurate, this warning only applies to span types. Apologies for any confusion. |
Closes #69693
Changes from #70260 will appear in this PR until main is synced again. They're pretty small and since they only affect codegen they're distinct from the diagnostic changes.