Skip to content

Remove assumption about a tree shape that compound assignment produces #79432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlekseyTs merged 1 commit into from
Jul 18, 2025
Merged

Remove assumption about a tree shape that compound assignment produces #79432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_AssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ private BoundExpression MakePropertyAssignment(
ArrayBuilder<LocalSymbol>? argTempsBuilder = null;
arguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref rewrittenReceiver,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
arguments,
property,
argsToParamsOpt,
Expand Down
34 changes: 6 additions & 28 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ BoundExpression visitArgumentsAndFinishRewrite(BoundCall node, BoundExpression?
ArrayBuilder<LocalSymbol>? temps = null;
var rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref rewrittenReceiver,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
arguments,
method,
argsToParamsOpt,
Expand Down Expand Up @@ -641,35 +641,13 @@ internal static bool IsCapturedPrimaryConstructorParameter(BoundExpression expre
primaryCtor.GetCapturedParameters().ContainsKey(parameter);
}

private enum ReceiverCaptureMode
{
/// <summary>
/// No special capture of the receiver, unless arguments need to refer to it.
/// For example, in case of a string interpolation handler.
/// </summary>
Default = 0,

/// <summary>
/// Used for a regular indexer compound assignment rewrite.
/// Everything is going to be in a single setter call with a getter call inside its value argument.
/// Only receiver and the indexes can be evaluated prior to evaluating the setter call.
/// </summary>
CompoundAssignment,

/// <summary>
/// Used for situations when additional arbitrary side-effects are possibly involved.
/// Think about deconstruction, etc.
/// </summary>
UseTwiceComplex
}

/// <summary>
/// Visits all arguments of a method, doing any necessary rewriting for interpolated string handler conversions that
/// might be present in the arguments and creating temps for any discard parameters.
/// </summary>
private ImmutableArray<BoundExpression> VisitArgumentsAndCaptureReceiverIfNeeded(
[NotNullIfNotNull(nameof(rewrittenReceiver))] ref BoundExpression? rewrittenReceiver,
ReceiverCaptureMode captureReceiverMode,
bool forceReceiverCapturing,
Copy link
Member

@jjonescz jjonescz Jul 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider documenting this parameter, since the enum documentation was removed.

ImmutableArray<BoundExpression> arguments,
Symbol methodOrIndexer,
ImmutableArray<int> argsToParamsOpt,
Expand All @@ -681,12 +659,12 @@ private ImmutableArray<BoundExpression> VisitArgumentsAndCaptureReceiverIfNeeded
Debug.Assert(argumentRefKindsOpt.IsDefault || argumentRefKindsOpt.Length == arguments.Length);
var requiresInstanceReceiver = methodOrIndexer.RequiresInstanceReceiver() && methodOrIndexer is not MethodSymbol { MethodKind: MethodKind.Constructor } and not FunctionPointerMethodSymbol;
Debug.Assert(!requiresInstanceReceiver || rewrittenReceiver != null || _inExpressionLambda);
Debug.Assert(captureReceiverMode == ReceiverCaptureMode.Default || (requiresInstanceReceiver && rewrittenReceiver != null && storesOpt is object));
Debug.Assert(!forceReceiverCapturing || (requiresInstanceReceiver && rewrittenReceiver != null && storesOpt is object));

BoundLocal? receiverTemp = null;
BoundAssignmentOperator? assignmentToTemp = null;

if (captureReceiverMode != ReceiverCaptureMode.Default ||
if (forceReceiverCapturing ||
(requiresInstanceReceiver && arguments.Any(a => usesReceiver(a))))
{
Debug.Assert(!_inExpressionLambda);
Expand All @@ -695,7 +673,7 @@ private ImmutableArray<BoundExpression> VisitArgumentsAndCaptureReceiverIfNeeded

RefKind refKind;

if (captureReceiverMode != ReceiverCaptureMode.Default)
if (forceReceiverCapturing)
{
// SPEC VIOLATION: It is not very clear when receiver of constrained callvirt is dereferenced - when pushed (in lexical order),
// SPEC VIOLATION: or when actual call is executed. The actual behavior seems to be implementation specific in different JITs.
Expand Down Expand Up @@ -802,7 +780,7 @@ private ImmutableArray<BoundExpression> VisitArgumentsAndCaptureReceiverIfNeeded
if (receiverTemp.LocalSymbol.IsRef &&
CodeGenerator.IsPossibleReferenceTypeReceiverOfConstrainedCall(receiverTemp) &&
!CodeGenerator.ReceiverIsKnownToReferToTempIfReferenceType(receiverTemp) &&
(captureReceiverMode == ReceiverCaptureMode.UseTwiceComplex ||
(forceReceiverCapturing ||
!CodeGenerator.IsSafeToDereferenceReceiverRefAfterEvaluatingArguments(rewrittenArguments)))
{
ReferToTempIfReferenceTypeReceiver(receiverTemp, ref assignmentToTemp, out extraRefInitialization, tempsOpt);
Expand Down
14 changes: 5 additions & 9 deletions ...pilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ private BoundExpression VisitBuiltInOrStaticCompoundAssignmentOperator(BoundComp

// This will be filled in with the LHS that uses temporaries to prevent
// double-evaluation of side effects.
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(node.Left, isRegularCompoundAssignment: true, stores, temps, isDynamic);
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(node.Left, stores, temps, isDynamic);
var lhsRead = MakeRValue(transformedLHS);
BoundExpression rewrittenAssignment;

Expand Down Expand Up @@ -341,7 +341,7 @@ private BoundDynamicMemberAccess TransformDynamicMemberAccess(BoundDynamicMember
return new BoundDynamicMemberAccess(memberAccess.Syntax, receiverTemp, memberAccess.TypeArgumentsOpt, memberAccess.Name, memberAccess.Invoked, memberAccess.Indexed, memberAccess.Type);
}

private BoundIndexerAccess TransformIndexerAccess(BoundIndexerAccess indexerAccess, bool isRegularCompoundAssignment, ArrayBuilder<BoundExpression> stores, ArrayBuilder<LocalSymbol> temps)
private BoundIndexerAccess TransformIndexerAccess(BoundIndexerAccess indexerAccess, ArrayBuilder<BoundExpression> stores, ArrayBuilder<LocalSymbol> temps)
{
var receiverOpt = indexerAccess.ReceiverOpt;
Debug.Assert(receiverOpt != null);
Expand Down Expand Up @@ -395,11 +395,7 @@ private BoundIndexerAccess TransformIndexerAccess(BoundIndexerAccess indexerAcce

ImmutableArray<BoundExpression> rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref transformedReceiver,
captureReceiverMode: CanChangeValueBetweenReads(receiverOpt) ?
(isRegularCompoundAssignment ?
ReceiverCaptureMode.CompoundAssignment :
ReceiverCaptureMode.UseTwiceComplex) :
ReceiverCaptureMode.Default,
forceReceiverCapturing: CanChangeValueBetweenReads(receiverOpt),
indexerAccess.Arguments,
indexerAccess.Indexer,
indexerAccess.ArgsToParamsOpt,
Expand Down Expand Up @@ -657,7 +653,7 @@ private BoundDynamicIndexerAccess TransformDynamicIndexerAccess(BoundDynamicInde
/// A side-effect-free expression representing the LHS.
/// The returned node needs to be lowered but its children are already lowered.
/// </returns>
private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalLHS, bool isRegularCompoundAssignment, ArrayBuilder<BoundExpression> stores, ArrayBuilder<LocalSymbol> temps, bool isDynamicAssignment)
private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalLHS, ArrayBuilder<BoundExpression> stores, ArrayBuilder<LocalSymbol> temps, bool isDynamicAssignment)
{
// There are five possible cases.
//
Expand Down Expand Up @@ -710,7 +706,7 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
var indexerAccess = (BoundIndexerAccess)originalLHS;
if (indexerAccess.GetRefKind() == RefKind.None)
{
return TransformIndexerAccess((BoundIndexerAccess)originalLHS, isRegularCompoundAssignment, stores, temps);
return TransformIndexerAccess((BoundIndexerAccess)originalLHS, stores, temps);
}
}
break;
Expand Down
2 changes: 1 addition & 1 deletion .../CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DeconstructionAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ private BoundExpression EvaluateSideEffectingArgumentToTemp(

default:
Debug.Assert(variable.Type is { });
var temp = this.TransformCompoundAssignmentLHS(variable, isRegularCompoundAssignment: false,
var temp = this.TransformCompoundAssignmentLHS(variable,
effects, temps, isDynamicAssignment: variable.Type.IsDynamic());
assignmentTargets.Add(new Binder.DeconstructionVariable(temp, variable.Syntax));
break;
Expand Down
2 changes: 1 addition & 1 deletion ...mpilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FunctionPointerInvocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ internal sealed partial class LocalRewriter
ArrayBuilder<LocalSymbol>? temps = null;
var rewrittenArgs = VisitArgumentsAndCaptureReceiverIfNeeded(
rewrittenReceiver: ref discardedReceiver,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
node.Arguments,
functionPointer,
argsToParamsOpt: default,
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ private BoundExpression MakeIndexerAccess(
ArrayBuilder<LocalSymbol>? temps = null;
ImmutableArray<BoundExpression> rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref rewrittenReceiver,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
arguments,
indexer,
argsToParamsOpt,
Expand Down Expand Up @@ -575,7 +575,7 @@ private BoundExpression GetUnderlyingIndexerOrSliceAccess(
AddPlaceholderReplacement(argumentPlaceholder, integerArgument);
ImmutableArray<BoundExpression> rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref receiver,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
indexerAccess.Arguments,
indexerAccess.Indexer,
indexerAccess.ArgsToParamsOpt,
Expand Down
2 changes: 1 addition & 1 deletion .../CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_NullCoalescingAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public override BoundNode VisitNullCoalescingAssignmentOperator(BoundNullCoalesc
Debug.Assert(node.LeftOperand.Type is { });

// Rewrite LHS with temporaries to prevent double-evaluation of side effects, as we'll need to use it multiple times.
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(node.LeftOperand, isRegularCompoundAssignment: false, stores, temps, node.LeftOperand.HasDynamicType());
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(node.LeftOperand, stores, temps, node.LeftOperand.HasDynamicType());
Debug.Assert(transformedLHS.Type is { });
var lhsRead = MakeRValue(transformedLHS);
BoundExpression loweredRight = VisitExpression(node.RightOperand);
Expand Down
2 changes: 1 addition & 1 deletion ...ompilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectCreationExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public override BoundNode VisitObjectCreationExpression(BoundObjectCreationExpre
ArrayBuilder<LocalSymbol>? tempsBuilder = null;
ImmutableArray<BoundExpression> rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref receiverDiscard,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
node.Arguments,
constructor,
node.ArgsToParamsOpt,
Expand Down
4 changes: 2 additions & 2 deletions .../Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ private BoundExpression MakeDynamicCollectionInitializer(BoundExpression rewritt
ArrayBuilder<LocalSymbol>? temps = null;
ImmutableArray<BoundExpression> rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref rewrittenReceiver,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
initializer.Arguments,
addMethod,
initializer.ArgsToParamsOpt,
Expand Down Expand Up @@ -251,7 +251,7 @@ private BoundExpression VisitObjectInitializerMember(BoundObjectInitializerMembe

var originalReceiver = rewrittenReceiver;
ArrayBuilder<LocalSymbol>? constructionTemps = null;
var rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(ref rewrittenReceiver, captureReceiverMode: ReceiverCaptureMode.Default, node.Arguments, node.MemberSymbol, node.ArgsToParamsOpt, node.ArgumentRefKindsOpt,
var rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(ref rewrittenReceiver, forceReceiverCapturing: false, node.Arguments, node.MemberSymbol, node.ArgsToParamsOpt, node.ArgumentRefKindsOpt,
storesOpt: null, ref constructionTemps);

if (constructionTemps != null)
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UnaryOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ private BoundExpression MakeInstanceCompoundAssignmentOperatorResult(SyntaxNode

// This will be filled in with the LHS that uses temporaries to prevent
// double-evaluation of side effects.
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(left, isRegularCompoundAssignment: true, tempInitializers, tempSymbols, isDynamicAssignment: false);
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(left, tempInitializers, tempSymbols, isDynamicAssignment: false);
Debug.Assert(TypeSymbol.Equals(operandType, transformedLHS.Type, TypeCompareKind.AllIgnoreOptions));

BoundAssignmentOperator tempAssignment;
Expand Down Expand Up @@ -582,7 +582,7 @@ public BoundExpression VisitBuiltInOrStaticIncrementOperator(BoundIncrementOpera

// This will be filled in with the LHS that uses temporaries to prevent
// double-evaluation of side effects.
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(node.Operand, isRegularCompoundAssignment: false, tempInitializers, tempSymbols, isDynamic);
BoundExpression transformedLHS = TransformCompoundAssignmentLHS(node.Operand, tempInitializers, tempSymbols, isDynamic);
TypeSymbol? operandType = transformedLHS.Type; //type of the variable being incremented
Debug.Assert(operandType is { });
Debug.Assert(TypeSymbol.Equals(operandType, node.Type, TypeCompareKind.ConsiderEverything2));
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ private BoundExpression MakeCall(MethodArgumentInfo methodArgumentInfo, SyntaxNo

var rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref expression,
captureReceiverMode: ReceiverCaptureMode.Default,
forceReceiverCapturing: false,
methodArgumentInfo.Arguments,
method,
argsToParamsOpt: default,
Expand Down
31 changes: 16 additions & 15 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenOperators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5228,7 +5228,7 @@ public static void Repro2(T arg)
1");
compilation.VerifyIL("test<T>.Repro1(T)", @"
{
// Code size 86 (0x56)
// Code size 84 (0x54)
.maxstack 4
.locals init (T& V_0,
T V_1)
Expand All @@ -5251,20 +5251,21 @@ .locals init (T& V_0,
IL_0026: add
IL_0027: constrained. ""T""
IL_002d: callvirt ""void c0.P1.set""
IL_0032: ldarga.s V_0
IL_0034: stloc.0
IL_0035: ldloc.0
IL_0036: ldobj ""T""
IL_003b: box ""T""
IL_0040: ldc.i4.1
IL_0041: ldloc.0
IL_0042: ldc.i4.1
IL_0043: constrained. ""T""
IL_0049: callvirt ""int c0.this[int].get""
IL_004e: ldc.i4.1
IL_004f: add
IL_0050: callvirt ""void c0.this[int].set""
IL_0055: ret
IL_0032: ldarg.0
IL_0033: stloc.1
IL_0034: ldloca.s V_1
IL_0036: stloc.0
IL_0037: ldloc.0
IL_0038: ldc.i4.1
IL_0039: ldloc.0
IL_003a: ldc.i4.1
IL_003b: constrained. ""T""
IL_0041: callvirt ""int c0.this[int].get""
IL_0046: ldc.i4.1
IL_0047: add
IL_0048: constrained. ""T""
IL_004e: callvirt ""void c0.this[int].set""
IL_0053: ret
}
").VerifyIL("test<T>.Repro2(T)", @"
{
Expand Down
Loading