Fix VariablesDeclaredWalker with recursive patterns

[RikkiGibson]

Closes #79489
@CyrusNajmabadi PTAL

[AlekseyTs]

    private void NoteDeclaredPatternVariables(BoundPattern pattern)

Instead of having a helper that must know how to dive deep into patterns and rely on containing nodes to call it, can we override appropriate Visit... methods for various bound patterns and rely on regular visit process to record information for them? #Closed

---

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/VariablesDeclaredWalker.cs:70 in e745150. [](commit_id = e745150, deletion_comment = False)

[AlekseyTs]

Done with review pass (commit 2), tests were not looked at. #Closed

[RikkiGibson]

can we override appropriate Visit... methods for various bound patterns and rely on regular visit process to record information for them?

I don't think this could be solved, for example, by only overriding Visit for the specific BoundPattern kinds, which directly contain variables. Instead you would have to declare all the Visit methods for all the pattern subtypes, and write the code to traverse the parts of the patterns which could contain variables. This is because the base type Visit methods already don't do this, to the best of my understanding. I think this approach, as well as the approach in iteration 2, would also end up requiring re-implementing "safe" traversal of binary patterns (because of the need to find variables in expr is > 0 and var variable, for example.)

A few alternatives I considered:

1. Declare an inner class PatternVisitor : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator, which we call into to visit patterns, which adds the variables it finds to VariablesDeclaredWalker._variablesDeclared. This would assume that we can't enter/exit the region in the middle of a pattern.
2. Change the base type class VariablesDeclaredWalker : AbstractRegionDataFlowPass and override some virtual method on DefiniteAssignment to allow reusing the definite assignment logic which traverses patterns in order to mark them as assigned.

It's possible I missed something that would make fixing this easier to accomplish. Let me know if you have any further thoughts.

[AlekseyTs]

I don't think this could be solved, for example, by only overriding Visit for the specific BoundPattern kinds, which directly contain variables. Instead you would have to declare all the Visit methods for all the pattern subtypes, and write the code to traverse the parts of the patterns which could contain variables.

I guess it would be fine for NoteDeclaredPatternVariables to implement its own traversal, but let's do it similar to the way DefiniteAssignmentPass.VisitPattern does it. I.e. it should throw for default case. #Closed

[AlekseyTs]

        if (IsInside)

Are there situations when we need to adjust the IsInside state while traversing the pattern? #Closed

---

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/VariablesDeclaredWalker.cs:72 in e745150. [](commit_id = e745150, deletion_comment = False)

[RikkiGibson]

Are there situations when we need to adjust the IsInside state while traversing the pattern?

It looks like it is possible to AnalyzeDataFlow with the Expression inside a pattern which takes constant values. I think only in error cases can you declare variables in such expressions, but, I added test RegionWithinSubpattern03 to make sure the API indicates that the variable declaration is present.

Out of date

[AlekseyTs]

Done with review pass (commit 9)

[RikkiGibson]

This walk is roughly based on DefiniteAssignment.VisitPattern.

[AlekseyTs]

LGTM (commit 10)