DIP 1034: detect noreturn statements #12222
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#12222" |
| fail_compilation/warn12809.d(108): Warning: statement is not reachable | ||
| fail_compilation/warn12809.d(115): Warning: statement is not reachable | ||
| fail_compilation/warn12809.d(122): Warning: statement is not reachable |
There was a problem hiding this comment.
The warning message is too vague. How about "Warning: statement 'statement description' is not reachable"?
There was a problem hiding this comment.
We've had that same message for many years. A better message would be off topic for this PR, but you can submit one that addresses that if you like.
|
Could the error message be : |
That would require some complex modifications to the code, as that information is not collected by blockExit(). |
|
The test cases raise the question of throwing. If we flag those statements as not reachable, does it mean that noreturn functions are implicitly nothrow ? |
That is a good question. However that shouldn't be the justification to stall this pr, as that would be better off discuss in the ng. |
I disagree. |
What are you suggesting then? Making noreturn functions nothrow? In this PR? |
| @@ -107,6 +107,8 @@ int blockExit(Statement s, FuncDeclaration func, bool mustNotThrow) | |||
| return; | |||
| } | |||
| } | |||
| if (s.exp.type.toBasetype().isTypeNoreturn()) | |||
| result = BE.halt; | |||
| if (canThrow(s.exp, func, mustNotThrow)) | |||
| result |= BE.throw_; | |||
There was a problem hiding this comment.
@Geod24 line 113 answers your question.
Throwing is orthogonal from returning, so no, noreturn does not imply nothrow. You can see that in the blockExit() code where throwing is "ORed" in. |
|
For reference: This was temporarily reverted in #12228 as it broke one project on Buildkite, and will be re-introduced soon. |
|
@Geod24 a better solution would be to change abort() from noreturn back to void, rather than stopping progress in implementing DIP1034. |
|
I just reverted the PR that caused the CI to go red, but point taken. |
This re-introduce commit 0fc1fc0, which was merged in dlang#12222 and subsequently reverted in commit 3c84532 (PR dlang#12228) as it broke some projects on Buildkite. The project has had a new release, so it should be safe to re-introduce.
Meaning we now get warnings for unreachable code.