-
Notifications
You must be signed in to change notification settings - Fork 56
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
Wrapping rules for let-chains? #169
Comments
What if the wrapping rules had more to do with the number of items in the chain? For example, something along the lines of "if there are two or more Let expressions in the chain than always wrap". That way each Let expression gets its own line and will be more readable. If there's just a single Let it's probably easy enough to figure out what's going on, and if we need to wrap the existing wrapping rules should cover us. So this: if let Some(ref first) = opt && let second = first {
} Would always become this: if let Some(ref first) = opt
&& let second = first
{
} But your earlier example, would remain unbroken: if a && let Some(b) = foo() {
} A similar rule could be introduced that states if there are 3 or more items in the chain and the Let expression isn't the first chain item then always break. So we'd break in both of the following cases: // Let at the end
if my_long_condition(foo, bar, baz)
&& some_more
&& let Some(x) = foo()
{
// lots of code...
use_x(x);
} // Let in the middle
if my_long_condition(foo, bar, baz)
&& let Some(x) = foo()
&& some_more
{
// lots of code...
use_x(x);
} But we wouldn't break here: if let Some(x) = foo() && my_long_condition(foo, bar, baz) && some_more {
} One might argue that it's still hard to understand where x is coming from in the above so if the rule were amended to be "If there are 3 or more items in the chain and there is at least 1 Let expression then always break", then we would break: if let Some(x) = foo()
&& my_long_condition(foo, bar, baz)
&& some_more
{
} I agree that these rules might be confusing to some users, but I think the improved readability is also important to consider. Let me know your thoughts! |
I think the horizontal position of if my_function(Some(get_thing()), variable, and_another) && let Some(x) = foo() { edit: I'll add this idea to the OP |
What about breaking after the // Let in the middle
if let Some(first) = my_long_condition(foo, bar, baz) &&
let Some(x) = foo() &&
some_more
{
// lots of code...
use_x(x);
} This would also improve (at least a bit) the following case if a &&
let Some(b) = foo() {
} |
This would be a diversion from the current style guide on Binary Operations, and I don't think changing that is an option at this point. Also that would not resolve the main concern here - the case where all the conditions fit on one line. |
Thanks for sharing your perspective @terhechte! @camsteffen is correct about the binops though, so that wouldn't be an available option anyway |
EDIT: Ignore this, it's already implied by option 2 together with other existing formatting rules. I'd like to propose two additional variations on option (2): 2a) Option 2, with an additional requirement: always break before and after every if condition()
&& let Some(binding) = expr
&& condition2(binding) && condition3()
&& let Some(binding2) = expr2
&& condition4(binding, binding2)
{
body();
} 2b) When a if condition()
&& let Some(binding) = expr
&& condition2(binding)
&& condition3()
&& let Some(binding2) = expr2
&& condition4(binding, binding2)
{
body();
} Of these two, (2a) would be more consistent with how we currently format boolean expressions, while (2b) seems preferable in general (and I'd propose doing that for all boolean expressions that require breaking if we ever have a rustfmt change over an edition, but that's unrelated to this issue). |
I thought we already break all binary operations if there are any line breaks? |
Ah, it would appear that we do, thank you. In that case, I think this reduces to option 2. |
While I understand the angle around visual scanning, I'm concerned that option 2 (always wrap) is a bit too blunt an instrument where something more surgical would be helpful. A few questions/things to note
if a
&& let Some(b) = foo()
{
// stuff
}
|
Good point! Fixed.
Yeah that may be true (it's unusual for |
Agreed! Not sure there's a good way to systematically inspect, but even getting feedback from individuals/teams would be helpful. I should also note that I'm absolutely open to incorporating a config option in rustfmt to give users some flexibility over this aspect of formatting, though obviously the default behavior remains an important question. I suppose there's also some variants on option 3, but which could incorporate factors like the total length of the control line and/or the number of elements, when let exprs are present (e.g. wrap the control line if the control line contains one or more let expressions unless the control line is not "small"). These types of rules do indeed have a higher cognitive load, but we do also have some fairly longstanding precedents e.g. various "small" measures, and chains to a degree as well |
If you guys want to customize the formatting of let chains, then option 3 is the most flexible and satisfies most scenarios/objectives. |
I used this feature in the compiler, and I'm in the camp It seems like the simplest variant, |
To be clear, feature stabilization is not blocked on formatting and I'm also adamantly opposed to doing that. It was briefly suggested for let-else statements, but was decided against after subsequent discussion and context. I've not seeing anything about let chain stabilization being blocked on formatting rules, but please feel free to loop me in on any conversations if that's being discussed. |
We need greater alignment between the formatting and language processes to provide more expeditious timing and a better user experience, but this is also why I feel so strongly that feature stabilization needs to remain orthogonal and not be gated on formatting. Formatting deserves thoughtful conversation and consideration, especially because whatever is decided will essentially be permanent. I absolutely do not want to be in a position where formatting considerations are rushed and forced through just because people want a language feature stabilized. That route will provide some short term gains in exchange for significant long term pain. It's wrong and we're not going to do that. I'd also push back on the characterization of these conversations as bikeshedding. The entire scope of this repository and process is explicitly to define specific formatting rules, so discussion of options of formatting rules is very much the focus here; not some unimportant detail of a larger topic. |
The more I think about this the more I feel that option 1 (do nothing) is the best approach for default behavior. I think it will be the most consistent with existing prescriptions, has the least cognitive burden and lower risk of formatting that's bad (or at least a poor use of space) in what I imagine will be the more common cases. I believe sticking with the status quo will work best for the majority of cases and that the scenarios discussed on this thread which appear to be the impetus for always wrapping seem to have viable workarounds. Some of those workarounds would be readily available to developers already (reordering the expressions, creating a local before the control flow expression to reduce the size of the binary expression that ends up in the control flow), etc. I'd also be open to some additional config options that could be used for non-default behavior to give additional control for the cases where they are needed (which could come in the form a single-line limit variants for binary expressions, as width based, expr count/nesting based, and/or the presence of certain expression kind variants). |
A concrete example where I think always breaking |
I agree with this point entirely. Because of the common length of a I also think a line break helps maintain a clear distinction between separate expressions and the RHS of a let. With |
This case would wrap regardless of the outcome of this issue IIUC. Wrapping of any binary expressions is forced because
Interesting point. I don't think any of options 1-4 would mitigate this problem since |
@camsteffen Per your comment #169 (comment) |
Correct me if I'm wrong, but strictly from an AST perspective let chains are simply a Binary expression that has at least one Let expression as an operand somewhere within that bin expression tree yes? Assuming that's accurate, I want to draw a distinction between the fact that rustfmt currently does nothing for Let expressions, regardless of whether or not they appear in a chain. This currently cascades back up and results in rustfmt not changing formatting for nodes higher in the tree, for example, rustfmt is currently unable to apply any formatting to the entire match expression in the below snippet. fn main() {
match foo {
a if let Some(b) = c => {}
}
} As such, I'm curious if folks would be willing to codify the rules for let expression variant in the Style Guide here so that we can at least handle cases like the one above while the conversation continues for chain handling? (yes, rustfmt can maintain current do-nothing behavior for chains while also handling formatting of a let expression in a guard) |
As for chains/let expr in a binary expr, do we perhaps need to revisit the rules for binary expressions more broadly? Do we need more aggressive and/or complex wrapping rules for those in general? I ask because I believe these expressions are currently only wrapped when approaching the max_width which can lead to some long running lines, and also because I wonder if some the theme behind the reasonings shared here for wrapping with a let expr could also reasonably/desirably be applicable for other expr variants too. I should note that question/hypothetical about broader binary expr rules would have to be associated with an edition change and is thus likely a separate/broader topic for discussion. However, we could certainly add an additional clause to those rules that require wrapping in the presence of let expr(s) at any point. Finally, I feel like we're a bit stuck on this. We've not had a wide enough volume of input nor even anything approaching a clear plurality IMO. Given that I wouldn't feel comfortable codifying any rules in my current role trying to maintain/triage the Style Guide. There's also not currently an authoritative body to make a call either. I'm not sure what would be our best path forward. @joshtriplett would you be comfortable making the call (to always wrap) as a former+future member of the Style Team, or if not too trivial, is it a call the Lang team (as the likely parent team of the Style team) would want to make? |
if let 1 = 1
&& true
{
} If you guys are going to force wrap, then please provide a stable Rustfmt option to disable it. Not everyone wants such formatting behaviour. |
FWIW @c410-f3r I agree with the perspective and it's part of why I've argued against the always wrap option in my preceding comments. However, it's important to keep in mind that these discussions and the goals of the style guide aren't to identify a formatting behavior that everyone wants/agrees with. I'd argue that doesn't actually exist, and it's why I'm not surprised that I've yet to find a single person who agrees with/prefers 100% of the Style Guide. I also previously mentioned an openness to config options on the rustfmt side. Understand your desire to have an escape hatch if the proposed option were selected (and to have that available on stable), but would suggest we keep discussion of rustfmt specifics and stabilization processes separate |
I'm not comfortable making that call unilaterally. I would be happy to poll the Lang team for an interim decision to unblock things, while we're working on getting the style team started up. |
FWIW I think let chains and let-else are going to be contentious formatting-wise no matter what choice is taken. This is basically guaranteed to be a "nobody's favorite" kind of choice. overly cute variantIn the code I've written with let chains, I've always written it such that the let Some(x) = x &&
let Some(y) = y
else {
// diverge
}
if let Some(x) = x
&& let Some(y) = y {
// use
} However, I freely admit that this is too cute, doesn't generalize well to e.g. Ultimately I think the most reasonable default is "do nothing special" (option 1). This falls squarely into being predictable and nonoffensive. This is definitely a place where rustfmt would benefit from having an "intent preserving" mode/ability, though, as the presence of a line break is perhaps the best indicator that a line break would be useful. Footnotes
|
Not that this will help solve the more immediate decision at hand, but, for broader awareness this is something we've been working on in rustfmt in different contexts with some measure of success, albeit not-yet-released (chains and function calls specifically). No promises, but I will keep this theme in mind for binary expressions as we look towards future work planning for rustfmt.
This made me laugh out loud, both because I think it's 100% correct but also because I'm not sure if/how often we actually see a different kind of choice in these contexts 😁 (edit: typos) |
At the very least, I 101% agree with a good number of the choices made by rustfmt that were contentious (e.g. block indentation only, no visually aligned indentation) so at least some choices are "someone's favorite" 🙂 |
I keep coming back to solution number 3 (wrap |
A possible fifth possibility that came up in some discussions, that I'd like to capture here:
This would avoid all four of the concerns listed in the top post of this: it avoids making the let bindings hard to find, it avoids absurd wrapping in the We could, similarly, use some other metric of "simple expression" in place of |
Semantic note: I use the phrase "wrap the expression" to mean "insert a line break before the expression" - or more specifically - "...before the binary operator preceding the expression". Another assumption to note: "always wrap Correct me if I'm wrong, but I believe option 5 could be re-worded as: "Always wrap
I think this could be added onto any of the other options. We can consider adding a "wrap after" rule orthogonally to our consideration of adding a "wrap before" rule. (Note the rationale for "always wrap after" was given in #169 (comment)) |
Minor point of pedantry, but would encourage striving for clarity when detailing suggested formatting rules to ensure sufficient delineation between the let expression itself vs. a binary expression that contains a let expression (a chain). My interpretation of prior comments is that most/all are likely referring to the latter, but also want to note that the style guide will need prescriptions for both. |
I just wanted to relay my experience: I've been using let chains (and let else) a lot in rustc, and I've found that
I'm also intrigued if we there's any mechanism for rustfmt to format nightly features up and only up the moment they are stabilized. If at the time stabilization happens a style decision hasn't been arrived at, then stop formatting in some way. This is purely for the benefit of rustc and similar codebases, so it is purely a self-serving request. |
@estebank We talked about this in today's @rust-lang/style meeting. We agreed that with the help of syntax highlighting or IDE support it'd be easier to find a
The Rust style already has the rule that once you've broken the condition across lines, the If an |
I think the only qualms I have with the potentially settled decision is the need for splitting let chains always, but in practice chains in the compiler are long enough that there will be little difference between my preferred solution (follow the current |
It seems that rust-lang/rust#110568 and rust-lang/rustfmt#5910 are trying to push this feature forward with a "one-ident-one-pattern-match" single line format, aka rule number 5, for whatever the reason (I couldn't find a discussion about it in a Zulip channel). If that is really the case and as previously commented, please consider providing a stable configuration to disable such force-wrap behaviour in order to allow wrapping according to the specified #![feature(let_chains)]
fn _let_chains() {
let _foo = true;
let _bar: Option<i32> = Some(1);
let _baz = true;
if _foo && let Some(elem) = _bar && _baz {
dbg!(elem);
}
}
fn _regular() {
let _foo = true;
let _bar: Option<i32> = Some(1);
let _baz = true;
if _foo && _bar.is_some() && _baz {
dbg!(_bar);
}
}
fn main() {
_let_chains();
_regular();
} |
@c410-f3r - please note this is not the correct place to continue repeating requests for rustfmt features, nor to discuss the stabilization of rustfmt features which do not even exist yet. I've already noted the possibility of rustfmt adding a configuration a few times, most recently in #169 (comment). However, neither the Style Team nor the Style Guide's default prescriptions cover rustfmt features, nor rustfmt's stabilization process. Finally, neither of the PRs you've mentioned are setting anything in stone. Let chains are still nightly only syntax, and the purpose of moving ahead is based on our procedure for nightly only syntax so that we can get something in place experimentally while still retaining the ability to evolve the formatting for nightly only syntax. This thread would be a good place for users to weigh in (particularly after the proposed formatting is provided) and share specific, concrete feedback and input on the formatting and/or why/how they think an alternative would be better. |
@RalfJung suggested this as an example of where the currently-proposed rules may not be optimal: if let Ok(name) = str::from_utf8(name) && is_dyn_sym(name) {
let ptr = this.fn_ptr(FnVal::Other(DynSym::from_str(name)));
this.write_pointer(ptr, dest)?;
} else {
this.write_null(dest)?;
} Under the current proposed rules, this would be formatted as: if let Ok(name) = str::from_utf8(name)
&& is_dyn_sym(name)
{
let ptr = this.fn_ptr(FnVal::Other(DynSym::from_str(name)));
this.write_pointer(ptr, dest)?;
} else {
this.write_null(dest)?;
} He (and others) have suggested that the first formatting may be better. One problem with adding so many line breaks as the second version does is that it makes a match str::from_utf8(name) {
Ok(name) if is_dyn_sym(name) => {
let ptr = this.fn_ptr(FnVal::Other(DynSym::from_str(name)));
this.write_pointer(ptr, dest)?;
}
_ => this.write_null(dest)?,
} From discussion, it seems that the issues that led to the tentative decision to format it in the second way are:
(Thanks to @compiler-errors for these.) Given that, here are two concrete proposals for how we could make this better:
There is of course also this option:
We could consider option 3 if it turns out that the costs of breaking these lines (e.g. in discouraging use of the feature) turn out to be higher than the potential for confusion over the precedence of Setting aside option 3 for the moment, the question for discussion on options 1 and 2 is whether they have any notable drawbacks and whether there are other similar carve-out rules we should consider adopting. |
The potential for precedence confusion remains in that example: between |
FWIW while I did assume that " |
In the spirit of rust-lang/rust#117161, we could also choose to handle potentially confusing uses by linting rather than by our choice of formatting. |
Feature tracking issue: rust-lang/rust#53667
Should we introduce a new guideline for wrapping let-chains?
The rationale is that let-chains allow
let
bindings to be anywhere in a line of code.let
is no longer consitently at the left side. It is good forlet
bindings to stand out and be easy to discover since they introduce new variables for the following scope. Oftentimes when reading code, I look backwards with the question "where did this variable come from?"Possible answers
No change. Wrap
let
expressions the same as any other expression.Always wrap
let
expressions.let
is only ever preceded by one token in the same line:if
,while
,&&
,||
, etc.Wrap
let
expressions if they begin N characters or more past the current indentation.For example, a
let
-expression may be on the same line if it is 15 characters from the left margin. If it is 16 characters from the left margin, it must break to the next line.Wrap
let
expressions if there are 2 (?) or morelet
expressions in the conditionAllow
single_identifier && let
on one line (if it fits line length limits), but require a line break before the&&
for any other expression. Always wrap after a let expression.Potential concerns for each answer
x
may be hard to find in this case:let
leftward by one character:Other considerations
The rules for
if
andwhile
will be the same.If a
let
expression wraps, it will cause all of the&&
or||
-separated expressions to be on separate lines.If there is just one
let
binding at the beginning, the condition may be on one line:The text was updated successfully, but these errors were encountered: