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

Existing equality operators should take precedence over compiler lowering for tuple equality #26809

Closed
TessenR opened this issue May 11, 2018 · 4 comments
Assignees
Labels
Area-Compilers Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@TessenR
Copy link

TessenR commented May 11, 2018

Currently compiler lowers tuple equality to t1.Item1 == t2.Item1 && t1.Item2 == t2.Item2 && ... even when there is an existing and applicable equality operator.

While it might not be a big problem by itself as hopefully nobody writes their own System.ValueTuple I wonder how extension everyting language feature would interact with this lowering since it would be possible to provide an extension equality operator for the System.ValueTuple type family.

Steps to Reproduce:

Compiler the following code with VS 15.6.x:

using System;
namespace System
{
    struct ValueTuple<T1, T2>
    {
      void M(ValueTuple<int, int> t)
      {
        var x = t == t;
      }
      public static extern bool operator ==(ValueTuple<T1, T2> t1, ValueTuple<T1, T2> t2);
      public static extern bool operator !=(ValueTuple<T1, T2> t1, ValueTuple<T1, T2> t2);
    }
}

It compiles without an error since it's allowed to provide your own System.ValueTuple type.
Now upgrade to VS 15.7 and C# 7.3.

Expected Behavior:

Code still compiles and uses equality operators.

Actual Behavior:
There are multiple errors for missing tuple members.

error CS8128: Member 'Item1' was not found on type 'ValueTuple<T1, T2>' from assembly '_, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.

If you add these members you'll still get different behavior from the previous version since the equality expression will be lowered instead of applying the existing and applicable operator.

@gafter
Copy link
Member

gafter commented May 12, 2018

Kind of late now.

@TessenR
Copy link
Author

TessenR commented May 14, 2018

So is it a bug or not?
Given it's not closed with the by design tag I assume it is a bug.
Given that these tests were reviewed and accepted by a few people including you I wonder if this is a deliberate compatibility break.

Is it supposed to be picked over extension equality operators too? It would be kinda really strange if extension operators would work and normal operators would not. It would also be quite strange if some specific extension operators won't work with some specific types. However since current design was tested and reviewed it seems intentional.

@jcouv could you please shed some light on this?

        [Fact]
        public void TestCustomOperatorPreferred()
        {
            var source = @"
namespace System
{
    public struct ValueTuple<T1, T2>
    {
        public T1 Item1;
        public T2 Item2;

        public ValueTuple(T1 item1, T2 item2)
        {
            this.Item1 = item1;
            this.Item2 = item2;
        }

        public static bool operator ==(ValueTuple<T1, T2> t1, ValueTuple<T1, T2> t2)
            => throw null;
        public static bool operator !=(ValueTuple<T1, T2> t1, ValueTuple<T1, T2> t2)
            => throw null;

        public override bool Equals(object o)
            => throw null;
        public override int GetHashCode()
            => throw null;
    }
}
public class C
{
    public static void Main()
    {
        var t1 = (1, 1);
        var t2 = (2, 2);
        System.Console.Write(t1 == t2);
        System.Console.Write(t1 != t2);
    }
}
";
            var comp = CreateStandardCompilation(source, options: TestOptions.DebugExe);
            comp.VerifyDiagnostics();
            // Note: tuple equality picked ahead of custom operator== (small compat break)
            CompileAndVerify(comp, expectedOutput: "FalseTrue");
        }

        [Fact]
        void TestTupleEqualityPreferredOverCustomOperator_Nested()
        {
            string source = @"
public class C
{
    public static void Main()
    {
        System.Console.Write( (1, 2, (3, 4)) == (1, 2, (3, 4)) );
    }
}
namespace System
{
    public struct ValueTuple<T1, T2>
    {
        public T1 Item1;
        public T2 Item2;

        public ValueTuple(T1 item1, T2 item2)
        {
            this.Item1 = item1;
            this.Item2 = item2;
        }

        public static bool operator ==(ValueTuple<T1, T2> t1, ValueTuple<T1, T2> t2)
            => throw null;
        public static bool operator !=(ValueTuple<T1, T2> t1, ValueTuple<T1, T2> t2)
            => throw null;

        public override bool Equals(object o)
            => throw null;

        public override int GetHashCode()
            => throw null;
    }
    public struct ValueTuple<T1, T2, T3>
    {
        public T1 Item1;
        public T2 Item2;
        public T3 Item3;

        public ValueTuple(T1 item1, T2 item2, T3 item3)
        {
            this.Item1 = item1;
            this.Item2 = item2;
            this.Item3 = item3;
        }
    }
}
";

            var comp = CreateStandardCompilation(source, options: TestOptions.DebugExe);
            comp.VerifyDiagnostics();

            // Note: tuple equality picked ahead of custom operator==
            CompileAndVerify(comp, expectedOutput: "True");
        }

@jcouv
Copy link
Member

jcouv commented May 14, 2018

Sorry for the late reply. I was thrown off by the use of extern when I first saw this issue, but in fact it's just a custom ValueTuple implementation with operator== implemented.
Yes, this compat break was a deliberate design decision. See speclet.
I'll update the doc for breaking changes accordingly. (PR pending review: #26844)

@jcouv jcouv added the Resolution-By Design The behavior reported in the issue matches the current design label May 14, 2018
@jcouv jcouv self-assigned this May 14, 2018
@jcouv jcouv added this to the 15.8 milestone May 14, 2018
@jcouv
Copy link
Member

jcouv commented May 14, 2018

Also, yes, the tuple comparison operators come ahead of any extensions, if/when "extension everything" is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

3 participants