C# -> VB: various fixes

[Alexilia]

No description provided.

[GrahamTheCoder]

This looks like a good candidate for a self verifying test

[GrahamTheCoder]

I'll add one in now - I'm a bit surprised, I thought to avoid all the visual basic magic, using the Is operator was recommended and would match CSharp more closely.

[GrahamTheCoder]

I've pushed the tests. Without your change on NodesVisitor.cs:1208 they pass, but with that change they fail.
Therefore I'm not convinced this is the right conversion. Could you take a look and either explain what problem you're fixing, or revert the change if it's wrong? Thanks!

[Alexilia]

Is operator compares two objects by reference. It does not perform value comparison.
= operator with two strings is compiled to IL using Microsoft.VisualBasic.Strings.Equals. So the comparison could be culture-aware. In VB null strings and empty strings are considered equal, and the way to compare a string with null is to use Is Nothing. In simple cases it can't be noticed because compiler use String.Intern.

[GrahamTheCoder]

Yep so I think we're agreed the runtime equivalent of CSharp "==" in VB is "Is".
If we're not, could you add a test case to the self verifying tests to show where they differ (to pass, the self verifying test must compile and pass in CSharp and after conversion to visual basic)

In terms of priorities for conversion, runtime equivalence is priority 1, idiomatic code in the target language is priority 3:
https://github.com/icsharpcode/CodeConverter/blob/master/.github/CONTRIBUTING.md#deciding-what-the-output-should-be
I. E. Code that works the same is key

Note: In CSharp you would use the Equals method to do invariant culture or case insensitive comparison.

[Alexilia]

Nop, in CSharp == is compiled to IL to [System.Runtime]System.String::op_Equality. It is more equivalent of VB =. These simple test passes in CS and not in VB with Is:

[Fact]
    public void NonInternStringsEqualsOperator() {
        string a = "10";
        string b = 10.ToString();
        Assert.True(a == b);
    }

Public Sub NonInternStringsEqualsOperator()
        Dim a As String = "10"
        Dim b As String = 10.ToString()
        Assert.[True](a Is b)
End Sub

but passes with =

Public Sub NonInternStringsEqualsOperator()
        Dim a As String = "10"
        Dim b As String = 10.ToString()
        Assert.[True](a = b)
End Sub

Empty string and null string in VB are the same because Nothing in VB is not null in CSharp, it's default value, for example 0 = Nothing for integers, so in any comparison null and empty string should be the same. The runtime functions also treat null like empty, so it should not be a problem.

[GrahamTheCoder]

Ah, my apologies for misunderstanding. I understand now - I had assumed "Is" called op_Equality, but in fact it calls reference equals. This means the implementation in master is flawed as your new interning related test (presumably) highlights.

But my previous tests you removed/changed show the implementation on this PR is also flawed. You'll need to restore those tests to find the correct solution to this problem. I think the solution is to call Equals(str1, str2) when both sides are known to be strings.

We could also do a test case for object == string that preserves the unintended reference equality comparison like this:
https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYCMBYAUBgBjwwGYACAbzzJrI3UutuYHsAjAKwFMBjYMgB5kAvGUwEAdABUWAZWBgAllADmACgCUAbibMaqcWQCeIsgCJxZnbj20DATjVDhoo9t1kAvnk9A

[GrahamTheCoder]

Sorry I took so long to get around to this. Looks really good, just a couple of minor suggestions.

[GrahamTheCoder]

Yep, I like this test, it shows that op_Equality is overridden and distinguishes from reference equality (which neither VB or C# does, but someone might guess from "==")