Skip to content

Fix receiver capturing for various extension scenarios #79472

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 9 commits into from
Jul 25, 2025
Merged

Fix receiver capturing for various extension scenarios #79472

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b079ba0
Temporarily add limited support for extension indexers
AlekseyTs Jul 17, 2025
cf4369f
Add tests for interesting scenarios
AlekseyTs Jul 17, 2025
df39446
More interesting tests
AlekseyTs Jul 17, 2025
47f2aa8
Adjust a test
AlekseyTs Jul 17, 2025
f478367
Fix receiver capturing for various extension scenarios
AlekseyTs Jul 18, 2025
eacecf1
Revert "Temporarily add limited support for extension indexers"
AlekseyTs Jul 18, 2025
814baf6
Disable tests for indexers
AlekseyTs Jul 18, 2025
3951497
PR feedback
AlekseyTs Jul 25, 2025
f6cbd13
Fixup a test baseline
AlekseyTs Jul 25, 2025
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
35 changes: 23 additions & 12 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
14 changes: 7 additions & 7 deletions src/Compilers/CSharp/Portable/Lowering/ExtensionMethodReferenceRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
{
Expand Down
118 changes: 96 additions & 22 deletions 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 @@ -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)
{
Expand All @@ -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:
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -168,7 +177,8 @@ private BoundExpression MakeStaticAssignmentOperator(
BoundExpression rewrittenLeft,
BoundExpression rewrittenRight,
bool isRef,
bool used)
bool used,
AssignmentKind assignmentKind)
{
switch (rewrittenLeft.Kind)
{
Expand All @@ -192,7 +202,8 @@ private BoundExpression MakeStaticAssignmentOperator(
false,
default(ImmutableArray<int>),
rewrittenRight,
used);
used,
assignmentKind);
}

case BoundKind.IndexerAccess:
Expand All @@ -212,7 +223,8 @@ private BoundExpression MakeStaticAssignmentOperator(
indexerAccess.Expanded,
indexerAccess.ArgsToParamsOpt,
rewrittenRight,
used);
used,
assignmentKind);
}

case BoundKind.Local:
Expand Down Expand Up @@ -248,7 +260,8 @@ private BoundExpression MakeStaticAssignmentOperator(
sequence.Value,
rewrittenRight,
isRef,
used),
used,
assignmentKind),
sequence.Type);
}
goto default;
Expand All @@ -264,6 +277,17 @@ private BoundExpression MakeStaticAssignmentOperator(
}
}

private bool IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(BoundExpression rewrittenReceiver, PropertySymbol property)
Copy link
Member

@jcouv jcouv Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads

Consider renaming to IsExtensionPropertyWithByValPossiblyStructReceiverWhichHasHomeAndCanChangeValueBetweenReads as only extension properties qualify and to avoid multiplying concepts ("home" versus "location"). #Closed

{
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,
Expand All @@ -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();
Expand All @@ -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) &&
Copy link
Member

@jjonescz jjonescz Jul 22, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Should we check that assignmentKind is AssignmentKind.SimpleAssignment instead? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jul 22, 2025

Choose a reason for hiding this comment

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

I am checking for assignments that I know don't need special treatment. Doing an opposite check is not robust in the long run, if new kind is added.

IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(rewrittenReceiver, property) &&
Copy link
Member

@jjonescz jjonescz Jul 22, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Do you now get some trophy for the longest method name in the codebase? :D #Resolved

(arguments.Length != 0 || !IsSafeForReordering(rewrittenRight, RefKind.None)))
{
// The receiever has location, but extension property/indexer takes receiver by value.
Copy link
Member

@jcouv jcouv Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Typo:

Suggested change
// The receiever has location, but extension property/indexer takes receiver by value.
// The receiver has location, but extension property/indexer takes receiver by value.
``` #Closed

// This means that we need to ensure that the the receiver value is read after
Copy link
Member

@jjonescz jjonescz Jul 22, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Suggested change
// This means that we need to ensure that the the receiver value is read after
// This means that we need to ensure that the receiver value is read after
``` #Resolved

// any side-effecting arguments and right hand side are evaluated, so that the
// the setter receives the last value of the receiver, not the value before the
Copy link
Member

@jjonescz jjonescz Jul 22, 2025 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Suggested change
// the setter receives the last value of the receiver, not the value before the
// setter receives the last value of the receiver, not the value before the
``` #Closed

// arguments/rhs were evaluated. Receiver sideeffects should be evaluated at
// the very beginning, of course.
Copy link
Member

@jjonescz jjonescz Jul 22, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

What happens when the receiver does not have a home; is that always an error scenario? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What happens when the receiver does not have a home; is that always an error scenario?

Not necessary. If the receiver doesn't have home, no one can change its value after we read it. Therefore, we don't need to worry about capturing it by reference and dereferencing it right before invoking the accessor. If we try to do so, the value will be captured in a temp anyway.


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))
Copy link
Member

@jjonescz jjonescz Jul 22, 2025 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Consider caching and reusing the result of IsSafeForReordering(rewrittenRight, RefKind.None) from above. I guess it might not be always computed, but perhaps having a bool? and recomputing it only if null would still be worth it. #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Jul 22, 2025

Choose a reason for hiding this comment

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

Consider caching and reusing the result of

I prefer to not complicate code trying to optimize something that is not yet proven to be a problem

{
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)
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
Loading
Loading