From 1af114b7ec7b13bea8fc36b14f2cbca5a888f1ef Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Mon, 23 Sep 2024 16:26:57 -0700 Subject: [PATCH 01/11] Use manual recursion for binary patterns Much like binary operators, we can have deeply-nested binary patterns. An example of this is our own IsBuildOnlyDiagnostic, which has ~2500 binary patterns in a single arm, and growing every release. To avoid stack depth issues, I took the same approach we do for binary operators; rewrite recursion to use a manual stack for these cases. Fixes https://github.com/dotnet/roslyn/issues/73439. --- .../CSharp/Portable/Binder/Binder_Patterns.cs | 184 +++++++++++------- .../Portable/Binder/DecisionDagBuilder.cs | 76 ++++++-- .../Binder/ExpressionVariableFinder.cs | 22 +++ .../Portable/Binder/LocalBinderFactory.cs | 21 ++ .../Portable/BoundTree/BoundTreeWalker.cs | 48 ++++- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 81 ++++++-- .../FlowAnalysis/DefiniteAssignment.cs | 21 +- .../FlowAnalysis/NullableWalker_Patterns.cs | 39 +++- .../DiagnosticsPass_ExpressionTrees.cs | 22 +++ .../CSharp/Test/EndToEnd/EndToEndTests.cs | 70 ++++++- 10 files changed, 458 insertions(+), 126 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs index 8f75c4fcfce84..96a8d1584519c 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs @@ -1734,99 +1734,137 @@ private BoundPattern BindBinaryPattern( bool hasErrors, BindingDiagnosticBag diagnostics) { - bool isDisjunction = node.Kind() == SyntaxKind.OrPattern; - if (isDisjunction) - { - MessageID.IDS_FeatureOrPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken); + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + + var binaryPatternStack = ArrayBuilder.GetInstance(); + BinaryPatternSyntax? currentNode = node; - permitDesignations = false; // prevent designators under 'or' - var left = BindPattern(node.Left, inputType, permitDesignations, hasErrors, diagnostics); - var right = BindPattern(node.Right, inputType, permitDesignations, hasErrors, diagnostics); + do + { + binaryPatternStack.Push(currentNode); + currentNode = currentNode.Left as BinaryPatternSyntax; + } while (currentNode != null); - // Compute the common type. This algorithm is quadratic, but disjunctive patterns are unlikely to be huge - var narrowedTypeCandidates = ArrayBuilder.GetInstance(2); - collectCandidates(left, narrowedTypeCandidates); - collectCandidates(right, narrowedTypeCandidates); - var narrowedType = leastSpecificType(node, narrowedTypeCandidates, diagnostics) ?? inputType; - narrowedTypeCandidates.Free(); + Debug.Assert(binaryPatternStack.Count > 0); - return new BoundBinaryPattern(node, disjunction: isDisjunction, left, right, inputType: inputType, narrowedType: narrowedType, hasErrors); + BoundPattern? result = null; - static void collectCandidates(BoundPattern pat, ArrayBuilder candidates) + while (binaryPatternStack.TryPop(out var binaryPattern)) + { + if (result == null) { - if (pat is BoundBinaryPattern { Disjunction: true } p) - { - collectCandidates(p.Left, candidates); - collectCandidates(p.Right, candidates); - } - else - { - candidates.Add(pat.NarrowedType); - } + Debug.Assert(binaryPattern.Left is not BinaryPatternSyntax); + result = BindPattern(binaryPattern.Left, inputType, permitDesignations, hasErrors, diagnostics); } - TypeSymbol? leastSpecificType(SyntaxNode node, ArrayBuilder candidates, BindingDiagnosticBag diagnostics) + result = bindBinaryPattern(result, this, binaryPattern, inputType, permitDesignations, hasErrors, diagnostics); + } + + binaryPatternStack.Free(); + Debug.Assert(result != null); + return result; + + static BoundPattern bindBinaryPattern( + BoundPattern preboundLeft, + Binder binder, + BinaryPatternSyntax node, + TypeSymbol inputType, + bool permitDesignations, + bool hasErrors, + BindingDiagnosticBag diagnostics) + { + bool isDisjunction = node.Kind() == SyntaxKind.OrPattern; + if (isDisjunction) { - Debug.Assert(candidates.Count >= 2); - CompoundUseSiteInfo useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics); - TypeSymbol? bestSoFar = candidates[0]; - // first pass: select a candidate for which no other has been shown to be an improvement. - for (int i = 1, n = candidates.Count; i < n; i++) + MessageID.IDS_FeatureOrPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken); + + permitDesignations = false; // prevent designators under 'or' + var right = binder.BindPattern(node.Right, inputType, permitDesignations, hasErrors, diagnostics); + + // Compute the common type. This algorithm is quadratic, but disjunctive patterns are unlikely to be huge + var narrowedTypeCandidates = ArrayBuilder.GetInstance(2); + collectCandidates(preboundLeft, narrowedTypeCandidates); + collectCandidates(right, narrowedTypeCandidates); + var narrowedType = leastSpecificType(node, narrowedTypeCandidates, diagnostics) ?? inputType; + narrowedTypeCandidates.Free(); + + return new BoundBinaryPattern(node, disjunction: isDisjunction, preboundLeft, right, inputType: inputType, narrowedType: narrowedType, hasErrors); + + static void collectCandidates(BoundPattern pat, ArrayBuilder candidates) { - TypeSymbol candidate = candidates[i]; - bestSoFar = lessSpecificCandidate(bestSoFar, candidate, ref useSiteInfo) ?? bestSoFar; + if (pat is BoundBinaryPattern { Disjunction: true } p) + { + collectCandidates(p.Left, candidates); + collectCandidates(p.Right, candidates); + } + else + { + candidates.Add(pat.NarrowedType); + } } - // second pass: check that it is no more specific than any candidate. - for (int i = 0, n = candidates.Count; i < n; i++) + + TypeSymbol? leastSpecificType(SyntaxNode node, ArrayBuilder candidates, BindingDiagnosticBag diagnostics) { - TypeSymbol candidate = candidates[i]; - TypeSymbol? spoiler = lessSpecificCandidate(candidate, bestSoFar, ref useSiteInfo); - if (spoiler is null) + Debug.Assert(candidates.Count >= 2); + CompoundUseSiteInfo useSiteInfo = binder.GetNewCompoundUseSiteInfo(diagnostics); + TypeSymbol? bestSoFar = candidates[0]; + // first pass: select a candidate for which no other has been shown to be an improvement. + for (int i = 1, n = candidates.Count; i < n; i++) { - bestSoFar = null; - break; + TypeSymbol candidate = candidates[i]; + bestSoFar = lessSpecificCandidate(bestSoFar, candidate, ref useSiteInfo) ?? bestSoFar; } + // second pass: check that it is no more specific than any candidate. + for (int i = 0, n = candidates.Count; i < n; i++) + { + TypeSymbol candidate = candidates[i]; + TypeSymbol? spoiler = lessSpecificCandidate(candidate, bestSoFar, ref useSiteInfo); + if (spoiler is null) + { + bestSoFar = null; + break; + } - // Our specificity criteria are transitive - Debug.Assert(spoiler.Equals(bestSoFar, TypeCompareKind.ConsiderEverything)); - } - - diagnostics.Add(node, useSiteInfo); - return bestSoFar; - } + // Our specificity criteria are transitive + Debug.Assert(spoiler.Equals(bestSoFar, TypeCompareKind.ConsiderEverything)); + } - // Given a candidate least specific type so far, attempt to refine it with a possibly less specific candidate. - TypeSymbol? lessSpecificCandidate(TypeSymbol bestSoFar, TypeSymbol possiblyLessSpecificCandidate, ref CompoundUseSiteInfo useSiteInfo) - { - if (bestSoFar.Equals(possiblyLessSpecificCandidate, TypeCompareKind.AllIgnoreOptions)) - { - // When the types are equivalent, merge them. - return bestSoFar.MergeEquivalentTypes(possiblyLessSpecificCandidate, VarianceKind.Out); + diagnostics.Add(node, useSiteInfo); + return bestSoFar; } - else if (Conversions.HasImplicitReferenceConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo)) - { - // When there is an implicit reference conversion from T to U, U is less specific - return possiblyLessSpecificCandidate; - } - else if (Conversions.HasBoxingConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo)) - { - // when there is a boxing conversion from T to U, U is less specific. - return possiblyLessSpecificCandidate; - } - else + + // Given a candidate least specific type so far, attempt to refine it with a possibly less specific candidate. + TypeSymbol? lessSpecificCandidate(TypeSymbol bestSoFar, TypeSymbol possiblyLessSpecificCandidate, ref CompoundUseSiteInfo useSiteInfo) { - // We have no improved candidate to offer. - return null; + if (bestSoFar.Equals(possiblyLessSpecificCandidate, TypeCompareKind.AllIgnoreOptions)) + { + // When the types are equivalent, merge them. + return bestSoFar.MergeEquivalentTypes(possiblyLessSpecificCandidate, VarianceKind.Out); + } + else if (binder.Conversions.HasImplicitReferenceConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo)) + { + // When there is an implicit reference conversion from T to U, U is less specific + return possiblyLessSpecificCandidate; + } + else if (binder.Conversions.HasBoxingConversion(bestSoFar, possiblyLessSpecificCandidate, ref useSiteInfo)) + { + // when there is a boxing conversion from T to U, U is less specific. + return possiblyLessSpecificCandidate; + } + else + { + // We have no improved candidate to offer. + return null; + } } } - } - else - { - MessageID.IDS_FeatureAndPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken); + else + { + MessageID.IDS_FeatureAndPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken); - var left = BindPattern(node.Left, inputType, permitDesignations, hasErrors, diagnostics); - var right = BindPattern(node.Right, left.NarrowedType, permitDesignations, hasErrors, diagnostics); - return new BoundBinaryPattern(node, disjunction: isDisjunction, left, right, inputType: inputType, narrowedType: right.NarrowedType, hasErrors); + var right = binder.BindPattern(node.Right, preboundLeft.NarrowedType, permitDesignations, hasErrors, diagnostics); + return new BoundBinaryPattern(node, disjunction: isDisjunction, preboundLeft, right, inputType: inputType, narrowedType: right.NarrowedType, hasErrors); + } } } } diff --git a/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs b/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs index 9fb7f496e7fad..acb20e79ed0d0 100644 --- a/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs +++ b/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs @@ -645,33 +645,71 @@ private Tests MakeTestsAndBindingsForBinaryPattern( out BoundDagTemp output, ArrayBuilder bindings) { - var builder = ArrayBuilder.GetInstance(2); - if (bin.Disjunction) + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + + var binaryPatternStack = ArrayBuilder.GetInstance(); + var currentNode = bin; + + do { - builder.Add(MakeTestsAndBindings(input, bin.Left, bindings)); - builder.Add(MakeTestsAndBindings(input, bin.Right, bindings)); - var result = Tests.OrSequence.Create(builder); - if (bin.InputType.Equals(bin.NarrowedType)) + binaryPatternStack.Push(currentNode); + currentNode = currentNode.Left as BoundBinaryPattern; + } while (currentNode != null); + + Tests? result = null; + BoundDagTemp? currentOutput = null; + + while (binaryPatternStack.TryPop(out currentNode)) + { + if (result == null) { - output = input; - return result; + Debug.Assert(currentNode.Left is not BoundBinaryPattern); + Debug.Assert(currentOutput == null); + result = MakeTestsAndBindings(input, currentNode.Left, out currentOutput, bindings); + } + + Debug.Assert(currentOutput != null); + result = makeTestsAndBindingsForBinaryPattern(this, result, currentOutput, input, currentNode, out currentOutput, bindings); + } + + Debug.Assert(result != null); + Debug.Assert(currentOutput != null); + output = currentOutput; + + binaryPatternStack.Free(); + + return result; + + static Tests makeTestsAndBindingsForBinaryPattern(DecisionDagBuilder @this, Tests leftTests, BoundDagTemp leftOutput, BoundDagTemp input, BoundBinaryPattern bin, out BoundDagTemp output, ArrayBuilder bindings) + { + var builder = ArrayBuilder.GetInstance(2); + if (bin.Disjunction) + { + builder.Add(leftTests); + builder.Add(@this.MakeTestsAndBindings(input, bin.Right, bindings)); + var result = Tests.OrSequence.Create(builder); + if (bin.InputType.Equals(bin.NarrowedType)) + { + output = input; + return result; + } + else + { + builder = ArrayBuilder.GetInstance(2); + builder.Add(result); + output = @this.MakeConvertToType(input: input, syntax: bin.Syntax, type: bin.NarrowedType, isExplicitTest: false, tests: builder); + return Tests.AndSequence.Create(builder); + } } else { - builder = ArrayBuilder.GetInstance(2); - builder.Add(result); - output = MakeConvertToType(input: input, syntax: bin.Syntax, type: bin.NarrowedType, isExplicitTest: false, tests: builder); + builder.Add(leftTests); + builder.Add(@this.MakeTestsAndBindings(leftOutput, bin.Right, out var rightOutput, bindings)); + output = rightOutput; + Debug.Assert(bin.HasErrors || output.Type.Equals(bin.NarrowedType, TypeCompareKind.AllIgnoreOptions)); return Tests.AndSequence.Create(builder); } } - else - { - builder.Add(MakeTestsAndBindings(input, bin.Left, out var leftOutput, bindings)); - builder.Add(MakeTestsAndBindings(leftOutput, bin.Right, out var rightOutput, bindings)); - output = rightOutput; - Debug.Assert(bin.HasErrors || output.Type.Equals(bin.NarrowedType, TypeCompareKind.AllIgnoreOptions)); - return Tests.AndSequence.Create(builder); - } } private Tests MakeTestsAndBindingsForRelationalPattern( diff --git a/src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs b/src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs index daa0cc7adad07..32d3a145843b8 100644 --- a/src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs +++ b/src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs @@ -339,6 +339,28 @@ public override void VisitBinaryExpression(BinaryExpressionSyntax node) operands.Free(); } + public override void VisitBinaryPattern(BinaryPatternSyntax node) + { + // Like binary expressions, binary patterns are left-associative, and can be deeply nested (even in our own code). + // Handle this with manual recursion. + PatternSyntax currentPattern = node; + + var rightPatternStack = ArrayBuilder.GetInstance(); + + while (currentPattern is BinaryPatternSyntax binaryPattern) + { + rightPatternStack.Push(binaryPattern.Right); + currentPattern = binaryPattern.Left; + } + + do + { + Visit(currentPattern); + } while (rightPatternStack.TryPop(out currentPattern)); + + rightPatternStack.Free(); + } + public override void VisitInvocationExpression(InvocationExpressionSyntax node) { if (receiverIsInvocation(node, out InvocationExpressionSyntax nested)) diff --git a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs index 1e6ee6bfc4953..3b14683a96c8d 100644 --- a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs +++ b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs @@ -796,6 +796,27 @@ public override void VisitSwitchExpression(SwitchExpressionSyntax node) } } + public override void VisitBinaryPattern(BinaryPatternSyntax node) + { + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + PatternSyntax currentPattern = node; + + var rightPatternStack = ArrayBuilder.GetInstance(); + + while (currentPattern is BinaryPatternSyntax binaryPattern) + { + rightPatternStack.Push(binaryPattern.Right); + currentPattern = binaryPattern.Left; + } + + do + { + Visit(currentPattern); + } while (rightPatternStack.TryPop(out currentPattern)); + + rightPatternStack.Free(); + } + public override void VisitIfStatement(IfStatementSyntax node) { Visit(node.Condition, _enclosing); diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs index 8f0df23b1e56c..9598ad87de250 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs @@ -103,7 +103,7 @@ protected BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator public sealed override BoundNode? VisitBinaryOperator(BoundBinaryOperator node) { - if (node.Left.Kind != BoundKind.BinaryOperator) + if (node.Left is not BoundBinaryOperator binary) { return base.VisitBinaryOperator(node); } @@ -112,12 +112,10 @@ protected BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator rightOperands.Push(node.Right); - var binary = (BoundBinaryOperator)node.Left; - BeforeVisitingSkippedBoundBinaryOperatorChildren(binary); rightOperands.Push(binary.Right); - BoundExpression current = binary.Left; + BoundExpression? current = binary.Left; while (current.Kind == BoundKind.BinaryOperator) { @@ -129,9 +127,9 @@ protected BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator this.Visit(current); - while (rightOperands.Count > 0) + while (rightOperands.TryPop(out current)) { - this.Visit(rightOperands.Pop()); + this.Visit(current); } rightOperands.Free(); @@ -142,6 +140,44 @@ protected virtual void BeforeVisitingSkippedBoundBinaryOperatorChildren(BoundBin { } + public sealed override BoundNode? VisitBinaryPattern(BoundBinaryPattern node) + { + if (node.Left is not BoundBinaryPattern binary) + { + return base.VisitBinaryPattern(node); + } + + var rightOperands = ArrayBuilder.GetInstance(); + + rightOperands.Push(node.Right); + BeforeVisitingSkippedBoundBinaryPatternChildren(binary); + rightOperands.Push(binary.Right); + + BoundPattern? current = binary.Left; + + while (current.Kind == BoundKind.BinaryPattern) + { + binary = (BoundBinaryPattern)current; + BeforeVisitingSkippedBoundBinaryPatternChildren(binary); + rightOperands.Push(binary.Right); + current = binary.Left; + } + + Visit(current); + + while (rightOperands.TryPop(out current)) + { + Visit(current); + } + + rightOperands.Free(); + return null; + } + + protected virtual void BeforeVisitingSkippedBoundBinaryPatternChildren(BoundBinaryPattern node) + { + } + public sealed override BoundNode? VisitCall(BoundCall node) { if (node.ReceiverOpt is BoundCall receiver1) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index b9e5185d9357a..494b4683a0b23 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -996,17 +996,26 @@ static bool patternMatchesNull(BoundPattern pattern) case BoundNegatedPattern negated: return !patternMatchesNull(negated.Negated); case BoundBinaryPattern binary: - if (binary.Disjunction) + var binaryPatterns = getBinaryPatterns(binary); + Debug.Assert(binaryPatterns.Peek().Left is not BoundBinaryPattern); + bool currentPatternMatchesNull = patternMatchesNull(binaryPatterns.Peek().Left); + + while (binaryPatterns.TryPop(out var currentBinary)) { - // `a?.b(out x) is null or C` - // pattern matches null if either subpattern matches null - var leftNullTest = patternMatchesNull(binary.Left); - return patternMatchesNull(binary.Left) || patternMatchesNull(binary.Right); + if (binary.Disjunction) + { + // `a?.b(out x) is null or C` + // pattern matches null if either subpattern matches null + currentPatternMatchesNull = currentPatternMatchesNull || patternMatchesNull(binary.Right); + } + + // `a?.b out x is not null and var c` + // pattern matches null only if both subpatterns match null + currentPatternMatchesNull = currentPatternMatchesNull && patternMatchesNull(binary.Right); } - // `a?.b out x is not null and var c` - // pattern matches null only if both subpatterns match null - return patternMatchesNull(binary.Left) && patternMatchesNull(binary.Right); + binaryPatterns.Free(); + return currentPatternMatchesNull; case BoundDeclarationPattern { IsVar: true }: case BoundDiscardPattern: return true; @@ -1027,21 +1036,31 @@ static bool patternMatchesNull(BoundPattern pattern) case BoundNegatedPattern negated: return !isBoolTest(negated.Negated); case BoundBinaryPattern binary: - if (binary.Disjunction) + var binaryPatterns = getBinaryPatterns(binary); + Debug.Assert(binaryPatterns.Peek().Left is not BoundBinaryPattern); + bool? currentBoolTest = isBoolTest(binaryPatterns.Peek().Left); + + while (binaryPatterns.TryPop(out var currentBinary)) { - // `(a != null && a.b(out x)) is true or true` matches `true` - // `(a != null && a.b(out x)) is true or false` matches any boolean - // both subpatterns must have the same bool test for the test to propagate out - var leftNullTest = isBoolTest(binary.Left); - return leftNullTest is null ? null : - leftNullTest != isBoolTest(binary.Right) ? null : - leftNullTest; + if (currentBinary.Disjunction) + { + // `(a != null && a.b(out x)) is true or true` matches `true` + // `(a != null && a.b(out x)) is true or false` matches any boolean + // both subpatterns must have the same bool test for the test to propagate out + var leftNullTest = currentBoolTest; + currentBoolTest = leftNullTest is null ? null : + leftNullTest != isBoolTest(binary.Right) ? null : + leftNullTest; + } + + // `(a != null && a.b(out x)) is true and true` matches `true` + // `(a != null && a.b(out x)) is true and var x` matches `true` + // `(a != null && a.b(out x)) is true and false` never matches and is a compile error + currentBoolTest ??= isBoolTest(binary.Right); } - // `(a != null && a.b(out x)) is true and true` matches `true` - // `(a != null && a.b(out x)) is true and var x` matches `true` - // `(a != null && a.b(out x)) is true and false` never matches and is a compile error - return isBoolTest(binary.Left) ?? isBoolTest(binary.Right); + binaryPatterns.Free(); + return currentBoolTest; case BoundConstantPattern { ConstantValue: { IsBoolean: false } }: case BoundDiscardPattern: case BoundTypePattern: @@ -1056,6 +1075,28 @@ static bool patternMatchesNull(BoundPattern pattern) throw ExceptionUtilities.UnexpectedValue(pattern.Kind); } } + + static ArrayBuilder getBinaryPatterns(BoundBinaryPattern binaryPattern) + { + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + + var stack = ArrayBuilder.GetInstance(); + + while (true) + { + stack.Push(binaryPattern); + if (binaryPattern.Left is BoundBinaryPattern leftBinaryPattern) + { + binaryPattern = leftBinaryPattern; + } + else + { + break; + } + } + + return stack; + } } public virtual void VisitPattern(BoundPattern pattern) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 67cae8265b05e..364990440b717 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -2063,10 +2063,25 @@ void assignPatternVariablesAndMarkReadFields(BoundPattern pattern, bool definite } case BoundKind.BinaryPattern: { + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + var pat = (BoundBinaryPattern)pattern; - bool def = definitely && !pat.Disjunction; - assignPatternVariablesAndMarkReadFields(pat.Left, def); - assignPatternVariablesAndMarkReadFields(pat.Right, def); + + while (true) + { + definitely = definitely && !pat.Disjunction; + assignPatternVariablesAndMarkReadFields(pat.Right, definitely); + + if (pat.Left is BoundBinaryPattern left) + { + pat = left; + } + else + { + assignPatternVariablesAndMarkReadFields(pat.Left, definitely); + break; + } + } break; } default: diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs index ad6105d4bb442..c55d166d05aa5 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs @@ -111,8 +111,26 @@ public override BoundNode VisitNegatedPattern(BoundNegatedPattern node) public override BoundNode VisitBinaryPattern(BoundBinaryPattern node) { - Visit(node.Left); - Visit(node.Right); + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + + var stack = ArrayBuilder.GetInstance(); + BoundBinaryPattern current = node; + do + { + TakeIncrementalSnapshot(current); + stack.Push(current); + current = current.Left as BoundBinaryPattern; + } while (current != null); + + Debug.Assert(stack.Peek().Left is not BoundBinaryPattern); + Visit(stack.Peek().Left); + + while (stack.TryPop(out current)) + { + Visit(current.Right); + } + + stack.Free(); return null; } @@ -199,8 +217,21 @@ private void LearnFromAnyNullPatterns( LearnFromAnyNullPatterns(inputSlot, inputType, p.Negated); break; case BoundBinaryPattern p: - LearnFromAnyNullPatterns(inputSlot, inputType, p.Left); - LearnFromAnyNullPatterns(inputSlot, inputType, p.Right); + // Do not use left recursion because we can have many nested binary patterns. + var current = p; + while (true) + { + LearnFromAnyNullPatterns(inputSlot, inputType, current.Right); + if (current.Left is BoundBinaryPattern left) + { + current = left; + } + else + { + LearnFromAnyNullPatterns(inputSlot, inputType, current.Left); + break; + } + } break; default: throw ExceptionUtilities.UnexpectedValue(pattern); diff --git a/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs b/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs index 1f401b2ef6eff..0c1e7f8d9078e 100644 --- a/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs +++ b/src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs @@ -676,6 +676,28 @@ public override BoundNode VisitBinaryOperator(BoundBinaryOperator node) return null; } + public override BoundNode VisitBinaryPattern(BoundBinaryPattern node) + { + // Do not use left recursion because we can have many nested binary patterns. + + BoundBinaryPattern current = node; + while (true) + { + Visit(current.Right); + if (current.Left is BoundBinaryPattern left) + { + current = left; + } + else + { + Visit(current.Left); + break; + } + } + + return null; + } + public override BoundNode VisitUserDefinedConditionalLogicalOperator(BoundUserDefinedConditionalLogicalOperator node) { CheckLiftedUserDefinedConditionalLogicalOperator(node); diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 9fc40f7fc40b3..0478b1efb9d6e 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -60,7 +60,7 @@ private static void RunInThread(Action action, TimeSpan? timeout = null) if (exception is object) { - throw exception; + Assert.False(true, exception.ToString()); } } @@ -723,5 +723,73 @@ class XAttribute : System.Attribute { } Assert.Collection(runResult.TrackedSteps["result_ForAttributeWithMetadataName"], step => Assert.True(step.Outputs.Single().Value is ClassDeclarationSyntax { Identifier.ValueText: "C1" })); } + + [Fact] + public void ManyBinaryPatterns() + { + const string Preamble = $""" + int i = 1; + + System.Console.Write(i is + """; + const string Append = $""" + + or + """; + const string Postscript = """ + + ? 1 : 0); + """; + + const int NumBinaryExpressions = 12_000; + + var builder = new StringBuilder(Preamble.Length + Postscript.Length + Append.Length * NumBinaryExpressions + 5 /* Max num digit characters */ * NumBinaryExpressions); + + builder.AppendLine(Preamble); + + builder.Append(0); + + for (int i = 1; i < NumBinaryExpressions; i++) + { + builder.Append(Append); + builder.Append(i); + } + + builder.AppendLine(Postscript); + + var source = builder.ToString(); + RunInThread(() => + { + var comp = CreateCompilation(source, options: TestOptions.DebugExe.WithConcurrentBuild(false)); + var verifier = CompileAndVerify(comp, expectedOutput: "1"); + verifier.VerifyIL("", """ + { + // Code size 32 (0x20) + .maxstack 2 + .locals init (int V_0, //i + bool V_1) + IL_0000: ldc.i4.1 + IL_0001: stloc.0 + IL_0002: ldloc.0 + IL_0003: ldc.i4 0x2edf + IL_0008: ble.un.s IL_000c + IL_000a: br.s IL_0010 + IL_000c: ldc.i4.1 + IL_000d: stloc.1 + IL_000e: br.s IL_0012 + IL_0010: ldc.i4.0 + IL_0011: stloc.1 + IL_0012: ldloc.1 + IL_0013: brtrue.s IL_0018 + IL_0015: ldc.i4.0 + IL_0016: br.s IL_0019 + IL_0018: ldc.i4.1 + IL_0019: call "void System.Console.Write(int)" + IL_001e: nop + IL_001f: ret + } + """); + }); + } } } From 09dee1176f7126b0f882ca2d7b5a4cf83df03fac Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Mon, 23 Sep 2024 17:04:34 -0700 Subject: [PATCH 02/11] Handle IOperation and CFG for deeply-nested patterns. --- .../Operations/CSharpOperationFactory.cs | 45 +++++++++++++++---- .../CSharp/Test/EndToEnd/EndToEndTests.cs | 12 +++++ .../Operations/ControlFlowGraphBuilder.cs | 45 +++++++++++++++---- 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs index 2b7e62c5393bc..0b929c6e7391b 100644 --- a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs +++ b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs @@ -2607,15 +2607,42 @@ private IOperation CreateBoundNegatedPatternOperation(BoundNegatedPattern boundN private IOperation CreateBoundBinaryPatternOperation(BoundBinaryPattern boundBinaryPattern) { - return new BinaryPatternOperation( - boundBinaryPattern.Disjunction ? BinaryOperatorKind.Or : BinaryOperatorKind.And, - (IPatternOperation)Create(boundBinaryPattern.Left), - (IPatternOperation)Create(boundBinaryPattern.Right), - boundBinaryPattern.InputType.GetPublicSymbol(), - boundBinaryPattern.NarrowedType.GetPublicSymbol(), - _semanticModel, - boundBinaryPattern.Syntax, - isImplicit: boundBinaryPattern.WasCompilerGenerated); + if (boundBinaryPattern.Left is not BoundBinaryPattern) + { + return createOperation(this, boundBinaryPattern, left: (IPatternOperation)Create(boundBinaryPattern.Left)); + } + + // Use a manual stack to avoid overflowing on deeply-nested binary patterns + var stack = ArrayBuilder.GetInstance(); + BoundBinaryPattern? current = boundBinaryPattern; + + do + { + stack.Push(current); + current = current.Left as BoundBinaryPattern; + } while (current != null); + + var result = (IPatternOperation)Create(stack.Peek().Left); + while (stack.TryPop(out current)) + { + result = createOperation(this, current, result); + } + + stack.Free(); + return result; + + static BinaryPatternOperation createOperation(CSharpOperationFactory @this, BoundBinaryPattern boundBinaryPattern, IPatternOperation left) + { + return new BinaryPatternOperation( + boundBinaryPattern.Disjunction ? BinaryOperatorKind.Or : BinaryOperatorKind.And, + left, + (IPatternOperation)@this.Create(boundBinaryPattern.Right), + boundBinaryPattern.InputType.GetPublicSymbol(), + boundBinaryPattern.NarrowedType.GetPublicSymbol(), + @this._semanticModel, + boundBinaryPattern.Syntax, + isImplicit: boundBinaryPattern.WasCompilerGenerated); + } } private ISwitchOperation CreateBoundSwitchStatementOperation(BoundSwitchStatement boundSwitchStatement) diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 0478b1efb9d6e..615eace122707 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -19,6 +19,8 @@ using System.Diagnostics; using Roslyn.Test.Utilities.TestGenerators; using System.Runtime.InteropServices; +using Microsoft.CodeAnalysis.FlowAnalysis; +using Microsoft.CodeAnalysis.Operations; namespace Microsoft.CodeAnalysis.CSharp.UnitTests.EndToEnd { @@ -789,6 +791,16 @@ .locals init (int V_0, //i IL_001f: ret } """); + + var tree = comp.SyntaxTrees[0]; + var isPattern = tree.GetRoot().DescendantNodes().OfType().Single(); + var model = comp.GetSemanticModel(tree); + var operation = model.GetOperation(isPattern); + Assert.NotNull(operation); + + for (; operation.Parent is not null; operation = operation.Parent) { } + + Assert.NotNull(ControlFlowGraph.Create((IMethodBodyOperation)operation)); }); } } diff --git a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs index e688c62a319ae..176bd0f1e9258 100644 --- a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs @@ -7559,15 +7559,42 @@ public override IOperation VisitRelationalPattern(IRelationalPatternOperation op public override IOperation VisitBinaryPattern(IBinaryPatternOperation operation, int? argument) { - return new BinaryPatternOperation( - operatorKind: operation.OperatorKind, - leftPattern: (IPatternOperation)VisitRequired(operation.LeftPattern), - rightPattern: (IPatternOperation)VisitRequired(operation.RightPattern), - inputType: operation.InputType, - narrowedType: operation.NarrowedType, - semanticModel: null, - syntax: operation.Syntax, - isImplicit: IsImplicit(operation)); + if (operation.LeftPattern is not IBinaryPatternOperation) + { + return createOperation(this, operation, (IPatternOperation)VisitRequired(operation.LeftPattern)); + } + + // Use a manual stack to avoid overflowing on deeply-nested binary patterns + var stack = ArrayBuilder.GetInstance(); + IBinaryPatternOperation? current = operation; + + do + { + stack.Push(current); + current = current.LeftPattern as IBinaryPatternOperation; + } while (current != null); + + var result = (IPatternOperation)VisitRequired(stack.Peek().LeftPattern); + while (stack.TryPop(out current)) + { + result = createOperation(this, current, result); + } + + stack.Free(); + return result; + + static BinaryPatternOperation createOperation(ControlFlowGraphBuilder @this, IBinaryPatternOperation operation, IPatternOperation left) + { + return new BinaryPatternOperation( + operatorKind: operation.OperatorKind, + leftPattern: left, + rightPattern: (IPatternOperation)@this.VisitRequired(operation.RightPattern), + inputType: operation.InputType, + narrowedType: operation.NarrowedType, + semanticModel: null, + syntax: operation.Syntax, + isImplicit: @this.IsImplicit(operation)); + } } public override IOperation VisitNegatedPattern(INegatedPatternOperation operation, int? argument) From d372b4b111c263702f4bf5488e8043d2afc1a87a Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Mon, 23 Sep 2024 17:20:59 -0700 Subject: [PATCH 03/11] Add one more location where binary patterns are visited, make the test code worst-case. --- .../Portable/BoundTree/BoundTreeRewriter.cs | 45 +++++++++++++++++++ .../CSharp/Test/EndToEnd/EndToEndTests.cs | 34 ++------------ 2 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs index 6f489cb0cf2ae..d8643e5259276 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs @@ -152,5 +152,50 @@ protected BoundTreeRewriterWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperat return left; } + + public sealed override BoundNode? VisitBinaryPattern(BoundBinaryPattern node) + { + BoundPattern child = node.Left; + + if (child.Kind != BoundKind.BinaryPattern) + { + return base.VisitBinaryPattern(node); + } + + var stack = ArrayBuilder.GetInstance(); + stack.Push(node); + + BoundBinaryPattern binary = (BoundBinaryPattern)child; + + while (true) + { + stack.Push(binary); + child = binary.Left; + + if (child.Kind != BoundKind.BinaryPattern) + { + break; + } + + binary = (BoundBinaryPattern)child; + } + + var left = (BoundPattern?)this.Visit(child); + Debug.Assert(left is { }); + + do + { + binary = stack.Pop(); + var right = (BoundPattern?)this.Visit(binary.Right); + Debug.Assert(right is { }); + left = binary.Update(binary.Disjunction, left, right, VisitType(binary.InputType), VisitType(binary.NarrowedType)); + } + while (stack.Count > 0); + + Debug.Assert((object)binary == node); + stack.Free(); + + return left; + } } } diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 615eace122707..93aa197d0e799 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -730,7 +730,7 @@ class XAttribute : System.Attribute { } public void ManyBinaryPatterns() { const string Preamble = $""" - int i = 1; + int i = 2; System.Console.Write(i is """; @@ -754,7 +754,8 @@ public void ManyBinaryPatterns() for (int i = 1; i < NumBinaryExpressions; i++) { builder.Append(Append); - builder.Append(i); + // Make sure the emitter has to handle lots of nodes + builder.Append(i * 2); } builder.AppendLine(Postscript); @@ -763,34 +764,7 @@ public void ManyBinaryPatterns() RunInThread(() => { var comp = CreateCompilation(source, options: TestOptions.DebugExe.WithConcurrentBuild(false)); - var verifier = CompileAndVerify(comp, expectedOutput: "1"); - verifier.VerifyIL("", """ - { - // Code size 32 (0x20) - .maxstack 2 - .locals init (int V_0, //i - bool V_1) - IL_0000: ldc.i4.1 - IL_0001: stloc.0 - IL_0002: ldloc.0 - IL_0003: ldc.i4 0x2edf - IL_0008: ble.un.s IL_000c - IL_000a: br.s IL_0010 - IL_000c: ldc.i4.1 - IL_000d: stloc.1 - IL_000e: br.s IL_0012 - IL_0010: ldc.i4.0 - IL_0011: stloc.1 - IL_0012: ldloc.1 - IL_0013: brtrue.s IL_0018 - IL_0015: ldc.i4.0 - IL_0016: br.s IL_0019 - IL_0018: ldc.i4.1 - IL_0019: call "void System.Console.Write(int)" - IL_001e: nop - IL_001f: ret - } - """); + CompileAndVerify(comp, expectedOutput: "1"); var tree = comp.SyntaxTrees[0]; var isPattern = tree.GetRoot().DescendantNodes().OfType().Single(); From c5124f3354ae8caddc527605be1cebcb494b324e Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Wed, 25 Sep 2024 16:15:44 -0700 Subject: [PATCH 04/11] Bugfixes --- .../CSharp/Portable/Binder/Binder_Patterns.cs | 10 ++-- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 12 +++-- .../FlowAnalysis/DefiniteAssignment.cs | 37 +++++++++----- .../PatternMatchingTests_ListPatterns.cs | 49 +++++++++++++++++++ .../Operations/OperationMapBuilder.cs | 22 +++++++++ 5 files changed, 108 insertions(+), 22 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs index 96a8d1584519c..4ff97b7520fc1 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs @@ -1736,12 +1736,13 @@ private BoundPattern BindBinaryPattern( { // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. - var binaryPatternStack = ArrayBuilder.GetInstance(); + var binaryPatternStack = ArrayBuilder<(BinaryPatternSyntax pat, bool permitDesignations)>.GetInstance(); BinaryPatternSyntax? currentNode = node; do { - binaryPatternStack.Push(currentNode); + permitDesignations = permitDesignations && currentNode.IsKind(SyntaxKind.AndPattern); + binaryPatternStack.Push((currentNode, permitDesignations)); currentNode = currentNode.Left as BinaryPatternSyntax; } while (currentNode != null); @@ -1749,8 +1750,9 @@ private BoundPattern BindBinaryPattern( BoundPattern? result = null; - while (binaryPatternStack.TryPop(out var binaryPattern)) + while (binaryPatternStack.TryPop(out var binaryPatternAndPermitDesignations)) { + (var binaryPattern, permitDesignations) = binaryPatternAndPermitDesignations; if (result == null) { Debug.Assert(binaryPattern.Left is not BinaryPatternSyntax); @@ -1776,9 +1778,9 @@ static BoundPattern bindBinaryPattern( bool isDisjunction = node.Kind() == SyntaxKind.OrPattern; if (isDisjunction) { + Debug.Assert(!permitDesignations); MessageID.IDS_FeatureOrPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken); - permitDesignations = false; // prevent designators under 'or' var right = binder.BindPattern(node.Right, inputType, permitDesignations, hasErrors, diagnostics); // Compute the common type. This algorithm is quadratic, but disjunctive patterns are unlikely to be huge diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 494b4683a0b23..6d9ceedb3339f 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -1002,16 +1002,17 @@ static bool patternMatchesNull(BoundPattern pattern) while (binaryPatterns.TryPop(out var currentBinary)) { - if (binary.Disjunction) + if (currentBinary.Disjunction) { // `a?.b(out x) is null or C` // pattern matches null if either subpattern matches null - currentPatternMatchesNull = currentPatternMatchesNull || patternMatchesNull(binary.Right); + currentPatternMatchesNull = currentPatternMatchesNull || patternMatchesNull(currentBinary.Right); + continue; } // `a?.b out x is not null and var c` // pattern matches null only if both subpatterns match null - currentPatternMatchesNull = currentPatternMatchesNull && patternMatchesNull(binary.Right); + currentPatternMatchesNull = currentPatternMatchesNull && patternMatchesNull(currentBinary.Right); } binaryPatterns.Free(); @@ -1049,14 +1050,15 @@ static bool patternMatchesNull(BoundPattern pattern) // both subpatterns must have the same bool test for the test to propagate out var leftNullTest = currentBoolTest; currentBoolTest = leftNullTest is null ? null : - leftNullTest != isBoolTest(binary.Right) ? null : + leftNullTest != isBoolTest(currentBinary.Right) ? null : leftNullTest; + continue; } // `(a != null && a.b(out x)) is true and true` matches `true` // `(a != null && a.b(out x)) is true and var x` matches `true` // `(a != null && a.b(out x)) is true and false` never matches and is a compile error - currentBoolTest ??= isBoolTest(binary.Right); + currentBoolTest ??= isBoolTest(currentBinary.Right); } binaryPatterns.Free(); diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 364990440b717..7c8fe033e1bdf 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -2063,25 +2063,36 @@ void assignPatternVariablesAndMarkReadFields(BoundPattern pattern, bool definite } case BoundKind.BinaryPattern: { - // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. - var pat = (BoundBinaryPattern)pattern; - while (true) + if (pat.Left is not BoundBinaryPattern) + { + bool def = definitely && !pat.Disjunction; + assignPatternVariablesAndMarkReadFields(pat.Left, def); + assignPatternVariablesAndMarkReadFields(pat.Right, def); + break; + } + + // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. + var stack = ArrayBuilder<(BoundBinaryPattern pattern, bool def)>.GetInstance(); + + do { definitely = definitely && !pat.Disjunction; - assignPatternVariablesAndMarkReadFields(pat.Right, definitely); + stack.Push((pat, definitely)); + pat = pat.Left as BoundBinaryPattern; + } while (pat is not null); - if (pat.Left is BoundBinaryPattern left) - { - pat = left; - } - else - { - assignPatternVariablesAndMarkReadFields(pat.Left, definitely); - break; - } + Debug.Assert(stack.Count > 0); + (pat, definitely) = stack.Peek(); + assignPatternVariablesAndMarkReadFields(pat.Left, definitely); + + while (stack.TryPop(out var patAndDef)) + { + assignPatternVariablesAndMarkReadFields(patAndDef.pattern.Right, patAndDef.def); } + + stack.Free(); break; } default: diff --git a/src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_ListPatterns.cs b/src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_ListPatterns.cs index 0dcfda3f30f71..1ac3077040c01 100644 --- a/src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_ListPatterns.cs +++ b/src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_ListPatterns.cs @@ -8867,6 +8867,55 @@ public void ListPattern_AbstractFlowPass_isBoolTest() ); } + [Fact, WorkItem(58738, "https://github.com/dotnet/roslyn/issues/58738")] + public void ListPattern_AbstractFlowPass_isBoolTest_Multiple() + { + var source = @" +var b = new[] { true }; + +if ((b is [var x] and x or x) is [true]) +{ +} + +if ((b is [var z] and z and z) is [true]) +{ +} +"; + var comp = CreateCompilationWithIndexAndRangeAndSpan(new[] { source, TestSources.GetSubArray }); + comp.VerifyEmitDiagnostics( + // 0.cs(4,16): error CS8780: A variable may not be declared within a 'not' or 'or' pattern. + // if ((b is [var x] and x or x) is [true]) + Diagnostic(ErrorCode.ERR_DesignatorBeneathPatternCombinator, "x").WithLocation(4, 16), + // 0.cs(4,23): error CS0029: Cannot implicitly convert type 'bool' to 'bool[]' + // if ((b is [var x] and x or x) is [true]) + Diagnostic(ErrorCode.ERR_NoImplicitConv, "x").WithArguments("bool", "bool[]").WithLocation(4, 23), + // 0.cs(4,23): error CS0165: Use of unassigned local variable 'x' + // if ((b is [var x] and x or x) is [true]) + Diagnostic(ErrorCode.ERR_UseDefViolation, "x").WithArguments("x").WithLocation(4, 23), + // 0.cs(4,28): error CS0029: Cannot implicitly convert type 'bool' to 'bool[]' + // if ((b is [var x] and x or x) is [true]) + Diagnostic(ErrorCode.ERR_NoImplicitConv, "x").WithArguments("bool", "bool[]").WithLocation(4, 28), + // 0.cs(4,34): error CS8985: List patterns may not be used for a value of type 'bool'. No suitable 'Length' or 'Count' property was found. + // if ((b is [var x] and x or x) is [true]) + Diagnostic(ErrorCode.ERR_ListPatternRequiresLength, "[true]").WithArguments("bool").WithLocation(4, 34), + // 0.cs(4,34): error CS0021: Cannot apply indexing with [] to an expression of type 'bool' + // if ((b is [var x] and x or x) is [true]) + Diagnostic(ErrorCode.ERR_BadIndexLHS, "[true]").WithArguments("bool").WithLocation(4, 34), + // 0.cs(8,23): error CS0029: Cannot implicitly convert type 'bool' to 'bool[]' + // if ((b is [var z] and z and z) is [true]) + Diagnostic(ErrorCode.ERR_NoImplicitConv, "z").WithArguments("bool", "bool[]").WithLocation(8, 23), + // 0.cs(8,29): error CS0029: Cannot implicitly convert type 'bool' to 'bool[]' + // if ((b is [var z] and z and z) is [true]) + Diagnostic(ErrorCode.ERR_NoImplicitConv, "z").WithArguments("bool", "bool[]").WithLocation(8, 29), + // 0.cs(8,35): error CS8985: List patterns may not be used for a value of type 'bool'. No suitable 'Length' or 'Count' property was found. + // if ((b is [var z] and z and z) is [true]) + Diagnostic(ErrorCode.ERR_ListPatternRequiresLength, "[true]").WithArguments("bool").WithLocation(8, 35), + // 0.cs(8,35): error CS0021: Cannot apply indexing with [] to an expression of type 'bool' + // if ((b is [var z] and z and z) is [true]) + Diagnostic(ErrorCode.ERR_BadIndexLHS, "[true]").WithArguments("bool").WithLocation(8, 35) + ); + } + [Fact, WorkItem(58738, "https://github.com/dotnet/roslyn/issues/58738")] public void ListPattern_AbstractFlowPass_patternMatchesNull() { diff --git a/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs b/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs index b6811ca8ebd2d..5f8d5053c0b70 100644 --- a/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/OperationMapBuilder.cs @@ -54,6 +54,28 @@ private sealed class Walker : OperationWalker return null; } + public override object? VisitBinaryPattern(IBinaryPatternOperation operation, Dictionary argument) + { + // In order to handle very large nested patterns, we implement manual iteration here. Our operations are not order sensitive, + // so we don't need to maintain a stack, just iterate through every level. + while (true) + { + RecordOperation(operation, argument); + Visit(operation.RightPattern, argument); + if (operation.LeftPattern is IBinaryPatternOperation nested) + { + operation = nested; + } + else + { + Visit(operation.LeftPattern, 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 1595759b54f04160fdcb00f3f851c036072e5e67 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 26 Sep 2024 11:58:26 -0700 Subject: [PATCH 05/11] PR feedback. --- .../CSharp/Portable/Binder/Binder_Patterns.cs | 26 +++++++-------- .../Portable/Binder/DecisionDagBuilder.cs | 22 +++---------- .../Portable/Binder/LocalBinderFactory.cs | 23 ++++++------- .../Portable/BoundTree/BoundTreeWalker.cs | 11 ++++--- .../MemberSemanticModel.NodeMapBuilder.cs | 32 +++++++++++++++++-- .../CSharp/Test/EndToEnd/EndToEndTests.cs | 18 +++++------ .../Operations/ControlFlowGraphBuilder.cs | 7 ++-- 7 files changed, 76 insertions(+), 63 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs index 4ff97b7520fc1..8cf9077623e2a 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs @@ -1748,19 +1748,19 @@ private BoundPattern BindBinaryPattern( Debug.Assert(binaryPatternStack.Count > 0); - BoundPattern? result = null; + var binaryPatternAndPermitDesignations = binaryPatternStack.Pop(); + BoundPattern? result = BindPattern(binaryPatternAndPermitDesignations.pat.Left, inputType, binaryPatternAndPermitDesignations.permitDesignations, hasErrors, diagnostics); - while (binaryPatternStack.TryPop(out var binaryPatternAndPermitDesignations)) + do { - (var binaryPattern, permitDesignations) = binaryPatternAndPermitDesignations; - if (result == null) - { - Debug.Assert(binaryPattern.Left is not BinaryPatternSyntax); - result = BindPattern(binaryPattern.Left, inputType, permitDesignations, hasErrors, diagnostics); - } - - result = bindBinaryPattern(result, this, binaryPattern, inputType, permitDesignations, hasErrors, diagnostics); - } + result = bindBinaryPattern( + result, + this, + binaryPatternAndPermitDesignations, + inputType, + hasErrors, + diagnostics); + } while (binaryPatternStack.TryPop(out binaryPatternAndPermitDesignations)); binaryPatternStack.Free(); Debug.Assert(result != null); @@ -1769,12 +1769,12 @@ private BoundPattern BindBinaryPattern( static BoundPattern bindBinaryPattern( BoundPattern preboundLeft, Binder binder, - BinaryPatternSyntax node, + (BinaryPatternSyntax pat, bool permitDesignations) patternAndPermitDesignations, TypeSymbol inputType, - bool permitDesignations, bool hasErrors, BindingDiagnosticBag diagnostics) { + (BinaryPatternSyntax node, bool permitDesignations) = patternAndPermitDesignations; bool isDisjunction = node.Kind() == SyntaxKind.OrPattern; if (isDisjunction) { diff --git a/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs b/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs index acb20e79ed0d0..0eeadbfbfea7a 100644 --- a/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs +++ b/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs @@ -656,25 +656,13 @@ private Tests MakeTestsAndBindingsForBinaryPattern( currentNode = currentNode.Left as BoundBinaryPattern; } while (currentNode != null); - Tests? result = null; - BoundDagTemp? currentOutput = null; + currentNode = binaryPatternStack.Pop(); + Tests? result = MakeTestsAndBindings(input, currentNode.Left, out output, bindings); - while (binaryPatternStack.TryPop(out currentNode)) + do { - if (result == null) - { - Debug.Assert(currentNode.Left is not BoundBinaryPattern); - Debug.Assert(currentOutput == null); - result = MakeTestsAndBindings(input, currentNode.Left, out currentOutput, bindings); - } - - Debug.Assert(currentOutput != null); - result = makeTestsAndBindingsForBinaryPattern(this, result, currentOutput, input, currentNode, out currentOutput, bindings); - } - - Debug.Assert(result != null); - Debug.Assert(currentOutput != null); - output = currentOutput; + result = makeTestsAndBindingsForBinaryPattern(this, result, output, input, currentNode, out output, bindings); + } while (binaryPatternStack.TryPop(out currentNode)); binaryPatternStack.Free(); diff --git a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs index 3b14683a96c8d..7c3ceab199d81 100644 --- a/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs +++ b/src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs @@ -799,22 +799,17 @@ public override void VisitSwitchExpression(SwitchExpressionSyntax node) public override void VisitBinaryPattern(BinaryPatternSyntax node) { // Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually. - PatternSyntax currentPattern = node; - - var rightPatternStack = ArrayBuilder.GetInstance(); - - while (currentPattern is BinaryPatternSyntax binaryPattern) - { - rightPatternStack.Push(binaryPattern.Right); - currentPattern = binaryPattern.Left; - } - - do + while (true) { - Visit(currentPattern); - } while (rightPatternStack.TryPop(out currentPattern)); + Visit(node.Right); + if (node.Left is not BinaryPatternSyntax binOp) + { + Visit(node.Left); + break; + } - rightPatternStack.Free(); + node = binOp; + } } public override void VisitIfStatement(IfStatementSyntax node) diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs index 9598ad87de250..e0327ba45b1bf 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs @@ -127,10 +127,11 @@ protected BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator this.Visit(current); - while (rightOperands.TryPop(out current)) + current = rightOperands.Pop(); + do { this.Visit(current); - } + } while (rightOperands.TryPop(out current)); rightOperands.Free(); return null; @@ -165,10 +166,12 @@ protected virtual void BeforeVisitingSkippedBoundBinaryOperatorChildren(BoundBin Visit(current); - while (rightOperands.TryPop(out current)) + current = rightOperands.Pop(); + + do { Visit(current); - } + } while (rightOperands.TryPop(out current)); rightOperands.Free(); return null; diff --git a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs index 1b5ecb4b89077..c6b464588d7f8 100644 --- a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs +++ b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs @@ -189,12 +189,11 @@ public override BoundNode Visit(BoundNode node) _map.Add(current.Syntax, current); } - // In machine-generated code we frequently end up with binary operator trees that are deep on the left, + // In machine-generated code we frequently end up with binary operator or pattern trees that are deep on the left, // such as a + b + c + d ... // To avoid blowing the call stack, we build an explicit stack to handle the left-hand recursion. - var binOp = current as BoundBinaryOperator; - if (binOp != null) + if (current is BoundBinaryOperator binOp) { var stack = ArrayBuilder.GetInstance(); @@ -223,6 +222,33 @@ public override BoundNode Visit(BoundNode node) stack.Free(); } + else if (current is BoundBinaryPattern binaryPattern) + { + var stack = ArrayBuilder.GetInstance(); + + stack.Push(binaryPattern.Right); + BoundPattern currentPattern = binaryPattern.Left; + binaryPattern = currentPattern as BoundBinaryPattern; + + while (binaryPattern != null) + { + if (ShouldAddNode(binaryPattern)) + { + _map.Add(binaryPattern.Syntax, binaryPattern); + } + + stack.Push(binaryPattern.Right); + currentPattern = binaryPattern.Left; + binaryPattern = currentPattern as BoundBinaryPattern; + } + + do + { + Visit(currentPattern); + } while (stack.TryPop(out currentPattern)); + + stack.Free(); + } else { base.Visit(current); diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 93aa197d0e799..3614cc2f4fc04 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -729,36 +729,36 @@ class XAttribute : System.Attribute { } [Fact] public void ManyBinaryPatterns() { - const string Preamble = $""" + const string preamble = $""" int i = 2; System.Console.Write(i is """; - const string Append = $""" + const string append = $""" or """; - const string Postscript = """ + const string postscript = """ ? 1 : 0); """; - const int NumBinaryExpressions = 12_000; + const int numBinaryExpressions = 12_000; - var builder = new StringBuilder(Preamble.Length + Postscript.Length + Append.Length * NumBinaryExpressions + 5 /* Max num digit characters */ * NumBinaryExpressions); + var builder = new StringBuilder(preamble.Length + postscript.Length + append.Length * numBinaryExpressions + 5 /* Max num digit characters */ * numBinaryExpressions); - builder.AppendLine(Preamble); + builder.AppendLine(preamble); builder.Append(0); - for (int i = 1; i < NumBinaryExpressions; i++) + for (int i = 1; i < numBinaryExpressions; i++) { - builder.Append(Append); + builder.Append(append); // Make sure the emitter has to handle lots of nodes builder.Append(i * 2); } - builder.AppendLine(Postscript); + builder.AppendLine(postscript); var source = builder.ToString(); RunInThread(() => diff --git a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs index 176bd0f1e9258..c6be8616a014e 100644 --- a/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs +++ b/src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs @@ -7574,11 +7574,12 @@ public override IOperation VisitBinaryPattern(IBinaryPatternOperation operation, current = current.LeftPattern as IBinaryPatternOperation; } while (current != null); - var result = (IPatternOperation)VisitRequired(stack.Peek().LeftPattern); - while (stack.TryPop(out current)) + current = stack.Pop(); + var result = (IPatternOperation)VisitRequired(current.LeftPattern); + do { result = createOperation(this, current, result); - } + } while (stack.TryPop(out current)); stack.Free(); return result; From 22d8e1edd4e5764081689a42312fd415a13ea701 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 26 Sep 2024 15:42:09 -0700 Subject: [PATCH 06/11] More feedback and fixes for CI breaks. --- .../CSharp/Portable/Binder/Binder_Patterns.cs | 39 +++++++++++-------- .../Portable/Binder/DecisionDagBuilder.cs | 2 +- .../OverloadResolution/OverloadResolution.cs | 2 +- .../Portable/BoundTree/BoundTreeRewriter.cs | 13 +++---- .../Portable/BoundTree/BoundTreeVisitors.cs | 21 +++++----- .../Portable/BoundTree/BoundTreeWalker.cs | 21 ++++------ .../Portable/BoundTree/NullabilityRewriter.cs | 4 +- .../Portable/BoundTree/UnboundLambda.cs | 2 +- .../CSharp/Portable/CodeGen/Optimizer.cs | 2 +- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 15 ++++--- .../NullableWalker.DebugVerifier.cs | 13 ++++--- .../Portable/FlowAnalysis/NullableWalker.cs | 11 ++++-- .../FlowAnalysis/NullableWalker_Patterns.cs | 12 ++++-- .../Lowering/LocalRewriter/LocalRewriter.cs | 2 +- .../Operations/CSharpOperationFactory.cs | 7 ++-- .../Test/Semantic/Semantics/OperatorTests.cs | 2 +- 16 files changed, 93 insertions(+), 75 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs index 8cf9077623e2a..7296ef8683f60 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs @@ -1749,7 +1749,9 @@ private BoundPattern BindBinaryPattern( Debug.Assert(binaryPatternStack.Count > 0); var binaryPatternAndPermitDesignations = binaryPatternStack.Pop(); - BoundPattern? result = BindPattern(binaryPatternAndPermitDesignations.pat.Left, inputType, binaryPatternAndPermitDesignations.permitDesignations, hasErrors, diagnostics); + BoundPattern result = BindPattern(binaryPatternAndPermitDesignations.pat.Left, inputType, binaryPatternAndPermitDesignations.permitDesignations, hasErrors, diagnostics); + var narrowedTypeCandidates = ArrayBuilder.GetInstance(2); + collectCandidates(result, narrowedTypeCandidates); do { @@ -1758,11 +1760,13 @@ private BoundPattern BindBinaryPattern( this, binaryPatternAndPermitDesignations, inputType, + narrowedTypeCandidates, hasErrors, diagnostics); } while (binaryPatternStack.TryPop(out binaryPatternAndPermitDesignations)); binaryPatternStack.Free(); + narrowedTypeCandidates.Free(); Debug.Assert(result != null); return result; @@ -1771,6 +1775,7 @@ static BoundPattern bindBinaryPattern( Binder binder, (BinaryPatternSyntax pat, bool permitDesignations) patternAndPermitDesignations, TypeSymbol inputType, + ArrayBuilder narrowedTypeCandidates, bool hasErrors, BindingDiagnosticBag diagnostics) { @@ -1784,27 +1789,11 @@ static BoundPattern bindBinaryPattern( var right = binder.BindPattern(node.Right, inputType, permitDesignations, hasErrors, diagnostics); // Compute the common type. This algorithm is quadratic, but disjunctive patterns are unlikely to be huge - var narrowedTypeCandidates = ArrayBuilder.GetInstance(2); - collectCandidates(preboundLeft, narrowedTypeCandidates); collectCandidates(right, narrowedTypeCandidates); var narrowedType = leastSpecificType(node, narrowedTypeCandidates, diagnostics) ?? inputType; - narrowedTypeCandidates.Free(); return new BoundBinaryPattern(node, disjunction: isDisjunction, preboundLeft, right, inputType: inputType, narrowedType: narrowedType, hasErrors); - static void collectCandidates(BoundPattern pat, ArrayBuilder candidates) - { - if (pat is BoundBinaryPattern { Disjunction: true } p) - { - collectCandidates(p.Left, candidates); - collectCandidates(p.Right, candidates); - } - else - { - candidates.Add(pat.NarrowedType); - } - } - TypeSymbol? leastSpecificType(SyntaxNode node, ArrayBuilder candidates, BindingDiagnosticBag diagnostics) { Debug.Assert(candidates.Count >= 2); @@ -1865,9 +1854,25 @@ static void collectCandidates(BoundPattern pat, ArrayBuilder candida MessageID.IDS_FeatureAndPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken); var right = binder.BindPattern(node.Right, preboundLeft.NarrowedType, permitDesignations, hasErrors, diagnostics); + narrowedTypeCandidates.Clear(); + narrowedTypeCandidates.Add(right.NarrowedType); return new BoundBinaryPattern(node, disjunction: isDisjunction, preboundLeft, right, inputType: inputType, narrowedType: right.NarrowedType, hasErrors); } } + + static void collectCandidates(BoundPattern pat, ArrayBuilder candidates) + { + if (pat is BoundBinaryPattern { Disjunction: true } p) + { + collectCandidates(p.Left, candidates); + collectCandidates(p.Right, candidates); + } + else + { + candidates.Add(pat.NarrowedType); + } + } + } } } diff --git a/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs b/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs index 0eeadbfbfea7a..8597cc16bdf25 100644 --- a/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs +++ b/src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs @@ -657,7 +657,7 @@ private Tests MakeTestsAndBindingsForBinaryPattern( } while (currentNode != null); currentNode = binaryPatternStack.Pop(); - Tests? result = MakeTestsAndBindings(input, currentNode.Left, out output, bindings); + Tests result = MakeTestsAndBindings(input, currentNode.Left, out output, bindings); do { diff --git a/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs b/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs index ad118c7a047fa..be9dfb48fce48 100644 --- a/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs +++ b/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs @@ -3239,7 +3239,7 @@ public override BoundNode Visit(BoundNode node) return null; } - protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { throw ExceptionUtilities.Unreachable(); } diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs index d8643e5259276..59dc52fdaf04c 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeRewriter.cs @@ -78,23 +78,22 @@ protected BoundTreeRewriterWithStackGuard(int recursionDepth) [return: NotNullIfNotNull(nameof(node))] public override BoundNode? Visit(BoundNode? node) { - var expression = node as BoundExpression; - if (expression != null) + if (node is BoundExpression or BoundPattern) { - return VisitExpressionWithStackGuard(ref _recursionDepth, expression); + return VisitExpressionOrPatternWithStackGuard(ref _recursionDepth, node); } return base.Visit(node); } - protected BoundExpression VisitExpressionWithStackGuard(BoundExpression node) + protected BoundNode VisitExpressionOrPatternWithStackGuard(BoundNode node) { - return VisitExpressionWithStackGuard(ref _recursionDepth, node); + return VisitExpressionOrPatternWithStackGuard(ref _recursionDepth, node); } - protected sealed override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected sealed override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { - return (BoundExpression)base.Visit(node); + return base.Visit(node); } } diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeVisitors.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeVisitors.cs index 5e93ce79faa9b..397ded7692115 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeVisitors.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeVisitors.cs @@ -184,9 +184,9 @@ public static Location GetTooLongOrComplexExpressionErrorLocation(BoundNode node { SyntaxNode syntax = node.Syntax; - if (!(syntax is ExpressionSyntax)) + if (syntax is not (ExpressionSyntax or PatternSyntax)) { - syntax = syntax.DescendantNodes(n => !(n is ExpressionSyntax)).OfType().FirstOrDefault() ?? syntax; + syntax = syntax.DescendantNodes(n => n is not (ExpressionSyntax or PatternSyntax)).FirstOrDefault(n => n is ExpressionSyntax or PatternSyntax) ?? syntax; } return syntax.GetFirstToken().GetLocation(); @@ -194,12 +194,13 @@ public static Location GetTooLongOrComplexExpressionErrorLocation(BoundNode node } /// - /// Consumers must provide implementation for . + /// Consumers must provide implementation for . /// [DebuggerStepThrough] - protected BoundExpression VisitExpressionWithStackGuard(ref int recursionDepth, BoundExpression node) + protected BoundNode VisitExpressionOrPatternWithStackGuard(ref int recursionDepth, BoundNode node) { - BoundExpression result; + Debug.Assert(node is BoundExpression or BoundPattern); + BoundNode result; recursionDepth++; #if DEBUG int saveRecursionDepth = recursionDepth; @@ -209,11 +210,11 @@ protected BoundExpression VisitExpressionWithStackGuard(ref int recursionDepth, { EnsureSufficientExecutionStack(recursionDepth); - result = VisitExpressionWithoutStackGuard(node); + result = VisitExpressionOrPatternWithoutStackGuard(node); } else { - result = VisitExpressionWithStackGuard(node); + result = VisitExpressionOrPatternWithStackGuard(node); } #if DEBUG @@ -235,11 +236,11 @@ protected virtual bool ConvertInsufficientExecutionStackExceptionToCancelledBySt #nullable enable [DebuggerStepThrough] - private BoundExpression? VisitExpressionWithStackGuard(BoundExpression node) + private BoundNode? VisitExpressionOrPatternWithStackGuard(BoundNode node) { try { - return VisitExpressionWithoutStackGuard(node); + return VisitExpressionOrPatternWithoutStackGuard(node); } catch (InsufficientExecutionStackException ex) { @@ -250,6 +251,6 @@ protected virtual bool ConvertInsufficientExecutionStackExceptionToCancelledBySt /// /// We should be intentional about behavior of derived classes regarding guarding against stack overflow. /// - protected abstract BoundExpression? VisitExpressionWithoutStackGuard(BoundExpression node); + protected abstract BoundNode? VisitExpressionOrPatternWithoutStackGuard(BoundNode node); } } diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs index e0327ba45b1bf..2dacfcdd0adac 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Collections.Immutable; +using System.Diagnostics; using Microsoft.CodeAnalysis.PooledObjects; namespace Microsoft.CodeAnalysis.CSharp @@ -69,23 +70,23 @@ protected BoundTreeWalkerWithStackGuard(int recursionDepth) public override BoundNode? Visit(BoundNode? node) { - var expression = node as BoundExpression; - if (expression != null) + if (node is BoundExpression or BoundPattern) { - return VisitExpressionWithStackGuard(ref _recursionDepth, expression); + return VisitExpressionOrPatternWithStackGuard(ref _recursionDepth, node); } return base.Visit(node); } - protected BoundExpression VisitExpressionWithStackGuard(BoundExpression node) + protected BoundNode VisitExpressionOrPatternWithStackGuard(BoundNode node) { - return VisitExpressionWithStackGuard(ref _recursionDepth, node); + return VisitExpressionOrPatternWithStackGuard(ref _recursionDepth, node); } - protected sealed override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected sealed override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { - return (BoundExpression)base.Visit(node); + Debug.Assert(node is BoundExpression or BoundPattern); + return base.Visit(node); } } @@ -151,7 +152,6 @@ protected virtual void BeforeVisitingSkippedBoundBinaryOperatorChildren(BoundBin var rightOperands = ArrayBuilder.GetInstance(); rightOperands.Push(node.Right); - BeforeVisitingSkippedBoundBinaryPatternChildren(binary); rightOperands.Push(binary.Right); BoundPattern? current = binary.Left; @@ -159,7 +159,6 @@ protected virtual void BeforeVisitingSkippedBoundBinaryOperatorChildren(BoundBin while (current.Kind == BoundKind.BinaryPattern) { binary = (BoundBinaryPattern)current; - BeforeVisitingSkippedBoundBinaryPatternChildren(binary); rightOperands.Push(binary.Right); current = binary.Left; } @@ -177,10 +176,6 @@ protected virtual void BeforeVisitingSkippedBoundBinaryOperatorChildren(BoundBin return null; } - protected virtual void BeforeVisitingSkippedBoundBinaryPatternChildren(BoundBinaryPattern node) - { - } - public sealed override BoundNode? VisitCall(BoundCall node) { if (node.ReceiverOpt is BoundCall receiver1) diff --git a/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs b/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs index db5389f0b60c1..c7dbecf3fab10 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/NullabilityRewriter.cs @@ -12,9 +12,9 @@ namespace Microsoft.CodeAnalysis.CSharp { internal sealed partial class NullabilityRewriter : BoundTreeRewriter { - protected override BoundExpression? VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode? VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { - return (BoundExpression)Visit(node); + return Visit(node); } public override BoundNode? VisitBinaryOperator(BoundBinaryOperator node) diff --git a/src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs b/src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs index 1447dff19215c..884b37029d32b 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs @@ -360,7 +360,7 @@ public static void GetReturnTypes(ArrayBuilder<(BoundReturnStatement, TypeWithAn return null; } - protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { throw ExceptionUtilities.Unreachable(); } diff --git a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs index 5f7659f3ae3ce..c628a423e0faa 100644 --- a/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs +++ b/src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs @@ -532,7 +532,7 @@ private BoundExpression VisitExpressionCoreWithStackGuard(BoundExpression node, } } - protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { throw ExceptionUtilities.Unreachable(); } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 6d9ceedb3339f..ff6cb09ee4364 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -361,19 +361,18 @@ protected BoundNode VisitAlways(BoundNode node) [DebuggerStepThrough] private BoundNode VisitWithStackGuard(BoundNode node) { - var expression = node as BoundExpression; - if (expression != null) + if (node is BoundExpression or BoundPattern) { - return VisitExpressionWithStackGuard(ref _recursionDepth, expression); + return VisitExpressionOrPatternWithStackGuard(ref _recursionDepth, node); } return base.Visit(node); } [DebuggerStepThrough] - protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { - return (BoundExpression)base.Visit(node); + return base.Visit(node); } protected override bool ConvertInsufficientExecutionStackExceptionToCancelledByStackGuardException() @@ -1112,6 +1111,12 @@ public override BoundNode VisitConstantPattern(BoundConstantPattern node) throw ExceptionUtilities.Unreachable(); } + public override BoundNode VisitBinaryPattern(BoundBinaryPattern node) + { + // All patterns are handled by VisitPattern + throw ExceptionUtilities.Unreachable(); + } + public override BoundNode VisitTupleLiteral(BoundTupleLiteral node) { return VisitTupleExpression(node); diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs index 4bb319b9dbd50..8250ef868480d 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs @@ -77,10 +77,13 @@ private void VerifyExpression(BoundExpression expression, bool overrideSkippedEx } } - protected override BoundExpression? VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode? VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { - VerifyExpression(node); - return (BoundExpression)base.Visit(node); + if (node is BoundExpression expr) + { + VerifyExpression(expr); + } + return base.Visit(node); } public override BoundNode? Visit(BoundNode? node) @@ -92,9 +95,9 @@ private void VerifyExpression(BoundExpression expression, bool overrideSkippedEx // _snapshotManager.VerifyNode(node); //} - if (node is BoundExpression expr) + if (node is BoundExpression or BoundPattern) { - return VisitExpressionWithStackGuard(ref _recursionDepth, expr); + return VisitExpressionOrPatternWithStackGuard(ref _recursionDepth, node); } return base.Visit(node); } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 9eaac42526bd5..5201f68631571 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -3452,12 +3452,17 @@ private void DeclareLocals(ImmutableArray locals) return null; } - protected override BoundExpression? VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode? VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { + Debug.Assert(node is BoundExpression or BoundPattern); Debug.Assert(!IsConditionalState); SetInvalidResult(); - _ = base.VisitExpressionWithoutStackGuard(node); - VisitExpressionWithoutStackGuardEpilogue(node); + _ = base.VisitExpressionOrPatternWithoutStackGuard(node); + if (node is BoundExpression expr) + { + VisitExpressionWithoutStackGuardEpilogue(expr); + } + return null; } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs index c55d166d05aa5..ef1325e4f6b3c 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs @@ -122,13 +122,14 @@ public override BoundNode VisitBinaryPattern(BoundBinaryPattern node) current = current.Left as BoundBinaryPattern; } while (current != null); - Debug.Assert(stack.Peek().Left is not BoundBinaryPattern); - Visit(stack.Peek().Left); + current = stack.Pop(); + Debug.Assert(current.Left is not BoundBinaryPattern); + Visit(current.Left); - while (stack.TryPop(out current)) + do { Visit(current.Right); - } + } while (stack.TryPop(out current)); stack.Free(); return null; @@ -221,10 +222,13 @@ private void LearnFromAnyNullPatterns( var current = p; while (true) { + // We don't need to visit in order here because we're only moving analysis in one direction: + // towards MaybeNull. Visiting the right or left first has no impact on the final state. LearnFromAnyNullPatterns(inputSlot, inputType, current.Right); if (current.Left is BoundBinaryPattern left) { current = left; + VisitForRewriting(current); } else { diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs index 6243dba63e5fe..26169860b09b2 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs @@ -270,7 +270,7 @@ private PEModuleBuilder? EmitModule } } - var visited = VisitExpressionWithStackGuard(node); + var visited = (BoundExpression)VisitExpressionOrPatternWithStackGuard(node); // If you *really* need to change the type, consider using an indirect method // like compound assignment does (extra flag only passed when it is an expression diff --git a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs index 0b929c6e7391b..e2478d0f67888 100644 --- a/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs +++ b/src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs @@ -2622,11 +2622,12 @@ private IOperation CreateBoundBinaryPatternOperation(BoundBinaryPattern boundBin current = current.Left as BoundBinaryPattern; } while (current != null); - var result = (IPatternOperation)Create(stack.Peek().Left); - while (stack.TryPop(out current)) + current = stack.Pop(); + var result = (IPatternOperation)Create(current.Left); + do { result = createOperation(this, current, result); - } + } while (stack.TryPop(out current)); stack.Free(); return result; diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs index 015c6a452124c..74966d84778a6 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/OperatorTests.cs @@ -8971,7 +8971,7 @@ static void M(C x, C y) private sealed class EmptyRewriter : BoundTreeRewriter { - protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpression node) + protected override BoundNode VisitExpressionOrPatternWithoutStackGuard(BoundNode node) { throw new NotImplementedException(); } From 977493eb6e5ea3f8d6116ea465bafd8c05d630e3 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 26 Sep 2024 16:53:53 -0700 Subject: [PATCH 07/11] Another while->do conversion --- .../CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 7c8fe033e1bdf..e009b16967e55 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -2083,14 +2083,13 @@ void assignPatternVariablesAndMarkReadFields(BoundPattern pattern, bool definite pat = pat.Left as BoundBinaryPattern; } while (pat is not null); - Debug.Assert(stack.Count > 0); - (pat, definitely) = stack.Peek(); - assignPatternVariablesAndMarkReadFields(pat.Left, definitely); + var patAndDef = stack.Pop(); + assignPatternVariablesAndMarkReadFields(patAndDef.pattern.Left, patAndDef.def); - while (stack.TryPop(out var patAndDef)) + do { assignPatternVariablesAndMarkReadFields(patAndDef.pattern.Right, patAndDef.def); - } + } while (stack.TryPop(out patAndDef)); stack.Free(); break; From fc0582553429fc97ea6ca5681565d54f954bdd95 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Thu, 26 Sep 2024 16:54:50 -0700 Subject: [PATCH 08/11] Remove unnecessary assert --- src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs index 7296ef8683f60..6a885bcc9944b 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs @@ -1767,7 +1767,6 @@ private BoundPattern BindBinaryPattern( binaryPatternStack.Free(); narrowedTypeCandidates.Free(); - Debug.Assert(result != null); return result; static BoundPattern bindBinaryPattern( From 89a21fc568c205de27ad3eaa357a73516e521460 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Fri, 27 Sep 2024 11:56:26 -0700 Subject: [PATCH 09/11] Reduce nested binary expressions to speed up test run, provide DebugVerifier override as well. --- .../NullableWalker.DebugVerifier.cs | 17 +++++++++++++++++ .../CSharp/Test/EndToEnd/EndToEndTests.cs | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs index 8250ef868480d..1fcb2e285bca8 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs @@ -262,6 +262,23 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperatorBase node) } } + public override BoundNode? VisitBinaryPattern(BoundBinaryPattern node) + { + // There can be deep recursion on the left side, so verify iteratively to avoid blowing the stack + while (true) + { + Visit(node.Right); + + if (node.Left is not BoundBinaryPattern child) + { + Visit(node.Left); + return null; + } + + node = child; + } + } + public override BoundNode? VisitConvertedTupleLiteral(BoundConvertedTupleLiteral node) { Visit(node.SourceTuple); diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 3614cc2f4fc04..35af6d168f4b5 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -743,7 +743,7 @@ public void ManyBinaryPatterns() ? 1 : 0); """; - const int numBinaryExpressions = 12_000; + const int numBinaryExpressions = 5_000; var builder = new StringBuilder(preamble.Length + postscript.Length + append.Length * numBinaryExpressions + 5 /* Max num digit characters */ * numBinaryExpressions); From 4d5160d36a86c54b96d232a9b2ad29c78e2d7300 Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Fri, 27 Sep 2024 14:06:06 -0700 Subject: [PATCH 10/11] Only take 1 incremental snapshot, improving perf by over 10x. --- .../CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs index ef1325e4f6b3c..316bedbc13338 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs @@ -117,12 +117,15 @@ public override BoundNode VisitBinaryPattern(BoundBinaryPattern node) BoundBinaryPattern current = node; do { - TakeIncrementalSnapshot(current); stack.Push(current); current = current.Left as BoundBinaryPattern; } while (current != null); current = stack.Pop(); + // We don't need to snapshot on the way down because the left spine of the tree will always have the same span start, and each + // call to TakeIncrementalSnapshot would overwrite the previous one with the new state. This can be a _significant_ performance + // improvement for deeply nested binary patterns; over 10x faster in some pathological cases. + TakeIncrementalSnapshot(current); Debug.Assert(current.Left is not BoundBinaryPattern); Visit(current.Left); From 4c33987ebb62145ded4db381625483d7d0abe2fa Mon Sep 17 00:00:00 2001 From: Fredric Silberberg Date: Tue, 1 Oct 2024 10:20:45 -0700 Subject: [PATCH 11/11] PR feedback. --- .../CSharp/Portable/Binder/Binder_Patterns.cs | 7 ++++--- src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs | 12 +++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs index 6a885bcc9944b..9755a4667ff09 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs @@ -1758,7 +1758,8 @@ private BoundPattern BindBinaryPattern( result = bindBinaryPattern( result, this, - binaryPatternAndPermitDesignations, + binaryPatternAndPermitDesignations.pat, + binaryPatternAndPermitDesignations.permitDesignations, inputType, narrowedTypeCandidates, hasErrors, @@ -1772,13 +1773,13 @@ private BoundPattern BindBinaryPattern( static BoundPattern bindBinaryPattern( BoundPattern preboundLeft, Binder binder, - (BinaryPatternSyntax pat, bool permitDesignations) patternAndPermitDesignations, + BinaryPatternSyntax node, + bool permitDesignations, TypeSymbol inputType, ArrayBuilder narrowedTypeCandidates, bool hasErrors, BindingDiagnosticBag diagnostics) { - (BinaryPatternSyntax node, bool permitDesignations) = patternAndPermitDesignations; bool isDisjunction = node.Kind() == SyntaxKind.OrPattern; if (isDisjunction) { diff --git a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs index 35af6d168f4b5..f1febfb5c79bc 100644 --- a/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs +++ b/src/Compilers/CSharp/Test/EndToEnd/EndToEndTests.cs @@ -726,17 +726,19 @@ class XAttribute : System.Attribute { } step => Assert.True(step.Outputs.Single().Value is ClassDeclarationSyntax { Identifier.ValueText: "C1" })); } - [Fact] - public void ManyBinaryPatterns() + [Theory] + [InlineData("or", "1")] + [InlineData("and not", "0")] + public void ManyBinaryPatterns(string pattern, string expectedOutput) { const string preamble = $""" int i = 2; System.Console.Write(i is """; - const string append = $""" + string append = $""" - or + {pattern} """; const string postscript = """ @@ -764,7 +766,7 @@ public void ManyBinaryPatterns() RunInThread(() => { var comp = CreateCompilation(source, options: TestOptions.DebugExe.WithConcurrentBuild(false)); - CompileAndVerify(comp, expectedOutput: "1"); + CompileAndVerify(comp, expectedOutput: expectedOutput); var tree = comp.SyntaxTrees[0]; var isPattern = tree.GetRoot().DescendantNodes().OfType().Single();