-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Collection expressions: use inline array for ReadOnlySpan<T> argument to builder method #69227
Conversation
f975cc1
to
4176dcd
Compare
… to builder method
4176dcd
to
d75c020
Compare
Will review tomorrow! |
|
||
internal override FileIdentifier? AssociatedFileIdentifier => null; | ||
|
||
internal override bool MangleName => false; // PROTOTYPE: Is this correct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var sideEffects = ArrayBuilder<BoundExpression>.GetInstance(); | ||
BoundExpression span; | ||
|
||
// PROTOTYPE: Check also ReadOnlySpan<T> parameter is not [UnscopedRef]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROTOTYPE comment (also elsewhere) #Closed
|
||
string typeName = PrivateImplementationDetails.GetInlineArrayTypeName(arrayLength); | ||
var privateImplClass = GetPrivateImplClass(syntaxNode, diagnostics); | ||
var typeAdapter = privateImplClass.GetType(typeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with GetType()
to match the existing GetMethod()
method used in the other Ensure
methods here.
|
||
var typeSymbol = new SynthesizedInlineArrayTypeSymbol(SourceModule, typeName, arrayLength, attributeConstructor); | ||
privateImplClass.TryAddSynthesizedType(typeSymbol.GetCciAdapter()); | ||
typeAdapter = privateImplClass.GetType(typeName)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're dereferencing typeAdapter
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider doing Debug.Assert(typeAdapter!.Name == typeName);
instead. That assertion should also ensure the nullable state of typeAdapter
is not-null.
internal static string GetInlineArrayTypeName(int arrayLength) | ||
{ | ||
Debug.Assert(arrayLength > 0); | ||
return $"$InlineArray{arrayLength}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the other nested types we add in private implementation details have a different naming convention. Consider aligning. Here's one that was recently added:
public string Name => _alignment == 1 ?
$"__StaticArrayInitTypeSize={_size}" :
$"__StaticArrayInitTypeSize={_size}_Align={_alignment}";
``` #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to __InlineArrayN<T>
but perhaps __InlineArrayTypeSize=N<T>
might be better, or at least more consistent. Feel free to suggest specific names.
|
||
namespace Microsoft.CodeAnalysis.CSharp.Symbols | ||
{ | ||
internal sealed class SynthesizedInlineArrayTypeSymbol : NamedTypeSymbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
public override bool IsReadOnly => true; | ||
|
||
public override Symbol? ContainingSymbol => null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ContainingSymbol null? #Closed
|
||
internal override ObsoleteAttributeData? ObsoleteAttributeData => null; | ||
|
||
public override ImmutableArray<Symbol> GetMembers() => ImmutableArray<Symbol>.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we return the field here? #Closed
|
||
public override ImmutableArray<Symbol> GetMembers() => ImmutableArray<Symbol>.Empty; | ||
|
||
public override ImmutableArray<Symbol> GetMembers(string name) => GetMembers().WhereAsArray(m => m.Name == name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be specialized to just return the name of the field #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation seems more robust, and WhereAsArray()
should avoid any allocations since the result should be either the entire array or an empty array.
|
||
public override ImmutableArray<Location> Locations => ImmutableArray<Location>.Empty; | ||
|
||
public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => ImmutableArray<SyntaxReference>.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Why return a result here, but throw on SynthesizedInlineArrayTypeSymbol.DeclaringSyntaxReferences
? #Closed
BoundExpression span; | ||
|
||
// PROTOTYPE: Check also ReadOnlySpan<T> parameter is not [UnscopedRef]. | ||
if (elements.Length > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be any kind of upper limit/heuristic? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, we don't think there should be a limit (see notes), because the elements are already explicit in the source and because the work around to use the heap is to use C# 11 syntax.
Debug.Assert(inlineArrayType.HasInlineArrayAttribute(out int inlineArrayLength) && inlineArrayLength == arrayLength); | ||
|
||
var intType = _factory.SpecialType(SpecialType.System_Int32); | ||
var elementRef = _factory.ModuleBuilderOpt.EnsureInlineArrayElementRefExists(syntax, intType, _diagnostics.DiagnosticBag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
var intType = _factory.SpecialType(SpecialType.System_Int32); | ||
var elementRef = _factory.ModuleBuilderOpt.EnsureInlineArrayElementRefExists(syntax, intType, _diagnostics.DiagnosticBag); | ||
elementRef = elementRef.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int i = 0; i < arrayLength; i++) | ||
{ | ||
var element = VisitExpression(elements[i]); | ||
var call = _factory.Call(null, elementRef, inlineArrayLocal, _factory.Literal(i), useStrictArgumentRefKinds: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add pseudo-code comments corresponding to what is generated #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding. It makes things much easier to follow :-)
var inlineArrayAsSpan = _factory.ModuleBuilderOpt.EnsureInlineArrayAsSpanExists(syntax, spanType, intType, _diagnostics.DiagnosticBag); | ||
inlineArrayAsSpan = inlineArrayAsSpan.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType)); | ||
|
||
spanType = spanType.Construct(ImmutableArray.Create(elementType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already constructed by EnsureInlineArrayAsSpanExists
/SynthesizedInlineArrayAsSpanMethod
. Consider grabbing the return type from there. #Closed
788b882
to
4cfe4e1
Compare
There is nothing to test here after switching to Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:9263 in 4cfe4e1. [](commit_id = 4cfe4e1, deletion_comment = True) |
// synthesized inline array types | ||
private readonly ConcurrentDictionary<string, Cci.INamedTypeDefinition> _synthesizedInlineArrayTypes = | ||
new ConcurrentDictionary<string, Cci.INamedTypeDefinition>(); | ||
|
||
// field types for different block sizes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems out-of-date #Closed
Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType)); | ||
|
||
// Create an inline array and assign to a local. | ||
// var tmp = new __InlineArrayN<T>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
public override bool IsReadOnly => true; | ||
|
||
public override Symbol? ContainingSymbol => _containingModule.GlobalNamespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right containing symbol? I thought this is a nested type inside the private implementation type (based on _orderedNestedTypes = orderedProxyTypes.Concat(orderedInlineArrayTypes)
) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types are actually top-level types even though they are visited in emit through PrivateImplementationDetails.GetNestedTypes()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's test that they can be referenced in any way in source (when loading from metadata), not merely inaccessible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From offline discussion, let's leave a comment here if the symbol cannot reference the proper containing symbol. Let's try to emit those types are nested types in the private implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed inline array types to be top-level types with names that cannot be used in source.
nit: consider breaking line (also applies below) In reply to: 1658987553 Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:2691 in 75ead79. [](commit_id = 75ead79, deletion_comment = False) |
if (elements.Length > 0 | ||
&& !elements.Any(i => i is BoundCollectionExpressionSpreadElement) | ||
&& _compilation.Assembly.RuntimeSupportsInlineArrayTypes | ||
&& (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we add tests to cover those conditions, or were those getting hit by existing scenarios? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Covered by CollectionBuilder_RefStructCollection
.
var inlineArrayType = _factory.ModuleBuilderOpt.EnsureInlineArrayTypeExists(syntax, _factory, arrayLength, _diagnostics.DiagnosticBag).Construct(ImmutableArray.Create(elementType)); | ||
Debug.Assert(inlineArrayType.HasInlineArrayAttribute(out int inlineArrayLength) && inlineArrayLength == arrayLength); | ||
|
||
var intType = _factory.SpecialType(SpecialType.System_Int32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test where type System.Int32
is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I believe we're handling that correctly here, and I don't think we'd get this far without an error in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tests for various other missing types, but not this one. Let's cover it too.
var privateImplClass = GetPrivateImplClass(syntaxNode, diagnostics); | ||
var typeAdapter = privateImplClass.GetType(typeName); | ||
|
||
if (typeAdapter is null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test with two collection expressions of the same size? (the InlineArray type will be re-used/shared) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CollectionBuilder_InlineArrayTypes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review (iteration 7)
@@ -320,6 +333,18 @@ public override IEnumerable<Cci.IMethodDefinition> GetMethods(EmitContext contex | |||
return method; | |||
} | |||
|
|||
internal Cci.INamespaceTypeDefinition? GetType(string name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug.Assert(Compilation.Assembly.RuntimeSupportsInlineArrayTypes); | ||
Debug.Assert(arrayLength > 0); | ||
|
||
string typeName = $"<>y__InlineArray{arrayLength}"; // see GeneratedNameKind.InlineArrayType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider $"<>{GeneratedNameKind.InlineArrayType}__InlineArray{arrayLength}"
or adding an assertion Debug.Assert("y" == GeneratedNameKind.InlineArrayType)
That way, GoToDefinition on GeneratedNameKind.InlineArrayType
leads somewhere useful and that we don't accidentally modify it.
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (iteration 12)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 13)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 14)
Use inline array type instances as the storage for spans passed to collection expression builder methods. The inline array types are synthesized by the compiler and emitted as internal types in the assembly.
Relates to test plan #66418