From 3a22e738f410e4cadb8bd2afb50f3d9a33d40508 Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Wed, 13 Mar 2024 11:12:51 -0700 Subject: [PATCH 01/23] Nested if/else if --- .../Portable/Binder/Binder_Statements.cs | 43 +++++++++- .../Portable/Binder/LocalBinderFactory.cs | 24 +++++- .../Portable/Binder/RefSafetyAnalysis.cs | 27 ++++++ .../CSharp/Portable/BoundTree/BoundNode.cs | 24 ++++++ .../Portable/BoundTree/NullabilityRewriter.cs | 43 ++++++++++ .../CSharp/Portable/CodeGen/CodeGenerator.cs | 29 +++++-- .../CSharp/Portable/CodeGen/EmitStatement.cs | 30 ++++++- .../CSharp/Portable/CodeGen/Optimizer.cs | 25 +++++- .../MemberSemanticModel.NodeMapBuilder.cs | 33 ++++++- .../Portable/Compiler/MethodCompiler.cs | 7 ++ .../Portable/FlowAnalysis/AbstractFlowPass.cs | 51 ++++++++--- .../NullableWalker.DebugVerifier.cs | 27 ++++++ .../DiagnosticsPass_ExpressionTrees.cs | 27 ++++++ .../Lowering/LocalRewriter/LocalRewriter.cs | 9 +- .../LocalRewriter_IfStatement.cs | 68 +++++++++++---- .../Operations/CSharpOperationFactory.cs | 53 ++++++++++-- .../CSharp/Portable/Parser/LanguageParser.cs | 64 ++++++++++++-- .../CSharp/Portable/Syntax/LookupPosition.cs | 24 +++++- .../CSharp/Test/Emit/CodeGen/CodeGenTests.cs | 86 +++++++++++++++++++ .../Operations/ControlFlowGraphBuilder.cs | 35 ++++++-- .../Operations/OperationMapBuilder.cs | 21 +++++ 21 files changed, 670 insertions(+), 80 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index b0f799d6ae6c3..e75a3af2e672c 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -2494,15 +2494,50 @@ 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); + var stack = ArrayBuilder<(IfStatementSyntax IfStatementSyntax, BoundExpression Condition, BoundStatement Consequence)>.GetInstance(); + + BoundStatement? alternative; + while (true) + { + var condition = BindBooleanExpression(node.Condition, diagnostics); + var consequence = BindPossibleEmbeddedStatement(node.Statement, diagnostics); + stack.Push((node, condition, consequence)); + + if (node.Else == null) + { + alternative = null; + break; + } + + var elseStatementSyntax = node.Else.Statement; + if (elseStatementSyntax is IfStatementSyntax ifStatementSyntax) + { + node = ifStatementSyntax; + } + else + { + alternative = BindPossibleEmbeddedStatement(elseStatementSyntax, diagnostics); + break; + } + } + + BoundStatement result; + do + { + var (ifStatementSyntax, condition, consequence) = stack.Pop(); + result = new BoundIfStatement(ifStatementSyntax, condition, consequence, alternative); + alternative = result; + } + while (stack.Any()); + + stack.Free(); - BoundStatement result = new BoundIfStatement(node, condition, consequence, alternative); 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 index 1e6ee6bfc4953..ee3eb7ba137e3 100644 --- a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs +++ b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs @@ -798,9 +798,27 @@ public override void VisitSwitchExpression(SwitchExpressionSyntax node) public override void VisitIfStatement(IfStatementSyntax node) { - Visit(node.Condition, _enclosing); - VisitPossibleEmbeddedStatement(node.Statement, _enclosing); - Visit(node.Else, _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; + } + else + { + VisitPossibleEmbeddedStatement(elseStatementSyntax, _enclosing); + break; + } + } } public override void VisitElseClause(ElseClauseSyntax node) diff --git a/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs b/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs index 44f2e3406ca1a..cde7850f0dc5f 100644 --- a/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs +++ b/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs @@ -1058,6 +1058,33 @@ private static ImmutableArray GetDeconstructionRightParts(Bound return null; } + 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; + } + else + { + this.Visit(alternative); + break; + } + } + + return null; + } + private static void Error(BindingDiagnosticBag diagnostics, ErrorCode code, SyntaxNodeOrToken syntax, params object[] args) { var location = syntax.GetLocation(); diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs index 8f92867f0dd10..806a41820e6e8 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs @@ -654,6 +654,30 @@ private void CheckDeclared(LocalSymbol local) return null; } + public 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; + } + public override BoundNode? VisitFixedStatement(BoundFixedStatement node) { AddAll(node.Locals); diff --git a/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs index db5389f0b60c1..a579f94cb1262 100644 --- 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/CodeGen/CodeGenerator.cs b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs index ce7045499e411..a55b6577b0c02 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs @@ -140,9 +140,9 @@ public CodeGenerator( debugFriendly: _ilEmitStyle != ILEmitStyle.Release, stackLocals: out _stackLocals); } - catch (BoundTreeVisitor.CancelledByStackGuardException ex) + catch (BoundTreeVisitor.CancelledByStackGuardException) { - ex.AddAnError(diagnostics); + //ex.AddAnError(diagnostics); // PROTOTYPE: Revert the changes to CodeGenerator and Optimizer and instead change VisitStatementList(). _boundBody = boundBody; } @@ -393,13 +393,29 @@ private void EmitSequencePointStatement(BoundSequencePoint node) } } - private void EmitSequencePointStatement(BoundSequencePointWithSpan node) + private void EmitSequencePointStatementBegin(BoundSequencePointWithSpan node) { TextSpan span = node.Span; if (span != default(TextSpan) && _emitPdbSequencePoints) { this.EmitSequencePoint(node.SyntaxTree, span); } + } + + private void EmitSequencePointStatementEnd(BoundSequencePointWithSpan node, int instructionsEmitted) + { + TextSpan span = node.Span; + if (instructionsEmitted == 0 && span != default(TextSpan) && _ilEmitStyle == ILEmitStyle.Debug) + { + // if there was no code emitted, then emit nop + // otherwise this point could get associated with some random statement, possibly in a wrong scope + _builder.EmitOpCode(ILOpCode.Nop); + } + } + + private void EmitSequencePointStatement(BoundSequencePointWithSpan node) + { + EmitSequencePointStatementBegin(node); BoundStatement statement = node.StatementOpt; int instructionsEmitted = 0; @@ -408,12 +424,7 @@ private void EmitSequencePointStatement(BoundSequencePointWithSpan node) instructionsEmitted = this.EmitStatementAndCountInstructions(statement); } - if (instructionsEmitted == 0 && span != default(TextSpan) && _ilEmitStyle == ILEmitStyle.Debug) - { - // if there was no code emitted, then emit nop - // otherwise this point could get associated with some random statement, possibly in a wrong scope - _builder.EmitOpCode(ILOpCode.Nop); - } + EmitSequencePointStatementEnd(node, instructionsEmitted); } private void EmitSavePreviousSequencePoint(BoundSavePreviousSequencePoint statement) diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs index 107a8f02f2481..c09429d6f1809 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs @@ -124,10 +124,36 @@ private int EmitStatementAndCountInstructions(BoundStatement statement) private void EmitStatementList(BoundStatementList list) { - for (int i = 0, n = list.Statements.Length; i < n; i++) + var stack = ArrayBuilder<(BoundStatementList, int, BoundSequencePointWithSpan, int)>.GetInstance(); + + stack.Push((list, 0, null, 0)); + + while (stack.Any()) { - EmitStatement(list.Statements[i]); + int i; + BoundSequencePointWithSpan sequencePointWithSpan; + int instructionsEmittedBegin; + (list, i, sequencePointWithSpan, instructionsEmittedBegin) = stack.Pop(); + for (int n = list.Statements.Length; i < n; i++) + { + var statement = list.Statements[i]; + if (statement is BoundSequencePointWithSpan { StatementOpt: BoundStatementList nestedList } nestedSequencePointWithSpan) + { + EmitSequencePointStatementBegin(nestedSequencePointWithSpan); + stack.Push((list, i + 1, sequencePointWithSpan, instructionsEmittedBegin)); + stack.Push((nestedList, 0, nestedSequencePointWithSpan, _builder.InstructionsEmitted)); + goto endOfLoop; + } + EmitStatement(statement); + } + if (sequencePointWithSpan is { }) + { + EmitSequencePointStatementEnd(sequencePointWithSpan, _builder.InstructionsEmitted - instructionsEmittedBegin); + } +endOfLoop: continue; } + + stack.Free(); } private void EmitNoOpStatement(BoundNoOpStatement statement) diff --git a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs index a704cc630aa4e..3484d0ea38ee1 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs @@ -566,7 +566,30 @@ private void PopEvalStack() public BoundNode VisitStatement(BoundNode node) { Debug.Assert(node == null || EvalStackIsEmpty()); - return VisitSideEffect(node); + + _recursionDepth++; + + BoundNode result; + if (_recursionDepth > 1) + { + StackGuard.EnsureSufficientExecutionStack(_recursionDepth); + + result = VisitSideEffect(node); + } + else + { + try + { + result = VisitSideEffect(node); + } + catch (InsufficientExecutionStackException ex) + { + throw new CancelledByStackGuardException(ex, node); + } + } + + _recursionDepth--; + return result; } public BoundNode VisitSideEffect(BoundNode node) diff --git a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs index 8a489265541c9..3b91a63b7048e 100644 --- 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,33 @@ 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; + } + 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 index 7940f2df63ec4..ff4f1cddcf9ad 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -1438,6 +1438,7 @@ internal static BoundStatement LowerBodyOrInitializer( return bodyWithoutLambdas; } + // PROTOTYPE: What about IteratorRewriter? BoundStatement bodyWithoutIterators = IteratorRewriter.Rewrite(bodyWithoutLambdas, method, methodOrdinal, stateMachineStateDebugInfoBuilder, lazyVariableSlotAllocator, compilationState, diagnostics, out IteratorStateMachine iteratorStateMachine); @@ -1446,6 +1447,7 @@ internal static BoundStatement LowerBodyOrInitializer( return bodyWithoutIterators; } + // PROTOTYPE: What about AsyncRewriter? BoundStatement bodyWithoutAsync = AsyncRewriter.Rewrite(bodyWithoutIterators, method, methodOrdinal, stateMachineStateDebugInfoBuilder, lazyVariableSlotAllocator, compilationState, diagnostics, out AsyncStateMachine asyncStateMachine); @@ -2166,6 +2168,11 @@ private static bool IsEmptyRewritePossible(BoundNode node) private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuard { + // PROTOTYPE: This override shouldn't be necessary. The base class should use a stack. + public override BoundNode? VisitIfStatement(BoundIfStatement node) + { + return node; // PROTOTYPE: Not implemented. + } } private sealed class UnboundLambdaFinder : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 9d4bef8a46fe0..efec31fbb1727 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -1707,19 +1707,50 @@ protected virtual void AfterVisitConversion(BoundConversion node) public override BoundNode VisitIfStatement(BoundIfStatement node) { // 5.3.3.5 If statements - VisitCondition(node.Condition); - TLocalState trueState = StateWhenTrue; - TLocalState falseState = StateWhenFalse; - SetState(trueState); - VisitStatement(node.Consequence); - trueState = this.State; - SetState(falseState); - if (node.AlternativeOpt != null) + + var stack = ArrayBuilder.GetInstance(); + + TLocalState trueState; + while (true) + { + VisitCondition(node.Condition); + trueState = StateWhenTrue; + TLocalState falseState = StateWhenFalse; + SetState(trueState); + VisitStatement(node.Consequence); + trueState = this.State; + SetState(falseState); + + var alternative = node.AlternativeOpt; + if (alternative is null) + { + break; + } + + SetState(falseState); + if (alternative is BoundIfStatement elseIfStatement) + { + stack.Push(trueState); + node = elseIfStatement; + } + else + { + VisitStatement(alternative); + break; + } + } + + while (true) { - VisitStatement(node.AlternativeOpt); + Join(ref this.State, ref trueState); + if (!stack.Any()) + { + break; + } + trueState = stack.Pop(); } - Join(ref this.State, ref trueState); + stack.Free(); return null; } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs index 9fad0aeeb0729..04757fa99dafe 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs @@ -159,6 +159,33 @@ private void VerifyExpression(BoundExpression expression, bool overrideSkippedEx return null; } + 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; + } + else + { + this.Visit(alternative); + break; + } + } + + return null; + } + public override BoundNode? VisitForEachStatement(BoundForEachStatement node) { Visit(node.IterationVariableType); diff --git a/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs b/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs index 1f401b2ef6eff..4e08050b2124e 100644 --- a/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs +++ b/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs @@ -1045,5 +1045,32 @@ public override BoundNode VisitCollectionExpression(BoundCollectionExpression no return base.VisitCollectionExpression(node); } + + 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; + } + else + { + this.Visit(alternative); + break; + } + } + + return null; + } } } diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs index 59cd4fd4afe8a..83cc84c6125cf 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs @@ -145,7 +145,8 @@ public static BoundStatement Rewrite( statement.CheckLocalsDefined(); var loweredStatement = localRewriter.VisitStatement(statement); Debug.Assert(loweredStatement is { }); - loweredStatement.CheckLocalsDefined(); + // PROTOTYPE: Add _recursionDepth to LocalsScanner and avoid recursing in the same way we're currently doing in StackOptimizerPass1. + //loweredStatement.CheckLocalsDefined(); sawLambdas = localRewriter._sawLambdas; sawLocalFunctions = localRewriter._availableLocalFunctionOrdinal != 0; sawAwaitInExceptionHandler = localRewriter._sawAwaitInExceptionHandler; @@ -154,13 +155,15 @@ public static BoundStatement Rewrite( { // Move spill sequences to a top-level statement. This handles "lifting" await and the switch expression. var spilledStatement = SpillSequenceSpiller.Rewrite(loweredStatement, method, compilationState, diagnostics); - spilledStatement.CheckLocalsDefined(); + // PROTOTYPE: Same comment as above. + //spilledStatement.CheckLocalsDefined(); loweredStatement = spilledStatement; } codeCoverageSpans = codeCoverageInstrumenter?.DynamicAnalysisSpans ?? ImmutableArray.Empty; #if DEBUG - LocalRewritingValidator.Validate(loweredStatement); + // PROTOTYPE: Same comment as above. + //LocalRewritingValidator.Validate(loweredStatement); localRewriter.AssertNoPlaceholderReplacements(); #endif return loweredStatement; diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs index e4f8d4642097b..45e2fa389b21f 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs @@ -6,8 +6,6 @@ using Microsoft.CodeAnalysis.CSharp.Symbols; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.PooledObjects; -using Microsoft.CodeAnalysis.Text; -using System.Collections.Immutable; namespace Microsoft.CodeAnalysis.CSharp { @@ -16,27 +14,63 @@ internal sealed partial class LocalRewriter public override BoundNode VisitIfStatement(BoundIfStatement node) { Debug.Assert(node != null); - var rewrittenCondition = VisitExpression(node.Condition); - var rewrittenConsequence = VisitStatement(node.Consequence); - Debug.Assert(rewrittenConsequence is { }); - var rewrittenAlternative = VisitStatement(node.AlternativeOpt); - var syntax = (IfStatementSyntax)node.Syntax; - - // EnC: We need to insert a hidden sequence point to handle function remapping in case - // the containing method is edited while methods invoked in the condition are being executed. - if (this.Instrument && !node.WasCompilerGenerated) + + var stack = ArrayBuilder<(BoundIfStatement, BoundExpression, BoundStatement)>.GetInstance(); + + BoundStatement? rewrittenAlternative; + while (true) { - rewrittenCondition = Instrumenter.InstrumentIfStatementCondition(node, rewrittenCondition, _factory); - } + var rewrittenCondition = VisitExpression(node.Condition); + var rewrittenConsequence = VisitStatement(node.Consequence); + Debug.Assert(rewrittenConsequence is { }); + stack.Push((node, rewrittenCondition, rewrittenConsequence)); - var result = RewriteIfStatement(syntax, rewrittenCondition, rewrittenConsequence, rewrittenAlternative, node.HasErrors); + var alternative = node.AlternativeOpt; + if (alternative is null) + { + rewrittenAlternative = null; + break; + } + + if (alternative is BoundIfStatement elseIfStatement) + { + node = elseIfStatement; + } + else + { + rewrittenAlternative = VisitStatement(alternative); + break; + } + } - // add sequence point before the whole statement - if (this.Instrument && !node.WasCompilerGenerated) + BoundStatement result; + do { - result = Instrumenter.InstrumentIfStatement(node, result); + var (ifStatement, rewrittenCondition, rewrittenConsequence) = stack.Pop(); + node = ifStatement; + + var syntax = (IfStatementSyntax)node.Syntax; + + // EnC: We need to insert a hidden sequence point to handle function remapping in case + // the containing method is edited while methods invoked in the condition are being executed. + if (this.Instrument && !node.WasCompilerGenerated) + { + rewrittenCondition = Instrumenter.InstrumentIfStatementCondition(node, rewrittenCondition, _factory); + } + + result = RewriteIfStatement(syntax, rewrittenCondition, rewrittenConsequence, rewrittenAlternative, node.HasErrors); + + // add sequence point before the whole statement + if (this.Instrument && !node.WasCompilerGenerated) + { + result = Instrumenter.InstrumentIfStatement(node, result); + } + + rewrittenAlternative = result; } + while (stack.Any()); + stack.Free(); return result; } diff --git a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs index 2b7e62c5393bc..35d6f4a1886fb 100644 --- a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs +++ b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs @@ -1801,15 +1801,50 @@ private IEmptyOperation CreateBoundNoOpStatementOperation(BoundNoOpStatement bou private IConditionalOperation CreateBoundIfStatementOperation(BoundIfStatement boundIfStatement) { - IOperation condition = Create(boundIfStatement.Condition); - IOperation whenTrue = Create(boundIfStatement.Consequence); - IOperation? whenFalse = Create(boundIfStatement.AlternativeOpt); - bool isRef = false; - SyntaxNode syntax = boundIfStatement.Syntax; - ITypeSymbol? type = null; - ConstantValue? constantValue = null; - bool isImplicit = boundIfStatement.WasCompilerGenerated; - return new ConditionalOperation(condition, whenTrue, whenFalse, isRef, _semanticModel, syntax, type, constantValue, isImplicit); + var stack = ArrayBuilder<(BoundIfStatement, IOperation, IOperation)>.GetInstance(); + + IOperation? whenFalse; + while (true) + { + IOperation condition = Create(boundIfStatement.Condition); + IOperation whenTrue = Create(boundIfStatement.Consequence); + Debug.Assert(whenTrue is { }); + stack.Push((boundIfStatement, condition, whenTrue)); + + var alternative = boundIfStatement.AlternativeOpt; + if (alternative is null) + { + whenFalse = null; + break; + } + + if (alternative is BoundIfStatement elseIfStatement) + { + boundIfStatement = elseIfStatement; + } + else + { + whenFalse = Create(alternative); + break; + } + } + + ConditionalOperation result; + do + { + var (ifStatement, condition, whenTrue) = stack.Pop(); + bool isRef = false; + SyntaxNode syntax = ifStatement.Syntax; + ITypeSymbol? type = null; + ConstantValue? constantValue = null; + bool isImplicit = ifStatement.WasCompilerGenerated; + result = new ConditionalOperation(condition, whenTrue, whenFalse, isRef, _semanticModel, syntax, type, constantValue, isImplicit); + whenFalse = result; + } + while (stack.Any()); + + stack.Free(); + return result; } private IWhileLoopOperation CreateBoundWhileStatementOperation(BoundWhileStatement boundWhileStatement) diff --git a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs index 887dfabee8fb7..e500a5819ede1 100644 --- a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs +++ b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs @@ -9357,16 +9357,64 @@ private IfStatementSyntax ParseIfStatement(SyntaxList attri { Debug.Assert(this.CurrentToken.Kind == SyntaxKind.IfKeyword); - return _syntaxFactory.IfStatement( - attributes, - this.EatToken(SyntaxKind.IfKeyword), - this.EatToken(SyntaxKind.OpenParenToken), - this.ParseExpressionCore(), - this.EatToken(SyntaxKind.CloseParenToken), - this.ParseEmbeddedStatement(), - this.ParseElseClauseOpt()); + var stack = ArrayBuilder<(SyntaxList, SyntaxToken, SyntaxToken, ExpressionSyntax, SyntaxToken, StatementSyntax, SyntaxToken)>.GetInstance(); + + StatementSyntax alternative = null; + while (true) + { + var ifKeyword = this.EatToken(SyntaxKind.IfKeyword); + var openParen = this.EatToken(SyntaxKind.OpenParenToken); + var condition = this.ParseExpressionCore(); + var closeParen = this.EatToken(SyntaxKind.CloseParenToken); + var consequence = this.ParseEmbeddedStatement(); + + var elseKeyword = this.CurrentToken.Kind != SyntaxKind.ElseKeyword ? + null : + this.EatToken(SyntaxKind.ElseKeyword); + stack.Push((attributes, ifKeyword, openParen, condition, closeParen, consequence, elseKeyword)); + + if (elseKeyword is null) + { + alternative = null; + break; + } + + if (this.CurrentToken.Kind != SyntaxKind.IfKeyword) + { + alternative = this.ParseEmbeddedStatement(); + break; + } + + attributes = default; + } + + IfStatementSyntax ifStatement; + do + { + var (attr, ifKeyword, openParen, condition, closeParen, consequence, elseKeyword) = stack.Pop(); + var elseClause = alternative is null ? + null : + _syntaxFactory.ElseClause( + elseKeyword, + alternative); + ifStatement = _syntaxFactory.IfStatement( + attr, + ifKeyword, + openParen, + condition, + closeParen, + consequence, + elseClause); + alternative = ifStatement; + } + while (stack.Any()); + + stack.Free(); + + return ifStatement; } + // PROTOTYPE: This method is similar to the original implementation of ParseIfStatement. Can we share code? private IfStatementSyntax ParseMisplacedElse(SyntaxList attributes) { Debug.Assert(this.CurrentToken.Kind == SyntaxKind.ElseKeyword); diff --git a/src/Compilers/CSharp/Portable/Syntax/LookupPosition.cs b/src/Compilers/CSharp/Portable/Syntax/LookupPosition.cs index 9015176b4b586..04d606e0d09c5 100644 --- a/src/Compilers/CSharp/Portable/Syntax/LookupPosition.cs +++ b/src/Compilers/CSharp/Portable/Syntax/LookupPosition.cs @@ -403,9 +403,7 @@ internal static SyntaxToken GetFirstExcludedToken(StatementSyntax statement) case SyntaxKind.GotoStatement: return ((GotoStatementSyntax)statement).SemicolonToken; case SyntaxKind.IfStatement: - IfStatementSyntax ifStmt = (IfStatementSyntax)statement; - ElseClauseSyntax? elseOpt = ifStmt.Else; - return GetFirstExcludedToken(elseOpt == null ? ifStmt.Statement : elseOpt.Statement); + return GetFirstExcludedIfStatementToken((IfStatementSyntax)statement); case SyntaxKind.LabeledStatement: return GetFirstExcludedToken(((LabeledStatementSyntax)statement).Statement); case SyntaxKind.LockStatement: @@ -452,6 +450,26 @@ internal static SyntaxToken GetFirstExcludedToken(StatementSyntax statement) } } + private static SyntaxToken GetFirstExcludedIfStatementToken(IfStatementSyntax ifStmt) + { + while (true) + { + ElseClauseSyntax? elseOpt = ifStmt.Else; + if (elseOpt is null) + { + return GetFirstExcludedToken(ifStmt.Statement); + } + if (elseOpt.Statement is IfStatementSyntax nestedIf) + { + ifStmt = nestedIf; + } + else + { + return GetFirstExcludedToken(elseOpt.Statement); + } + } + } + internal static bool IsInAnonymousFunctionOrQuery(int position, SyntaxNode lambdaExpressionOrQueryNode) { Debug.Assert(lambdaExpressionOrQueryNode.IsAnonymousFunction() || lambdaExpressionOrQueryNode.IsQuery()); diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs index 190e152c175b4..4ed821aa681e7 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs @@ -10,6 +10,7 @@ using System.Threading; using Microsoft.CodeAnalysis.CSharp.Emit; using Microsoft.CodeAnalysis.CSharp.Symbols; +using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.CSharp.Test.Utilities; using Microsoft.CodeAnalysis.Emit; using Microsoft.CodeAnalysis.Test.Resources.Proprietary; @@ -17324,5 +17325,90 @@ .maxstack 1 } "); } + + [WorkItem("https://github.com/dotnet/roslyn/issues/72393")] + [Theory] + [InlineData(2)] + [InlineData(5000)] + public void NestedIfElse(int n) + { + var builder = new System.Text.StringBuilder(); + builder.AppendLine(""" + #nullable enable + class Program + { + static void F(int i) + { + if (i == 0) { } + """); + for (int i = 0; i < n; i++) + { + builder.AppendLine($$""" + else if (i == {{i}}) { } + """); + } + builder.AppendLine(""" + } + } + """); + + var source = builder.ToString(); + var comp = CreateCompilation(source); + comp.VerifyEmitDiagnostics(); + + var tree = comp.SyntaxTrees.Single(); + var model = comp.GetSemanticModel(tree); + var node = tree.GetRoot().DescendantNodes().OfType().Single(); + + // Avoid using ControlFlowGraphVerifier.GetControlFlowGraph() since that calls + // TestOperationVisitor.VerifySubTree() which has quadratic behavior using + // MemberSemanticModel.GetEnclosingBinderInternalWithinRoot(). + var operation = (Microsoft.CodeAnalysis.Operations.IMethodBodyOperation)model.GetOperation(node); + var graph = Microsoft.CodeAnalysis.FlowAnalysis.ControlFlowGraph.Create(operation); + + if (n == 2) + { + var symbol = model.GetDeclaredSymbol(node); + ControlFlowGraphVerifier.VerifyGraph(comp, """ + Block[B0] - Entry + Statements (0) + Next (Regular) Block[B1] + Block[B1] - Block + Predecessors: [B0] + Statements (0) + Jump if False (Regular) to Block[B2] + IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 0') + Left: + IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 0) (Syntax: '0') + Next (Regular) Block[B4] + Block[B2] - Block + Predecessors: [B1] + Statements (0) + Jump if False (Regular) to Block[B3] + IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 0') + Left: + IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 0) (Syntax: '0') + Next (Regular) Block[B4] + Block[B3] - Block + Predecessors: [B2] + Statements (0) + Jump if False (Regular) to Block[B4] + IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 1') + Left: + IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1') + Next (Regular) Block[B4] + Block[B4] - Exit + Predecessors: [B1] [B2] [B3*2] + Statements (0) + """, + graph, symbol); + } + } } } diff --git a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs index 1d16642e7b7f9..5d11ef6275012 100644 --- a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs @@ -1568,18 +1568,39 @@ private void VisitMethodBodies(IBlockOperation? blockBody, IBlockOperation? expr // alternative; // afterif: - BasicBlockBuilder? whenFalse = null; - VisitConditionalBranch(operation.Condition, ref whenFalse, jumpIfTrue: false); + var stack = ArrayBuilder<(IConditionalOperation, BasicBlockBuilder)>.GetInstance(); - VisitStatement(operation.WhenTrue); + while (true) + { + BasicBlockBuilder? whenFalse = null; + VisitConditionalBranch(operation.Condition, ref whenFalse, jumpIfTrue: false); + Debug.Assert(whenFalse is { }); + VisitStatement(operation.WhenTrue); + var afterIf = new BasicBlockBuilder(BasicBlockKind.Block); + UnconditionalBranch(afterIf); - var afterIf = new BasicBlockBuilder(BasicBlockKind.Block); - UnconditionalBranch(afterIf); + AppendNewBlock(whenFalse); + stack.Push((operation, afterIf)); + + if (operation.WhenFalse is IConditionalOperation { WhenFalse: not null } nested) + { + operation = nested; + } + else + { + break; + } + } - AppendNewBlock(whenFalse); VisitStatement(operation.WhenFalse); + do + { + var (conditional, afterIf) = stack.Pop(); + AppendNewBlock(afterIf); + } + while (stack.Any()); - AppendNewBlock(afterIf); + stack.Free(); } return null; diff --git a/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs b/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs index 34abe6d500ed4..48be8601cee34 100644 --- a/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs @@ -53,6 +53,27 @@ private sealed class Walker : OperationWalker return null; } + public override object? VisitConditional(IConditionalOperation operation, Dictionary argument) + { + while (true) + { + RecordOperation(operation, argument); + Visit(operation.Condition, argument); + Visit(operation.WhenTrue, argument); + if (operation.WhenFalse is IConditionalOperation nested) + { + operation = nested; + } + else + { + Visit(operation.WhenFalse, argument); + break; + } + } + + return null; + } + internal override object? VisitNoneOperation(IOperation operation, Dictionary argument) { // OperationWalker skips these nodes by default, to avoid having public consumers deal with NoneOperation. From 86a1bf8eb5cba1d7be61fd2c641af311242fe21f Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Wed, 10 Jul 2024 10:02:15 -0700 Subject: [PATCH 02/23] BindIfStatement --- .../Portable/Binder/Binder_Statements.cs | 74 +++++++++++-------- .../Portable/Binder/LocalBinderFactory.cs | 40 ++++++---- 2 files changed, 69 insertions(+), 45 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index e75a3af2e672c..12cfb37da30c1 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -2497,45 +2497,61 @@ private void GenerateImplicitConversionErrorsForTupleLiteralArguments( #nullable enable private BoundStatement BindIfStatement(IfStatementSyntax node, BindingDiagnosticBag diagnostics) { - var stack = ArrayBuilder<(IfStatementSyntax IfStatementSyntax, BoundExpression Condition, BoundStatement Consequence)>.GetInstance(); + return bindIfStatement(this, node, diagnostics); - BoundStatement? alternative; - while (true) + static BoundStatement bindIfStatement(Binder binder, IfStatementSyntax node, BindingDiagnosticBag diagnostics) { - var condition = BindBooleanExpression(node.Condition, diagnostics); - var consequence = BindPossibleEmbeddedStatement(node.Statement, diagnostics); - stack.Push((node, condition, consequence)); + var stack = ArrayBuilder<(Binder, IfStatementSyntax IfStatementSyntax, BoundExpression Condition, BoundStatement Consequence, bool WrapWithVariables)>.GetInstance(); - if (node.Else == null) + BoundStatement? alternative; + bool wrapWithVariables = false; + while (true) { - alternative = null; - break; - } + var condition = binder.BindBooleanExpression(node.Condition, diagnostics); + var consequence = binder.BindPossibleEmbeddedStatement(node.Statement, diagnostics); + stack.Push((binder, node, condition, consequence, wrapWithVariables)); - var elseStatementSyntax = node.Else.Statement; - if (elseStatementSyntax is IfStatementSyntax ifStatementSyntax) - { - node = ifStatementSyntax; + 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; + wrapWithVariables = true; + } + else + { + alternative = binder.BindPossibleEmbeddedStatement(elseStatementSyntax, diagnostics); + break; + } } - else + + BoundStatement result; + do { - alternative = BindPossibleEmbeddedStatement(elseStatementSyntax, diagnostics); - break; + BoundExpression condition; + BoundStatement consequence; + (binder, node, condition, consequence, wrapWithVariables) = stack.Pop(); + result = new BoundIfStatement(node, condition, consequence, alternative); + if (wrapWithVariables) + { + result = binder.WrapWithVariablesIfAny(node, result); + } + alternative = result; } - } + while (stack.Any()); - BoundStatement result; - do - { - var (ifStatementSyntax, condition, consequence) = stack.Pop(); - result = new BoundIfStatement(ifStatementSyntax, condition, consequence, alternative); - alternative = result; - } - while (stack.Any()); + stack.Free(); - stack.Free(); - - return result; + return result; + } } #nullable disable diff --git a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs index ee3eb7ba137e3..9a2e9e9ae4234 100644 --- a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs +++ b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs @@ -798,10 +798,11 @@ public override void VisitSwitchExpression(SwitchExpressionSyntax node) public override void VisitIfStatement(IfStatementSyntax node) { + Binder enclosing = _enclosing; while (true) { - Visit(node.Condition, _enclosing); - VisitPossibleEmbeddedStatement(node.Statement, _enclosing); + Visit(node.Condition, enclosing); + VisitPossibleEmbeddedStatement(node.Statement, enclosing); if (node.Else == null) { @@ -812,10 +813,11 @@ public override void VisitIfStatement(IfStatementSyntax node) if (elseStatementSyntax is IfStatementSyntax ifStatementSyntax) { node = ifStatementSyntax; + enclosing = GetBinderForPossibleEmbeddedStatement(node, enclosing); } else { - VisitPossibleEmbeddedStatement(elseStatementSyntax, _enclosing); + VisitPossibleEmbeddedStatement(elseStatementSyntax, enclosing); break; } } @@ -1037,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); } } From b705b76ed3e0bf58d7887c1399388accaa0f2f84 Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Mon, 15 Jul 2024 17:32:10 -0700 Subject: [PATCH 03/23] Fix tests --- src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs index c09429d6f1809..3051b82ab552a 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs @@ -137,7 +137,7 @@ private void EmitStatementList(BoundStatementList list) for (int n = list.Statements.Length; i < n; i++) { var statement = list.Statements[i]; - if (statement is BoundSequencePointWithSpan { StatementOpt: BoundStatementList nestedList } nestedSequencePointWithSpan) + if (statement is BoundSequencePointWithSpan { StatementOpt: BoundStatementList { Kind: BoundKind.StatementList } nestedList } nestedSequencePointWithSpan) { EmitSequencePointStatementBegin(nestedSequencePointWithSpan); stack.Push((list, i + 1, sequencePointWithSpan, instructionsEmittedBegin)); From e7d2673ee4342cbc4b75d51bc6d1786aeb9fcdaf Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Sat, 21 Sep 2024 14:14:47 -0700 Subject: [PATCH 04/23] Simplify logic in ControlFlowGraph builder and Binder --- .../Portable/Binder/Binder_Statements.cs | 10 +- .../IOperationTests_IIfStatement.cs | 149 ++++++++++++++++++ .../Operations/ControlFlowGraphBuilder.cs | 98 +++++------- 3 files changed, 195 insertions(+), 62 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs index 34a80b8c5d4b4..15cba6704b579 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @@ -2501,15 +2501,14 @@ private BoundStatement BindIfStatement(IfStatementSyntax node, BindingDiagnostic static BoundStatement bindIfStatement(Binder binder, IfStatementSyntax node, BindingDiagnosticBag diagnostics) { - var stack = ArrayBuilder<(Binder, IfStatementSyntax IfStatementSyntax, BoundExpression Condition, BoundStatement Consequence, bool WrapWithVariables)>.GetInstance(); + var stack = ArrayBuilder<(Binder, IfStatementSyntax IfStatementSyntax, BoundExpression Condition, BoundStatement Consequence)>.GetInstance(); BoundStatement? alternative; - bool wrapWithVariables = false; while (true) { var condition = binder.BindBooleanExpression(node.Condition, diagnostics); var consequence = binder.BindPossibleEmbeddedStatement(node.Statement, diagnostics); - stack.Push((binder, node, condition, consequence, wrapWithVariables)); + stack.Push((binder, node, condition, consequence)); if (node.Else == null) { @@ -2524,7 +2523,6 @@ static BoundStatement bindIfStatement(Binder binder, IfStatementSyntax node, Bin Debug.Assert(b != null); binder = b; node = ifStatementSyntax; - wrapWithVariables = true; } else { @@ -2538,9 +2536,9 @@ static BoundStatement bindIfStatement(Binder binder, IfStatementSyntax node, Bin { BoundExpression condition; BoundStatement consequence; - (binder, node, condition, consequence, wrapWithVariables) = stack.Pop(); + (binder, node, condition, consequence) = stack.Pop(); result = new BoundIfStatement(node, condition, consequence, alternative); - if (wrapWithVariables) + if (stack.Any()) { result = binder.WrapWithVariablesIfAny(node, result); } diff --git a/src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIfStatement.cs b/src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIfStatement.cs index a515cc28f9774..30dea08887385 100644 --- a/src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIfStatement.cs +++ b/src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IIfStatement.cs @@ -3702,5 +3702,154 @@ void M(dynamic a, bool result) VerifyFlowGraphAndDiagnosticsForTest(source, expectedFlowGraph, expectedDiagnostics); } + + [CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)] + [Fact] + public void IfFlow_25() + { + string source = @" +class P +{ + void M(bool a, bool b) +/**/{ + if (a) + { + a = false; + } + else if (b) + { + b = false; + } + }/**/ +} +"; + string expectedFlowGraph = @" +Block[B0] - Entry + Statements (0) + Next (Regular) Block[B1] +Block[B1] - Block + Predecessors: [B0] + Statements (0) + Jump if False (Regular) to Block[B3] + IParameterReferenceOperation: a (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'a') + Next (Regular) Block[B2] +Block[B2] - Block + Predecessors: [B1] + Statements (1) + IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'a = false;') + Expression: + ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Boolean) (Syntax: 'a = false') + Left: + IParameterReferenceOperation: a (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'a') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Boolean, Constant: False) (Syntax: 'false') + Next (Regular) Block[B5] +Block[B3] - Block + Predecessors: [B1] + Statements (0) + Jump if False (Regular) to Block[B5] + IParameterReferenceOperation: b (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'b') + Next (Regular) Block[B4] +Block[B4] - Block + Predecessors: [B3] + Statements (1) + IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'b = false;') + Expression: + ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Boolean) (Syntax: 'b = false') + Left: + IParameterReferenceOperation: b (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'b') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Boolean, Constant: False) (Syntax: 'false') + Next (Regular) Block[B5] +Block[B5] - Exit + Predecessors: [B2] [B3] [B4] + Statements (0) +"; + var expectedDiagnostics = DiagnosticDescription.None; + + VerifyFlowGraphAndDiagnosticsForTest(source, expectedFlowGraph, expectedDiagnostics); + } + + [CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)] + [Fact] + public void IfFlow_26() + { + string source = @" +class P +{ + void M(bool a, bool b, bool c) +/**/{ + if (a) + { + a = false; + } + else if (b) + { + b = false; + } + else + { + c = false; + } + }/**/ +} +"; + string expectedFlowGraph = @" +Block[B0] - Entry + Statements (0) + Next (Regular) Block[B1] +Block[B1] - Block + Predecessors: [B0] + Statements (0) + Jump if False (Regular) to Block[B3] + IParameterReferenceOperation: a (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'a') + Next (Regular) Block[B2] +Block[B2] - Block + Predecessors: [B1] + Statements (1) + IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'a = false;') + Expression: + ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Boolean) (Syntax: 'a = false') + Left: + IParameterReferenceOperation: a (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'a') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Boolean, Constant: False) (Syntax: 'false') + Next (Regular) Block[B6] +Block[B3] - Block + Predecessors: [B1] + Statements (0) + Jump if False (Regular) to Block[B5] + IParameterReferenceOperation: b (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'b') + Next (Regular) Block[B4] +Block[B4] - Block + Predecessors: [B3] + Statements (1) + IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'b = false;') + Expression: + ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Boolean) (Syntax: 'b = false') + Left: + IParameterReferenceOperation: b (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'b') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Boolean, Constant: False) (Syntax: 'false') + Next (Regular) Block[B6] +Block[B5] - Block + Predecessors: [B3] + Statements (1) + IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'c = false;') + Expression: + ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: System.Boolean) (Syntax: 'c = false') + Left: + IParameterReferenceOperation: c (OperationKind.ParameterReference, Type: System.Boolean) (Syntax: 'c') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Boolean, Constant: False) (Syntax: 'false') + Next (Regular) Block[B6] +Block[B6] - Exit + Predecessors: [B2] [B4] [B5] + Statements (0) +"; + var expectedDiagnostics = DiagnosticDescription.None; + + VerifyFlowGraphAndDiagnosticsForTest(source, expectedFlowGraph, expectedDiagnostics); + } } } diff --git a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs index dfea7bc0d9a91..cd100a2a680a6 100644 --- a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs @@ -1536,73 +1536,59 @@ private void VisitMethodBodies(IBlockOperation? blockBody, IBlockOperation? expr { if (operation == _currentStatement) { - if (operation.WhenFalse == null) - { - // if (condition) - // consequence; - // - // becomes - // - // GotoIfFalse condition afterif; - // consequence; - // afterif: + // if (condition) + // consequence; + // + // becomes + // + // GotoIfFalse condition afterif; + // consequence; + // afterif: - BasicBlockBuilder? afterIf = null; - VisitConditionalBranch(operation.Condition, ref afterIf, jumpIfTrue: false); - VisitStatement(operation.WhenTrue); - AppendNewBlock(afterIf); - } - else - { - // if (condition) - // consequence; - // else - // alternative - // - // becomes - // - // GotoIfFalse condition alt; - // consequence - // goto afterif; - // alt: - // alternative; - // afterif: - var stack = ArrayBuilder<(IConditionalOperation, BasicBlockBuilder)>.GetInstance(); + // if (condition) + // consequence; + // else + // alternative + // + // becomes + // + // GotoIfFalse condition alt; + // consequence + // goto afterif; + // alt: + // alternative; + // afterif: - while (true) - { - BasicBlockBuilder? whenFalse = null; - VisitConditionalBranch(operation.Condition, ref whenFalse, jumpIfTrue: false); - Debug.Assert(whenFalse is { }); - VisitStatement(operation.WhenTrue); - var afterIf = new BasicBlockBuilder(BasicBlockKind.Block); - UnconditionalBranch(afterIf); + var afterIf = new BasicBlockBuilder(BasicBlockKind.Block); - AppendNewBlock(whenFalse); - stack.Push((operation, afterIf)); + while (true) + { + BasicBlockBuilder? whenFalse = null; + VisitConditionalBranch(operation.Condition, ref whenFalse, jumpIfTrue: false); + Debug.Assert(whenFalse is { }); + VisitStatement(operation.WhenTrue); + UnconditionalBranch(afterIf); - if (operation.WhenFalse is IConditionalOperation { WhenFalse: not null } nested) - { - operation = nested; - } - else - { - break; - } - } + AppendNewBlock(whenFalse); - VisitStatement(operation.WhenFalse); - do + if (operation.WhenFalse is IConditionalOperation nested) + { + operation = nested; + } + else { - var (conditional, afterIf) = stack.Pop(); - AppendNewBlock(afterIf); + break; } - while (stack.Any()); + } - stack.Free(); + if (operation.WhenFalse is not null) + { + VisitStatement(operation.WhenFalse); } + AppendNewBlock(afterIf); + return null; } else From ac712a1040a2be0f7618f224d6507fdb0073a903 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Sat, 21 Sep 2024 18:03:17 -0700 Subject: [PATCH 05/23] Fix a couple of visitors --- .../Compilation/MemberSemanticModel.NodeMapBuilder.cs | 4 ++++ .../CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs | 9 +++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs index a5edc38fd4de9..5c4b3fcc59247 100644 --- a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs +++ b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs @@ -301,6 +301,10 @@ public override BoundNode VisitIfStatement(BoundIfStatement node) if (alternative is BoundIfStatement elseIfStatement) { node = elseIfStatement; + if (ShouldAddNode(node)) + { + _map.Add(node.Syntax, node); + } } else { diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index efec31fbb1727..a7287b2403039 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -1708,7 +1708,7 @@ public override BoundNode VisitIfStatement(BoundIfStatement node) { // 5.3.3.5 If statements - var stack = ArrayBuilder.GetInstance(); + var stack = ArrayBuilder<(TLocalState, BoundIfStatement)>.GetInstance(); TLocalState trueState; while (true) @@ -1727,11 +1727,11 @@ public override BoundNode VisitIfStatement(BoundIfStatement node) break; } - SetState(falseState); if (alternative is BoundIfStatement elseIfStatement) { - stack.Push(trueState); node = elseIfStatement; + stack.Push((trueState, node)); + EnterRegionIfNeeded(node); } else { @@ -1747,7 +1747,8 @@ public override BoundNode VisitIfStatement(BoundIfStatement node) { break; } - trueState = stack.Pop(); + (trueState, node) = stack.Pop(); + LeaveRegionIfNeeded(node); } stack.Free(); From aa4fdc81971170ba8fe25f36e085f033e6961d16 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 08:56:33 -0700 Subject: [PATCH 06/23] Formatting --- .../Core/Portable/Operations/ControlFlowGraphBuilder.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs index cd100a2a680a6..4a2c0db260781 100644 --- a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs @@ -1545,7 +1545,6 @@ private void VisitMethodBodies(IBlockOperation? blockBody, IBlockOperation? expr // consequence; // afterif: - // if (condition) // consequence; // else From e64b41854928da85f321cf49b5cd0d40d3152af1 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 09:50:03 -0700 Subject: [PATCH 07/23] Fixup ParseIfStatement --- .../CSharp/Portable/Parser/LanguageParser.cs | 28 ++++++++++++++----- .../CSharp/Test/EndToEnd/EndToEndTests.cs | 2 +- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs index 6b686145d264f..c726c423143e1 100644 --- a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs +++ b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs @@ -7938,9 +7938,9 @@ or SyntaxKind.MinusMinusToken /// Those will instead be parsed out as script-fields/methods. private StatementSyntax ParseStatementCore(SyntaxList attributes, bool isGlobal) { - if (canReuseStatement(attributes, isGlobal)) + if (TryReuseStatement(attributes, isGlobal) is { } reused) { - return (StatementSyntax)this.EatNode(); + return reused; } ResetPoint resetPointBeforeStatement = this.GetResetPoint(); @@ -8016,6 +8016,16 @@ private StatementSyntax ParseStatementCore(SyntaxList attri _recursionDepth--; this.Release(ref resetPointBeforeStatement); } + } + + private StatementSyntax TryReuseStatement(SyntaxList attributes, bool isGlobal) + { + if (canReuseStatement(attributes, isGlobal)) + { + return (StatementSyntax)this.EatNode(); + } + + return null; bool canReuseStatement(SyntaxList attributes, bool isGlobal) { @@ -9537,7 +9547,7 @@ private IfStatementSyntax ParseIfStatement(SyntaxList attri { Debug.Assert(this.CurrentToken.Kind == SyntaxKind.IfKeyword); - var stack = ArrayBuilder<(SyntaxList, SyntaxToken, SyntaxToken, ExpressionSyntax, SyntaxToken, StatementSyntax, SyntaxToken)>.GetInstance(); + var stack = ArrayBuilder<(SyntaxToken, SyntaxToken, ExpressionSyntax, SyntaxToken, StatementSyntax, SyntaxToken)>.GetInstance(); StatementSyntax alternative = null; while (true) @@ -9551,7 +9561,7 @@ private IfStatementSyntax ParseIfStatement(SyntaxList attri var elseKeyword = this.CurrentToken.Kind != SyntaxKind.ElseKeyword ? null : this.EatToken(SyntaxKind.ElseKeyword); - stack.Push((attributes, ifKeyword, openParen, condition, closeParen, consequence, elseKeyword)); + stack.Push((ifKeyword, openParen, condition, closeParen, consequence, elseKeyword)); if (elseKeyword is null) { @@ -9565,20 +9575,24 @@ private IfStatementSyntax ParseIfStatement(SyntaxList attri break; } - attributes = default; + alternative = TryReuseStatement(attributes: default, isGlobal: false); + if (alternative is not null) + { + break; + } } IfStatementSyntax ifStatement; do { - var (attr, ifKeyword, openParen, condition, closeParen, consequence, elseKeyword) = stack.Pop(); + var (ifKeyword, openParen, condition, closeParen, consequence, elseKeyword) = stack.Pop(); var elseClause = alternative is null ? null : _syntaxFactory.ElseClause( elseKeyword, alternative); ifStatement = _syntaxFactory.IfStatement( - attr, + attributeLists: stack.Any() ? default : attributes, ifKeyword, openParen, condition, diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 9fc40f7fc40b3..557f234d2f35d 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -411,7 +411,7 @@ public void NestedIfStatements() int nestingLevel = (IntPtr.Size, ExecutionConditionUtil.Configuration) switch { (4, ExecutionConfiguration.Debug) => 310, - (4, ExecutionConfiguration.Release) => 1650, + (4, ExecutionConfiguration.Release) => 1419, (8, ExecutionConfiguration.Debug) => 200, (8, ExecutionConfiguration.Release) => 780, _ => throw new Exception($"Unexpected configuration {IntPtr.Size * 8}-bit {ExecutionConditionUtil.Configuration}") From 61dd4c5e277f1c9c19ea0dfdd193e74b5e5ce690 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 09:55:18 -0700 Subject: [PATCH 08/23] Move new test --- .../CSharp/Test/Emit/CodeGen/CodeGenTests.cs | 85 ------------------- .../CSharp/Test/EndToEnd/EndToEndTests.cs | 85 +++++++++++++++++++ 2 files changed, 85 insertions(+), 85 deletions(-) diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs index fe5c463e74f10..6d9cb9eb9b160 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs @@ -17325,90 +17325,5 @@ .maxstack 1 } "); } - - [WorkItem("https://github.com/dotnet/roslyn/issues/72393")] - [Theory] - [InlineData(2)] - [InlineData(5000)] - public void NestedIfElse(int n) - { - var builder = new System.Text.StringBuilder(); - builder.AppendLine(""" - #nullable enable - class Program - { - static void F(int i) - { - if (i == 0) { } - """); - for (int i = 0; i < n; i++) - { - builder.AppendLine($$""" - else if (i == {{i}}) { } - """); - } - builder.AppendLine(""" - } - } - """); - - var source = builder.ToString(); - var comp = CreateCompilation(source); - comp.VerifyEmitDiagnostics(); - - var tree = comp.SyntaxTrees.Single(); - var model = comp.GetSemanticModel(tree); - var node = tree.GetRoot().DescendantNodes().OfType().Single(); - - // Avoid using ControlFlowGraphVerifier.GetControlFlowGraph() since that calls - // TestOperationVisitor.VerifySubTree() which has quadratic behavior using - // MemberSemanticModel.GetEnclosingBinderInternalWithinRoot(). - var operation = (Microsoft.CodeAnalysis.Operations.IMethodBodyOperation)model.GetOperation(node); - var graph = Microsoft.CodeAnalysis.FlowAnalysis.ControlFlowGraph.Create(operation); - - if (n == 2) - { - var symbol = model.GetDeclaredSymbol(node); - ControlFlowGraphVerifier.VerifyGraph(comp, """ - Block[B0] - Entry - Statements (0) - Next (Regular) Block[B1] - Block[B1] - Block - Predecessors: [B0] - Statements (0) - Jump if False (Regular) to Block[B2] - IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 0') - Left: - IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') - Right: - ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 0) (Syntax: '0') - Next (Regular) Block[B4] - Block[B2] - Block - Predecessors: [B1] - Statements (0) - Jump if False (Regular) to Block[B3] - IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 0') - Left: - IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') - Right: - ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 0) (Syntax: '0') - Next (Regular) Block[B4] - Block[B3] - Block - Predecessors: [B2] - Statements (0) - Jump if False (Regular) to Block[B4] - IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 1') - Left: - IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') - Right: - ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1') - Next (Regular) Block[B4] - Block[B4] - Exit - Predecessors: [B1] [B2] [B3*2] - Statements (0) - """, - graph, symbol); - } - } } } diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 557f234d2f35d..baec99df253ef 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -450,6 +450,91 @@ static void Main() } } + [WorkItem("https://github.com/dotnet/roslyn/issues/72393")] + [Theory] + [InlineData(2)] + [InlineData(5000)] + public void NestedIfElse(int n) + { + var builder = new System.Text.StringBuilder(); + builder.AppendLine(""" + #nullable enable + class Program + { + static void F(int i) + { + if (i == 0) { } + """); + for (int i = 0; i < n; i++) + { + builder.AppendLine($$""" + else if (i == {{i}}) { } + """); + } + builder.AppendLine(""" + } + } + """); + + var source = builder.ToString(); + var comp = CreateCompilation(source); + comp.VerifyEmitDiagnostics(); + + var tree = comp.SyntaxTrees.Single(); + var model = comp.GetSemanticModel(tree); + var node = tree.GetRoot().DescendantNodes().OfType().Single(); + + // Avoid using ControlFlowGraphVerifier.GetControlFlowGraph() since that calls + // TestOperationVisitor.VerifySubTree() which has quadratic behavior using + // MemberSemanticModel.GetEnclosingBinderInternalWithinRoot(). + var operation = (Microsoft.CodeAnalysis.Operations.IMethodBodyOperation)model.GetOperation(node); + var graph = Microsoft.CodeAnalysis.FlowAnalysis.ControlFlowGraph.Create(operation); + + if (n == 2) + { + var symbol = model.GetDeclaredSymbol(node); + ControlFlowGraphVerifier.VerifyGraph(comp, """ + Block[B0] - Entry + Statements (0) + Next (Regular) Block[B1] + Block[B1] - Block + Predecessors: [B0] + Statements (0) + Jump if False (Regular) to Block[B2] + IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 0') + Left: + IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 0) (Syntax: '0') + Next (Regular) Block[B4] + Block[B2] - Block + Predecessors: [B1] + Statements (0) + Jump if False (Regular) to Block[B3] + IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 0') + Left: + IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 0) (Syntax: '0') + Next (Regular) Block[B4] + Block[B3] - Block + Predecessors: [B2] + Statements (0) + Jump if False (Regular) to Block[B4] + IBinaryOperation (BinaryOperatorKind.Equals) (OperationKind.Binary, Type: System.Boolean) (Syntax: 'i == 1') + Left: + IParameterReferenceOperation: i (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'i') + Right: + ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1') + Next (Regular) Block[B4] + Block[B4] - Exit + Predecessors: [B1] [B2] [B3*2] + Statements (0) + """, + graph, symbol); + } + } + [WorkItem(42361, "https://github.com/dotnet/roslyn/issues/42361")] [ConditionalFact(typeof(WindowsOrLinuxOnly))] public void Constraints() From c8578eb3e51e3cf6f9b8a7477e95fcb1f10e776f Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 10:00:58 -0700 Subject: [PATCH 09/23] Don't run new test in IOperation leg --- src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index baec99df253ef..58872d811cf9f 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -451,7 +451,7 @@ static void Main() } [WorkItem("https://github.com/dotnet/roslyn/issues/72393")] - [Theory] + [ConditionalTheory(typeof(NoIOperationValidation))] [InlineData(2)] [InlineData(5000)] public void NestedIfElse(int n) From f79507199da07e43c546b1f7975602c8ddfa4e8b Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 10:50:01 -0700 Subject: [PATCH 10/23] Revert using chnages --- src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs index 6d9cb9eb9b160..3ae7ccf2a7ec9 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs @@ -11,7 +11,6 @@ using Basic.Reference.Assemblies; using Microsoft.CodeAnalysis.CSharp.Emit; using Microsoft.CodeAnalysis.CSharp.Symbols; -using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.CSharp.Test.Utilities; using Microsoft.CodeAnalysis.Emit; using Microsoft.CodeAnalysis.Test.Utilities; From 5ac169159742730dcd67cb923f693e946e883064 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 11:56:15 -0700 Subject: [PATCH 11/23] Adjust nesting depth expectations --- src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 58872d811cf9f..dde5260b4796c 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -413,7 +413,7 @@ public void NestedIfStatements() (4, ExecutionConfiguration.Debug) => 310, (4, ExecutionConfiguration.Release) => 1419, (8, ExecutionConfiguration.Debug) => 200, - (8, ExecutionConfiguration.Release) => 780, + (8, ExecutionConfiguration.Release) => 479, _ => throw new Exception($"Unexpected configuration {IntPtr.Size * 8}-bit {ExecutionConditionUtil.Configuration}") }; @@ -453,7 +453,11 @@ static void Main() [WorkItem("https://github.com/dotnet/roslyn/issues/72393")] [ConditionalTheory(typeof(NoIOperationValidation))] [InlineData(2)] +#if DEBUG + [InlineData(2000)] +#else [InlineData(5000)] +#endif public void NestedIfElse(int n) { var builder = new System.Text.StringBuilder(); From 771e40649ae58fc8baf82c3cfaa9c2374a6d9fbb Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 17:50:51 -0700 Subject: [PATCH 12/23] Add IL verification --- .../DynamicInstrumentationTests.cs | 74 ++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs b/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs index 026652a9acfc7..43e89f3ed63a3 100644 --- a/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs +++ b/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs @@ -3094,7 +3094,79 @@ static void Test() Method1: x = 0 " + checker.ExpectedOutput; - var verifier = CompileAndVerify(source, expectedOutput, options: TestOptions.ReleaseExe); + var verifier = CompileAndVerify(source, expectedOutput: null, options: TestOptions.ReleaseExe); + verifier.VerifyIL("Class1.Method1", +@" +{ + // Code size 138 (0x8a) + .maxstack 5 + .locals init (bool[] V_0) + IL_0000: ldsfld ""bool[][] .PayloadRoot0"" + IL_0005: ldtoken ""void Class1.Method1(int)"" + IL_000a: ldelem.ref + IL_000b: stloc.0 + IL_000c: ldloc.0 + IL_000d: brtrue.s IL_0034 + IL_000f: ldsfld ""System.Guid .MVID"" + IL_0014: ldtoken ""void Class1.Method1(int)"" + IL_0019: ldtoken Source Document 0 + IL_001e: ldsfld ""bool[][] .PayloadRoot0"" + IL_0023: ldtoken ""void Class1.Method1(int)"" + IL_0028: ldelema ""bool[]"" + IL_002d: ldc.i4.7 + IL_002e: call ""bool[] Microsoft.CodeAnalysis.Runtime.Instrumentation.CreatePayload(System.Guid, int, int, ref bool[], int)"" + IL_0033: stloc.0 + IL_0034: ldloc.0 + IL_0035: ldc.i4.0 + IL_0036: ldc.i4.1 + IL_0037: stelem.i1 + IL_0038: ldloc.0 + IL_0039: ldc.i4.1 + IL_003a: ldc.i4.1 + IL_003b: stelem.i1 + IL_003c: ldstr ""Method1: x = {0}"" + IL_0041: ldarg.1 + IL_0042: box ""int"" + IL_0047: call ""string string.Format(string, object)"" + IL_004c: call ""void System.Console.WriteLine(string)"" + IL_0051: ldloc.0 + IL_0052: ldc.i4.6 + IL_0053: ldc.i4.1 + IL_0054: stelem.i1 + IL_0055: ldarg.1 + IL_0056: ldc.i4.0 + IL_0057: ble.s IL_0073 + IL_0059: ldloc.0 + IL_005a: ldc.i4.2 + IL_005b: ldc.i4.1 + IL_005c: stelem.i1 + IL_005d: ldstr ""Method1: x > 0"" + IL_0062: call ""void System.Console.WriteLine(string)"" + IL_0067: ldloc.0 + IL_0068: ldc.i4.3 + IL_0069: ldc.i4.1 + IL_006a: stelem.i1 + IL_006b: ldarg.0 + IL_006c: ldc.i4.0 + IL_006d: call ""void Class1.Method1(int)"" + IL_0072: ret + IL_0073: ldloc.0 + IL_0074: ldc.i4.5 + IL_0075: ldc.i4.1 + IL_0076: stelem.i1 + IL_0077: ldarg.1 + IL_0078: ldc.i4.0 + IL_0079: bge.s IL_0089 + IL_007b: ldloc.0 + IL_007c: ldc.i4.4 + IL_007d: ldc.i4.1 + IL_007e: stelem.i1 + IL_007f: ldstr ""Method1: x < 0"" + IL_0084: call ""void System.Console.WriteLine(string)"" + IL_0089: ret +} +"); + checker.CompleteCheck(verifier.Compilation, source); verifier.VerifyDiagnostics(); From 17dfd90ed828d84b96747bbaf3359a212293720e Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Tue, 24 Sep 2024 18:56:13 -0700 Subject: [PATCH 13/23] Adjust LocalRewriter.VisitIfStatement --- .../CSharp/Portable/CodeGen/CodeGenerator.cs | 4 +- .../CSharp/Portable/CodeGen/EmitStatement.cs | 30 +----- .../CSharp/Portable/CodeGen/Optimizer.cs | 25 +---- .../Portable/Compiler/MethodCompiler.cs | 2 - .../CodeCoverageInstrumenter.cs | 4 +- .../Instrumentation/CompoundInstrumenter.cs | 4 +- .../Instrumentation/DebugInfoInjector.cs | 4 +- .../Lowering/Instrumentation/Instrumenter.cs | 2 +- .../LocalRewriter_IfStatement.cs | 94 ++++++++++++++----- 9 files changed, 80 insertions(+), 89 deletions(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs index a55b6577b0c02..9ac975d349644 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs @@ -140,9 +140,9 @@ public CodeGenerator( debugFriendly: _ilEmitStyle != ILEmitStyle.Release, stackLocals: out _stackLocals); } - catch (BoundTreeVisitor.CancelledByStackGuardException) + catch (BoundTreeVisitor.CancelledByStackGuardException ex) { - //ex.AddAnError(diagnostics); // PROTOTYPE: Revert the changes to CodeGenerator and Optimizer and instead change VisitStatementList(). + ex.AddAnError(diagnostics); _boundBody = boundBody; } diff --git a/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs b/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs index 3051b82ab552a..107a8f02f2481 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs @@ -124,36 +124,10 @@ private int EmitStatementAndCountInstructions(BoundStatement statement) private void EmitStatementList(BoundStatementList list) { - var stack = ArrayBuilder<(BoundStatementList, int, BoundSequencePointWithSpan, int)>.GetInstance(); - - stack.Push((list, 0, null, 0)); - - while (stack.Any()) + for (int i = 0, n = list.Statements.Length; i < n; i++) { - int i; - BoundSequencePointWithSpan sequencePointWithSpan; - int instructionsEmittedBegin; - (list, i, sequencePointWithSpan, instructionsEmittedBegin) = stack.Pop(); - for (int n = list.Statements.Length; i < n; i++) - { - var statement = list.Statements[i]; - if (statement is BoundSequencePointWithSpan { StatementOpt: BoundStatementList { Kind: BoundKind.StatementList } nestedList } nestedSequencePointWithSpan) - { - EmitSequencePointStatementBegin(nestedSequencePointWithSpan); - stack.Push((list, i + 1, sequencePointWithSpan, instructionsEmittedBegin)); - stack.Push((nestedList, 0, nestedSequencePointWithSpan, _builder.InstructionsEmitted)); - goto endOfLoop; - } - EmitStatement(statement); - } - if (sequencePointWithSpan is { }) - { - EmitSequencePointStatementEnd(sequencePointWithSpan, _builder.InstructionsEmitted - instructionsEmittedBegin); - } -endOfLoop: continue; + EmitStatement(list.Statements[i]); } - - stack.Free(); } private void EmitNoOpStatement(BoundNoOpStatement statement) diff --git a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs index 4bde3fbb1de10..5f7659f3ae3ce 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs @@ -566,30 +566,7 @@ private void PopEvalStack() public BoundNode VisitStatement(BoundNode node) { Debug.Assert(node == null || EvalStackIsEmpty()); - - _recursionDepth++; - - BoundNode result; - if (_recursionDepth > 1) - { - StackGuard.EnsureSufficientExecutionStack(_recursionDepth); - - result = VisitSideEffect(node); - } - else - { - try - { - result = VisitSideEffect(node); - } - catch (InsufficientExecutionStackException ex) - { - throw new CancelledByStackGuardException(ex, node); - } - } - - _recursionDepth--; - return result; + return VisitSideEffect(node); } public BoundNode VisitSideEffect(BoundNode node) diff --git a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 5be3d0d406d2a..10dd7660b3d76 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -1448,7 +1448,6 @@ internal static BoundStatement LowerBodyOrInitializer( return bodyWithoutLambdas; } - // PROTOTYPE: What about IteratorRewriter? BoundStatement bodyWithoutIterators = IteratorRewriter.Rewrite(bodyWithoutLambdas, method, methodOrdinal, stateMachineStateDebugInfoBuilder, lazyVariableSlotAllocator, compilationState, diagnostics, out IteratorStateMachine iteratorStateMachine); @@ -1457,7 +1456,6 @@ internal static BoundStatement LowerBodyOrInitializer( return bodyWithoutIterators; } - // PROTOTYPE: What about AsyncRewriter? BoundStatement bodyWithoutAsync = AsyncRewriter.Rewrite(bodyWithoutIterators, method, methodOrdinal, stateMachineStateDebugInfoBuilder, lazyVariableSlotAllocator, compilationState, diagnostics, out AsyncStateMachine asyncStateMachine); diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CodeCoverageInstrumenter.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CodeCoverageInstrumenter.cs index 7f0e79edfd813..5c57cc091dab8 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CodeCoverageInstrumenter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CodeCoverageInstrumenter.cs @@ -371,9 +371,9 @@ public override BoundStatement InstrumentForEachStatementDeconstructionVariables return AddDynamicAnalysis(original, base.InstrumentForEachStatementDeconstructionVariablesDeclaration(original, iterationVarDecl)); } - public override BoundStatement InstrumentIfStatement(BoundIfStatement original, BoundStatement rewritten) + public override BoundStatement InstrumentIfStatementConditionalGoto(BoundIfStatement original, BoundStatement rewritten) { - return AddDynamicAnalysis(original, base.InstrumentIfStatement(original, rewritten)); + return AddDynamicAnalysis(original, base.InstrumentIfStatementConditionalGoto(original, rewritten)); } public override BoundStatement InstrumentWhileStatementConditionalGotoStartOrBreak(BoundWhileStatement original, BoundStatement ifConditionGotoStart) diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CompoundInstrumenter.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CompoundInstrumenter.cs index 82f5dda4cb4b9..289614c47adf9 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CompoundInstrumenter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/CompoundInstrumenter.cs @@ -131,9 +131,9 @@ public override BoundExpression InstrumentForStatementCondition(BoundForStatemen return Previous.InstrumentForStatementCondition(original, rewrittenCondition, factory); } - public override BoundStatement InstrumentIfStatement(BoundIfStatement original, BoundStatement rewritten) + public override BoundStatement InstrumentIfStatementConditionalGoto(BoundIfStatement original, BoundStatement rewritten) { - return Previous.InstrumentIfStatement(original, rewritten); + return Previous.InstrumentIfStatementConditionalGoto(original, rewritten); } public override BoundExpression InstrumentIfStatementCondition(BoundIfStatement original, BoundExpression rewrittenCondition, SyntheticBoundNodeFactory factory) diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs index 5f12229415f08..a9fabf63b6b51 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DebugInfoInjector.cs @@ -355,12 +355,12 @@ public override BoundExpression InstrumentForStatementCondition(BoundForStatemen return AddConditionSequencePoint(base.InstrumentForStatementCondition(original, rewrittenCondition, factory), original.Syntax, factory); } - public override BoundStatement InstrumentIfStatement(BoundIfStatement original, BoundStatement rewritten) + public override BoundStatement InstrumentIfStatementConditionalGoto(BoundIfStatement original, BoundStatement rewritten) { var syntax = (IfStatementSyntax)original.Syntax; return new BoundSequencePointWithSpan( syntax, - base.InstrumentIfStatement(original, rewritten), + base.InstrumentIfStatementConditionalGoto(original, rewritten), TextSpan.FromBounds( syntax.IfKeyword.SpanStart, syntax.CloseParenToken.Span.End), diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/Instrumenter.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/Instrumenter.cs index 561559771f6c2..0114fd9055ac3 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/Instrumenter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/Instrumenter.cs @@ -189,7 +189,7 @@ public virtual BoundExpression InstrumentForStatementCondition(BoundForStatement return rewrittenCondition; } - public virtual BoundStatement InstrumentIfStatement(BoundIfStatement original, BoundStatement rewritten) + public virtual BoundStatement InstrumentIfStatementConditionalGoto(BoundIfStatement original, BoundStatement rewritten) { Debug.Assert(original.Syntax.Kind() == SyntaxKind.IfStatement); return InstrumentStatement(original, rewritten); diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs index 45e2fa389b21f..5315bccd1cba4 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs @@ -15,63 +15,105 @@ public override BoundNode VisitIfStatement(BoundIfStatement node) { Debug.Assert(node != null); - var stack = ArrayBuilder<(BoundIfStatement, BoundExpression, BoundStatement)>.GetInstance(); + var stack = ArrayBuilder<(BoundIfStatement, GeneratedLabelSymbol, int)>.GetInstance(); + var builder = ArrayBuilder.GetInstance(); - BoundStatement? rewrittenAlternative; while (true) { var rewrittenCondition = VisitExpression(node.Condition); var rewrittenConsequence = VisitStatement(node.Consequence); Debug.Assert(rewrittenConsequence is { }); - stack.Push((node, rewrittenCondition, rewrittenConsequence)); - var alternative = node.AlternativeOpt; - if (alternative is null) + // EnC: We need to insert a hidden sequence point to handle function remapping in case + // the containing method is edited while methods invoked in the condition are being executed. + if (this.Instrument && !node.WasCompilerGenerated) { - rewrittenAlternative = null; - break; + rewrittenCondition = Instrumenter.InstrumentIfStatementCondition(node, rewrittenCondition, _factory); } - if (alternative is BoundIfStatement elseIfStatement) + var elseIfStatement = node.AlternativeOpt as BoundIfStatement; + BoundStatement? rewrittenAlternative = null; + + if (elseIfStatement is null) { - node = elseIfStatement; + rewrittenAlternative = VisitStatement(node.AlternativeOpt); } - else + + var afterif = new GeneratedLabelSymbol("afterif"); + stack.Push((node, afterif, builder.Count)); + + if (elseIfStatement is null && rewrittenAlternative is null) { - rewrittenAlternative = VisitStatement(alternative); + // if (condition) + // consequence; + // + // becomes + // + // GotoIfFalse condition afterif; + // consequence; + // afterif: + + builder.Add(new BoundConditionalGoto(rewrittenCondition.Syntax, rewrittenCondition, false, afterif)); + builder.Add(rewrittenConsequence); break; } + else + { + // if (condition) + // consequence; + // else + // alternative + // + // becomes + // + // GotoIfFalse condition alt; + // consequence + // goto afterif; + // alt: + // alternative; + // afterif: + + var alt = new GeneratedLabelSymbol("alternative"); + + builder.Add(new BoundConditionalGoto(rewrittenCondition.Syntax, rewrittenCondition, false, alt)); + builder.Add(rewrittenConsequence); + builder.Add(BoundSequencePoint.CreateHidden()); + var syntax = (IfStatementSyntax)node.Syntax; + builder.Add(new BoundGotoStatement(syntax, afterif)); + builder.Add(new BoundLabelStatement(syntax, alt)); + + if (rewrittenAlternative is not null) + { + builder.Add(rewrittenAlternative); + break; + } + + Debug.Assert(elseIfStatement is not null); + + node = elseIfStatement; + } } - BoundStatement result; do { - var (ifStatement, rewrittenCondition, rewrittenConsequence) = stack.Pop(); - node = ifStatement; + (node, var afterif, var conditionalGotoIndex) = stack.Pop(); + Debug.Assert(builder[conditionalGotoIndex] is BoundConditionalGoto); var syntax = (IfStatementSyntax)node.Syntax; - // EnC: We need to insert a hidden sequence point to handle function remapping in case - // the containing method is edited while methods invoked in the condition are being executed. - if (this.Instrument && !node.WasCompilerGenerated) - { - rewrittenCondition = Instrumenter.InstrumentIfStatementCondition(node, rewrittenCondition, _factory); - } - - result = RewriteIfStatement(syntax, rewrittenCondition, rewrittenConsequence, rewrittenAlternative, node.HasErrors); + builder.Add(BoundSequencePoint.CreateHidden()); + builder.Add(new BoundLabelStatement(syntax, afterif)); // add sequence point before the whole statement if (this.Instrument && !node.WasCompilerGenerated) { - result = Instrumenter.InstrumentIfStatement(node, result); + builder[conditionalGotoIndex] = Instrumenter.InstrumentIfStatementConditionalGoto(node, builder[conditionalGotoIndex]); } - - rewrittenAlternative = result; } while (stack.Any()); stack.Free(); - return result; + return new BoundStatementList(node.Syntax, builder.ToImmutableAndFree(), node.HasErrors); } private static BoundStatement RewriteIfStatement( From 3253943cbbbd03b8f13ad61d584880582099e074 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 08:37:12 -0700 Subject: [PATCH 14/23] Revert remaining changes in CodeGenerator.cs --- .../CSharp/Portable/CodeGen/CodeGenerator.cs | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs index 9ac975d349644..ce7045499e411 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs @@ -393,29 +393,13 @@ private void EmitSequencePointStatement(BoundSequencePoint node) } } - private void EmitSequencePointStatementBegin(BoundSequencePointWithSpan node) + private void EmitSequencePointStatement(BoundSequencePointWithSpan node) { TextSpan span = node.Span; if (span != default(TextSpan) && _emitPdbSequencePoints) { this.EmitSequencePoint(node.SyntaxTree, span); } - } - - private void EmitSequencePointStatementEnd(BoundSequencePointWithSpan node, int instructionsEmitted) - { - TextSpan span = node.Span; - if (instructionsEmitted == 0 && span != default(TextSpan) && _ilEmitStyle == ILEmitStyle.Debug) - { - // if there was no code emitted, then emit nop - // otherwise this point could get associated with some random statement, possibly in a wrong scope - _builder.EmitOpCode(ILOpCode.Nop); - } - } - - private void EmitSequencePointStatement(BoundSequencePointWithSpan node) - { - EmitSequencePointStatementBegin(node); BoundStatement statement = node.StatementOpt; int instructionsEmitted = 0; @@ -424,7 +408,12 @@ private void EmitSequencePointStatement(BoundSequencePointWithSpan node) instructionsEmitted = this.EmitStatementAndCountInstructions(statement); } - EmitSequencePointStatementEnd(node, instructionsEmitted); + if (instructionsEmitted == 0 && span != default(TextSpan) && _ilEmitStyle == ILEmitStyle.Debug) + { + // if there was no code emitted, then emit nop + // otherwise this point could get associated with some random statement, possibly in a wrong scope + _builder.EmitOpCode(ILOpCode.Nop); + } } private void EmitSavePreviousSequencePoint(BoundSavePreviousSequencePoint statement) From e54179ec922fd736de6f57fdc97c3206c677c6aa Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 16:15:29 -0700 Subject: [PATCH 15/23] Move VisitIfStatement override to base --- .../Portable/Binder/RefSafetyAnalysis.cs | 27 ------------------- .../CSharp/Portable/BoundTree/BoundNode.cs | 24 ----------------- .../Portable/BoundTree/BoundTreeWalker.cs | 24 +++++++++++++++++ .../Lowering/LocalRewriter/LocalRewriter.cs | 17 +++++++----- 4 files changed, 35 insertions(+), 57 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs b/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs index 85afea61298bd..3b49411795218 100644 --- a/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs +++ b/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs @@ -1112,33 +1112,6 @@ private static ImmutableArray GetDeconstructionRightParts(Bound return null; } - 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; - } - else - { - this.Visit(alternative); - break; - } - } - - return null; - } - private static void Error(BindingDiagnosticBag diagnostics, ErrorCode code, SyntaxNodeOrToken syntax, params object[] args) { var location = syntax.GetLocation(); diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs index 806a41820e6e8..8f92867f0dd10 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs @@ -654,30 +654,6 @@ private void CheckDeclared(LocalSymbol local) return null; } - public 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; - } - public override BoundNode? VisitFixedStatement(BoundFixedStatement node) { AddAll(node.Locals); diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs index 8f0df23b1e56c..030b62c8d0d02 100644 --- 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/Lowering/LocalRewriter/LocalRewriter.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs index 83cc84c6125cf..96ac4dd7907b8 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs @@ -1146,6 +1146,17 @@ private CompoundUseSiteInfo GetNewCompoundUseSiteInfo() /// private sealed class LocalRewritingValidator : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator { + public override BoundNode? Visit(BoundNode? node) + { + if (node is BoundIfStatement) + { + Fail(node); + return null; + } + + return base.Visit(node); + } + /// /// Asserts that no unexpected nodes survived local rewriting. /// @@ -1173,12 +1184,6 @@ public static void Validate(BoundNode node) return null; } - public override BoundNode? VisitIfStatement(BoundIfStatement node) - { - Fail(node); - return null; - } - public override BoundNode? VisitDeconstructionVariablePendingInference(DeconstructionVariablePendingInference node) { Fail(node); From 2b7c88bb4c91561591276a16c7cde345078dabb8 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 17:14:51 -0700 Subject: [PATCH 16/23] Change base for EmptyRewriter --- .../Portable/BoundTree/BoundTreeRewriter.cs | 43 +++++++++++++++++++ .../Portable/Compiler/MethodCompiler.cs | 7 +-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs index 6f489cb0cf2ae..02099bcd64cc7 100644 --- 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.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/Compiler/MethodCompiler.cs b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs index 10dd7660b3d76..303c221951270 100644 --- a/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs +++ b/src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs @@ -2180,13 +2180,8 @@ private static bool IsEmptyRewritePossible(BoundNode node) } } - private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuard + private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator { - // PROTOTYPE: This override shouldn't be necessary. The base class should use a stack. - public override BoundNode? VisitIfStatement(BoundIfStatement node) - { - return node; // PROTOTYPE: Not implemented. - } } private sealed class UnboundLambdaFinder : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator From b337684a5896a64d5080d636366296eea7414493 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 17:27:39 -0700 Subject: [PATCH 17/23] Remove obsolete PROTOTYPE comments --- .../Portable/Lowering/LocalRewriter/LocalRewriter.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs index 96ac4dd7907b8..0dd4d8a7742db 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs @@ -145,8 +145,7 @@ public static BoundStatement Rewrite( statement.CheckLocalsDefined(); var loweredStatement = localRewriter.VisitStatement(statement); Debug.Assert(loweredStatement is { }); - // PROTOTYPE: Add _recursionDepth to LocalsScanner and avoid recursing in the same way we're currently doing in StackOptimizerPass1. - //loweredStatement.CheckLocalsDefined(); + loweredStatement.CheckLocalsDefined(); sawLambdas = localRewriter._sawLambdas; sawLocalFunctions = localRewriter._availableLocalFunctionOrdinal != 0; sawAwaitInExceptionHandler = localRewriter._sawAwaitInExceptionHandler; @@ -155,15 +154,13 @@ public static BoundStatement Rewrite( { // Move spill sequences to a top-level statement. This handles "lifting" await and the switch expression. var spilledStatement = SpillSequenceSpiller.Rewrite(loweredStatement, method, compilationState, diagnostics); - // PROTOTYPE: Same comment as above. - //spilledStatement.CheckLocalsDefined(); + spilledStatement.CheckLocalsDefined(); loweredStatement = spilledStatement; } codeCoverageSpans = codeCoverageInstrumenter?.DynamicAnalysisSpans ?? ImmutableArray.Empty; #if DEBUG - // PROTOTYPE: Same comment as above. - //LocalRewritingValidator.Validate(loweredStatement); + LocalRewritingValidator.Validate(loweredStatement); localRewriter.AssertNoPlaceholderReplacements(); #endif return loweredStatement; From 2dfbfeb7141631e2e562f670ec72a81854332e26 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 19:09:26 -0700 Subject: [PATCH 18/23] Simplify CreateBoundIfStatementOperation --- .../Portable/Operations/CSharpOperationFactory.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs index 35d6f4a1886fb..a49c025b48b12 100644 --- a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs +++ b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs @@ -1801,15 +1801,12 @@ private IEmptyOperation CreateBoundNoOpStatementOperation(BoundNoOpStatement bou private IConditionalOperation CreateBoundIfStatementOperation(BoundIfStatement boundIfStatement) { - var stack = ArrayBuilder<(BoundIfStatement, IOperation, IOperation)>.GetInstance(); + var stack = ArrayBuilder.GetInstance(); IOperation? whenFalse; while (true) { - IOperation condition = Create(boundIfStatement.Condition); - IOperation whenTrue = Create(boundIfStatement.Consequence); - Debug.Assert(whenTrue is { }); - stack.Push((boundIfStatement, condition, whenTrue)); + stack.Push(boundIfStatement); var alternative = boundIfStatement.AlternativeOpt; if (alternative is null) @@ -1832,12 +1829,14 @@ private IConditionalOperation CreateBoundIfStatementOperation(BoundIfStatement b ConditionalOperation result; do { - var (ifStatement, condition, whenTrue) = stack.Pop(); + boundIfStatement = stack.Pop(); + IOperation condition = Create(boundIfStatement.Condition); + IOperation whenTrue = Create(boundIfStatement.Consequence); bool isRef = false; - SyntaxNode syntax = ifStatement.Syntax; + SyntaxNode syntax = boundIfStatement.Syntax; ITypeSymbol? type = null; ConstantValue? constantValue = null; - bool isImplicit = ifStatement.WasCompilerGenerated; + bool isImplicit = boundIfStatement.WasCompilerGenerated; result = new ConditionalOperation(condition, whenTrue, whenFalse, isRef, _semanticModel, syntax, type, constantValue, isImplicit); whenFalse = result; } From 3331985887ebda4c0c406fbcc1ae25a0d97705c6 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 19:18:19 -0700 Subject: [PATCH 19/23] Revert unintended change --- .../Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs b/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs index 43e89f3ed63a3..dc9347a1974da 100644 --- a/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs +++ b/src/Compilers/CSharp/Test/Emit/Emit/DynamicAnalysis/DynamicInstrumentationTests.cs @@ -3094,7 +3094,7 @@ static void Test() Method1: x = 0 " + checker.ExpectedOutput; - var verifier = CompileAndVerify(source, expectedOutput: null, options: TestOptions.ReleaseExe); + var verifier = CompileAndVerify(source, expectedOutput, options: TestOptions.ReleaseExe); verifier.VerifyIL("Class1.Method1", @" { From 77cf5f9aecb6732c307e45c67b759abb8668003c Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Wed, 25 Sep 2024 19:22:16 -0700 Subject: [PATCH 20/23] Remove PROTOTYPE comment on ParseMisplacedElse --- src/Compilers/CSharp/Portable/Parser/LanguageParser.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs index c726c423143e1..3f4f401c5a95d 100644 --- a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs +++ b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs @@ -9608,7 +9608,6 @@ private IfStatementSyntax ParseIfStatement(SyntaxList attri return ifStatement; } - // PROTOTYPE: This method is similar to the original implementation of ParseIfStatement. Can we share code? private IfStatementSyntax ParseMisplacedElse(SyntaxList attributes) { Debug.Assert(this.CurrentToken.Kind == SyntaxKind.ElseKeyword); From 90f043b71e945daca987d0628a2396bc11c2a48d Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Thu, 26 Sep 2024 08:10:00 -0700 Subject: [PATCH 21/23] Adjust supported depth of nesting --- src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index dde5260b4796c..c46bfa8f3b768 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -413,7 +413,7 @@ public void NestedIfStatements() (4, ExecutionConfiguration.Debug) => 310, (4, ExecutionConfiguration.Release) => 1419, (8, ExecutionConfiguration.Debug) => 200, - (8, ExecutionConfiguration.Release) => 479, + (8, ExecutionConfiguration.Release) => 474, _ => throw new Exception($"Unexpected configuration {IntPtr.Size * 8}-bit {ExecutionConditionUtil.Configuration}") }; From 487db6e9101a8e58384114b5bff05d877542fefd Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Thu, 26 Sep 2024 11:13:30 -0700 Subject: [PATCH 22/23] Seal an override --- src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index a7287b2403039..dbe7ac50e5b22 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -1704,7 +1704,7 @@ protected virtual void AfterVisitConversion(BoundConversion node) { } - public override BoundNode VisitIfStatement(BoundIfStatement node) + public sealed override BoundNode VisitIfStatement(BoundIfStatement node) { // 5.3.3.5 If statements From 0811e7e2118a826d2bfa62aa29a0279185502266 Mon Sep 17 00:00:00 2001 From: AlekseyTs Date: Mon, 30 Sep 2024 19:35:08 -0700 Subject: [PATCH 23/23] PR feedback --- .../LocalRewriter_ForEachStatement.cs | 2 - .../LocalRewriter_IfStatement.cs | 65 +++++-------------- .../LocalRewriter_LockStatement.cs | 1 - .../LocalRewriter_UsingStatement.cs | 1 - .../Operations/CSharpOperationFactory.cs | 5 -- .../CSharp/Portable/Parser/LanguageParser.cs | 13 ++-- 6 files changed, 20 insertions(+), 67 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs index 7937be4379a86..306b42bad04f0 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ForEachStatement.cs @@ -404,7 +404,6 @@ private bool TryGetDisposeMethod(SyntaxNode forEachSyntax, ForEachEnumeratorInfo syntax: forEachSyntax, rewrittenCondition: _factory.IsNotNullReference(boundEnumeratorVar), rewrittenConsequence: disposeCallStatement, - rewrittenAlternativeOpt: null, hasErrors: false); } @@ -459,7 +458,6 @@ private bool TryGetDisposeMethod(SyntaxNode forEachSyntax, ForEachEnumeratorInfo resultKind: LookupResultKind.Viable, type: _compilation.GetSpecialType(SpecialType.System_Boolean)), rewrittenConsequence: disposeCallStatement, - rewrittenAlternativeOpt: null, hasErrors: false); // IDisposable d = e as IDisposable; diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs index 5315bccd1cba4..6bdd62e1dfa93 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs @@ -62,7 +62,7 @@ public override BoundNode VisitIfStatement(BoundIfStatement node) // if (condition) // consequence; // else - // alternative + // alternative; // // becomes // @@ -120,59 +120,26 @@ private static BoundStatement RewriteIfStatement( SyntaxNode syntax, BoundExpression rewrittenCondition, BoundStatement rewrittenConsequence, - BoundStatement? rewrittenAlternativeOpt, bool hasErrors) { var afterif = new GeneratedLabelSymbol("afterif"); var builder = ArrayBuilder.GetInstance(); - if (rewrittenAlternativeOpt == null) - { - // if (condition) - // consequence; - // - // becomes - // - // GotoIfFalse condition afterif; - // consequence; - // afterif: - - builder.Add(new BoundConditionalGoto(rewrittenCondition.Syntax, rewrittenCondition, false, afterif)); - builder.Add(rewrittenConsequence); - builder.Add(BoundSequencePoint.CreateHidden()); - builder.Add(new BoundLabelStatement(syntax, afterif)); - var statements = builder.ToImmutableAndFree(); - return new BoundStatementList(syntax, statements, hasErrors); - } - else - { - // if (condition) - // consequence; - // else - // alternative - // - // becomes - // - // GotoIfFalse condition alt; - // consequence - // goto afterif; - // alt: - // alternative; - // afterif: - - var alt = new GeneratedLabelSymbol("alternative"); - - builder.Add(new BoundConditionalGoto(rewrittenCondition.Syntax, rewrittenCondition, false, alt)); - builder.Add(rewrittenConsequence); - builder.Add(BoundSequencePoint.CreateHidden()); - builder.Add(new BoundGotoStatement(syntax, afterif)); - builder.Add(new BoundLabelStatement(syntax, alt)); - builder.Add(rewrittenAlternativeOpt); - builder.Add(BoundSequencePoint.CreateHidden()); - builder.Add(new BoundLabelStatement(syntax, afterif)); - return new BoundStatementList(syntax, builder.ToImmutableAndFree(), hasErrors); - } - + // if (condition) + // consequence; + // + // becomes + // + // GotoIfFalse condition afterif; + // consequence; + // afterif: + + builder.Add(new BoundConditionalGoto(rewrittenCondition.Syntax, rewrittenCondition, false, afterif)); + builder.Add(rewrittenConsequence); + builder.Add(BoundSequencePoint.CreateHidden()); + builder.Add(new BoundLabelStatement(syntax, afterif)); + var statements = builder.ToImmutableAndFree(); + return new BoundStatementList(syntax, statements, hasErrors); } } } diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_LockStatement.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_LockStatement.cs index 70692a4d9b647..7df64ae2978e2 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_LockStatement.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_LockStatement.cs @@ -163,7 +163,6 @@ public override BoundNode VisitLockStatement(BoundLockStatement node) lockSyntax, boundLockTakenTemp, exitCall, - null, node.HasErrors); return new BoundBlock( diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs index 249c7b23c0f1e..a7dcc68c4d583 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs @@ -410,7 +410,6 @@ private BoundStatement RewriteUsingStatementTryFinally( syntax: resourceSyntax, rewrittenCondition: ifCondition, rewrittenConsequence: disposeStatement, - rewrittenAlternativeOpt: null, hasErrors: false); } diff --git a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs index a49c025b48b12..bfe4dcad1c618 100644 --- a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs +++ b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs @@ -1809,11 +1809,6 @@ private IConditionalOperation CreateBoundIfStatementOperation(BoundIfStatement b stack.Push(boundIfStatement); var alternative = boundIfStatement.AlternativeOpt; - if (alternative is null) - { - whenFalse = null; - break; - } if (alternative is BoundIfStatement elseIfStatement) { diff --git a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs index 3f4f401c5a95d..427b8d72c71b1 100644 --- a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs +++ b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs @@ -8020,20 +8020,15 @@ private StatementSyntax ParseStatementCore(SyntaxList attri private StatementSyntax TryReuseStatement(SyntaxList attributes, bool isGlobal) { - if (canReuseStatement(attributes, isGlobal)) + if (this.IsIncrementalAndFactoryContextMatches && + this.CurrentNode is Syntax.StatementSyntax && + !isGlobal && // Top-level statements are reused by ParseMemberDeclarationOrStatementCore when possible. + attributes.Count == 0) { return (StatementSyntax)this.EatNode(); } return null; - - bool canReuseStatement(SyntaxList attributes, bool isGlobal) - { - return this.IsIncrementalAndFactoryContextMatches && - this.CurrentNode is Syntax.StatementSyntax && - !isGlobal && // Top-level statements are reused by ParseMemberDeclarationOrStatementCore when possible. - attributes.Count == 0; - } } private StatementSyntax ParseStatementCoreRest(SyntaxList attributes, bool isGlobal, ref ResetPoint resetPointBeforeStatement)