fix noreturn/implicit discard check logic #23681
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Superb work. |
|
Thanks for your hard work on this PR! Hint: mm: orc; opt: speed; options: -d:release |
This was referenced Jul 15, 2024
Merged
Closed
Araq
pushed a commit
that referenced
this pull request
Jul 16, 2024
…a simple analysis (#23839) follow up #23681 ref https://forum.nim-lang.org/t/11987 ref #23836 (comment)
Araq
pushed a commit
that referenced
this pull request
Jul 16, 2024
narimiran
pushed a commit
that referenced
this pull request
Aug 29, 2024
fixes #10440, fixes #13871, fixes #14665, fixes #19672, fixes #23677 The false positive in #23677 was caused by behavior in `implicitlyDiscardable` where only the last node of `if`/`case`/`try` etc expressions were considered, as in the final node of the final branch (in this case `else`). To fix this we use the same iteration in `implicitlyDiscardable` that we use in `endsInNoReturn`, with the difference that for an `if`/`case`/`try` statement to be implicitly discardable, all of its branches must be implicitly discardable. `noreturn` calls are also considered implicitly discardable for this reason, otherwise stuff like `if true: discardableCall() else: error()` doesn't compile. However `endsInNoReturn` also had bugs, one where `finally` was considered in noreturn checking when it shouldn't, another where only `nkIfStmt` was checked and not `nkIfExpr`, and the node given for the error message was bad. So `endsInNoReturn` now skips over `skipForDiscardable` which no longer contains `nkIfStmt`/`nkCaseStmt`/`nkTryStmt`, stores the first encountered returning node in a var parameter for the error message, and handles `finally` and `nkIfExpr`. Fixing #23677 already broke a line in `syncio` so some package code might be affected. (cherry picked from commit 42e8472)
narimiran
pushed a commit
that referenced
this pull request
Aug 30, 2024
) follow up #23681 ref https://forum.nim-lang.org/t/11987 (cherry picked from commit 648f82c)
narimiran
pushed a commit
that referenced
this pull request
Aug 30, 2024
…a simple analysis (#23839) follow up #23681 ref https://forum.nim-lang.org/t/11987 ref #23836 (comment) (cherry picked from commit b7a275d)
narimiran
pushed a commit
that referenced
this pull request
Aug 31, 2024
fixes #10440, fixes #13871, fixes #14665, fixes #19672, fixes #23677 The false positive in #23677 was caused by behavior in `implicitlyDiscardable` where only the last node of `if`/`case`/`try` etc expressions were considered, as in the final node of the final branch (in this case `else`). To fix this we use the same iteration in `implicitlyDiscardable` that we use in `endsInNoReturn`, with the difference that for an `if`/`case`/`try` statement to be implicitly discardable, all of its branches must be implicitly discardable. `noreturn` calls are also considered implicitly discardable for this reason, otherwise stuff like `if true: discardableCall() else: error()` doesn't compile. However `endsInNoReturn` also had bugs, one where `finally` was considered in noreturn checking when it shouldn't, another where only `nkIfStmt` was checked and not `nkIfExpr`, and the node given for the error message was bad. So `endsInNoReturn` now skips over `skipForDiscardable` which no longer contains `nkIfStmt`/`nkCaseStmt`/`nkTryStmt`, stores the first encountered returning node in a var parameter for the error message, and handles `finally` and `nkIfExpr`. Fixing #23677 already broke a line in `syncio` so some package code might be affected. (cherry picked from commit 42e8472)
narimiran
pushed a commit
that referenced
this pull request
Aug 31, 2024
…a simple analysis (#23839) follow up #23681 ref https://forum.nim-lang.org/t/11987 ref #23836 (comment) (cherry picked from commit b7a275d)
narimiran
pushed a commit
that referenced
this pull request
Aug 31, 2024
) follow up #23681 ref https://forum.nim-lang.org/t/11987 (cherry picked from commit 648f82c)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #10440, fixes #13871, fixes #14665, fixes #19672, fixes #23677
The false positive in #23677 was caused by behavior in
implicitlyDiscardablewhere only the last node ofif/case/tryetc expressions were considered, as in the final node of the final branch (in this caseelse). To fix this we use the same iteration inimplicitlyDiscardablethat we use inendsInNoReturn, with the difference that for anif/case/trystatement to be implicitly discardable, all of its branches must be implicitly discardable.noreturncalls are also considered implicitly discardable for this reason, otherwise stuff likeif true: discardableCall() else: error()doesn't compile.However
endsInNoReturnalso had bugs, one wherefinallywas considered in noreturn checking when it shouldn't, another where onlynkIfStmtwas checked and notnkIfExpr, and the node given for the error message was bad. SoendsInNoReturnnow skips overskipForDiscardablewhich no longer containsnkIfStmt/nkCaseStmt/nkTryStmt, stores the first encountered returning node in a var parameter for the error message, and handlesfinallyandnkIfExpr.Fixing #23677 already broke a line in
syncioso some package code might be affected.