Merge pull request #64621 from RikkiGibson/decl-expression-scope-17.4

Set val escape on expression variables (#64414)

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

@@ -1943,7 +1943,11 @@ private static void GetInvocationArgumentsForEscape(
                         parameters[argsToParamsOpt.IsDefault ? argIndex : argsToParamsOpt[argIndex]] :
                         null;
 
-                    if (mixableArguments is not null && isMixableParameter(parameter))
+                    if (mixableArguments is not null
+                        && isMixableParameter(parameter)
+                        // assume any expression variable is a valid mixing destination,
+                        // since we will infer a legal val-escape for it (if it doesn't already have a narrower one).
+                        && isMixableArgument(argument))
                     {
                         mixableArguments.Add(new MixableDestination(parameter, argument));
                     }
@@ -1968,6 +1972,9 @@ parameter is not null &&
                 parameter.Type.IsRefLikeType &&
                 parameter.RefKind.IsWritableReference();
 
+            static bool isMixableArgument(BoundExpression argument) =>
+                argument is not (BoundDeconstructValuePlaceholder { VariableSymbol: not null } or BoundLocal { DeclarationKind: not BoundLocalDeclarationKind.None });
+
             static EscapeArgument getReceiver(Symbol symbol, BoundExpression receiver)
             {
                 if (symbol is FunctionPointerMethodSymbol)
@@ -2195,6 +2202,30 @@ private bool UseUpdatedEscapeRulesForInvocation(Symbol symbol)
             return method?.UseUpdatedEscapeRules == true;
         }
 
+        private static bool ShouldInferDeclarationExpressionValEscape(BoundExpression argument, [NotNullWhen(true)] out SourceLocalSymbol? localSymbol)
+        {
+            var symbol = argument switch
+            {
+                BoundDeconstructValuePlaceholder p => p.VariableSymbol,
+                BoundLocal { DeclarationKind: not BoundLocalDeclarationKind.None } l => l.LocalSymbol,
+                _ => null
+            };
+            if (symbol is SourceLocalSymbol { ValEscapeScope: CallingMethodScope } local)
+            {
+                localSymbol = local;
+                return true;
+            }
+            else
+            {
+                // No need to infer a val escape for a global variable.
+                // These are only used in top-level statements in scripting mode,
+                // and since they are class fields, their scope is always CallingMethod.
+                Debug.Assert(symbol is null or SourceLocalSymbol or GlobalExpressionVariable);
+                localSymbol = null;
+                return false;
+            }
+        }
+
         /// <summary>
         /// Validates whether the invocation is valid per no-mixing rules.
         /// Returns <see langword="false"/> when it is not valid and produces diagnostics (possibly more than one recursively) that helps to figure the reason.
@@ -2239,7 +2270,7 @@ private bool CheckInvocationArgMixing(
             var escapeArguments = ArrayBuilder<EscapeArgument>.GetInstance();
             GetInvocationArgumentsForEscape(
                 symbol,
-                receiver: null, // receiver handled explicitly below
+                receiverOpt,
                 parameters,
                 argsOpt,
                 argRefKindsOpt: default,
@@ -2252,43 +2283,51 @@ private bool CheckInvocationArgMixing(
             {
                 foreach (var (_, argument, refKind) in escapeArguments)
                 {
+                    if (ShouldInferDeclarationExpressionValEscape(argument, out _))
+                    {
+                        // assume any expression variable is a valid mixing destination,
+                        // since we will infer a legal val-escape for it (if it doesn't already have a narrower one).
+                        continue;
+                    }
+
                     if (refKind.IsWritableReference() && argument.Type?.IsRefLikeType == true)
                     {
                         escapeTo = Math.Min(escapeTo, GetValEscape(argument, scopeOfTheContainingExpression));
                     }
                 }
 
-                if (escapeTo == scopeOfTheContainingExpression)
-                {
-                    // cannot fail. common case.
-                    return true;
-                }
+                var hasMixingError = false;
 
+                // track the widest scope that arguments could safely escape to.
+                // use this scope as the inferred STE of declaration expressions.
+                var inferredDestinationValEscape = CallingMethodScope;
                 foreach (var (parameter, argument, _) in escapeArguments)
                 {
-                    var valid = CheckValEscape(argument.Syntax, argument, scopeOfTheContainingExpression, escapeTo, false, diagnostics);
-
-                    if (!valid)
+                    // in the old rules, we assume that refs cannot escape into ref struct variables.
+                    // e.g. in `dest = M(ref arg)`, we assume `ref arg` will not escape into `dest`, but `arg` might.
+                    inferredDestinationValEscape = Math.Max(inferredDestinationValEscape, GetValEscape(argument, scopeOfTheContainingExpression));
+                    if (!hasMixingError && !CheckValEscape(argument.Syntax, argument, scopeOfTheContainingExpression, escapeTo, false, diagnostics))
                     {
                         string parameterName = GetInvocationParameterName(parameter);
                         Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, symbol, parameterName);
-                        return false;
+                        hasMixingError = true;
+                    }
+                }
+
+                foreach (var (_, argument, _) in escapeArguments)
+                {
+                    if (ShouldInferDeclarationExpressionValEscape(argument, out var localSymbol))
+                    {
+                        localSymbol.SetValEscape(inferredDestinationValEscape);
                     }
                 }
+
+                return !hasMixingError;
             }
             finally
             {
                 escapeArguments.Free();
             }
-
-            // check val escape of receiver if ref-like
-            if (receiverOpt?.Type?.IsRefLikeType == true)
-            {
-                // Should we also report ErrorCode.ERR_CallArgMixing if CheckValEscape() fails?
-                return CheckValEscape(receiverOpt.Syntax, receiverOpt, scopeOfTheContainingExpression, escapeTo, false, diagnostics);
-            }
-
-            return true;
         }
 
         private bool CheckInvocationArgMixingWithUpdatedRules(
@@ -2352,9 +2391,32 @@ private bool CheckInvocationArgMixingWithUpdatedRules(
                 }
             }
 
+            inferDeclarationExpressionValEscape();
+
             mixableArguments.Free();
             escapeValues.Free();
             return valid;
+
+            void inferDeclarationExpressionValEscape()
+            {
+                // find the widest scope that arguments could safely escape to.
+                // use this scope as the inferred STE of declaration expressions.
+                var inferredDestinationValEscape = CallingMethodScope;
+                foreach (var (_, fromArg, _, isRefEscape) in escapeValues)
+                {
+                    inferredDestinationValEscape = Math.Max(inferredDestinationValEscape, isRefEscape
+                        ? GetRefEscape(fromArg, scopeOfTheContainingExpression)
+                        : GetValEscape(fromArg, scopeOfTheContainingExpression));
+                }
+
+                foreach (var (_, fromArg, _, _) in escapeValues)
+                {
+                    if (ShouldInferDeclarationExpressionValEscape(fromArg, out var localSymbol))
+                    {
+                        localSymbol.SetValEscape(inferredDestinationValEscape);
+                    }
+                }
+            }
         }
 
         private static bool IsReceiverRefReadOnly(Symbol methodOrPropertySymbol) => methodOrPropertySymbol switch

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

@@ -276,7 +276,7 @@ private bool MakeDeconstructionConversion(
                     return false;
                 }
 
-                var inputPlaceholder = new BoundDeconstructValuePlaceholder(syntax, rightValEscape, type);
+                var inputPlaceholder = new BoundDeconstructValuePlaceholder(syntax, variableSymbol: null, rightValEscape, type);
                 BoundExpression deconstructInvocation = MakeDeconstructInvocationExpression(variables.Count,
                     inputPlaceholder, rightSyntax, diagnostics, outPlaceholders: out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders, out _, variables);
 
@@ -648,7 +648,13 @@ private BoundExpression MakeDeconstructInvocationExpression(
                 {
                     var variableOpt = variablesOpt?[i].Single;
                     uint valEscape = variableOpt is null ? LocalScopeDepth : GetValEscape(variableOpt, LocalScopeDepth);
-                    var variable = new OutDeconstructVarPendingInference(receiverSyntax, valEscape);
+                    var variableSymbol = variableOpt switch
+                    {
+                        DeconstructionVariablePendingInference { VariableSymbol: var symbol } => symbol,
+                        BoundLocal { DeclarationKind: BoundLocalDeclarationKind.WithExplicitType or BoundLocalDeclarationKind.WithInferredType, LocalSymbol: var symbol } => symbol,
+                        _ => null,
+                    };
+                    var variable = new OutDeconstructVarPendingInference(receiverSyntax, variableSymbol: variableSymbol, valEscape);
                     analyzedArguments.Arguments.Add(variable);
                     analyzedArguments.RefKinds.Add(RefKind.Out);
                     outVars.Add(variable);

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

@@ -182,7 +182,7 @@ internal override BoundStatement BindForEachDeconstruction(BindingDiagnosticBag
             ExpressionSyntax variables = ((ForEachVariableStatementSyntax)_syntax).Variable;
 
             // Tracking narrowest safe-to-escape scope by default, the proper val escape will be set when doing full binding of the foreach statement
-            var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, this.LocalScopeDepth, inferredType.Type ?? CreateErrorType("var"));
+            var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, variableSymbol: null, this.LocalScopeDepth, inferredType.Type ?? CreateErrorType("var"));
 
             DeclarationExpressionSyntax declaration = null;
             ExpressionSyntax expression = null;
@@ -355,7 +355,7 @@ private BoundForEachStatement BindForEachPartsWorker(BindingDiagnosticBag diagno
                         var variables = node.Variable;
                         if (variables.IsDeconstructionLeft())
                         {
-                            var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, collectionEscape, iterationVariableType.Type).MakeCompilerGenerated();
+                            var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, variableSymbol: null, collectionEscape, iterationVariableType.Type).MakeCompilerGenerated();
                             DeclarationExpressionSyntax declaration = null;
                             ExpressionSyntax expression = null;
                             BoundDeconstructionAssignmentOperator deconstruction = BindDeconstruction(

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

@@ -104,6 +104,7 @@
   -->
   <Node Name="BoundDeconstructValuePlaceholder" Base="BoundValuePlaceholderBase">
     <Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
+    <Field Name="VariableSymbol" Type="Symbol?"/>
     <Field Name="ValEscape" Type="uint" Null="NotApplicable"/>
   </Node>
 
@@ -2379,6 +2380,7 @@
   <Node Name="OutDeconstructVarPendingInference" Base="BoundExpression">
     <!-- Type is not significant for this node type; always null -->
     <Field Name="Type" Type="TypeSymbol?" Override="true" Null="always"/>
+    <Field Name="VariableSymbol" Type="Symbol?"/>
     <Field Name="ValEscape" Type="uint" Null="NotApplicable"/>
   </Node>
 

src/Compilers/CSharp/Portable/BoundTree/OutDeconstructVarPendingInference.cs

@@ -15,7 +15,7 @@ public BoundDeconstructValuePlaceholder SetInferredTypeWithAnnotations(TypeWithA
         {
             Debug.Assert(Placeholder is null);
 
-            Placeholder = new BoundDeconstructValuePlaceholder(this.Syntax, ValEscape, type.Type, hasErrors: this.HasErrors || !success);
+            Placeholder = new BoundDeconstructValuePlaceholder(this.Syntax, variableSymbol: VariableSymbol, ValEscape, type.Type, hasErrors: this.HasErrors || !success);
             return Placeholder;
         }