Use manual recursion for binary patterns

[333fred]

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 #73439.

[333fred]

@dotnet/roslyn-compiler for review. I cannot stress strongly enough, review with whitespace off. Otherwise all the extractions to local functions will be much harder to review.

[333fred]

Also note: A much simpler workaround would just be to break up IsBuildOnlyDiagnostic into a few different pattern arms. I didn't take that approach because that would just leave a maintenance task on us every so often to fix up the compilation when some builds randomly start failing; this seems much less painful long term, and I don't think the original code is unreasonable.

[333fred]

Converting this back to draft while I find a few more places that binary operators are handled that we probably need to handle patterns as well.

[333fred]

Alright. There are a couple of locations in the optimizer and emit where we do specially handle binary operators, but we these nodes don't survive the local rewriter, and there are existing assertions that we don't have any patterns at that point. This is ready for review.

[AlekseyTs]

@333fred It looks like there are legitimate test failures

[333fred]

@AlekseyTs I believe they should be fixed now.

[AlekseyTs]

@333fred It looks like EndToEnd tests are crashing, likely related to the added test

[AlekseyTs]

collectCandidates(preboundLeft, narrowedTypeCandidates);

Perhaps the assumption from the comment above that "disjunctive patterns are unlikely to be huge" has been proven wrong. Therefore, it might make sense to keep the set of candidate types from the left as we bubble up, instead of revisiting the subtree over and over again. Also, it might be worth considering to collect only distinct types to decrease amount of memory used. #Closed

[333fred]

I'll take the former suggestion, but I'll avoid the latter suggestion for now. We can do it in a followup if it's necessary, but it's a bigger change and I don't think we need to look at it yet.

[AlekseyTs]

while (currentPattern is BinaryPatternSyntax binaryPattern)

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

[333fred]

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

[AlekseyTs]

// Users (such as ourselves) can have many, many nested binary patterns. To avoid crashing, do left recursion manually.

Should we add a similar override to DebugVerifier? #Closed

[AlekseyTs]

Done with review pass (commit 5)

[AlekseyTs]

LGTM (commit 10)

[333fred]

@dotnet/roslyn-compiler for a second review

[cston]

(BinaryPatternSyntax pat, bool permitDesignations) patternAndPermitDesignations

Consider splitting the tuple into two separate parameters.

[333fred]

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

[cston]

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.

[cston]

(ExpressionSyntax or PatternSyntax)

Consider creating a static local function since this same check is used three times.

[333fred]

This doesn't really feel like the type of thing that needs to be extracted to a local function; it honestly feels like that would impede readability, not help. I'd prefer to leave this as is.

[cston]

or

Consider testing deep nesting of and patterns as well since there is some difference in Binder.BindBinaryPattern().

Console.Write(i is not 1 and not 2 and ...);