Overload Resolution Priority #74190

333fred · 2024-06-27T20:54:22Z

This implements the Overload Resolution Priority language feature.

Test Plan: #74131
Champion issue: dotnet/csharplang#7706
Speclet: https://github.com/dotnet/csharplang/blob/main/proposals/overload-resolution-priority.md

…added.

…on accessors.

333fred · 2024-06-27T20:55:21Z

This feature is small enough that, rather than do a full feature branch with multiple PRs, I've just done it as a single PR. I don't really recommend a commit-by-commit review here, I'll go through and comment on the actually interesting parts of the implementation. Most of it is actually just tests and attribute decoding boilerplate.

333fred · 2024-06-27T21:02:53Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -1698,6 +1711,78 @@ private int GetTheBestCandidateIndex<TMember>(ArrayBuilder<MemberResolutionResul
            return currentBestIndex;
        }

+        private static void RemoveLowerPriorityMembers<TMember>(ArrayBuilder<MemberResolutionResult<TMember>> results)


This method, and the calls to this method in the current file, are the actually interesting parts of the feature. This implements the rules from the speclet, and I've tried to be perf conscious with a couple of early outs to avoid any allocations in the very common scenarios where we have no adjusted priorities.

333fred · 2024-06-27T21:03:31Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -2346,5 +2346,10 @@ 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


I'll address these prototypes after we do the test plan review with the compiler team. They're here for now to make it easier for me to merge in main.

cston · 2024-06-28T21:40:15Z

.vscode/settings.json

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


false

Was this change intentional?

Yes, though I can revert if you want to keep it separate.

Yes, though I can revert if you want to keep it separate.

I'm not sure what the change affects, but it looks like it's unrelated to overload resolution, so consider reverting and creating a separate PR if needed.

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

src/Dependencies/PooledObjects/ArrayBuilder.cs

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

src/Compilers/CSharp/Portable/CSharpResources.resx

src/Compilers/CSharp/Portable/Symbols/PropertySymbol.cs

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

src/Compilers/CSharp/Test/Emit3/OverloadResolutionPriorityTests.cs

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

src/Compilers/CSharp/Test/Emit3/OverloadResolutionPriorityTests.cs

jjonescz · 2024-07-01T08:43:13Z

src/Compilers/CSharp/Test/Emit3/OverloadResolutionPriorityTests.cs

Will this feature be implemented for VB as well?

Yes, some amount of the feature will be implemented for VB.

333fred · 2024-07-01T17:58:12Z

@cston addressed or answered your comments, please take another look. @jcouv for review.

src/Compilers/CSharp/Test/Emit3/OverloadResolutionPriorityTests.cs

jcouv

Done with review pass (iteration 15). Tests not looked at much yet

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

jcouv · 2024-07-01T19:15:57Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>Cannot use 'OverloadResolutionPriorityAttribute' on an accessor method.</value>
+  </data>
+  <data name="ERR_CannotApplyOverloadResolutionPriorityToExplicitImplementation" xml:space="preserve">
+    <value>Cannot use 'OverloadResolutionPriorityAttribute' on an explicit interface implementation.</value>


spec: Is this mentioned in the spec?

It's an ignored location. I can certainly update the spec to more completely list every location that is ignored by the language, but it does fall out of the rules.

jcouv · 2024-07-01T19:26:48Z

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

+                        int32 priority
+                    ) cil managed 
+                {
+                    // Method begins at RVA 0x2069


nit: consider doing a bit of cleanup in the IL (remove unnecessary portions, compact the signatures)
nit: also consider moving this IL into OverloadResolutionPriorityTests.cs since that's the only usage and it's unlikely we'll need this in other test files

I'll do some cleanup, but I always prefer to put attributes here. It's the only usage today, but there's no guarantee that continue to the be case, and in my experience we're more likely to copy definitions around in the future, not move them.

jcouv · 2024-07-01T19:59:37Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceDestructorSymbol.cs

+
+        internal sealed override int? TryGetOverloadResolutionPriority()
+        {
+            return null;


Is there a test for destructor?
spec: Should it be an error?

I'll add it. It should indeed be an error, since it's ignored.

jcouv · 2024-07-01T20:06:08Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs

@@ -369,6 +369,28 @@ internal override (CSharpAttributeData?, BoundAttribute?) EarlyDecodeWellKnownAt
                    // diagnostics that might later get thrown away as possible when binding method calls.
                    return (null, null);
                }
+                else if (CSharpAttributeData.IsTargetEarlyAttribute(arguments.AttributeType, arguments.AttributeSyntax, AttributeDescription.OverloadResolutionPriorityAttribute))
+                {
+                    if (this is SourcePropertyAccessorSymbol or SourceEventAccessorSymbol)


spec: What is the plan for local functions and lambdas? Seems like they should be errors.
Same question for conversion and unary operators.

Technically speaking, I don't think any of these (except lambdas) are places where the attribute would be ignored: it may be rather pointless to put them there, but overload resolution will run on operators, conversions, and local function invocations. Lambdas would be ignored though, so we can error there.

Local functions and unary operators Yes, they do involve overload resolution, but overloads are not possible so an overload resolution attribute doesn't make sense. I thought we error on positions where the attribute will be ignored.
Update: I see your point that the attribute will be not ignored, merely that it doesn't do anything.

User-defined conversion operators: I don't see where overload resolution is involved in the spec or the code.

jcouv · 2024-07-01T20:38:08Z

src/Compilers/CSharp/Test/Emit3/OverloadResolutionPriorityTests.cs

+                {
+                }
+
+                [OverloadResolutionPriority(1)]


It'd be good to leave a comment (attribute is intentionally ignored)

jcouv · 2024-07-01T20:40:57Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

+            if (!_packedFlags.IsOverloadResolutionPriorityPopulated)
+            {
+                if (_containingType.ContainingPEModule.Module.TryGetOverloadResolutionPriorityValue(_handle, out int priority))
+                {


Should we ignore the priority when it is present on invalid PE members (property/event accessors, override, destructor, non-indexer properties, ...)?

We can't easily do that for accessors (and I have tests demonstrating the behavior), since we don't track, at the PEMethodSymbol layer, if this method is an accessor for some property or event. I instead opted to just read them, and not care whether the language may not permit them to be declared. We could certainly optimize the override/destructor/non-indexer properties, but it didn't seem like it would be worth the effort to me. If a tree falls in the forest but no one is there to hear it, did it make a sound?

jcouv · 2024-07-01T21:13:57Z

src/Compilers/CSharp/Portable/Symbols/PropertySymbol.cs

+        {
+            get
+            {
+                if (!this.IsIndexer)


I'm not very familiar with the concept, but I see that we also have "indexed properties" in metadata (see IsIndexedProperty) which also have parameters. As far as I can tell, they may not be considered indexers (IsIndexer). Should the attribute apply to those?

Yeah, it probably should, thanks for the reminder.

jcouv · 2024-07-01T21:31:14Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+                case []:
+                case [_]:
+                // We only look at methods and indexers, so if this isn't one of those scenarios, we don't need to do anything.
+                case [{ Member: not (MethodSymbol or PropertySymbol { IsIndexer: true }) }, _, ..]:


Aside from indexed properties, are there any other symbol that hits this case?
Depending on the decision on indexed properties (see comment elsewhere) this case may not be useful.

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

333fred added 9 commits June 27, 2024 13:34

Initial implementation of method symbols. Many more tests need to be …

9b97133

…added.

Add application validation and more testing

c5dfbe8

Add support for constructors, add some tests around cycles.

76716e0

Increase test coverage and mitigate a binding cycle with constructors.

409da23

Add support for indexers

3123da0

Add several new tests.

aff5004

Add many more tests, error when putting overload resolution priority …

b50bde6

…on accessors.

Adjust test baseline after rebase

5d17914

Update language feature status.

524a0e2

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 27, 2024

Broaden early out.

014f1b3

333fred commented Jun 27, 2024

View reviewed changes

333fred added the Feature - Overload Resolution Priority label Jun 27, 2024

333fred added 2 commits June 28, 2024 09:37

More accessor tests, support erroring on event accessors.

349b1e2

Add more langversion tests

d57d1d0

333fred marked this pull request as ready for review June 28, 2024 17:01

333fred requested a review from a team as a code owner June 28, 2024 17:01

jcouv self-assigned this Jun 28, 2024

333fred requested review from cston and jcouv June 28, 2024 17:03