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

Overload Resolution Priority #74190

Merged
merged 26 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9b97133
Initial implementation of method symbols. Many more tests need to be …
333fred May 4, 2024
c5dfbe8
Add application validation and more testing
333fred Jun 21, 2024
76716e0
Add support for constructors, add some tests around cycles.
333fred Jun 21, 2024
409da23
Increase test coverage and mitigate a binding cycle with constructors.
333fred Jun 24, 2024
3123da0
Add support for indexers
333fred Jun 25, 2024
aff5004
Add several new tests.
333fred Jun 25, 2024
b50bde6
Add many more tests, error when putting overload resolution priority …
333fred Jun 26, 2024
5d17914
Adjust test baseline after rebase
333fred Jun 27, 2024
524a0e2
Update language feature status.
333fred Jun 27, 2024
014f1b3
Broaden early out.
333fred Jun 27, 2024
349b1e2
More accessor tests, support erroring on event accessors.
333fred Jun 27, 2024
d57d1d0
Add more langversion tests
333fred Jun 28, 2024
a83752e
PR feedback, support explicit implementations.
333fred Jul 1, 2024
08382a8
Typo
333fred Jul 1, 2024
b2904ac
Missing attributes.
333fred Jul 1, 2024
196eea0
Respond to PR feedback.
333fred Jul 3, 2024
c0c1730
PR feedback. Simplify CanHaveOverloadResolutionPriority significantly…
333fred Jul 8, 2024
e833d16
More tests from test plan review
333fred Jul 9, 2024
730aea7
Merge remote-tracking branch 'upstream/main' into feature/overload-re…
333fred Jul 9, 2024
f3c6c98
Update LangVersion test after merge with main, address final prototypes.
333fred Jul 9, 2024
336236d
Latest round of PR feedback and additional testing.
333fred Jul 9, 2024
1cba1e8
Final pieces of feedback from Julien
333fred Jul 9, 2024
ae1569e
Add OverloadResolutionPriorityAttribute to the compiler test plan.
333fred Jul 9, 2024
d46b524
Remove asserts that can be hit in rare bad metadata cases.
333fred Jul 9, 2024
50112d3
Beef up a cycle test and address a race condition. We were binding ob…
333fred Jul 10, 2024
a0d6cb4
PR feedback from Chuck
333fred Jul 10, 2024
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
2 changes: 1 addition & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@
"azure-pipelines.customSchemaFile": ".vscode/dnceng-schema.json",
"dotnet.defaultSolution": "Roslyn.sln",
"dotnet.completion.showCompletionItemsFromUnimportedNamespaces": true,
"dotnet.testWindow.disableAutoDiscovery": true
"dotnet.testWindow.disableAutoDiscovery": false
333fred marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 1 addition & 1 deletion docs/Language Feature Status.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ efforts behind them.
| [Params-collections](https://github.com/dotnet/csharplang/issues/7700) | main | [Merged to 17.10p3](https://github.com/dotnet/roslyn/issues/71137) | [AlekseyTs](https://github.com/AlekseyTs) | [RikkiGibson](https://github.com/RikkiGibson), [333fred](https://github.com/333fred) | [akhera99](https://github.com/akhera99) (needs IDE fixer) | [MadsTorgersen](https://github.com/MadsTorgersen), [AlekseyTs](https://github.com/AlekseyTs) |
| [Ref/unsafe in iterators/async](https://github.com/dotnet/csharplang/blob/main/proposals/ref-unsafe-in-iterators-async.md) | [RefInAsync](https://github.com/dotnet/roslyn/tree/features/RefInAsync) | [Merged into 17.11p2](https://github.com/dotnet/roslyn/issues/72662) | [jjonescz](https://github.com/jjonescz) | [AlekseyTs](https://github.com/AlekseyTs), [cston](https://github.com/cston) | (no IDE impact) | |
| [Ref Struct Interfaces](https://github.com/dotnet/csharplang/issues/7608) | [RefStructInterfaces](https://github.com/dotnet/roslyn/tree/features/RefStructInterfaces) | [Merged into 17.11p2](https://github.com/dotnet/roslyn/issues/72124) | [AlekseyTs](https://github.com/AlekseyTs) | [cston](https://github.com/cston), [jjonescz](https://github.com/jjonescz) | [ToddGrun](https://github.com/ToddGrun) | [agocke](https://github.com/agocke), [jaredpar](https://github.com/jaredpar) |
| [Overload Resolution Priority](https://github.com/dotnet/csharplang/issues/7706) | PR not yet available | [In Progress](https://github.com/dotnet/roslyn/issues/74131) | [333fred](https://github.com/333fred) | [jcouv](https://github.com/jcouv), [cston](https://github.com/cston) | Not yet assigned | [333fred](https://github.com/333fred) |
| [Overload Resolution Priority](https://github.com/dotnet/csharplang/issues/7706) | main | [Merged to 17.12p1](https://github.com/dotnet/roslyn/issues/74131) | [333fred](https://github.com/333fred) | [jcouv](https://github.com/jcouv), [cston](https://github.com/cston) | Not yet assigned | [333fred](https://github.com/333fred) |
| [Partial properties](https://github.com/dotnet/csharplang/issues/6420) | [partial-properties](https://github.com/dotnet/roslyn/tree/features/partial-properties) | [Merged into 17.11p3](https://github.com/dotnet/roslyn/issues/73090) | [RikkiGibson](https://github.com/RikkiGibson) | [jcouv](https://github.com/jcouv), [333fred](https://github.com/333fred) | [Cosifne](https://github.com/Cosifne) | [333fred](https://github.com/333fred), [RikkiGibson](https://github.com/RikkiGibson) |

# C# 12.0
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6543,7 +6543,7 @@ protected BoundExpression BindClassCreationExpression(
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
OverloadResolutionResult<MethodSymbol> overloadResolutionResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
ImmutableArray<MethodSymbol> accessibleConstructors = GetAccessibleConstructorsForOverloadResolution(type, ref useSiteInfo);
this.OverloadResolution.ObjectCreationOverloadResolution(accessibleConstructors, analyzedArguments, overloadResolutionResult, dynamicResolution: true, ref useSiteInfo);
this.OverloadResolution.ObjectCreationOverloadResolution(accessibleConstructors, analyzedArguments, overloadResolutionResult, dynamicResolution: true, isEarlyAttributeBinding: IsEarlyAttributeBinder, ref useSiteInfo);

if (overloadResolutionResult.HasAnyApplicableMember)
{
Expand Down Expand Up @@ -7014,7 +7014,7 @@ internal bool TryPerformConstructorOverloadResolution(
if (candidateConstructors.Any())
{
// We have at least one accessible candidate constructor, perform overload resolution with accessible candidateConstructors.
this.OverloadResolution.ObjectCreationOverloadResolution(candidateConstructors, analyzedArguments, result, dynamicResolution: false, ref useSiteInfo);
this.OverloadResolution.ObjectCreationOverloadResolution(candidateConstructors, analyzedArguments, result, dynamicResolution: false, isEarlyAttributeBinding: IsEarlyAttributeBinder, ref useSiteInfo);

if (result.Succeeded)
{
Expand All @@ -7028,7 +7028,7 @@ internal bool TryPerformConstructorOverloadResolution(
// We might have a best match constructor which is inaccessible.
// Try overload resolution with all instance constructors to generate correct diagnostics and semantic info for this case.
OverloadResolutionResult<MethodSymbol> inaccessibleResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
this.OverloadResolution.ObjectCreationOverloadResolution(allInstanceConstructors, analyzedArguments, inaccessibleResult, dynamicResolution: false, ref useSiteInfo);
this.OverloadResolution.ObjectCreationOverloadResolution(allInstanceConstructors, analyzedArguments, inaccessibleResult, dynamicResolution: false, isEarlyAttributeBinding: IsEarlyAttributeBinder, ref useSiteInfo);

if (inaccessibleResult.Succeeded)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@
#nullable disable

using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
{
[SuppressMessage("Performance", "CA1067", Justification = "Equality not actually implemented")]
internal readonly struct BinaryOperatorAnalysisResult
internal readonly struct BinaryOperatorAnalysisResult : IMemberResolutionResultWithPriority<MethodSymbol>
{
public readonly Conversion LeftConversion;
public readonly Conversion RightConversion;
Expand All @@ -35,6 +36,8 @@ public bool HasValue
get { return this.Kind != OperatorAnalysisResultKind.Undefined; }
}

MethodSymbol IMemberResolutionResultWithPriority<MethodSymbol>.MemberWithPriority => Signature.Method;

public override bool Equals(object obj)
{
// implement if needed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1069,13 +1069,15 @@ private void BinaryOperatorOverloadResolution(
return;
}

var candidates = result.Results;
RemoveLowerPriorityMembers<BinaryOperatorAnalysisResult, MethodSymbol>(candidates);

// SPEC: Otherwise, the best function member is the one function member that is better than all other function
// SPEC: members with respect to the given argument list, provided that each function member is compared to all
// SPEC: other function members using the rules in 7.5.3.2. If there is not exactly one function member that is
// SPEC: better than all other function members, then the function member invocation is ambiguous and a binding-time
// SPEC: error occurs.

var candidates = result.Results;
// Try to find a single best candidate
int bestIndex = GetTheBestCandidateIndex(left, right, candidates, ref useSiteInfo);
if (bestIndex != -1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@
#nullable disable

using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp
{
internal readonly struct UnaryOperatorAnalysisResult
internal readonly struct UnaryOperatorAnalysisResult : IMemberResolutionResultWithPriority<MethodSymbol>
{
public readonly UnaryOperatorSignature Signature;
public readonly Conversion Conversion;
Expand All @@ -33,6 +31,8 @@ public bool HasValue
get { return this.Kind != OperatorAnalysisResultKind.Undefined; }
}

MethodSymbol IMemberResolutionResultWithPriority<MethodSymbol>.MemberWithPriority => Signature.Method;

public static UnaryOperatorAnalysisResult Applicable(UnaryOperatorSignature signature, Conversion conversion)
{
return new UnaryOperatorAnalysisResult(OperatorAnalysisResultKind.Applicable, signature, conversion);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,15 @@ private void UnaryOperatorOverloadResolution(
return;
}

var candidates = result.Results;
RemoveLowerPriorityMembers<UnaryOperatorAnalysisResult, MethodSymbol>(candidates);

// SPEC: Otherwise, the best function member is the one function member that is better than all other function
// SPEC: members with respect to the given argument list, provided that each function member is compared to all
// SPEC: other function members using the rules in 7.5.3.2. If there is not exactly one function member that is
// SPEC: better than all other function members, then the function member invocation is ambiguous and a binding-time
// SPEC: error occurs.

var candidates = result.Results;
// Try to find a single best candidate
int bestIndex = GetTheBestCandidateIndex(operand, candidates, ref useSiteInfo);
if (bestIndex != -1)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace Microsoft.CodeAnalysis.CSharp;

internal interface IMemberResolutionResultWithPriority<TMember> where TMember : Symbol
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
TMember? MemberWithPriority { get; }
jcouv marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@

using System;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Symbols;

namespace Microsoft.CodeAnalysis.CSharp
{
/// <summary>
/// Represents the results of overload resolution for a single member.
/// </summary>
[SuppressMessage("Performance", "CA1067", Justification = "Equality not actually implemented")]
internal readonly struct MemberResolutionResult<TMember> where TMember : Symbol
internal readonly struct MemberResolutionResult<TMember> : IMemberResolutionResultWithPriority<TMember> where TMember : Symbol
{
private readonly TMember _member;
private readonly TMember _leastOverriddenMember;
Expand Down Expand Up @@ -121,6 +122,8 @@ internal MemberAnalysisResult Result
get { return _result; }
}

TMember IMemberResolutionResultWithPriority<TMember>.MemberWithPriority => LeastOverriddenMember;

public override bool Equals(object? obj)
{
throw new NotSupportedException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,20 +98,20 @@ private static bool SingleValidResult<TMember>(ArrayBuilder<MemberResolutionResu

// Perform overload resolution on the given method group, with the given arguments and
// names. The names can be null if no names were supplied to any arguments.
public void ObjectCreationOverloadResolution(ImmutableArray<MethodSymbol> constructors, AnalyzedArguments arguments, OverloadResolutionResult<MethodSymbol> result, bool dynamicResolution, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
public void ObjectCreationOverloadResolution(ImmutableArray<MethodSymbol> constructors, AnalyzedArguments arguments, OverloadResolutionResult<MethodSymbol> result, bool dynamicResolution, bool isEarlyAttributeBinding, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
Debug.Assert(!dynamicResolution || arguments.HasDynamicArgument);

var results = result.ResultsBuilder;

// First, attempt overload resolution not getting complete results.
PerformObjectCreationOverloadResolution(results, constructors, arguments, completeResults: false, dynamicResolution: dynamicResolution, ref useSiteInfo);
PerformObjectCreationOverloadResolution(results, constructors, arguments, completeResults: false, dynamicResolution: dynamicResolution, isEarlyAttributeBinding, ref useSiteInfo);

if (!OverloadResolutionResultIsValid(results, arguments.HasDynamicArgument))
{
// We didn't get a single good result. Get full results of overload resolution and return those.
result.Clear();
PerformObjectCreationOverloadResolution(results, constructors, arguments, completeResults: true, dynamicResolution: dynamicResolution, ref useSiteInfo);
PerformObjectCreationOverloadResolution(results, constructors, arguments, completeResults: true, dynamicResolution: dynamicResolution, isEarlyAttributeBinding, ref useSiteInfo);
}
}

Expand Down Expand Up @@ -361,6 +361,8 @@ private void PerformMemberOverloadResolution<TMember>(

if ((options & Options.DynamicResolution) == 0)
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
RemoveLowerPriorityMembers<MemberResolutionResult<TMember>, TMember>(results);

// SPEC: The best method of the set of candidate methods is identified. If a single best method cannot be identified,
// SPEC: the method invocation is ambiguous, and a binding-time error occurs.

Expand Down Expand Up @@ -1603,6 +1605,7 @@ private void PerformObjectCreationOverloadResolution(
AnalyzedArguments arguments,
bool completeResults,
bool dynamicResolution,
bool isEarlyAttributeBinding,
ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
// SPEC: The instance constructor to invoke is determined using the overload resolution
Expand All @@ -1620,6 +1623,16 @@ private void PerformObjectCreationOverloadResolution(

if (!dynamicResolution)
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
if (!isEarlyAttributeBinding)
{
// If we're still decoding early attributes, we can get into a cycle here where we attempt to decode early attributes,
// which causes overload resolution, which causes us to attempt to decode early attributes, etc. Concretely, this means
// that OverloadResolutionPriorityAttribute won't affect early bound attributes, so you can't use OverloadResolutionPriorityAttribute
// to adjust what constructor of OverloadResolutionPriorityAttribute is chosen. See `CycleOnOverloadResolutionPriorityConstructor_02` for
// an example.
RemoveLowerPriorityMembers<MemberResolutionResult<MethodSymbol>, MethodSymbol>(results);
}

// The best method of the set of candidate methods is identified. If a single best
// method cannot be identified, the method invocation is ambiguous, and a binding-time
// error occurs.
Expand Down Expand Up @@ -1698,6 +1711,83 @@ private int GetTheBestCandidateIndex<TMember>(ArrayBuilder<MemberResolutionResul
return currentBestIndex;
}

private static void RemoveLowerPriorityMembers<TMemberResolution, TMember>(ArrayBuilder<TMemberResolution> results)
where TMemberResolution : IMemberResolutionResultWithPriority<TMember>
where TMember : Symbol
{
// - Then, the reduced set of candidate members is grouped by declaring type. Within each group:
// - Candidate function members are ordered by *overload_resolution_priority*.
// - All members that have a lower *overload_resolution_priority* than the highest found within its declaring type group are removed.
// - The reduced groups are then recombined into the final set of applicable candidate function members.

switch (results)
{
// Can't prune anything unless there's at least 2 candidates
case []:
case [_]:
return;
}
333fred marked this conversation as resolved.
Show resolved Hide resolved

// Attempt to avoid any allocations by starting with a quick pass through all results and seeing if any have non-default priority. If so, we'll do the full sort and filter.
if (results.All(r => r.MemberWithPriority?.GetOverloadResolutionPriority() is null or 0))
{
// All default, nothing to do
return;
}

bool removedMembers = false;
var resultsByContainingType = PooledDictionary<NamedTypeSymbol, OneOrMany<TMemberResolution>>.GetInstance();

foreach (var result in results)
{
if (result.MemberWithPriority is null)
{
// Can happen for things like built-in binary operators
continue;
}

var containingType = result.MemberWithPriority.ContainingType;
if (resultsByContainingType.TryGetValue(containingType, out var previousResults))
{
var previousPriority = previousResults.First().MemberWithPriority.GetOverloadResolutionPriority();
var currentPriority = result.MemberWithPriority.GetOverloadResolutionPriority();

if (currentPriority > previousPriority)
{
removedMembers = true;
resultsByContainingType[containingType] = OneOrMany.Create(result);
}
else if (currentPriority == previousPriority)
{
resultsByContainingType[containingType] = previousResults.Add(result);
}
else
{
removedMembers = true;
Debug.Assert(previousResults.All(r => r.MemberWithPriority.GetOverloadResolutionPriority() == previousPriority));
}
}
else
{
resultsByContainingType.Add(containingType, OneOrMany.Create(result));
}
}

if (!removedMembers)
{
// No changes, so we can just return
resultsByContainingType.Free();
return;
}

results.Clear();
foreach (var (_, resultsForType) in resultsByContainingType)
{
results.AddRange(resultsForType);
}
resultsByContainingType.Free();
}

private void RemoveWorseMembers<TMember>(ArrayBuilder<MemberResolutionResult<TMember>> results, AnalyzedArguments arguments, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
where TMember : Symbol
{
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -7974,6 +7974,16 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureAllowsRefStructConstraint" xml:space="preserve">
<value>allows ref struct constraint</value>
</data>
<!-- PROTOTYPE: Condense -->
<data name="ERR_CannotApplyOverloadResolutionPriorityToOverride" xml:space="preserve">
333fred marked this conversation as resolved.
Show resolved Hide resolved
<value>Cannot use 'OverloadResolutionPriorityAttribute' on an overriding member.</value>
</data>
<data name="ERR_CannotApplyOverloadResolutionPriorityToMember" xml:space="preserve">
<value>Cannot use 'OverloadResolutionPriorityAttribute' on this member.</value>
</data>
<data name="IDS_OverloadResolutionPriority" xml:space="preserve">
<value>overload resolution priority</value>
</data>
<data name="ERR_InlineArrayAttributeOnRecord" xml:space="preserve">
<value>Attribute 'System.Runtime.CompilerServices.InlineArray' cannot be applied to a record struct.</value>
</data>
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2346,5 +2346,9 @@ internal enum ErrorCode
// Note: you will need to do the following after adding warnings:
// 1) Re-generate compiler code (eng\generate-compiler-code.cmd).
// 2) Update ErrorFacts.IsBuildOnlyDiagnostic (src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs)

// PROTOTYPE: Condense
333fred marked this conversation as resolved.
Show resolved Hide resolved
ERR_CannotApplyOverloadResolutionPriorityToOverride = 9500,
ERR_CannotApplyOverloadResolutionPriorityToMember = 9501,
}
}
Loading