Visit binary operators in ref safety analysis

[jjonescz]

Alternative to #72935.
Fixes #72873.

[jaredpar]

Given this method is only used for assertion purposes did you consider giving it an assert style named and making it conditional on DEBUG? For example:

[Conditional("DEBUG")]
private void AssertVisitCore(BoundNode? node)
``` #Resolved

[jjonescz]

Making it just Conditional would fail to compile as it uses _visited field which is only available #if DEBUG.

[jaredpar]

Can use an #if DEBUG within the method too. Personally I find the code easier to process when minimize the number of directives, particularly for call sites. Will defer to others here.

[cston]

In this case, there are only a couple of call sites, so I think I prefer the explicit-ness of #if DEBUG at the declaration and at the call sites so it's clear at the callers that there is no impact on RELEASE builds.

[AlekseyTs]

My first choice in a situation like this will be to rely on conditional compilation vs. the attribute. It completely eliminates the method (including in produced metadata) for release build. Whereas the attribute doesn't have this effect. In my opinion the attribute is useful when we deal with a public API, which is not the case here.

[AlekseyTs]

VisitCore

I think we should use a name that clearly indicates the purpose of this method. "VisitCore" doesn't tell to me that the purpose is to perform some debug time validation. Quite the opposite, it tells me the method performs some critical task and it is surprising to see that it not called in release build. #Closed

[AlekseyTs]

this.VisitCore(op);

It looks like, If we were not overriding BoundTreeWalker.VisitBinaryOperator, we would be calling VisitCore for each parent, before visiting any child. It also looks like this is not what this method is doing. It looks like it visits children and then calls VisitCore for the parent. I think that manual stack management should preserve the regular order of operations. #Closed

[AlekseyTs]

VisitBinaryOperator

I do not see any good reason to use drastically different algorithm in this method by comparison to BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator.VisitBinaryOperator implementation. This could be explained by desire to call VisitCore on the way "out" rather than on the way "in", but I think we actually want to do that the other way. #Closed

[AlekseyTs]

        if (_visited is { } && _visited.Count <= MaxTrackVisited)

At the very least, I think we should call VisitCore here. But I also think that what we should really do is to override VisitCall and have it call VisitCore at the right time instead in order to preserve the regular order of operations. For example, it looks like if we simply call VisitCore here, we would be calling it twice for the top level BoundCall node. #Closed

---

Refers to: src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs:708 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

[AlekseyTs]

Done with review pass (commit 2)

[AlekseyTs]

        rightOperands.Push(binary.Right);

As a better alternative to overriding this method in derived type and duplicating the stack management process, I think we can add a protected virtual method called "BeforeVisitingSkippedBoundBinaryOperatorChildren" or something like that and do BeforeVisitingSkippedBoundBinaryOperatorChildren(binary); here and in the loop below. RefSafetyAnalysis would override it and call VisitCore from that method. #Closed

---

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs:117 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

[AlekseyTs]

            rightOperands.Push(binary.Right);

Here is the second place to call BeforeVisitingSkippedBoundBinaryOperatorChildren(binary); #Closed

---

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs:124 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

[AlekseyTs]

                calls.Push(node);

Similar to VisitBinaryOperator, here we could call BeforeVisitingSkippedBoundCallChildren(node);, override it RefSafetyAnalysis and call VisitCore there. #Closed

---

Refers to: src/Compilers/CSharp/Portable/BoundTree/BoundTreeWalker.cs:150 in e7f5146. [](commit_id = e7f5146, deletion_comment = False)

[AlekseyTs]

LGTM (commit 3)