Skip to content

Extensions: honor OverloadResolutionPriority attribute #78219

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 6 commits into from
Apr 28, 2025
Merged

Extensions: honor OverloadResolutionPriority attribute #78219

merged 6 commits into from
Apr 28, 2025

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Apr 19, 2025 •
edited
Loading

Following LDM 2025-04-16

The "containing type" for purpose of ORPA is the enclosing static class.
We synthesize an ORPA on the implementation method for accessors to have same prioritization between usage as extension and usage via static/implementation method.

Relates to test plan #76130

@jcouv jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Apr 19, 2025
@jcouv jcouv self-assigned this Apr 19, 2025
@ghost ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 19, 2025
@jcouv jcouv marked this pull request as ready for review April 22, 2025 16:14
@jcouv jcouv requested a review from a team as a code owner April 22, 2025 16:14
@jcouv jcouv requested review from AlekseyTs and jjonescz April 22, 2025 16:14
AlekseyTs
AlekseyTs reviewed Apr 22, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/PropertySymbol.cs

internal bool CanHaveOverloadResolutionPriority => !IsOverride && !IsExplicitInterfaceImplementation && (IsIndexer || IsIndexedProperty);
internal bool CanHaveOverloadResolutionPriority
=> !IsOverride && !IsExplicitInterfaceImplementation && (IsIndexer || IsIndexedProperty || this.GetIsNewExtensionMember());
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

|| this.GetIsNewExtensionMember()

I didn't think we decided to allow priorities on properties #Closed

AlekseyTs
AlekseyTs reviewed Apr 22, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
return (null, null);
}
else if (IsIndexer && CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.OverloadResolutionPriorityAttribute))
else if ((IsIndexer || this.GetIsNewExtensionMember()) && CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.OverloadResolutionPriorityAttribute))
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

|| this.GetIsNewExtensionMember()

Similar comment #Closed

AlekseyTs
AlekseyTs reviewed Apr 22, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
internal sealed override int TryGetOverloadResolutionPriority()
{
Debug.Assert(this.IsIndexer);
Debug.Assert(this.IsIndexer || this.GetIsNewExtensionMember());
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

|| this.GetIsNewExtensionMember()

Here as well #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 22, 2025

Done with review pass (commit 1)

@jcouv jcouv marked this pull request as draft April 23, 2025 07:51
AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs Outdated
{
var arg = new TypedConstant(DeclaringCompilation.GetSpecialType(SpecialType.System_Int32), TypedConstantKind.Primitive, priority);

AddSynthesizedAttribute(ref attributes, DeclaringCompilation.TrySynthesizeAttribute(
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

AddSynthesizedAttribute(ref attributes, DeclaringCompilation.TrySynthesizeAttribute(

I think we should simply copy the user applied attribute, the one applied to the property. #Closed

AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs Outdated
if (UnderlyingMethod is SourcePropertyAccessorSymbol { AssociatedSymbol: SourcePropertySymbolBase extensionProperty })
{
var priority = extensionProperty.OverloadResolutionPriority;
if (priority != 0)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

priority != 0

Should the user applied attribute (if there is one) from the property be copied unconditionally? #Closed

AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/SubstitutedMethodSymbol.cs Outdated
}

internal override int TryGetOverloadResolutionPriority()
=> UnderlyingMethod.TryGetOverloadResolutionPriority();
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

UnderlyingMethod

OriginalDefinition? #Closed

AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Extensions/RewrittenLambdaOrLocalFunctionSymbol.cs Outdated
}

internal override int TryGetOverloadResolutionPriority()
=> UnderlyingMethod.TryGetOverloadResolutionPriority();
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

UnderlyingMethod

_originalMethod? #Closed

AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingMethodSymbol.cs Outdated
}

internal override int TryGetOverloadResolutionPriority()
=> UnderlyingMethod.TryGetOverloadResolutionPriority();
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

UnderlyingMethod

_underlyingMethod? #Closed

AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Extensions/SourceExtensionImplementationMethodSymbol.cs Outdated
var arg = new TypedConstant(DeclaringCompilation.GetSpecialType(SpecialType.System_Int32), TypedConstantKind.Primitive, priority);

AddSynthesizedAttribute(ref attributes, DeclaringCompilation.TrySynthesizeAttribute(
WellKnownMember.System_Runtime_CompilerServices_OverloadResolutionPriorityAttribute__ctor,
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

WellKnownMember.System_Runtime_CompilerServices_OverloadResolutionPriorityAttribute__ctor

It looks like we assume that this member can be successfully found. Why? #Closed

AlekseyTs
AlekseyTs reviewed Apr 24, 2025
View reviewed changes
src/Compilers/Core/Portable/WellKnownMember.cs Outdated
System_Text_Encoding__get_UTF8,
System_Text_Encoding__GetString,

System_Runtime_CompilerServices_OverloadResolutionPriorityAttribute__ctor,
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

System_Runtime_CompilerServices_OverloadResolutionPriorityAttribute__ctor

I do not think we need this entry #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 24, 2025

Done with review pass (commit 2)

AlekseyTs
AlekseyTs approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 4), assuming CI is passing

@jcouv jcouv marked this pull request as ready for review April 25, 2025 14:51
@jcouv
Copy link
Member Author

jcouv commented Apr 25, 2025

@jjonescz for review. Thanks

@jcouv jcouv merged commit 73f70f4 into dotnet:main Apr 28, 2025
28 checks passed
@jcouv jcouv deleted the extensions-orpa branch April 28, 2025 08:54
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 28, 2025
This was referenced Apr 30, 2025
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request May 31, 2025
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 18.0 P1 Aug 19, 2025
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

@AlekseyTs AlekseyTs AlekseyTs 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

18.0 P1

Development

Successfully merging this pull request may close these issues.

4 participants

@jcouv @AlekseyTs @jjonescz @RikkiGibson