Avoid stack overflow in stack overflow prevention code - fixes #1047

Author: GrahamTheCoder

CHANGELOG.md

@@ -10,6 +10,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ### VB -> C#
 
+* Avoid stack overflow in stack overflow prevention code [#1047](https://github.com/icsharpcode/CodeConverter/issues/1047)
+* Convert optional DateTime parameters [#1056](https://github.com/icsharpcode/CodeConverter/issues/1056)
 * Convert optional parameters before ref parameters using attributes to avoid compile error [#1057](https://github.com/icsharpcode/CodeConverter/issues/1057)
 * Remove square brackets when escaping labels and identifiers [#1043](https://github.com/icsharpcode/CodeConverter/issues/1043) and [#1044](https://github.com/icsharpcode/CodeConverter/issues/1044)
 * Exit Property now returns value assigned to return variable [#1051](https://github.com/icsharpcode/CodeConverter/issues/1051)

CodeConverter/CSharp/ExpressionNodeVisitor.cs

@@ -2,6 +2,7 @@
 using System.Data;
 using System.Globalization;
 using System.Linq.Expressions;
+using System.Runtime.CompilerServices;
 using System.Xml.Linq;
 using ICSharpCode.CodeConverter.CSharp.Replacements;
 using ICSharpCode.CodeConverter.Util.FromRoslyn;
@@ -845,30 +846,35 @@ private CSharpSyntaxNode ConvertAddressOf(VBSyntax.UnaryExpressionSyntax node, E
         return expr;
     }
 
-    public override async Task<CSharpSyntaxNode> VisitBinaryExpression(VBasic.Syntax.BinaryExpressionSyntax node)
+    public override async Task<CSharpSyntaxNode> VisitBinaryExpression(VBasic.Syntax.BinaryExpressionSyntax entryNode)
     {
-        // Manually walk tree bottom up to avoid stack overflow for deeply nested binary expressions like 3 + 4 + 5 + ...
+        // Walk down the syntax tree for deeply nested binary expressions to avoid stack overflow
+        // e.g. 3 + 4 + 5 + ...
         // Test "DeeplyNestedBinaryExpressionShouldNotStackOverflowAsync()" skipped because it's too slow
-        while (node.Left is VBSyntax.BinaryExpressionSyntax l) {
-            node = l;
-        }
 
         ExpressionSyntax csLhs = null;
-        for (; node is not null; node = BinaryParentWithLhs(node)) {
-            csLhs = (ExpressionSyntax) await ConvertBinaryExpressionAsync(node, csLhs);
+        int levelsToConvert = 0;
+        VBSyntax.BinaryExpressionSyntax currentNode = entryNode;
+
+        // Walk down the nested levels to count them
+        for (var nextNode = entryNode; nextNode != null; currentNode = nextNode, nextNode = currentNode.Left as VBSyntax.BinaryExpressionSyntax, levelsToConvert++) {
+            // 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.
+            if (await RewriteBinaryOperatorOrNullAsync(nextNode) is { } operatorNode) {
+                csLhs = operatorNode;
+                break;
+            }
         }
 
-        return csLhs;
+        // Walk back up the same levels converting as we go.
+        for (; levelsToConvert > 0; currentNode = currentNode!.Parent as VBSyntax.BinaryExpressionSyntax, levelsToConvert--) {
+            csLhs = (ExpressionSyntax)await ConvertBinaryExpressionAsync(currentNode, csLhs);
+        }
 
-        VBSyntax.BinaryExpressionSyntax BinaryParentWithLhs(SyntaxNode currentSyntaxNode) => currentSyntaxNode.Parent is VBSyntax.BinaryExpressionSyntax {Left: var l} p && l == currentSyntaxNode ? p : null;
+        return csLhs;
     }
 
     private async Task<CSharpSyntaxNode> ConvertBinaryExpressionAsync(VBasic.Syntax.BinaryExpressionSyntax node, ExpressionSyntax lhs = null, ExpressionSyntax rhs = null)
     {
-        if (await _operatorConverter.ConvertRewrittenBinaryOperatorOrNullAsync(node, TriviaConvertingExpressionVisitor.IsWithinQuery) is { } operatorNode) {
-            return operatorNode;
-        }
-
         lhs ??= await node.Left.AcceptAsync<ExpressionSyntax>(TriviaConvertingExpressionVisitor);
         rhs ??= await node.Right.AcceptAsync<ExpressionSyntax>(TriviaConvertingExpressionVisitor);
 
@@ -926,6 +932,9 @@ private async Task<CSharpSyntaxNode> ConvertBinaryExpressionAsync(VBasic.Syntax.
         return node.Parent.IsKind(VBasic.SyntaxKind.SimpleArgument) ? exp : exp.AddParens();
     }
 
+    private async Task<ExpressionSyntax> RewriteBinaryOperatorOrNullAsync(VBSyntax.BinaryExpressionSyntax node) =>
+        await _operatorConverter.ConvertRewrittenBinaryOperatorOrNullAsync(node, TriviaConvertingExpressionVisitor.IsWithinQuery);
+
     private async Task<CSharpSyntaxNode> WithRemovedRedundantConversionOrNullAsync(VBSyntax.InvocationExpressionSyntax conversionNode, ISymbol invocationSymbol)
     {
         if (invocationSymbol?.ContainingNamespace.MetadataName != nameof(Microsoft.VisualBasic) ||

Tests/CSharp/ExpressionTests/BinaryExpressionTests.cs

@@ -724,6 +724,30 @@ public void Foo()
 }");
     }
 
+    [Fact]
+    public async Task RewrittenObjectOperatorDoesNotStackOverflowAsync()
+    {
+        await TestConversionVisualBasicToCSharpAsync(@"
+Sub S()
+    Dim a As Object
+    Dim b As Object
+    If (1=Integer.Parse(""1"") And b And a = 1) Then
+    Else If (a = 1 Or b Or a = 2 Or a = 3) Then
+    End If
+End Sub", @"
+public void S()
+{
+    var a = default(object);
+    var b = default(object);
+    if (Conversions.ToBoolean(Operators.AndObject(Operators.AndObject(1 == int.Parse(""1""), b), Operators.ConditionalCompareObjectEqual(a, 1, false))))
+    {
+    }
+    else if (Conversions.ToBoolean(Operators.OrObject(Operators.OrObject(Operators.OrObject(Operators.ConditionalCompareObjectEqual(a, 1, false), b), Operators.ConditionalCompareObjectEqual(a, 2, false)), Operators.ConditionalCompareObjectEqual(a, 3, false))))
+    {
+    }
+}");
+    }
+
     [Fact(Skip = "Too slow")]
     public async Task DeeplyNestedBinaryExpressionShouldNotStackOverflowAsync()
     {