Support long chains of else if statements (#74317)

Avoid recursion when visiting nested `else if` statements. Closes #72393. --------- Co-authored-by: AlekseyTs <[email protected]>
dotnet · Oct 1, 2024 · 8d8276d · 8d8276d
1 parent 2f2dbce
commit 8d8276d
Show file tree

Hide file tree

Showing 27 changed files with 967 additions and 183 deletions.
diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
@@ -2513,15 +2513,64 @@ private void GenerateImplicitConversionErrorsForTupleLiteralArguments(
             }
         }
 
+#nullable enable
         private BoundStatement BindIfStatement(IfStatementSyntax node, BindingDiagnosticBag diagnostics)
         {
-            var condition = BindBooleanExpression(node.Condition, diagnostics);
-            var consequence = BindPossibleEmbeddedStatement(node.Statement, diagnostics);
-            BoundStatement alternative = (node.Else == null) ? null : BindPossibleEmbeddedStatement(node.Else.Statement, diagnostics);
+            return bindIfStatement(this, node, diagnostics);
 
-            BoundStatement result = new BoundIfStatement(node, condition, consequence, alternative);
-            return result;
+            static BoundStatement bindIfStatement(Binder binder, IfStatementSyntax node, BindingDiagnosticBag diagnostics)
+            {
+                var stack = ArrayBuilder<(Binder, IfStatementSyntax IfStatementSyntax, BoundExpression Condition, BoundStatement Consequence)>.GetInstance();
+
+                BoundStatement? alternative;
+                while (true)
+                {
+                    var condition = binder.BindBooleanExpression(node.Condition, diagnostics);
+                    var consequence = binder.BindPossibleEmbeddedStatement(node.Statement, diagnostics);
+                    stack.Push((binder, node, condition, consequence));
+
+                    if (node.Else == null)
+                    {
+                        alternative = null;
+                        break;
+                    }
+
+                    var elseStatementSyntax = node.Else.Statement;
+                    if (elseStatementSyntax is IfStatementSyntax ifStatementSyntax)
+                    {
+                        var b = binder.GetBinder(ifStatementSyntax);
+                        Debug.Assert(b != null);
+                        binder = b;
+                        node = ifStatementSyntax;
+                    }
+                    else
+                    {
+                        alternative = binder.BindPossibleEmbeddedStatement(elseStatementSyntax, diagnostics);
+                        break;
+                    }
+                }
+
+                BoundStatement result;
+                do
+                {
+                    BoundExpression condition;
+                    BoundStatement consequence;
+                    (binder, node, condition, consequence) = stack.Pop();
+                    result = new BoundIfStatement(node, condition, consequence, alternative);
+                    if (stack.Any())
+                    {
+                        result = binder.WrapWithVariablesIfAny(node, result);
+                    }
+                    alternative = result;
+                }
+                while (stack.Any());
+
+                stack.Free();
+
+                return result;
+            }
         }
+#nullable disable
 
         internal BoundExpression BindBooleanExpression(ExpressionSyntax node, BindingDiagnosticBag diagnostics)
         {

diff --git a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
@@ -798,9 +798,29 @@ public override void VisitSwitchExpression(SwitchExpressionSyntax node)
 
         public override void VisitIfStatement(IfStatementSyntax node)
         {
-            Visit(node.Condition, _enclosing);
-            VisitPossibleEmbeddedStatement(node.Statement, _enclosing);
-            Visit(node.Else, _enclosing);
+            Binder enclosing = _enclosing;
+            while (true)
+            {
+                Visit(node.Condition, enclosing);
+                VisitPossibleEmbeddedStatement(node.Statement, enclosing);
+
+                if (node.Else == null)
+                {
+                    break;
+                }
+
+                var elseStatementSyntax = node.Else.Statement;
+                if (elseStatementSyntax is IfStatementSyntax ifStatementSyntax)
+                {
+                    node = ifStatementSyntax;
+                    enclosing = GetBinderForPossibleEmbeddedStatement(node, enclosing);
+                }
+                else
+                {
+                    VisitPossibleEmbeddedStatement(elseStatementSyntax, enclosing);
+                    break;
+                }
+            }
         }
 
         public override void VisitElseClause(ElseClauseSyntax node)
@@ -1019,23 +1039,29 @@ private Binder GetBinderForPossibleEmbeddedStatement(StatementSyntax statement,
             }
         }
 
-        private void VisitPossibleEmbeddedStatement(StatementSyntax statement, Binder enclosing)
+        private Binder GetBinderForPossibleEmbeddedStatement(StatementSyntax statement, Binder enclosing)
         {
-            if (statement != null)
+            CSharpSyntaxNode embeddedScopeDesignator;
+            // Some statements by default do not introduce its own scope for locals.
+            // For example: Expression Statement, Return Statement, etc. However,
+            // when a statement like that is an embedded statement (like IfStatementSyntax.Statement),
+            // then it should introduce a scope for locals declared within it. Here we are detecting
+            // such statements and creating a binder that should own the scope.
+            enclosing = GetBinderForPossibleEmbeddedStatement(statement, enclosing, out embeddedScopeDesignator);
+
+            if (embeddedScopeDesignator != null)
             {
-                CSharpSyntaxNode embeddedScopeDesignator;
-                // Some statements by default do not introduce its own scope for locals.
-                // For example: Expression Statement, Return Statement, etc. However,
-                // when a statement like that is an embedded statement (like IfStatementSyntax.Statement),
-                // then it should introduce a scope for locals declared within it. Here we are detecting
-                // such statements and creating a binder that should own the scope.
-                enclosing = GetBinderForPossibleEmbeddedStatement(statement, enclosing, out embeddedScopeDesignator);
+                AddToMap(embeddedScopeDesignator, enclosing);
+            }
 
-                if (embeddedScopeDesignator != null)
-                {
-                    AddToMap(embeddedScopeDesignator, enclosing);
-                }
+            return enclosing;
+        }
 
+        private void VisitPossibleEmbeddedStatement(StatementSyntax statement, Binder enclosing)
+        {
+            if (statement != null)
+            {
+                enclosing = GetBinderForPossibleEmbeddedStatement(statement, enclosing);
                 Visit(statement, enclosing);
             }
         }

diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs
@@ -152,5 +152,48 @@ protected BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperat
 
             return left;
         }
+
+        public sealed override BoundNode? VisitIfStatement(BoundIfStatement node)
+        {
+            if (node.AlternativeOpt is not BoundIfStatement ifStatement)
+            {
+                return base.VisitIfStatement(node);
+            }
+
+            var stack = ArrayBuilder<BoundIfStatement>.GetInstance();
+            stack.Push(node);
+
+            BoundStatement? alternative;
+            while (true)
+            {
+                stack.Push(ifStatement);
+
+                alternative = ifStatement.AlternativeOpt;
+                if (alternative is not BoundIfStatement nextIfStatement)
+                {
+                    break;
+                }
+
+                ifStatement = nextIfStatement;
+            }
+
+            alternative = (BoundStatement?)this.Visit(alternative);
+
+            do
+            {
+                ifStatement = stack.Pop();
+
+                BoundExpression condition = (BoundExpression)this.Visit(ifStatement.Condition);
+                BoundStatement consequence = (BoundStatement)this.Visit(ifStatement.Consequence);
+
+                alternative = ifStatement.Update(condition, consequence, alternative);
+            }
+            while (stack.Count > 0);
+
+            Debug.Assert((object)ifStatement == node);
+            stack.Free();
+
+            return alternative;
+        }
     }
 }
diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs
@@ -195,5 +195,29 @@ protected virtual void VisitArguments(BoundCall node)
         {
             this.VisitList(node.Arguments);
         }
+
+        public sealed override BoundNode? VisitIfStatement(BoundIfStatement node)
+        {
+            while (true)
+            {
+                Visit(node.Condition);
+                Visit(node.Consequence);
+                var alternative = node.AlternativeOpt;
+                if (alternative is null)
+                {
+                    break;
+                }
+                if (alternative is BoundIfStatement elseIfStatement)
+                {
+                    node = elseIfStatement;
+                }
+                else
+                {
+                    Visit(alternative);
+                    break;
+                }
+            }
+            return null;
+        }
     }
 }
diff --git a/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs
@@ -27,6 +27,49 @@ internal sealed partial class NullabilityRewriter : BoundTreeRewriter
             return VisitBinaryOperatorBase(node);
         }
 
+        public override BoundNode? VisitIfStatement(BoundIfStatement node)
+        {
+            var stack = ArrayBuilder<(BoundIfStatement, BoundExpression, BoundStatement)>.GetInstance();
+
+            BoundStatement? rewrittenAlternative;
+            while (true)
+            {
+                var rewrittenCondition = (BoundExpression)Visit(node.Condition);
+                var rewrittenConsequence = (BoundStatement)Visit(node.Consequence);
+                Debug.Assert(rewrittenConsequence is { });
+                stack.Push((node, rewrittenCondition, rewrittenConsequence));
+
+                var alternative = node.AlternativeOpt;
+                if (alternative is null)
+                {
+                    rewrittenAlternative = null;
+                    break;
+                }
+
+                if (alternative is BoundIfStatement elseIfStatement)
+                {
+                    node = elseIfStatement;
+                }
+                else
+                {
+                    rewrittenAlternative = (BoundStatement)Visit(alternative);
+                    break;
+                }
+            }
+
+            BoundStatement result;
+            do
+            {
+                var (ifStatement, rewrittenCondition, rewrittenConsequence) = stack.Pop();
+                result = ifStatement.Update(rewrittenCondition, rewrittenConsequence, rewrittenAlternative);
+                rewrittenAlternative = result;
+            }
+            while (stack.Any());
+
+            stack.Free();
+            return result;
+        }
+
         private BoundNode VisitBinaryOperatorBase(BoundBinaryOperatorBase binaryOperator)
         {
             // Use an explicit stack to avoid blowing the managed stack when visiting deeply-recursive

diff --git a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs
@@ -5,14 +5,12 @@
 #nullable disable
 
 using System.Collections.Generic;
-using System.Collections.Immutable;
+using System.Diagnostics;
 using Microsoft.CodeAnalysis.Collections;
+using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Roslyn.Utilities;
-using System.Diagnostics;
-using System.Linq;
-using Microsoft.CodeAnalysis.CSharp.Symbols;
 
 namespace Microsoft.CodeAnalysis.CSharp
 {
@@ -287,6 +285,37 @@ public override BoundNode VisitBinaryOperator(BoundBinaryOperator node)
                 throw ExceptionUtilities.Unreachable();
             }
 
+            public override BoundNode VisitIfStatement(BoundIfStatement node)
+            {
+                while (true)
+                {
+                    this.Visit(node.Condition);
+                    this.Visit(node.Consequence);
+
+                    var alternative = node.AlternativeOpt;
+                    if (alternative is null)
+                    {
+                        break;
+                    }
+
+                    if (alternative is BoundIfStatement elseIfStatement)
+                    {
+                        node = elseIfStatement;
+                        if (ShouldAddNode(node))
+                        {
+                            _map.Add(node.Syntax, node);
+                        }
+                    }
+                    else
+                    {
+                        this.Visit(alternative);
+                        break;
+                    }
+                }
+
+                return null;
+            }
+
             protected override bool ConvertInsufficientExecutionStackExceptionToCancelledByStackGuardException()
             {
                 return false;

diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
@@ -2187,7 +2187,7 @@ private static bool IsEmptyRewritePossible(BoundNode node)
             }
         }
 
-        private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuard
+        private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
         {
         }