dotnet / csharplang

The official repo for the design of the C# programming language
11.13k stars 1.01k forks source link

[Proposal] Improve overload resolution for delegate compatibility #3277

Open 333fred opened 4 years ago

333fred commented 4 years ago

Improve overload resolution for delegate compatibility

Summary

Improve overload resolution when looking for applicable methods by removing methods that cannot be compatible. This will allow the following code to compile (it does not today):

public class Program1
{
    delegate void MyAction<T>(T x);

    void Y(long x) { }

    void D(MyAction<int> o) { }
    void D(MyAction<long> o) { }

    void T()
    {
        D(Y); // Ambiguous between both D calls, despite the fact that `void D(MyAction<int>)` is not a valid target.
    }
}

Motivation

In the spirit of the other overload resolution changes we've made recently, removing things that already don't work, this would allow currently ambiguous code to compile and improve user experience.

Detailed design

Given the example in the Summary section, the reason why it does not compile today is straightforward, if requiring an intimate knowledge of the spec to figure out:

  1. We attempt to perform overload resolution on the call to D. To do that, we start by computing the applicable function members in the method group D.
  2. Overload resolution for applicable function members states that, for value parameters, there must be an implicit conversion from every parameter in the call to the given method in D if the method in D is applicable.
  3. In attempting to determine if that is true for D(MyAction<int>), we need to determine if the method group Y is applicable to MyAction<T> where T is int.
  4. We again perform overload resolution, this time on Y.
  5. The applicable function member definition looks that the first parameter of Y, which is of type long. There is an implicit numeric conversion from int to long. There are no more parameters, so Y is applicable to MyAction<int> and a method group conversion exists.
    • Delegate compatibility has not come into play here yet! Even though Y is not compatible with MyAction<int>, it is applicable.
  6. The first overload resolution determines that D(MyAction<int>) is an applicable member.
  7. Overload resolution looks at the next member, D(MyAction<long>). The same sub-steps run again, and D(MyAction<long>) is determined to be applicable.
  8. Neither method is more specific, so overload resolution fails.

All of this occurs despite the fact that we can unambiguously determine that D(MyAction<int>) can never be a valid target for D(Y): Y isn't compatible with MyAction<int>. This is because the applicable function member algorithm looks for any implicit conversion for parameter types, and delegate compatibility looks only for implicit reference conversions. The proposed change, then, is to the first step of the method group conversions section of the spec (addition is italicized):

  • A single method M is selected corresponding to a method invocation (Method invocations) of the form E(A), with the following modifications:
    • The argument list A is a list of expressions, each classified as a variable and with the type and modifier (ref or out) of the corresponding parameter in the formal_parameter_list of D.
    • The candidate methods considered are only those methods that are applicable in their normal form (Applicable function member), not those applicable only in their expanded form.
    • The (Applicable function member) algorithm considers only implicit reference conversions for value parameters.

Drawbacks

Any change is work.

Alternatives

Doing nothing.

Unresolved questions

N/A

Design meetings

alrz commented 4 years ago

Speaking of overload resolution, any chance https://github.com/dotnet/csharplang/issues/129 could be considered too? Though it's more about type inference, I'd say it is aligned with the motivation stated here.

tmat commented 3 years ago

Maybe related: https://github.com/dotnet/csharplang/discussions/4303

333fred commented 3 years ago

No, I don't believe that's related.