Fix receiver capturing for various extension scenarios (#79472)

Fixes #79379
Fixes #79436

Author: AlekseyTs

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

@@ -720,28 +720,39 @@ private void EmitArgument(BoundExpression argument, RefKind refKind)
                     EmitExpression(argument, true);
                     break;
 
-                case RefKind.In:
-                    var temp = EmitAddress(argument, AddressKind.ReadOnly);
-                    AddExpressionTemp(temp);
-                    break;
-
                 default:
-                    Debug.Assert(refKind is RefKind.Ref or RefKind.Out or RefKindExtensions.StrictIn);
-                    // NOTE: passing "ReadOnlyStrict" here. 
-                    //       we should not get an address of a copy if at all possible
-                    var unexpectedTemp = EmitAddress(argument, refKind == RefKindExtensions.StrictIn ? AddressKind.ReadOnlyStrict : AddressKind.Writeable);
-                    if (unexpectedTemp != null)
+                    Debug.Assert(refKind is RefKind.In or RefKind.Ref or RefKind.Out or RefKindExtensions.StrictIn);
+                    var temp = EmitAddress(argument, GetArgumentAddressKind(refKind));
+                    if (temp != null)
                     {
                         // interestingly enough "ref dynamic" sometimes is passed via a clone
                         // receiver of a ref field can be cloned too
-                        Debug.Assert(argument.Type.IsDynamic() || argument is BoundFieldAccess { FieldSymbol.RefKind: not RefKind.None }, "passing args byref should not clone them into temps");
-                        AddExpressionTemp(unexpectedTemp);
+                        Debug.Assert(refKind is RefKind.In || argument.Type.IsDynamic() || argument is BoundFieldAccess { FieldSymbol.RefKind: not RefKind.None }, "passing args byref should not clone them into temps");
+                        AddExpressionTemp(temp);
                     }
 
                     break;
             }
         }
 
+        internal static AddressKind GetArgumentAddressKind(RefKind refKind)
+        {
+            switch (refKind)
+            {
+                case RefKind.None:
+                    throw ExceptionUtilities.UnexpectedValue(refKind);
+
+                case RefKind.In:
+                    return AddressKind.ReadOnly;
+
+                default:
+                    Debug.Assert(refKind is RefKind.Ref or RefKind.Out or RefKindExtensions.StrictIn);
+                    // NOTE: returning "ReadOnlyStrict" here. 
+                    //       we should not get an address of a copy if at all possible
+                    return refKind == RefKindExtensions.StrictIn ? AddressKind.ReadOnlyStrict : AddressKind.Writeable;
+            }
+        }
+
         private void EmitAddressOfExpression(BoundAddressOfOperator expression, bool used)
         {
             // NOTE: passing "ReadOnlyStrict" here. 

src/Compilers/CSharp/Portable/Lowering/ExtensionMethodReferenceRewriter.cs

@@ -109,13 +109,13 @@ static BoundExpression updateCall(
                         if (receiverRefKind != RefKind.None)
                         {
                             var builder = ArrayBuilder<RefKind>.GetInstance(method.ParameterCount, RefKind.None);
-                            builder[0] = argumentRefKindFromReceiverRefKind(receiverRefKind);
+                            builder[0] = ReceiverArgumentRefKindFromReceiverRefKind(receiverRefKind);
                             argumentRefKinds = builder.ToImmutableAndFree();
                         }
                     }
                     else
                     {
-                        argumentRefKinds = argumentRefKinds.Insert(0, argumentRefKindFromReceiverRefKind(receiverRefKind));
+                        argumentRefKinds = argumentRefKinds.Insert(0, ReceiverArgumentRefKindFromReceiverRefKind(receiverRefKind));
                     }
 
                     invokedAsExtensionMethod = true;
@@ -141,14 +141,14 @@ static BoundExpression updateCall(
                     boundCall.ResultKind,
                     originalMethodsOpt,
                     type);
-
-                static RefKind argumentRefKindFromReceiverRefKind(RefKind receiverRefKind)
-                {
-                    return SyntheticBoundNodeFactory.ArgumentRefKindFromParameterRefKind(receiverRefKind, useStrictArgumentRefKinds: false);
-                }
             }
         }
 
+        public static RefKind ReceiverArgumentRefKindFromReceiverRefKind(RefKind receiverRefKind)
+        {
+            return SyntheticBoundNodeFactory.ArgumentRefKindFromParameterRefKind(receiverRefKind, useStrictArgumentRefKinds: false);
+        }
+
         [return: NotNullIfNotNull(nameof(method))]
         private static MethodSymbol? VisitMethodSymbolWithExtensionRewrite(BoundTreeRewriter rewriter, MethodSymbol? method)
         {

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

@@ -81,15 +81,24 @@ private BoundExpression VisitAssignmentOperator(BoundAssignmentOperator node, bo
                     break;
             }
 
-            return MakeStaticAssignmentOperator(node.Syntax, loweredLeft, loweredRight, node.IsRef, used);
+            return MakeStaticAssignmentOperator(node.Syntax, loweredLeft, loweredRight, node.IsRef, used, AssignmentKind.SimpleAssignment);
+        }
+
+        private enum AssignmentKind
+        {
+            SimpleAssignment,
+            CompoundAssignment,
+            IncrementDecrement,
+            Deconstruction,
+            NullCoalescingAssignment,
         }
 
         /// <summary>
         /// Generates a lowered form of the assignment operator for the given left and right sub-expressions.
         /// Left and right sub-expressions must be in lowered form.
         /// </summary>
         private BoundExpression MakeAssignmentOperator(SyntaxNode syntax, BoundExpression rewrittenLeft, BoundExpression rewrittenRight,
-            bool used, bool isChecked, bool isCompoundAssignment)
+            bool used, bool isChecked, AssignmentKind assignmentKind)
         {
             switch (rewrittenLeft.Kind)
             {
@@ -102,15 +111,15 @@ private BoundExpression MakeAssignmentOperator(SyntaxNode syntax, BoundExpressio
                         indexerAccess.ArgumentNamesOpt,
                         indexerAccess.ArgumentRefKindsOpt,
                         rewrittenRight,
-                        isCompoundAssignment, isChecked);
+                        isCompoundAssignment: assignmentKind == AssignmentKind.CompoundAssignment, isChecked);
 
                 case BoundKind.DynamicMemberAccess:
                     var memberAccess = (BoundDynamicMemberAccess)rewrittenLeft;
                     return _dynamicFactory.MakeDynamicSetMember(
                         memberAccess.Receiver,
                         memberAccess.Name,
                         rewrittenRight,
-                        isCompoundAssignment,
+                        isCompoundAssignment: assignmentKind == AssignmentKind.CompoundAssignment,
                         isChecked).ToExpression();
 
                 case BoundKind.EventAccess:
@@ -132,7 +141,7 @@ private BoundExpression MakeAssignmentOperator(SyntaxNode syntax, BoundExpressio
                     throw ExceptionUtilities.Unreachable();
 
                 default:
-                    return MakeStaticAssignmentOperator(syntax, rewrittenLeft, rewrittenRight, isRef: false, used: used);
+                    return MakeStaticAssignmentOperator(syntax, rewrittenLeft, rewrittenRight, isRef: false, used: used, assignmentKind);
             }
         }
 
@@ -168,7 +177,8 @@ private BoundExpression MakeStaticAssignmentOperator(
             BoundExpression rewrittenLeft,
             BoundExpression rewrittenRight,
             bool isRef,
-            bool used)
+            bool used,
+            AssignmentKind assignmentKind)
         {
             switch (rewrittenLeft.Kind)
             {
@@ -192,7 +202,8 @@ private BoundExpression MakeStaticAssignmentOperator(
                             false,
                             default(ImmutableArray<int>),
                             rewrittenRight,
-                            used);
+                            used,
+                            assignmentKind);
                     }
 
                 case BoundKind.IndexerAccess:
@@ -212,7 +223,8 @@ private BoundExpression MakeStaticAssignmentOperator(
                             indexerAccess.Expanded,
                             indexerAccess.ArgsToParamsOpt,
                             rewrittenRight,
-                            used);
+                            used,
+                            assignmentKind);
                     }
 
                 case BoundKind.Local:
@@ -248,7 +260,8 @@ private BoundExpression MakeStaticAssignmentOperator(
                                 sequence.Value,
                                 rewrittenRight,
                                 isRef,
-                                used),
+                                used,
+                                assignmentKind),
                             sequence.Type);
                     }
                     goto default;
@@ -264,6 +277,17 @@ private BoundExpression MakeStaticAssignmentOperator(
             }
         }
 
+        private bool IsExtensionPropertyWithByValPossiblyStructReceiverWhichHasHomeAndCanChangeValueBetweenReads(BoundExpression rewrittenReceiver, PropertySymbol property)
+        {
+            return CanChangeValueBetweenReads(rewrittenReceiver, localsMayBeAssignedOrCaptured: true, structThisCanChangeValueBetweenReads: true) &&
+                   IsNewExtensionMemberWithByValPossiblyStructReceiver(property) &&
+                   CodeGen.CodeGenerator.HasHome(rewrittenReceiver,
+                                       CodeGen.CodeGenerator.AddressKind.ReadOnlyStrict,
+                                       _factory.CurrentFunction,
+                                       peVerifyCompatEnabled: false,
+                                       stackLocalsOpt: null);
+        }
+
         private BoundExpression MakePropertyAssignment(
             SyntaxNode syntax,
             BoundExpression? rewrittenReceiver,
@@ -273,7 +297,8 @@ private BoundExpression MakePropertyAssignment(
             bool expanded,
             ImmutableArray<int> argsToParamsOpt,
             BoundExpression rewrittenRight,
-            bool used)
+            bool used,
+            AssignmentKind assignmentKind)
         {
             // Rewrite property assignment into call to setter.
             var setMethod = property.GetOwnOrInheritedSetMethod();
@@ -293,25 +318,73 @@ private BoundExpression MakePropertyAssignment(
             }
 
             ArrayBuilder<LocalSymbol>? argTempsBuilder = null;
+
+            bool needSpecialExtensionPropertyReceiverReadOrder = false;
+            ArrayBuilder<BoundExpression>? storesOpt = null;
+
+            if (rewrittenReceiver is not null &&
+                assignmentKind is not (AssignmentKind.CompoundAssignment or AssignmentKind.NullCoalescingAssignment or AssignmentKind.Deconstruction or AssignmentKind.IncrementDecrement) &&
+                IsExtensionPropertyWithByValPossiblyStructReceiverWhichHasHomeAndCanChangeValueBetweenReads(rewrittenReceiver, property) &&
+                (arguments.Length != 0 || !IsSafeForReordering(rewrittenRight, RefKind.None)))
+            {
+                // The receiver has location, but extension property/indexer takes receiver by value.
+                // This means that we need to ensure that the receiver value is read after
+                // any side-effecting arguments and right hand side are evaluated, so that the
+                // setter receives the last value of the receiver, not the value before the
+                // arguments/rhs were evaluated. Receiver side effects should be evaluated at
+                // the very beginning, of course.
+
+                needSpecialExtensionPropertyReceiverReadOrder = true;
+                storesOpt = ArrayBuilder<BoundExpression>.GetInstance();
+            }
+
+#if DEBUG
+            BoundExpression? rewrittenReceiverBeforePossibleCapture = rewrittenReceiver;
+#endif
             arguments = VisitArgumentsAndCaptureReceiverIfNeeded(
                 ref rewrittenReceiver,
-                forceReceiverCapturing: false,
+                forceReceiverCapturing: needSpecialExtensionPropertyReceiverReadOrder,
                 arguments,
                 property,
                 argsToParamsOpt,
                 argumentRefKindsOpt,
-                storesOpt: null,
+                storesOpt,
                 ref argTempsBuilder);
 
-            arguments = MakeArguments(
-                arguments,
-                property,
-                expanded,
-                argsToParamsOpt,
-                ref argumentRefKindsOpt,
-                ref argTempsBuilder,
-                invokedAsExtensionMethod: false);
+            if (needSpecialExtensionPropertyReceiverReadOrder)
+            {
+#if DEBUG
+                Debug.Assert(rewrittenReceiverBeforePossibleCapture != (object?)rewrittenReceiver);
+#endif
+                Debug.Assert(storesOpt is { });
+                Debug.Assert(storesOpt.Count != 0);
+                Debug.Assert(argTempsBuilder is not null);
+
+                // Store everything that is non-trivial into a temporary; record the
+                // stores in storesToTemps and make the actual argument a reference to the temp.
+                arguments = ExtractSideEffectsFromArguments(arguments, property, expanded, argsToParamsOpt, ref argumentRefKindsOpt, storesOpt, argTempsBuilder);
+
+                if (!IsSafeForReordering(rewrittenRight, RefKind.None))
+                {
+                    BoundLocal capturedRight = _factory.StoreToTemp(rewrittenRight, out BoundAssignmentOperator assignmentToTemp, refKind: RefKind.None);
+                    argTempsBuilder.Add(capturedRight.LocalSymbol);
+                    storesOpt.Add(assignmentToTemp);
+                    rewrittenRight = capturedRight;
+                }
+            }
+            else
+            {
+                arguments = MakeArguments(
+                    arguments,
+                    property,
+                    expanded,
+                    argsToParamsOpt,
+                    ref argumentRefKindsOpt,
+                    ref argTempsBuilder,
+                    invokedAsExtensionMethod: false);
+            }
 
+            var sideEffects = storesOpt is null ? [] : storesOpt.ToImmutableAndFree();
             var argTemps = argTempsBuilder.ToImmutableAndFree();
 
             if (used)
@@ -342,7 +415,7 @@ private BoundExpression MakePropertyAssignment(
                 return new BoundSequence(
                     syntax,
                     AppendToPossibleNull(argTemps, rhsTemp),
-                    ImmutableArray.Create(setterCall),
+                    sideEffects.Add(setterCall), // https://github.com/dotnet/roslyn/issues/78829 - there is no test coverage for 'sideEffects' on this code path
                     boundRhs,
                     rhsTemp.Type);
             }
@@ -357,14 +430,15 @@ private BoundExpression MakePropertyAssignment(
 
                 if (argTemps.IsDefaultOrEmpty)
                 {
+                    Debug.Assert(sideEffects.IsEmpty);
                     return setterCall;
                 }
                 else
                 {
                     return new BoundSequence(
                         syntax,
                         argTemps,
-                        ImmutableArray<BoundExpression>.Empty,
+                        sideEffects,
                         setterCall,
                         setMethod.ReturnType);
                 }