Collection literals: address comments (#68793)

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

@@ -3960,7 +3960,6 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres
                     return GetValEscape(switchExpr.SwitchArms.SelectAsArray(a => a.Value), scopeOfTheContainingExpression);
 
                 case BoundKind.CollectionLiteralExpression:
-                    // PROTOTYPE: Revisit if spans may be optimized to avoid heap allocation.
                     return CallingMethodScope;
 
                 default:
@@ -4490,7 +4489,6 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF
                     return true;
 
                 case BoundKind.CollectionLiteralExpression:
-                    // PROTOTYPE: Revisit if spans may be optimized to avoid heap allocation.
                     return true;
 
                 default:

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -575,7 +575,7 @@ private BoundCollectionLiteralExpression BindArrayOrSpanCollectionLiteral(
             if (elements.Any(e => e is BoundCollectionLiteralSpreadElement))
             {
                 // The array initializer includes at least one spread element, so we'll create an intermediate List<T> instance.
-                // PROTOTYPE: Avoid the intermediate list if the compile-time type of the spread element includes a length.
+                // https://github.com/dotnet/roslyn/issues/68785: Avoid intermediate List<T> if all spread elements have Length property.
                 _ = GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_List_T__ToArray, diagnostics, syntax: syntax);
                 var result = BindCollectionInitializerCollectionLiteral(
                     node,
@@ -642,7 +642,7 @@ private BoundCollectionLiteralExpression BindCollectionInitializerCollectionLite
             if (targetType is NamedTypeSymbol namedType)
             {
                 var analyzedArguments = AnalyzedArguments.GetInstance();
-                // PROTOTYPE: Should we use List<T>(int capacity) constructor when the size is known?
+                // https://github.com/dotnet/roslyn/issues/68785: Use ctor with `int capacity` when the size is known.
                 collectionCreation = BindClassCreationExpression(syntax, namedType.Name, syntax, namedType, analyzedArguments, diagnostics);
                 collectionCreation.WasCompilerGenerated = true;
                 analyzedArguments.Free();
@@ -659,18 +659,13 @@ private BoundCollectionLiteralExpression BindCollectionInitializerCollectionLite
             }
 
             var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
-            // PROTOTYPE: When generating a List<T>, should we use the well-known
-            // member List<T>.Add() rather than relying on lookup?
             var collectionInitializerAddMethodBinder = this.WithAdditionalFlags(BinderFlags.CollectionInitializerAddMethod);
             var builder = ArrayBuilder<BoundExpression>.GetInstance(node.Elements.Length);
             foreach (var element in node.Elements)
             {
                 var result = element switch
                 {
                     BoundBadExpression => element,
-                    // PROTOTYPE: Should spread elements support target type?
-                    // string[] a = [..b ? [null] : []];
-                    // See CollectionLiteralTests.SpreadElement_05, _06.
                     BoundCollectionLiteralSpreadElement spreadElement => BindCollectionInitializerSpreadElementAddMethod(
                             (SpreadElementSyntax)spreadElement.Syntax,
                             spreadElement,
@@ -706,7 +701,7 @@ private BoundCollectionLiteralExpression BindListInterfaceCollectionLiteral(
             TypeSymbol elementType,
             BindingDiagnosticBag diagnostics)
         {
-            // PROTOTYPE: Improve perf. For instance, emit [] as Array.Empty<T>() rather than a List<T>.
+            // https://github.com/dotnet/roslyn/issues/68785: Emit [] as Array.Empty<T>() rather than a List<T>.
             var result = BindCollectionInitializerCollectionLiteral(
                 node,
                 CollectionLiteralTypeKind.ListInterface,

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -575,7 +575,6 @@ public Conversion ClassifyStandardConversion(BoundExpression sourceExpression, T
             return Conversion.NoConversion;
         }
 
-        // PROTOTYPE: Ensure collection literal conversions are not considered standard implicit conversions.
         private static bool IsStandardImplicitConversionFromExpression(ConversionKind kind)
         {
             if (IsStandardImplicitConversionFromType(kind))
@@ -1662,10 +1661,11 @@ static bool implementsIEnumerable(CSharpCompilation compilation, TypeSymbol targ
                 }
 
                 // This implementation differs from Binder.CollectionInitializerTypeImplementsIEnumerable().
-                // That method checks for an implicit conversion from IEnumerable to the collection type,
-                // but that would allow: Nullable<StructCollection> s = [];
-                // PROTOTYPE: Perhaps adjust the behavior of Binder.CollectionInitializerTypeImplementsIEnumerable()
-                // and use that method instead.
+                // That method checks for an implicit conversion from IEnumerable to the collection type, to
+                // match earlier implementation, even though it states that walking the implemented interfaces
+                // would be better. If we use CollectionInitializerTypeImplementsIEnumerable() here, we'd need
+                // to check for nullable to disallow: Nullable<StructCollection> s = [];
+                // Instead, we just walk the implemented interfaces.
                 var ienumerableType = compilation.GetSpecialType(SpecialType.System_Collections_IEnumerable);
                 return allInterfaces.Any(static (a, b) => areEqual(a, b), ienumerableType);
             }

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -2233,9 +2233,9 @@ internal enum ErrorCode
         ERR_InlineArrayBadIndex = 9172,
         ERR_NamedArgumentForInlineArray = 9173,
 
-        ERR_CollectionLiteralTargetTypeNotConstructible = 9500, // PROTOTYPE: Update error numbers.
-        ERR_ExpressionTreeContainsCollectionLiteral = 9501,
-        ERR_CollectionLiteralNoTargetType = 9503,
+        ERR_CollectionLiteralTargetTypeNotConstructible = 9174,
+        ERR_ExpressionTreeContainsCollectionLiteral = 9175,
+        ERR_CollectionLiteralNoTargetType = 9176,
 
         #endregion
 

src/Compilers/CSharp/Portable/Errors/MessageID.cs

@@ -274,7 +274,7 @@ internal enum MessageID
         IDS_FeatureInstanceMemberInNameof = MessageBase + 12835,
 
         IDS_FeatureInlineArrays = MessageBase + 12836,
-        IDS_FeatureCollectionLiterals = MessageBase + 12900, // PROTOTYPE: Update message number.
+        IDS_FeatureCollectionLiterals = MessageBase + 12837,
     }
 
     // Message IDs may refer to strings that need to be localized.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

@@ -3473,12 +3473,12 @@ protected override void VisitStatement(BoundStatement statement)
 
         public override BoundNode? VisitCollectionLiteralExpression(BoundCollectionLiteralExpression node)
         {
-            // PROTOTYPE: Do we need to call inferInitialObjectState() to set the initial state of the instance?
+            // https://github.com/dotnet/roslyn/issues/68786: Use inferInitialObjectState() to set the initial
+            // state of the instance: see the call to InheritNullableStateOfTrackableStruct() in particular.
             int containerSlot = GetOrCreatePlaceholderSlot(node);
-            bool delayCompletionForType = false; // PROTOTYPE: Should be true if the collection literal is target typed.
+            bool delayCompletionForType = false; // https://github.com/dotnet/roslyn/issues/68786: Should be true if the collection literal is target typed.
 
-            // PROTOTYPE: Test nullability of elements when the collection literal is target typed
-            // and the inferred target type has distinct element type nullability.
+            // https://github.com/dotnet/roslyn/issues/68786: Set nullability of elements from the inferred target type nullability.
             foreach (var element in node.Elements)
             {
                 switch (element)
@@ -3488,7 +3488,7 @@ protected override void VisitStatement(BoundStatement statement)
                         var completion = VisitCollectionElementInitializer(initializer, collectionType, delayCompletionForType);
                         if (completion is { })
                         {
-                            // PROTOTYPE: Complete the analysis later.
+                            // https://github.com/dotnet/roslyn/issues/68786: Complete the analysis later.
                             completion(containerSlot, collectionType);
                         }
                         break;

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionLiteral.cs

@@ -61,17 +61,17 @@ private BoundExpression VisitArrayOrSpanCollectionLiteralExpression(BoundCollect
             if (elements.Any(i => i is BoundCollectionLiteralSpreadElement))
             {
                 // The array initializer includes at least one spread element, so we'll create an intermediate List<T> instance.
-                // PROTOTYPE: Avoid the intermediate list if the compile-time type of the spread element includes a length.
-                // PROTOTYPE: Use Enumerable.TryGetNonEnumeratedCount() at runtime for other cases.
+                // https://github.com/dotnet/roslyn/issues/68785: Avoid intermediate List<T> if all spread elements have Length property.
+                // https://github.com/dotnet/roslyn/issues/68785: Emit Enumerable.TryGetNonEnumeratedCount() and avoid intermediate List<T> at runtime.
                 var listType = _compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T).Construct(elementType);
                 var listToArray = ((MethodSymbol)_compilation.GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_List_T__ToArray)!).AsMember(listType);
                 var list = VisitCollectionInitializerCollectionLiteralExpression(node);
-                array = _factory.Call(list, listToArray); // PROTOTYPE: Improve perf. For instance, avoid copying to a new array.
+                array = _factory.Call(list, listToArray);
             }
             else
             {
                 int arrayLength = elements.Length;
-                // PROTOTYPE: Should [] be emitted as Array.Empty<T>()?
+                // https://github.com/dotnet/roslyn/issues/68785: Emit [] as Array.Empty<T>() rather than a List<T>.
                 var initialization = (arrayLength == 0)
                     ? null
                     : new BoundArrayInitialization(
@@ -149,7 +149,7 @@ private BoundExpression VisitListInterfaceCollectionLiteralExpression(BoundColle
             Debug.Assert(!_inExpressionLambda);
             Debug.Assert(node.Type is { });
 
-            // PROTOTYPE: Improve perf. For instance, emit [] as Array.Empty<T>() rather than a List<T>.
+            // https://github.com/dotnet/roslyn/issues/68785: Emit [] as Array.Empty<T>() rather than a List<T>.
             var list = VisitCollectionInitializerCollectionLiteralExpression(node);
             return _factory.Convert(node.Type, list);
         }

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

@@ -1247,7 +1247,7 @@ ImmutableArray<IOperation> createChildren(ImmutableArray<BoundExpression> elemen
             {
                 var element = expression switch
                 {
-                    BoundCollectionElementInitializer initializer => initializer.Arguments.FirstOrDefault(),
+                    BoundCollectionElementInitializer initializer => initializer.Arguments.First(),
                     _ => expression,
                 };
                 return Create(element);

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IArrayElementReferenceExpression.cs

@@ -525,7 +525,7 @@ public void F()
 ";
             var expectedDiagnostics = new DiagnosticDescription[]
             {
-                // (6,27): error CS9503: There is no target type for the collection literal.
+                // (6,27): error CS9176: There is no target type for the collection literal.
                 //         var a = /*<bind>*/[0]/*</bind>*/;
                 Diagnostic(ErrorCode.ERR_CollectionLiteralNoTargetType, "[0]").WithLocation(6, 27)
             };