Extensions: interceptors #79010

jcouv · 2025-06-17T18:04:11Z

Relates to test plan #76130

jjonescz · 2025-06-20T08:50:39Z

src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs

+}
+""";
+        var comp = CreateCompilation([source, interceptors, s_attributesSource], parseOptions: RegularNextWithInterceptors);
+        comp.VerifyEmitDiagnostics();


Should we verify execution to see that the interception succeeded?

Applies to other new tests that don't have compilation errors. #Resolved

jjonescz · 2025-06-20T09:02:02Z

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

+
+            static ParameterSymbol? getReceiverParameter(MethodSymbol method, bool classicExtensionNotInvokedAsStatic)
+            {
+                if (method.IsExtensionMethod && classicExtensionNotInvokedAsStatic)


Do we have tests for extension methods that are invoked as static?

jjonescz · 2025-06-20T09:02:26Z

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

-                interceptor.TryGetThisParameter(out var interceptorThisParameter) ? interceptorThisParameter : null;
-            switch (methodThisParameter, interceptorThisParameterForCompare)
+            ParameterSymbol? methodReceiverParameter = getReceiverParameter(method, classicExtensionNotInvokedAsStatic: invokedAsExtensionMethod);
+            ParameterSymbol? interceptorThisParameterForCompare = getReceiverParameter(interceptor, classicExtensionNotInvokedAsStatic: invokedAsExtensionMethod || needToReduceInterceptor);


Why does invokedAsExtensionMethod play a role in determining the receiver parameter of the interceptor? #Resolved

AlekseyTs · 2025-06-20T11:01:26Z

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

@@ -141,6 +141,7 @@ private void InterceptCallAndAdjustArguments(
            bool invokedAsExtensionMethod,
            Syntax.SimpleNameSyntax? nameSyntax)
        {
+            // Note: the extension method rewriter comes after the local rewriter, so we're still dealing with skeleton methods here


skeleton methods here

"declaration form of methods"? #Closed

AlekseyTs · 2025-06-20T11:05:34Z

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


-            if (!MemberSignatureComparer.InterceptorsComparer.Equals(method, symbolForCompare))
+            MethodSymbol? interceptorForCompare =
+                interceptor.GetIsNewExtensionMember() && invokedAsExtensionMethod ? interceptor.TryGetCorrespondingExtensionImplementationMethod() :


interceptor.GetIsNewExtensionMember() && invokedAsExtensionMethod

Is condition interceptor.GetIsNewExtensionMember() && invokedAsExtensionMethod ever true? #Closed

AlekseyTs · 2025-06-20T11:08:48Z

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


-            if (!MemberSignatureComparer.InterceptorsComparer.Equals(method, symbolForCompare))
+            MethodSymbol? interceptorForCompare =
+                interceptor.GetIsNewExtensionMember() && invokedAsExtensionMethod ? interceptor.TryGetCorrespondingExtensionImplementationMethod() :


interceptor.TryGetCorrespondingExtensionImplementationMethod()

This looks suspicious. In general getting implementation method is moving us to opposite direction by comparison to what ReducedExtensionMethodSymbol.Create does on the other branch. #Closed

AlekseyTs · 2025-06-20T11:17:06Z

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

-            var interceptorThisParameterForCompare = needToReduce ? interceptor.Parameters[0] :
-                interceptor.TryGetThisParameter(out var interceptorThisParameter) ? interceptorThisParameter : null;
-            switch (methodThisParameter, interceptorThisParameterForCompare)
+            ParameterSymbol? methodReceiverParameter = getReceiverParameter(method, classicExtensionNotInvokedAsStatic: invokedAsExtensionMethod);


classicExtensionNotInvokedAsStatic

I do not see any benefits from this rename. I think we should stick to the usual name invokedAsExtensionMethod that is well understood and is used throughout the codebase. Different name leads to believe that we are dealing with a different concept. #Closed

AlekseyTs · 2025-06-20T11:52:37Z

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

-            var interceptorThisParameterForCompare = needToReduce ? interceptor.Parameters[0] :
-                interceptor.TryGetThisParameter(out var interceptorThisParameter) ? interceptorThisParameter : null;
-            switch (methodThisParameter, interceptorThisParameterForCompare)
+            ParameterSymbol? methodReceiverParameter = getReceiverParameter(method, classicExtensionNotInvokedAsStatic: invokedAsExtensionMethod);


invokedAsExtensionMethod

This value feels wrong. methodThisParameter didn't depend on invokedAsExtensionMethod before and the renamed value methodReceiverParameter shouldn't depend on it too, meaning we should always pass false here. Otherwise, it looks like there is a behavior change here for a legacy scenario, however, there is no indication (in a comment or in a PR description) that there is an intent to fix a bug. #Closed

AlekseyTs · 2025-06-20T11:54:48Z

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

-                interceptor.TryGetThisParameter(out var interceptorThisParameter) ? interceptorThisParameter : null;
-            switch (methodThisParameter, interceptorThisParameterForCompare)
+            ParameterSymbol? methodReceiverParameter = getReceiverParameter(method, classicExtensionNotInvokedAsStatic: invokedAsExtensionMethod);
+            ParameterSymbol? interceptorThisParameterForCompare = getReceiverParameter(interceptor, classicExtensionNotInvokedAsStatic: invokedAsExtensionMethod || needToReduceInterceptor);


invokedAsExtensionMethod || needToReduceInterceptor

Combination of these values also looks suspicious and there is no explanation provided why this is the right condition to use for the entire breadth of scenarios. #Closed

AlekseyTs · 2025-06-20T12:10:22Z

            interceptor = interceptor.Construct(typeArgumentsBuilder.ToImmutableAndFree());

Is this going to do the right thing for a new extension method? #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs:175 in 34c2420. [](commit_id = 34c2420, deletion_comment = False)

AlekseyTs · 2025-06-20T12:17:58Z

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

+                    if (!argumentRefKindsOpt.IsDefault)
+                    {
+                        argumentRefKindsOpt = argumentRefKindsOpt.Insert(0, thisRefKind);
+                    }
                }
            }

            method = interceptor;


method = interceptor;

Let's say interceptor is an instance extension and method is invoked as extension. It looks like we are going to use implementation form of interceptor to match the signature to method. So far good. But then here we are replacing method with interceptor (the declaration form). How are they going to align in the BoundCall that will be created. There would be no receiver and too many arguments in that call. I am failing to connect the dots. What am I missing? #Closed

We're constructing a busted BoundCall from lowering (lacks receiver, has extra argument) but the ExtensionMethodReferenceRewriter hid the problem. I'll add an assertion there. Thanks for catching this

AlekseyTs · 2025-06-20T12:28:07Z

        Debug.Assert(!interceptor.ContainingType.IsGenericType);

It is not clear what guarantees this for new extensions. And it is not clear why would we want to have this restriction for new extensions, given that implementation methods necessary satisfy the restriction. BTW, it looks like we don't have tests for generic extension blocks. #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs:153 in 34c2420. [](commit_id = 34c2420, deletion_comment = False)

AlekseyTs · 2025-06-20T12:42:49Z

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

@@ -141,6 +141,7 @@ private void InterceptCallAndAdjustArguments(
            bool invokedAsExtensionMethod,
            Syntax.SimpleNameSyntax? nameSyntax)
        {
+            // Note: the extension method rewriter comes after the local rewriter, so we're still dealing with skeleton methods here
            if (this._compilation.TryGetInterceptor(nameSyntax) is not var (attributeLocation, interceptor))


if (this._compilation.TryGetInterceptor(nameSyntax) is not var (attributeLocation, interceptor))

This function went through some rather complicated changes with no explanation why the logic should be this way or that way. I am guessing that the complexity is coming from a desire to reconcile an interception of a classic extension invocation by a new style instance extension method. I am suggesting to use an alternative implementation strategy. Starting with the following:

Revert all the changes

Right after this line succeeds, if interceptor is a new extension, replace it with implementation form. There is no taboo against using implementation forms during lowering. Especially in a case like this, where we are not dealing with a method explicitly referenced by a user at the call site.

Adjust method.TryGetThisParameter(out var methodThisParameter); to get extension parameter for instance extensions.

Let the rest of the method take its course and do what it used to do.

If that doesn't address some scenarios, I will be happy to discuss them and suggest appropriate adjustments to the strategy. #Closed

Thanks. That approach is much simpler indeed :-) I'd stuck myself with constraint of leaving the ExtensionMethodReferenceRewriter deal with replacement :-/

AlekseyTs · 2025-06-20T12:45:46Z

Done with review pass (commit 1), test are not reviewed.

AlekseyTs · 2025-06-24T19:32:07Z

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

@@ -73,6 +73,8 @@ public static BoundNode VisitCall(BoundTreeRewriter rewriter, BoundCall node)

            static BoundExpression visitArgumentsAndFinishRewrite(BoundTreeRewriter rewriter, BoundCall node, BoundExpression? rewrittenReceiver)
            {
+                Debug.Assert(!node.Method.GetIsNewExtensionMember() || node.Method.IsStatic || node.ReceiverOpt is not null);


!node.Method.GetIsNewExtensionMember()

Is this condition necessary? #Closed

AlekseyTs · 2025-06-24T19:39:29Z

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

@@ -147,6 +147,11 @@ private void InterceptCallAndAdjustArguments(
                return;
            }

+            if (interceptor.GetIsNewExtensionMember() && interceptor.TryGetCorrespondingExtensionImplementationMethod() is { } implementationMethod)


interceptor.TryGetCorrespondingExtensionImplementationMethod() is { } implementationMethod

It doesn't feel like we can continue otherwise. If there is a guarantee that the implementation method is present (for example, it is always defined in the same source module), then we should throw unreachable. If there is no guarantee, then we should rep[ort an error and recover. #Closed

AlekseyTs · 2025-06-24T19:58:28Z

src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs

+}
+""";
+        var comp = CreateCompilation([source, interceptors, s_attributesSource], parseOptions: RegularPreviewWithInterceptors);
+        comp.VerifyEmitDiagnostics(


comp.VerifyEmitDiagnostics(

Is it still possible to execute the code? #Closed

AlekseyTs · 2025-06-24T19:59:51Z

Done with review pass (commit 2)

AlekseyTs · 2025-06-25T11:32:02Z

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

@@ -73,6 +73,8 @@ public static BoundNode VisitCall(BoundTreeRewriter rewriter, BoundCall node)

            static BoundExpression visitArgumentsAndFinishRewrite(BoundTreeRewriter rewriter, BoundCall node, BoundExpression? rewrittenReceiver)
            {
+                Debug.Assert(node.Method.MethodKind != MethodKind.Ordinary || node.Method.IsStatic || node.ReceiverOpt is not null);


node.Method.MethodKind != MethodKind.Ordinary

It is not clear to me why this condition is necessary. Wouldn't we want node.Method.IsStatic || node.ReceiverOpt is not null to be true always? #Closed

AlekseyTs · 2025-06-25T11:35:11Z

Done with review pass (commit 3)

AlekseyTs

LGTM (commit 4)

jcouv self-assigned this Jun 17, 2025

jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Jun 17, 2025

Extensions: interceptors

34c2420

jcouv force-pushed the extensions-interceptors branch from 6469d19 to 34c2420 Compare June 17, 2025 18:10

jcouv marked this pull request as ready for review June 17, 2025 21:08

jcouv requested a review from a team as a code owner June 17, 2025 21:08

jcouv requested review from jjonescz and AlekseyTs June 19, 2025 15:27

jjonescz reviewed Jun 20, 2025

View reviewed changes

AlekseyTs reviewed Jun 20, 2025

View reviewed changes

Address feedback

6e1dc48

AlekseyTs reviewed Jun 24, 2025

View reviewed changes

Address feedback

378132a

AlekseyTs reviewed Jun 25, 2025

View reviewed changes

Adjust assertion

e4d30cd

AlekseyTs approved these changes Jun 25, 2025

View reviewed changes

jcouv requested a review from jjonescz June 25, 2025 20:11

jjonescz approved these changes Jun 26, 2025

View reviewed changes

jcouv merged commit d3350fa into dotnet:main Jun 26, 2025
24 checks passed

jcouv deleted the extensions-interceptors branch June 26, 2025 17:53

dotnet-policy-service bot added this to the Next milestone Jun 26, 2025

dotnet-bot mentioned this pull request Jul 2, 2025

[Automated] PRs inserted in VS build main-10801.176 #79222

Closed

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025

Extensions: interceptors #79010

Extensions: interceptors #79010

Uh oh!

Conversation

jcouv commented Jun 17, 2025

Uh oh!

jjonescz Jun 20, 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 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Jun 20, 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!

AlekseyTs Jun 20, 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 Jun 20, 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 Jun 20, 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 Jun 20, 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 Jun 20, 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 Jun 20, 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 commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jun 20, 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 Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jun 20, 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 Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 20, 2025

Uh oh!

AlekseyTs Jun 24, 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 Jun 24, 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 Jun 24, 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 commented Jun 24, 2025

Uh oh!

AlekseyTs Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jjonescz Jun 20, 2025 •

edited by jcouv

Loading

jjonescz Jun 20, 2025 •

edited by jcouv

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs commented Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs commented Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 20, 2025 •

edited

Loading

AlekseyTs Jun 24, 2025 •

edited

Loading

AlekseyTs Jun 24, 2025 •

edited

Loading

AlekseyTs Jun 24, 2025 •

edited

Loading

AlekseyTs Jun 25, 2025 •

edited

Loading