Add proposal for overload resolution priority

[333fred]

This is my replacement proposal for what I originally wrote up in #7707.

[KalleOlaviNiemitalo]

As the proposed OverloadResolutionPriorityAttribute has AttributeTargets.Method, I presume it cannot be applied to events, properties, or indexers, but can be applied to their accessors. Indexers can be overloaded but the proposed text changes only 12.8.9.2 (Method invocations), rather than 12.8.11.3 (Indexer access) or 12.6.4 (Overload resolution). Will the attribute on an accessor then be ignored, or will a diagnostic be required?

(An indexer might have get and set accessors, and OverloadResolutionPriorityAttribute on each, but not necessarily the same Priority. 12.8.11.3 (Indexer access) is currently specified as doing overload resolution on entire indexers rather than on their accessors; so if the overload priority feature were intended to support indexers at the accessor level — such that x[y] += 1 were lowered to x.set_Item(y, x.get_Item(y) + 1) and then the overloads for each method call were resolved separately, potentially selecting different indexer signatures — then 12.8.11.3 would have to be rewritten. But I don't think developers would like having to maintain software that depends on gnarly overload resolution like that.)

[yaakov-h]

The file name looks wrong to me, and as such is annoyingly not triggering GitHub’s markdown renderer.

[TahirAhmadov]

Should the attribute be inherited? If not, what is the priority of the overriding member?
If the attribute is specified on a virtual member, should an override of that member be required to repeat the attribute?

With Obsolete, you have to specify the Obsolete on overriding members to avoid the warning; I think it should be a similar approach here:

• If overriding member doesn't have the attribute, it defaults to 0, and shows a warning;
• If overriding member has the attribute, then the specified value is used.

Should the attribute support constructors?

Yes.

Re. member types - I second @KalleOlaviNiemitalo 's concerns - the attribute should be applicable to methods, constructors, and indexer properties, but not to accessors (get, set, add, remove).

PS. Overall, I like this more than the BinaryCompatOnly approach, good on @333fred for changing the focus to this new approach.

[TonyValenti]

I'm not a fan of it being completely open ended either the numbers. I think there should be some predefined values/enums that help. Ie:
OverloadResolutionPriority.Default
OverloadResolutionPriority.PreferSpans
OverloadResolutionPriority.BinaryCompatibilityOnly

[hamarb123]

Just added my thoughts on a few aspects. Mostly looks good to me :)

[AlekseyTs]

I understand that this is a C# repo. However, in order to achieve the motivating goal, I think we need VB to pay attention to the attributes as well. Therefore, it would be good to create a VB specific proposal as well.

[hez2010]

Just to note that allowing overload priority could open a can of worms and shouldn't be rushed out to the public. I would instead recommend making OverloadResolutionPriorityAttribute an internal type for now if we really want this feature, so that only BCL can make use of this feature. If there're 3rd library authors wanting to use this feature, they can define their own OverloadResolutionPriorityAttribute type to opt-in.

[TonyValenti]

I've had mixed feelings about that as well. I have kind of thought that this should be tagged as a "requires preview features" feature.

I also really think that, in order to be truly useful, we need the c# compiler to allow methods that vary only by return type. I have plenty of methods I want to upgrade the return type on.

[colejohnson66]

Could OverloadResolutionPriorityAttribute itself be marked with the new ExperimentalAttribute? This would require users to acknowledge awareness of said “can of worms”.

[HaloFour]

I imagine that the compiler will follow the common pattern of not caring where the attribute comes from, only the fully qualified name of the attribute class. In that case, assuming that there is agreement on what the attribute name should be, the language can definitely ship with support for the feature without the BCL necessarily exposing one. But at the same time that means that someone could define their own and make use of this feature regardless of whether the BCL exposes it.

[jaredpar]

Just to note that allowing overload priority could open a can of worms and shouldn't be rushed out to the public

This statement without specific examples is not actionable.

I would instead recommend making OverloadResolutionPriorityAttribute an internal type for now if we really want this feature, so that only BCL can make use of this feature

The BCL is not the only team that has found a strong need for this.

[jaredpar]

Could OverloadResolutionPriorityAttribute itself be marked with the new ExperimentalAttribute? This would require users to acknowledge awareness of said “can of worms”.

At the moment the can of worms is empty

[jaredpar]

I imagine that the compiler will follow the common pattern of not caring where the attribute comes from, only the fully qualified name of the attribute class

Agree this is the most likely outcome but the spec should be clear about this. @333fred.

[TahirAhmadov]

I've also been wondering what people are referring to by "can of worms". It's strictly an opt-in feature; BCL maintainers generally do a great job at controlling what changes are approved. I don't expect anything "bad" to come out of it, except for potentially some invalid usage by 3-rd party libraries, but that can be said about almost any feature.

[333fred]

Agree this is the most likely outcome but the spec should be clear about this.

No previous specification I'm aware of has been clear about this, I'm not sure what's different here?

[333fred]

@AlekseyTs I've addressed your feedback on this. For VB, let's iron out what we want to do for C#, and then look at adapting it to VB.

[AlekseyTs]

LGTM (commit 4)

[hez2010]

Does this also apply to extension methods? I think yes as the attribute applies on any method.

Assuming we already have:

class C1
{
    public void M(int[] a) => Console.WriteLine("Array");
}

Now we have a

static class Ext
{
    [OverloadResolutionPriority(1)]
    public static void M(this C1 c, ReadOnlySpan<int> s) => Console.WriteLine("Span");
}

IIRC c.M([42]) would be resolved to Span, right?

However, what if we have

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

and now a developer who doesn't own the type C1 wants to cut in between the Span and ReadOnlySpan so that the priority should be greater than 1 but less than 2, what should the code be like? Should we change the priority to a floating-point number?

What if two extension methods from different type (or even different assembly) having with the same overload priority?

static class Ext1
{
    [OverloadResolutionPriority(1)]
    public static void M(this C1 c, Span<int> s) => Console.WriteLine("Span");
}

static class Ext2
{
    [OverloadResolutionPriority(1)]
    public static void M(this C1 c, ReadOnlySpan<int> s) => Console.WriteLine("ReadOnlySpan");
}

We have extension methods, and, in the future, we will also have extension types, there'll be many conflicts and ambiguities due to this if we introduce explicit overload priority.

You have no way to prevent such conflicting happening as two extension methods to a same type can be from two different assemblies with no dependency on each other.

This is one of what I said by the can of worms.

[333fred]

IIRC c.M([42]) would be resolved to Span, right?

No. Quoting from the proposal:

Because this pruning occurs at the very end of the overload resolution process, it does mean that a base type cannot make its members higher-priority than any derived type. This is intentional, and prevents an arms-race from occuring where a base type may try to always
be better than a derived type.

and now a developer who doesn't own the type C1 wants to cut in between the Span and ReadOnlySpan so that the priority should be greater than 1 but less than 2, what should the code be like?

They can't without deriving from the type and adding a new overload, which they could already do.

We have extension methods, and, in the future, we will also have extension types, there'll be many conflicts and ambiguities due to this if we introduce explicit overload priority.

No new conflicts or ambiguities exist. Extensions can already play games with namespace distance to influence priority, and OverloadResolutionPriority, as the final step in the process, does not come before namespace distance.

Do you have any concerns not addressed by the proposal?

[hez2010]

Do you have any concerns not addressed by the proposal?

Nope. I'm okay with it as long as the overload priority only affects the resolution of overloads which are defined in the type itself.

[TonyValenti]

I think they should affect Extension methods as well. This way I can upgrade those too.

[333fred]

I think they should affect Extension methods as well. This way I can upgrade those too.

They do, but as worded currently, only within the same type. For example:

static class Ext1
{
    [OverloadResolutionPriority(1)]
    public static void M(this C1 c, Span<int> s) => Console.WriteLine("Span");
    [OverloadResolutionPriority(0)]
    public static void M(this C1 c, ReadOnlySpan<int> s) => Console.WriteLine("ReadOnlySpan");
}

static class Ext2
{
    [OverloadResolutionPriority(0)]
    public static void M(this C1 c, ReadOnlySpan<int> s) => Console.WriteLine("ReadOnlySpan");
}

Within Ext1, the span overload would be preferred over the readonlyspan overload. However, between Ext1 and Ext2, overload resolution won't attempt to find any priorities, so resolution would likely end up picking Ext2.M as it is a better function member (because Span is convertible to a ReadOnlySpan).

[333fred]

I'm going to go ahead and merge this so we can put the open questions on the LDM agenda.