Avoid asserting unvisited nested binary operators #72935
Conversation
ghost
added
Area-Compilers
untriaged
|
@jjonescz Could you please provide more details about the change. What is happening and why. Why you think the change is the right change to make? |
|
It doesn't look like the scenario in the issue is an error scenario, If that is the case, it is not clear to me why things aren't visited |
|
There's a comment in the code that tried explaining this: // Nested binary operators on the left side are not visited by BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator.To elaborate - BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator doesn't visit nested BoundBinaryOperators, it only visits the operands. This is done for a good reason - to avoid stack overflows. Hence I think BoundBinaryOperators not being visited is the expected behavior and the "debug verifier" should be adjusted to not assert when BoundBinaryOperators are not visited. |
| GetValEscape(binary.Right, scopeOfTheContainingExpression)); | ||
| #if DEBUG | ||
| // Nested binary operators on the left side are not visited by BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator. | ||
| var assertNestedVisited = binary.Left is not BoundBinaryOperator; |
There was a problem hiding this comment.
I'm not sure how exactly would we do that - for expression like A+B+C+D calling Visit at this point on the left-hand side would recursively visit A+B+C, then we would get to this point again for A+B+C and call Visit on A+B - so we would visit the same nodes multiple times which we probably don't want (apart from another assert firing, it seems inefficient).
There was a problem hiding this comment.
I'm not sure how exactly would we do that
The assert checks that the node is in some set. Perhaps we can add an entry in the into the set right here.
There was a problem hiding this comment.
You're right, that looks like a good solution, thanks!
| ), | ||
| GetValEscape(binary.Right, scopeOfTheContainingExpression | ||
| #if DEBUG | ||
| , assertNestedVisited |
There was a problem hiding this comment.
It's not necessary to pass it for the right-hand side, thanks.
There was a problem hiding this comment.
It's not necessary to pass it for the right-hand side
Not necessary or wrong?
There was a problem hiding this comment.
Wrong in the sense that we would not check that the right-hand side was visited.
| return Math.Max(GetValEscape(binary.Left, scopeOfTheContainingExpression), | ||
| GetValEscape(binary.Right, scopeOfTheContainingExpression)); | ||
| #if DEBUG | ||
| // Nested binary operators on the left side are not visited by BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator. |
There was a problem hiding this comment.
Nested binary operators on the left side are not visited by BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator.
It is not obvious to me that not visiting a user-defined binary operator on the left is the right thing to do for the purpose of this visitor. The fact that the base class skips it completely doesn't mean that doing the same is appropriate for every derived visitor.
There was a problem hiding this comment.
Note, BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator also does similar trick for calls. Should they be treated specially too?
There was a problem hiding this comment.
Note, BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator also does similar trick for calls. Should they be treated specially too?
That does not seem needed, as the ref safety analysis doesn't seem to (even indirectly) call AssertVisited on the receiver like it does for binary operator (where escape of the left-hand side is checked which in turn calls AssertVisited on it)
There was a problem hiding this comment.
However, it is true we need special handling in other methods than just GetValEscape, for example CheckValEscape
There was a problem hiding this comment.
It is not obvious to me that not visiting a user-defined binary operator on the left is the right thing to do for the purpose of this visitor. The fact that the base class skips it completely doesn't mean that doing the same is appropriate for every derived visitor.
RefSafetyAnalysis does not currently override VisitBinaryOperator, so assuming that's correct, the optimization seems valid, i.e., it should be transparent to RefSafetyAnalysis that only the operands are visited, not the operators themselves.
Note that the handling of binary operators (and also the special handling of user-defined binary operators) doesn't happen in VisitBinaryOperator, it happens in GetValEscape/CheckValEscape, so it's unrelated to the walker's visiting behavior.
There was a problem hiding this comment.
Note, BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator also does similar trick for calls. Should they be treated specially too?
That does not seem needed, as the ref safety analysis doesn't seem to (even indirectly) call AssertVisited on the receiver like it does for binary operator (where escape of the left-hand side is checked which in turn calls AssertVisited on it)
Actually, AssertVisited is called on receivers. But there is not such problem for a different reason - the BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator handling of nested calls doesn't skip visiting any nodes.
|
Done with review pass (commit 3) |
2 participants
Fixes #72873.