Fix regression with deleted catch(Exception) block

[adamdruppe]

I think it was introduced in 0be1f3d

when you have a recursive function, it stops trying to see if it throws and thus incorrectly marks it as fallthrough. Then later, in statement semantic you get into this block:

    /* If the try body never throws, we can eliminate any catches
     * of recoverable exceptions. */

and it elides the whole catch block, which leads to bizarre runtime results.

See https://forum.dlang.org/thread/qohwofoqnooswcbgyzja@forum.dlang.org

[dlang-bot]

Thanks for your pull request and interest in making D better, @adamdruppe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#23126"

[thewilsonator]

test case?

[adamdruppe]

Steve posted one in the linked thread. But first need to see if the old regression can be fixed without introducing new regressions. dmd can't build itself anymore here because nested recursive functions no longer are compatible with nothrow.

(though i compiled a few of my programs with the change and there were no problems. it comes down to using nothrow sometimes. but it is normally inferred in nested functions, so that should work without an explicit mark anyway.)

[adamdruppe]

looks like the build succeeds otherwise so added the test case now too.

@schveiguy found an old issue #17906 that refers to this and the test case is something he reduced.

im running some tests on the opend side too, it looks like a success there but there was obviously some divergence in the patch (the patch on opend was a one liner!)

[adamdruppe]

So this is a hacky whackamole fix rn because of that inferred thing. Make the test function a template and you get the same failure again.

I think the real fix is more involved - it probably shouldn't use this same function for both cases of inference and catch elision.

Other option is to skip the catch elision logic. It's probably not worth doing in the frontend anyway tbh.

[thewilsonator]

otherwise looks good

[thewilsonator]

need to update frontend.h

[adamdruppe]

kinda nuts the CI checks the useless frontend.h but not the actually important declaration.h

[adamdruppe]

but idk my problem is if i change the test to

void foo()(bool shouldthrow) // note the () here are new
{
    if(shouldthrow) {
        throw new Exception("here");
    }
    try {
        foo(true);
    }
    catch(Exception e) {
        // happy
    }
    catch(Throwable t) {
        assert(0, "should not reach here");
    }
}

void main()
{
    foo(false);
}

it still fails so i kinda wanna go with the other approach.....

[adamdruppe]

Here's the other approach I had in mind:

#23127

We can merge one or the other. Or technically could merge both as they attack different parts of the problem, but I'll leave that for y'all to decide.

That other approach i think is the one i'll ship with opend as I think I like it better.....

[rikkimax]

You shouldn't need a new field for this: nothrowInprocess exists.

[adamdruppe]

nothrow is not in process at the time the function is called, indeed, nothrow is never in process in the test case added here, as attributes are not inferred there at all.

It is looking at the try statement in complete isolation as part of TryCatchStatement early semantic, it removes part of the AST before even getting to inference.

The reason the other PR fails the code looks like this:

        try
        {
            foreach (e; r)
            {
                emplaceRef!E(result[cnt], e);
                ++cnt;
            }
        } catch (Exception e)
        {
            //https://issues.dlang.org/show_bug.cgi?id=22185
            //Make any uninitialized elements safely destructible.
            foreach (ref elem; result[cnt..$])
            {
                import core.internal.lifetime : emplaceInitializer;
                emplaceInitializer(elem);
            }
            throw e;
        }

And because of it removing that catch branch entirely if it thinks the try part is nothrow, the throw e in the catch is no longer considered as part of the nothrow inference - this is all done before inference.....