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

ValueTuple.Equals issue #320

Open
ghost opened this issue Jun 25, 2018 · 31 comments
Open

ValueTuple.Equals issue #320

ghost opened this issue Jun 25, 2018 · 31 comments
Labels
LDM Considering LDM has reviewed and we think this has merit

Comments

@ghost
Copy link

ghost commented Jun 25, 2018

I tried this code:

Dim t1 As (Double, Single) = (1.0R, 2.0F)
Dim t2 As (Integer, Integer) = (1, 2)
Console.WriteLine(t1 = t2)

But VB.NET says:

Operator '=' is not defined for types '(Double, Single)' and '(Integer, Integer)'.

The unusual thing is that C# accepts this:

(double, float) t1 = (1d, 2f);
(int, int) t2 = (1, 2);
Console.WriteLine(t1 == t2);

Why can't VB.NET compare the two tuples, while C# can?
Note that VB.NET has no problem in assigning one of the tuples to the other one:
t1 = t2

@reduckted
Copy link
Contributor

Very interesting. For reference, here's the original C# code, the IL that it compiled to, and the C# that it decompiles to:

static void Main()
{
    (double, float) t1 = (1d, 2f);
    (int, int) t2 = (1, 2);
    Console.WriteLine(t1 == t2);
}
.method private hidebysig static 
    void Main () cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 70 (0x46)
    .maxstack 3
    .entrypoint
    .locals init (
        [0] valuetype [System.Runtime]System.ValueTuple`2<float64, float32>,
        [1] valuetype [System.Runtime]System.ValueTuple`2<float64, float32>,
        [2] valuetype [System.Runtime]System.ValueTuple`2<int32, int32>
    )

    IL_0000: ldloca.s 0
    IL_0002: ldc.r8 1
    IL_000b: ldc.r4 2
    IL_0010: call instance void valuetype [System.Runtime]System.ValueTuple`2<float64, float32>::.ctor(!0, !1)
    IL_0015: ldc.i4.1
    IL_0016: ldc.i4.2
    IL_0017: newobj instance void valuetype [System.Runtime]System.ValueTuple`2<int32, int32>::.ctor(!0, !1)
    IL_001c: ldloc.0
    IL_001d: stloc.1
    IL_001e: stloc.2
    IL_001f: ldloc.1
    IL_0020: ldfld !0 valuetype [System.Runtime]System.ValueTuple`2<float64, float32>::Item1
    IL_0025: ldloc.2
    IL_0026: ldfld !0 valuetype [System.Runtime]System.ValueTuple`2<int32, int32>::Item1
    IL_002b: conv.r8
    IL_002c: bne.un.s IL_003f

    IL_002e: ldloc.1
    IL_002f: ldfld !1 valuetype [System.Runtime]System.ValueTuple`2<float64, float32>::Item2
    IL_0034: ldloc.2
    IL_0035: ldfld !1 valuetype [System.Runtime]System.ValueTuple`2<int32, int32>::Item2
    IL_003a: conv.r4
    IL_003b: ceq
    IL_003d: br.s IL_0040

    IL_003f: ldc.i4.0

    IL_0040: call void [System.Console]System.Console::WriteLine(bool)
    IL_0045: ret
} // end of method Program::Main
private static void Main()
{
    ValueTuple<double, float> valueTuple = new ValueTuple<double, float>(1.0, 2f);
    ValueTuple<int, int> valueTuple2 = new ValueTuple<int, int>(1, 2);
    ValueTuple<double, float> valueTuple3 = valueTuple;
    ValueTuple<int, int> valueTuple4 = valueTuple2;
    Console.WriteLine(valueTuple3.Item1 == (double)valueTuple4.Item1 && valueTuple3.Item2 == (float)valueTuple4.Item2);
}

Not really sure why it's copying the tuples before it compares them, but whatever. ¯\_(ツ)_/¯

Interestingly, you can use the .Equals() method in VB as a workaround for now, which actually seems to compile to what you'd kind of expect the = operator to compile to:

Sub Main()
    Dim t1 As (Double, Single) = (1.0R, 2.0F)
    Dim t2 As (Integer, Integer) = (1, 2)
    Console.WriteLine(t1.Equals(t2))
End Sub
.method public static 
    void Main () cil managed 
{
    .custom instance void [mscorlib]System.STAThreadAttribute::.ctor() = (
        01 00 00 00
    )
    // Method begins at RVA 0x2128
    // Code size 64 (0x40)
    .maxstack 3
    .entrypoint
    .locals init (
        [0] valuetype [mscorlib]System.ValueTuple`2<float64, float32>,
        [1] valuetype [mscorlib]System.ValueTuple`2<int32, int32>,
        [2] valuetype [mscorlib]System.ValueTuple`2<int32, int32>
    )

    IL_0000: ldloca.s 0
    IL_0002: ldc.r8 1
    IL_000b: ldc.r4 2
    IL_0010: call instance void valuetype [mscorlib]System.ValueTuple`2<float64, float32>::.ctor(!0, !1)
    IL_0015: ldloca.s 1
    IL_0017: ldc.i4.1
    IL_0018: ldc.i4.2
    IL_0019: call instance void valuetype [mscorlib]System.ValueTuple`2<int32, int32>::.ctor(!0, !1)
    IL_001e: ldloca.s 0
    IL_0020: ldloc.1
    IL_0021: stloc.2
    IL_0022: ldloc.2
    IL_0023: ldfld !0 valuetype [mscorlib]System.ValueTuple`2<int32, int32>::Item1
    IL_0028: conv.r8
    IL_0029: ldloc.2
    IL_002a: ldfld !1 valuetype [mscorlib]System.ValueTuple`2<int32, int32>::Item2
    IL_002f: conv.r4
    IL_0030: newobj instance void valuetype [mscorlib]System.ValueTuple`2<float64, float32>::.ctor(!0, !1)
    IL_0035: call instance bool valuetype [mscorlib]System.ValueTuple`2<float64, float32>::Equals(valuetype [mscorlib]System.ValueTuple`2<!0, !1>)
    IL_003a: call void [mscorlib]System.Console::WriteLine(bool)
    IL_003f: ret
} // end of method Module1::Main

And for completeness, here's the C# code that the VB code decompiles to:

[STAThread]
public static void Main()
{
    ValueTuple<double, float> valueTuple = new ValueTuple<double, float>(1.0, 2f);
    ValueTuple<int, int> valueTuple2 = new ValueTuple<int, int>(1, 2);
    ValueTuple<int, int> valueTuple3 = valueTuple2;
    Console.WriteLine(valueTuple.Equals(new ValueTuple<double, float>((double)valueTuple3.Item1, (float)valueTuple3.Item2)));
}

@ghost
Copy link
Author

ghost commented Jun 25, 2018

@reduckted
Thanks for the info. I am aware that equals work fine.
I don't think it is a problem in compiling code to IL code, because = operator uses Equals method internally. I think that VB.NET sees two deffirent types of tuples and has no rule to convert between them, so it can't find a suitable = operator to use. This is wrong, becuase = operator should be safely used betwwen any two tuples, even they differ in length and element types (it can return false). This is the first time I see C# smarter than VB.NET!
I think the overload = operator should be defined in ValueTuple class, and VB.NET should accept it between any two tuples.

@reduckted
Copy link
Contributor

@MohammadHamdyGhanem I don't think it is a problem in compiling code to IL code, because = operator uses Equals method internally.

The = operator only works if it's been defined on the type. It doesn't get automatically converted into the .Equals() method. For example, if we use the old-style Tuple(Of T1, T2) class, you get the same error when you try to use the = operator.

Dim t As Tuple(Of Double, Single)
Console.WriteLine(t = t)
                  ~~~~~ ' Operator '=' is not defined for types 'Tuple(Of Double, Single)' and 'Tuple(Of Double, Single)'.

In fact, take your original code, and compare t1 with itself and it still doesn't work.

Dim t1 As (Double, Single) = (1.0R, 2.0F)
Console.WriteLine(t1 = t1)
                  ~~~~~~~ ' Operator '=' is not defined for types '(Double, Single)' and '(Double, Single)'.

So the issue isn't that you can't compare different types of ValueTuple with the = operator - it's that you can't compare ValueTuple with the = operator at all!

Perhaps someone with more knowledge of the compiler could explain how it handles ValueTuple internally. I assume it does some sort of special handling. I've looked through dotnet/roslyn#347, but like so many roslyn issues, it was just magically implemented and closed without any documentation or a referenced PR. 😢

@Berrysoft
Copy link

The comparison of ValueTuple with the == and != operators is a new feature in C# 7.3, but not in Visual Basic.

@Nukepayload2
Copy link

I think we can't copy c#'s implementation of this feature to VB directly. Because the equality and the inequality compare operators are special in VB. For example, Nothing = String.Empty is True in VB, but null == string.Empty is false in C#.

@Nukepayload2
Copy link

Nukepayload2 commented Jun 25, 2018

@reduckted The Operator = can't be defined in the ValueTuple itself. They should be handled by compilers, because each programming languages have different rules of comparison. The following codes shows the difference between the expected VB equality behavior and the existing C# behavior.

VB

' Compile with /optioncompare:text
Dim a As (Object, String) = (Nothing, "ABC")
Dim b As (String, String) = ("", "aBc")
MsgBox(a = b) ' The output should be True. Because a.GetType.GenericTypeArguments.Length = b.GetType.GenericTypeArguments.Length AndAlso Nothing = "" AndAlso "ABC" = "aBc".

C#

(Object, String) a = (null, "ABC");
(String, String) b = ("", "ABC");
Console.WriteLine(a == b); // The output should be False. Because !a.GetType().Equals(b.GetType()) .

@ghost
Copy link
Author

ghost commented Jun 25, 2018

@reduckted @Berrysoft @Nukepayload2
Thanks, I got it. Is this issue going to be solved?

@Nukepayload2
Copy link

I've found a related LDM note. https://github.com/dotnet/vblang/blob/master/meetings/2018/vbldm-notes-2018.02.28.md

@ghost
Copy link
Author

ghost commented Jun 26, 2018

@reduckted @Berrysoft @Nukepayload2 @srivatsn @migueldeicaza
The issue of comparing empty string and null string can be solved in another way: Any null string should be implicitly converted to empty string. This will eliminate the need for extra work in = operator.
VB.NET programmers treat null string and empty string equally, so I think this will not break anything.

@Nukepayload2
Copy link

The extra work in Operator = can't be eliminated in this way, because Option Compare Text should be implemented as well. String equality and inequality operators should be replaced with Operators.CompareString by the compiler.

@ghost
Copy link
Author

ghost commented Jun 26, 2018

@Nukepayload2
OK. There is something I can't get: ValueTuple.= should just call ValueTuple.Equals which already works fine! I see no problem!
Besides: Whatever rules VB.NEt uses, the already implemented in each type.=, and ValueTuple.= eventually is a sum of checking for element types eauality. If two elements T1.Item1 and t2.Item1. are string, so (T1 = T2) code will contain T1.Item1 = T2.Item1 part, which will use VB.NET rules of compairing strings.
I don't really see any problem!

@Nukepayload2
Copy link

@MohammadHamdyGhanem
ValueTuple.Equals does not have parameters to pass Option Strict and Option Compare, so it can't be used as the equality operator.
You can compile and run the following program, and decompile it to learn the differences between Equals and the equality operator.

Option Strict Off
Option Compare Text
Module Program
    Sub Main(args As String())
        Dim a = (1, "ABC", Nothing)
        Dim b = ("1", "abc", "")
        Dim tupleEquals = a.Equals(b)
        Dim vbEquality = a.Item1 = b.Item1 AndAlso a.Item2 = b.Item2 AndAlso a.Item3 = b.Item3
        Console.WriteLine(tupleEquals)
        Console.WriteLine(vbEquality)
    End Sub
End Module

Decompiled:

Imports Microsoft.VisualBasic.CompilerServices
Imports System.Runtime.CompilerServices
Friend Module Program
    <STAThread>
    Public Sub Main(args As String())
        Dim valueTuple As ValueTuple(Of Integer, String, Object) = New ValueTuple(Of Integer, String, Object)(1, "ABC", Nothing)
        Dim valueTuple1 As ValueTuple(Of String, String, String) = New ValueTuple(Of String, String, String)("1", "abc", "")
        Dim flag As Boolean = valueTuple.Equals(valueTuple1)
        Dim obj As Object = If(CDbl(valueTuple.Item1) <> Conversions.ToDouble(valueTuple1.Item1) OrElse EmbeddedOperators.CompareString(valueTuple.Item2, valueTuple1.Item2, True) <> 0, False, Conversions.ToBoolean(Operators.CompareObjectEqual(valueTuple.Item3, valueTuple1.Item3, True)))
        Console.WriteLine(flag)
        Console.WriteLine(RuntimeHelpers.GetObjectValue(obj))
    End Sub
End Module

@KathleenDollard
Copy link
Contributor

Tuple equality was not initially added for value tuples, because there are some gnarly corners. Check out the Support for == and != for ValueTuple. I recall gnarly corners, but the resulting proposal is rather clean.

I'd love to see us work together on a proposal that makes sense to Visual Basic.

Note: it does need to be strict in the language sense - Visual Basic programmers often don't care about the difference between an empty string and null, but the language must. And how to handle Option Binary/Text, Option Explicit and Option Strict.

@KathleenDollard
Copy link
Contributor

@MohammadHamdyGhanem

Equals can be overridden on a type to return a different value than the equals operator. It's certainly a bad idea, but the language still needs to handle it correctly.

In this case, it would be weird for the = operator on a tuple to follow Equals, and not =. That's why C# chose to recursively compare each item with the == operator.

It could be that VB just needs identical rules with extra paragraphs for the Option cases. I believe that the less subtle differences with C# the better.

@ghost
Copy link
Author

ghost commented Jul 7, 2018

@reduckted @Berrysoft @Nukepayload2 @KathleenDollard @srivatsn
I wrote this:

    Function AreEqual(T1 As ITuple, T2 As ITuple) As Boolean
        If T1.Length <> T2.Length Then Return False
        For i = 0 To T1.Length - 1
            If T1(i) <> T2(i) Then Return False
        Next
        Return True
    End Function

And used it like this:

Dim T1 = (2, 1.3, "")
Dim T2 = (2, 1.3, Nothing)
Console.WriteLine(AreEqual(T1, T2)) ' True
Console.WriteLine(T1.Equals(T2)) ' False

As I said: I see no problem at all. We need to do nothing more. We just compare each pair with = operator or <> operator, and VB.NET will apply all its roles!
You can usr the AreEqual function to implement the = operator for ValueTuples.
Thanks.

@paul1956
Copy link

paul1956 commented Jul 7, 2018

Your function only works as long as the file it is compiled in has the same Options Compare value as the file using it. This has bitten me more than once, there probably should be an analyzer to detect public functions that compare strings even today.

' FIle 1
Option Compare Text
Dim T1 = (2, 1.3, "A")
Dim T2 = (2, 1.3, "a")
Console.WriteLine(AreEqual(T1, T2)) ' False expected True

' File 2
Option Compare Binary
Function AreEqual(T1 As ITuple, T2 As ITuple) As Boolean
        If T1.Length <> T2.Length Then Return False
        For i = 0 To T1.Length - 1
            If T1(i) <> T2(i) Then Return False
        Next
        Return True
    End Function

@ghost
Copy link
Author

ghost commented Jul 7, 2018

@paul1956
This is just an illustration, but the compiler will apply the comparison options of code context on the "AreEqual" code. There is one problem: this code will not work if Option strict is On!.. SO, the compiler can force "option strict off" on the "AreEqual" code only!

@paul1956
Copy link

paul1956 commented Jul 8, 2018

@MohammadHamdyGhanem as a library your example has the issue I posted, if your intent is to have the compiler generate the code inline then I agree. Also there is a feature request to let Options be applied to blocks, this would address the issue if you want to use your code in a library and today you can disable the warning manually around the call to AreEqual and shown in #117 .

@ghost
Copy link
Author

ghost commented Jul 8, 2018

@paul1956
"Inline" is exactly what I mean by context. Thanks.
In fact I didn't use Options for years. I use VB.NET with its defualt options.

@paul1956
Copy link

paul1956 commented Jul 9, 2018

@MohammadHamdyGhanem I never trusted, agreed with or remember the defaults, and I reuse a lot of source between projects so I specify Options in every file if it matters.

@gafter
Copy link
Member

gafter commented Jul 15, 2018

We'll take this as a request to support elementwise equality operators for tuples in VB.

@gafter gafter added LDM Considering LDM has reviewed and we think this has merit LDM Review in progress LDM Review is in progress labels Jul 15, 2018
@ghost
Copy link
Author

ghost commented Jul 15, 2018

@gafter
Thanks.
I want to ask why there is no minor updates for vb.bet?

@gafter
Copy link
Member

gafter commented Jul 16, 2018

Wait, we did add minor versions. In language version 15.3 we added tuple element names inferred from the tuple expressions, and in 15.5 we added support for a digit separator _ in a leading position in a literal, Private Protected, and named arguments in non-trailing positions.

@KathleenDollard
Copy link
Contributor

I thought this was in the docs, but found this quicker: https://stackoverflow.com/questions/32122660/how-to-change-the-vb-net-language-version-in-visual-studio-2015

There are two reasons you might think there are no minor updates:

  • We haven't updated the "What's New" help
  • We don't have a way to set hte language in the UI, as in C#

But when you set via the project file, you get the new features.

@ghost
Copy link
Author

ghost commented Jul 17, 2018

@KathleenDollard
Thanks, but I'm sad VB.NET is treated like this!

@paul1956
Copy link

@KathleenDollard Those instructions really don't help much with VS17 where the line you need to change is not 4 and the value you need to set it to is not obvious. At a minimum there should be detailed instructions so that us dumb VB users can follow them. It is sort of backwards, C# has expert developers but they get super support including a QuickFix, but beginner VB users are told to hack XML without instructions of what we are even supposed to hack and when you click on the error in VS you get C# instructions. Sorry for venting. Also instructions need to be provided for running tests using new features.

@KathleenDollard
Copy link
Contributor

@MohammadHamdyGhanem

I understand your frustration.

As to the What's New: @jcouv created a doc to make it easy for people to work on this. Language-Version-History.md. @gafter pointed out wikipedia is a source for VB history.

As for setting the language version: I don't know why I didn't see this before, but directions for setting the Visual Basic language version have been in the docs for some time.

I don't remember what I searched for, but if anyone has a search they think should find this page that doesn't, I'll pass it on to docs and see if they can improve the SEO.

@paul1956
Copy link

@KathleenDollard "setting VB language version" is the search I tried but the link in the error is really the issue, where there are instruction saying use "15.6", and several useless issues and the one you point to is not listed.

https://www.bing.com/search?q=BC30241+Visual+Basic+Named+argument+expected.+Please+use+language+version+%7b0%7d+or+greater+to+use+non-trailing+named+arguments.&form=VSHELP

@KathleenDollard
Copy link
Contributor

Paul,

Would you mind opening an issue in dotnet/roslyn asking that the error message be updated to include the link. That seems doable shortish term.

Kathleen

@KathleenDollard KathleenDollard removed the LDM Review in progress LDM Review is in progress label Jul 20, 2018
@AnthonyDGreen
Copy link
Contributor

I agree with @gafter and @KathleenDollard on elementwise = and <> operators. That's the simplest design and you get all of the VB semantics for free. Because = and <> includes all of the Option stuff, null vs empty strings, and user-defined vs intrinsic operators stuff, numeric conversions, nullable lifting, etc. By analogy, Select Case is defined in terms of language =, not .Equals. So when System.Type overloaded op_Equal in .NET 4.0 it just lit up in Select Case that you could write:

Select Case obj.GetType()
    Case GetType(Integer)
End Select

Likewise, if you just use element-wise recursive = for tuples this will just start working:

Select Case GetVelocity()
    Case (0, 0) ' Standing still

End Select

(Of course, you might want to optimize the codegen for that code pattern).

@jrmoreno1
Copy link

@ghost: I'd suggest changing the example to:

 Dim t1 As (Integer, Integer) = (1, 2)
 Dim t2 = t1
 Console.WriteLine(t1 = t2)

Because the fact that it can't compare two different types is irrelevant, if it can't compare the same types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LDM Considering LDM has reviewed and we think this has merit
Projects
None yet
Development

No branches or pull requests

8 participants