reserved width for unsafe in match arm pattern - #7097
Conversation
|
one question? should this change be gated? |
|
@AsthaMishra Thank you for working on this. Yeah, let's gate this fix on |
|
Also are we sure that things are correct for other types of blocks? Let's add test cases for them just to be sure. |
…iles also updated for same
| try {} | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
Looks like we're forcing const, async, gen, and try blocks to be wrapped in an outer block. Does that always happen or just when we can't fit everything onto one line?
unsafe 6 chars
const 5 chars
async 5 chars
gen 3 chars
try 3 chars
All of these are getting tested against the same ( ExampleTypeX::VariantAlphaSampleXYZ, ExampleTypeX::VariantBetaXYZ) tuple. To make sure we're exhaustively testing this I would like to test the following cases for each type of block:
- 1 char below the
max_widthlimit when accounting for the pattern,=>,keyword,{, and any whitespace in between.. - everything properly fits on 1 line at exactly the
max_widthlimit when accounting for the pattern,=>,keyword,{, and any whitespace in between.. - 1 char over the
max_widthlimit when accounting for the pattern,=>,keyword,{, and any whitespace in between.
There was a problem hiding this comment.
Looks like we're forcing
const,async,gen, andtryblocks to be wrapped in an outer block. Does that always happen or just when we can't fit everything onto one line?
outer block only added when we can't fit everything in one line
There was a problem hiding this comment.
Test cases added for above mentioned pointers. There was one more bug , when empty blocks exceeds max-width, we get line overflow error.
reason -
Lines 530 to 537 in 2d897e2
if is_block || (!body_str.contains('\n') && unicode_str_width(body_str) <= body_shape.width)
code sees if is_block is true and 'OR' condition is bypassed i.e. if there is no new line in body_str and if it fits in the available column space. this is only for what comes after => in same line.
Fix: add a way to enforce execution of (!body_str.contains('\n') && unicode_str_width(body_str) <= body_shape.width for empty blocks
let enforce_empty_block_width =
is_empty_block && context.config.style_edition() >= StyleEdition::Edition2027;
match rewrite {
Ok(ref body_str)
if (is_block && !enforce_empty_block_width)
|| (!body_str.contains('\n')
&& unicode_str_width(body_str) <= body_shape.width) =>
{
return combine_orig_body(body_str);
}
_ => rewrite,
}
| ast::ExprKind::Block(block, None) | ||
| if is_unsafe_block(block) |
There was a problem hiding this comment.
Is it ever possible to have an unsafe block with a label?
There was a problem hiding this comment.
'a: unsafe{ 5 } gives compile error - after 'a -> a loop or block is expected.
| // everything properly fits on 1 line at exactly the max_width limit | ||
| (ExampleTypeX::VariantABCDE, ExampleTypeX::VariantBetaXYZ) => unsafe {}, | ||
| // 1 char over the max_width limit | ||
| (ExampleTypeX::VariantABCDEF, ExampleTypeX::VariantBetaXYZ) => unsafe {}, |
There was a problem hiding this comment.
I'm trying to understand why there's this inconsistency between the unsafe formatting and const, async, gen and try examples below.
Do we need to update the pattern matching for these other kinds of blocks in rewrite_match_arm?
There was a problem hiding this comment.
pattern matching seems correct we are doing same thing in expr.rs as well and for ast::ExprKind::Block unsafe cases are handled in if branch.
we are just missing pattern arm for unsafe block,
Lines 280 to 295 in 7bc6cd7
lable block works, - ast::ExprKind::Block(_, Some(label))
const , async, gen and try - these works - default arm
| // 12 = ` => unsafe {` | ||
| shape | ||
| .sub_width(12, arm.span)? | ||
| .offset_left(pipe_offset, arm.span)? |
There was a problem hiding this comment.
If we know the block is empty, should we:
// 13 = ` => unsafe {}`
sub_width(13, arm.span)| ( | ||
| ExampleTypeX::VariantABCDEFGHIJKL, | ||
| ExampleTypeX::VariantBetaXYZ, | ||
| ) => unsafe { non_empty_block() }, |
There was a problem hiding this comment.
Based on these tests it seems that we'll never wrap the pattern like this for const, async, gen, or try blocks.
There was a problem hiding this comment.
Based on these tests it seems that we'll never wrap the pattern like this for
const,async,gen, ortryblocks.
yes, wrapping only happens for when is_block = false.
Lines 460 to 471 in 7bc6cd7
if is_block i.e. label block and unsafe block - wrapping is not done
for const, async, gen, try wrapping happens at 490
Lines 473 to 493 in 7bc6cd7
Issue :
unsafeis a block with labelNoneand when patterns code executes and label: None is found, execution goes to fallback arm where only 5 column spaces are reserved (which is correct forasync,const,genandtryblocks) but unsafe needs 12 column spaces, this is what causingmax_widthviolation and when run with--config error_on_line_overflow=trueit does give line overflow error for max-width 80Fix : added an arm for unsafe block to reserve required space for unsafe
Fixes : #6848