Skip to content

Extensions: rewrite delegate and function pointer creations #77378

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

Jump to bottom
Merged
merged 5 commits into from
Mar 4, 2025
Merged

Extensions: rewrite delegate and function pointer creations #77378

merged 5 commits into from
Mar 4, 2025

Conversation

@AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Feb 28, 2025 •
edited by jcouv
Loading

Test plan #76130

@AlekseyTs AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Feb 28, 2025
@AlekseyTs AlekseyTs requested review from jcouv and jjonescz February 28, 2025 16:25
@AlekseyTs AlekseyTs requested a review from a team as a code owner February 28, 2025 16:25
@ghost ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 28, 2025
@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Mar 3, 2025

@jcouv, @dotnet/roslyn-compiler For the second review.

@jcouv jcouv self-assigned this Mar 3, 2025
jcouv
jcouv reviewed Mar 3, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Lowering/ExtensionMethodReferenceRewriter.cs Outdated
{
return updateDelegateCreation(node, VisitMethodSymbolWithExtensionRewrite(rewriter, node.MethodOpt), (BoundExpression)rewriter.Visit(node.Argument), rewriter.VisitType(node.Type));

static BoundNode updateDelegateCreation(BoundDelegateCreationExpression node, MethodSymbol? methodOpt, BoundExpression argument, TypeSymbol type)
Copy link
Member

@jcouv jcouv Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit: consider inlining so that the Update call is at the top-level or change the local function returns isExtensionMethod value. I'm happy to do it for you in a follow-up PR if it's any inconvenience and you're okay with it #Closed

jcouv
jcouv approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

jcouv
jcouv reviewed Mar 4, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs
// NOTE: Delegate type compatibility is important, but is not part of the existence check.

Debug.Assert(method.ParameterCount == parameterCount + (methodGroup.IsExtensionMethodGroup ? 1 : 0));
bool isExtensionMethod = methodGroup.IsExtensionMethodGroup && method.ContainingSymbol is not NamedTypeSymbol { IsExtension: true };
Copy link
Member

@jcouv jcouv Mar 4, 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.

method.ContainingSymbol is not NamedTypeSymbol { IsExtension: true };

nit: consider !method.GetIsNewExtensionMember() #Pending

Copy link
Contributor Author

@AlekseyTs AlekseyTs Mar 4, 2025

Choose a reason for hiding this comment

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

nit: consider !method.GetIsNewExtensionMember()

I'll follow up in a different PR

@jcouv
Copy link
Member

jcouv commented Mar 4, 2025 •
edited
Loading 

    public void M() { System.Console.Write(i); }

Did you intend to check the expected output of CompileAndVerify? #Closed

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

@jcouv
Copy link
Member

jcouv commented Mar 4, 2025 

    public void M() { System.Console.Write(i); }

Never mind. There's a merge commit. I'll fixup in next PR

In reply to: 2699072400

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

@AlekseyTs
Copy link
Contributor Author

AlekseyTs commented Mar 4, 2025 

    public void M() { System.Console.Write(i); }

I didn't make changes here. You are probably looking at a diff created by a merge with your changes

In reply to: 2699072400

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

jcouv
jcouv approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5)

@AlekseyTs AlekseyTs merged commit 66d8e92 into dotnet:features/extensions Mar 4, 2025
24 checks passed
This was referenced Apr 1, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjonescz jjonescz jjonescz approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Extension Everything The extension everything feature untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AlekseyTs @jcouv @jjonescz