Enable basic end-to-end scenarios of extension method invocation. #77320

AlekseyTs · 2025-02-23T20:26:25Z

No description provided.

AlekseyTs · 2025-02-24T13:03:53Z

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

jcouv · 2025-02-24T20:42:46Z

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

+
+            return rewrittenCall;
+
+            BoundExpression visitArgumentsAndFinishRewrite(BoundCall node, BoundExpression? rewrittenReceiver)


nit: consider passing rewriter explicitly and marking local function as static to avoid capture #Resolved

jcouv · 2025-02-24T20:56:55Z

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

+                        }
+                        else
+                        {
+                            argumentRefKinds = argumentRefKinds.Insert(0, SyntheticBoundNodeFactory.ArgumentRefKindFromParameterRefKind(receiverRefKind, useStrictArgumentRefKinds: false));


false

Consider adding a test for this value #Resolved

Consider adding a test for this value

Testing for ref receivers is to be done, I added a couple of tests with prototype comment. I am not planning to test more in this PR.

BTW, the added tests break for true, but quite possibly they take the other branch that has the same logic.

Yes, the added tests take the other branch

jcouv · 2025-02-24T21:16:17Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+
+                return uncommon.LazyExtensionParameter.Value;
+
+                ParameterSymbol makeExtensionParameter()


nit: consider making static to avoid captures, or limiting capture to this. Same for getMarkerMethodSymbol below #Resolved

jcouv · 2025-02-24T21:39:26Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+                {
+                    foreach (var member in @this.ContainingType.GetMembers(SourceExtensionImplementationMethodSymbol.GetImplementationName(method)))
+                    {
+                        if (member is not MethodSymbol { HasSpecialName: true, IsStatic: true } candidate)


Should we check the accessibility of the implementation method (should match the skeleton method's)? #Resolved

Should we check the accessibility of the implementation method (should match the skeleton method's)?

I'll add a PROTOTYPE comment.

jcouv · 2025-02-24T21:39:58Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+
+            return uncommon.LazyImplementationMap.GetOrAdd(
+                method,
+                static (method, @this) =>


nit: consider making this a local function #Resolved

consider making this a local function

What would be the motivation?

Code with long embedded lambdas is not as readable. The last argument of GetOrAdd would not be lost after the long lambda

jcouv · 2025-02-24T21:49:21Z

src/Compilers/Core/Portable/Symbols/WellKnownMemberNames.cs

+        /// <summary>
+        /// The name of marker method for an extension type.
+        /// </summary>
+        internal const string ExtensionMarkerMethodName = "<Extension>$"; // PROTOTYPE confirm name with WG


// PROTOTYPE confirm name with WG

nit: we could remove the comment #Resolved

jcouv

LGTM Thanks (iteration 4)

jcouv · 2025-02-24T22:23:20Z

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

+            // Code size 1 (0x1)
+            .maxstack 8
+            IL_0000: ret
+        } // end of method '<>E__0'::'<Extension>$'


Consider adding a LangVer test (you can consume extension methods from metadata from any language version) #Resolved

Consider adding a LangVer test (you can consume extension methods from metadata from any language version)

I think the behavior should be different, the suggested behavior goes against the spirit of the language version feature, I think.

This level of testing is outside of scope of the PR. It is not about binding call-sites. While the language check should be performed for a call-site during binding.

AlekseyTs · 2025-02-24T23:34:55Z

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

jjonescz · 2025-02-24T14:05:26Z

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

+                else
+                {
+                    loweredBody = ExtensionMethodReferenceRewriter.Rewrite(loweredBody);
+                }


Should we add if (loweredBody.HasErrors) { return loweredBody; } after this "rewriting stage" similarly to the other stages around here? #Resolved

These visitors have no way of reporting errors and are not expected to produce bad nodes.

jjonescz · 2025-02-24T14:21:27Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

@@ -366,8 +377,174 @@ public override ExtendedSpecialType ExtendedSpecialType
            }
        }

-        // PROTOTYPE metadata is undone
-        internal sealed override ParameterSymbol ExtensionParameter => throw new NotImplementedException();
+        internal sealed override ParameterSymbol ExtensionParameter


Consider wrapping this bulk of new code in #nullable enable #Resolved

jjonescz · 2025-02-24T14:29:03Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+            }
+        }
+
+        public override MethodSymbol TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)


Suggested change

public override MethodSymbol TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)

public sealed override MethodSymbol TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)

``` #Resolved

jjonescz · 2025-02-24T14:33:53Z

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

@@ -84,5 +103,49 @@ internal sealed override ParameterSymbol? ExtensionParameter
                }
            }
        }
+
+        public override MethodSymbol? TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)


Suggested change

public override MethodSymbol? TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)

public sealed override MethodSymbol? TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)

``` #Resolved

jjonescz · 2025-02-24T14:39:14Z

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

+    /// <summary>
+    /// Replaces references to extension methods with references to their implementation methods
+    /// </summary>
+    internal class ExtensionMethodReferenceRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator


Suggested change

internal class ExtensionMethodReferenceRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator

internal sealed class ExtensionMethodReferenceRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator

``` #Resolved

Enable basic end-to-end scenarios of extension method invocation.

e163995

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

AlekseyTs requested review from jjonescz and jcouv February 23, 2025 20:26

AlekseyTs requested a review from a team as a code owner February 23, 2025 20:26

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 23, 2025

AlekseyTs added 3 commits February 23, 2025 14:39

Formatting

b94d0a7

Implement retargeting to unblock CI

5583d29

Fixup

a9da93e

jcouv reviewed Feb 24, 2025

View reviewed changes

jcouv self-assigned this Feb 24, 2025

jcouv approved these changes Feb 24, 2025

View reviewed changes

PR feedback

bae413e

jcouv reviewed Feb 24, 2025

View reviewed changes

build-analysis bot mentioned this pull request Feb 25, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jjonescz approved these changes Feb 25, 2025

View reviewed changes

PR Feedback

d5db0ad

AlekseyTs enabled auto-merge (squash) February 25, 2025 14:49

AlekseyTs merged commit 74d162a into dotnet:features/extensions Feb 25, 2025
25 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable basic end-to-end scenarios of extension method invocation. #77320

Enable basic end-to-end scenarios of extension method invocation. #77320

AlekseyTs commented Feb 23, 2025

AlekseyTs commented Feb 24, 2025

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2025

AlekseyTs Feb 24, 2025

jcouv Feb 24, 2025

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2025

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2025

jcouv Feb 24, 2025

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv left a comment

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Feb 24, 2025

AlekseyTs commented Feb 24, 2025

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

AlekseyTs Feb 25, 2025

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading


		return rewrittenCall;

		BoundExpression visitArgumentsAndFinishRewrite(BoundCall node, BoundExpression? rewrittenReceiver)


		return uncommon.LazyExtensionParameter.Value;

		ParameterSymbol makeExtensionParameter()

	public override MethodSymbol TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)
	public sealed override MethodSymbol TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)
	``` #Resolved

	public override MethodSymbol? TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)
	public sealed override MethodSymbol? TryGetCorrespondingExtensionImplementationMethod(MethodSymbol method)
	``` #Resolved

	internal class ExtensionMethodReferenceRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
	internal sealed class ExtensionMethodReferenceRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
	``` #Resolved

Enable basic end-to-end scenarios of extension method invocation. #77320

Enable basic end-to-end scenarios of extension method invocation. #77320

Conversation

AlekseyTs commented Feb 23, 2025

AlekseyTs commented Feb 24, 2025

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 24, 2025

jjonescz Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jjonescz Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jjonescz Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jjonescz Feb 24, 2025 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jcouv Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading

jjonescz Feb 24, 2025 •

edited by AlekseyTs

Loading