Skip to content

Commit 67c9246

Browse files
GrahamTheCoderGrahamTheCoder
committed
Try to minimize the number of null coalesces
1 parent b5def68 commit 67c9246

6 files changed

Lines changed: 39 additions & 27 deletions

File tree

‎CodeConverter/CSharp/ExpressionNodeVisitor.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -729,7 +729,7 @@ public override async Task<CSharpSyntaxNode> VisitBinaryExpression(VBasic.Syntax
729729
_visualBasicEqualityComparison.TryConvertToNullOrEmptyCheck(node, lhs, rhs, out CSharpSyntaxNode visitBinaryExpression)) {
730730
return visitBinaryExpression;
731731
}
732-
(lhs, rhs) = _visualBasicEqualityComparison.AdjustForVbStringComparison(node.Left, lhs, lhsTypeInfo, node.Right, rhs, rhsTypeInfo);
732+
(lhs, rhs) = _visualBasicEqualityComparison.AdjustForVbStringComparison(node.Left, lhs, lhsTypeInfo, false, node.Right, rhs, rhsTypeInfo, false);
733733
omitConversion = true; // Already handled within for the appropriate types (rhs can become int in comparison)
734734
break;
735735
case VisualBasicEqualityComparison.RequiredType.Object:

‎CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -642,12 +642,15 @@ public override async Task<SyntaxList<StatementSyntax>> VisitSelectBlock(VBSynta
642642
{
643643
var vbExpr = node.SelectStatement.Expression;
644644
var vbEquality = CommonConversions.VisualBasicEqualityComparison;
645-
var (csExpr, stmts, csExprWithSourceMapping) = await GetExpressionWithoutSideEffectsAsync(vbExpr, "switchExpr");
645+
var (csSwitchExpr, stmts, csExprToReuse) = await GetExpressionWithoutSideEffectsAsync(vbExpr, "switchExpr");
646646
var switchExprTypeInfo = _semanticModel.GetTypeInfo(vbExpr);
647647
var isStringComparison = switchExprTypeInfo.ConvertedType?.SpecialType == SpecialType.System_String;
648648
var caseInsensitiveStringComparison = vbEquality.OptionCompareTextCaseInsensitive &&
649649
isStringComparison;
650-
csExpr = vbEquality.VbCoerceToNonNullString(vbExpr, csExpr, switchExprTypeInfo);
650+
if (isStringComparison) {
651+
csSwitchExpr = vbEquality.VbCoerceToNonNullString(vbExpr, csSwitchExpr, switchExprTypeInfo);
652+
csExprToReuse = vbEquality.VbCoerceToNonNullString(vbExpr, csExprToReuse, switchExprTypeInfo);
653+
}
651654

652655
var usedConstantValues = new HashSet<object>();
653656
var sections = new List<SwitchSectionSyntax>();
@@ -666,7 +669,7 @@ public override async Task<SyntaxList<StatementSyntax>> VisitSelectBlock(VBSynta
666669

667670
// Pass both halves in case we can optimize away the check based on the switch expr
668671
if (isStringComparison && !caseInsensitiveStringComparison) {
669-
csCase = vbEquality.VbCoerceToNonNullString(vbExpr, csExpr, switchExprTypeInfo, s.Value, correctTypeExpressionSyntax.Expr, caseTypeInfo).rhs;
672+
csCase = vbEquality.VbCoerceToNonNullString(vbExpr, csExprToReuse, switchExprTypeInfo, true, s.Value, correctTypeExpressionSyntax.Expr, caseTypeInfo, false).rhs;
670673
}
671674
var caseSwitchLabelSyntax = !caseInsensitiveStringComparison && correctTypeExpressionSyntax.IsConst && notAlreadyUsed
672675
? (SwitchLabelSyntax)SyntaxFactory.CaseSwitchLabel(csCase)
@@ -677,11 +680,11 @@ public override async Task<SyntaxList<StatementSyntax>> VisitSelectBlock(VBSynta
677680
} else if (c is VBSyntax.RelationalCaseClauseSyntax relational) {
678681
var operatorKind = VBasic.VisualBasicExtensions.Kind(relational);
679682
var relationalValue = (ExpressionSyntax)await relational.Value.AcceptAsync(_expressionVisitor);
680-
var binaryExp = SyntaxFactory.BinaryExpression(operatorKind.ConvertToken(TokenContext.Local), csExpr, relationalValue);
683+
var binaryExp = SyntaxFactory.BinaryExpression(operatorKind.ConvertToken(TokenContext.Local), csExprToReuse, relationalValue);
681684
labels.Add(WrapInCasePatternSwitchLabelSyntax(node, relational.Value, binaryExp, caseInsensitiveStringComparison, treatAsBoolean: true));
682685
} else if (c is VBSyntax.RangeCaseClauseSyntax range) {
683-
var lowerBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, (ExpressionSyntax)await range.LowerBound.AcceptAsync(_expressionVisitor), csExpr);
684-
var upperBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, csExpr, (ExpressionSyntax)await range.UpperBound.AcceptAsync(_expressionVisitor));
686+
var lowerBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, (ExpressionSyntax)await range.LowerBound.AcceptAsync(_expressionVisitor), csExprToReuse);
687+
var upperBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, csExprToReuse, (ExpressionSyntax)await range.UpperBound.AcceptAsync(_expressionVisitor));
685688
var withinBounds = SyntaxFactory.BinaryExpression(SyntaxKind.LogicalAndExpression, lowerBoundCheck, upperBoundCheck);
686689
labels.Add(WrapInCasePatternSwitchLabelSyntax(node, range.LowerBound, withinBounds, caseInsensitiveStringComparison, treatAsBoolean: true));
687690
} else throw new NotSupportedException(c.Kind().ToString());
@@ -696,7 +699,7 @@ public override async Task<SyntaxList<StatementSyntax>> VisitSelectBlock(VBSynta
696699
sections.Add(SyntaxFactory.SwitchSection(SyntaxFactory.List(labels), list));
697700
}
698701

699-
var switchStatementSyntax = ValidSyntaxFactory.SwitchStatement(csExprWithSourceMapping, sections);
702+
var switchStatementSyntax = ValidSyntaxFactory.SwitchStatement(csSwitchExpr, sections);
700703
return stmts.Add(switchStatementSyntax);
701704
}
702705

@@ -751,8 +754,9 @@ private CasePatternSwitchLabelSyntax WrapInCasePatternSwitchLabelSyntax(VBSyntax
751754
ValidSyntaxFactory.VarType, SyntaxFactory.SingleVariableDesignation(varName));
752755
ExpressionSyntax csLeft = SyntaxFactory.IdentifierName(varName), csRight = expression;
753756
if (caseInsensitiveTextComparison) {
757+
// We know lhs isn't null, because we always coalesce it in the switch expression
754758
(csLeft, csRight) = CommonConversions.VisualBasicEqualityComparison
755-
.AdjustForVbStringComparison(node.SelectStatement.Expression, csLeft, typeInfo, vbCase, expression, _semanticModel.GetTypeInfo(vbCase));
759+
.AdjustForVbStringComparison(node.SelectStatement.Expression, csLeft, typeInfo, true, vbCase, csRight, _semanticModel.GetTypeInfo(vbCase), false);
756760
}
757761
expression = SyntaxFactory.BinaryExpression(SyntaxKind.EqualsExpression, csLeft, csRight);
758762

‎CodeConverter/CSharp/VisualBasicEqualityComparison.cs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,28 +72,29 @@ public RequiredType GetObjectEqualityType(VBSyntax.BinaryExpressionSyntax node,
7272

7373

7474

75-
public (ExpressionSyntax lhs, ExpressionSyntax rhs) VbCoerceToNonNullString(VBSyntax.ExpressionSyntax vbLeft, ExpressionSyntax csLeft, TypeInfo lhsTypeInfo, VBSyntax.ExpressionSyntax vbRight, ExpressionSyntax csRight, TypeInfo rhsTypeInfo)
75+
public (ExpressionSyntax lhs, ExpressionSyntax rhs) VbCoerceToNonNullString(VBSyntax.ExpressionSyntax vbLeft, ExpressionSyntax csLeft, TypeInfo lhsTypeInfo, bool leftKnownNotNull, VBSyntax.ExpressionSyntax vbRight, ExpressionSyntax csRight, TypeInfo rhsTypeInfo, bool rightKnownNotNull)
7676
{
7777
if (IsNonEmptyStringLiteral(vbLeft) || IsNonEmptyStringLiteral(vbRight)) {
78+
// If one of the strings is "foo", the other string won't equal it if it's null or empty, so it doesn't matter which one we use in comparison
7879
return (csLeft, csRight);
7980
}
80-
return (VbCoerceToNonNullString(vbLeft, csLeft, lhsTypeInfo), VbCoerceToNonNullString(vbRight, csRight, rhsTypeInfo));
81+
return (VbCoerceToNonNullString(vbLeft, csLeft, lhsTypeInfo, leftKnownNotNull), VbCoerceToNonNullString(vbRight, csRight, rhsTypeInfo, rightKnownNotNull));
8182
}
8283

8384
private static bool IsNonEmptyStringLiteral(VBSyntax.ExpressionSyntax vbExpr)
8485
{
8586
return vbExpr.SkipParens().IsKind(VBSyntaxKind.StringLiteralExpression) && vbExpr is VBSyntax.LiteralExpressionSyntax literal && !IsEmptyString(literal);
8687
}
8788

88-
public ExpressionSyntax VbCoerceToNonNullString(VBSyntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, TypeInfo typeInfo)
89+
public ExpressionSyntax VbCoerceToNonNullString(VBSyntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, TypeInfo typeInfo, bool knownNotNull = false)
8990
{
9091
bool isStringType = typeInfo.Type.SpecialType == SpecialType.System_String;
9192

9293
if (IsNothingOrEmpty(vbNode)) {
9394
return EmptyStringExpression();
9495
}
9596

96-
if (!CanBeNull(vbNode)) {
97+
if (knownNotNull || !CanBeNull(vbNode)) {
9798
return csNode;
9899
}
99100

@@ -202,9 +203,9 @@ private static ExpressionSyntax NegateIfNeeded(VBSyntax.BinaryExpressionSyntax n
202203
: SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, positiveExpression);
203204
}
204205

205-
public (ExpressionSyntax csLeft, ExpressionSyntax csRight) AdjustForVbStringComparison(VBSyntax.ExpressionSyntax vbLeft, ExpressionSyntax csLeft, TypeInfo lhsTypeInfo, VBSyntax.ExpressionSyntax vbRight, ExpressionSyntax csRight, TypeInfo rhsTypeInfo)
206+
public (ExpressionSyntax csLeft, ExpressionSyntax csRight) AdjustForVbStringComparison(VBSyntax.ExpressionSyntax vbLeft, ExpressionSyntax csLeft, TypeInfo lhsTypeInfo, bool leftKnownNotNull, VBSyntax.ExpressionSyntax vbRight, ExpressionSyntax csRight, TypeInfo rhsTypeInfo, bool rightKnownNotNull)
206207
{
207-
(csLeft, csRight) = VbCoerceToNonNullString(vbLeft, csLeft, lhsTypeInfo, vbLeft, csRight, rhsTypeInfo);
208+
(csLeft, csRight) = VbCoerceToNonNullString(vbLeft, csLeft, lhsTypeInfo, leftKnownNotNull, vbRight, csRight, rhsTypeInfo, rightKnownNotNull);
208209
if (OptionCompareTextCaseInsensitive) {
209210
ExtraUsingDirectives.Add("System.Globalization");
210211
var compareOptions = SyntaxFactory.Argument(GetCompareTextCaseInsensitiveCompareOptions());

‎Tests/CSharp/ExpressionTests/StringExpressionTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public void Foo()
182182
{
183183
string s1 = null;
184184
string s2 = """";
185-
if (CultureInfo.CurrentCulture.CompareInfo.Compare(s1, s2, CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) != 0)
185+
if (CultureInfo.CurrentCulture.CompareInfo.Compare(s1 ?? """", s2 ?? """", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) != 0)
186186
{
187187
throw new Exception();
188188
}

‎Tests/CSharp/MissingSemanticModelInfo/ExpressionTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ public PositionEnum PositionEnumFromString(string pS, MissingType missing)
199199
{
200200
var tPos = default(PositionEnum);
201201
var switchExpr = pS.ToUpper();
202-
switch (switchExpr)
202+
switch (switchExpr ?? """")
203203
{
204204
case ""NONE"":
205205
case ""0"":

‎Tests/CSharp/StatementTests/StatementTests.cs‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1585,15 +1585,15 @@ public partial class TestClass
15851585
public static string TimeAgo(string x)
15861586
{
15871587
var switchExpr = Strings.UCase(x);
1588-
switch (switchExpr)
1588+
switch (switchExpr ?? """")
15891589
{
1590-
case var @case when @case == Strings.UCase(""a""):
1591-
case var case1 when case1 == Strings.UCase(""b""):
1590+
case var @case when @case == (Strings.UCase(""a"") ?? """"):
1591+
case var case1 when case1 == (Strings.UCase(""b"") ?? """"):
15921592
{
15931593
return ""ab"";
15941594
}
15951595
1596-
case var case2 when case2 == Strings.UCase(""c""):
1596+
case var case2 when case2 == (Strings.UCase(""c"") ?? """"):
15971597
{
15981598
return ""c"";
15991599
}
@@ -1768,31 +1768,38 @@ await TestConversionVisualBasicToCSharpAsync(@"
17681768
Option Compare Text
17691769
17701770
Class Issue579SelectCaseWithCaseInsensitiveTextCompare
1771-
Private Function Test(astr_Temp As String) As Boolean
1771+
Private Function Test(astr_Temp As String) As Nullable(Of Boolean)
17721772
Select Case astr_Temp
17731773
Case ""Test""
17741774
Return True
1775-
Case Else
1775+
Case astr_Temp
17761776
Return False
1777+
Case Else
1778+
Return Nothing
17771779
End Select
17781780
End Function
17791781
End Class", @"using System.Globalization;
17801782
17811783
internal partial class Issue579SelectCaseWithCaseInsensitiveTextCompare
17821784
{
1783-
private bool Test(string astr_Temp)
1785+
private bool? Test(string astr_Temp)
17841786
{
1785-
switch (astr_Temp)
1787+
switch (astr_Temp ?? """")
17861788
{
1787-
case var @case when CultureInfo.CurrentCulture.CompareInfo.Compare(@case ?? """", ""Test"" ?? """", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0:
1789+
case var @case when CultureInfo.CurrentCulture.CompareInfo.Compare(@case, ""Test"", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0:
17881790
{
17891791
return true;
17901792
}
17911793
1792-
default:
1794+
case var case1 when CultureInfo.CurrentCulture.CompareInfo.Compare(case1, astr_Temp ?? """", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0:
17931795
{
17941796
return false;
17951797
}
1798+
1799+
default:
1800+
{
1801+
return default;
1802+
}
17961803
}
17971804
}
17981805
}

0 commit comments

Comments
 (0)