Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Use manual recursion for binary patterns #75212

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
192 changes: 118 additions & 74 deletions src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1734,100 +1734,144 @@ private BoundPattern BindBinaryPattern(
bool hasErrors,
BindingDiagnosticBag diagnostics)
{
bool isDisjunction = node.Kind() == SyntaxKind.OrPattern;
if (isDisjunction)
// Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually.

var binaryPatternStack = ArrayBuilder<(BinaryPatternSyntax pat, bool permitDesignations)>.GetInstance();
BinaryPatternSyntax? currentNode = node;

do
{
MessageID.IDS_FeatureOrPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken);
permitDesignations = permitDesignations && currentNode.IsKind(SyntaxKind.AndPattern);
binaryPatternStack.Push((currentNode, permitDesignations));
currentNode = currentNode.Left as BinaryPatternSyntax;
} while (currentNode != null);

permitDesignations = false; // prevent designators under 'or'
var left = BindPattern(node.Left, inputType, permitDesignations, hasErrors, diagnostics);
var right = BindPattern(node.Right, inputType, permitDesignations, hasErrors, diagnostics);
Debug.Assert(binaryPatternStack.Count > 0);

// Compute the common type. This algorithm is quadratic, but disjunctive patterns are unlikely to be huge
var narrowedTypeCandidates = ArrayBuilder<TypeSymbol>.GetInstance(2);
collectCandidates(left, narrowedTypeCandidates);
collectCandidates(right, narrowedTypeCandidates);
var narrowedType = leastSpecificType(node, narrowedTypeCandidates, diagnostics) ?? inputType;
narrowedTypeCandidates.Free();
var binaryPatternAndPermitDesignations = binaryPatternStack.Pop();
BoundPattern result = BindPattern(binaryPatternAndPermitDesignations.pat.Left, inputType, binaryPatternAndPermitDesignations.permitDesignations, hasErrors, diagnostics);
var narrowedTypeCandidates = ArrayBuilder<TypeSymbol>.GetInstance(2);
collectCandidates(result, narrowedTypeCandidates);

return new BoundBinaryPattern(node, disjunction: isDisjunction, left, right, inputType: inputType, narrowedType: narrowedType, hasErrors);
do
{
result = bindBinaryPattern(
result,
this,
binaryPatternAndPermitDesignations,
inputType,
narrowedTypeCandidates,
hasErrors,
diagnostics);
} while (binaryPatternStack.TryPop(out binaryPatternAndPermitDesignations));

static void collectCandidates(BoundPattern pat, ArrayBuilder<TypeSymbol> candidates)
{
if (pat is BoundBinaryPattern { Disjunction: true } p)
{
collectCandidates(p.Left, candidates);
collectCandidates(p.Right, candidates);
}
else
{
candidates.Add(pat.NarrowedType);
}
}
binaryPatternStack.Free();
narrowedTypeCandidates.Free();
return result;

TypeSymbol? leastSpecificType(SyntaxNode node, ArrayBuilder<TypeSymbol> candidates, BindingDiagnosticBag diagnostics)
static BoundPattern bindBinaryPattern(
BoundPattern preboundLeft,
Binder binder,
(BinaryPatternSyntax pat, bool permitDesignations) patternAndPermitDesignations,
Copy link
Member

Choose a reason for hiding this comment

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

(BinaryPatternSyntax pat, bool permitDesignations) patternAndPermitDesignations

Consider splitting the tuple into two separate parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was originally separate parameters; I moved it to the tuple for simplicity of the calling code.

Copy link
Member

Choose a reason for hiding this comment

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

The method is not using the tuple as a tuple though. It feels like the caller should be responsible for deconstructing the tuple rather than the method.

TypeSymbol inputType,
ArrayBuilder<TypeSymbol> narrowedTypeCandidates,
bool hasErrors,
BindingDiagnosticBag diagnostics)
{
(BinaryPatternSyntax node, bool permitDesignations) = patternAndPermitDesignations;
bool isDisjunction = node.Kind() == SyntaxKind.OrPattern;
if (isDisjunction)
{
Debug.Assert(candidates.Count >= 2);
CompoundUseSiteInfo<AssemblySymbol> 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++)
Debug.Assert(!permitDesignations);
MessageID.IDS_FeatureOrPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken);

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
collectCandidates(right, narrowedTypeCandidates);
var narrowedType = leastSpecificType(node, narrowedTypeCandidates, diagnostics) ?? inputType;

return new BoundBinaryPattern(node, disjunction: isDisjunction, preboundLeft, right, inputType: inputType, narrowedType: narrowedType, hasErrors);

TypeSymbol? leastSpecificType(SyntaxNode node, ArrayBuilder<TypeSymbol> candidates, BindingDiagnosticBag diagnostics)
{
TypeSymbol candidate = candidates[i];
bestSoFar = lessSpecificCandidate(bestSoFar, candidate, ref useSiteInfo) ?? bestSoFar;
Debug.Assert(candidates.Count >= 2);
CompoundUseSiteInfo<AssemblySymbol> 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++)
{
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;
}
// second pass: check that it is no more specific than any candidate.
for (int i = 0, n = candidates.Count; i < n; i++)

// 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<AssemblySymbol> useSiteInfo)
{
TypeSymbol candidate = candidates[i];
TypeSymbol? spoiler = lessSpecificCandidate(candidate, bestSoFar, ref useSiteInfo);
if (spoiler is null)
if (bestSoFar.Equals(possiblyLessSpecificCandidate, TypeCompareKind.AllIgnoreOptions))
{
bestSoFar = null;
break;
// 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;
}

// Our specificity criteria are transitive
Debug.Assert(spoiler.Equals(bestSoFar, TypeCompareKind.ConsiderEverything));
}
}
else
{
MessageID.IDS_FeatureAndPattern.CheckFeatureAvailability(diagnostics, node.OperatorToken);

diagnostics.Add(node, useSiteInfo);
return bestSoFar;
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);
}
}

// 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<AssemblySymbol> useSiteInfo)
static void collectCandidates(BoundPattern pat, ArrayBuilder<TypeSymbol> candidates)
{
if (pat is BoundBinaryPattern { Disjunction: true } p)
{
if (bestSoFar.Equals(possiblyLessSpecificCandidate, TypeCompareKind.AllIgnoreOptions))
{
// When the types are equivalent, merge them.
return bestSoFar.MergeEquivalentTypes(possiblyLessSpecificCandidate, VarianceKind.Out);
}
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
{
// We have no improved candidate to offer.
return null;
}
collectCandidates(p.Left, candidates);
collectCandidates(p.Right, candidates);
}
else
{
candidates.Add(pat.NarrowedType);
}
}
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);
}
}
}
}
64 changes: 45 additions & 19 deletions src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -645,33 +645,59 @@ private Tests MakeTestsAndBindingsForBinaryPattern(
out BoundDagTemp output,
ArrayBuilder<BoundPatternBinding> bindings)
{
var builder = ArrayBuilder<Tests>.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<BoundBinaryPattern>.GetInstance();
var currentNode = bin;

do
{
binaryPatternStack.Push(currentNode);
currentNode = currentNode.Left as BoundBinaryPattern;
} while (currentNode != null);

currentNode = binaryPatternStack.Pop();
Tests result = MakeTestsAndBindings(input, currentNode.Left, out output, bindings);

do
{
result = makeTestsAndBindingsForBinaryPattern(this, result, output, input, currentNode, out output, bindings);
} while (binaryPatternStack.TryPop(out currentNode));

binaryPatternStack.Free();

return result;

static Tests makeTestsAndBindingsForBinaryPattern(DecisionDagBuilder @this, Tests leftTests, BoundDagTemp leftOutput, BoundDagTemp input, BoundBinaryPattern bin, out BoundDagTemp output, ArrayBuilder<BoundPatternBinding> bindings)
{
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))
var builder = ArrayBuilder<Tests>.GetInstance(2);
if (bin.Disjunction)
{
output = input;
return result;
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<Tests>.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<Tests>.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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<PatternSyntax>.GetInstance();

while (currentPattern is BinaryPatternSyntax binaryPattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

while (currentPattern is BinaryPatternSyntax binaryPattern)

Consider using a loop with a post-condition. We know the condition is true on entry.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that results in less-clear code, like we have in VisitBinaryExpression above. I'd prefer to keep this as is.

{
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))
Expand Down
16 changes: 16 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,22 @@ 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.
while (true)
{
Visit(node.Right);
if (node.Left is not BinaryPatternSyntax binOp)
{
Visit(node.Left);
break;
}

node = binOp;
}
}

public override void VisitIfStatement(IfStatementSyntax node)
{
Visit(node.Condition, _enclosing);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
Loading
Loading