fix(merged_tree): introduce cycle detection to mitigate a comment bundling bug #611

I'm curious how you came to the conclusion that the stack overflow comes from this particular method

It's been a bit since I wrote the original patch, so I don't remember all the details, but I think this method was what was repeated in the logs of the original panic, so that's what I went on to patch. But I think the bug in comment bundling just creates cycles in general, and so it might be hard to predict which particular method is going to explode because of that.

This seems off: it is expected that a merged tree will contain multiple identical copies of CommutativeChildSeparator, which aren't symptoms of any problem.
The loop detection should rather be done at the Leader or AstNode level

I think you're right; would you have a suggestion as to which part of the code I could put the loop detection in? What I'm thinking of is doing it right after the construction of the AST honestly, by employing one of the cycle detection algorithms

I think the bug in comment bundling just creates cycles in general

Hmm, I'm curious how that would happen. But if it does, then those would be cycles of AstNodes already? If that's the case, then the stack overflow would happen basically on any further transformation run on the trees, such as their matching.

I think you're right; would you have a suggestion as to which part of the code I could put the loop detection in?

I don't know, I would try to find a concrete example that reaches a stack overflow and take it from there. I can run a large-scale benchmark again and try to find a fitting case.

I found an example: #609 (comment)