From 7462e7c8bd0701b0884c76bc9e50a1324c382f1b Mon Sep 17 00:00:00 2001 From: Robert Clipsham Date: Fri, 3 May 2019 16:04:49 +0100 Subject: [PATCH 1/2] Implement correct string comparisons for VB Closes #308 --- .../CSharp/NodesVisitor.cs | 42 +++++++++++++++++-- .../VBToCS/ExpressionTests.vb | 7 ++++ Tests/CSharp/ExpressionTests.cs | 34 +++++++++++++-- Tests/CSharp/MemberTests.cs | 2 +- Tests/CSharp/StatementTests.cs | 8 ++-- 5 files changed, 83 insertions(+), 10 deletions(-) diff --git a/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs b/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs index 065ea3e78..fa288cf7e 100644 --- a/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs +++ b/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs @@ -37,6 +37,7 @@ class NodesVisitor : VBasic.VisualBasicSyntaxVisitor private static readonly Type ExtensionAttributeType = typeof(ExtensionAttribute); private readonly TypeConversionAnalyzer _typeConversionAnalyzer; public CommentConvertingNodesVisitor TriviaConvertingVisitor { get; } + private bool _optionCompareText = false; private CommonConversions CommonConversions { get; } @@ -118,6 +119,9 @@ public override CSharpSyntaxNode VisitCompilationUnit(VBSyntax.CompilationUnitSy .GroupBy(u => u.ToString()) .Select(g => g.First()); + _optionCompareText = node.Options.Any(x => x.NameKeyword.ValueText.Equals("Compare", StringComparison.OrdinalIgnoreCase) && + x.ValueKeyword.ValueText.Equals("Text", StringComparison.OrdinalIgnoreCase)); + return SyntaxFactory.CompilationUnit( SyntaxFactory.List(), SyntaxFactory.List(usingDirectiveSyntax), @@ -1674,10 +1678,11 @@ public override CSharpSyntaxNode VisitBinaryExpression(VBSyntax.BinaryExpression var lhs = _typeConversionAnalyzer.AddExplicitConversion(node.Left, (ExpressionSyntax)node.Left.Accept(TriviaConvertingVisitor)); var rhs = _typeConversionAnalyzer.AddExplicitConversion(node.Right, (ExpressionSyntax)node.Right.Accept(TriviaConvertingVisitor)); + var stringType = _semanticModel.Compilation.GetTypeByMetadataName("System.String"); + var lhsTypeInfo = _semanticModel.GetTypeInfo(node.Left); + var rhsTypeInfo = _semanticModel.GetTypeInfo(node.Right); + if (node.IsKind(VBasic.SyntaxKind.ConcatenateExpression)) { - var stringType = _semanticModel.Compilation.GetTypeByMetadataName("System.String"); - var lhsTypeInfo = _semanticModel.GetTypeInfo(node.Left); - var rhsTypeInfo = _semanticModel.GetTypeInfo(node.Right); if (lhsTypeInfo.Type.SpecialType != SpecialType.System_String && lhsTypeInfo.ConvertedType.SpecialType != SpecialType.System_String && rhsTypeInfo.Type.SpecialType != SpecialType.System_String && @@ -1686,6 +1691,36 @@ public override CSharpSyntaxNode VisitBinaryExpression(VBSyntax.BinaryExpression } } + if (node.IsKind(VBasic.SyntaxKind.EqualsExpression, VBasic.SyntaxKind.NotEqualsExpression)) { + if (lhsTypeInfo.ConvertedType?.SpecialType == SpecialType.System_String && + rhsTypeInfo.ConvertedType?.SpecialType == SpecialType.System_String) { + bool lhsEmpty = lhs is LiteralExpressionSyntax les && + (les.IsKind(SyntaxKind.NullLiteralExpression) || + (les.IsKind(SyntaxKind.StringLiteralExpression) && string.IsNullOrEmpty(les.Token.ValueText))); + bool rhsEmpty = rhs is LiteralExpressionSyntax res && + (res.IsKind(SyntaxKind.NullLiteralExpression) || + (res.IsKind(SyntaxKind.StringLiteralExpression) && string.IsNullOrEmpty(res.Token.ValueText))); + + if (lhsEmpty || rhsEmpty) { + var arg = lhsEmpty ? rhs : lhs; + var nullOrEmpty = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, SyntaxFactory.IdentifierName("string"), SyntaxFactory.IdentifierName("IsNullOrEmpty")), + SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(new[] { SyntaxFactory.Argument(arg) }))); + return node.IsKind(VBasic.SyntaxKind.EqualsExpression) ? (CSharpSyntaxNode)nullOrEmpty : SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, nullOrEmpty); + } else { + _extraUsingDirectives.Add("Microsoft.VisualBasic.CompilerServices"); + var textCompare = SyntaxFactory.Argument(SyntaxFactory.NameColon("TextCompare"), + default(SyntaxToken), + _optionCompareText ? SyntaxFactory.LiteralExpression(SyntaxKind.TrueLiteralExpression) : SyntaxFactory.LiteralExpression(SyntaxKind.FalseLiteralExpression)); + var compareString = SyntaxFactory.InvocationExpression( + SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, SyntaxFactory.IdentifierName("Operators"), SyntaxFactory.IdentifierName("CompareString")), + SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(new[] { SyntaxFactory.Argument(lhs), SyntaxFactory.Argument(rhs), textCompare }))); + lhs = compareString; + rhs = SyntaxFactory.LiteralExpression(SyntaxKind.NumericLiteralExpression, SyntaxFactory.Literal(0)); + } + } + } + if (node.IsKind(VBasic.SyntaxKind.ExponentiateExpression, VBasic.SyntaxKind.ExponentiateAssignmentStatement)) { return SyntaxFactory.InvocationExpression( @@ -1693,6 +1728,7 @@ public override CSharpSyntaxNode VisitBinaryExpression(VBSyntax.BinaryExpression ExpressionSyntaxExtensions.CreateArgList(lhs, rhs)); } + var kind = VBasic.VisualBasicExtensions.Kind(node).ConvertToken(TokenContext.Local); var op = SyntaxFactory.Token(CSharpUtil.GetExpressionOperatorTokenKind(kind)); diff --git a/TestData/SelfVerifyingTests/VBToCS/ExpressionTests.vb b/TestData/SelfVerifyingTests/VBToCS/ExpressionTests.vb index c10a2a320..b6f117216 100644 --- a/TestData/SelfVerifyingTests/VBToCS/ExpressionTests.vb +++ b/TestData/SelfVerifyingTests/VBToCS/ExpressionTests.vb @@ -24,6 +24,13 @@ Module Program Assert.Equal(x, 3.5D) End Sub + + Public Sub TestStringComparison() + Dim s1 As String = Nothing + Dim s2 As String = "" + Assert.True(s1 = s2) + End Sub + Sub TestCat() Dim vBoolean As Boolean = Nothing diff --git a/Tests/CSharp/ExpressionTests.cs b/Tests/CSharp/ExpressionTests.cs index 0ea884114..75b5aa125 100644 --- a/Tests/CSharp/ExpressionTests.cs +++ b/Tests/CSharp/ExpressionTests.cs @@ -642,6 +642,34 @@ private void TestMethod() }"); } + [Fact] + public void StringCompare() + { + TestConversionVisualBasicToCSharp(@"Public Class Class1 + Sub Foo() + Dim s1 As String = Nothing + Dim s2 As String = """" + If s1 <> s2 Then + Throw New Exception() + End If + End Sub +End Class", @"using System; +using Microsoft.VisualBasic.CompilerServices; + +public class Class1 +{ + public void Foo() + { + string s1 = null; + string s2 = """"; + if (Operators.CompareString(s1, s2, TextCompare: false) != 0) + throw new Exception(); + } +}"); + } + + + [Fact] public void StringConcatPrecedence() { @@ -912,7 +940,7 @@ End Sub { private void TestMethod(string str) { - bool result = (str == """") ? true : false; + bool result = (string.IsNullOrEmpty(str)) ? true : false; } }"); } @@ -928,7 +956,7 @@ End Sub { private void TestMethod(string str) { - bool result = !((str == """") ? true : false); + bool result = !((string.IsNullOrEmpty(str)) ? true : false); } }"); } @@ -944,7 +972,7 @@ End Sub { private void TestMethod(string str) { - int result = 5 - ((str == """") ? 1 : 2); + int result = 5 - ((string.IsNullOrEmpty(str)) ? 1 : 2); } }"); } diff --git a/Tests/CSharp/MemberTests.cs b/Tests/CSharp/MemberTests.cs index f6009fc64..d7bb5b17d 100644 --- a/Tests/CSharp/MemberTests.cs +++ b/Tests/CSharp/MemberTests.cs @@ -175,7 +175,7 @@ public string Y { set { - if (value != """") + if (!string.IsNullOrEmpty(value)) Y = """"; else _y = """"; diff --git a/Tests/CSharp/StatementTests.cs b/Tests/CSharp/StatementTests.cs index d18c56d5d..7154f5517 100644 --- a/Tests/CSharp/StatementTests.cs +++ b/Tests/CSharp/StatementTests.cs @@ -732,19 +732,21 @@ End If Next Return -1 End Function -End Class", @"class TestClass +End Class", @"using Microsoft.VisualBasic.CompilerServices; + +class TestClass { public static int FindTextInCol(string w, int pTitleRow, int startCol, string needle) { var loopTo = w.Length; for (int c = startCol; c <= loopTo; c++) { - if (needle == """") + if (string.IsNullOrEmpty(needle)) { if (string.IsNullOrWhiteSpace(w[c].ToString())) return c; } - else if (w[c].ToString() == needle) + else if (Operators.CompareString(w[c].ToString(), needle, TextCompare: false) == 0) return c; } return -1; From d108f3074997e5df64c4d6e186743e754eaded02 Mon Sep 17 00:00:00 2001 From: Robert Clipsham Date: Mon, 6 May 2019 11:05:26 +0100 Subject: [PATCH 2/2] Cleaner comparison handling for string literals --- .../CSharp/NodesVisitor.cs | 10 ++- Tests/CSharp/ExpressionTests.cs | 71 ++++++++++++++++++- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs b/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs index fa288cf7e..f9c8cde8c 100644 --- a/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs +++ b/ICSharpCode.CodeConverter/CSharp/NodesVisitor.cs @@ -107,6 +107,9 @@ public override CSharpSyntaxNode VisitCompilationUnit(VBSyntax.CompilationUnitSy var options = (VBasic.VisualBasicCompilationOptions)_semanticModel.Compilation.Options; var importsClauses = options.GlobalImports.Select(gi => gi.Clause).Concat(node.Imports.SelectMany(imp => imp.ImportsClauses)).ToList(); + _optionCompareText = node.Options.Any(x => x.NameKeyword.ValueText.Equals("Compare", StringComparison.OrdinalIgnoreCase) && + x.ValueKeyword.ValueText.Equals("Text", StringComparison.OrdinalIgnoreCase)); + var attributes = SyntaxFactory.List(node.Attributes.SelectMany(a => a.AttributeLists).SelectMany(ConvertAttribute)); var sourceAndConverted = node.Members.Select(m => (Source: m, Converted: ConvertMember(m))).ToReadOnlyCollection(); var convertedMembers = string.IsNullOrEmpty(options.RootNamespace) @@ -119,9 +122,6 @@ public override CSharpSyntaxNode VisitCompilationUnit(VBSyntax.CompilationUnitSy .GroupBy(u => u.ToString()) .Select(g => g.First()); - _optionCompareText = node.Options.Any(x => x.NameKeyword.ValueText.Equals("Compare", StringComparison.OrdinalIgnoreCase) && - x.ValueKeyword.ValueText.Equals("Text", StringComparison.OrdinalIgnoreCase)); - return SyntaxFactory.CompilationUnit( SyntaxFactory.List(), SyntaxFactory.List(usingDirectiveSyntax), @@ -1697,9 +1697,11 @@ public override CSharpSyntaxNode VisitBinaryExpression(VBSyntax.BinaryExpression bool lhsEmpty = lhs is LiteralExpressionSyntax les && (les.IsKind(SyntaxKind.NullLiteralExpression) || (les.IsKind(SyntaxKind.StringLiteralExpression) && string.IsNullOrEmpty(les.Token.ValueText))); + bool lhsLiteral = lhs is LiteralExpressionSyntax && lhs.IsKind(SyntaxKind.StringLiteralExpression); bool rhsEmpty = rhs is LiteralExpressionSyntax res && (res.IsKind(SyntaxKind.NullLiteralExpression) || (res.IsKind(SyntaxKind.StringLiteralExpression) && string.IsNullOrEmpty(res.Token.ValueText))); + bool rhsLiteral = rhs is LiteralExpressionSyntax && rhs.IsKind(SyntaxKind.StringLiteralExpression); if (lhsEmpty || rhsEmpty) { var arg = lhsEmpty ? rhs : lhs; @@ -1707,6 +1709,8 @@ public override CSharpSyntaxNode VisitBinaryExpression(VBSyntax.BinaryExpression SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, SyntaxFactory.IdentifierName("string"), SyntaxFactory.IdentifierName("IsNullOrEmpty")), SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(new[] { SyntaxFactory.Argument(arg) }))); return node.IsKind(VBasic.SyntaxKind.EqualsExpression) ? (CSharpSyntaxNode)nullOrEmpty : SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, nullOrEmpty); + } else if (!_optionCompareText && (lhsLiteral || rhsLiteral)) { + // If either side is a literal, and we're in binary comparison mode, we can use normal comparison logic } else { _extraUsingDirectives.Add("Microsoft.VisualBasic.CompilerServices"); var textCompare = SyntaxFactory.Argument(SyntaxFactory.NameColon("TextCompare"), diff --git a/Tests/CSharp/ExpressionTests.cs b/Tests/CSharp/ExpressionTests.cs index 75b5aa125..8a1619489 100644 --- a/Tests/CSharp/ExpressionTests.cs +++ b/Tests/CSharp/ExpressionTests.cs @@ -645,13 +645,25 @@ private void TestMethod() [Fact] public void StringCompare() { - TestConversionVisualBasicToCSharp(@"Public Class Class1 + TestConversionVisualBasicToCSharpWithoutComments(@"Public Class Class1 Sub Foo() Dim s1 As String = Nothing Dim s2 As String = """" If s1 <> s2 Then Throw New Exception() End If + If s1 = ""something"" Then + Throw New Exception() + End If + If ""something"" = s1 Then + Throw New Exception() + End If + If s1 = Nothing Then + ' + End If + If s1 = """" Then + ' + End If End Sub End Class", @"using System; using Microsoft.VisualBasic.CompilerServices; @@ -664,11 +676,68 @@ public void Foo() string s2 = """"; if (Operators.CompareString(s1, s2, TextCompare: false) != 0) throw new Exception(); + if (s1 == ""something"") + throw new Exception(); + if (""something"" == s1) + throw new Exception(); + if (string.IsNullOrEmpty(s1)) + { + } + if (string.IsNullOrEmpty(s1)) + { + } } }"); } + [Fact] + public void StringCompareText() + { + TestConversionVisualBasicToCSharpWithoutComments(@"Option Compare Text +Public Class Class1 + Sub Foo() + Dim s1 As String = Nothing + Dim s2 As String = """" + If s1 <> s2 Then + Throw New Exception() + End If + If s1 = ""something"" Then + Throw New Exception() + End If + If ""something"" = s1 Then + Throw New Exception() + End If + If s1 = Nothing Then + ' + End If + If s1 = """" Then + ' + End If + End Sub +End Class", @"using System; +using Microsoft.VisualBasic.CompilerServices; +public class Class1 +{ + public void Foo() + { + string s1 = null; + string s2 = """"; + if (Operators.CompareString(s1, s2, TextCompare: true) != 0) + throw new Exception(); + if (Operators.CompareString(s1, ""something"", TextCompare: true) == 0) + throw new Exception(); + if (Operators.CompareString(""something"", s1, TextCompare: true) == 0) + throw new Exception(); + if (string.IsNullOrEmpty(s1)) + { + } + if (string.IsNullOrEmpty(s1)) + { + } + } +}"); + } [Fact] public void StringConcatPrecedence()