When the same SECTION is encountered multiple times in one cycle, catch crashes on assert

[mjerabek]

Describe the bug

Suppose we have nested SECTIONs in a helper function and call that function from one TEST_CASE, from the same section. Then the same sections are entered multiple times (because they have the same name and location). The same could be achieved by a loop.

In that scenario, Catch asserts/segfaults (see the attached compiler explorer link).

Expected behavior

Either it should work "as expected", or it should error out with a reasonable error message. Also, the documentation of SECTION should include clear warning that this is not supported (also a link to DYNAMIC_SECTION would be handy).

Reproduction steps

https://godbolt.org/z/rMP8vf6xe

Platform information:

• Catch version: devel

[mjerabek]

Note: this should hopefully be somewhat coherent, but I've been staring at the code for a few hours and it's late, so sorry if it isnt :smile:

The problem

When the section is acquired the second time, we have already ctx.completedCycle(), so we don't call tryOpen(). But since the section isComplete(), close() is still called, leading to the crash, because we go past the root tracker.

So, the minimal solution is to not enter the section if ctx.completedCycle(). But after the "section" finished for the first time, it NeedsAnotherRun (because it has more subsections), and therefore isOpen() is true. And it generally cannot be false for NeedsAnotherRun, because we can enter such a section if !ctx.completedCycle() (TODO: really?).

So, could we just check ctx.completedCycle() in isOpen()? Well, no. Because all currently opened sections (with more following children) would suddenly become not opened.

The solution

So, we cannot hack it, we have to do it properly. The goal is to track if a section was already encountered during the current cycle and if so, it's an error.

I think the most robust solution is to remember the serial number of the cycle in each Tracker and in SectionTracker::acquire, if we find that the requested section's last recorded cycle is the current cycle, that's the error. (Side note: we don't have to worry about overflow, one-digit binary counter would be enough for this)

Extension of the solution

Alternatively, instead of erroring out, we can say "ok, let's make this a new section and make it work". That would mean that the section tracker tracks also "generation" in addition to name and location.

What about generators

I think it could work the same - if we encounter a generator a second time in the same cycle, we just say it's a different generator. Although I have not studied how generators are implemented yet - even understanding how sections work took me a few hours.

Why it might not be a good idea after all

The problem comes with reporting - how do we distinguish between the two identical sections? We don't. Hence, I think we should force the user to insert (dynamic) sections to make the sections unique.

How to report the error

Catch should probably just throw an exception. But could the malicious user just catch(...){} it? Probably yes. Am I getting overly paranoid? Definitely.

[mjerabek]

Another potentially interesting test case with generators:

TEST_CASE("foo") {
    std::cout << "new run\n";
    for (int i = 0; i < 3; ++i) {
        SECTION("section") {
            int n = GENERATE(1, 2);
            std::cout << n << "\n";
        }
    }
}

Edit: probably not interesting after all - it's the SECTION that will make it or break it. And generators directly in a loop are handled correctly now.