From 570a3505b9dd0fcf61b4d9f6a8f2dd6b04cb7e54 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sat, 29 Jul 2017 22:12:18 +0900 Subject: [PATCH 1/4] Make definitive_tactic more generic with separator length --- src/expr.rs | 14 +++++++++++--- src/imports.rs | 1 + src/items.rs | 10 ++++++++-- src/lists.rs | 10 +++++++--- src/types.rs | 2 +- src/vertical.rs | 2 +- 6 files changed, 29 insertions(+), 10 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 68272f5af84..5621bcbf1b9 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -485,7 +485,7 @@ where Some(width) => { let tactic = ListTactic::LimitedHorizontalVertical(context.config.array_width()); - definitive_tactic(&items, tactic, width) + definitive_tactic(&items, tactic, 2, width) } None => DefinitiveListTactic::Vertical, } @@ -494,6 +494,7 @@ where definitive_tactic( &items, ListTactic::LimitedHorizontalVertical(context.config.array_width()), + 2, nested_shape.width, ) } else { @@ -591,7 +592,12 @@ fn rewrite_closure_fn_decl( .width .checked_sub(ret_str.len() + 1) .unwrap_or(0); - let tactic = definitive_tactic(&item_vec, ListTactic::HorizontalVertical, horizontal_budget); + let tactic = definitive_tactic( + &item_vec, + ListTactic::HorizontalVertical, + 2, + horizontal_budget, + ); let arg_shape = match tactic { DefinitiveListTactic::Horizontal => try_opt!(arg_shape.sub_width(ret_str.len() + 1)), _ => arg_shape, @@ -1668,7 +1674,7 @@ fn rewrite_match_pattern( ); let items: Vec<_> = pat_strs.into_iter().map(ListItem::from_str).collect(); - let tactic = definitive_tactic(&items, ListTactic::HorizontalVertical, pat_shape.width); + let tactic = definitive_tactic(&items, ListTactic::HorizontalVertical, 3, pat_shape.width); let fmt = ListFormatting { tactic: tactic, separator: " |", @@ -2216,6 +2222,7 @@ where let tactic = definitive_tactic( &*item_vec, ListTactic::LimitedHorizontalVertical(args_max_width), + 2, one_line_width, ); @@ -2756,6 +2763,7 @@ where let tactic = definitive_tactic( &item_vec, ListTactic::HorizontalVertical, + 2, nested_shape.width, ); let fmt = ListFormatting { diff --git a/src/imports.rs b/src/imports.rs index 00b6711cf9b..bc743649e84 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -451,6 +451,7 @@ fn rewrite_use_list( let tactic = definitive_tactic( &items[first_index..], context.config.imports_layout(), + 2, remaining_width, ); diff --git a/src/items.rs b/src/items.rs index f21277e9e83..8b4fd4c725e 100644 --- a/src/items.rs +++ b/src/items.rs @@ -2241,6 +2241,7 @@ fn rewrite_args( let tactic = definitive_tactic( &arg_items, context.config.fn_args_density().to_list_tactic(), + 2, one_line_budget, ); let budget = match tactic { @@ -2423,7 +2424,12 @@ where { let item_vec = items.collect::>(); - let tactic = definitive_tactic(&item_vec, ListTactic::HorizontalVertical, one_line_budget); + let tactic = definitive_tactic( + &item_vec, + ListTactic::HorizontalVertical, + 2, + one_line_budget, + ); let fmt = ListFormatting { tactic: tactic, separator: ",", @@ -2636,7 +2642,7 @@ fn rewrite_where_clause( let item_vec = items.collect::>(); // FIXME: we don't need to collect here if the where_layout isn't // HorizontalVertical. - let tactic = definitive_tactic(&item_vec, context.config.where_layout(), budget); + let tactic = definitive_tactic(&item_vec, context.config.where_layout(), 2, budget); let mut comma_tactic = context.config.trailing_comma(); // Kind of a hack because we don't usually have trailing commas in where clauses. diff --git a/src/lists.rs b/src/lists.rs index 5a6225a5903..56ac10691de 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -135,7 +135,12 @@ impl DefinitiveListTactic { } } -pub fn definitive_tactic(items: I, tactic: ListTactic, width: usize) -> DefinitiveListTactic +pub fn definitive_tactic( + items: I, + tactic: ListTactic, + sep_len: usize, + width: usize, +) -> DefinitiveListTactic where I: IntoIterator + Clone, T: AsRef, @@ -155,7 +160,6 @@ where }; let (sep_count, total_width) = calculate_width(items.clone()); - let sep_len = ", ".len(); // FIXME: make more generic? let total_sep_len = sep_len * sep_count.checked_sub(1).unwrap_or(0); let real_total = total_width + total_sep_len; @@ -640,7 +644,7 @@ pub fn struct_lit_tactic( (IndentStyle::Visual, 1) => ListTactic::HorizontalVertical, _ => context.config.struct_lit_multiline_style().to_list_tactic(), }; - definitive_tactic(items, prelim_tactic, h_shape.width) + definitive_tactic(items, prelim_tactic, 2, h_shape.width) } else { DefinitiveListTactic::Vertical } diff --git a/src/types.rs b/src/types.rs index 71e6bcdcd18..3144efe9db0 100644 --- a/src/types.rs +++ b/src/types.rs @@ -348,7 +348,7 @@ where let item_vec: Vec<_> = items.collect(); - let tactic = definitive_tactic(&*item_vec, ListTactic::HorizontalVertical, budget); + let tactic = definitive_tactic(&*item_vec, ListTactic::HorizontalVertical, 2, budget); let fmt = ListFormatting { tactic: tactic, diff --git a/src/vertical.rs b/src/vertical.rs index 57876d644c2..08ef6281baa 100644 --- a/src/vertical.rs +++ b/src/vertical.rs @@ -221,7 +221,7 @@ fn rewrite_aligned_items_inner( span.hi, ).collect::>(); - let tactic = definitive_tactic(&items, ListTactic::HorizontalVertical, one_line_width); + let tactic = definitive_tactic(&items, ListTactic::HorizontalVertical, 2, one_line_width); let fmt = ListFormatting { tactic: tactic, From e588f2fd7b0445944f15341f9b45051f440b82b4 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Mon, 31 Jul 2017 16:23:42 +0900 Subject: [PATCH 2/4] Make definitive_tactic more generic via enum Separator --- src/expr.rs | 19 ++++++++++++------- src/imports.rs | 4 ++-- src/items.rs | 13 +++++++++---- src/lists.rs | 24 +++++++++++++++++++++--- src/types.rs | 9 +++++++-- src/vertical.rs | 9 +++++++-- 6 files changed, 58 insertions(+), 20 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 5621bcbf1b9..cf2bc337247 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -24,7 +24,7 @@ use config::{Config, ControlBraceStyle, IndentStyle, MultilineStyle, Style}; use items::{span_hi_for_arg, span_lo_for_arg}; use lists::{definitive_tactic, itemize_list, shape_for_tactic, struct_lit_formatting, struct_lit_shape, struct_lit_tactic, write_list, DefinitiveListTactic, ListFormatting, - ListItem, ListTactic, SeparatorTactic}; + ListItem, ListTactic, Separator, SeparatorTactic}; use macros::{rewrite_macro, MacroPosition}; use patterns::{can_be_overflowed_pat, TuplePatField}; use rewrite::{Rewrite, RewriteContext}; @@ -485,7 +485,7 @@ where Some(width) => { let tactic = ListTactic::LimitedHorizontalVertical(context.config.array_width()); - definitive_tactic(&items, tactic, 2, width) + definitive_tactic(&items, tactic, Separator::Comma, width) } None => DefinitiveListTactic::Vertical, } @@ -494,7 +494,7 @@ where definitive_tactic( &items, ListTactic::LimitedHorizontalVertical(context.config.array_width()), - 2, + Separator::Comma, nested_shape.width, ) } else { @@ -595,7 +595,7 @@ fn rewrite_closure_fn_decl( let tactic = definitive_tactic( &item_vec, ListTactic::HorizontalVertical, - 2, + Separator::Comma, horizontal_budget, ); let arg_shape = match tactic { @@ -1674,7 +1674,12 @@ fn rewrite_match_pattern( ); let items: Vec<_> = pat_strs.into_iter().map(ListItem::from_str).collect(); - let tactic = definitive_tactic(&items, ListTactic::HorizontalVertical, 3, pat_shape.width); + let tactic = definitive_tactic( + &items, + ListTactic::HorizontalVertical, + Separator::VerticalBar, + pat_shape.width, + ); let fmt = ListFormatting { tactic: tactic, separator: " |", @@ -2222,7 +2227,7 @@ where let tactic = definitive_tactic( &*item_vec, ListTactic::LimitedHorizontalVertical(args_max_width), - 2, + Separator::Comma, one_line_width, ); @@ -2763,7 +2768,7 @@ where let tactic = definitive_tactic( &item_vec, ListTactic::HorizontalVertical, - 2, + Separator::Comma, nested_shape.width, ); let fmt = ListFormatting { diff --git a/src/imports.rs b/src/imports.rs index bc743649e84..e0049698bfb 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -17,7 +17,7 @@ use Shape; use codemap::SpanUtils; use config::IndentStyle; use lists::{definitive_tactic, itemize_list, write_list, DefinitiveListTactic, ListFormatting, - ListItem, SeparatorTactic}; + ListItem, Separator, SeparatorTactic}; use rewrite::{Rewrite, RewriteContext}; use types::{rewrite_path, PathContext}; use utils; @@ -451,7 +451,7 @@ fn rewrite_use_list( let tactic = definitive_tactic( &items[first_index..], context.config.imports_layout(), - 2, + Separator::Comma, remaining_width, ); diff --git a/src/items.rs b/src/items.rs index 8b4fd4c725e..89028afdc10 100644 --- a/src/items.rs +++ b/src/items.rs @@ -23,7 +23,7 @@ use config::{BraceStyle, Config, Density, IndentStyle, ReturnIndent, Style}; use expr::{format_expr, is_empty_block, is_simple_block_stmt, rewrite_assign_rhs, rewrite_call_inner, ExprType}; use lists::{definitive_tactic, itemize_list, write_list, DefinitiveListTactic, ListFormatting, - ListItem, ListTactic, SeparatorTactic}; + ListItem, ListTactic, Separator, SeparatorTactic}; use rewrite::{Rewrite, RewriteContext}; use types::join_bounds; use utils::{colon_spaces, contains_skip, end_typaram, format_defaultness, format_mutability, @@ -2241,7 +2241,7 @@ fn rewrite_args( let tactic = definitive_tactic( &arg_items, context.config.fn_args_density().to_list_tactic(), - 2, + Separator::Comma, one_line_budget, ); let budget = match tactic { @@ -2427,7 +2427,7 @@ where let tactic = definitive_tactic( &item_vec, ListTactic::HorizontalVertical, - 2, + Separator::Comma, one_line_budget, ); let fmt = ListFormatting { @@ -2642,7 +2642,12 @@ fn rewrite_where_clause( let item_vec = items.collect::>(); // FIXME: we don't need to collect here if the where_layout isn't // HorizontalVertical. - let tactic = definitive_tactic(&item_vec, context.config.where_layout(), 2, budget); + let tactic = definitive_tactic( + &item_vec, + context.config.where_layout(), + Separator::Comma, + budget, + ); let mut comma_tactic = context.config.trailing_comma(); // Kind of a hack because we don't usually have trailing commas in where clauses. diff --git a/src/lists.rs b/src/lists.rs index 56ac10691de..048f11a1856 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -135,10 +135,28 @@ impl DefinitiveListTactic { } } +/// The type of separator for lists. +#[derive(Eq, PartialEq, Debug)] +pub enum Separator { + Comma, + VerticalBar, +} + +impl Separator { + pub fn len(&self) -> usize { + match *self { + // 2 = `, ` + Separator::Comma => 2, + // 3 = ` | ` + Separator::VerticalBar => 3, + } + } +} + pub fn definitive_tactic( items: I, tactic: ListTactic, - sep_len: usize, + sep: Separator, width: usize, ) -> DefinitiveListTactic where @@ -160,7 +178,7 @@ where }; let (sep_count, total_width) = calculate_width(items.clone()); - let total_sep_len = sep_len * sep_count.checked_sub(1).unwrap_or(0); + let total_sep_len = sep.len() * sep_count.checked_sub(1).unwrap_or(0); let real_total = total_width + total_sep_len; if real_total <= limit && !pre_line_comments && @@ -644,7 +662,7 @@ pub fn struct_lit_tactic( (IndentStyle::Visual, 1) => ListTactic::HorizontalVertical, _ => context.config.struct_lit_multiline_style().to_list_tactic(), }; - definitive_tactic(items, prelim_tactic, 2, h_shape.width) + definitive_tactic(items, prelim_tactic, Separator::Comma, h_shape.width) } else { DefinitiveListTactic::Vertical } diff --git a/src/types.rs b/src/types.rs index 3144efe9db0..8e0cf2436de 100644 --- a/src/types.rs +++ b/src/types.rs @@ -22,7 +22,7 @@ use codemap::SpanUtils; use config::{IndentStyle, Style, TypeDensity}; use expr::{rewrite_pair, rewrite_tuple, rewrite_unary_prefix, wrap_args_with_parens}; use items::{format_generics_item_list, generics_shape_from_config}; -use lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListTactic, +use lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListTactic, Separator, SeparatorTactic}; use rewrite::{Rewrite, RewriteContext}; use utils::{colon_spaces, extra_offset, format_mutability, last_line_width, mk_sp, wrap_str}; @@ -348,7 +348,12 @@ where let item_vec: Vec<_> = items.collect(); - let tactic = definitive_tactic(&*item_vec, ListTactic::HorizontalVertical, 2, budget); + let tactic = definitive_tactic( + &*item_vec, + ListTactic::HorizontalVertical, + Separator::Comma, + budget, + ); let fmt = ListFormatting { tactic: tactic, diff --git a/src/vertical.rs b/src/vertical.rs index 08ef6281baa..7f43fa8681a 100644 --- a/src/vertical.rs +++ b/src/vertical.rs @@ -20,7 +20,7 @@ use codemap::SpanUtils; use comment::contains_comment; use expr::rewrite_field; use items::{rewrite_struct_field, rewrite_struct_field_prefix}; -use lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListTactic}; +use lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListTactic, Separator}; use rewrite::{Rewrite, RewriteContext}; use utils::{contains_skip, mk_sp}; @@ -221,7 +221,12 @@ fn rewrite_aligned_items_inner( span.hi, ).collect::>(); - let tactic = definitive_tactic(&items, ListTactic::HorizontalVertical, 2, one_line_width); + let tactic = definitive_tactic( + &items, + ListTactic::HorizontalVertical, + Separator::Comma, + one_line_width, + ); let fmt = ListFormatting { tactic: tactic, From 568a9adf8780c80b3301d25981a4926307d73e50 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Mon, 31 Jul 2017 16:24:07 +0900 Subject: [PATCH 3/4] Avoid early line breaks in match condition --- src/expr.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index cf2bc337247..0a1a4a58316 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1531,17 +1531,25 @@ fn rewrite_match( return None; } - // 6 = `match `, 2 = ` {` + // Do not take the rhs overhead from the upper expressions into account + // when rewriting match condition. + let new_width = try_opt!(context.config.max_width().checked_sub(shape.used_width())); + let cond_shape = Shape { + width: new_width, + ..shape + }; + // 6 = `match ` let cond_shape = match context.config.control_style() { - Style::Legacy => try_opt!(shape.shrink_left(6).and_then(|s| s.sub_width(2))), - Style::Rfc => try_opt!(shape.offset_left(6).and_then(|s| s.sub_width(2))), + Style::Legacy => try_opt!(cond_shape.shrink_left(6)), + Style::Rfc => try_opt!(cond_shape.offset_left(6)), }; let cond_str = try_opt!(cond.rewrite(context, cond_shape)); let alt_block_sep = String::from("\n") + &shape.indent.block_only().to_string(context.config); let block_sep = match context.config.control_brace_style() { ControlBraceStyle::AlwaysNextLine => &alt_block_sep, _ if last_line_extendable(&cond_str) => " ", - _ if cond_str.contains('\n') => &alt_block_sep, + // 2 = ` {` + _ if cond_str.contains('\n') || cond_str.len() + 2 > cond_shape.width => &alt_block_sep, _ => " ", }; From 36b347b123f1fffe0b3b8776726d6a8e2950bb3a Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Mon, 31 Jul 2017 16:24:18 +0900 Subject: [PATCH 4/4] Update tests --- tests/source/match.rs | 20 ++++++++++++++++++++ tests/target/match.rs | 24 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/tests/source/match.rs b/tests/source/match.rs index bbd6d48827c..cd55c7a1a61 100644 --- a/tests/source/match.rs +++ b/tests/source/match.rs @@ -388,3 +388,23 @@ fn issue525() { TaskState::Failed => "failed", }); } + +// #1838, #1839 +fn match_with_near_max_width() { + let (this_line_uses_99_characters_and_is_formatted_properly, x012345) = match some_expression { + _ => unimplemented!(), + }; + + let (should_be_formatted_like_the_line_above_using_100_characters, x0) = match some_expression { + _ => unimplemented!(), + }; + + let (should_put_the_brace_on_the_next_line_using_101_characters, x0000) = match some_expression + { + _ => unimplemented!(), + }; + match m { + Variant::Tag | Variant::Tag2 | Variant::Tag3 | Variant::Tag4 | Variant::Tag5 | Variant::Tag6 => + {} + } +} diff --git a/tests/target/match.rs b/tests/target/match.rs index d0b7e665459..cb4d6c1ed62 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -426,3 +426,27 @@ fn issue525() { }, ); } + +// #1838, #1839 +fn match_with_near_max_width() { + let (this_line_uses_99_characters_and_is_formatted_properly, x012345) = match some_expression { + _ => unimplemented!(), + }; + + let (should_be_formatted_like_the_line_above_using_100_characters, x0) = match some_expression { + _ => unimplemented!(), + }; + + let (should_put_the_brace_on_the_next_line_using_101_characters, x0000) = match some_expression + { + _ => unimplemented!(), + }; + match m { + Variant::Tag | + Variant::Tag2 | + Variant::Tag3 | + Variant::Tag4 | + Variant::Tag5 | + Variant::Tag6 => {} + } +}