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

[API Proposal]: OverloadResolutionPriorityAttribute #102173

Closed
333fred opened this issue May 13, 2024 · 23 comments · Fixed by #102176
Closed

[API Proposal]: OverloadResolutionPriorityAttribute #102173

333fred opened this issue May 13, 2024 · 23 comments · Fixed by #102176
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@333fred
Copy link
Member

333fred commented May 13, 2024

Background and motivation

In order to support the C# proposed feature https://github.com/dotnet/csharplang/blob/d5bab90e846f9b3e9a7a4e552249643148f0194d/proposals/overload-resolution-priority.md, we add a new attribute to the BCL: OverloadResolutionPriorityAttribute. This feature allows applying the attribute to adjust the priority of an overload within a type. This will allow consumers like the rest of the BCL to adjust overload resolution for scenarios where a new, better API should be preferred (but won't be because overload resolution would prefer the existing one), or to resolve scenarios that will otherwise be ambiguous. One prime example is Debug.Assert; with this API, we could adjust void Debug.Assert(bool condition, string? message) to instead be [OverloadResolutionPriority(1)] Debug.Assert(bool condition, [CallerArgumentExpression(nameof(condition))] string? message = ""). The real change here would be a bit more complex due to the number of overloads that Debug.Assert already has, but this is a simple enough example to convey they intent.

API Proposal

namespace System.Runtime.CompilerServices;

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Property, AllowMultiple = false, Inherited = false)]
public sealed class OverloadResolutionPriorityAttribute(int priority)
{
    public int Priority => priority;
}

API Usage

using System.Runtime.CompilerServices;

var d = new C1();
int[] arr = [1, 2, 3];
d.M(arr); // Prints "Span"

class C1
{
    [OverloadResolutionPriority(1)]
    public void M(ReadOnlySpan<int> s) => Console.WriteLine("Span");
    // Default overload resolution priority
    public void M(int[] a) => Console.WriteLine("Array");
}

Alternative Designs

No response

Risks

One of the BCL's main uses of OverloadResolutionPriorityAttribute will be to effectively make source-breaking changes while maintaining binary compatibility. Care will have to be taken when using the attribute to ensure only the intended consequences occur. For a few of these cases, it can effectively make specific overloads impossible to call directly, and users would have to resort to using something like delegate conversion, UnsafeAccessor, reflection, or manual IL.

@333fred 333fred added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 13, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 13, 2024
@333fred 333fred added area-System.Runtime.CompilerServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 13, 2024
@333fred 333fred self-assigned this May 13, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label May 13, 2024
@tannergooding tannergooding added this to the 9.0.0 milestone May 13, 2024
@tannergooding tannergooding removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 13, 2024
@stephentoub stephentoub added the blocking Marks issues that we want to fast track in order to unblock other important work label May 13, 2024
333fred added a commit that referenced this issue May 13, 2024
333fred added a commit that referenced this issue May 13, 2024
333fred added a commit that referenced this issue May 13, 2024
@bartonjs
Copy link
Member

bartonjs commented May 14, 2024

Video

Looks good as proposed.

namespace System.Runtime.CompilerServices;

[AttributeUsage(
    AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Property,
    AllowMultiple = false,
    Inherited = false)]
public sealed class OverloadResolutionPriorityAttribute(int priority)
{
    public int Priority => priority;
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels May 14, 2024
333fred added a commit that referenced this issue May 14, 2024
stephentoub pushed a commit that referenced this issue May 15, 2024
* Add OverloadResolutionPriorityAttribute

Closes #102173.

* Base types

* Switch from primary ctor to regular ctor, add docs.
@davidfowl
Copy link
Member

cc @captainsafia @BrennanConroy @DamianEdwards

@Mr0N
Copy link

Mr0N commented May 16, 2024

And how can you understand what method is being invoked there? Now it's clear that if it's int[], then there will be a method with int[] and so on. And then what happens, I call the method Get(int[] array) with an int[] array, but Get(IList array) is invoked.

Do have to guess which method will be called? Doesn't that violate SOLID principles?

@333fred
Copy link
Member Author

333fred commented May 16, 2024

@Mr0N you'll understand it in exactly the same way you do today: by looking at quick-info and seeing what was chosen, or by looking at the compiled code and seeing what was emitted. I similarly don't understand how this would violate SOLID principles; the primary purpose of this attribute is to allow API authors who regret an API decision but can't fix it due to overloading rules and binary compat to fix those issues.

@Mr0N
Copy link

Mr0N commented May 16, 2024

@Mr0N you'll understand it in exactly the same way you do today: by looking at quick-info and seeing what was chosen, or by looking at the compiled code and seeing what was emitted. I similarly don't understand how this would violate SOLID principles; the primary purpose of this attribute is to allow API authors who regret an API decision but can't fix it due to overloading rules and binary compat to fix those issues.

Without knowing where this attribute is set in which method, I can't use the class. For example, if there's a DLL library with such a class, how am I supposed to know which method will be invoked? Essentially, I'm dependent on the internal implementation of the class.

Dependency inversion principle

@333fred
Copy link
Member Author

333fred commented May 16, 2024

No more than you already are? This attribute does not affect runtime behavior in anyway. It affects the compiler, and the impacts are completely visible at every call site it impacts. There's no silent, unexpected changes when using this feature. Can you give a specific example of what you're concerned by?

@Mr0N
Copy link

Mr0N commented May 17, 2024

I expect that Test(TestObject obj) will be called here, but Test(ITestObject obj) is being called instead. To understand which method overload will be invoked, I have to read the class code. So now I have to read every class before calling a method with the same types.

https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYCMBYAUKgBgAJVMA6AGQEsoBHAbj0JPIHUBTAI0dzynYDuRALIBPACqiADuwAUASjLj2AZ2Cz+Q5WoDynAFbsAxuvnyemkROlzF29Zft7DJhUQCGKogEknB48DmeER4TOhWkjJ4AN7BRPEA2joAbuxgADYA9u5wAEqqmekQwFSZUAAKYKVVwKKymPIAunHxqADMJAAsRPayvqrAzgFEmQby0QC+LS3tXT0Dsn4uwCNjk3hTuDTAaQBm7kbsPksB6/jhJyZnYUQpaVk5+SqFxaUVVZk1ogCCwMBVnGKcm2REq1SotXkLSIIF+/yogJ2kyAA

using System;
using System.Linq;
using System.Web;

new MyType().Test(new TestObject());
new MyType().Test(new TestObject() as ITestObject);
 

class MyType
{
    [OverloadResolutionPriority(1)]
    public void Test(ITestObject obj){}
   
   public void Test(TestObject obj){}
}
interface ITestObject{}
class TestObject{}

class OverloadResolutionPriorityAttribute(int Priority)
    :Attribute{}


@haefele
Copy link

haefele commented May 17, 2024

To understand which method overload will be invoked, I have to read the class code

@Mr0N are you saying, that you know all overload resolution rules on the top of your mind and can currently always tell 100 % correctly which overload will be called by the compiler?

Are you also concerned about this scenario:

In V2 of a class, someone adds a MORE specific overload to a method you're calling already.
Say you're calling Test(object o) as Test(1) and someone adds a Test(int i) in V2?

Isn't that the same situation as with this proposed [OverloadResolutionPriorityAttribute] but in reverse?

That happened already multiple times! The most interesting case of it I remember is when string interpolation handlers were added.
Suddenly you didn't call Method(string) anymore.

I personally didn't care about it, because it made my code FASTER. Well, to be honest, in the beginning I had NO IDEA that was changed.

@Mr0N
Copy link

Mr0N commented May 17, 2024

To understand which method overload will be invoked, I have to read the class code

@Mr0N are you saying, that you know all overload resolution rules on the top of your mind and can currently always tell 100 % correctly which overload will be called by the compiler?

Are you also concerned about this scenario:

In V2 of a class, someone adds a MORE specific overload to a method you're calling already.
Say you're calling Test(object o) as Test(1) and someone adds a Test(int i) in V2?

Isn't that the same situation as with this proposed [OverloadResolutionPriorityAttribute] but in reverse?

That happened already multiple times! The most interesting case of it I remember is when string interpolation handlers were added. Suddenly you didn't call Method(string) anymore.

I personally didn't care about it, because it made my code FASTER. Well, to be honest, in the beginning I had NO IDEA that was changed.

if I pass an int[] array and the method signature is currently SetInfo(int[] array), I'm pretty sure this method will be called. If you add such an attribute, it will be very difficult to understand which method will be called. You would need to run a debugger to see which method was invoked, and even then, I don't know how you could figure out which method was called — the old one, the new one, or some extension method, and so on. This attribute will confuse everything.

@333fred
Copy link
Member Author

333fred commented May 17, 2024

You would need to run a debugger to see which method was invoked,

You would not, the IDE will happily show you the method that is being invoked. GoToDef will work, quick info will work, etc.

if I pass an int[] array and the method signature is currently SetInfo(int[] array), I'm pretty sure this method will be called.

Can you tell me which overload is called here, using the existing rules from C# 1?

using System;

var d = new Derived();
d.M(new int[] { 1, 2, 3 });

class Base
{
    public void M(int[] s) => Console.WriteLine("Base");
}

class Derived : Base
{
    public void M(ReadOnlySpan<int> a) => Console.WriteLine("Derived");
}

Interpolated strings are another example where what you might think they call (the string overload) is not what they always call.

This API is a tool to allow developers to fix mistakes of the past. Yes, it can be abused, but lots of things in the language can be abused. The tooling has excellent support for giving you information about what your code is actually doing, whether you're in the IDE, the debugger, or even just reading IL.

@333fred
Copy link
Member Author

333fred commented May 17, 2024

I expect that Test(TestObject obj) will be called here, but Test(ITestObject obj) is being called instead. To understand which method overload will be invoked, I have to read the class code. So now I have to read every class before calling a method with the same types.

This example would not change behavior with OverloadResolutionPriority. There is no implicit conversion from TestObject to ITestObject, so the Test(ITestObject) overload is not applicable. Priority is a last step before determining which applicable overload is better, it doesn't suddenly force the system to insert explicit casts where you couldn't before. I highly recommend reading the actual feature spec before getting concerned over a feature that doesn't work that way 🙂.

@CyrusNajmabadi
Copy link
Member

You might also think you're calling a particular extension method, when in reality the compiler is pulling in a different one from a closer namespace.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented May 17, 2024

@Mr0N The point of the example is the usage of ReadOnlySpan. Your version uses arrays in both Base and Derived.

@CyrusNajmabadi
Copy link
Member

As another example, imagine an overload that takes an object and another that takes a string, with an optional argument after. You pass a string. Which gets called?

Fundamentally, you can't simply look at the API. You have to understand overload resolution.

@Mr0N
Copy link

Mr0N commented May 17, 2024

using System;

var d = new Derived();
d.M(new int[] { 1, 2, 3 });

class Base
{
    public void M(int[] s) => Console.WriteLine("Base");
}

class Derived : Base
{
    public void M(ReadOnlySpan<int> a) => Console.WriteLine("Derived");
}

Well, it might be doing something like in that example, behavior, it adds the word 'new' itself.

using System;

var d = new Derived();
d.M(new int[] { 1, 2, 3 });

class Base
{
    public void M(int[] s) => Console.WriteLine("Base");
}

class Derived : Base
{
    public new void M(ReadOnlySpan<int> a) => Console.WriteLine("Derived");
}

@Mr0N
Copy link

Mr0N commented May 17, 2024

If this is indeed the case, then it can be stated in the documentation that it is normal behavior for the 'new' operator to be added

@333fred
Copy link
Member Author

333fred commented May 17, 2024

I'm not sure what you're using the new keyword for here; the only thing that changes about the example is adding a warning that new isn't required on Derived.M(ROS<string>).

@Mr0N
Copy link

Mr0N commented May 17, 2024

I'm not sure what you're using the new keyword for here; the only thing that changes about the example is adding a warning that new isn't required on Derived.M(ROS<string>).

Well, I mean, the compiler might add the word new by default, and because of this, it calls public void M(ReadOnlySpan a) instead of how it ideally should be public void M(int[] a)

@Mr0N
Copy link

Mr0N commented May 17, 2024

image
The word new was added here.

@Mr0N
Copy link

Mr0N commented May 17, 2024

Well, it's just a guess.

@333fred
Copy link
Member Author

333fred commented May 17, 2024

@Mr0N new is not required here. Lacking it does produce a warning, as you can see in your screenshot, but it is not reflected in metadata at all. When Sharplab decompiles the IL there, it is making a best guess at the original code, and the original code would have likely put new there to silence the warning, because in your example you are indeed hiding a base member. The presence of new is simply a hint to the person reading your code in 6 months that you knew what you were doing and did intentionally hide a base member with the same signature. Its presence, or lack thereof, has no impact to overload resolution.

Regardless of that, the example that I showed does not require new, and in fact putting new there, as you did in your response, will result in a compiler warning that new is not required because Derived.M(ReadOnlySpan<int>) is not hiding anything. What you are observing there is the way that overload resolution has worked for derived types since C# 1.0; it works on a type-by-type basis, and does not continue looking up the inheritance hierarchy when it encounters an applicable method. The point I was making with this example is that, since C# 1.0, the presence of an overload whose arguments are all exactly the same type as what you pass to the method group does not guarantee that that overload will be picked. Overload resolution priority does not change this fact.

@Mr0N
Copy link

Mr0N commented May 18, 2024

@Mr0N new is not required here. Lacking it does produce a warning, as you can see in your screenshot, but it is not reflected in metadata at all. When Sharplab decompiles the IL there, it is making a best guess at the original code, and the original code would have likely put new there to silence the warning, because in your example you are indeed hiding a base member. The presence of new is simply a hint to the person reading your code in 6 months that you knew what you were doing and did intentionally hide a base member with the same signature. Its presence, or lack thereof, has no impact to overload resolution.

Regardless of that, the example that I showed does not require new, and in fact putting new there, as you did in your response, will result in a compiler warning that new is not required because Derived.M(ReadOnlySpan<int>) is not hiding anything. What you are observing there is the way that overload resolution has worked for derived types since C# 1.0; it works on a type-by-type basis, and does not continue looking up the inheritance hierarchy when it encounters an applicable method. The point I was making with this example is that, since C# 1.0, the presence of an overload whose arguments are all exactly the same type as what you pass to the method group does not guarantee that that overload will be picked. Overload resolution priority does not change this fact.

Well, okay then, maybe an attribute is really a good solution. It would be better if there was some attribute that fixes everything, so wouldn't have to use numbers in attribute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants