Emit extension implementation methods #77197

AlekseyTs · 2025-02-13T04:20:08Z

Relates to test plan #76130

AlekseyTs · 2025-02-13T14:55:28Z

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

jjonescz · 2025-02-14T11:09:09Z

src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs

+            {
+                if (method is { IsStatic: false, ContainingType: SourceNamedTypeSymbol { IsExtension: true, ExtensionParameter: { } parameter } })
+                {
+                    return new WithParametersBinder([parameter], resultBinder);


Should the doc comment of WithParametersBinder be updated to mention this scenario?

/// <summary> /// Binder used to place the parameters of a method, property, indexer, or delegate /// in scope when binding <param> tags inside of XML documentation comments /// and `nameof` in certain attribute positions. /// </summary> ``` #Resolved

Should the doc comment of WithParametersBinder be updated to mention this scenario?

Mentioned this in a PROTOTYPE comment

jjonescz · 2025-02-14T11:09:46Z

src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs

                        method = method ?? GetMethodSymbol(methodDecl, resultBinder);
                        isIteratorBody = method.IsIterator;
+                        resultBinder = WithExtensionReceiverParameterBinderIfNecessary(resultBinder, method);
                        resultBinder = new InMethodBinder(method, resultBinder);


Wouldn't it be better to update InMethodBinder to handle extension receiver? #Resolved

Wouldn't it be better to update InMethodBinder to handle extension receiver?

It depends. For example, whether we consider method parameters and receiver parameters in the same scope and what are the shadowing rules might affect the decision. My goal in this PR is to quickly enable test scenarios for implementation methods. I'll add a prototype comment.

jjonescz · 2025-02-14T11:19:22Z

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

+
+            bool haveExtraParameter = sourceMethod.ParameterCount != implementationMethod.ParameterCount;
+            if (haveExtraParameter)
+            {


Consider asserting that implementationMethod.ParameterCount - 1 == sourceMethod.ParameterCount #Resolved

dotnet-policy-service · 2025-02-14T15:20:35Z

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

AlekseyTs · 2025-02-14T15:30:54Z

I had to rebase because, for some reason, GitHub was including all commits from #77216 into this PR as well (despite the fact that that PR was already merged). Sorry for any inconvenience that that might cause. #Closed

jcouv · 2025-02-14T18:57:03Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

                    AddSynthesizedTypeMembersIfNecessary(builder, declaredMembersAndInitializers, diagnostics);
                    AddSynthesizedConstructorsIfNecessary(builder, declaredMembersAndInitializers, diagnostics);
+
+                    if (TypeKind == TypeKind.Class)


nit: consider tightening this check to only top-level non-generic static classes #Closed

jcouv · 2025-02-14T19:14:22Z

src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs

+        public override MethodKind MethodKind => MethodKind.Ordinary;
+        public override bool IsImplicitlyDeclared => true;
+
+        internal override bool HasSpecialName => true;


Did we have a motivating scenario for this? I thought we wanted to hold off on marking as special name until a scenario was identified #Closed

We are generating a special name for this method, that is enough motivation in my mind (like we do for property accessors an the like). The flag doesn't cause any harm to anyone, but it can be used to quickly filter methods in metadata.

jcouv · 2025-02-14T19:16:49Z

src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs

+            get
+            {
+                return _originalMethod.ParameterCount +
+                    (_originalMethod.IsStatic || _originalMethod.ContainingType is not SourceNamedTypeSymbol { ExtensionParameter: { } } ? 0 : 1);


Would it be better to not create an implementation method in the first place when the ExtensionParameter is faulty? #Closed

I do not know. So far that didn't cause an problems.

jcouv · 2025-02-14T21:01:42Z

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

+            return localFunctions.SelectAsArray(static (l, map) => (MethodSymbol)map[l], _symbolMap);
+        }
+
+        [return: NotNullIfNotNull("symbol")]


"symbol"

nameof(symbol)? #Closed

jcouv · 2025-02-14T21:09:45Z

""".Replace("[mscorlib]", ExecutionConditionUtil.IsMonoOrCoreClr ? "[netstandard]" : "[mscorlib]"));

I don't mind this approach, but consider moving this to VerifyTypeIL helper at top of the file instead of repeating. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:2989 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

jcouv · 2025-02-14T21:12:51Z

public void Implementation_InstanceMethod_08_WithLocalFunction_Generic()

Consider testing with generic local function too #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:3688 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

AlekseyTs · 2025-02-14T21:13:31Z

""".Replace("[mscorlib]", ExecutionConditionUtil.IsMonoOrCoreClr ? "[netstandard]" : "[mscorlib]"));

I do not want to be required to provide a base line with specific references. For example, I might use base line from core CLR run, then the helper wouldn't work

In reply to: 2660273404

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:2989 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

jcouv · 2025-02-14T21:20:43Z

extension(object _)

I noticed you use _ as the receiver parameter name in static scenarios. Is there a reason we can't just omit the name? #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:4547 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

jcouv · 2025-02-14T21:25:07Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

+                            {
+                                ILBuilder builder = new ILBuilder(_moduleBeingBuiltOpt, new LocalSlotManager(slotAllocator: null), OptimizationLevel.Release, areLocalsZeroed: false);
+
+                                // throw null;


nit: consider expanding the comment to provide more context "// Emit methods in extensions as skeletons:" #Closed

jcouv · 2025-02-14T21:27:25Z

    var verifier = CompileAndVerify(comp).VerifyDiagnostics();

Should we leave PROTOTYPE markers to execute these tests once we're able to emit call sites? #Resolved

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:4735 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

jcouv · 2025-02-14T21:28:02Z

src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenMethodSymbol.cs

+
+        internal sealed override bool HasAsyncMethodBuilderAttribute(out TypeSymbol? builderArgument)
+        {
+            // PROTOTYPE(roles): Test this code path


roles

nit: Is this intentional? #Closed

jcouv

Done with review pass (iteration 3)

AlekseyTs · 2025-02-14T21:42:28Z

extension(object _)

Is there a reason we can't just omit the name?

It is my intent to use a name in these tests.

In reply to: 2660289257

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:4547 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

AlekseyTs · 2025-02-14T21:45:09Z

    var verifier = CompileAndVerify(comp).VerifyDiagnostics();

We don't have to and I intend to port end-to-end tests from roles branch. I think they will cover execution of interesting scenarios

In reply to: 2660301916

Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:4735 in d358e8f. [](commit_id = d358e8f, deletion_comment = False)

jcouv

LGTM Thanks (iteration 4)

AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Feb 13, 2025

AlekseyTs requested a review from a team as a code owner February 13, 2025 04:20

ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 13, 2025

AlekseyTs requested review from jcouv and jjonescz February 13, 2025 14:54

jjonescz reviewed Feb 14, 2025

View reviewed changes

AlekseyTs requested review from a team as code owners February 14, 2025 15:19

dotnet-policy-service bot added VSCode Needs UX Triage Needs API Review Needs to be reviewed by the API review council labels Feb 14, 2025

AlekseyTs added 3 commits February 14, 2025 07:23

Emit extension implementation methods

8a20235

Fixup

603984b

PR feedback and followup on merge from main

d358e8f

AlekseyTs force-pushed the extensions_02 branch from 26ae105 to d358e8f Compare February 14, 2025 15:25

AlekseyTs requested a review from jjonescz February 14, 2025 15:26

jcouv reviewed Feb 14, 2025

View reviewed changes

jcouv self-assigned this Feb 14, 2025

PR feedback

e022bba

AlekseyTs requested a review from jcouv February 14, 2025 23:00

jcouv approved these changes Feb 15, 2025

View reviewed changes

jjonescz approved these changes Feb 17, 2025

View reviewed changes

AlekseyTs merged commit 7981391 into dotnet:features/extensions Feb 18, 2025
24 checks passed

This was referenced Apr 1, 2025

[Automated] PRs inserted in VS build main-35931.46 #77930

Closed

[Automated] PRs inserted in VS build feature.d18initial-10502.05 #77975

Closed

Emit extension implementation methods #77197

Emit extension implementation methods #77197

Uh oh!

Conversation

AlekseyTs commented Feb 13, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Feb 13, 2025

Uh oh!

jjonescz Feb 14, 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 Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Feb 14, 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 Feb 14, 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!

jjonescz Feb 14, 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!

dotnet-policy-service bot commented Feb 14, 2025

Uh oh!

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

Uh oh!

Uh oh!

jcouv Feb 14, 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 Feb 14, 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 Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 14, 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 Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented Feb 14, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Feb 14, 2025

Uh oh!

jcouv commented Feb 14, 2025 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jcouv Feb 14, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Feb 13, 2025 •

edited by jcouv

Loading

jjonescz Feb 14, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 14, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Feb 14, 2025 •

edited

Loading

jjonescz Feb 14, 2025 •

edited by AlekseyTs

Loading

AlekseyTs commented Feb 14, 2025 •

edited

Loading

jcouv Feb 14, 2025 •

edited

Loading

jcouv Feb 14, 2025 •

edited

Loading

jcouv Feb 14, 2025 •

edited

Loading

jcouv Feb 14, 2025 •

edited

Loading

jcouv commented Feb 14, 2025 •

edited by AlekseyTs

Loading

jcouv commented Feb 14, 2025 •

edited by AlekseyTs

Loading

jcouv commented Feb 14, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 14, 2025 •

edited

Loading

jcouv commented Feb 14, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 14, 2025 •

edited

Loading