-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
loop-break-value (issue #961) #1624
Conversation
I can't make line edits without an open PR, so I did this instead.
…icson2314-patch-1
[motivation]: #motivation | ||
|
||
This pattern is currently hard to implement without resorting to a function or | ||
closure wrapping the loop: |
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.
It's not that hard:
fn f() {
let outcome;
loop {
// get and process some input, e.g. from the user or from a list of
// files
let result = get_result();
if successful() {
outcome = result;
break;
}
// otherwise keep trying
};
use_the_result(outcome);
}
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.
Yes, you're right. I was able to reduce my problem to this pattern:
let computation = {
let mut result = None;
loop {
if let Some(r) = self.do_something() {
result = Some(s);
break;
}
}
result.unwrap().do_computation()
};
self.use(computation);
Lifetimes make some of these problems horrible to think about.
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.
Damn. The problem with being British is you expect people to spot the sarcasm.
- Having to mutable a variable just to return a value is inelegant
- Having to wrap with
Option
then unwrap just because there isn't an appropriate default value is clumsy - Having to do this while considering the lifetimes of generated references is confusing
- It's a lot of extra code. Compare the above to:
let computation = loop {
if let Some(r) = self.do_something() {
break r;
}
}.do_computation();
self.use(computation);
(noting that due to reference lifetimes it may be possible to give the value fed to do_computation()
a name in the outer scope).
Is that sufficient motivation?
The new lifetime rules under discussion may well simplify this problem, both with-and-without break-with-value, but I still think break-with-value is a neat pattern (or alternately, a generic break-from-block-with-value, but that opens another can of worms: the value if not broken out of).
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.
Well, @sfackler 's example works fine without a mutable variable or an Option
. Am I missing something?
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.
Maybe, and maybe it's a good thing. Sounds like it's time I un-learned to never leave variables uninitialised (thanks to Rust's great flow & lifetime analysis).
Points 3 and 4 still hold (the example above can be four words shorter), but this weakens the argument.
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.
Point 3 - please give an example where these confusing-to-consider lifetimes are involved.
Point 4 - "a lot of extra code" when talking about essentially one extra assignment?
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 current pattern is not necessarily hard, but the main problem I think is that the new pattern expresses much better what is going on.
It is immediately clear that the loop is meant to set the value, while with the code of @sfackler this is not te case. Especially when considering loops with more content or with multiple exit points.
Btw, I have a scary macro that mostly implements this as well as labeled blocks, in case you want to play around. |
Considering the fact that other blatantly stupid constructs like |
|
@notriddle good point; I have no idea. @upsuper: already discussed a lot; explicitly not part of the RFC |
I think that the requirement for |
```rust | ||
assert_eq!(loop { break; }, ()); | ||
assert_eq!(loop { break 5; }, 5); | ||
let x = 'a loop { |
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.
syntactic nit: this should be 'a: loop {
and 'b: loop {
on the line below.
I look forward to this compiling: fn main() {
}
struct X;
type B = Result<(X), (X)>; // That signature looks scary.
const please: () = ();
macro_rules! escape { ($x:expr, $y:expr) => (()); }
fn out(_: (), _: ()) { }
const now: B = Ok(X);
fn attempt_escape() -> B {
'prison: loop {
if break out (please, continue) {
escape! (oh, yeah?)
} else {
return now :(
// You got caught. Maybe next time you should try wearing cool sunnies like this:
B)
}
}
} |
This reverts commit 3651781.
I like this improvement a lot. Ruby is also an expression-oriented language and allows I especially like the fact that this is a nice improvement to an existing expression form with minimal and intuitive new syntax. Like the original issue thread, I have some concerns with other extensions, and am glad this RFC split out just |
I'm basically 👍 on this. Limiting to |
Nice. I'm still watching this but from what I gather no changes are needed to the RFC? |
@vitiral interesting point. But the RFC states that Besides, sometimes side-effects are much more important than return values. TBH I don't understand the Python ambiguity problem very well since it always seemed clear to me (yes, in Python). |
the return value of any statement ending in
So you don't need a
|
Would it be better if let x = for name in suggest_names() {
if like(name) {
break name;
}
} finally {
random_name()
}; |
@Amanieu do you mean use |
I'd like to know from the people who object to just using If not, there's no point bike-shedding further on alternative names. The choices involving |
I'm still very pro my suggestion of using On 3 Oct 2016 11:09 pm, "Diggory Blake" [email protected] wrote:
|
Something like the last 10 comments (I didn't count) are reinventing or rehashing ideas, and then the objections to them, almost verbatim from the other thread. Nicely illustrating, if anyone needed it, why that feature was deliberately left out of this RFC. |
@rfcbot resolved Cost/benefit and tail calls I've been persuaded that, on the whole, this RFC moves the language toward greater uniformity, and that in particular proper tall calls are not necessarily a better choice. Let's do it! |
All relevant subteam members have reviewed. No concerns remain. |
🔔 This RFC is entering its final comment period, with disposition to merge. 🔔 |
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 have some concerns and general style nits.
|
||
### Result type of loop | ||
|
||
Currently the result type of a 'loop' without 'break' is `!` (never returns), |
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.
s/'/`/
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 RFC is written in English, not code. shrug. 'loop' isn't a code snippet in this context, it's a quoted keyword.
### Result type of loop | ||
|
||
Currently the result type of a 'loop' without 'break' is `!` (never returns), | ||
which may be coerced to any type), and the result type of a 'loop' with 'break' |
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.
s/'/`/
I will not nit on any more of the formatting, but the issues with it probably should be fixed across the document. Namely, plain apostrophe (') does not begin/end an inline code snippet.
* if a loop is "broken" via `break;` or `break 'label;`, the loop's result type must be `()` | ||
* if a loop is "broken" via `break EXPR;` or `break 'label EXPR;`, `EXPR` must evaluate to type `T` | ||
* as a special case, if a loop is "broken" via `break EXPR;` or `break 'label EXPR;` where `EXPR` evaluates to type `!` (does not return), this does not place a constraint on the type of the loop | ||
* if external constaint on the loop's result type exist (e.g. `let x: S = loop { ... };`), then `T` must be coercible to this type |
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.
This list still has issues with handling of coercion. For example I imagine something like this ought to be valid:
loop {
// ...
break (expr: SomeT);
// ...
break (expr: OtherT);
}: RT
where SomeT
and OtherT
both coerce to RT
. If this discrepancy was fixed, the list could avoid having to contain the last two notes/requirements. I also dislike those two extra requirements-exceptions-rules, because they have potential to diverge from type rules elsewhere in the language.
(Why this matters? Substitute SomeT = &[u8; 42]
, OtherT = &[u8; 32]
, RT = &[u8]
.)
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 think the typing and coersions rules here can be exactly like those for match arms or if-else.
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.
Please write a diff against the RFC; I was not 100% sure how to handle coercions (same with the !
special case).
Refine rough edges wrt coercion
I just created a new issue to discuss other implementation details for returing from |
Just noticed this slightly confusion
It's obvious doing this with a closure would be more readable, but that's frequently true for Is it desirable to support a less confusing form of this?
If so, does |
@burdges That's the orthogonal (but synergistic) "break from any block" feature, I'm surprised I can't find any existing issue or RFC for it. cc @Ericson2314? |
@glaebhoerl I don't think anybody wrote an RFC but we've both brought it up many times (e.g. ? And catch). Maybe it will finally stick :). |
@burdges this would not be backwards compatible. If I had code like:
currently this would break out of the |
1. `break;` | ||
2. `break 'label;` | ||
3. `break EXPR;` | ||
4. `break 'label EXPR;` |
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.
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 say that the ;
is not necessary. It should map how return
works, and the following compiles:
fn test() -> bool {
if true {
return true
} else {
return false
}
}
fn test2() -> bool {
return false
}
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.
Yes just like today
---------------
(break 'a e): !
It's obviously "break from named blocks" @vitiral not literally "break from any block", so no compatibility issues. I donnot if it's a good idea or not, just thought it might be worth asking. |
It has been one week since all blocks to the FCP were resolved. |
RFC has been merged! Further discussion should happen on the tracking issue. |
This is the culmination of recent discussion in issue #961, minus discussion about extensions to for/while/while let (for which there is no clear solution).