Check ref safety of arg mixing in collection initializers

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

@@ -4272,7 +4272,10 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext scopeOfTheCo
                         var initializerOpt = objectCreation.InitializerExpressionOpt;
                         if (initializerOpt != null)
                         {
-                            escape = escape.Intersect(GetValEscape(initializerOpt, scopeOfTheContainingExpression));
+                            escape = escape.Intersect(
+                                initializerOpt is BoundCollectionInitializerExpression colInitExpr
+                                ? GetValEscapeOfCollectionInitializer(colInitExpr, scopeOfTheContainingExpression, receiverScope: escape)
+                                : GetValEscape(initializerOpt, scopeOfTheContainingExpression));
                         }
 
                         return escape;
@@ -4455,11 +4458,7 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext scopeOfTheCo
 
                 case BoundKind.CollectionInitializerExpression:
                     var colExpr = (BoundCollectionInitializerExpression)expr;
-                    return GetValEscape(colExpr.Initializers, scopeOfTheContainingExpression);
-
-                case BoundKind.CollectionElementInitializer:
-                    var colElement = (BoundCollectionElementInitializer)expr;
-                    return GetValEscape(colElement.Arguments, scopeOfTheContainingExpression);
+                    return GetValEscapeOfCollectionInitializer(colExpr, scopeOfTheContainingExpression, receiverScope: SafeContext.CallingMethod);
 
                 case BoundKind.ObjectInitializerMember:
                     // this node generally makes no sense outside of the context of containing initializer
@@ -4468,11 +4467,18 @@ internal SafeContext GetValEscape(BoundExpression expr, SafeContext scopeOfTheCo
                     return scopeOfTheContainingExpression;
 
                 case BoundKind.ImplicitReceiver:
-                case BoundKind.ObjectOrCollectionValuePlaceholder:
                     // binder uses this as a placeholder when binding members inside an object initializer
                     // just say it does not escape anywhere, so that we do not get false errors.
                     return scopeOfTheContainingExpression;
 
+                case BoundKind.ObjectOrCollectionValuePlaceholder:
+                    if (_placeholderScopes?.TryGetValue((BoundObjectOrCollectionValuePlaceholder)expr, out var scope) == true)
+                    {
+                        return scope;
+                    }
+
+                    return scopeOfTheContainingExpression;
+
                 case BoundKind.InterpolatedStringHandlerPlaceholder:
                     // The handler placeholder cannot escape out of the current expression, as it's a compiler-synthesized
                     // location.
@@ -4589,6 +4595,17 @@ private SafeContext GetValEscapeOfObjectInitializer(BoundObjectInitializerExpres
             return result;
         }
 
+        private SafeContext GetValEscapeOfCollectionInitializer(BoundCollectionInitializerExpression colExpr, SafeContext scopeOfTheContainingExpression, SafeContext receiverScope)
+        {
+            // We are interested in the CheckValEscape's return value, but it can be false only if not in an unsafe region.
+            using var _ = new UnsafeRegion(this, inUnsafeRegion: false);
+
+            // If arg mixing fails (i.e., some arguments could escape into the receiver), make the result scoped.
+            return CheckValEscapeOfCollectionInitializer(colExpr, escapeFrom: scopeOfTheContainingExpression, escapeTo: receiverScope, BindingDiagnosticBag.Discarded)
+                ? receiverScope
+                : scopeOfTheContainingExpression;
+        }
+
         private SafeContext GetValEscapeOfObjectMemberInitializer(BoundExpression expr, SafeContext scopeOfTheContainingExpression)
         {
             SafeContext result;
@@ -4774,6 +4791,18 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, SafeContext
                     }
                     return true;
 
+                case BoundKind.ObjectOrCollectionValuePlaceholder:
+                    if (_placeholderScopes?.TryGetValue((BoundObjectOrCollectionValuePlaceholder)expr, out var scope) == true)
+                    {
+                        if (!scope.IsConvertibleTo(escapeTo))
+                        {
+                            Error(diagnostics, inUnsafeRegion ? ErrorCode.WRN_EscapeVariable : ErrorCode.ERR_EscapeVariable, node, expr.Syntax);
+                            return inUnsafeRegion;
+                        }
+                        return true;
+                    }
+                    goto default;
+
                 case BoundKind.Local:
                     var localSymbol = ((BoundLocal)expr).LocalSymbol;
                     if (!GetLocalScopes(localSymbol).ValEscapeScope.IsConvertibleTo(escapeTo))
@@ -5257,17 +5286,9 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, SafeContext
                     var initExpr = (BoundObjectInitializerExpression)expr;
                     return CheckValEscapeOfObjectInitializer(initExpr, escapeFrom, escapeTo, diagnostics);
 
-                // this would be correct implementation for CollectionInitializerExpression 
-                // however it is unclear if it is reachable since the initialized type must implement IEnumerable
                 case BoundKind.CollectionInitializerExpression:
                     var colExpr = (BoundCollectionInitializerExpression)expr;
-                    return CheckValEscape(colExpr.Initializers, escapeFrom, escapeTo, diagnostics);
-
-                // this would be correct implementation for CollectionElementInitializer 
-                // however it is unclear if it is reachable since the initialized type must implement IEnumerable
-                case BoundKind.CollectionElementInitializer:
-                    var colElement = (BoundCollectionElementInitializer)expr;
-                    return CheckValEscape(colElement.Arguments, escapeFrom, escapeTo, diagnostics);
+                    return CheckValEscapeOfCollectionInitializer(colExpr, escapeFrom, escapeTo, diagnostics);
 
                 case BoundKind.PointerElementAccess:
                     var accessedExpression = ((BoundPointerElementAccess)expr).Expression;
@@ -5561,21 +5582,53 @@ private bool CheckValEscapeOfObjectInitializer(BoundObjectInitializerExpression
             return true;
         }
 
-#nullable disable
-
-        private bool CheckValEscape(ImmutableArray<BoundExpression> expressions, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
+        private bool CheckValEscapeOfCollectionInitializer(BoundCollectionInitializerExpression colExpr, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
         {
-            foreach (var expression in expressions)
+            //    return new C() { element };
+            //
+            // is equivalent to
+            //
+            //    var c = new C();
+            //    c.Add(element);
+            //    return c;
+            //
+            // So we check arg mixing of `(receiverPlaceholder).Add(element)` calls
+            // where the placeholder has `escapeTo` scope and the call has `escapeFrom` scope.
+
+            bool result = true;
+            using var _ = new PlaceholderRegion(this, [(colExpr.Placeholder, escapeTo)]) { ForceRemoveOnDispose = true };
+            foreach (var initializer in colExpr.Initializers)
             {
-                if (!CheckValEscape(expression.Syntax, expression, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics))
+                switch (initializer)
                 {
-                    return false;
+                    case BoundCollectionElementInitializer init:
+                        result &= CheckInvocationArgMixing(
+                            init.Syntax,
+                            MethodInfo.Create(init.AddMethod),
+                            receiverOpt: colExpr.Placeholder,
+                            receiverIsSubjectToCloning: init.InitialBindingReceiverIsSubjectToCloning,
+                            parameters: init.AddMethod.Parameters,
+                            argsOpt: init.Arguments,
+                            argRefKindsOpt: default,
+                            argsToParamsOpt: init.ArgsToParamsOpt,
+                            scopeOfTheContainingExpression: escapeFrom,
+                            diagnostics);
+                        break;
+
+                    case BoundDynamicCollectionElementInitializer:
+                        break;
+
+                    default:
+                        Debug.Fail($"{initializer.Kind} expression of {initializer.Type} type");
+                        break;
                 }
             }
 
-            return true;
+            return result;
         }
 
+#nullable disable
+
         private bool CheckInterpolatedStringHandlerConversionEscape(BoundExpression expression, SafeContext escapeFrom, SafeContext escapeTo, BindingDiagnosticBag diagnostics)
         {
             var data = expression.GetInterpolatedStringHandlerData();

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

@@ -6473,6 +6473,7 @@ BoundExpression bindCollectionInitializerElementAddMethod(
                         boundCall.Method,
                         boundCall.Arguments,
                         boundCall.ReceiverOpt,
+                        boundCall.InitialBindingReceiverIsSubjectToCloning,
                         boundCall.Expanded,
                         boundCall.ArgsToParamsOpt,
                         boundCall.DefaultArguments,

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

@@ -153,6 +153,8 @@ private ref struct PlaceholderRegion
             private readonly RefSafetyAnalysis _analysis;
             private readonly ArrayBuilder<(BoundValuePlaceholderBase, SafeContext)> _placeholders;
 
+            public bool ForceRemoveOnDispose { get; init; }
+
             public PlaceholderRegion(RefSafetyAnalysis analysis, ArrayBuilder<(BoundValuePlaceholderBase, SafeContext)> placeholders)
             {
                 _analysis = analysis;
@@ -167,7 +169,7 @@ public void Dispose()
             {
                 foreach (var (placeholder, _) in _placeholders)
                 {
-                    _analysis.RemovePlaceholderScope(placeholder);
+                    _analysis.RemovePlaceholderScope(placeholder, forceRemove: ForceRemoveOnDispose);
                 }
                 _placeholders.Free();
             }
@@ -200,16 +202,17 @@ private void AddPlaceholderScope(BoundValuePlaceholderBase placeholder, SafeCont
             _placeholderScopes[placeholder] = valEscapeScope;
         }
 
-#pragma warning disable IDE0060
-        private void RemovePlaceholderScope(BoundValuePlaceholderBase placeholder)
+        private void RemovePlaceholderScope(BoundValuePlaceholderBase placeholder, bool forceRemove)
         {
             Debug.Assert(_placeholderScopes?.ContainsKey(placeholder) == true);
 
             // https://github.com/dotnet/roslyn/issues/65961: Currently, analysis may require subsequent calls
             // to GetRefEscape(), etc. for the same expression so we cannot remove placeholders eagerly.
-            //_placeholderScopes.Remove(placeholder);
+            if (forceRemove)
+            {
+                _placeholderScopes?.Remove(placeholder);
+            }
         }
-#pragma warning restore IDE0060
 
         private SafeContext GetPlaceholderScope(BoundValuePlaceholderBase placeholder)
         {

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -2057,6 +2057,8 @@
     <Field Name="Arguments" Type="ImmutableArray&lt;BoundExpression&gt;"/>
     <!-- Used for IOperation to enable translating the initializer to a IDynamicInvocationOperation -->
     <Field Name="ImplicitReceiverOpt" Type="BoundExpression?" />
+    <!-- Whether receiver will need to be cloned during emit (only valid before lowering) -->
+    <Field Name="InitialBindingReceiverIsSubjectToCloning" Type="ThreeState"/>
     <Field Name="Expanded" Type="bool"/>
     <Field Name="ArgsToParamsOpt" Type="ImmutableArray&lt;int&gt;" Null="allow"/>
     <Field Name="DefaultArguments" Type="BitVector" />