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.

Sign up for GitHub

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

C# -> VB: various fixes #533

Merged
merged 17 commits into from
Mar 5, 2020
Merged

C# -> VB: various fixes #533

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
30e5b3e
C# -> VB: convert PrefixUnaryExpression
Alexilia Feb 17, 2020
9e40c64
C# -> VB: rename members when they are conflicted with interface members
Alexilia Feb 18, 2020
71012fd
C# -> VB: continue rename subsequent members
Alexilia Feb 19, 2020
8dda51b
C# -> VB: improve equals expression for strings
Alexilia Feb 19, 2020
f27a51e
Merge branch 'work'
Alexilia Feb 25, 2020
376f3b9
CS -> VB: duplicating imports
Alexilia Feb 25, 2020
e1bda96
Merge remote-tracking branch 'upstream/master'
Alexilia Feb 25, 2020
c468d5d
Merge branch 'master' into alexilia-master
GrahamTheCoder Mar 2, 2020
c981596
Remove commented
GrahamTheCoder Mar 2, 2020
170de31
Fix post merge compile error
GrahamTheCoder Mar 2, 2020
3ad34f8
Add self verifying tests
GrahamTheCoder Mar 2, 2020
6fbafbb
Fix bug with arrow expression void methods
GrahamTheCoder Mar 2, 2020
16d84a8
update tests
Alexilia Mar 4, 2020
66008b6
Add back tests which show flaw in this PR's implementation
GrahamTheCoder Mar 5, 2020
f32c2b8
Reduce tests cases to focus on a single case - null literal
GrahamTheCoder Mar 5, 2020
64de292
Use Equals and recharacterize tests
GrahamTheCoder Mar 5, 2020
5a84966
Remove redundant files
GrahamTheCoder Mar 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions ICSharpCode.CodeConverter/VB/CaseConflictResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,20 @@ public static IEnumerable<IEnumerable<ISymbol>> GetLocalSymbolSets(INamespaceOrT
}

private static IEnumerable<(ISymbol Original, string NewName)> GetUniqueNamesForSymbolSet(IEnumerable<ISymbol> symbols) {
var membersByCaseInsensitiveName = symbols.ToLookup(m => m.Name, m => m, StringComparer.OrdinalIgnoreCase);
var membersByCaseInsensitiveName = symbols.ToLookup(m => GetName(m), m => m, StringComparer.OrdinalIgnoreCase);
var names = new HashSet<string>(membersByCaseInsensitiveName.Select(ms => ms.Key),
StringComparer.OrdinalIgnoreCase);
var symbolsWithNewNames = membersByCaseInsensitiveName.Where(ms => ms.Count() > 1)
.SelectMany(symbolGroup => GetSymbolsWithNewNames(symbolGroup.ToArray(), names));
return symbolsWithNewNames;
}
private static string GetName(ISymbol m) {
if (m.CanBeReferencedByName)
return m.Name;
if (m.ExplicitInterfaceImplementations().Any())
return m.Name.Split('.').Last();
return m.Name;
}

private static IEnumerable<(ISymbol Original, string NewName)> GetSymbolsWithNewNames(IReadOnlyCollection<ISymbol> symbolGroup, HashSet<string> names)
{
Expand All @@ -68,7 +75,7 @@ private static (ISymbol Original, string NewName)[] GetMethodSymbolsWithNewNames
bool specialSymbolUsingName)
{
var methodsByCaseInsensitiveSignature = methodSymbols
.ToLookup(m => (m.Name.ToLowerInvariant(), m.GetParameterSignature()))
.ToLookup(m => (GetName(m).ToLowerInvariant(), m.GetParameterSignature()))
.Where(g => g.Count() > 1)
.SelectMany(clashingMethodGroup =>
{
Expand All @@ -90,7 +97,7 @@ private static (ISymbol Original, string NewName)[] GetMethodSymbolsWithNewNames
private static IEnumerable<(ISymbol Original, string NewName)> GetSymbolsWithNewNames(
IEnumerable<ISymbol> toRename, Func<string, bool> canUse, bool canKeepOne)
{
var symbolsWithNewNames = toRename.OrderByDescending(x => x.DeclaredAccessibility).ThenByDescending(x => x.Kind == SymbolKind.Parameter).Skip(canKeepOne ? 1 :0).Select(tr =>
var symbolsWithNewNames = toRename.OrderByDescending(x => x.DeclaredAccessibility).ThenByDescending(x => x.Kind == SymbolKind.Parameter || x.Kind == SymbolKind.Property).Skip(canKeepOne ? 1 :0).Select(tr =>
{
string newName = NameGenerator.GenerateUniqueName(GetBaseName(tr), canUse);
return (Original: tr, NewName: newName);
Expand All @@ -105,8 +112,8 @@ private static async Task<Project> PerformRenames(Project project, IReadOnlyColl
project = solution.GetProject(project.Id);
var compilation = await project.GetCompilationAsync();
ISymbol currentDeclaration = SymbolFinder.FindSimilarSymbols(originalSymbol, compilation).FirstOrDefault();
if (currentDeclaration == null) break; //Must have already renamed this symbol for a different reason

if (currentDeclaration == null)
continue; //Must have already renamed this symbol for a different reason
GrahamTheCoder marked this conversation as resolved.
Show resolved Hide resolved
solution = await Renamer.RenameSymbolAsync(solution, currentDeclaration, newName, solution.Workspace.Options);
}

Expand All @@ -116,7 +123,8 @@ private static async Task<Project> PerformRenames(Project project, IReadOnlyColl
private static string GetBaseName(ISymbol declaration)
{
string prefix = declaration.Kind.ToString().ToLowerInvariant()[0] + "_";
return prefix + declaration.Name.Substring(0, 1).ToUpperInvariant() + declaration.Name.Substring(1);
string name = GetName(declaration);
return prefix + name.Substring(0, 1).ToUpperInvariant() + name.Substring(1);
}
}
}
17 changes: 8 additions & 9 deletions ICSharpCode.CodeConverter/VB/NodesVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,13 +911,13 @@ public override VisualBasicSyntaxNode VisitParenthesizedExpression(CSS.Parenthes
public override VisualBasicSyntaxNode VisitPrefixUnaryExpression(CSS.PrefixUnaryExpressionSyntax node)
{
var kind = CS.CSharpExtensions.Kind(node).ConvertToken(TokenContext.Local);
if (IsReturnValueDiscarded(node)) {
return SyntaxFactory.AssignmentStatement(
kind,
(ExpressionSyntax)node.Operand.Accept(TriviaConvertingVisitor),
SyntaxFactory.Token(VBUtil.GetExpressionOperatorTokenKind(kind)), _commonConversions.Literal(1)
);
}
//if (IsReturnValueDiscarded(node)) {
// return SyntaxFactory.AssignmentStatement(
// kind,
// (ExpressionSyntax)node.Operand.Accept(TriviaConvertingVisitor),
// SyntaxFactory.Token(VBUtil.GetExpressionOperatorTokenKind(kind)), _commonConversions.Literal(1)
// );
//}
GrahamTheCoder marked this conversation as resolved.
Show resolved Hide resolved
if (kind == SyntaxKind.AddAssignmentStatement || kind == SyntaxKind.SubtractAssignmentStatement) {
string operatorName;
if (kind == SyntaxKind.AddAssignmentStatement)
Expand Down Expand Up @@ -1230,8 +1230,7 @@ public override VisualBasicSyntaxNode VisitBinaryExpression(CSS.BinaryExpression

var isReferenceComparison = node.Left.IsKind(CS.SyntaxKind.NullLiteralExpression) ||
node.Right.IsKind(CS.SyntaxKind.NullLiteralExpression) ||
leftType.IsReferenceType ||
rightType.IsReferenceType;
leftType.IsReferenceType && rightType.IsReferenceType && (leftType.SpecialType != SpecialType.System_String || rightType.SpecialType != SpecialType.System_String);

if (SyntaxTokenExtensions.IsKind(node.OperatorToken, CS.SyntaxKind.EqualsEqualsToken) && isReferenceComparison) {
return SyntaxFactory.IsExpression(vbLeft, vbRight);
Expand Down
75 changes: 72 additions & 3 deletions Tests/VB/ExpressionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void TestMethod(string str)
}
}", @"Friend Class TestClass
Private Sub TestMethod(ByVal str As String)
Dim result As Boolean = If(str Is """", True, False)
Dim result As Boolean = If(str = """", True, False)
End Sub
End Class");
}
Expand Down Expand Up @@ -171,7 +171,7 @@ void TestMethod(string str)
}
}", @"Friend Class TestClass
Private Sub TestMethod(ByVal str As String)
Dim result As Boolean = If(str Is """", CSharpImpl.__Throw(Of Boolean)(New Exception(""empty"")), False)
Dim result As Boolean = If(str = """", CSharpImpl.__Throw(Of Boolean)(New Exception(""empty"")), False)
End Sub

Private Class CSharpImpl
Expand Down Expand Up @@ -828,7 +828,7 @@ Inherits ObjectModel.ObservableCollection(Of String)

Public Sub New()
AddHandler PropertyChanged, Sub(o, e)
If e.PropertyName Is ""AnyProperty"" Then
If e.PropertyName = ""AnyProperty"" Then
Add(""changed"")
Else
RemoveAt(0)
Expand Down Expand Up @@ -866,6 +866,75 @@ End Function
Public Sub New()
Dim str As String = create(Me)
End Sub
End Class");
}
[Fact]
public async Task PrefixUnaryExpression_SingleLineFunction() {
await TestConversionCSharpToVisualBasic(
@"public class TestClass {
public TestClass() {
System.Func<string, bool> func = o => !string.IsNullOrEmpty(""test"");
}
}",
@"Public Class TestClass
Public Sub New()
Dim func As Func(Of String, Boolean) = Function(o) Not String.IsNullOrEmpty(""test"")
End Sub
End Class");
}
[Fact]
public async Task EqualsExpression() {
await TestConversionCSharpToVisualBasic(
@"public class TestClass {
public TestClass() {
int i = 0;
int j = 0;
string s1 = ""string1"";
string s2 = ""string2"";
object object1 = s1;
object object2 = s2;
if(i == j)
DoSomething();
if(i == s2)
DoSomething();
if(i == object1)
DoSomething();
if(s1 == j)
DoSomething();
if(s1 == s2)
DoSomething();
if(s1 == object2)
DoSomething();
if(object1 == j)
DoSomething();
if(object1 == s2)
DoSomething();
if(object1 == object2)
DoSomething();
}
public void DoSomething() { }
}",
@"Public Class TestClass
Public Sub New()
Dim i As Integer = 0
Dim j As Integer = 0
Dim s1 As String = ""string1""
Dim s2 As String = ""string2""
Dim object1 As Object = s1
Dim object2 As Object = s2
If i = j Then DoSomething()
If i = s2 Then DoSomething()
If i = object1 Then DoSomething()
If s1 = j Then DoSomething()
If s1 = s2 Then DoSomething()
If s1 Is object2 Then DoSomething()
If object1 = j Then DoSomething()
If object1 Is s2 Then DoSomething()
If object1 Is object2 Then DoSomething()
End Sub

Public Sub DoSomething()
End Sub
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good candidate for a self verifying test

Copy link
Member

@GrahamTheCoder GrahamTheCoder Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@GrahamTheCoder GrahamTheCoder Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@GrahamTheCoder GrahamTheCoder Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

End Class");
}
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/VB/MemberTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,7 @@ public override bool Equals(object obj) {
Public Property Email As String

Protected Overloads Function Equals(ByVal other As MailEmployee) As Boolean
Return Email Is other.Email
Return Email = other.Email
End Function

Public Overrides Function Equals(ByVal obj As Object) As Boolean
Expand Down
33 changes: 33 additions & 0 deletions Tests/VB/SpecialConversionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,39 @@ End RemoveHandler
f_Value?(sender, e)
End RaiseEvent
End Event
End Class");
}
[Fact]
public async Task CaseConflict_FieldAndInterfaceProperty() {
await TestConversionCSharpToVisualBasic(
@"using System;
public interface IInterface {
int Prop { get; set; }
}
public class TestClass : IInterface {
int prop;
int IInterface.Prop {
get { return prop; }
set { prop = value;}
}
}",
@"Public Interface IInterface
Property Prop As Integer
End Interface

Public Class TestClass
Implements IInterface

Private f_Prop As Integer

Private Property Prop As Integer Implements IInterface.Prop
Get
Return f_Prop
End Get
Set(ByVal value As Integer)
f_Prop = value
End Set
End Property
End Class");
}
[Fact]
Expand Down