Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upRFC: if- and while-let-chains #2260
Conversation
Centril
added
the
T-lang
label
Dec 24, 2017
This comment has been minimized.
This comment has been minimized.
RFC #2046 does help you with rightward drift: 'label :{
let expr = if let ExprAddrOf(_, ref expr) = expr.node {
expr
} else {
break 'label None;
};
let expr = if let ExprArray(ref exprs) = expr.node {
exprs
} else {
break 'label None;
};
let expr = if let Some(expr) = exprs.last() {
expr
} else {
break 'label None;
};
// ...
Some((lit.as_str(), exprs.len()))
}Now if you add one simple macro, 'label : {
let expr = unwrap_or_none!(ExprAddrOf(_, ref expr) = expr.node, 'label);
let exprs = unwrap_or_none!(ExprArray(ref exprs) = expr.node, 'label);
let expr = unwrap_or_none!(Some(expr) = exprs.last(), 'label);
let lit = unwrap_or_none!(ExprLit(ref lit) = expr.node, 'label);
let lit = unwrap_or_none!(LitKind::Str(ref lit, _) = lit.node, 'label);
Some((lit.as_str(), exprs.len()))
}This allows you to do non-trivial code between the let declarations, something that is very inconvenient with this proposal, so it fixes the ergonomics issues much better than this RFC. |
This comment has been minimized.
This comment has been minimized.
|
With this, we don't need
The |
This comment has been minimized.
This comment has been minimized.
|
Another alternative to this RFC: if guards on if's: fn get_argument_fmtstr_parts(expr: &Expr) -> Option<(InternedString, usize)> {
if let ExprAddrOf(_, ref expr) = expr.node // &["…", "…", …]
if let ExprArray(ref exprs) = expr.node
if let Some(expr) = exprs.last()
if let ExprLit(ref lit) = expr.node
if let LitKind::Str(ref lit, _) = lit.node {
Some((lit.as_str(), exprs.len()))
} else {
None
}
}This would be more consistent to what match is doing, and I think there are no parser ambiguities. |
This comment has been minimized.
This comment has been minimized.
Wyverald
commented
Dec 24, 2017
|
I find this syntax very confusing. At a glance this looks very much like parallel I still do think the best way to deal with this kind of right-ward drift is with early returns, with something like |
This comment has been minimized.
This comment has been minimized.
|
I agree with @Wyverald that the , syntax is confusing. I wondered at first too whether it was "or" mode or "and" mode (it's "and" mode). I think my suggestion for |
This comment has been minimized.
This comment has been minimized.
|
@Havvy You certainely could write @Wyverald We did not consider that this could be confused with parallel assignment at all. If that is an inference that many make, we need to change the syntax of course. @est31 We didn't consider this, and that is true. But the first non-macro snippet you listed is not very ergonomic or readable and has a lot of repetition. The macro does reduce this, but repeats I did briefly consider: (and yes, there should be no parser ambiguities) fn get_argument_fmtstr_parts(expr: &Expr) -> Option<(InternedString, usize)> {
if let ExprAddrOf(_, ref expr) = expr.node // &["…", "…", …]
if let ExprArray(ref exprs) = expr.node
if let Some(expr) = exprs.last()
if let ExprLit(ref lit) = expr.node
if let LitKind::Str(ref lit, _) = lit.node {
Some((lit.as_str(), exprs.len()))
} else {
None
}
}but went on since it is slightly more verbose. However, it aligns pretty well and is a syntax I could live with. I will certainly add it as an alternative if it is not changed to be the main proposal instead. |
This comment has been minimized.
This comment has been minimized.
|
@est31 If we go with your proposed syntax however, we need to consider what that means for |
This comment has been minimized.
This comment has been minimized.
Yeah, that's why I added a macro snippet. The first snippet was only to reduce rightward drift.
You can probably minimize this further by creating a macro inside the
I don't have any views/thoughts on the precedence. |
This comment has been minimized.
This comment has been minimized.
Right. I will update the RFC with a more nuanced description in a while (holidays, so, yeah... ^,^)
Sounds about right. However, the macro is specific to returning
We'll have to resolve this together tho I think unless we're OK with shutting the door to |
This comment has been minimized.
This comment has been minimized.
|
This is a very well written RFC. Thanks for writing it. Well written RFCs really improve this process and are are so very helpful for all involved. For reference, this topic was previously discussed in #929. |
This comment has been minimized.
This comment has been minimized.
|
Short review (I'll probably elaborate later):
Syntax is the most important aspect in this particular case - the whole point of the change is resyntax nested conditions and conditions with weird inverted order into something nicer looking. |
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov ohhh, this
|
This comment has been minimized.
This comment has been minimized.
I am sure I have written that way on many occasions.
I think the reasons we state in the RFC for rejecting this are valid.
I am not a fan of this syntax. It is inconsistent with the order already used in I also think that patterns are more often than not shorter than what they pattern match on, so reversed order would be particularly bad for alignment. The keyword I'm assuming that
My bad. I could have been clearer - what I meant really is: the most important bit is reducing rightward drift and improving ergonomics - the exact syntax can be changed ;) Updated above. |
This comment has been minimized.
This comment has been minimized.
|
@matklad That's a very nice and interesting solution indeed =) With slight change: let name = meta.name();
when {
set.is_some() =>
error::set_again(name),
let &NameValue(_, Lit::Int(val, ty)) = meta,
val <= u32::MAX as u64,
is_unsigned(ty) =>
*set = Some(val as u32),
else =>
error::weight_malformed(name),
}I see a few problems with your and my version of your syntax - hopefully you can address them? :
|
This comment has been minimized.
This comment has been minimized.
|
For comparison: if expr.node is ExprAddrOf(_, ref expr)
&& expr.node is ExprArray(ref exprs)
&& exprs.last() is Some(expr)
&& expr.node is ExprLit(ref lit)
&& lit.node is LitKind::Str(ref lit, _) {
Some((lit.as_str(), exprs.len()))
} else {
None
} if let ExprAddrOf(_, ref expr) = expr.node
, let ExprArray(ref exprs) = expr.node
, let Some(expr) = exprs.last()
, let ExprLit(ref lit) = expr.node
, let LitKind::Str(ref lit, _) = lit.node {
Some((lit.as_str(), exprs.len()))
} else {
None
} |
This comment has been minimized.
This comment has been minimized.
|
@Centril I don't really want to derail this RFC thread into discussing That said
val x = if (cond)
bar
else
baz
vs
val x = when {
cond -> bar
else -> baz
} |
This comment has been minimized.
This comment has been minimized.
Yeah, I'd probably write this too with less/greater and some other operators, but pattern matching is
It is very much desirable for non-exhaustive pattern matching construction to be an expression, so it's not confusion, it's correct understanding. (Also, the
It's the order in
Can't say much about alignment. rustfmt have standard rules for
Let's look at these function for example https://github.com/rust-lang/rust/blob/master/src/librustc/ty/sty.rs#L1263-L1518
Yes, |
This comment has been minimized.
This comment has been minimized.
You are not derailing, it was an interesting proposal. There's having one way, which I am not a proponent of, and there's having 10 ways, which I am not a proponent of either, and somewhere you have to strike a balance... ;)
No matter, it is what we got... so don't we have to be consistent with the inconsistency of
match expr_on {
pat_1 => expr_1,
...
}Here,
Sounds reasonable. You've convinced me on this point =) However, |
This comment has been minimized.
This comment has been minimized.
|
You've all made great points. I don't feel very strongly about the particular syntax proposed in the RFC, so at this point I am certainly willing to rework this RFC, with some help, into either |
This comment has been minimized.
This comment has been minimized.
|
Fwiw, as written this rfc seems to follow the ideas in #929 fairly closely and while 'is' was mentioned in that issue, it was relatively recently added in the thread and easy to miss. As such, it isn't surprising it wasn't included already in the rfc as a variant or option. A positive note of both these is they both appear to make the language consistent and more general which is a huge improvement over the current situation in which 'if let' is considered an eyesore. Having said that, the greater potential benefit of the 'is' is pretty cool. Something I didn't see in this rfc is the inclusion of 'where' as seen in the following example taken from #929 (comment) : if let a = foo(), let b = a.bar(), let c = b.baz() where c > 2, let d = c.quux() {
// all operations succeeded
} else {
// something failed
} |
This comment has been minimized.
This comment has been minimized.
|
@mdinger I think we just forgot about it when writing about alternatives because there are so many potential syntactic variants, you have to stop somewhere ;) With respect to |
This comment has been minimized.
This comment has been minimized.
|
I'm in favor of chaining And sure, you could say that an The whole |
This comment has been minimized.
This comment has been minimized.
|
My problem with if opt_x is Some(x) && x > 10 {
println!("{}", x);
}there are just so many tiny changes that make it no longer work: if opt_x is Some(x) || x > 10 {
if (b == opt_x is Some(x)) && x > 10 {
if foo(opt_x is Some(x)) && x > 10 {I'm scared just thinking of how to write a thorough but non-surprising rule here. Aside: I prefer I think I'm personally coming around to @est31's "if guards on ifs". I like that it would extend to |
This comment has been minimized.
This comment has been minimized.
It's interesting to compare it with Kotlin, which also uses The differences is that instead of destructing, Kotlin's However, as long as the compiler does all the inference work for you, actually using this feature is easy: you don't have to replay the analysis in your head when reading or writing code, because the compiler catches all errors. |
This comment has been minimized.
This comment has been minimized.
|
I think |
This comment has been minimized.
This comment has been minimized.
I disagree that this behavior should be called weird, this is essentially control-flow sensitive typechecking, which is more or less mainstream today, and can be found in C#, Kotlin and TypeScript. |
This comment has been minimized.
This comment has been minimized.
|
@matklad Do you have some relevant papers I could read on this topic perhaps? Might be useful when rewriting the RFC if we decide to go in this direction. PS: You have some private messages on IRC ;) |
This comment has been minimized.
This comment has been minimized.
|
As far as I can see, C#, Kotlin and TypeScript (and other languages I've heard of) all have only flow-sensitive type "refinement", rather than type-dependent bindings. The typing judgements may look similar or identical for both, but the implementation in a compiler will be different (name resolution is affected, not just type checking) and the UX may be rather different as well. It certainly feels different to me, and I'm perfectly comfortable with the idea of flow-sensitive typing. |
This comment has been minimized.
This comment has been minimized.
stepancheg
commented
Jan 3, 2018
|
@petrochenkov @matklad for the record, same parser ambuity issue issue exists without
or this:
I still think |
This comment has been minimized.
This comment has been minimized.
I60R
commented
Jan 5, 2018
|
@petrochenkov At least it solves:
Yes, it don't returns |
This comment has been minimized.
This comment has been minimized.
|
The survey ran from 2017-12-31 06:25 to ~2018-01-06 ~14:00. 373 people took the survey. It was distributed to:
Breakdown of preferences (ordered by the answer and not how much it was liked/disliked):
Note: "none of them" being disliked means that the person dislikes none of the 6 preceding alternatives. To decrease the risk of bias, the order of the answers were randomized. A summary of the survey: https://docs.google.com/forms/d/e/1FAIpQLScwG0Y3ynA9aJZ-iprOey_GyCNeFMO9MSDJR1kiskpjsjL1Mw/viewanalytics A CVS file of the survey: https://drive.google.com/file/d/1awyvryblSHFH9J77TPutW5BrRlKr0EKZ/view?usp=sharing A PDF for the survey: https://drive.google.com/file/d/14ofF5on_Z_XLvhPr1I4dVgCfcybQO2GY/view?usp=sharing |
This comment has been minimized.
This comment has been minimized.
|
@Centril thanks for running the survey! While having a general "what looks best" poll is great, I think the most valuable part of it were the comments. They contain a bunch of very interesting arguments and most of them were actually on-topic unlike most of the comments inside this thread :p. |
This comment has been minimized.
This comment has been minimized.
|
Indeed the comments are very valuable and they show that the choices people have made are not made on a whim but that the answers are well reasoned. Thanks a lot to @panicbit for helping out with making the survey. |
This comment has been minimized.
This comment has been minimized.
|
@Centril I hope you won't just blindly take the results of this poll and go with whatever was most popular? This was never what the poll was about. |
This comment has been minimized.
This comment has been minimized.
|
Certainly not blindly. I would put it this way: Other proposals now have a somewhat increased burden of proof to show why the reasons put forth by the majority, such as "this is more consistent with current Rust" are less important than the reasons put forth by the minority positions. The reasons for picking the majority position as well as not picking the minority ones seem well argued by the respondents. |
This comment has been minimized.
This comment has been minimized.
|
The poll was made in a way where no sum up of the arguments and the downsides and upsides and hidden gotchas of the various proposals was provided. This means that the people filling out the questions weren't able to make an informed descision unless they thought about those themselves, and I'm sure that most just answered basing on their gut feeling. Even if you provide the information, many people might not read it, so basing decisions on polls is no good solution. The proposal provided very interesting arguments like e.g. saying that Also, note that the least disliked option is to do nothing. Either way, there hasn't been a single involvement by the lang team on this. What are their opinions on the matter? Which option is best for them? |
This comment has been minimized.
This comment has been minimized.
|
I've read at least 300 of the answers, and we can cherry pick answers, but my feeling is that a lot of them were informed and good comments.
That's a misinterpretation of the meaning of disliking none of them. Disliking none of them means that you can live with any of the 6 syntaxes proposed. At least that is what the question/answer intended. |
This comment has been minimized.
This comment has been minimized.
|
Reading through the summary, this comment (I didn't look from whom) stood out to me:
I think my wish right now would be to |
This comment has been minimized.
This comment has been minimized.
|
As this RFC is written today, it is wholly insufficient to cover alternatives and discuss the pros and cons. The survey has also highlighted may arguments for and against each alternative. The guide-level-explanation might also be changed to use a different syntax. Furthermore, this RFC has had Given all this, I think the best course of action is to close the RFC. I will be doing so in a day unless there are major objections from folks. |
This comment has been minimized.
This comment has been minimized.
|
I'm preparing a prototype for in-expression bindings right now, btw. Many (EDIT: but not all!) comments against |
This comment has been minimized.
This comment has been minimized.
|
@petrochenkov The keyword
My only nit is that |
This comment has been minimized.
This comment has been minimized.
rpjohnst
commented
Jan 6, 2018
•
|
I do prefer I also agree with @scottmcm that |
This comment has been minimized.
This comment has been minimized.
This is just bad wording on my side, I didn't mean it's a summary for all reactions, just the most popular concern. |
This comment has been minimized.
This comment has been minimized.
|
Closing as promised. |
Centril
closed this
Jan 8, 2018
petrochenkov
referenced this pull request
Jan 10, 2018
Merged
Assignment to immutable argument: diagnostic tweak #47261
This comment has been minimized.
This comment has been minimized.
|
So, I finally completed my prototype of non-exhaustive pattern-matching expressions, including in-expression bindings. Some notes:
let x;
match Some(10) {
Some(..) | None => {
x = 11;
}
_ => {} // Unreachable
}
let y = x; // Currently ERROR: use of possibly uninitialized variable: `x`
|
Centril
referenced this pull request
Feb 23, 2018
Closed
Macroless mapping of patterns -> bool #2153
petrochenkov
referenced this pull request
Feb 25, 2018
Merged
Allow `if` and `match` in constants #2342
This comment has been minimized.
This comment has been minimized.
I60R
commented
Jul 3, 2018
•
|
My suggestion about
|
Centril commentedDec 24, 2017
•
edited
Rendered
This RFC was co-authored with @scottmcm ;)
Here's a survey on some choices of syntax.
Please answer it if you have the time.
And please don't spread the survey around.
https://goo.gl/forms/Wx61sx9s03N5SGA52
Before considering this RFC, please remember what is most important: reducing rightward drift and improving ergonomics in control flow. The exact syntax can always be changed into a direction you prefer. If you don't like the syntax, please write a comment about that below or agree with someone who has put forth your view. Thank you :)
EDIT: The exact syntax regarding
&&or,or chainingifis very much in flux right now.This RFC extends
if let- andwhile let-expressions with chaining, allowingyou to combining multiple
lets and conditions together naturally. With thisRFC implemented, you will, among other things, now be able to write: