Bug in spec for better conversion from expression

[gafter]

@gafter commented on Tue Sep 27 2016

The spec for better conversion from expression doesn't match the implementation. The spec says

7.5.3.3 Better conversion from expression
Given an implicit conversion C1 that converts from an expression E to a type T1, and an implicit conversion C2 that converts from an expression E to a type T2, C1 is a better conversion than C2 if E does not exactly match T2 and one of the following holds:
• E exactly matches T1 (§7.5.3.4)
• T1 is a better conversion target than T2 (§7.5.3.5)

but the implementation does

            bool t1MatchesExactly = ExpressionMatchExactly(node, t1, ref useSiteDiagnostics);
            bool t2MatchesExactly = ExpressionMatchExactly(node, t2, ref useSiteDiagnostics);

            if (t1MatchesExactly)
            {
                if (!t2MatchesExactly)
                {
                    // - E exactly matches T1
                    return BetterResult.Left;
                } // NOTE NO ELSE HERE
            }
            else if (t2MatchesExactly)
            {
                // - E exactly matches T2
                return BetterResult.Right;
            }

            // - T1 is a better conversion target than T2
            return BetterConversionTarget(node, t1, conv1, t2, conv2, ref useSiteDiagnostics, out okToDowngradeToNeither);

Specifically, if both match exactly, then we test for a better conversion target. This is what is implemented:

7.5.3.3 Better conversion from expression
Given an implicit conversion C1 that converts from an expression E to a type T1, and an implicit conversion C2 that converts from an expression E to a type T2, C1 is a better conversion than C2 if one of the following holds:
• E does not exactly match T2 and E exactly matches T1 (§7.5.3.4)
• E either exactly matches both T1 and T2, or exactly matches neither T1 and T2, and also T1 is a better conversion target than T2 (§7.5.3.5)

---

@gafter commented on Tue Sep 27 2016

This issue is preventing the proposed spec approach for out var overload resolution:

an inferred “out var” argument exactly matches (see 7.5.3.4 Exactly matching Expression) every type.

from having the effect of causing out var arguments to ignore the corresponding parameter position.

---

@gafter commented on Tue Sep 27 2016

This appears to be mooted in the ECMA version. But the ECMA version has no concept of exactly matching expression.

---

@AlekseyTs commented on Tue Sep 27 2016

It looks like the implementation is intentional, we would want the following scenario compiling without errors and using the second overload:

using System;
namespace ConsoleApplication2
{
    public static class Foo
    {
        public static U IfNotNull<T, U>(this T value, Func<T, U> selector)
        {
            System.Console.WriteLine("IfNotNull<T, U>(this T value, Func<T, U> selector");
            return value != null ? selector(value) : default(U);
        }
        public static U IfNotNull<T, U>(this T? source, Func<T, U> selector) where T : struct
        {
            System.Console.WriteLine("IfNotNull<T, U>(this T? source, Func<T, U> selector)");
            return source.HasValue ? selector(source.Value) : default(U);
        }
    }
    class Program
    {
        static void Main(string[] args)
        {
            double? val = null;
            var d1 = val.IfNotNull(v => v / 100);
            var d2 = Foo.IfNotNull(val, v => v / 100);
        }
    }
}

---

@gafter commented on Tue Sep 27 2016

Indeed. Moreover, we want to implement the ECMA spec instead of the old C# 4 spec, which is quoted above in the OP.

---

@gafter commented on Tue Sep 27 2016

Closing, as we are intending to use the ECMA spec as our base, and it does not have this problem.

---

@MadsTorgersen commented on Tue Sep 27 2016

We cannot use the ECMA spec as our base, because it does not reflect the changes for "better betterness" that were done in C# 6.0. Instead, the ECMA spec needs to embrace the Microsoft spec when it gets updated for C# 6.0.

I agree that there is a discrepancy between how "better betterness" was spec'ed and how it's implemented. The implementation is already in use, so unless there's a strong reason not to (I don't see any), we should go with what's implemented.

That means adjusting the spec a bit, so that better conversion target is checked both when neither matches precisely, and when both do. Something like this:

7.5.3.3 Better conversion from expression
Given an implicit conversion C1 that converts from an expression E to a type T1, and an implicit conversion C2 that converts from an expression E to a type T2, C1 is a better conversion than C2 if E does not exactly match T2 and one of the following holds:
• E exactly matches T1 and E does not exactly match T2 (§7.5.3.4)
• E exactly matches both or neither of T1 and T2, and T1 is a better conversion target than T2 (§7.5.3.5)

---

@gafter commented on Tue Sep 27 2016

@MadsTorgersen
I think you intended to delete

E does not exactly match T2 and

from the prefix to the bulleted list.

---

@gafter commented on Tue Sep 27 2016

@MadsTorgersen That reopen the question if how you would like to handle out var in the spec.

[BillWagner]

This is for §12.6.4.4 in the standard.

This is part of the "better betterness" for C# 6.0. Moving to dotnet/csharpstandard.

[jskeet]

Related: #157

Also related: https://github.com/dotnet/roslyn/blob/main/docs/specs/CSharp%206/Better%20Betterness.md

[jskeet]

For the record: Neal asked this to be assigned to him (against his better judgement).