Support long chains of `else if` statements #74317

cston · 2024-07-09T19:32:42Z

Avoid recursion when visiting nested else if statements.

See #72393.

cston · 2024-09-10T17:22:05Z

/azp run roslyn-CI

azure-pipelines · 2024-09-10T17:22:22Z

Azure Pipelines successfully started running 1 pipeline(s).

AlekseyTs · 2024-09-19T22:54:22Z

src/Compilers/CSharp/Portable/BoundTree/BoundNode.cs

@@ -654,6 +654,30 @@ private void CheckDeclared(LocalSymbol local)
                return null;
            }

+            public override BoundNode? VisitIfStatement(BoundIfStatement node)


public override BoundNode? VisitIfStatement(BoundIfStatement node)

Move this to BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator? #Closed

Sounds good.

cston · 2024-09-28T17:44:46Z

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)


VisitIfStatement

Consider deriving DiagnosticsPass from BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator. That should allow removing this override and simplifying VisitBinaryOperator. #Resolved

Consider deriving DiagnosticsPass from BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator. That should allow removing this override and simplifying VisitBinaryOperator.

I'll open a follow-up issue for this refactoring. There is another PR coming about binary patterns. Will take care of all of them together.

Opened #75319.

cston · 2024-09-28T17:57:15Z

        BoundStatement? rewrittenAlternativeOpt,

Is RewriteIfStatement() still called with non-null alternative? If not, let's remove this. #Resolved

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs:123 in 487db6e. [](commit_id = 487db6e, deletion_comment = False)

cston · 2024-09-28T18:09:20Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IfStatement.cs

+                    // if (condition)
+                    //     consequence;
+                    // else 
+                    //     alternative


alternative

Minor: missing ; #Resolved

cston · 2024-09-28T18:16:03Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                return (StatementSyntax)this.EatNode();
+            }
+
+            return null;

            bool canReuseStatement(SyntaxList<AttributeListSyntax> attributes, bool isGlobal)


canReuseStatement

Minor: Consider inlining this local function. #Resolved

AlekseyTs · 2024-09-30T20:49:35Z

@dotnet/roslyn-compiler Please review

333fred

A couple of minor comments

333fred · 2024-09-30T23:55:35Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -2180,7 +2180,7 @@ private static bool IsEmptyRewritePossible(BoundNode node)
            }
        }

-        private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuard
+        private sealed class EmptyRewriter : BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator


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

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

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

333fred · 2024-10-01T00:13:52Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

@@ -1143,6 +1143,17 @@ private CompoundUseSiteInfo<AssemblySymbol> GetNewCompoundUseSiteInfo()
        /// </summary>
        private sealed class LocalRewritingValidator : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
        {
+            public override BoundNode? Visit(BoundNode? node)
+            {
+                if (node is BoundIfStatement)


I don't understand what this changes. What does this get us? #Resolved

I don't understand what this changes. What does this get us?

It gets us a validation that we don't have BoundIfStatements post lowering. We no longer can enforce that by overriding VisitIfStatement because it is now sealed in base. I don't think it makes sense to "unseal" it for the purpose of debug-only visitor.

We no longer can enforce that by overriding VisitIfStatement because it is now sealed in base.

Ah, that's what I was missing. Thanks.

333fred · 2024-10-01T00:18:26Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+                var alternative = boundIfStatement.AlternativeOpt;
+                if (alternative is null)
+                {
+                    whenFalse = null;
+                    break;
+                }
+
+                if (alternative is BoundIfStatement elseIfStatement)
+                {
+                    boundIfStatement = elseIfStatement;
+                }
+                else
+                {
+                    whenFalse = Create(alternative);
+                    break;
+                }


Nit: Create is null-resileint. Consider simplifying this to just check if alternative is a BoundIfStatement, and unconditionally calling Create and break in the else. #Resolved

cston · 2024-10-01T05:40:16Z

LGTM, although it looks like I cannot approve the PR, perhaps because I created the PR.

AlekseyTs · 2024-10-01T15:26:07Z

Chuck reviewed my changes, and, I reviewed his initial changes. In my opinion, that should count for a second sign-off. Merging.

…terns * upstream/main: (267 commits) Support long chains of `else if` statements (dotnet#74317) Update dependencies from https://github.com/dotnet/source-build-externals build 20240930.2 Fix the path to the proposal (dotnet#75302) Fix TSA tooling (dotnet#75307) Clarify the bug template to request a code snippet (dotnet#75306) Bump razor for serialization changes (dotnet#75282) Disallow declaration of indexers in absence of proper DefaultMemberAttribute. (dotnet#75099) stoub use ref Simpler Simplify Switch to a threadlocal storage to prevent locks add comment don't mess with user caret in smart rename Update LanguageServer references Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2548898 Use common helper method Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2548278 Field-backed properties: additional tests (dotnet#75283) Revert "Updates content exclusion for on-the-fly-docs (dotnet#75172)" (dotnet#75279) (dotnet#75284) ...

This is a follow up on #74317

Nested if/else if

3a22e73

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 9, 2024

cston force-pushed the 72393 branch from eb19c89 to 25d5429 Compare July 12, 2024 21:19

BindIfStatement

86a1bf8

cston force-pushed the 72393 branch from 25d5429 to 86a1bf8 Compare July 12, 2024 21:21

Fix tests

b705b76

Merge 'main' into 72393

f362bb2

AlekseyTs reviewed Sep 19, 2024

View reviewed changes

AlekseyTs added 19 commits September 21, 2024 14:14

Simplify logic in ControlFlowGraph builder and Binder

e7d2673

Fix a couple of visitors

ac712a1

Formatting

aa4fdc8

Fixup ParseIfStatement

e64b418

Move new test

61dd4c5

Don't run new test in IOperation leg

c8578eb

Revert using chnages

f795071

Adjust nesting depth expectations

5ac1691

Add IL verification

771e406

Adjust LocalRewriter.VisitIfStatement

17dfd90

Revert remaining changes in CodeGenerator.cs

3253943

Move VisitIfStatement override to base

e54179e

Change base for EmptyRewriter

2b7c88b

Remove obsolete PROTOTYPE comments

b337684

Simplify CreateBoundIfStatementOperation

2dfbfeb

Revert unintended change

3331985

Remove PROTOTYPE comment on ParseMisplacedElse

77cf5f9

Adjust supported depth of nesting

90f043b

Seal an override

487db6e

cston commented Sep 28, 2024

View reviewed changes

AlekseyTs marked this pull request as ready for review September 30, 2024 20:48

AlekseyTs requested a review from a team as a code owner September 30, 2024 20:48

AlekseyTs requested review from a team and removed request for a team September 30, 2024 20:48

333fred reviewed Oct 1, 2024

View reviewed changes

PR feedback

0811e7e

333fred approved these changes Oct 1, 2024

View reviewed changes

AlekseyTs merged commit 8d8276d into dotnet:main Oct 1, 2024
24 checks passed

dotnet-policy-service bot added this to the Next milestone Oct 1, 2024

AlekseyTs mentioned this pull request Oct 1, 2024

Consider deriving DiagnosticsPass from BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator. #75319

Open

This was referenced Oct 9, 2024

[Automated] PRs inserted in VS build feature.debugger.main-35407.82 #75445

Closed

[Automated] PRs inserted in VS build main-35407.57 #75446

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35410.215 #75481

Closed

AlekseyTs added a commit that referenced this pull request Oct 23, 2024

IncrementalBinder - properly bind else-if statement (#75579)

bc89495

This is a follow up on #74317

AlekseyTs mentioned this pull request Oct 23, 2024

IncrementalBinder - properly bind else-if statement #75579

Merged

akhera99 modified the milestones: Next, 17.13 P1 Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support long chains of `else if` statements #74317

Support long chains of `else if` statements #74317

cston commented Jul 9, 2024 •

edited

Loading

cston commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

AlekseyTs Sep 19, 2024 •

edited

Loading

cston Sep 19, 2024

cston Sep 28, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Sep 30, 2024

AlekseyTs Oct 1, 2024

cston commented Sep 28, 2024 •

edited by AlekseyTs

Loading

cston Sep 28, 2024 •

edited by AlekseyTs

Loading

cston Sep 28, 2024 •

edited by AlekseyTs

Loading

AlekseyTs commented Sep 30, 2024

333fred left a comment

333fred Sep 30, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Oct 1, 2024

AlekseyTs Oct 1, 2024

333fred Oct 1, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Oct 1, 2024

333fred Oct 1, 2024

333fred Oct 1, 2024 •

edited by AlekseyTs

Loading

cston commented Oct 1, 2024

AlekseyTs commented Oct 1, 2024

Support long chains of else if statements #74317

Support long chains of else if statements #74317

Conversation

cston commented Jul 9, 2024 • edited Loading

cston commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

AlekseyTs Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

cston Sep 19, 2024

Choose a reason for hiding this comment

cston Sep 28, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Sep 30, 2024

Choose a reason for hiding this comment

AlekseyTs Oct 1, 2024

Choose a reason for hiding this comment

cston commented Sep 28, 2024 • edited by AlekseyTs Loading

cston Sep 28, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Sep 28, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs commented Sep 30, 2024

333fred left a comment

Choose a reason for hiding this comment

333fred Sep 30, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Oct 1, 2024

Choose a reason for hiding this comment

AlekseyTs Oct 1, 2024

Choose a reason for hiding this comment

333fred Oct 1, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Oct 1, 2024

Choose a reason for hiding this comment

333fred Oct 1, 2024

Choose a reason for hiding this comment

333fred Oct 1, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston commented Oct 1, 2024

AlekseyTs commented Oct 1, 2024

Support long chains of `else if` statements #74317

Support long chains of `else if` statements #74317

cston commented Jul 9, 2024 •

edited

Loading

AlekseyTs Sep 19, 2024 •

edited

Loading

cston Sep 28, 2024 •

edited by AlekseyTs

Loading

cston commented Sep 28, 2024 •

edited by AlekseyTs

Loading

cston Sep 28, 2024 •

edited by AlekseyTs

Loading

cston Sep 28, 2024 •

edited by AlekseyTs

Loading

333fred Sep 30, 2024 •

edited by AlekseyTs

Loading

333fred Oct 1, 2024 •

edited by AlekseyTs

Loading

333fred Oct 1, 2024 •

edited by AlekseyTs

Loading