From 262feb33373b2d6b909f00e86be635c6e19886b6 Mon Sep 17 00:00:00 2001 From: Linda_pp Date: Sat, 9 Sep 2023 10:05:07 +0900 Subject: [PATCH] Fix checking if newline is needed before `else` in let-else statement Fixes 5901 Take leading attributes and comments into consideration when determining if we need to wrap the `else` keyword onto the next line. --- src/items.rs | 19 ++++++++-- tests/source/let_else.rs | 20 ++++++++++ tests/source/let_else_v2.rs | 56 ++++++++++++++++++++++++++++ tests/target/let_else.rs | 31 ++++++++++++++++ tests/target/let_else_v2.rs | 73 +++++++++++++++++++++++++++++++++++++ 5 files changed, 196 insertions(+), 3 deletions(-) create mode 100644 tests/source/let_else_v2.rs create mode 100644 tests/target/let_else_v2.rs diff --git a/src/items.rs b/src/items.rs index 4d82e192b7d..48c37df812c 100644 --- a/src/items.rs +++ b/src/items.rs @@ -75,6 +75,7 @@ fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option false, )? }; + let let_kw_offset = result.len() - "let ".len(); // 4 = "let ".len() let pat_shape = shape.offset_left(4)?; @@ -127,8 +128,15 @@ fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option if let Some(block) = else_block { let else_kw_span = init.span.between(block.span); + // Strip attributes and comments to check if newline is needed before the else + // keyword from the initializer part. (#5901) + let init_str = if context.config.version() == Version::Two { + &result[let_kw_offset..] + } else { + result.as_str() + }; let force_newline_else = pat_str.contains('\n') - || !same_line_else_kw_and_brace(&result, context, else_kw_span, nested_shape); + || !same_line_else_kw_and_brace(init_str, context, else_kw_span, nested_shape); let else_kw = rewrite_else_kw_with_comments( force_newline_else, true, @@ -146,11 +154,16 @@ fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option 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 assign_str_with_else_kw = if context.config.version() == Version::Two { + &result[let_kw_offset..] + } else { + result.as_str() + }; + let available_space = max_width.saturating_sub(assign_str_with_else_kw.len()); let allow_single_line = !force_newline_else && available_space > 0 - && allow_single_line_let_else_block(&result, block); + && allow_single_line_let_else_block(assign_str_with_else_kw, block); let mut rw_else_block = rewrite_let_else_block(block, allow_single_line, context, shape)?; diff --git a/tests/source/let_else.rs b/tests/source/let_else.rs index 85b3604ad3c..cb2859e805d 100644 --- a/tests/source/let_else.rs +++ b/tests/source/let_else.rs @@ -160,3 +160,23 @@ fn with_trailing_try_operator() { // Maybe this is a workaround? let Ok(Some(next_bucket)) = ranking_rules[cur_ranking_rule_index].next_bucket(ctx, logger, &ranking_rule_universes[cur_ranking_rule_index]) else { return }; } + +fn issue5901() { + #[cfg(target_os = "linux")] + let Some(x) = foo else { todo!() }; + + #[cfg(target_os = "linux")] + // Some comments between attributes and let-else statement + let Some(x) = foo else { todo!() }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = foo else { todo!() }; + + // The else block will be multi-lined because attributes and comments before `let` + // are included when calculating max width + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + // Some comments between attributes and let-else statement + let Some(x) = foo() else { todo!() }; +} diff --git a/tests/source/let_else_v2.rs b/tests/source/let_else_v2.rs new file mode 100644 index 00000000000..a420fbcf95b --- /dev/null +++ b/tests/source/let_else_v2.rs @@ -0,0 +1,56 @@ +// rustfmt-version: Two +// rustfmt-single_line_let_else_max_width: 100 + +fn issue5901() { + #[cfg(target_os = "linux")] + let Some(x) = foo else { todo!() }; + + #[cfg(target_os = "linux")] + // Some comments between attributes and let-else statement + let Some(x) = foo else { todo!() }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = foo else { todo!() }; + + // The else block is multi-lined + #[cfg(target_os = "linux")] + let Some(x) = foo else { return; }; + + // The else block will be single-lined because attributes and comments before `let` + // are no longer included when calculating max width + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + // Some comments between attributes and let-else statement + let Some(x) = foo else { todo!() }; + + // Some more test cases for v2 formatting with attributes + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = opt + // pre else keyword line-comment + else { return; }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = opt else + // post else keyword line-comment + { return; }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Foo {x: Bar(..), y: FooBar(..), z: Baz(..)} = opt else { + return; + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(Ok((Message::ChangeColor(super::color::Color::Rgb(r, g, b)), Point { x, y, z }))) = opt else { + return; + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = very_very_very_very_very_very_very_very_very_very_very_very_long_expression_in_assign_rhs() else { return; }; +} diff --git a/tests/target/let_else.rs b/tests/target/let_else.rs index 6554a0961c0..f6560e85462 100644 --- a/tests/target/let_else.rs +++ b/tests/target/let_else.rs @@ -252,3 +252,34 @@ fn with_trailing_try_operator() { return; }; } + +fn issue5901() { + #[cfg(target_os = "linux")] + let Some(x) = foo + else { + todo!() + }; + + #[cfg(target_os = "linux")] + // Some comments between attributes and let-else statement + let Some(x) = foo + else { + todo!() + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = foo + else { + todo!() + }; + + // The else block will be multi-lined because attributes and comments before `let` + // are included when calculating max width + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + // Some comments between attributes and let-else statement + let Some(x) = foo() else { + todo!() + }; +} diff --git a/tests/target/let_else_v2.rs b/tests/target/let_else_v2.rs new file mode 100644 index 00000000000..b25ac1609d8 --- /dev/null +++ b/tests/target/let_else_v2.rs @@ -0,0 +1,73 @@ +// rustfmt-version: Two +// rustfmt-single_line_let_else_max_width: 100 + +fn issue5901() { + #[cfg(target_os = "linux")] + let Some(x) = foo else { todo!() }; + + #[cfg(target_os = "linux")] + // Some comments between attributes and let-else statement + let Some(x) = foo else { todo!() }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = foo else { todo!() }; + + // The else block is multi-lined + #[cfg(target_os = "linux")] + let Some(x) = foo else { + return; + }; + + // The else block will be single-lined because attributes and comments before `let` + // are no longer included when calculating max width + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + // Some comments between attributes and let-else statement + let Some(x) = foo else { todo!() }; + + // Some more test cases for v2 formatting with attributes + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = opt + // pre else keyword line-comment + else { + return; + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = opt else + // post else keyword line-comment + { + return; + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Foo { + x: Bar(..), + y: FooBar(..), + z: Baz(..), + } = opt + else { + return; + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(Ok((Message::ChangeColor(super::color::Color::Rgb(r, g, b)), Point { x, y, z }))) = + opt + else { + return; + }; + + #[cfg(target_os = "linux")] + #[cfg(target_arch = "x86_64")] + let Some(x) = + very_very_very_very_very_very_very_very_very_very_very_very_long_expression_in_assign_rhs() + else { + return; + }; +}