csharpstandard icon indicating copy to clipboard operation
csharpstandard copied to clipboard

C#6 §11.11.9 (Delegate equality operators) vs. delegate variance

Open KalleOlaviNiemitalo opened this issue 2 years ago • 8 comments

ECMA-334:2022 §11.11.9 (Delegate equality operators) specifies:

If operator overload resolution resolves to either delegate equality operator, and the binding-time types of both operands are delegate types as described in §19 rather than System.Delegate, and there is no identity conversion between the binding-type operand types, a binding-time error occurs.

Note: This rule prevents comparisons which can never consider non-null values as equal due to being references to instances of different types of delegates. end note

Does that require a binding-time error in this case:

namespace DelegateVariance
{
    delegate void Dee<in T>(T value);

    class Program
    {
        static void Main()
        {
            Dee<object> d1 = Ignore;
            Dee<string> d2 = d1;

            System.Console.WriteLine(d1 == d2);
        }

        static void Ignore(object obj) {}
    }
}

If I understand correctly, the binding-time types are Dee<object> and Dee<string>, and there is no identity conversion between those, although there is an implicit reference conversion from Dee<object> to Dee<string>. The standard would then seem to require a binding-time error, but:

  • The rationale in the note does not apply because the comparison returns true if it is allowed to execute.
  • Roslyn 4.3.0-3.22465.2+7d8de40e2bfe5bd2546ca49177166741af4c8e07 does not report a compile-time error for this.

(Inspired by https://github.com/dotnet/command-line-api/issues/1888.)

KalleOlaviNiemitalo avatar Nov 02 '22 00:11 KalleOlaviNiemitalo

Thanks, we'll discuss this. (I wonder whether the implicit conversion is being used as part of overload resolution, leading to a result of the delegate equality operator between two (effective) operands of type Dee<object>. But I haven't checked in detail yet.)

jskeet avatar Nov 02 '22 07:11 jskeet

I thought this could be fixed by changing the wording to "and there is neither an identity conversion nor an implicit reference conversion between the binding-type operand types", but consider the case with two type parameters:


namespace DelegateVariance
{
    delegate void Dee2<in T1, in T2>(T1 value1, T2 value2);

    class Program
    {
        static void Main()
        {
            Dee2<object, object> d = Ignore;
            Dee2<object, string> d1 = d;
            Dee2<string, object> d2 = d;

            System.Console.WriteLine(d1 == d2);
        }

        static void Ignore(object obj1, object obj2) {}
    }
}

Here, there is no implicit reference conversion between the compile-time types of d1 and d2, but comparing them for equality can still be meaningful.

Surprisingly though, sharplab.io shows:

warning CS0252: Possible unintended reference comparison; to get a value comparison, cast the left hand side to type 'MulticastDelegate'

i.e. it did not use bool operator ==(System.Delegate x, System.Delegate y); from §11.11.9. I don't understand how this is justified by §11.4.5 (Binary operator overload resolution) and §11.6.4 (Overload resolution). Microsoft does define public static bool operator == (MulticastDelegate? d1, MulticastDelegate? d2);, which is not a predefined equality operator in §11.11.9; is it then considered a user-defined operator?

KalleOlaviNiemitalo avatar Nov 05 '22 15:11 KalleOlaviNiemitalo

If I read section 11.11.7 correctly, there is a predefined operator public static bool operator == (MulticastDelegate d1, MulticastDelegate d2);, which compares by reference equality instead of looking at invocation lists; and likewise for each delegate type (declared with the delegate keyword). However, these predefined operators are never selected by overload resolution because MulticastDelegate has a user-defined equality operator.

KalleOlaviNiemitalo avatar Nov 05 '22 15:11 KalleOlaviNiemitalo

If that is the intended interpretation, then the standard should specify the MulticastDelegate type so that implementations are not allowed to select the predefined equality comparison operators in overload resolution.

KalleOlaviNiemitalo avatar Nov 08 '22 21:11 KalleOlaviNiemitalo

MulticastDelegate is intentionally in neither the CLI or C# Standards. It is a historical artefact from the MS CLI implementation and is not intended to be user-visible.

Nigel-Ecma avatar Nov 09 '22 05:11 Nigel-Ecma

I now see 11.11.7 (Reference type equality operators) says "Every class type C", which excludes delegate types, but includes MulticastDelegate.

If an implementation interposes MulticastDelegate between the Delegate class and delegate types like .NET does, but does not define a user-defined operator ==(MulticastDelegate, MulticastDelegate), then 11.11.7 requires a predefined reference-equality operator ==(MulticastDelegate, MulticastDelegate), which can be selected by overload resolution on delegate types. Would such an implementation be conforming? If that is not intended, I think 11.11.7 should exclude all types derived from Delegate, although it wouldn't need to mention MulticastDelegate specifically.

KalleOlaviNiemitalo avatar Nov 09 '22 06:11 KalleOlaviNiemitalo

(We've failed to discuss this before.)

Aim for today's meeting, to pick between:

  • Decide it's not an issue (seems unlikely; thanks @KalleOlaviNiemitalo for writing it up so clearly)
  • Decide it's an issue we want to address before C# 7, and assign it
  • Decide it's an issue we want to address eventually, but deprioritize (just remove the meeting-discuss label)

jskeet avatar Mar 01 '23 12:03 jskeet

Meeting notes:

  • We would like to avoid mentioning MulticastDelegate
  • We believe the compiler code here to be relevant, but more work is needed to trace this deeper
  • The gist might be "if there's some common type that there's a reference conversion from to each of the operand types, let's use that" - but we need to be rather more precise
  • Low priority, but should be fixed eventually

jskeet avatar Mar 01 '23 20:03 jskeet