Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support long chains of else if statements #74317

Merged
merged 24 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 54 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2494,15 +2494,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)
{
Expand Down
58 changes: 42 additions & 16 deletions src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}
Expand Down
43 changes: 43 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
24 changes: 24 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
43 changes: 43 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2180,7 +2180,7 @@ private static bool IsEmptyRewritePossible(BoundNode node)
}
}

private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuard
private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
Copy link
Member

@333fred 333fred Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about making this change for my BinaryPatterns change, but I don't think it's actually desirable; rather, this is why I updated StackGuard to handle binary patterns as well as expressions, so that it would throw a CancelledByStackGuard exception and enter the catch above. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This visitor and its usage were added in #65398. It looks there was no specific motivation around verifying stack guard behavior. Rather it looks like it was about asserting that Update implementations are working for all possible nodes. The purpose is still preserved, in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, the change made to BoundTreeRewriterWithStackGuard around binary patterns probably makes sense since they are used in expression context. However, it is probably not that useful because patterns do not survive the very first rewrite phase (lowering).

{
}

Expand Down
Loading