Skip to content

Commit 0644982

Browse files
GrahamTheCodergoogle-labs-jules[bot]
authored and
claude
committed
Fix VB -> C# char comparison with empty string
Map VB.NET char comparison with an empty string to char.MinValue to match runtime behavior and prevent logic errors during compilation. For `Option Compare Text`, map it appropriately using `Microsoft.VisualBasic.CompilerServices.Conversions.ToString` to prevent `char` being compared to an empty string. Added characterization tests. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
1 parent acb0756 commit 0644982

15 files changed

+208
-622
lines changed

‎CodeConverter/CSharp/BinaryExpressionConverter.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,9 @@ private async Task<CSharpSyntaxNode> ConvertBinaryExpressionAsync(VBasic.Syntax.
8787
}
8888
if (lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char) {
8989
// Do nothing, char comparison
90-
} else if ((lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Right)) ||
91-
(rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && lhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Left))) {
90+
} else if (!_visualBasicEqualityComparison.OptionCompareTextCaseInsensitive &&
91+
((lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Right)) ||
92+
(rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && lhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Left)))) {
9293
if (lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char) {
9394
rhs = CS.SyntaxFactory.MemberAccessExpression(CS.SyntaxKind.SimpleMemberAccessExpression, CS.SyntaxFactory.PredefinedType(CS.SyntaxFactory.Token(CS.SyntaxKind.CharKeyword)), CS.SyntaxFactory.IdentifierName("MinValue"));
9495
omitConversion = true;

‎CodeConverter/CSharp/BinaryExpressionConverter.cs.orig‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
using ICSharpCode.CodeConverter.Util.FromRoslyn;
2+
3+
namespace ICSharpCode.CodeConverter.CSharp;
4+
5+
internal class BinaryExpressionConverter
6+
{
7+
private readonly SemanticModel _semanticModel;
8+
private readonly IOperatorConverter _operatorConverter;
9+
private readonly VisualBasicEqualityComparison _visualBasicEqualityComparison;
10+
private readonly VisualBasicNullableExpressionsConverter _visualBasicNullableTypesConverter;
11+
public CommonConversions CommonConversions { get; }
12+
13+
public CommentConvertingVisitorWrapper TriviaConvertingExpressionVisitor { get; }
14+
15+
public BinaryExpressionConverter(SemanticModel semanticModel, IOperatorConverter operatorConverter, VisualBasicEqualityComparison visualBasicEqualityComparison,
16+
VisualBasicNullableExpressionsConverter visualBasicNullableTypesConverter, CommonConversions commonConversions)
17+
{
18+
CommonConversions = commonConversions;
19+
_semanticModel = semanticModel;
20+
_operatorConverter = operatorConverter;
21+
_visualBasicEqualityComparison = visualBasicEqualityComparison;
22+
_visualBasicNullableTypesConverter = visualBasicNullableTypesConverter;
23+
TriviaConvertingExpressionVisitor = commonConversions.TriviaConvertingExpressionVisitor;
24+
}
25+
26+
public async Task<CSharpSyntaxNode> ConvertBinaryExpressionAsync(VBSyntax.BinaryExpressionSyntax entryNode)
27+
{
28+
// Walk down the syntax tree for deeply nested binary expressions to avoid stack overflow
29+
// e.g. 3 + 4 + 5 + ...
30+
// Test "DeeplyNestedBinaryExpressionShouldNotStackOverflowAsync()" skipped because it's too slow
31+
32+
CSSyntax.ExpressionSyntax csLhs = null;
33+
int levelsToConvert = 0;
34+
VBSyntax.BinaryExpressionSyntax currentNode = entryNode;
35+
36+
// Walk down the nested levels to count them
37+
for (var nextNode = entryNode; nextNode != null; currentNode = nextNode, nextNode = currentNode.Left as VBSyntax.BinaryExpressionSyntax, levelsToConvert++) {
38+
// Don't go beyond a rewritten operator because that code has many paths that can call VisitBinaryExpression. Passing csLhs through all of that would harm the code quality more than it's worth to help that edge case.
39+
if (await RewriteBinaryOperatorOrNullAsync(nextNode) is { } operatorNode) {
40+
csLhs = operatorNode;
41+
break;
42+
}
43+
}
44+
45+
// Walk back up the same levels converting as we go.
46+
for (; levelsToConvert > 0; currentNode = currentNode!.Parent as VBSyntax.BinaryExpressionSyntax, levelsToConvert--) {
47+
csLhs = (CSSyntax.ExpressionSyntax)await ConvertBinaryExpressionAsync(currentNode, csLhs);
48+
}
49+
50+
return csLhs;
51+
}
52+
53+
private async Task<CSharpSyntaxNode> ConvertBinaryExpressionAsync(VBasic.Syntax.BinaryExpressionSyntax node, CSSyntax.ExpressionSyntax lhs, CSSyntax.ExpressionSyntax rhs = null)
54+
{
55+
lhs ??= await node.Left.AcceptAsync<CSSyntax.ExpressionSyntax>(TriviaConvertingExpressionVisitor);
56+
rhs ??= await node.Right.AcceptAsync<CSSyntax.ExpressionSyntax>(TriviaConvertingExpressionVisitor);
57+
58+
var lhsTypeInfo = _semanticModel.GetTypeInfo(node.Left);
59+
var rhsTypeInfo = _semanticModel.GetTypeInfo(node.Right);
60+
61+
ITypeSymbol forceLhsTargetType = null;
62+
bool omitRightConversion = false;
63+
bool omitConversion = false;
64+
if (lhsTypeInfo.Type != null && rhsTypeInfo.Type != null)
65+
{
66+
if (node.IsKind(VBasic.SyntaxKind.ConcatenateExpression) &&
67+
!lhsTypeInfo.Type.IsEnumType() && !rhsTypeInfo.Type.IsEnumType() &&
68+
!lhsTypeInfo.Type.IsDateType() && !rhsTypeInfo.Type.IsDateType())
69+
{
70+
omitRightConversion = true;
71+
omitConversion = lhsTypeInfo.Type.SpecialType == SpecialType.System_String ||
72+
rhsTypeInfo.Type.SpecialType == SpecialType.System_String;
73+
if (lhsTypeInfo.ConvertedType.SpecialType != SpecialType.System_String) {
74+
forceLhsTargetType = CommonConversions.KnownTypes.String;
75+
}
76+
}
77+
}
78+
79+
var objectEqualityType = _visualBasicEqualityComparison.GetObjectEqualityType(node, lhsTypeInfo, rhsTypeInfo);
80+
81+
switch (objectEqualityType) {
82+
case VisualBasicEqualityComparison.RequiredType.StringOnly:
83+
if (lhsTypeInfo.ConvertedType?.SpecialType == SpecialType.System_String &&
84+
rhsTypeInfo.ConvertedType?.SpecialType == SpecialType.System_String &&
85+
_visualBasicEqualityComparison.TryConvertToNullOrEmptyCheck(node, lhs, rhs, lhsTypeInfo, rhsTypeInfo, out CSharpSyntaxNode visitBinaryExpression)) {
86+
return visitBinaryExpression;
87+
}
88+
if (lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char) {
89+
// Do nothing, char comparison
90+
} else if ((lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Right)) ||
91+
(rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && lhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Left))) {
92+
if (lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char) {
93+
rhs = CS.SyntaxFactory.MemberAccessExpression(CS.SyntaxKind.SimpleMemberAccessExpression, CS.SyntaxFactory.PredefinedType(CS.SyntaxFactory.Token(CS.SyntaxKind.CharKeyword)), CS.SyntaxFactory.IdentifierName("MinValue"));
94+
omitConversion = true;
95+
} else {
96+
lhs = CS.SyntaxFactory.MemberAccessExpression(CS.SyntaxKind.SimpleMemberAccessExpression, CS.SyntaxFactory.PredefinedType(CS.SyntaxFactory.Token(CS.SyntaxKind.CharKeyword)), CS.SyntaxFactory.IdentifierName("MinValue"));
97+
omitConversion = true;
98+
}
99+
} else {
100+
(lhs, rhs) = _visualBasicEqualityComparison.AdjustForVbStringComparison(node.Left, lhs, lhsTypeInfo, false, node.Right, rhs, rhsTypeInfo, false);
101+
omitConversion = true; // Already handled within for the appropriate types (rhs can become int in comparison)
102+
}
103+
break;
104+
case VisualBasicEqualityComparison.RequiredType.Object:
105+
return _visualBasicEqualityComparison.GetFullExpressionForVbObjectComparison(lhs, rhs, VisualBasicEqualityComparison.ComparisonKind.Equals, node.IsKind(VBasic.SyntaxKind.NotEqualsExpression));
106+
}
107+
108+
var lhsTypeIgnoringNullable = lhsTypeInfo.Type.GetNullableUnderlyingType() ?? lhsTypeInfo.Type;
109+
var rhsTypeIgnoringNullable = rhsTypeInfo.Type.GetNullableUnderlyingType() ?? rhsTypeInfo.Type;
110+
omitConversion |= lhsTypeIgnoringNullable != null && rhsTypeIgnoringNullable != null &&
111+
lhsTypeIgnoringNullable.IsEnumType() && SymbolEqualityComparer.Default.Equals(lhsTypeIgnoringNullable, rhsTypeIgnoringNullable)
112+
&& !node.IsKind(VBasic.SyntaxKind.AddExpression, VBasic.SyntaxKind.SubtractExpression, VBasic.SyntaxKind.MultiplyExpression, VBasic.SyntaxKind.DivideExpression, VBasic.SyntaxKind.IntegerDivideExpression, VBasic.SyntaxKind.ModuloExpression)
113+
&& forceLhsTargetType == null;
114+
lhs = omitConversion ? lhs : CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Left, lhs, forceTargetType: forceLhsTargetType);
115+
rhs = omitConversion || omitRightConversion ? rhs : CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Right, rhs);
116+
117+
var kind = VBasic.VisualBasicExtensions.Kind(node).ConvertToken();
118+
var op = CS.SyntaxFactory.Token(CSharpUtil.GetExpressionOperatorTokenKind(kind));
119+
120+
var csBinExp = CS.SyntaxFactory.BinaryExpression(kind, lhs, op, rhs);
121+
var exp = _visualBasicNullableTypesConverter.WithBinaryExpressionLogicForNullableTypes(node, lhsTypeInfo, rhsTypeInfo, csBinExp, lhs, rhs);
122+
return node.Parent.IsKind(VBasic.SyntaxKind.SimpleArgument) ? exp : exp.AddParens();
123+
}
124+
125+
private async Task<CSSyntax.ExpressionSyntax> RewriteBinaryOperatorOrNullAsync(VBSyntax.BinaryExpressionSyntax node) =>
126+
await _operatorConverter.ConvertRewrittenBinaryOperatorOrNullAsync(node, TriviaConvertingExpressionVisitor.IsWithinQuery);
127+
}

‎CodeConverter/CSharp/BinaryExpressionConverter.cs.rej‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
--- BinaryExpressionConverter.cs
2+
+++ BinaryExpressionConverter.cs
3+
@@ -87,8 +87,9 @@
4+
}
5+
if (lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char) {
6+
// Do nothing, char comparison
7+
- } else if ((lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Right)) ||
8+
- (rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && lhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Left))) {
9+
+ } else if (!_visualBasicEqualityComparison.OptionCompareTextCaseInsensitive &&
10+
+ ((lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && rhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Right)) ||
11+
+ (rhsTypeInfo.Type?.SpecialType == SpecialType.System_Char && lhsTypeInfo.Type?.SpecialType == SpecialType.System_String && _visualBasicEqualityComparison.IsNothingOrEmpty(node.Left)))) {
12+
if (lhsTypeInfo.Type?.SpecialType == SpecialType.System_Char) {
13+
rhs = CS.SyntaxFactory.MemberAccessExpression(CS.SyntaxKind.SimpleMemberAccessExpression, CS.SyntaxFactory.PredefinedType(CS.SyntaxFactory.Token(CS.SyntaxKind.CharKeyword)), CS.SyntaxFactory.IdentifierName("MinValue"));
14+
omitConversion = true;

‎Program.vb‎

Lines changed: 0 additions & 9 deletions
This file was deleted.

‎TestDotNet/Program.vb‎

Lines changed: 0 additions & 12 deletions
This file was deleted.

TestDotNet/TestDotNet.vbproj renamed to TestDotNet/TestComparison.vbproj

File renamed without changes.

‎TestDotNet/TestComparisonCS.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
using System;
2+
using Microsoft.VisualBasic.CompilerServices;
3+
using System.Globalization;
4+
5+
class Program
6+
{
7+
static void Main()
8+
{
9+
char testChar = default;
10+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare(Conversions.ToString(testChar), "", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
11+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare(Conversions.ToString(testChar), "a", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
12+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare("a", Conversions.ToString(testChar), CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
13+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare("ab", Conversions.ToString(testChar), CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
14+
15+
char c = 'a';
16+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare(Conversions.ToString(c), "a", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
17+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare(Conversions.ToString(c), "ab", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
18+
19+
string s = "";
20+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare(Conversions.ToString(c), s, CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
21+
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.Compare(Conversions.ToString(testChar), s, CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0);
22+
}
23+
}

TestEquality.csproj renamed to TestDotNet/TestComparisonCS.csproj

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
<Project Sdk="Microsoft.NET.Sdk">
2-
32
<PropertyGroup>
43
<OutputType>Exe</OutputType>
5-
<TargetFramework>net6.0</TargetFramework>
4+
<TargetFramework>net8.0</TargetFramework>
65
</PropertyGroup>
7-
86
</Project>

‎TestDotNet/TestComparisonVB.vb‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
Option Compare Text
2+
Imports System
3+
4+
Module Program
5+
Sub Main()
6+
Dim testChar As Char = Nothing
7+
Console.WriteLine(testChar = "")
8+
Console.WriteLine(testChar = "a")
9+
Console.WriteLine("a" = testChar)
10+
Console.WriteLine("ab" = testChar)
11+
12+
Dim c As Char = "a"c
13+
Console.WriteLine(c = "a")
14+
Console.WriteLine(c = "ab")
15+
16+
Dim s As String = ""
17+
Console.WriteLine(c = s)
18+
Console.WriteLine(testChar = s)
19+
End Sub
20+
End Module

‎TestDotNet/TestComparisonVB.vbproj‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<OutputType>Exe</OutputType>
4+
<TargetFramework>net8.0</TargetFramework>
5+
</PropertyGroup>
6+
</Project>

0 commit comments

Comments
 (0)