New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Issue 21443 - scope (failure) with a return breaks safety #14269
Conversation
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
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 "stable + dmd#14269" |
src/dmd/statementsem.d
Outdated
// @@@DEPRECATED_2.112@@@ | ||
// Deprecated in 2.100, transform into an error in 2.112 | ||
if (sc.os.tok == TOK.onScopeFailure) | ||
rs.deprecation("`return` statements cannot be in `scope(failure)` bodies. Use try-catch blocks for this purpose"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put "Use try-catch blocks for this purpose" in a deprecationSupplemental
.
|
||
where an error is circumvented by a return. If a return is indeed desired | ||
in such situations, then the solution is to simply use a try-catch block | ||
for the function body. In addition, `scope(exit)` and `scope(success)` already present this restriction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "In addition" is not related to to the try-catch block workaround, I would put it in a new paragraph and start with "Note" or something.
@dkorpel Done + rebased on top of stable. |
6a1f85f
to
4473e8a
Compare
Phobos seems to have some returns in |
The issue is here: https://github.com/dlang/phobos/blob/38243e3716087fbe8cbe364b2383bf73ef73cefa/std/typecons.d#L4908 I think we can replace this with a traditional |
In other words, I think this has already found a bug in Phobos. EDIT: oh wait, that's only inside a unittest. Just change the unittest, it's not testing that specific feature of phobos. |
Is the documentation updated? Great to see this in there! |
@RazvanN7 can you point the rationale out in https://dlang.org/deprecate.html ? |
No description provided.