Skip to content
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

Open
camsteffen opened this issue Feb 1, 2022 · 41 comments
Open

Wrapping rules for let-chains? #169

camsteffen opened this issue Feb 1, 2022 · 41 comments

Comments

@camsteffen
Copy link

camsteffen commented Feb 1, 2022

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 for let 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

  1. No change. Wrap let expressions the same as any other expression.

  2. Always wrap let expressions. let is only ever preceded by one token in the same line: if, while, &&, ||, etc.

  3. 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.

  4. Wrap let expressions if there are 2 (?) or more let expressions in the condition

  5. Allow 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

  1. The introduction of x may be hard to find in this case:
    if my_long_condition(foo, bar, baz) && some_more && let Some(x) = foo() {
        // lots of code...
        use_x(x);
  2. Wrapping for this case seems a little unnecessary since it only shifts let leftward by one character:
    if a
        && let Some(b) = foo()
    {
  3. Added complexity to formatting logic. Difficult to follow/verify the guideline without automated tooling. The subtlety of the logic may be surprising to users.
  4. This case is suboptimal (similar to 1):
    if my_function(Some(get_thing()), variable, and_another) && let Some(x) = foo() {

Other considerations

The rules for if and while will be the same.

If a let expression wraps, it will cause all of the && or ||-separated expressions to be on separate lines.

if something
    && let Some(x) = foo() && something_else { // bad - `&& something_else` should be on its own line

If there is just one let binding at the beginning, the condition may be on one line:

if let Some(foo) = do_something() && another_condition {
@ytmimi
Copy link

ytmimi commented Feb 2, 2022

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!

@camsteffen
Copy link
Author

camsteffen commented Feb 2, 2022

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 lets. 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

@terhechte
Copy link

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() {
}

@camsteffen
Copy link
Author

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.

@calebcartwright
Copy link
Member

Thanks for sharing your perspective @terhechte! @camsteffen is correct about the binops though, so that wouldn't be an available option anyway

@joshtriplett
Copy link
Member

joshtriplett commented Mar 17, 2022

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).

@camsteffen
Copy link
Author

I thought we already break all binary operations if there are any line breaks?

@joshtriplett
Copy link
Member

Ah, it would appear that we do, thank you.

In that case, I think this reduces to option 2.

@calebcartwright
Copy link
Member

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

  • The formatting of the potential concern for option 2 isn't correct because it would fail to comply with the 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
}
  • I understand the scenario that option 2 is trying to guard against (vertical scanning up through the block and then horizontal scanning to the end of the control line), however, do we really think that type of scenario will be more common than the simpler type shown above?
  • The existing rules in relevant contexts where these will typically appear (e.g. guards, control flows) include their own wrapping rules. So while the scenario option 2 protects against can, and certainly will, occur, it does require somewhat of a perfect storm wherein it's long enough to start being a scanning challenge, but too short to trigger those other wrapping rules (and obviously every expression in the control line would need to be single-line formattable too)

@camsteffen
Copy link
Author

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?

@calebcartwright
Copy link
Member

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

@c410-f3r
Copy link

If you guys want to customize the formatting of let chains, then option 3 is the most flexible and satisfies most scenarios/objectives.

@petrochenkov
Copy link

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 lets 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.

@calebcartwright
Copy link
Member

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.

@calebcartwright
Copy link
Member

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.

@calebcartwright
Copy link
Member

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).

@joshtriplett
Copy link
Member

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

@joshtriplett
Copy link
Member

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.

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.

@camsteffen
Copy link
Author

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 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.

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.

@joshtriplett
Copy link
Member

@camsteffen Per your comment #169 (comment)
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.

@calebcartwright
Copy link
Member

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)

@calebcartwright
Copy link
Member

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?

@c410-f3r
Copy link

c410-f3r commented Jun 2, 2022

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.

@calebcartwright
Copy link
Member

calebcartwright commented Jun 2, 2022

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

@joshtriplett
Copy link
Member

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.

@CAD97
Copy link

CAD97 commented Jul 8, 2022

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 variant

In the code I've written with let chains, I've always written it such that the lets all appear visually aligned — but very interestingly, without any actual visual alignment indentation, or relying on tab width! Specifically, I've writ

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. else if or match guards, and has some objective problems beyond that1.


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

  1. the big one is that the let-else case requires seeing the && at the end of the line to know the two lets are both attached to the else, as opposed to only the second; indentation is used to indicate expression continuation everywhere elsewhere in the rustfmt style

@calebcartwright
Copy link
Member

calebcartwright commented Jul 8, 2022

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 😁

(edit: typos)

@CAD97
Copy link

CAD97 commented Jul 8, 2022

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 😁

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" 🙂

@camsteffen
Copy link
Author

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.

@joshtriplett
Copy link
Member

joshtriplett commented Nov 30, 2022

A possible fifth possibility that came up in some discussions, that I'd like to capture here:

  1. Allow 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.

@camsteffen
Copy link
Author

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 #169 (comment))

@calebcartwright
Copy link
Member

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.

@estebank
Copy link

estebank commented Jan 18, 2023

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 lean towards the same formatting rules as regular if conditions (I believe this is 1? same as petrochenkov above)
  • let in the middle of short conditions have been easy to find for me, due to syntax highlighting and repeating text in the path of the types involved (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 chains tend to always be long enough any time there are more than 3 elements that the condition needs to be broken up so each line ends up starting with a let, making them obvious as desired by some here
  • again, let chains with two conditions don't present trouble for me to notice it. I'm looking at cases like https://github.com/rust-lang/rust/blob/fd7a15971069156f1741fb3a3c34c9f0c54d5744/compiler/rustc_ast/src/token.rs#L606-L613 and https://github.com/rust-lang/rust/blob/2600d6245bbc0d04e57e4ce561a6c07bf6b55951/compiler/rustc_hir_typeck/src/expr.rs#L835-L841
  • 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)
  • I keep having trouble formatting long conditions, where I find us having to be... "creative"
  • currently as soon as rustfmt encounters a let chain it completely gives up on the expression, meaning that I keep finding improperly formatted 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 this

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.

@joshtriplett
Copy link
Member

@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.

@estebank
Copy link

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.

@c410-f3r
Copy link

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 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();
}

@calebcartwright
Copy link
Member

@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.

@traviscross
Copy link

@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:

  1. We could allow trailing parenthesized expressions to fall on one line since that would negate concerns over precedence.
  2. We could allow function or method calls (and unary operations on those) in the trailing expressions to fall on one line as those too do not raise concerns over precedence.

There is of course also this option:

  1. Allow all trailing expressions to fall on one line (but not multiple 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.

@CAD97
Copy link

CAD97 commented Oct 25, 2023

The potential for precedence confusion remains in that example: between if (let pat = expr) && test versus if let pat = (expr && test).

@RalfJung
Copy link
Member

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.

@traviscross
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests