Skip to content

Conversation

@AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Jun 10, 2025

Relates to test plan #76130

@AlekseyTs AlekseyTs requested review from jcouv and jjonescz June 10, 2025 20:11
@AlekseyTs AlekseyTs requested a review from a team as a code owner June 10, 2025 20:11
@AlekseyTs AlekseyTs added Area-Compilers Feature - Extension Everything The extension everything feature labels Jun 10, 2025
@jcouv jcouv self-assigned this Jun 10, 2025
@AlekseyTs
Copy link
Contributor Author

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


foreach (var scope in new ExtensionScopes(this))
{
extensionDeclarationsInSingleScope.Clear();
Copy link
Member

@jcouv jcouv Jun 11, 2025

Choose a reason for hiding this comment

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

I'd missed in previous PRs: just to confirm, we don't need to clear result too? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd missed in previous PRs: just to confirm, we don't need to clear result too?

BinaryOperatorExtensionOverloadResolutionInSingleScope clears it before repopulating.

@jcouv
Copy link
Member

jcouv commented Jun 11, 2025

// "An extension operator may not have the same signature as a predefined operator." not yet implemented

nit: Can be removed now #Resolved


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:16198 in 7d3d14c. [](commit_id = 7d3d14c, deletion_comment = False)

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 (commit 2)

static void Main()
{
var s1 = new S1();
s1 += s1;
Copy link
Member

@jjonescz jjonescz Jun 12, 2025

Choose a reason for hiding this comment

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

Consider adding a test verifying IL so it can be seen what this lowers to. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider adding a test verifying IL so it can be seen what this lowers to.

I put the lowest priority on testing IL. There is nothing tricky about it apart from switching to implementation method. So, I think that IL verification, plus observing runtime behavior is sufficiently good. In addition at the bottom of this file there is a PROTOTYPE comment to increase coverage for runtime behavior.

@AlekseyTs
Copy link
Contributor Author

// "An extension operator may not have the same signature as a predefined operator." not yet implemented

nit: Can be removed now

I'll take care of this later.


In reply to: 2964115516


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionOperatorsTests.cs:16198 in 7d3d14c. [](commit_id = 7d3d14c, deletion_comment = False)

@AlekseyTs AlekseyTs merged commit ee0b8ec into dotnet:features/extensions Jun 12, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything The extension everything feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants