Open camsteffen opened 2 years ago
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!
What if the wrapping rules had more to do with the number of items in the chain?
I think the horizontal position of let
is the important thing, not the number of let
s. I wouldn't gamble that the following case is rare:
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 &&
so that the lets line up nicely?
// 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() {
}
What about breaking after the
&&
so that the lets line up nicely?
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 let
expression, such that let pat = expr
is only ever preceded by one token in the same line (if
, while
, &&
) and a boolean expression never appears after it:
if condition()
&& let Some(binding) = expr
&& condition2(binding) && condition3()
&& let Some(binding2) = expr2
&& condition4(binding, binding2)
{
body();
}
2b) When a let
expression is present anywhere in a condition, always break each top-level expression (whether let
or boolean) onto its own line:
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 the control line is broken for any reason, then the opening brace should be on its own line and not indented.
clause for control flow expressions. It would instead have to be formatted as below. Obviously subjective, but this makes me grimace:
if a
&& let Some(b) = foo()
{
// stuff
}
The formatting of the potential concern for option 2 isn't correct
Good point! Fixed.
it does require somewhat of a perfect storm
Yeah that may be true (it's unusual for let
to actually end up very far to the right). But in a way that may be a bad thing. Your brain might get used to seeing let
on the left side cause that's how it usually works out, but in the unusual case that it isn't, you might miss it. It would be interesting see statistics on that. In real code using let-chains, what is the distribution of the horizontal position of let
?
In real code using let-chains, what is the distribution of the horizontal position of let?
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 1
I think, i.e. if a && b && c { ... }
should be formatted identically regardless of whether a/b/c
have any let
s in them or not.
It seems like the simplest variant, let
in the middle of a line looks fine in practice, and I'm more interested in stabilizing this sooner than in continuing the bikeshedding.
Feature stabilization in the language being blocked on formatting decisions seems like an unfortunate situation to me.
Feature stabilization in the language being blocked on formatting decisions seems like an unfortunate situation to me.
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.
I'm more interested in stabilizing this sooner than in continuing the bikeshedding.
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 && let
to the start of a new line helps make bindings easier to notice: https://github.com/rust-lang/rust/pull/95631/
Yeah that may be true (it's unusual for
let
to actually end up very far to the right). But in a way that may be a bad thing. Your brain might get used to seeinglet
on the left side cause that's how it usually works out, but in the unusual case that it isn't, you might miss it.
I agree with this point entirely. Because of the common length of a let
binding, let expressions will commonly wrap to the start of a line anyway, so when they don't, they'll be easy to overlook.
I also think a line break helps maintain a clear distinction between separate expressions and the RHS of a let. With let pat = expr && anotherexpr
, even if you know that expr
doesn't have type bool
, it's easy for your mental parser to group expr && anotherexpr
together. Breaking to a new line makes it easier to do quick mental parsing and skimming.
A concrete example where I think always breaking
&& let
to the start of a new line helps make bindings easier to notice: rust-lang/rust#95631
This case would wrap regardless of the outcome of this issue IIUC. Wrapping of any binary expressions is forced because let
through the end of the condition does not fit on the same line.
With
let pat = expr && anotherexpr
, even if you know thatexpr
doesn't have typebool
, it's easy for your mental parser to groupexpr && anotherexpr
together. Breaking to a new line makes it easier to do quick mental parsing and skimming.
Interesting point. I don't think any of options 1-4 would mitigate this problem since let
is at the beginning. My intention with option 2 was that it does not necessarily wrap after let
expressions. So perhaps option 5 is "always wrap chains containing a let
expression". Wrapping after the let
expression is kind of a separate question.
@camsteffen Per your comment https://github.com/rust-dev-tools/fmt-rfcs/issues/169#issuecomment-1072370574 We already require wrapping the entire condition if it doesn't fit on one line. So the only time this would come up is with a short condition containing a single let followed by one or more boolean conditions that all fit on one line. I personally think in that case we should still force wrapping.
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 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?
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.
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.
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
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 is basically guaranteed to be a "nobody's favorite" kind of choice.
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 :grin:
(edit: typos)
This is basically guaranteed to be a "nobody's favorite" kind of choice.
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 :grin:
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 let
if it is N+ characters from the margin). I just think that will produce the most consistently agreeable results. The other solutions have warts as outlined in OP. I think the "complexity" downside of 3 is negligible - I generally don't give any thought to what is the logic behind what the formatter does. I would like to have the assurance that let
doesn't drift too far from the margin.
A possible fifth possibility that came up in some discussions, that I'd like to capture here:
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.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 if a && let
, it doesn't add complexity (people can easily and intuitively do it by hand), and it avoids having complex expressions to the left of a let
.
We could, similarly, use some other metric of "simple expression" in place of single_identifier
. I like "no more than a single identifier" though, to still make bindings easy to find when scanning code.
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 let
expressions" probably assumes "...unless it is the start of a condition expression". That is, I assume we never want to have ...something = if\n let...
.
Correct me if I'm wrong, but I believe option 5 could be re-worded as: "Always wrap let
expressions. But they can be on the same line if it is only preceded by condition consisting of a single identifier." So it is adding an exception and then adding an exception to the exception, and is actually more complex than option 3 IMO.
Always wrap after a let expression.
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 https://github.com/rust-lang/fmt-rfcs/issues/169#issuecomment-1087054004)
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
hir::ExprKind::*
might as well be a bullseye emoji for me now, I can identify it written on the side of a car driving down a highway while I'm skydiving)let
, making them obvious as desired by some here{
on its own line, at the same indentation level as the if
to be able to identify the body of the expression at a glance (aligning it to the condition or leaving it in the last let is quite hard for me to parse)if
expressions and bodies just because the condition had let chains, I'd like to know if there's anything I can help with to change thisI'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 let
in the middle of the line, but it's a general guiding principle of the Rust style that Rust should be readable even in non-syntax-aware contexts such as diffs. Given that, I think it's worth having a line break to aid with finding the let
.
I personally prefer to have the { on its own line, at the same indentation level as the if to be able to identify the body of the expression at a glance (aligning it to the condition or leaving it in the last let is quite hard for me to parse)
The Rust style already has the rule that once you've broken the condition across lines, the {
goes on a line by itself. I'm hoping that'll capture the property you're going for here.
If an if let
ends up all on one line, {
will end up at the end of that line if it fits, and I don't personally think we should change that unless we were to change it systematically for all if
conditions, which I don't think we should do.
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 if
condition formatting) and the agreed upon one (more than 1 let
expression makes the conditions go on their own line). I don't think that my ability to parse let
embedded in conditions is purely due to syntax highlighting, and I haven't encountered a case where missing them would have caused problems, but I understand it is not everyone's position. I'm really glad to find out there's forward movement on this.
It seems that https://github.com/rust-lang/rust/pull/110568 and https://github.com/rust-lang/rustfmt/pull/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 max_width
.
#![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 https://github.com/rust-lang/style-team/issues/169#issuecomment-1145320589.
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
shorter (and arguably prettier), so such formatting may discourage the use of let-chains
. E.g.:
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:
- Multiple
let PAT = EXPR && let PAT2 = EXPR2
being confusing to read because the second let is not left-aligned.- Trailing expressions being confused because of the precedence of the
&&
(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:
let PAT = EXPR
fragments).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 &&
. It's perhaps worth discussing at least.
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 if (let pat = expr) && test
versus if let pat = (expr && test)
.
FWIW while I did assume that "let
should be at the left" was the reason for the ewlines, the precedence concern did not occur to me. When would one want to write if let pat = (expr && test)
? The chances that that is what I meant seem basically 0, so I don't think we should add 2 newlines to defend against this extremely unlikely interpretation.
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
let
expressions the same as any other expression.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.let
expressions if there are 2 (?) or morelet
expressions in the conditionsingle_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: