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.

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

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

Merged
merged 8 commits into from
Jun 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dc3c8cb
Skip checks for overridden or hidden members in overload resolution w…
cston May 24, 2023
2067bd8
Update tests
cston May 25, 2023
2d9f622
Skip RemoveLessDerivedMembers for extension methods
cston Jun 6, 2023
1fc1bc7
Update comment
cston Jun 6, 2023
dc04f02
Check for multiple valid results
cston Jun 7, 2023
ada84d1
Check extension method containing type
cston Jun 7, 2023
61623be
Check for multiple valid results
cston Jun 8, 2023
f684fb6
Check diagnostics
cston Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7397,7 +7397,8 @@ protected MethodGroupResolution BindExtensionMethod(
isMethodGroupConversion: isMethodGroupConversion,
allowRefOmittedArguments: allowRefOmittedArguments,
returnRefKind: returnRefKind,
returnType: returnType);
returnType: returnType,
isExtensionMethodResolution: true);
diagnostics.Add(expression, useSiteInfo);
var sealedDiagnostics = diagnostics.ToReadOnlyAndFree();

Expand Down Expand Up @@ -9151,6 +9152,7 @@ private MethodGroupResolution ResolveDefaultMethodGroup(
returnRefKind,
returnType,
isFunctionPointerResolution,
isExtensionMethodResolution: false,
callingConvention);

// Note: the MethodGroupResolution instance is responsible for freeing its copy of analyzed arguments
Expand Down
111 changes: 68 additions & 43 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,14 @@ public void MethodInvocationOverloadResolution(
RefKind returnRefKind = default,
TypeSymbol returnType = null,
bool isFunctionPointerResolution = false,
bool isExtensionMethodResolution = false,
in CallingConventionInfo callingConventionInfo = default)
{
MethodOrPropertyOverloadResolution(
methods, typeArguments, receiver, arguments, result,
isMethodGroupConversion, allowRefOmittedArguments, ref useSiteInfo, inferWithDynamic,
allowUnexpandedForm, returnRefKind, returnType, isFunctionPointerResolution, in callingConventionInfo);
allowUnexpandedForm, returnRefKind, returnType, isFunctionPointerResolution: isFunctionPointerResolution,
isExtensionMethodResolution: isExtensionMethodResolution, in callingConventionInfo);
}

// Perform overload resolution on the given property group, with the given arguments and
Expand Down Expand Up @@ -168,16 +170,23 @@ internal void MethodOrPropertyOverloadResolution<TMember>(
RefKind returnRefKind = default,
TypeSymbol returnType = null,
bool isFunctionPointerResolution = false,
bool isExtensionMethodResolution = false,
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().

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.

members.All(static m => m.ContainingSymbol is NamedTypeSymbol { BaseTypeNoUseSiteDiagnostics.SpecialType: SpecialType.System_Object }));

// First, attempt overload resolution not getting complete results.
PerformMemberOverloadResolution(
results, members, typeArguments, receiver, arguments, completeResults: false, isMethodGroupConversion,
returnRefKind, returnType, allowRefOmittedArguments, isFunctionPointerResolution, callingConventionInfo,
ref useSiteInfo, inferWithDynamic, allowUnexpandedForm);
ref useSiteInfo, inferWithDynamic: inferWithDynamic, allowUnexpandedForm: allowUnexpandedForm, checkOverriddenOrHidden: checkOverriddenOrHidden);

if (!OverloadResolutionResultIsValid(results, arguments.HasDynamicArgument))
{
Expand All @@ -187,7 +196,7 @@ internal void MethodOrPropertyOverloadResolution<TMember>(
results, members, typeArguments, receiver, arguments,
completeResults: true, isMethodGroupConversion, returnRefKind, returnType,
allowRefOmittedArguments, isFunctionPointerResolution, callingConventionInfo,
ref useSiteInfo, allowUnexpandedForm: allowUnexpandedForm);
ref useSiteInfo, inferWithDynamic: false, allowUnexpandedForm: allowUnexpandedForm, checkOverriddenOrHidden: checkOverriddenOrHidden);
}
}

Expand Down Expand Up @@ -239,8 +248,9 @@ private void PerformMemberOverloadResolution<TMember>(
bool isFunctionPointerResolution,
in CallingConventionInfo callingConventionInfo,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo,
bool inferWithDynamic = false,
bool allowUnexpandedForm = true)
bool inferWithDynamic,
bool allowUnexpandedForm,
bool checkOverriddenOrHidden)
where TMember : Symbol
{
// SPEC: The binding-time processing of a method invocation of the form M(A), where M is a
Expand All @@ -256,7 +266,7 @@ private void PerformMemberOverloadResolution<TMember>(
// overriding or hiding member is not in a subtype of the type containing the
// (potentially) overridden or hidden member.
Dictionary<NamedTypeSymbol, ArrayBuilder<TMember>> containingTypeMapOpt = null;
if (members.Count > 50) // TODO: fine-tune this value
if (checkOverriddenOrHidden && members.Count > 50) // TODO: fine-tune this value
{
containingTypeMapOpt = PartitionMembersByContainingType(members);
}
Expand All @@ -276,7 +286,8 @@ private void PerformMemberOverloadResolution<TMember>(
containingTypeMapOpt,
inferWithDynamic: inferWithDynamic,
useSiteInfo: ref useSiteInfo,
allowUnexpandedForm: allowUnexpandedForm);
allowUnexpandedForm: allowUnexpandedForm,
checkOverriddenOrHidden: checkOverriddenOrHidden);
}

// CONSIDER: use containingTypeMapOpt for RemoveLessDerivedMembers?
Expand All @@ -288,7 +299,10 @@ private void PerformMemberOverloadResolution<TMember>(
RemoveInaccessibleTypeArguments(results, ref useSiteInfo);

// SPEC: The set of candidate methods is reduced to contain only methods from the most derived types.
RemoveLessDerivedMembers(results, ref useSiteInfo);
if (checkOverriddenOrHidden)
{
RemoveLessDerivedMembers(results, ref useSiteInfo);
}

if (Compilation.LanguageVersion.AllowImprovedOverloadCandidates())
{
Expand Down Expand Up @@ -824,9 +838,12 @@ private void AddMemberToCandidateSet<TMember>(
Dictionary<NamedTypeSymbol, ArrayBuilder<TMember>> containingTypeMapOpt,
bool inferWithDynamic,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo,
bool allowUnexpandedForm)
bool allowUnexpandedForm,
bool checkOverriddenOrHidden = true)
where TMember : Symbol
{
Debug.Assert(checkOverriddenOrHidden || containingTypeMapOpt is null);

// SPEC VIOLATION:
//
// The specification states that the method group that resulted from member lookup has
Expand Down Expand Up @@ -856,47 +873,50 @@ private void AddMemberToCandidateSet<TMember>(
// clever in filtering out methods from less-derived classes later, but we'll cross that
// bridge when we come to it.

if (members.Count < 2)
{
// No hiding or overriding possible.
}
else if (containingTypeMapOpt == null)
if (checkOverriddenOrHidden)
{
if (MemberGroupContainsMoreDerivedOverride(members, member, checkOverrideContainingType: true, ref useSiteInfo))
if (members.Count < 2)
{
// Don't even add it to the result set. We'll add only the most-overriding members.
return;
// No hiding or overriding possible.
}
else if (containingTypeMapOpt == null)
{
if (MemberGroupContainsMoreDerivedOverride(members, member, checkOverrideContainingType: true, ref useSiteInfo))
{
// Don't even add it to the result set. We'll add only the most-overriding members.
return;
}

if (MemberGroupHidesByName(members, member, ref useSiteInfo))
if (MemberGroupHidesByName(members, member, ref useSiteInfo))
{
return;
}
}
else if (containingTypeMapOpt.Count == 1)
{
return;
// No hiding or overriding since all members are in the same type.
}
}
else if (containingTypeMapOpt.Count == 1)
{
// No hiding or overriding since all members are in the same type.
}
else
{
// NOTE: only check for overriding/hiding in subtypes of f.ContainingType.
NamedTypeSymbol memberContainingType = member.ContainingType;
foreach (var pair in containingTypeMapOpt)
else
{
NamedTypeSymbol otherType = pair.Key;
if (otherType.IsDerivedFrom(memberContainingType, TypeCompareKind.ConsiderEverything, useSiteInfo: ref useSiteInfo))
// NOTE: only check for overriding/hiding in subtypes of f.ContainingType.
NamedTypeSymbol memberContainingType = member.ContainingType;
foreach (var pair in containingTypeMapOpt)
{
ArrayBuilder<TMember> others = pair.Value;

if (MemberGroupContainsMoreDerivedOverride(others, member, checkOverrideContainingType: false, ref useSiteInfo))
NamedTypeSymbol otherType = pair.Key;
if (otherType.IsDerivedFrom(memberContainingType, TypeCompareKind.ConsiderEverything, useSiteInfo: ref useSiteInfo))
{
// Don't even add it to the result set. We'll add only the most-overriding members.
return;
}
ArrayBuilder<TMember> others = pair.Value;

if (MemberGroupHidesByName(others, member, ref useSiteInfo))
{
return;
if (MemberGroupContainsMoreDerivedOverride(others, member, checkOverrideContainingType: false, ref useSiteInfo))
{
// Don't even add it to the result set. We'll add only the most-overriding members.
return;
}

if (MemberGroupHidesByName(others, member, ref useSiteInfo))
{
return;
}
}
}
}
Expand Down Expand Up @@ -1275,19 +1295,24 @@ private static void RemoveLessDerivedMembers<TMember>(ArrayBuilder<MemberResolut
// work necessary to eliminate methods on base types of Mammal when we eliminated
// methods on base types of Cat.

if (IsLessDerivedThanAny(result.LeastOverriddenMember.ContainingType, results, ref useSiteInfo))
if (IsLessDerivedThanAny(index: f, result.LeastOverriddenMember.ContainingType, results, ref useSiteInfo))
{
results[f] = result.WithResult(MemberAnalysisResult.LessDerived());
}
}
}

// Is this type a base type of any valid method on the list?
private static bool IsLessDerivedThanAny<TMember>(TypeSymbol type, ArrayBuilder<MemberResolutionResult<TMember>> results, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
private static bool IsLessDerivedThanAny<TMember>(int index, TypeSymbol type, ArrayBuilder<MemberResolutionResult<TMember>> results, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
where TMember : Symbol
{
for (int f = 0; f < results.Count; ++f)
{
if (f == index)
{
continue;
}

var result = results[f];

if (!result.Result.IsValid)
Expand Down Expand Up @@ -1642,7 +1667,7 @@ private void RemoveWorseMembers<TMember>(ArrayBuilder<MemberResolutionResult<TMe
/// <summary>
/// Returns the parameter type (considering params).
/// </summary>
private TypeSymbol GetParameterType(ParameterSymbol parameter, MemberAnalysisResult result)
private static TypeSymbol GetParameterType(ParameterSymbol parameter, MemberAnalysisResult result)
{
var type = parameter.Type;
if (result.Kind == MemberResolutionKind.ApplicableInExpandedForm &&
Expand Down
Loading