Collection expressions: require applicable constructor and Add method for target type implementing IEnumerable

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 8.md

@@ -1,5 +1,30 @@
 # This document lists known breaking changes in Roslyn after .NET 7 all the way to .NET 8.
 
+## Collection expression target type must have constructor and `Add` method
+
+***Introduced in Visual Studio 2022 version 17.10***
+
+*Conversion* of a collection expression to a `struct` or `class` that implements `System.Collections.IEnumerable` and *does not* have a `CollectionBuilderAttribute`
+requires the target type to have an accessible constructor that can be called with no arguments and an accessible `Add` method
+that can be called with a single argument of [*iteration type*](https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/statements.md#1395-the-foreach-statement) of the target type.
+
+Previously, the constructor and `Add` methods were required for *construction* of the collection instance but not for *conversion*.
+That meant the following call was ambiguous since both `char[]` and `string` were valid target types for the collection expression.
+The call is no longer ambiguous because `string` does not have a parameterless constructor or `Add` method.
+```csharp
+Print(['a', 'b', 'c']); // calls Print(char[])
+
+static void Print(char[] arg) { }
+static void Print(string arg) { }
+```
+
+Previously, an `Add` method was not required for an empty collection expression.
+That meant the following assignment was allowed even though `Dictionary<TKey, TValue>` does not have an accessible `Add(KeyValuePair<TKey, TValue>)` method.
+The assignment is now an error.
+```csharp
+Dictionary<string, object> d = []; // error: no accessible Add(KeyValuePair<string, object>)
+```
+
 ## `ref` arguments can be passed to `in` parameters
 
 ***Introduced in Visual Studio 2022 version 17.8p2***

src/Analyzers/CSharp/Tests/UseCollectionInitializer/UseCollectionInitializerTests_CollectionExpression.cs

@@ -5151,6 +5151,43 @@ void M()
             """);
     }
 
+    [Fact]
+    public async Task TestNotInDictionary()
+    {
+        await TestMissingInRegularAndScriptAsync(
+            """
+            using System.Collections.Generic;
+            class Program
+            {
+                static void Main()
+                {
+                    Dictionary<string, object> d = new Dictionary<string, object>() { };
+                }
+            }
+            """);
+    }
+
+    [Fact]
+    public async Task TestNotInIEnumerableAndIncompatibleAdd()
+    {
+        await TestMissingInRegularAndScriptAsync(
+            """
+            using System.Collections;
+            class Program
+            {
+                static void Main()
+                {
+                    MyCollection c = new MyCollection() { };
+                }
+            }
+            class MyCollection : IEnumerable
+            {
+                public void Add(string s) { }
+                IEnumerator IEnumerable.GetEnumerator() => null;
+            }
+            """);
+    }
+
     [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71607")]
     public async Task TestAddRangeOfCollectionExpression1()
     {

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

@@ -112,6 +112,12 @@ internal enum BinderFlags : uint
         /// </summary>
         InExpressionTree = 1 << 30,
 
+        /// <summary>
+        /// Indicates the binder is used during collection expression conversion
+        /// to verify applicable methods are available.
+        /// </summary>
+        CollectionExpressionConversionValidation = 1u << 31,
+
         // Groups
 
         AllClearedAtExecutableCodeBoundary = InLockBody | InCatchBlock | InCatchFilter | InFinallyBlock | InTryBlockOfTryCatch | InNestedFinallyBlock,

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

@@ -654,35 +654,25 @@ private BoundCollectionExpression ConvertCollectionExpression(
             if (collectionTypeKind is CollectionExpressionTypeKind.ImplementsIEnumerable)
             {
                 implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
-                if (targetType is NamedTypeSymbol namedType)
-                {
-                    var analyzedArguments = AnalyzedArguments.GetInstance();
-                    // 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();
-                }
-                else if (targetType is TypeParameterSymbol typeParameter)
-                {
-                    var analyzedArguments = AnalyzedArguments.GetInstance();
-                    collectionCreation = BindTypeParameterCreationExpression(syntax, typeParameter, analyzedArguments, initializerOpt: null, typeSyntax: syntax, wasTargetTyped: true, diagnostics);
-                    collectionCreation.WasCompilerGenerated = true;
-                    analyzedArguments.Free();
-                }
-                else
-                {
-                    collectionCreation = new BoundBadExpression(syntax, LookupResultKind.NotCreatable, ImmutableArray<Symbol?>.Empty, ImmutableArray<BoundExpression>.Empty, targetType);
-                }
+                collectionCreation = BindCollectionExpressionConstructor(syntax, targetType, diagnostics);
 
                 var collectionInitializerAddMethodBinder = this.WithAdditionalFlags(BinderFlags.CollectionInitializerAddMethod);
                 foreach (var element in elements)
                 {
-                    BoundNode convertedElement = BindCollectionExpressionElementAddMethod(
-                        element,
-                        collectionInitializerAddMethodBinder,
-                        implicitReceiver,
-                        diagnostics,
-                        out _);
+                    BoundNode convertedElement = element is BoundCollectionExpressionSpreadElement spreadElement ?
+                        (BoundNode)BindCollectionExpressionSpreadElementAddMethod(
+                            (SpreadElementSyntax)spreadElement.Syntax,
+                            spreadElement,
+                            collectionInitializerAddMethodBinder,
+                            implicitReceiver,
+                            diagnostics) :
+                        BindCollectionInitializerElementAddMethod(
+                            (ExpressionSyntax)element.Syntax,
+                            ImmutableArray.Create((BoundExpression)element),
+                            hasEnumerableInitializerType: true,
+                            collectionInitializerAddMethodBinder,
+                            diagnostics,
+                            implicitReceiver);
                     builder.Add(convertedElement);
                 }
             }
@@ -766,6 +756,50 @@ BoundNode bindSpreadElement(BoundCollectionExpressionSpreadElement element, Type
             }
         }
 
+        internal BoundExpression BindCollectionExpressionConstructor(SyntaxNode syntax, TypeSymbol targetType, BindingDiagnosticBag diagnostics)
+        {
+            BoundExpression collectionCreation;
+            var analyzedArguments = AnalyzedArguments.GetInstance();
+            if (targetType is NamedTypeSymbol namedType)
+            {
+                var binder = WithAdditionalFlags(BinderFlags.CollectionExpressionConversionValidation);
+                collectionCreation = binder.BindClassCreationExpression(syntax, namedType.Name, syntax, namedType, analyzedArguments, diagnostics);
+                collectionCreation.WasCompilerGenerated = true;
+            }
+            else if (targetType is TypeParameterSymbol typeParameter)
+            {
+                collectionCreation = BindTypeParameterCreationExpression(syntax, typeParameter, analyzedArguments, initializerOpt: null, typeSyntax: syntax, wasTargetTyped: true, diagnostics);
+                collectionCreation.WasCompilerGenerated = true;
+            }
+            else
+            {
+                throw ExceptionUtilities.UnexpectedValue(targetType);
+            }
+            analyzedArguments.Free();
+            return collectionCreation;
+        }
+
+        internal bool HasCollectionExpressionApplicableConstructor(SyntaxNode syntax, TypeSymbol targetType, BindingDiagnosticBag diagnostics)
+        {
+            var collectionCreation = BindCollectionExpressionConstructor(syntax, targetType, diagnostics);
+            return !collectionCreation.HasErrors;
+        }
+
+        internal bool HasCollectionExpressionApplicableAddMethod(SyntaxNode syntax, TypeSymbol targetType, TypeSymbol elementType, BindingDiagnosticBag diagnostics)
+        {
+            var implicitReceiver = new BoundObjectOrCollectionValuePlaceholder(syntax, isNewInstance: true, targetType) { WasCompilerGenerated = true };
+            var elementPlaceholder = new BoundValuePlaceholder(syntax, elementType) { WasCompilerGenerated = true };
+            var addMethodBinder = WithAdditionalFlags(BinderFlags.CollectionInitializerAddMethod | BinderFlags.CollectionExpressionConversionValidation);
+            var result = BindCollectionInitializerElementAddMethod(
+                syntax,
+                ImmutableArray.Create<BoundExpression>(elementPlaceholder),
+                hasEnumerableInitializerType: true,
+                addMethodBinder,
+                diagnostics,
+                implicitReceiver);
+            return !result.HasErrors;
+        }
+
         /// <summary>
         /// If the element is from a collection type where elements are added with collection initializers,
         /// return the argument to the collection initializer Add method or null if the element is not a
@@ -868,6 +902,19 @@ private void GenerateImplicitConversionErrorForCollectionExpression(
                 var elementType = elementTypeWithAnnotations.Type;
                 Debug.Assert(elementType is { });
 
+                if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerable)
+                {
+                    if (!HasCollectionExpressionApplicableConstructor(node.Syntax, targetType, diagnostics))
+                    {
+                        reportedErrors = true;
+                    }
+
+                    if (!HasCollectionExpressionApplicableAddMethod(node.Syntax, targetType, elementType, diagnostics))
+                    {
+                        reportedErrors = true;
+                    }
+                }
+
                 var elements = node.Elements;
                 var useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
                 foreach (var element in elements)

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

@@ -5819,7 +5819,7 @@ private BoundExpression BindUnexpectedComplexElementInitializer(InitializerExpre
         }
 
         private BoundExpression BindCollectionInitializerElementAddMethod(
-            ExpressionSyntax elementInitializer,
+            SyntaxNode elementInitializer,
             ImmutableArray<BoundExpression> boundElementInitializerExpressions,
             bool hasEnumerableInitializerType,
             Binder collectionInitializerAddMethodBinder,
@@ -5937,31 +5937,6 @@ static void copyRelevantAddMethodDiagnostics(BindingDiagnosticBag source, Bindin
         }
 
 #nullable enable
-        internal BoundNode BindCollectionExpressionElementAddMethod(
-            BoundNode element,
-            Binder collectionInitializerAddMethodBinder,
-            BoundObjectOrCollectionValuePlaceholder implicitReceiver,
-            BindingDiagnosticBag diagnostics,
-            out bool hasErrors)
-        {
-            var result = element is BoundCollectionExpressionSpreadElement spreadElement ?
-                (BoundNode)BindCollectionExpressionSpreadElementAddMethod(
-                    (SpreadElementSyntax)spreadElement.Syntax,
-                    spreadElement,
-                    collectionInitializerAddMethodBinder,
-                    implicitReceiver,
-                    diagnostics) :
-                BindCollectionInitializerElementAddMethod(
-                    (ExpressionSyntax)element.Syntax,
-                    ImmutableArray.Create((BoundExpression)element),
-                    hasEnumerableInitializerType: true,
-                    collectionInitializerAddMethodBinder,
-                    diagnostics,
-                    implicitReceiver);
-            hasErrors = result.HasErrors;
-            return result;
-        }
-
         private BoundCollectionExpressionSpreadElement BindCollectionExpressionSpreadElementAddMethod(
             SpreadElementSyntax syntax,
             BoundCollectionExpressionSpreadElement element,

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

@@ -9,7 +9,6 @@
 using System.Linq;
 using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Microsoft.CodeAnalysis.PooledObjects;
-using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.CSharp
 {
@@ -183,6 +182,19 @@ protected override Conversion GetCollectionExpressionConversion(
 
             Debug.Assert(elementType is { });
 
+            if (collectionTypeKind == CollectionExpressionTypeKind.ImplementsIEnumerable)
+            {
+                if (!_binder.HasCollectionExpressionApplicableConstructor(syntax, targetType, BindingDiagnosticBag.Discarded))
+                {
+                    return Conversion.NoConversion;
+                }
+
+                if (!_binder.HasCollectionExpressionApplicableAddMethod(syntax, targetType, elementType, BindingDiagnosticBag.Discarded))
+                {
+                    return Conversion.NoConversion;
+                }
+            }
+
             var elements = node.Elements;
             var builder = ArrayBuilder<Conversion>.GetInstance(elements.Length);
             foreach (var element in elements)

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -2570,10 +2570,10 @@ kind2 is CollectionExpressionTypeKind.Span &&
                 return true;
             }
 
-            // - T1 is System.ReadOnlySpan<E1> or System.Span<E1>, and T2 is an array_or_array_interface_or_string_type
+            // - T1 is System.ReadOnlySpan<E1> or System.Span<E1>, and T2 is an array_or_array_interface
             //    with iteration type E2, and an implicit conversion exists from E1 to E2
             if (kind1 is CollectionExpressionTypeKind.ReadOnlySpan or CollectionExpressionTypeKind.Span &&
-                IsSZArrayOrArrayInterfaceOrString(t2, out elementType2) &&
+                IsSZArrayOrArrayInterface(t2, out elementType2) &&
                 hasImplicitConversion(elementType1, elementType2, ref useSiteInfo))
             {
                 return true;
@@ -2593,14 +2593,8 @@ bool hasImplicitConversion(TypeSymbol source, TypeSymbol destination, ref Compou
                 Conversions.ClassifyImplicitConversionFromType(source, destination, ref useSiteInfo).IsImplicit;
         }
 
-        private bool IsSZArrayOrArrayInterfaceOrString(TypeSymbol type, out TypeSymbol elementType)
+        private static bool IsSZArrayOrArrayInterface(TypeSymbol type, out TypeSymbol elementType)
         {
-            if (type.SpecialType == SpecialType.System_String)
-            {
-                elementType = Compilation.GetSpecialType(SpecialType.System_Char);
-                return true;
-            }
-
             if (type is ArrayTypeSymbol { IsSZArray: true } arrayType)
             {
                 elementType = arrayType.ElementType;

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolutionResult.cs

@@ -504,8 +504,25 @@ internal void ReportDiagnostics<T>(
                         // but no argument was supplied for it then the first such method is 
                         // the best bad method.
                         case MemberResolutionKind.RequiredParameterMissing:
-                            // CONSIDER: for consistency with dev12, we would goto default except in omitted ref cases.
-                            ReportMissingRequiredParameter(firstSupported, diagnostics, delegateTypeBeingInvoked, symbols, location);
+                            if ((binder.Flags & BinderFlags.CollectionExpressionConversionValidation) != 0)
+                            {
+                                if (receiver is null)
+                                {
+                                    Debug.Assert(firstSupported.Member is MethodSymbol { MethodKind: MethodKind.Constructor });
+                                    diagnostics.Add(ErrorCode.ERR_CollectionExpressionMissingConstructor, location);
+                                }
+                                else
+                                {
+                                    Debug.Assert(firstSupported.Member is MethodSymbol { Name: "Add" });
+                                    int argumentOffset = arguments.IsExtensionMethodInvocation ? 1 : 0;
+                                    diagnostics.Add(ErrorCode.ERR_CollectionExpressionMissingAdd, location, arguments.Arguments[argumentOffset].Type, firstSupported.Member);
+                                }
+                            }
+                            else
+                            {
+                                // CONSIDER: for consistency with dev12, we would goto default except in omitted ref cases.
+                                ReportMissingRequiredParameter(firstSupported, diagnostics, delegateTypeBeingInvoked, symbols, location);
+                            }
                             return;
 
                         // NOTE: For some reason, there is no specific handling for this result kind.
@@ -1092,18 +1109,29 @@ private bool HadBadArguments(
                 // ErrorCode.ERR_BadArgTypesForCollectionAdd or ErrorCode.ERR_InitializerAddHasParamModifiers
                 // as there is no explicit call to Add method.
 
-                foreach (var parameter in method.GetParameters())
+                int argumentOffset = arguments.IsExtensionMethodInvocation ? 1 : 0;
+                var parameters = method.GetParameters();
+
+                for (int i = argumentOffset; i < parameters.Length; i++)
                 {
-                    if (parameter.RefKind != RefKind.None)
+                    if (parameters[i].RefKind != RefKind.None)
                     {
                         //  The best overloaded method match '{0}' for the collection initializer element cannot be used. Collection initializer 'Add' methods cannot have ref or out parameters.
                         diagnostics.Add(ErrorCode.ERR_InitializerAddHasParamModifiers, location, symbols, method);
                         return true;
                     }
                 }
 
-                //  The best overloaded Add method '{0}' for the collection initializer has some invalid arguments
-                diagnostics.Add(ErrorCode.ERR_BadArgTypesForCollectionAdd, location, symbols, method);
+                if (flags.Includes(BinderFlags.CollectionExpressionConversionValidation))
+                {
+                    Debug.Assert(arguments.Arguments.Count == argumentOffset + 1);
+                    diagnostics.Add(ErrorCode.ERR_CollectionExpressionMissingAdd, location, arguments.Arguments[argumentOffset].Type, method);
+                }
+                else
+                {
+                    //  The best overloaded Add method '{0}' for the collection initializer has some invalid arguments
+                    diagnostics.Add(ErrorCode.ERR_BadArgTypesForCollectionAdd, location, symbols, method);
+                }
             }
 
             foreach (var arg in badArg.Result.BadArgumentsOpt.TrueBits())

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6859,6 +6859,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_CollectionExpressionTargetNoElementType" xml:space="preserve">
     <value>Collection expression target '{0}' has no element type.</value>
   </data>
+  <data name="ERR_CollectionExpressionMissingConstructor" xml:space="preserve">
+    <value>Collection expression type must have an applicable constructor that can be called with no arguments.</value>
+  </data>
+  <data name="ERR_CollectionExpressionMissingAdd" xml:space="preserve">
+    <value>Collection expression type must have an applicable instance or extension method 'Add' that can be called with an argument of iteration type '{0}'. The best overloaded method is '{1}'.</value>
+  </data>
   <data name="ERR_CollectionBuilderAttributeInvalidType" xml:space="preserve">
     <value>The CollectionBuilderAttribute builder type must be a non-generic class or struct.</value>
   </data>