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

Champion "Improved overload candidates" (15.7) #98

Open
1 of 5 tasks
gafter opened this issue Feb 14, 2017 · 18 comments
Open
1 of 5 tasks

Champion "Improved overload candidates" (15.7) #98

gafter opened this issue Feb 14, 2017 · 18 comments
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Milestone

Comments

@gafter
Copy link
Member

gafter commented Feb 14, 2017

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (done, rejected, inactive)
  • Spec'ed

see dotnet/roslyn#250

The overload resolution rules have been updated in nearly every C# language update to improve the experience for programmers, making ambiguous invocations select the "obvious" choice. This has to be done carefully to preserve backward compatibility, but since we are usually resolving what would otherwise be error cases, these enhancements usually work out nicely.

There are three cases we might consider improving in C# 7:

  1. When a method group contains both instance and static members, we discard the instance members if invoked without an instance receiver or context, and discard the static members if invoked with an instance receiver. When there is no receiver, we include only static members in a static context, otherwise both static and instance members. When the receiver is ambiguously an instance or type due to a color-color situation, we include both. A static context, where an implicit this instance receiver cannot be used, includes the body of members where no this is defined, such as static members, as well as places where this cannot be used, such as field initializers and constructor-initializers.
  2. When a method group contains some generic methods whose type arguments do not satisfy their constraints, these members are removed from the candidate set.
  3. For a method group conversion, candidate methods whose return type doesn't match up with the delegate's return type are removed from the set.

There are probably additional improvements we could consider.

@gafter gafter changed the title Bestest Betterness Champion Bestest Betterness Feb 15, 2017
@gafter gafter changed the title Champion Bestest Betterness Champion "Bestest Betterness" Feb 21, 2017
@gafter gafter modified the milestones: 7.2 candidate, 7.X candidate Feb 22, 2017
@gafter
Copy link
Member Author

gafter commented May 6, 2017

We should also look whether we should do something similar for the lambda conversion (say the conversion doesn't exist when the return type is wrong). See dotnet/roslyn#8942

@MadsTorgersen MadsTorgersen modified the milestones: 7.X candidate, 7.3 candidate Oct 25, 2017
@jcouv
Copy link
Member

jcouv commented Oct 25, 2017

@gafter Per LDM today, since it not technically a change to overload resolution, but rather the steps before (weeding out candidates) and after (removing any redundant logic to final verification). Should we rename the issue?

@gafter
Copy link
Member Author

gafter commented Oct 26, 2017

@jcouv I think keeping the name stable helps us recall what we were talking about. But I don't feel strongly about it one way or another. Do you have a suggested name? "Better method candidate selection"?

@scalablecory
Copy link

This likely deserves a whole other discussion, but: overload resolution based on generic constraints would be so wonderful.

@gafter
Copy link
Member Author

gafter commented Feb 8, 2018

@scalablecory This would have the side-effect of permitting overload resolution based on generic constraints. You would still have to ensure that the method signatures are different enough that they would not clash in IL.

FYI, I'm implementing this now.

gafter added a commit to gafter/roslyn that referenced this issue Feb 10, 2018
- Refine candidate set based on static/instance receiver, or static context
- Refine candidate methods whose type parameter constraints are violated.
- For a method group conversion, remove methods with the wrong return type or ref mode.

Implements dotnet/csharplang#98

See also dotnet#24675 for a proposal to improve the quality of diagnostics for the method group conversion. This change occasionally exposes this opportunity for diagnostic improvement (search for references to dotnet#24675 in the source to find examples).
gafter added a commit to dotnet/roslyn that referenced this issue Feb 20, 2018
…4756)

- Implement improved overload candidates, aka "Bestest Betternes".
- Refine candidate set based on static/instance receiver, or static context
- Refine candidate methods whose type parameter constraints are violated.
- For a method group conversion, remove methods with the wrong return type or ref mode.
- Implement improved diagnostics for some cases of method group conversion failure

Implements dotnet/csharplang#98
Fixes #24675
@jcouv jcouv changed the title Champion "Bestest Betterness" Champion "Improved overload candidates" (15.7) Mar 16, 2018
@alrz
Copy link
Contributor

alrz commented May 25, 2018

the "smallish feature" tag on this blows my mind. there should be a sign next to anything related to overload resolution that reads "KEEP OUT".

@Kryptos-FR
Copy link

Is there any plan to change the behavior describe here: dotnet/roslyn#250 (comment)

Resolving the overload with the float overload seems wrong, unless the reason was that sizeof(float) and sizeof(int) are both 4 bytes. But in that case, I would expect the resolution with long to be with the double overload. Unfortunately, it still resolves to the float overload.

Things get weirder when a third overload with decimal comes into play: now we have overload ambiguity. One can wonder why we didn't have it with float and double in the first place.

As soon as a decimal overload exists, it is always ambiguous, even if we remove either float or 1decimal` overload. What is the rational with that? IMHO the rule should change to always be ambiguous and rejected by the compiler, so that the developer has to cast the value to the desired type. So many mistakes could be avoided that way.

void Test()
{
    Method(char.MaxValue);
    Method(byte.MaxValue);
    Method(short.MaxValue);
    Method(int.MaxValue);
    Method(long.MaxValue);
    Method(float.MaxValue);
}

void Method(float value) { } // all calls resolved to that overload
void Method(double value) { }
//void Method(decimal value) { }

@HaloFour
Copy link
Contributor

Changing the behavior where the code currently compiles would be a breaking change and could adversely affect existing programs. IIRC all of the improvements above were to eliminate overload candidates in situations where the code would fail to compile due to an ambiguity but where some of the candidates would not be invocable.

@gafter
Copy link
Member Author

gafter commented Jun 29, 2018

@Kryptos-FR No, there are no such plans.

@Kryptos-FR
Copy link

@HaloFour It could be an opt-in compilation switch to enforce "stricter" rules regarding overload resolution. In that mode the above examples would fail to compile with a meaningful error message but without that switch (default behavior) it will compile as usual.
At the very least the Roslyn analyzers should be updated to display a warning in such cases.

@HaloFour
Copy link
Contributor

HaloFour commented Jul 2, 2018

@Kryptos-FR

It could be an opt-in compilation switch

That would create a dialect in the language which the team would be very unlikely to consider.

@Logerfo
Copy link
Contributor

Logerfo commented Oct 2, 2018

@richard-fine It has been shipped in C# 7.3, as you can see here, which comes with Visual Studio 15.7.

@gafter
Copy link
Member Author

gafter commented Oct 3, 2018

The language spec is stuck at version 6 while we work with ECMA to plan how we're going to evolve it going forward. Having said that, you can find specs of varying degrees of precision for recently shipped features

dahlbyk added a commit to dahlbyk/Xamarin.Forms that referenced this issue Jul 9, 2019
CS0121: The call is ambiguous between the following methods or
properties: 'Device.InvokeOnMainThreadAsync(Action)' and
'Device.InvokeOnMainThreadAsync(Func<Task>)'

dotnet/roslyn#14885
dotnet/csharplang#98
StephaneDelcroix pushed a commit to xamarin/Xamarin.Forms that referenced this issue Jul 16, 2019
…6718)

* [Core] Use local functions

* [Core] Extract DeviceUnitTests

* [Core] Ensure actually called from main thread

* [Core] Add InvokeOnMainThread tests

WithAsyncTask is expected to fail.

* [Core] Rename local wrapper functions

* [Core] Avoid ambiguous call

CS0121: The call is ambiguous between the following methods or
properties: 'Device.InvokeOnMainThreadAsync(Action)' and
'Device.InvokeOnMainThreadAsync(Func<Task>)'

dotnet/roslyn#14885
dotnet/csharplang#98
@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Oct 16, 2020
@tcfialho
Copy link

Any news about that?

@333fred
Copy link
Member

333fred commented Feb 27, 2021

This has been implemented since C# 7.3.

@jcouv
Copy link
Member

jcouv commented May 14, 2021

@Nintynuts dotnet/roslyn#26077 was closed because it doesn't repro as of C# 7.3.
Please file a bug with a code snippet/repro if you think this feature was not implemented as spec'ed in C# 7.3.

@las-nsc
Copy link

las-nsc commented May 14, 2021

@jcouv I tried to build an example to demonstrate and couldn't reproduce the issue. It turned out that a nuget reference to Microsoft.Net.Compilers was causing the issue.

@gafter
Copy link
Member Author

gafter commented May 13, 2022

Standard spec in progress at dotnet/csharpstandard#263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests