Merge pull request #2826 from DavidObando/fix/issue-2818

Fix nested struct field assignment emission

Author: DavidObando

src/Core/CodeAnalysis/Binding/Binder.cs

@@ -2126,6 +2126,7 @@ private static void BindStaticInitializerBlocks(
         {
             IsStatic = true,
             StaticOwnerType = structSym,
+            IsStaticInitializer = true,
         };
 
         var previousPackage = parentScope.SetCurrentDeclaringPackage(structSym.PackageName);

src/Core/CodeAnalysis/Binding/DeclarationBinder.Structs.cs

@@ -3331,15 +3331,26 @@ private void BindDeferredFieldInitializers(
         // Bind `shared` static field initializers.
         if (staticFieldInitializers.Count > 0)
         {
-            var staticInitBuilder = ImmutableDictionary.CreateBuilder<FieldSymbol, BoundExpression>();
-            foreach (var (fieldSym, initSyntax, fieldType) in staticFieldInitializers)
+            var previousFunction = getCurrentFunction();
+            var staticInitializerContext = CreateFieldInitializerAccessibilityContext(structSymbol);
+            staticInitializerContext.IsStaticInitializer = true;
+            setCurrentFunction(staticInitializerContext);
+            try
             {
-                var boundInit = bindExpression(initSyntax);
-                var convertedInit = conversions.BindConversion(initSyntax.Location, boundInit, fieldType);
-                staticInitBuilder[fieldSym] = convertedInit;
-            }
+                var staticInitBuilder = ImmutableDictionary.CreateBuilder<FieldSymbol, BoundExpression>();
+                foreach (var (fieldSym, initSyntax, fieldType) in staticFieldInitializers)
+                {
+                    var boundInit = bindExpression(initSyntax);
+                    var convertedInit = conversions.BindConversion(initSyntax.Location, boundInit, fieldType);
+                    staticInitBuilder[fieldSym] = convertedInit;
+                }
 
-            structSymbol.SetStaticFieldInitializers(staticInitBuilder.ToImmutable());
+                structSymbol.SetStaticFieldInitializers(staticInitBuilder.ToImmutable());
+            }
+            finally
+            {
+                setCurrentFunction(previousFunction);
+            }
         }
 
         // Bind instance field initializers. These run before the constructor

src/Core/CodeAnalysis/Binding/ExpressionBinder.Assignments.cs

@@ -365,6 +365,44 @@ private bool ReceiverBlocksValueTypeMemberWrite(BoundExpression receiver)
             && !ReceiverTypeIsReference(bve.Variable.Type);
     }
 
+    private bool IsWritableStructFieldReceiver(BoundExpression receiver)
+    {
+        if (receiver is BoundVariableExpression)
+        {
+            return !ReceiverBlocksValueTypeMemberWrite(receiver);
+        }
+
+        if (receiver is BoundDereferenceExpression dereference)
+        {
+            return TypeSymbol.IsUnmanagedPointer(dereference.Operand.Type);
+        }
+
+        if (receiver is not BoundFieldAccessExpression fieldAccess)
+        {
+            return false;
+        }
+
+        if (fieldAccess.Receiver == null)
+        {
+            var staticConstructorOwner = function?.IsStaticInitializer == true
+                ? function.StaticOwnerType as Symbol
+                : null;
+            return fieldAccess.Field.IsStaticAddressLegal(staticConstructorOwner);
+        }
+
+        if (fieldAccess.Field.IsReadOnly
+            && !IsReadOnlyFieldAssignmentAllowed(
+                fieldAccess.Field,
+                fieldAccess.StructType,
+                ReceiverExpressionIsThis(fieldAccess.Receiver)))
+        {
+            return false;
+        }
+
+        return ReceiverTypeIsReference(fieldAccess.Receiver.Type)
+            || IsWritableStructFieldReceiver(fieldAccess.Receiver);
+    }
+
     /// <summary>
     /// Issue #947: returns <see langword="true"/> when the bound
     /// <paramref name="receiver"/> denotes the enclosing function's <c>this</c>
@@ -937,9 +975,21 @@ private BoundExpression BindFieldAssignmentExpression(FieldAssignmentExpressionS
             $"{structSymbol.Name}.{field.Name}");
 
         var converted = conversions.BindConversion(syntax.Value.Location, BindValue(field.Type), field.Type);
-        if (implicitFieldReceiverExpr != null
+        var usesExpressionReceiver = implicitFieldReceiverExpr != null
             || (assignmentReceiver is BoundVariableExpression narrowedVariable && narrowedVariable.NarrowedType != null)
-            || assignmentReceiver is BoundUnaryExpression)
+            || assignmentReceiver is BoundUnaryExpression;
+        if (usesExpressionReceiver
+            && !structSymbol.IsClass
+            && !IsWritableStructFieldReceiver(assignmentReceiver))
+        {
+            Diagnostics.ReportFieldAssignmentThroughStructTemporary(
+                syntax.EqualsToken.Location,
+                field.Name,
+                structSymbol);
+            return new BoundErrorExpression(syntax);
+        }
+
+        if (usesExpressionReceiver)
         {
             return BoundFieldAssignmentExpression.WithExpressionReceiver(null, assignmentReceiver, structSymbol, field, converted);
         }
@@ -1345,6 +1395,15 @@ private BoundExpression TryBindChainedCompoundAssignment(
                 Diagnostics.ReportCannotAssign(syntax.OperatorToken.Location, memberName);
             }
 
+            if (!structSym.IsClass && !IsWritableStructFieldReceiver(boundReceiver))
+            {
+                Diagnostics.ReportFieldAssignmentThroughStructTemporary(
+                    syntax.OperatorToken.Location,
+                    field.Name,
+                    structSym);
+                return new BoundErrorExpression(syntax);
+            }
+
             var leftRead = new BoundFieldAccessExpression(null, boundReceiver, declaringType, field);
             var binary = TryBindCompoundBinaryOperation(baseOpSyntaxKind, leftRead, boundRhs, syntax.Value.Location);
             if (binary == null)
@@ -1976,6 +2035,15 @@ private BoundExpression BindMemberFieldAssignmentExpression(MemberFieldAssignmen
                     $"{declaringType.Name}.{field.Name}");
 
                 var converted = conversions.BindConversion(syntax.Value.Location, BindValue(field.Type), field.Type);
+                if (!structSym.IsClass && !IsWritableStructFieldReceiver(receiver))
+                {
+                    Diagnostics.ReportFieldAssignmentThroughStructTemporary(
+                        syntax.EqualsToken.Location,
+                        field.Name,
+                        structSym);
+                    return new BoundErrorExpression(syntax);
+                }
+
                 return BoundFieldAssignmentExpression.WithExpressionReceiver(null, receiver, declaringType, field, converted);
             }
 

src/Core/CodeAnalysis/DiagnosticBag.Reports.Statements.cs

@@ -75,6 +75,19 @@ public void ReportMissingReturnExpression(TextLocation location, TypeSymbol retu
     public void ReportCannotAssign(TextLocation location, string name)
     => Report(location, DiagnosticDescriptors.CannotAssign, name);
 
+    /// <summary>
+    /// Reports a field write through a non-addressable struct value, where the
+    /// write would otherwise target a temporary copy and be discarded.
+    /// </summary>
+    /// <param name="location">The assignment operator location.</param>
+    /// <param name="fieldName">The field being assigned.</param>
+    /// <param name="receiverType">The struct receiver type.</param>
+    public void ReportFieldAssignmentThroughStructTemporary(
+        TextLocation location,
+        string fieldName,
+        TypeSymbol receiverType)
+    => Report(location, DiagnosticDescriptors.FieldAssignmentThroughStructTemporary, fieldName, receiverType);
+
     /// <summary>
     /// Issue #946: reports that an <c>init</c>-only property was assigned
     /// outside of object initialization. An <c>init</c>-only property may only

src/Core/CodeAnalysis/DiagnosticDescriptors.cs

@@ -369,6 +369,7 @@ internal static class DiagnosticDescriptors
     internal static readonly DiagnosticDescriptor AmbiguousSourceType = new("GS0496", DiagnosticSeverity.Error, "Type '{0}' is ambiguous between two or more imported packages that each declare a same-named type; qualify the reference with its package name to disambiguate (issue #2455).");
     internal static readonly DiagnosticDescriptor AsyncIteratorFunctionLiteralNotSupported = new("GS0497", DiagnosticSeverity.Error, "Async iterator function literals returning '{0}' are not supported; declare a named async iterator function and reference it instead.");
     internal static readonly DiagnosticDescriptor GotoIntoExceptionHandler = new("GS0498", DiagnosticSeverity.Error, "A goto cannot enter a catch or finally handler at label '{0}'.");
+    internal static readonly DiagnosticDescriptor FieldAssignmentThroughStructTemporary = new("GS0499", DiagnosticSeverity.Error, "Cannot modify field '{0}' of struct type '{1}' because the receiver is not writable storage. Store the receiver in a mutable variable first.");
     internal static readonly DiagnosticDescriptor CannotTakeAddressOfNonLvalue = new("GS9001", DiagnosticSeverity.Error, "Cannot take address of '{0}': expression is not an lvalue.");
     internal static readonly DiagnosticDescriptor ArgumentMustBePassedByRef = new("GS9002", DiagnosticSeverity.Error, "Argument {0} to '{1}' must be passed by reference (`&`).");
     internal static readonly DiagnosticDescriptor VariableNotDefinitelyAssignedForRef = new("GS9003", DiagnosticSeverity.Error, "Variable '{0}' must be definitely assigned before being passed by `ref`.");

src/Core/CodeAnalysis/Emit/MethodBodyEmitter.MemberAccess.cs

@@ -534,7 +534,7 @@ private void EmitFieldAccess(BoundFieldAccessExpression fa)
             // ADR-0122 §4 / issue #1034: `(*p).field` / `p->field` read. The
             // pointer value IS the struct's address, so load the pointer and
             // `ldfld` directly — avoiding a wasteful `ldobj` of the whole struct.
-            this.EmitExpression(deref.Operand);
+            this.EmitInstanceReceiver(deref.Operand);
         }
         else if (!receiverIsClass && fa.Receiver is BoundVariableExpression bv && this.TryLoadStructVariableAddress(bv))
         {
@@ -662,14 +662,11 @@ private void EmitFieldAssignment(BoundFieldAssignmentExpression fas)
                     + "Check AssignmentValueSpillCollector and its ancestor walker.");
             }
 
-            // Issue #1688: this receiver is a plain value push for `stfld`
-            // (not an address), so needAddress: false — TryEmitCachedReceiver
-            // falls back to a normal EmitExpression when no compound-reuse
-            // slot was planned (the common #1614 simple-assignment case).
-            if (!this.TryEmitCachedReceiver(addressReceiver ?? fas.ReceiverExpression, needAddress: false))
-            {
-                this.EmitExpression(addressReceiver ?? fas.ReceiverExpression);
-            }
+            // Issue #2818: stfld needs an object reference for a class
+            // receiver, but a managed pointer for a value-type receiver.
+            // EmitInstanceReceiver preserves both shapes and routes nested
+            // addressable field chains through the shared-receiver cache.
+            this.EmitInstanceReceiver(addressReceiver ?? fas.ReceiverExpression);
 
             // Issue #1235 (object-initializer follow-up): a `T{Field: value}`
             // literal on a class-constrained type parameter lowers to an
@@ -1781,7 +1778,7 @@ private void EmitFieldAddress(BoundFieldAccessExpression fa)
         {
             // ADR-0122 §4/§10: `(*p).field` / `p->field`. The pointer value IS
             // the struct's address, so load the pointer directly before ldflda.
-            this.EmitExpression(deref.Operand);
+            this.EmitInstanceReceiver(deref.Operand);
         }
         else if (!receiverIsClass && fa.Receiver is BoundVariableExpression bv && this.TryLoadStructVariableAddress(bv))
         {
@@ -1796,7 +1793,11 @@ private void EmitFieldAddress(BoundFieldAccessExpression fa)
         }
         else
         {
-            this.EmitExpression(fa.Receiver);
+            // Issue #2818: an addressable value-type field chain may have a
+            // side-effecting reference-type root shared by compound read/write
+            // (`GetHolder().Value.Id += 5`). Route the owner through the
+            // receiver cache instead of evaluating that root again.
+            this.EmitInstanceReceiver(fa.Receiver);
         }
 
         this.il.OpCode(ILOpCode.Ldflda);

src/Core/CodeAnalysis/Emit/ReflectionMetadataEmitter.cs

@@ -4299,22 +4299,7 @@ private bool CanLoadVariableAddressForReceiverSpill(
     /// <param name="field">The field whose address is being considered.</param>
     /// <returns><c>true</c> when <c>ldsflda</c> of the field is legal here.</returns>
     internal bool IsStaticFieldAddressLegalHere(FieldSymbol field)
-    {
-        if (!(field.IsReadOnly && field.IsStatic) || field.IsConst)
-        {
-            return true;
-        }
-
-        switch (this.emitCtx.CurrentStaticConstructorOwner)
-        {
-            case StructSymbol s:
-                return !s.StaticFields.IsDefaultOrEmpty && s.StaticFields.Contains(field);
-            case InterfaceSymbol i:
-                return !i.StaticFields.IsDefaultOrEmpty && i.StaticFields.Contains(field);
-            default:
-                return false;
-        }
-    }
+        => field.IsStaticAddressLegal(this.emitCtx.CurrentStaticConstructorOwner);
 
     private bool IsAddressableFieldAccessForReceiverSpill(
         BoundFieldAccessExpression fa,

src/Core/CodeAnalysis/Emit/SlotPlanner.cs

@@ -1022,8 +1022,9 @@ protected override void VisitClrPropertyAssignmentExpression(BoundClrPropertyAss
         // the shape TryBindChainedCompoundAssignment produces for
         // `getObj().Field += x` (the compound RHS embeds a
         // BoundFieldAccessExpression over the identical receiver instance) —
-        // must spill the receiver to a temp and evaluate it exactly once
-        // instead of once for the read and once more for the write.
+        // must preserve receiver identity across read and write. Reference
+        // receivers are cached directly; addressable value-type fields cache
+        // their owning receiver so the write still targets original storage.
         protected override void VisitFieldAssignmentExpression(BoundFieldAssignmentExpression node)
         {
             if (node.ReceiverExpression != null)
@@ -1163,10 +1164,37 @@ private void AddIfCompoundReused(BoundExpression receiver, BoundExpression value
             }
 
             var found = ReceiverReuseWalker.ContainsReceiverReference(value, receiver);
-            if (found)
+            if (!found)
             {
-                this.sink.Add(receiver);
+                return;
+            }
+
+            // Pointer field syntax unwraps `*p` before emit. Cache the pointer
+            // operand itself so both the field read and write reuse one value.
+            if (receiver is BoundDereferenceExpression dereference
+                && TypeSymbol.IsUnmanagedPointer(dereference.Operand.Type))
+            {
+                this.sink.Add(dereference.Operand);
+                return;
+            }
+
+            // An addressable value-type field must stay in its owning storage:
+            // caching the field value would turn the write into a temp-local
+            // mutation. Cache the side-effecting owner instead, then let
+            // EmitFieldAddress rebuild the same address for read and write.
+            if (receiver is BoundFieldAccessExpression fieldAccess
+                && ReflectionMetadataEmitter.IsValueTypeSymbol(receiver.Type)
+                && !this.needsRvalueReceiverSpill(receiver, this.function, this.locals))
+            {
+                if (fieldAccess.Receiver != null)
+                {
+                    this.AddIfCompoundReused(fieldAccess.Receiver, value);
+                }
+
+                return;
             }
+
+            this.sink.Add(receiver);
         }
     }
 

src/Core/CodeAnalysis/Symbols/FieldSymbol.cs

@@ -129,4 +129,25 @@ public void SetFixedBuffer(TypeSymbol elementType, int length)
         FixedBufferElementType = elementType;
         FixedBufferLength = length;
     }
+
+    /// <summary>
+    /// Returns whether this static field may be addressed in the current
+    /// static-constructor context.
+    /// </summary>
+    /// <param name="staticConstructorOwner">The type whose <c>.cctor</c> is active, or <c>null</c>.</param>
+    /// <returns><c>true</c> when taking the field address is legal.</returns>
+    internal bool IsStaticAddressLegal(Symbol staticConstructorOwner)
+    {
+        if (!(IsReadOnly && IsStatic) || IsConst)
+        {
+            return true;
+        }
+
+        return staticConstructorOwner switch
+        {
+            StructSymbol s => !s.StaticFields.IsDefaultOrEmpty && s.StaticFields.Contains(this),
+            InterfaceSymbol i => !i.StaticFields.IsDefaultOrEmpty && i.StaticFields.Contains(this),
+            _ => false,
+        };
+    }
 }

src/Core/CodeAnalysis/Symbols/FunctionSymbol.cs

@@ -436,6 +436,9 @@ public ImmutableArray<TypeParameterSymbol> TypeParameters
     /// <summary>Gets a value indicating whether this function is a P/Invoke stub (ADR-0086).</summary>
     public bool IsPInvoke => PInvokeMetadata != null;
 
+    /// <summary>Gets or sets a value indicating whether this synthetic function represents a type's static-constructor context.</summary>
+    internal bool IsStaticInitializer { get; set; }
+
     /// <summary>
     /// ADR-0105 Phase 2 — re-points this (reused) function symbol at the
     /// declaration node of a freshly-parsed syntax tree whose member signature