Fix #17906 regression with deleted catch(Exception) block

[adamdruppe]

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 had 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. While the nothrow test on the recursive function is not corrected in this commit, it matters less when we no longer use the wrong data to suppress codegen.

I expect the cost of an unnecessary catch block is much smaller than the cost this bug has caused.

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.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

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#23127"

[rikkimax]

I am inclined to think that this is the wrong direction to go in.

Instead blockExit should be looking at if a function is in progress, if it is, assume it throws.

[adamdruppe]

Now this test result surprises me. I can't even explain why it'd fail yet.

but @rikkimax see other potential approach here: #23126

[adamdruppe]

Problem with the "function is in progress" idea is if you assume it throws, it breaks nothrow nested recursive functions that currently work (including ones in dmd's backend).

I think the perfect solution is to separate out the inference function from the isolated statement analysis. But I'm not even sure.

[rikkimax]

The correct answer is to remove nothrow from the type system entirely.

The compiler is not capable of resolving effects like this correctly.

[adamdruppe]

I'm writing stuff in both PR threads now lol but this test failure described here: #23126 (comment)

generic code that does try X(); catch(Exception e) throw e; is still inferred nothrow if X is nothrow because it deletes the whole catch block, including the rethrow.

lol i thought this PR was the lower regression risk but apparently not.

[adamdruppe]

but yeah im inclined to agree nothrow being even in the language is a mistake, maybe i should just make it a harmless noop.... but idk

[rikkimax]

I don't understand why blockExit is returning that the function doesn't throw.

It should be.

[adamdruppe]

That's what the other PR is about, trying to fix blockExit. But basically the idea is a recursive call is going to have the same nothrow as the rest of the function, so it currently skips those calls, marking them a simple fallthrough so it defers to the rest of the analysis. If the function does nothing, that's nothrow, so that's the default assumption.

This works fine when you actually do analyze the whole function. But the try/catch semantic only looks at the try body in isolation, outside of context, that skip of the recursive thing doesn't work.

Inference does look at it all. So does explicit annotation. So the correct fix is probably to make this do it too. But that's tricky too because inferring the attribute is done after the AST modification - and as seen by the failing tests here it kinda has to be. So now we have a circular dependency....

this is why i opened the PRs btw, i figured these weren't quite right but i didn't know how to do it right, so want to check with you all before shipping anything. I'm perfectly willing to settle for incremental improvement but if a good solution comes up that's obviously the best.

[rikkimax]

Actually I think we're close here.

If nothrow is in progress, do what we are doing now, otherwise assume throws.

It's probably missing an and expression for some check somewhere.

[adamdruppe]

maybe the inference function could ignore the catch blocks if the try block is nothrow...