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

Jump to bottom

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 4 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
100 changes: 59 additions & 41 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,21 @@ 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.
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?


// 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 +194,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 +246,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 +264,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 +284,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 +297,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 +836,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 +871,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 @@ -1642,7 +1660,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
117 changes: 117 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/OverloadResolutionPerfTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,5 +780,122 @@ public void DefiniteAssignment_ManySwitchCasesAndLabels()
// int tmp2; // unused
Diagnostic(ErrorCode.WRN_UnreferencedVar, "tmp2").WithArguments("tmp2").WithLocation(9, 13));
}

[ConditionalFact(typeof(IsRelease))]
[WorkItem("https://github.com/dotnet/roslyn/issues/67926")]
public void ExtensionOverloadsDistinctClasses_01()
{
const int n = 1000;

var builder = new StringBuilder();
builder.AppendLine(
$$"""
class Program
{
static void Main()
{
var o = new object();
var c = new C1();
o.F(c, c => o.F(c, null));
}
}
""");

for (int i = 0; i < n; i++)
{
builder.AppendLine(
$$"""
class C{{i}} { }
static class E{{i}}
{
public static void F(this object o, C{{i}} c, System.Action<C{{i}}> a) { }
}
""");
}

string source = builder.ToString();
var comp = CreateCompilation(source);
comp.VerifyDiagnostics();
}

[ConditionalFact(typeof(IsRelease))]
[WorkItem("https://github.com/dotnet/roslyn/issues/67926")]
public void ExtensionOverloadsDistinctClasses_02()
{
const int n = 1000;

var builder = new StringBuilder();
builder.AppendLine(
$$"""
class Program
{
static void Main()
{
var o = new object();
o.F(null, c => o.F(c, null));
}
}
""");

for (int i = 0; i < n; i++)
{
builder.AppendLine(
$$"""
class C{{i}} { }
static class E{{i}}
{
public static void F(this object o, C{{i}} c, System.Action<C{{i}}> a) { }
}
""");
}

string source = builder.ToString();
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (6,11): error CS0121: The call is ambiguous between the following methods or properties: 'E0.F(object, C0, Action<C0>)' and 'E1.F(object, C1, Action<C1>)'
// o.F(null, c => o.F(c, null));
Diagnostic(ErrorCode.ERR_AmbigCall, "F").WithArguments("E0.F(object, C0, System.Action<C0>)", "E1.F(object, C1, System.Action<C1>)").WithLocation(6, 11));
}

[ConditionalFact(typeof(IsRelease))]
[WorkItem("https://github.com/dotnet/roslyn/issues/67926")]
public void ExtensionOverloadsDistinctClasses_03()
{
const int n = 1000;

var builder = new StringBuilder();
builder.AppendLine(
$$"""
class Program
{
static void Main()
{
var o = new object();
var c = new C1();
o.F(c, c => { o.F( });
}
}
""");

for (int i = 0; i < n; i++)
{
builder.AppendLine(
$$"""
class C{{i}} { }
static class E{{i}}
{
public static void F(this object o, C{{i}} c, System.Action<C{{i}}> a) { }
}
""");
}

string source = builder.ToString();
var comp = CreateCompilation(source);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
var tree = comp.SyntaxTrees.Single();
var model = comp.GetSemanticModel(tree);
var expr = tree.GetCompilationUnitRoot().DescendantNodes().OfType<Syntax.InvocationExpressionSyntax>().Last();
Assert.Equal("o.F( ", expr.ToString());
_ = model.GetTypeInfo(expr);
}
}
}