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

[Proposal] Improve overload resolution for delegate compatibility #3277

Open
1 of 4 tasks
333fred opened this issue Mar 19, 2020 · 3 comments
Open
1 of 4 tasks

[Proposal] Improve overload resolution for delegate compatibility #3277

333fred opened this issue Mar 19, 2020 · 3 comments
Assignees
Labels
Needs Implementation The specification for this issue has been approved, it needs an implementation Proposal champion Proposal
Milestone

Comments

@333fred
Copy link
Member

333fred commented Mar 19, 2020

Improve overload resolution for delegate compatibility

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
  • Specification: Not Started

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
Copy link
Contributor

alrz commented Mar 19, 2020

Speaking of overload resolution, any chance #129 could be considered too? Though it's more about type inference, I'd say it is aligned with the motivation stated here.

333fred added a commit to 333fred/csharplang that referenced this issue Mar 19, 2020
This fixes a very similar problem to dotnet#3277, where this code is unable to be resolved:
```cs
interface I1{}
interface I2{}

public unsafe class C : I1, I2 {
    void M(I1 i) {}
    static void M(I2 i) {}
    public void M1() {
        delegate*<C, void> a = M; // Ambiguous because both M's are applicable
    }
}
```
With this change, the instance method M is not applicable, so there is no ambiguity.
333fred added a commit that referenced this issue Mar 19, 2020
* Only allow static methods for applicable members

This fixes a very similar problem to #3277, where this code is unable to be resolved:
```cs
interface I1{}
interface I2{}

public unsafe class C : I1, I2 {
    void M(I1 i) {}
    static void M(I2 i) {}
    public void M1() {
        delegate*<C, void> a = M; // Ambiguous because both M's are applicable
    }
}
```
With this change, the instance method M is not applicable, so there is no ambiguity.
@MadsTorgersen MadsTorgersen moved this from TRIAGE NEEDED to 9.0 Candidate in Language Version Planning Mar 23, 2020
@MadsTorgersen MadsTorgersen added this to the 9.0 candidate milestone Mar 23, 2020
@MadsTorgersen MadsTorgersen modified the milestones: 9.0 candidate, Any Time Sep 9, 2020
@333fred 333fred moved this from 9.0 Candidate to Any Time in Language Version Planning Sep 9, 2020
@333fred 333fred added the Needs Implementation The specification for this issue has been approved, it needs an implementation label Oct 15, 2020
@tmat
Copy link
Member

tmat commented Jan 5, 2021

Maybe related: #4303

@333fred
Copy link
Member Author

333fred commented Jan 6, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Implementation The specification for this issue has been approved, it needs an implementation Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests

4 participants