implement `single_line_let_else_max_width` #5790

ytmimi · 2023-06-20T13:50:38Z

This is a follow up PR to #5690

This allows users to configure the maximum length of a single line let-else statements. let-else statements that otherwise meet the requirements to be formatted on a single line (as described by the style guide) will have their divergent else block formatted over multiple lines if they exceed this length.

calebcartwright · 2023-06-20T14:00:02Z

Thanks for getting this up so quickly! We're only going to get this one shot at it, so will need to make sure this is both complete/perfect (as much as it can be) and stable which is a bit of an usual path for a net-new config opt.

Will try to review later tonight after work

calebcartwright · 2023-06-20T14:08:12Z

also @fee1-dead if you have the bandwidth an are interested in giving this a review your perspective would be greatly appreciated

src/config/config_type.rs

src/items.rs

fee1-dead

LGTM. Had a few nits

src/config/options.rs

src/items.rs

calebcartwright

Implementation LGTM, thank you! Few requested changes/additions to the docs and tests then should be good to go

calebcartwright · 2023-06-23T19:20:42Z

Configurations.md

+    let Some(x) = opt else { return };
+
+    let Some(x) = some_very_very_very_very_long_name else {
+        return;


let's remove the semicolon here.

in this context we want to demonstrate snippets that are eligible for single line based on all other rules, so that whether they are wrapped is solely dependent upon length and the value of the config option. the semis are load bearing though which means the snippets would always have to be wrapped regardless

Suggested change

return;

return

Unfortunately I can't remove the ;. The configuration_snippet_tests would fail without it since rustfmt will add the ; by default.

Sometimes I forget that we can't readily utilize other config options (e.g. trailing_semicolon) in these snippets.

I also don't recall whether we've discussed this before, but I think having the ability to specify additional options in the snippets (perhaps similar to what we do in the system&idempotence tests?) would be helpful

I think the idea has briefly come up before. It's a change that I'd really like to see! I think it'll also be very helpful if we need to demonstrate formatting differences between version (and eventually style_edition)

Configurations.md

calebcartwright · 2023-06-23T19:23:18Z

Configurations.md

+    let Some(y) = opt else {
+        return;
+    };


Same ordering suggestion as prior section

Configurations.md

src/config/mod.rs

calebcartwright · 2023-06-23T19:26:59Z

src/config/options.rs

@@ -236,6 +236,9 @@ pub struct WidthHeuristics {
    // Maximum line length for single line if-else expressions. A value
    // of zero means always break if-else expressions.
    pub(crate) single_line_if_else_max_width: usize,
+    // Maximum line length for single line let-else statements. A value of zero means
+    // always format the divergent `else block` over multiple lines.


Same theme as prior comments

Suggested change

// always format the divergent `else block` over multiple lines.

// always format the divergent `else` block over multiple lines.

calebcartwright · 2023-06-23T19:33:34Z

src/items.rs

+                let max_width =
+                    std::cmp::min(shape.width, context.config.single_line_let_else_max_width());
+
+                // If available_space hits zero we know for sure this will be a multi-lined block
+                let available_space = max_width.saturating_sub(result.len());
+
+                let allow_single_line = !force_newline_else
+                    && available_space > 0
+                    && allow_single_line_let_else_block(&result, block);

                let mut rw_else_block =
                    rewrite_let_else_block(block, allow_single_line, context, shape)?;

-                if allow_single_line && !rw_else_block.contains('\n') {
-                    let available_space = shape.width.saturating_sub(result.len());
-                    if available_space <= rw_else_block.len() {
-                        // writing this on one line would exceed the available width
-                        rw_else_block = rewrite_let_else_block(block, false, context, shape)?;
-                    }
+                let single_line_else = !rw_else_block.contains('\n');
+                // +1 for the trailing `;`
+                let else_block_exceeds_width = rw_else_block.len() + 1 > available_space;
+
+                if allow_single_line && single_line_else && else_block_exceeds_width {
+                    // writing this on one line would exceed the available width
+                    // so rewrite the else block over multiple lines.
+                    rw_else_block = rewrite_let_else_block(block, false, context, shape)?;


I concur with some of the earlier discussions on this that the high level theme of limit derivation, checking if a formatted version can fit within that limit, else format over multiple lines, feels like a potentially recurrent pattern that could be encapsulated in some manner that lends itself to reuse.

However, even if so, that's the type of internal change we could make afterwards with no impact on resultant behavior so I'd also agree that should be punted to a hypothetical future iteration.

great! appreciate you sharing your thoughts on that!

calebcartwright · 2023-06-23T19:38:01Z

tests/target/configs/single_line_let_else_max_width/50.rs

+    let Some(d) = some_very_very_very_very_long_name else {
+        return;
+    };
+}


Could we include some additional cases at least here (with the val set to 50) that cover the other init expr scenarios?

E.g.

// Because the initializer expr is multi-lined the else is placed on it's own line. let Some(x) = some_really_really_really_really_really_really_really_really_really_long_name____H else { return; };

As well as the multilined init expr that starts on the same line as the assignment operator and ends with a sequence of one or more parens/brackets/braces?

ytmimi · 2023-06-27T03:27:30Z

@calebcartwright let me know if these additional test cases are good. I'm happy to add any others you think are necessary for the new option.

calebcartwright · 2023-06-27T15:39:02Z

tests/target/configs/single_line_let_else_max_width/100.rs

+    let Expr::Slice(ast::ExprSlice {
+        lower,
+        upper,
+        step,
+        range: _,
+    }) = slice.as_ref() else {
+        return;
+    };


Glad we added this test, as I'm unsure whether this is desired behavior.

It would appear that the prescriptions in https://doc.rust-lang.org/nightly/style-guide/statements.html#else-blocks-let-else-statements do not explicitly cover the scenario where the statement is multilined prior to the the else block with an initializer expression that is single line.

I feel like one could read this:

Otherwise, the else keyword and opening brace should be placed on the next line after the end of the initializer expression, and the else keyword should have the same indentation level as the let keyword.

As implying that if any of the prior scenarios aren't true, then break before the else keyword, but that is rather implicit. Thoughts? 🤔

posted in zulip for style team as well https://rust-lang.zulipchat.com/#narrow/stream/346005-t-style/topic/let-else.20style.20guide.20inconsistency.3F/near/370115141

I'll investigate this case and look into what's goin on. Now that you point it out I think I would have expected it to be formatted like:

let Expr::Slice(ast::ExprSlice { lower, upper, step, range: _, }) = slice.as_ref() else { return; };

The changes to produce the above formatting are pretty minimal. Let me know if I should go ahead and amend this PR once you hear back from t-style.

calebcartwright · 2023-06-27T21:05:12Z

I wouldn't be opposed to merging this as-is, and tackling any potential changes needed to address the above scenario in a separate PR.

My only slight reservation is that it may create some ambiguity for folks subscribed to the tracking issue (i.e. this PR and the associated issue are the last pending "big steps" articulated there), though I guess the same would be true for the blog post.

Lmk what you think; I'm probably leaning towards merging but not a strongly held position

ytmimi · 2023-06-27T21:10:05Z

updating the PR now

calebcartwright · 2023-06-27T21:15:01Z

updating the PR now

in that case I guess we'll park this until there's direction one way or another from t-style 😄

I think there's enough here to justify having separate commits, though I also think the current 10 could probably be condensed into a smaller number. That's not strictly necessary, but wanted to share my observation in case you concur and would be interested in adjusting the commit history

ytmimi · 2023-06-27T21:22:16Z

Yeah, there's definitely room to reduce the number of commits. I'm happy to rebase! I just figured it was easier to have individual commits while working on this.

If we get word from t-style I can always remove the last commit. I'll hold off on making changes until it's clear how to move forward, though I guess we have discretion to make this change without an amendment to the guide, since this is the implied formatting from the style guide, correct?

calebcartwright · 2023-06-27T21:32:28Z

though I guess we have discretion to make this change without an amendment to the guide, since this is the implied formatting from the style guide, correct?

Eh, I suspect this is something the Style team needs to explicitly respond to one way or the other, even if the position is "we defer to tools to interpret", though I suspect that's the least likely outcome

calebcartwright · 2023-06-28T20:34:18Z

though I guess we have discretion to make this change without an amendment to the guide, since this is the implied formatting from the style guide, correct?

Eh, I suspect this is something the Style team needs to explicitly respond to one way or the other, even if the position is "we defer to tools to interpret", though I suspect that's the least likely outcome

Seems wrapping is the expected behavior, and I anticipate an update to the rules to make this more explicit. As such I'm probably going to merge this later tonight, and move the blog post out of draft.

ytmimi · 2023-06-28T21:00:29Z

alright, I'll try to get the rebase in soon, just have a few more things I need to do for work

calebcartwright · 2023-06-28T21:54:08Z

alright, I'll try to get the rebase in soon, just have a few more things I need to do for work

sorry I forgot about modifying the commit history. no rush, whenever you're ready.

i just wanted to share that we had an answer on the expected behavior from the style team

This allows users to configure the maximum length of a single line `let-else` statements. `let-else` statements that otherwise meet the requirements to be formatted on a single line will have their divergent `else` block formatted over multiple lines if they exceed this length. **Note**: `single_line_let_else_max_widt` will be introduced as a stable configuration option.

By reversing the logic I felt that the code became a clearer. Also, added a comment to make it clear that we need to take the trailing semicolon for the `let-else` statement into account.

This rule wasn't explicity stated in the style guide so it was missed, but luckily we caught it during testing.

ytmimi · 2023-06-29T13:15:09Z

@calebcartwright I think we'er good to go now. I've broken this up into 3 commits. The first fully implements single_line_let_else_max_width, while the other 2 make tweaks to the let-else formatting code.

Of those 2 extra commits the first inverts some existing logic, but doesn't change any formatting behavior, and the second implements wrapping the else when the pattern is too long.

ytmimi requested a review from calebcartwright June 20, 2023 13:50

ytmimi force-pushed the let_else_formatting_with_option branch from 0f02d2b to 32fdb2d Compare June 20, 2023 14:01

fee1-dead reviewed Jun 20, 2023

View reviewed changes

src/config/config_type.rs Show resolved Hide resolved

src/items.rs Outdated Show resolved Hide resolved

ytmimi added pr-waiting-on-author pr-follow-up-review-pending and removed pr-waiting-on-author labels Jun 21, 2023

fee1-dead approved these changes Jun 23, 2023

View reviewed changes

src/config/options.rs Outdated Show resolved Hide resolved

src/items.rs Outdated Show resolved Hide resolved

ytmimi force-pushed the let_else_formatting_with_option branch from c2673b2 to 3c37268 Compare June 23, 2023 11:31

calebcartwright reviewed Jun 23, 2023

View reviewed changes

calebcartwright added pr-waiting-on-author and removed pr-follow-up-review-pending labels Jun 23, 2023

ytmimi force-pushed the let_else_formatting_with_option branch from 9d1b66b to 84364ba Compare June 27, 2023 03:24

ytmimi removed the pr-waiting-on-author label Jun 27, 2023

calebcartwright reviewed Jun 27, 2023

View reviewed changes

ytmimi added 3 commits June 29, 2023 09:03

update else_block_exceeds_width calculation in let-else rewrite

760184a

By reversing the logic I felt that the code became a clearer. Also, added a comment to make it clear that we need to take the trailing semicolon for the `let-else` statement into account.

wrap else to next line if let-else pattern is multi-lined

3abd617

This rule wasn't explicity stated in the style guide so it was missed, but luckily we caught it during testing.

ytmimi force-pushed the let_else_formatting_with_option branch from e889a3f to 3abd617 Compare June 29, 2023 13:07

calebcartwright merged commit 1de65a2 into rust-lang:master Jul 1, 2023

calebcartwright mentioned this pull request Jul 1, 2023

"short" config option for let-else statements? #5684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `single_line_let_else_max_width` #5790

implement `single_line_let_else_max_width` #5790

ytmimi commented Jun 20, 2023 •

edited

Loading

calebcartwright commented Jun 20, 2023

calebcartwright commented Jun 20, 2023

fee1-dead left a comment

calebcartwright left a comment

calebcartwright Jun 23, 2023

ytmimi Jun 27, 2023 •

edited

Loading

calebcartwright Jun 27, 2023

ytmimi Jun 29, 2023

calebcartwright Jun 23, 2023

calebcartwright Jun 23, 2023

calebcartwright Jun 23, 2023

ytmimi Jun 27, 2023

calebcartwright Jun 23, 2023

ytmimi commented Jun 27, 2023

calebcartwright Jun 27, 2023

calebcartwright Jun 27, 2023

ytmimi Jun 27, 2023

ytmimi Jun 27, 2023

calebcartwright commented Jun 27, 2023

ytmimi commented Jun 27, 2023

calebcartwright commented Jun 27, 2023

ytmimi commented Jun 27, 2023

calebcartwright commented Jun 27, 2023

calebcartwright commented Jun 28, 2023

ytmimi commented Jun 28, 2023

calebcartwright commented Jun 28, 2023

ytmimi commented Jun 29, 2023

	// always format the divergent `else block` over multiple lines.
	// always format the divergent `else` block over multiple lines.

implement single_line_let_else_max_width #5790

implement single_line_let_else_max_width #5790

Conversation

ytmimi commented Jun 20, 2023 • edited Loading

calebcartwright commented Jun 20, 2023

calebcartwright commented Jun 20, 2023

fee1-dead left a comment

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Jun 27, 2023

ytmimi commented Jun 27, 2023

calebcartwright commented Jun 27, 2023

ytmimi commented Jun 27, 2023

calebcartwright commented Jun 27, 2023

calebcartwright commented Jun 28, 2023

ytmimi commented Jun 28, 2023

calebcartwright commented Jun 28, 2023

ytmimi commented Jun 29, 2023

implement `single_line_let_else_max_width` #5790

implement `single_line_let_else_max_width` #5790

ytmimi commented Jun 20, 2023 •

edited

Loading

ytmimi Jun 27, 2023 •

edited

Loading