Fix receiver capturing for various extension scenarios #79472

AlekseyTs · 2025-07-18T19:57:12Z

Fixes #79379
Fixes #79436

Fixes dotnet#79379 Fixes dotnet#79436

This reverts commit c2412e0b5ecd6b14f4ba9d7a66a52e05f511128f.

AlekseyTs · 2025-07-21T11:16:32Z

/azp run

azure-pipelines · 2025-07-21T11:16:50Z

Azure Pipelines successfully started running 2 pipeline(s).

AlekseyTs · 2025-07-21T11:26:05Z

@jcouv, @jjonescz, @dotnet/roslyn-compiler Please review

AlekseyTs · 2025-07-21T15:33:37Z

/azp run

azure-pipelines · 2025-07-21T15:33:55Z

Azure Pipelines successfully started running 2 pipeline(s).

jcouv · 2025-07-21T21:42:13Z

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

                }

-                receiverTemp = _factory.StoreToTemp(rewrittenReceiver, out assignmentToTemp, refKind);
+                receiverTemp = _factory.StoreToTemp(rewrittenReceiver, out assignmentToTemp, (refKind is RefKind.RefReadOnlyParameter or RefKind.In) ? RefKindExtensions.StrictIn : refKind);


(refKind is RefKind.RefReadOnlyParameter or RefKind.In) ? RefKindExtensions.StrictIn : refKind

I didn't follow why we need this change. When I revert it, it doesn't seem to affect any extensions test.
If the change is desirable, would we also want it in TransformPropertyOrEventReceiver when computing the refKind for the temp for the receiver? #Closed

When I revert it, it doesn't seem to affect any extensions test.

Added InterpolationTests.StructReceiver_Lvalue_08 which crashes compiler without the change.

Adding similar change to TransformPropertyOrEventReceiver for consistency and future proofing

jcouv · 2025-07-21T21:47:48Z

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

+
+                if (IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(rewrittenReceiver, indexer))
+                {
+                    // The receiever has location, but extension indexer takes receiver by value.


Typo:

Suggested change

// The receiever has location, but extension indexer takes receiver by value.

// The receiver has location, but extension indexer takes receiver by value.

``` #Closed

jcouv · 2025-07-21T21:48:42Z

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

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


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

jcouv · 2025-07-21T21:50:15Z

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

            }
        }

+        private bool IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(BoundExpression rewrittenReceiver, PropertySymbol property)


IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads

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

jcouv · 2025-07-21T21:50:50Z

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

+                ArrayBuilder<BoundExpression>? storesOpt = null;
+
+                if (IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(rewrittenReceiver, indexer))
+                {


~~Consider leaving follow-up comment to cover this logic when implementing extension indexers~~ Never mind, I see you added skipped tests #Closed

jcouv · 2025-07-21T22:02:36Z

public void PropertyAccess_Set_01()

nit: consider adding tests for disambiguation invocation syntax (E.set_P1(this, Program.Get1()), where the setter will see the old value of the struct) and instance invocation scenarios (S1 implements P1, where the setter sees the updated value of the struct) #Closed

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:5570 in 814baf6. [](commit_id = 814baf6, deletion_comment = False)

jcouv · 2025-07-21T22:09:06Z

public void IndexerAccess_CompoundAssignment_01()

Thank you!

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:11763 in 814baf6. [](commit_id = 814baf6, deletion_comment = False)

jcouv

LGTM Thanks (commit 7) with some nits to consider

AlekseyTs · 2025-07-22T08:22:29Z

@jjonescz, @dotnet/roslyn-compiler For a second review

jjonescz · 2025-07-22T08:31:27Z

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

+            ArrayBuilder<BoundExpression>? storesOpt = null;
+
+            if (rewrittenReceiver is not null &&
+                assignmentKind is not (AssignmentKind.CompoundAssignment or AssignmentKind.NullCoalescingAssignment or AssignmentKind.Deconstruction or AssignmentKind.IncrementDecrement) &&


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

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.

jjonescz · 2025-07-22T08:32:01Z

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

+                (arguments.Length != 0 || !IsSafeForReordering(rewrittenRight, RefKind.None)))
+            {
+                // The receiever has location, but extension property/indexer takes receiver by value.
+                // This means that we need to ensure that the the receiver value is read after


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

jjonescz · 2025-07-22T08:32:22Z

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

+                // The receiever has location, but extension property/indexer takes receiver by value.
+                // This means that we need to ensure that the the receiver value is read after
+                // 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


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

jjonescz · 2025-07-22T08:37:09Z

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

+                // 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))


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

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

jjonescz · 2025-07-22T08:38:10Z

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

+
+            if (rewrittenReceiver is not null &&
+                assignmentKind is not (AssignmentKind.CompoundAssignment or AssignmentKind.NullCoalescingAssignment or AssignmentKind.Deconstruction or AssignmentKind.IncrementDecrement) &&
+                IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(rewrittenReceiver, property) &&


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

jjonescz · 2025-07-22T08:51:53Z

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

+        ///   the sideeffects of capturing are the sifeeffects of the sequence and its result is the captured value.
+        ///   
+        /// All temps introduced by this function for capturing purposes (including the temp capturing the receiver) are appended
+        /// to <paramref name="tempsOpt"/>, which is allocated, if 'null' on input.


Suggested change

/// to <paramref name="tempsOpt"/>, which is allocated, if 'null' on input.

/// to <paramref name="tempsOpt"/>, which is allocated if 'null' on input.

``` #Resolved

jjonescz · 2025-07-22T09:04:00Z

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

+                    // The receiever has location, but extension indexer takes receiver by value.
+                    // This means that we need to ensure that the the receiver value is read after
+                    // any side-effecting arguments are evaluated, so that the
+                    // the setter receives the last value of the receiver, not the value before the


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

jjonescz · 2025-07-22T09:04:14Z

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

+                if (IsPropertyWithByValPossiblyStructReceiverWhichHasLocationAndCanChangeValueBetweenReads(rewrittenReceiver, indexer))
+                {
+                    // The receiever has location, but extension indexer takes receiver by value.
+                    // This means that we need to ensure that the the receiver value is read after


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

jjonescz · 2025-07-22T09:10:26Z

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

    [Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78137")]
    [CombinatorialData]
-    public void InterpolationHandler_ReceiverParameter_ByIn_WithConstantReceiver(bool useMetadataRef)
+    public void InterpolationHandler_ReceiverParameter_Generic_ByRef(bool useMetadataRef)


Consider referencing #79379 in the modified/added tests #Resolved

jjonescz · 2025-07-22T09:15:21Z

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

+                // 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
+                // arguments/rhs were evaluated. Receiver sideeffects should be evaluated at
+                // the very beginning, of course.


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

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.

jcouv

LGTM Thanks (commit 8)

AlekseyTs added 7 commits July 18, 2025 12:43

Temporarily add limited support for extension indexers

b079ba0

Add tests for interesting scenarios

cf4369f

More interesting tests

df39446

Adjust a test

47f2aa8

Fix receiver capturing for various extension scenarios

f478367

Fixes dotnet#79379 Fixes dotnet#79436

Revert "Temporarily add limited support for extension indexers"

eacecf1

This reverts commit c2412e0b5ecd6b14f4ba9d7a66a52e05f511128f.

Disable tests for indexers

814baf6

AlekseyTs requested review from jcouv and jjonescz July 18, 2025 19:57

AlekseyTs added the Area-Compilers label Jul 18, 2025

AlekseyTs requested a review from a team as a code owner July 18, 2025 19:57

AlekseyTs added the Feature - Extension Everything The extension everything feature label Jul 18, 2025

jcouv self-assigned this Jul 18, 2025

jcouv reviewed Jul 21, 2025

View reviewed changes

jcouv approved these changes Jul 21, 2025

View reviewed changes

jjonescz reviewed Jul 22, 2025

View reviewed changes

jjonescz approved these changes Jul 22, 2025

View reviewed changes

PR feedback

3951497

AlekseyTs enabled auto-merge (squash) July 25, 2025 17:37

jcouv approved these changes Jul 25, 2025

View reviewed changes

Fixup a test baseline

f6cbd13

AlekseyTs changed the base branch from main to features/extensions July 25, 2025 19:45

AlekseyTs merged commit 1d9820f into dotnet:features/extensions Jul 25, 2025
24 checks passed

This was referenced Aug 19, 2025

[Automated] PRs inserted in VS build main-10918.105 #79952

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10918.21 #79956

Closed

[Automated] PRs inserted in VS build feature.Wix5-10921.95 #80011

Closed

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

	// 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

	// 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

	/// to <paramref name="tempsOpt"/>, which is allocated, if 'null' on input.
	/// to <paramref name="tempsOpt"/>, which is allocated if 'null' on input.
	``` #Resolved

Fix receiver capturing for various extension scenarios #79472

Fix receiver capturing for various extension scenarios #79472

Uh oh!

Conversation

AlekseyTs commented Jul 18, 2025

Uh oh!

AlekseyTs commented Jul 21, 2025

Uh oh!

azure-pipelines bot commented Jul 21, 2025

Uh oh!

AlekseyTs commented Jul 21, 2025

Uh oh!

AlekseyTs commented Jul 21, 2025

Uh oh!

azure-pipelines bot commented Jul 21, 2025

Uh oh!

jcouv Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Jul 21, 2025

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 22, 2025

Uh oh!

jjonescz Jul 22, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 22, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 22, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 22, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 22, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 22, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 22, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

jcouv Jul 21, 2025 •

edited

Loading

jcouv Jul 21, 2025 •

edited

Loading

jcouv Jul 21, 2025 •

edited

Loading

jcouv Jul 21, 2025 •

edited

Loading

jcouv Jul 21, 2025 •

edited

Loading

jcouv commented Jul 21, 2025 •

edited

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by jcouv

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by jcouv

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

jjonescz Jul 22, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Jul 22, 2025 •

edited

Loading