From 805987b4b11b3a2518403ee063e5809f6a0178d1 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Fri, 6 Apr 2018 22:32:30 +0900 Subject: [PATCH 1/5] Add tests for merge_imports config option --- tests/source/merge_imports.rs | 26 ++++++++++++++++++++++++++ tests/target/merge_imports.rs | 18 ++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/source/merge_imports.rs create mode 100644 tests/target/merge_imports.rs diff --git a/tests/source/merge_imports.rs b/tests/source/merge_imports.rs new file mode 100644 index 00000000000..8033e8d8061 --- /dev/null +++ b/tests/source/merge_imports.rs @@ -0,0 +1,26 @@ +// rustfmt-merge_imports: true +// rustfmt-reorder_imports: true +// rustfmt-reorder_imported_names: true + +use a::{c,d,b}; +use a::{d, e, b, a, f}; +use a::{f, g, c}; + +#[doc(hidden)] +use a::b; +use a::c; +use a::d; + +use a::{c, d, e}; +#[doc(hidden)] +use a::b; +use a::d; + +pub use foo::bar; +use foo::{a, b, c}; +pub use foo::foobar; + +use a::{b::{c::*}}; +use a::{b::{c::{}}}; +use a::{b::{c::d}}; +use a::{b::{c::{xxx, yyy, zzz}}}; diff --git a/tests/target/merge_imports.rs b/tests/target/merge_imports.rs new file mode 100644 index 00000000000..9ce6ef7ee7a --- /dev/null +++ b/tests/target/merge_imports.rs @@ -0,0 +1,18 @@ +// rustfmt-merge_imports: true +// rustfmt-reorder_imports: true +// rustfmt-reorder_imported_names: true + +use a::{a, b, c, d, e, f, g}; + +#[doc(hidden)] +use a::b; +use a::{c, d}; + +#[doc(hidden)] +use a::b; +use a::{c, d, e}; + +use foo::{a, b, c}; +pub use foo::{bar, foobar}; + +use a::b::c::{d, xxx, yyy, zzz, *}; From 5dd203eabe747903c3374c1edf5b8e3baaa6a886 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Fri, 6 Apr 2018 22:34:41 +0900 Subject: [PATCH 2/5] Add merge_imports config option --- Configurations.md | 22 ++++++++++++++++++++++ src/config/mod.rs | 1 + 2 files changed, 23 insertions(+) diff --git a/Configurations.md b/Configurations.md index ca804b2f7f2..3122275e300 100644 --- a/Configurations.md +++ b/Configurations.md @@ -1084,6 +1084,28 @@ use foo::{aaa, fff}; ``` +## `merge_imports` + +Merge multiple imports into a single nested import. + +- **Default value**: `false` +- **Possible values**: `true`, `false` +- **Stable**: No + +#### `false` (default): + +```rust +use foo::{a, c, d}; +use foo::{b, g}; +use foo::{e, f}; +``` + +#### `true`: + +```rust +use foo::{a, b, c, d, e, f, g}; +``` + ## `match_block_trailing_comma` diff --git a/src/config/mod.rs b/src/config/mod.rs index 3676aed4ba9..292b2cd6915 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -66,6 +66,7 @@ create_config! { // Imports imports_indent: IndentStyle, IndentStyle::Visual, false, "Indent of imports"; imports_layout: ListTactic, ListTactic::Mixed, false, "Item layout inside a import block"; + merge_imports: bool, false, false, "Merge imports"; // Ordering reorder_extern_crates: bool, true, false, "Reorder extern crate statements alphabetically"; From 1954513ace66d81b62692f208fdaa91ab48c9757 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Fri, 6 Apr 2018 22:35:04 +0900 Subject: [PATCH 3/5] Merge imports with the same prefix into a single nested import --- src/imports.rs | 282 +++++++++++++++++++++++++++++++++++++++++++++++-- src/lists.rs | 10 ++ src/reorder.rs | 32 ++++-- 3 files changed, 306 insertions(+), 18 deletions(-) diff --git a/src/imports.rs b/src/imports.rs index 9a02ee64fb7..76fa7816f0d 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -12,7 +12,7 @@ use std::cmp::Ordering; use config::lists::*; use syntax::ast::{self, UseTreeKind}; -use syntax::codemap::{BytePos, Span}; +use syntax::codemap::{self, BytePos, Span, DUMMY_SP}; use codemap::SpanUtils; use config::IndentStyle; @@ -24,6 +24,7 @@ use utils::mk_sp; use visitor::FmtVisitor; use std::borrow::Cow; +use std::fmt; /// Returns a name imported by a `use` declaration. e.g. returns `Ordering` /// for `std::cmp::Ordering` and `self` for `std::cmp::self`. @@ -89,7 +90,7 @@ impl<'a> FmtVisitor<'a> { // sorting. // FIXME we do a lot of allocation to make our own representation. -#[derive(Debug, Clone, Eq, PartialEq)] +#[derive(Clone, Eq, PartialEq)] pub enum UseSegment { Ident(String, Option), Slf(Option), @@ -98,12 +99,12 @@ pub enum UseSegment { List(Vec), } -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct UseTree { pub path: Vec, pub span: Span, // Comment information within nested use tree. - list_item: Option, + pub list_item: Option, // Additional fields for top level use items. // Should we have another struct for top-level use items rather than reusing this? visibility: Option, @@ -143,12 +144,84 @@ impl UseSegment { } } +pub fn merge_use_trees(use_trees: Vec) -> Vec { + let mut result = Vec::with_capacity(use_trees.len()); + for use_tree in use_trees { + if use_tree.has_comment() || use_tree.attrs.is_some() { + result.push(use_tree); + continue; + } + + for flattened in use_tree.flatten() { + merge_use_trees_inner(&mut result, flattened); + } + } + result +} + +fn merge_use_trees_inner(trees: &mut Vec, use_tree: UseTree) { + for tree in trees.iter_mut() { + if tree.share_prefix(&use_tree) { + tree.merge(use_tree); + return; + } + } + + trees.push(use_tree); +} + +impl fmt::Debug for UseTree { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self) + } +} + +impl fmt::Debug for UseSegment { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self) + } +} + +impl fmt::Display for UseSegment { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match *self { + UseSegment::Glob => write!(f, "*"), + UseSegment::Ident(ref s, _) => write!(f, "{}", s), + UseSegment::Slf(..) => write!(f, "self"), + UseSegment::Super(..) => write!(f, "super"), + UseSegment::List(ref list) => { + write!(f, "{{")?; + for (i, item) in list.iter().enumerate() { + let is_last = i == list.len() - 1; + write!(f, "{}", item)?; + if !is_last { + write!(f, ", ")?; + } + } + write!(f, "}}") + } + } + } +} +impl fmt::Display for UseTree { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + for (i, segment) in self.path.iter().enumerate() { + let is_last = i == self.path.len() - 1; + write!(f, "{}", segment)?; + if !is_last { + write!(f, "::")?; + } + } + write!(f, "") + } +} + impl UseTree { // Rewrite use tree with `use ` and a trailing `;`. pub fn rewrite_top_level(&self, context: &RewriteContext, shape: Shape) -> Option { let mut result = String::with_capacity(256); if let Some(ref attrs) = self.attrs { - result.push_str(&attrs.rewrite(context, shape)?); + result.push_str(&attrs.rewrite(context, shape).expect("rewrite attr")); if !result.is_empty() { result.push_str(&shape.indent.to_string_with_newline(context.config)); } @@ -168,6 +241,17 @@ impl UseTree { Some(result) } + // FIXME: Use correct span? + fn from_path(path: Vec, span: Span) -> UseTree { + UseTree { + path, + span, + list_item: None, + visibility: None, + attrs: None, + } + } + pub fn from_ast_with_normalization( context: &RewriteContext, item: &ast::Item, @@ -360,6 +444,131 @@ impl UseTree { self.path.push(last); self } + + fn has_comment(&self) -> bool { + self.list_item.as_ref().map_or(false, |list_item| { + list_item.pre_comment.is_some() || list_item.post_comment.is_some() + }) + } + + fn same_visibility(&self, other: &UseTree) -> bool { + match (&self.visibility, &other.visibility) { + ( + Some(codemap::Spanned { + node: ast::VisibilityKind::Inherited, + .. + }), + None, + ) + | ( + None, + Some(codemap::Spanned { + node: ast::VisibilityKind::Inherited, + .. + }), + ) + | (None, None) => true, + ( + Some(codemap::Spanned { node: lnode, .. }), + Some(codemap::Spanned { node: rnode, .. }), + ) => lnode == rnode, + _ => false, + } + } + + fn share_prefix(&self, other: &UseTree) -> bool { + if self.path.is_empty() || other.path.is_empty() || self.attrs.is_some() + || !self.same_visibility(other) + { + false + } else { + self.path[0] == other.path[0] + } + } + + fn flatten(self) -> Vec { + if self.path.is_empty() { + return vec![self]; + } + match self.path.clone().last().unwrap() { + UseSegment::List(list) => { + let prefix = &self.path[..self.path.len() - 1]; + let mut result = vec![]; + for nested_use_tree in list.into_iter() { + for mut flattend in nested_use_tree.clone().flatten().iter_mut() { + let mut new_path = prefix.to_vec(); + new_path.append(&mut flattend.path); + result.push(UseTree { + path: new_path, + span: self.span, + list_item: None, + visibility: self.visibility.clone(), + attrs: None, + }); + } + } + + result + } + _ => vec![self], + } + } + + fn merge(&mut self, other: UseTree) { + let mut new_path = vec![]; + let mut len = 0; + for (i, (mut a, b)) in self.path + .clone() + .iter_mut() + .zip(other.path.clone().into_iter()) + .enumerate() + { + if *a == b { + len = i + 1; + new_path.push(b); + continue; + } else { + len = i; + break; + } + } + if let Some(merged) = merge_rest(&self.path, &other.path, len) { + new_path.push(merged); + self.span = self.span.to(other.span); + } + self.path = new_path; + } +} + +fn merge_rest(a: &[UseSegment], b: &[UseSegment], len: usize) -> Option { + let a_rest = &a[len..]; + let b_rest = &b[len..]; + if a_rest.is_empty() && b_rest.is_empty() { + return None; + } + if a_rest.is_empty() { + return Some(UseSegment::List(vec![ + UseTree::from_path(vec![UseSegment::Slf(None)], DUMMY_SP), + UseTree::from_path(b_rest.to_vec(), DUMMY_SP), + ])); + } + if b_rest.is_empty() { + return Some(UseSegment::List(vec![ + UseTree::from_path(vec![UseSegment::Slf(None)], DUMMY_SP), + UseTree::from_path(a_rest.to_vec(), DUMMY_SP), + ])); + } + if let UseSegment::List(mut list) = a_rest[0].clone() { + merge_use_trees_inner(&mut list, UseTree::from_path(b_rest.to_vec(), DUMMY_SP)); + list.sort(); + return Some(UseSegment::List(list.clone())); + } + let mut list = vec![ + UseTree::from_path(a_rest.to_vec(), DUMMY_SP), + UseTree::from_path(b_rest.to_vec(), DUMMY_SP), + ]; + list.sort(); + Some(UseSegment::List(list)) } impl PartialOrd for UseSegment { @@ -461,9 +670,12 @@ fn rewrite_nested_use_tree( IndentStyle::Visual => shape.visual_indent(0), }; for use_tree in use_tree_list { - let mut list_item = use_tree.list_item.clone()?; - list_item.item = use_tree.rewrite(context, nested_shape); - list_items.push(list_item); + if let Some(mut list_item) = use_tree.list_item.clone() { + list_item.item = use_tree.rewrite(context, nested_shape); + list_items.push(list_item); + } else { + list_items.push(ListItem::from_str(use_tree.rewrite(context, nested_shape)?)); + } } let (tactic, remaining_width) = if use_tree_list.iter().any(|use_segment| { use_segment @@ -683,6 +895,60 @@ mod test { parser.parse_in_list() } + macro parse_use_trees($($s:expr),* $(,)*) { + vec![ + $(parse_use_tree($s),)* + ] + } + + #[test] + fn test_use_tree_merge() { + macro test_merge([$($input:expr),* $(,)*], [$($output:expr),* $(,)*]) { + assert_eq!( + merge_use_trees(parse_use_trees!($($input,)*)), + parse_use_trees!($($output,)*), + ); + } + + test_merge!(["a::b::{c, d}", "a::b::{e, f}"], ["a::b::{c, d, e, f}"]); + test_merge!(["a::b::c", "a::b"], ["a::b::{self, c}"]); + test_merge!(["a::b", "a::b"], ["a::b"]); + test_merge!(["a", "a::b", "a::b::c"], ["a::{self, b::{self, c}}"]); + test_merge!( + ["a::{b::{self, c}, d::e}", "a::d::f"], + ["a::{b::{self, c}, d::{e, f}}"] + ); + test_merge!( + ["a::d::f", "a::{b::{self, c}, d::e}"], + ["a::{b::{self, c}, d::{e, f}}"] + ); + test_merge!( + ["a::{c, d, b}", "a::{d, e, b, a, f}", "a::{f, g, c}"], + ["a::{a, b, c, d, e, f, g}"] + ); + } + + #[test] + fn test_use_tree_flatten() { + assert_eq!( + parse_use_tree("a::b::{c, d, e, f}").flatten(), + parse_use_trees!("a::b::c", "a::b::d", "a::b::e", "a::b::f",) + ); + + assert_eq!( + parse_use_tree("a::b::{c::{d, e, f}, g, h::{i, j, k}}").flatten(), + parse_use_trees![ + "a::b::c::d", + "a::b::c::e", + "a::b::c::f", + "a::b::g", + "a::b::h::i", + "a::b::h::j", + "a::b::h::k", + ] + ); + } + #[test] fn test_use_tree_normalize() { assert_eq!( diff --git a/src/lists.rs b/src/lists.rs index c4bd21869fb..266cd1b472b 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -80,6 +80,16 @@ pub struct ListItem { } impl ListItem { + pub fn empty() -> ListItem { + ListItem { + pre_comment: None, + pre_comment_style: ListItemCommentStyle::None, + item: None, + post_comment: None, + new_lines: false, + } + } + pub fn inner_as_ref(&self) -> &str { self.item.as_ref().map_or("", |s| s) } diff --git a/src/reorder.rs b/src/reorder.rs index 15191995d02..099c25cfd9b 100644 --- a/src/reorder.rs +++ b/src/reorder.rs @@ -22,7 +22,7 @@ use syntax::{ast, attr, codemap::Span}; use attr::filter_inline_attrs; use codemap::LineRangeUtils; use comment::combine_strs_with_missing_comments; -use imports::UseTree; +use imports::{merge_use_trees, UseTree}; use items::{is_mod_decl, rewrite_extern_crate, rewrite_mod}; use lists::{itemize_list, write_list, ListFormatting, ListItem}; use rewrite::{Rewrite, RewriteContext}; @@ -117,29 +117,41 @@ fn rewrite_reorderable_items( match reorderable_items[0].node { // FIXME: Remove duplicated code. ast::ItemKind::Use(..) => { - let normalized_items: Vec<_> = reorderable_items + let mut normalized_items: Vec<_> = reorderable_items .iter() .filter_map(|item| UseTree::from_ast_with_normalization(context, item)) .collect(); - - // 4 = "use ", 1 = ";" - let nested_shape = shape.offset_left(4)?.sub_width(1)?; + let cloned = normalized_items.clone(); + // Add comments before merging. let list_items = itemize_list( context.snippet_provider, - normalized_items.iter(), + cloned.iter(), "", ";", |item| item.span.lo(), |item| item.span.hi(), - |item| item.rewrite_top_level(context, nested_shape), + |_item| Some("".to_owned()), span.lo(), span.hi(), false, ); + for (item, list_item) in normalized_items.iter_mut().zip(list_items) { + item.list_item = Some(list_item.clone()); + } + if context.config.merge_imports() { + normalized_items = merge_use_trees(normalized_items); + } + normalized_items.sort(); - let mut item_pair_vec: Vec<_> = list_items.zip(&normalized_items).collect(); - item_pair_vec.sort_by(|a, b| a.1.cmp(b.1)); - let item_vec: Vec<_> = item_pair_vec.into_iter().map(|pair| pair.0).collect(); + // 4 = "use ", 1 = ";" + let nested_shape = shape.offset_left(4)?.sub_width(1)?; + let item_vec: Vec<_> = normalized_items + .into_iter() + .map(|use_tree| ListItem { + item: use_tree.rewrite_top_level(context, nested_shape), + ..use_tree.list_item.unwrap_or_else(|| ListItem::empty()) + }) + .collect(); wrap_reorderable_items(context, &item_vec, nested_shape) } From 4a7e45ec28fcbee14cd143402c1129d30cef39f0 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Fri, 6 Apr 2018 23:03:11 +0900 Subject: [PATCH 4/5] Simplify UseTree::has_comment --- src/imports.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/imports.rs b/src/imports.rs index 76fa7816f0d..03df4aed6af 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -446,9 +446,7 @@ impl UseTree { } fn has_comment(&self) -> bool { - self.list_item.as_ref().map_or(false, |list_item| { - list_item.pre_comment.is_some() || list_item.post_comment.is_some() - }) + self.list_item.as_ref().map_or(false, ListItem::has_comment) } fn same_visibility(&self, other: &UseTree) -> bool { @@ -526,7 +524,6 @@ impl UseTree { if *a == b { len = i + 1; new_path.push(b); - continue; } else { len = i; break; From 8820a59bd55d51ab4d30d7f215eb1993cae5aeaa Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Tue, 10 Apr 2018 12:36:41 +0900 Subject: [PATCH 5/5] Resolve review comments --- src/imports.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/imports.rs b/src/imports.rs index 03df4aed6af..c889ece4d07 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -172,13 +172,13 @@ fn merge_use_trees_inner(trees: &mut Vec, use_tree: UseTree) { impl fmt::Debug for UseTree { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self) + fmt::Display::fmt(self, f) } } impl fmt::Debug for UseSegment { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self) + fmt::Display::fmt(self, f) } } @@ -221,7 +221,7 @@ impl UseTree { pub fn rewrite_top_level(&self, context: &RewriteContext, shape: Shape) -> Option { let mut result = String::with_capacity(256); if let Some(ref attrs) = self.attrs { - result.push_str(&attrs.rewrite(context, shape).expect("rewrite attr")); + result.push_str(&attrs.rewrite(context, shape)?); if !result.is_empty() { result.push_str(&shape.indent.to_string_with_newline(context.config)); } @@ -242,6 +242,10 @@ impl UseTree { } // FIXME: Use correct span? + // The given span is essentially incorrect, since we are reconstructing + // use statements. This should not be a problem, though, since we have + // already tried to extract comment and observed that there are no comment + // around the given use item, and the span will not be used afterward. fn from_path(path: Vec, span: Span) -> UseTree { UseTree { path, @@ -514,22 +518,18 @@ impl UseTree { fn merge(&mut self, other: UseTree) { let mut new_path = vec![]; - let mut len = 0; - for (i, (mut a, b)) in self.path + for (mut a, b) in self.path .clone() .iter_mut() .zip(other.path.clone().into_iter()) - .enumerate() { if *a == b { - len = i + 1; new_path.push(b); } else { - len = i; break; } } - if let Some(merged) = merge_rest(&self.path, &other.path, len) { + if let Some(merged) = merge_rest(&self.path, &other.path, new_path.len()) { new_path.push(merged); self.span = self.span.to(other.span); }