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: bestest betterness #250

Closed
gafter opened this issue Feb 5, 2015 · 22 comments
Closed

Proposal: bestest betterness #250

gafter opened this issue Feb 5, 2015 · 22 comments

Comments

@gafter
Copy link
Member

gafter commented Feb 5, 2015

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.
  2. When a method group contains some generic methods whose type parameters 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.

@theoy
Copy link
Contributor

theoy commented Feb 5, 2015

Suggestion would be to really identify the key scenarios before getting too far in this direction. I strongly supported the C# 5 changes for overload resolution changes because they made a significant difference to the experience in using async lambdas in key APIs such as Task.Run(...) and Task.ContinueWith(...).

It's also not the end of the world if we forego changes here if key language features or APIs don't need it.

Either way, I'm sure this particular discussion will prove fascinating and provide tons of puzzles for people to find tiny expected compat breaks. 😄

@mikedn
Copy link

mikedn commented Feb 5, 2015

There's one case that is unlikely to be fixed due to compatibility but that I think it's worth mentioning because there's nothing "better" about the overload choice.

The conversion rules are such that a float overload is picked over a double overload when the argument is an integer:

static void foo(double d) {
    Console.WriteLine("{0:f}", d);
}
static void foo(float f) {
    Console.WriteLine("{0:f}", f);
}
static void Main() {
    foo(int.MaxValue);
}

This prints 2147484000.00 instead of 2147483647.00. The rules are suitable for reference conversions but for integer-floating point conversions the rules lead to precision loss.

@HaloFour
Copy link

HaloFour commented Feb 5, 2015

@mikedn Yeow, that seems really wrong. Glad I've never run into that before (or at least I don't think I have).

In my opinion I would think that the overload resolution would always favor double over float where an implicit conversion is required. And in any case if there is the potential for loss of data that an explicit cast should be required, although I know that would annoy people.

Having come from doing a lot of ETL in the financial sector where a favored format was Excel files it was amazing how many times I had to deal with the fact that files would be effectively corrupted due to someone not noticing that Excel implicitly converts 16 digits worth of text to a floating point value which would strip the last digit.

@gafter
Copy link
Member Author

gafter commented Feb 5, 2015

@theoy These particular proposed changes are driven by recurring customer pain points.

@BrannonKing
Copy link

So has the issue of constraints not being part of the method footprint been addressed in C# 5.0 and I somehow missed it? History: http://blogs.msdn.com/b/ericlippert/archive/2009/12/10/constraints-are-not-part-of-the-signature.aspx

If not I would still really like to see constraint resolution support added. I would like to have extension methods that only differ by constraint.

@HaloFour
Copy link

HaloFour commented Feb 5, 2015

@BrannonKing That's actually not permitted by the CLR. The generic parameters are a part of the signature but the constraints on those parameters are not so you cannot have an assembly where the same method exists with only differing generic constraints.

However, C# could potentially take those generic constraints into consideration with overload resolution so that Eric's sample doesn't result in a compiler error. That would be nice.

@jmarolf
Copy link
Contributor

jmarolf commented Feb 5, 2015

Related is considering whether or not the return type matches in a method group conversion to affect whether or not the conversion exists.

Consider the case below, which recently bit me for the two overloads(1,2) for CodeAction.Create:

using System;
public class A { }
public class B { }
public class C { }
public static class Program
{
    static void Main() { Create(ReturnNull); }
    private static A ReturnNull(C arg) { return null; }
    static void Create(Func<C, A> func) { }
    static void Create(Func<C, B> func) { }
}

In both the rolsyn and native compilers we get:

Error: The call is ambiguous between the following methods or properties: 'Program.Create(System.Func<C,A>)' and 'Program.Create(System.Func<C,B>)'

But if we explicitly cast, it compiles without issue:

using System;
public class A { }
public class B { }
public class C { }
public static class Program
{
    static void Main() { Create((Func<C, A>)ReturnNull); }
    private static A ReturnNull(C arg) { return null; }
    static void Create(Func<C, A> func) { }
    static void Create(Func<C, B> func) { }
}

and if we remove one method it will correctly identify the return type of "ReturnNull"

using System;
public class A { }
public class B { }
public class C { }
public static class Program
{
    static void Main() { Create(ReturnNull); }
    private static A ReturnNull(C arg) { return null; }
    static void Create(Func<C, B> func) { }
}

Error: 'A Program.ReturnNull(C)' has the wrong return type

There is no successful conversion from Func<C, A> to Func<C, B>. It really feels weird for the compiler to pick a conversion it knows won't succeed and then say the case is ambiguous.

@ljw1004
Copy link
Contributor

ljw1004 commented Mar 11, 2015

Eric Lippert wrote about the difficulties in eliminating static methods from the candidate set, relating to the "Color Color" problem...
http://blogs.msdn.com/b/ericlippert/archive/2009/07/06/color-color.aspx

@paulomorgado
Copy link

That won't happen in C# if it's already known to be invoking an instance method: instance.Method().

And if you're used to qualify everything in the instance with this, you're already in an happy place.

@ljw1004
Copy link
Contributor

ljw1004 commented Mar 11, 2015

@paulomorgado , that's exactly what Eric wrote. He wrote that

EITHER

  • You have one set of overload rules for cases like "this.foo()" and "instance.foo()"
  • and a different set of overload rules for cases like "Class1.foo()"
  • and a different set of overload rules for cases like "Color.foo()"

OR

  • you have just one common set of overload rules for all three cases.

Eric wrote, "rather than complicate the language further, we made the pragmatic choice of simply deferring the staticness check until after the bestness check. That’s not perfect but it is good enough for most cases and it solves the common problem"

@gafter
Copy link
Member Author

gafter commented Mar 11, 2015

@ljw1004 I agree with what you wrote and Eric's post. However, the difference is fairly small - you can think of it as being a change to how you construct the candidate sets that are fed to overload resolution. Either way, there is real customer benefit.

@iskiselev
Copy link

Not sure if here is proper place (probably it should be in separate issue). Looks like generic method type inference could be improved a little bit more for following situation:

public interface IA<out TA>
        where TA : IA
    {
        TA Item { get; }
    }

    public interface IA : IA<IA> { }

    public class A : IA, IA<A>
    {
        public A Item { get; set; }
        IA IA<IA>.Item => Item;
    }

    public static class Helper
    {
        public static TA GetItem<TA>(this IA<TA> parent)
            where TA : IA
        {
            return parent.Item;
        }
    }

We can't use new A().GetItem(), as there are two candidates: GetItem<A>() and GetItem<IA>(). It could be solve if one in such situations method with most derived return type would be selected (if there is only one such method). Than, in above example GetItem<A>() would be selected, as A:IA.

@alrz
Copy link
Member

alrz commented Sep 15, 2016

What is current workaournd for this scenario?

class A     { protected void M<T>(T x) {} } 
class B : A { protected new void M<T>(T x) where T : B => base.M(x); } // additional logic
class C : B { public void F() => M(5); } // ERROR

Is there any? (other than using different names and dynamic type check)

@bbarry
Copy link

bbarry commented Sep 15, 2016

@alrz there is reflection:

class C : B {
    public void F() =>
        typeof(A)
            .GetMethod("M", BindingFlags.Instance | BindingFlags.NonPublic)
            .MakeGenericMethod(typeof(int))
            .Invoke(this, new object[] {5});
}

I think you could IL weave also.

No good workarounds though.

@mattwar
Copy link
Contributor

mattwar commented Sep 15, 2016

@alrz I'm not sure what you are trying to work around.

In your example is the need to use the 'new' keyword the workaround you are using to cause the error to occur? Are you suggesting that you should just be able to redefine the constraints in a sub type to enforce more restrictive rules?

If that were true, how would you rule out invocations that by-passed your hierarchy?

class A     { protected void M<T>(T x) {} } 
class B : A { protected override void M<T>(T x) where T : B => base.M(x); } // additional logic
class C : B { public void F() => ((A)this).M(5); } // NO ERROR

@alrz
Copy link
Member

alrz commented Sep 15, 2016

@bbarry Using reflection instead of (possible) static dispatch? No, thanks! I'd rather eat cake than using reflection for this! I've came up with a solution, but not so generic,

class B : A { protected void M(B x) => base.M(x); } // additional logic

@mattwar I know that overriding with different constraints violates polymorphism, that's why I've used new keyword there. Basically, point (2) in the OP would solve the problem so that M(5) would call A.M<T>. I actually did try ((A)this) but since the method is protected it won't work.

@gafter
Copy link
Member Author

gafter commented Jan 14, 2017

See also #15911 for possible related (lambda rather than method group) opportunity to improve overload resolution.

@Pzixel
Copy link

Pzixel commented Jan 15, 2017

Is it possible to change current overload resolution for Instance methods always take precedence over extension methods? Because it's quite annoying. For example if we have instance method which takes object and extension with same name which takes int and you are calling obj.Foo(5) I'd like the extension method to be called because it fits better. It is very useful when we have want to add some specialization to instance method which provides some basic functionality..

@gafter
Copy link
Member Author

gafter commented Jan 15, 2017

Yes, the current overload resolution rules will
never choose an extension method if an instance method is applicable

@gafter
Copy link
Member Author

gafter commented Jan 15, 2017

Changing that would change the behavior of existing programs so we cannot realistically change it

@jhudsoncedaron
Copy link

@Pzixel : That sounds like a use of #pragma and a locally modified C# compiler. Personally I wouldn't attempt that one because of the risk of catastrophic failure (I would only dare such a thing if the original code would generate a compiler error on the original compiler). I feel obliged to point out this solution none the less.

@gafter
Copy link
Member Author

gafter commented Mar 28, 2017

This is now tracked at dotnet/csharplang#98

@gafter gafter closed this as completed Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests