Skip to content
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.

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

Skip checks for overridden or hidden members in overload resolution with extension methods #68316

Merged
merged 8 commits into from
Jun 10, 2023

Conversation

cston
Copy link
Member

@cston cston commented May 25, 2023

Improves Intellisense performance by ~35% for scenario in #67926.

in CallingConventionInfo callingConventionInfo = default)
where TMember : Symbol
{
var results = result.ResultsBuilder;

// No need to check for overridden or hidden methods
// if the members were resolved as extension methods.
bool checkOverriddenOrHidden = !isExtensionMethodResolution;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we confirm that all containing types are static types derived from System.Object?

@cston cston marked this pull request as ready for review June 6, 2023 22:58
@cston cston requested a review from a team as a code owner June 6, 2023 22:58
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

@cston
Copy link
Member Author

cston commented Jun 7, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@cston cston requested a review from a team June 7, 2023 15:13
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 7)

@cston cston requested a review from a team June 8, 2023 19:16
@cston
Copy link
Member Author

cston commented Jun 9, 2023

@dotnet/roslyn-compiler for a second review, thanks.

@RikkiGibson RikkiGibson self-assigned this Jun 9, 2023
var refA = CompileIL(sourceA, prependDefaultHeader: false);

// The calls to B.F3(o) and o.F3() should bind to B.F3 and should not be
// considered ambiguous with A.F3 because B is derived from A.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the language specify this?

Copy link
Member Author

@cston cston Jun 9, 2023

Choose a reason for hiding this comment

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

I'm not sure if the language specifies this, but this is the existing behavior, so I wanted to avoid an unnecessary breaking change.

// No need to check for overridden or hidden methods if the members were
// resolved as extension methods and the extension methods are defined in
// types derived from System.Object.
bool checkOverriddenOrHidden = !(isExtensionMethodResolution &&
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it feels like this might be easier to understand as !isExtensionMethodResolution || !members.All(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, both forms seem equally difficult to read. I'll leave it as is since the current form matches the comment above which seems relatively clear.

in CallingConventionInfo callingConventionInfo = default)
where TMember : Symbol
{
var results = result.ResultsBuilder;

// No need to check for overridden or hidden methods if the members were
// resolved as extension methods and the extension methods are defined in
// types derived from System.Object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this optimization only applicable for extension method overload resolution? What about normal static method overload resolution?

Copy link
Member Author

Choose a reason for hiding this comment

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

I limited the optimization to extension methods because it was easy to catch that case in the caller. I think it's less likely there is an issue with static method overloads since the containing types in that case would need to be in the same class or base classes, so few containing types and, as a result, few entries in the dictionary returned from PartitionMembersByContainingType().

@cston cston merged commit ef55b11 into dotnet:main Jun 10, 2023
@cston cston deleted the 67926 branch June 10, 2023 01:18
@ghost ghost added this to the Next milestone Jun 10, 2023
mavasani pushed a commit to mavasani/roslyn that referenced this pull request Jun 10, 2023
@RikkiGibson RikkiGibson modified the milestones: Next, 17.7 P3 Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants