From af404b998c87bb28ffb0a36a2f779a7d8371a0d1 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 6 Sep 2017 18:44:33 +0900 Subject: [PATCH 1/6] Factor out rewrite_extern_crate() --- src/visitor.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index 1fe8655309f3..2de2f6865df1 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -367,17 +367,8 @@ impl<'a> FmtVisitor<'a> { } } ast::ItemKind::ExternCrate(_) => { - self.format_missing_with_indent(source!(self, item.span).lo()); - let new_str = self.snippet(item.span); - if contains_comment(&new_str) { - self.buffer.push_str(&new_str) - } else { - let no_whitespace = - &new_str.split_whitespace().collect::>().join(" "); - self.buffer - .push_str(&Regex::new(r"\s;").unwrap().replace(no_whitespace, ";")); - } - self.last_pos = source!(self, item.span).hi(); + let rw = rewrite_extern_crate(&self.get_context(), item); + self.push_rewrite(item.span, rw); } ast::ItemKind::Struct(ref def, ref generics) => { let rewrite = { @@ -1046,3 +1037,15 @@ fn get_derive_args(context: &RewriteContext, attr: &ast::Attribute) -> Option None, }) } + +// Rewrite `extern crate foo;` WITHOUT attributes. +pub fn rewrite_extern_crate(context: &RewriteContext, item: &ast::Item) -> Option { + assert!(is_extern_crate(item)); + let new_str = context.snippet(item.span); + Some(if contains_comment(&new_str) { + new_str + } else { + let no_whitespace = &new_str.split_whitespace().collect::>().join(" "); + String::from(&*Regex::new(r"\s;").unwrap().replace(no_whitespace, ";")) + }) +} From 662ee46e67dd04fae3e624d246b0bd7537273752 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 6 Sep 2017 18:47:50 +0900 Subject: [PATCH 2/6] Use write_list() to format imports --- src/imports.rs | 192 +++++++++++++++++++++++++++---------------------- src/visitor.rs | 4 +- 2 files changed, 107 insertions(+), 89 deletions(-) diff --git a/src/imports.rs b/src/imports.rs index 33efce19a40a..7cfe5d8b6b98 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -8,20 +8,21 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use std::cmp::{self, Ordering}; +use std::cmp::Ordering; use syntax::{ast, ptr}; use syntax::codemap::{BytePos, Span}; -use Shape; +use {Shape, Spanned}; use codemap::SpanUtils; +use comment::combine_strs_with_missing_comments; use config::IndentStyle; use lists::{definitive_tactic, itemize_list, write_list, DefinitiveListTactic, ListFormatting, ListItem, Separator, SeparatorPlace, SeparatorTactic}; use rewrite::{Rewrite, RewriteContext}; use types::{rewrite_path, PathContext}; -use utils; -use visitor::FmtVisitor; +use utils::{format_visibility, mk_sp}; +use visitor::{rewrite_extern_crate, FmtVisitor}; fn path_of(a: &ast::ViewPath_) -> &ast::Path { match *a { @@ -185,95 +186,115 @@ impl Rewrite for ast::ViewPath { } } +// Rewrite `use foo;` WITHOUT attributes. +fn rewrite_import( + context: &RewriteContext, + vis: &ast::Visibility, + vp: &ast::ViewPath, + attrs: &[ast::Attribute], + shape: Shape, +) -> Option { + let vis = format_visibility(vis); + // 4 = `use `, 1 = `;` + let rw = shape + .offset_left(vis.len() + 4) + .and_then(|shape| shape.sub_width(1)) + .and_then(|shape| match vp.node { + // If we have an empty path list with no attributes, we erase it + ast::ViewPath_::ViewPathList(_, ref path_list) + if path_list.is_empty() && attrs.is_empty() => + { + Some("".into()) + } + _ => vp.rewrite(context, shape), + }); + match rw { + Some(ref s) if !s.is_empty() => Some(format!("{}use {};", vis, s)), + _ => rw, + } +} + +fn rewrite_imports( + context: &RewriteContext, + use_items: &[ptr::P], + shape: Shape, + span: Span, +) -> Option { + let items = itemize_list( + context.codemap, + use_items.iter(), + "", + |item| item.span().lo(), + |item| item.span().hi(), + |item| { + let attrs_str = try_opt!(item.attrs.rewrite(context, shape)); + + let missed_span = if item.attrs.is_empty() { + mk_sp(item.span.lo(), item.span.lo()) + } else { + mk_sp(item.attrs.last().unwrap().span.hi(), item.span.lo()) + }; + + let item_str = match item.node { + ast::ItemKind::Use(ref vp) => { + try_opt!(rewrite_import(context, &item.vis, vp, &item.attrs, shape)) + } + ast::ItemKind::ExternCrate(..) => try_opt!(rewrite_extern_crate(context, item)), + _ => return None, + }; + + combine_strs_with_missing_comments( + context, + &attrs_str, + &item_str, + missed_span, + shape, + false, + ) + }, + span.lo(), + span.hi(), + false, + ); + let mut item_pair_vec: Vec<_> = items.zip(use_items.iter()).collect(); + item_pair_vec.sort_by(|a, b| compare_use_items(context, a.1, b.1).unwrap()); + let item_vec: Vec<_> = item_pair_vec.into_iter().map(|pair| pair.0).collect(); + + let fmt = ListFormatting { + tactic: DefinitiveListTactic::Vertical, + separator: "", + trailing_separator: SeparatorTactic::Never, + separator_place: SeparatorPlace::Back, + shape: shape, + ends_with_newline: true, + preserve_newline: false, + config: context.config, + }; + + write_list(&item_vec, &fmt) +} + impl<'a> FmtVisitor<'a> { pub fn format_imports(&mut self, use_items: &[ptr::P]) { - // Find the location immediately before the first use item in the run. This must not lie - // before the current `self.last_pos` - let pos_before_first_use_item = use_items - .first() - .map(|p_i| { - cmp::max( - self.last_pos, - p_i.attrs - .iter() - .map(|attr| attr.span.lo()) - .min() - .unwrap_or(p_i.span.lo()), - ) - }) - .unwrap_or(self.last_pos); - // Construct a list of pairs, each containing a `use` item and the start of span before - // that `use` item. - let mut last_pos_of_prev_use_item = pos_before_first_use_item; - let mut ordered_use_items = use_items - .iter() - .map(|p_i| { - let new_item = (&*p_i, last_pos_of_prev_use_item); - last_pos_of_prev_use_item = p_i.span.hi(); - new_item - }) - .collect::>(); - let pos_after_last_use_item = last_pos_of_prev_use_item; - // Order the imports by view-path & other import path properties - ordered_use_items.sort_by(|a, b| { - compare_use_items(&self.get_context(), a.0, b.0).unwrap() - }); - // First, output the span before the first import - let prev_span_str = self.snippet(utils::mk_sp(self.last_pos, pos_before_first_use_item)); - // Look for purely trailing space at the start of the prefix snippet before a linefeed, or - // a prefix that's entirely horizontal whitespace. - let prefix_span_start = match prev_span_str.find('\n') { - Some(offset) if prev_span_str[..offset].trim().is_empty() => { - self.last_pos + BytePos(offset as u32) - } - None if prev_span_str.trim().is_empty() => pos_before_first_use_item, - _ => self.last_pos, - }; - // Look for indent (the line part preceding the use is all whitespace) and excise that - // from the prefix - let span_end = match prev_span_str.rfind('\n') { - Some(offset) if prev_span_str[offset..].trim().is_empty() => { - self.last_pos + BytePos(offset as u32) - } - _ => pos_before_first_use_item, - }; - - self.last_pos = prefix_span_start; - self.format_missing(span_end); - for ordered in ordered_use_items { - // Fake out the formatter by setting `self.last_pos` to the appropriate location before - // each item before visiting it. - self.last_pos = ordered.1; - self.visit_item(ordered.0); + if use_items.is_empty() { + return; } - self.last_pos = pos_after_last_use_item; + + let lo = use_items.first().unwrap().span().lo(); + let hi = use_items.last().unwrap().span().hi(); + let span = mk_sp(lo, hi); + let rw = rewrite_imports(&self.get_context(), use_items, self.shape(), span); + self.push_rewrite(span, rw); } - pub fn format_import( - &mut self, - vis: &ast::Visibility, - vp: &ast::ViewPath, - span: Span, - attrs: &[ast::Attribute], - ) { - let vis = utils::format_visibility(vis); - // 4 = `use `, 1 = `;` - let rw = self.shape() - .offset_left(vis.len() + 4) - .and_then(|shape| shape.sub_width(1)) - .and_then(|shape| match vp.node { - // If we have an empty path list with no attributes, we erase it - ast::ViewPath_::ViewPathList(_, ref path_list) - if path_list.is_empty() && attrs.is_empty() => - { - Some("".into()) - } - _ => vp.rewrite(&self.get_context(), shape), - }); + pub fn format_import(&mut self, item: &ast::Item, vp: &ast::ViewPath) { + let span = item.span; + let shape = self.shape(); + let rw = rewrite_import(&self.get_context(), &item.vis, vp, &item.attrs, shape); match rw { Some(ref s) if s.is_empty() => { // Format up to last newline - let prev_span = utils::mk_sp(self.last_pos, source!(self, span).lo()); + let prev_span = mk_sp(self.last_pos, source!(self, span).lo()); let span_end = match self.snippet(prev_span).rfind('\n') { Some(offset) => self.last_pos + BytePos(offset as u32), None => source!(self, span).lo(), @@ -282,7 +303,6 @@ impl<'a> FmtVisitor<'a> { self.last_pos = source!(self, span).hi(); } Some(ref s) => { - let s = format!("{}use {};", vis, s); self.format_missing_with_indent(source!(self, span).lo()); self.buffer.push_str(&s); self.last_pos = source!(self, span).hi(); diff --git a/src/visitor.rs b/src/visitor.rs index 2de2f6865df1..76f6e4914e20 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -342,9 +342,7 @@ impl<'a> FmtVisitor<'a> { } match item.node { - ast::ItemKind::Use(ref vp) => { - self.format_import(&item.vis, vp, item.span, &item.attrs); - } + ast::ItemKind::Use(ref vp) => self.format_import(&item, vp), ast::ItemKind::Impl(..) => { self.format_missing_with_indent(source!(self, item.span).lo()); let snippet = self.snippet(item.span); From 0d748cf4c87d0892f79ac8c1ae23e7b6be24da99 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 6 Sep 2017 18:48:09 +0900 Subject: [PATCH 3/6] Update tests --- tests/target/configs-reorder_imports_in_group-false.rs | 1 - tests/target/issue-1124.rs | 4 ---- 2 files changed, 5 deletions(-) diff --git a/tests/target/configs-reorder_imports_in_group-false.rs b/tests/target/configs-reorder_imports_in_group-false.rs index 42778d91dd86..29460da50aaf 100644 --- a/tests/target/configs-reorder_imports_in_group-false.rs +++ b/tests/target/configs-reorder_imports_in_group-false.rs @@ -5,7 +5,6 @@ use dolor; /// This comment should stay with `use ipsum;` use ipsum; - use lorem; use sit; use std::io; diff --git a/tests/target/issue-1124.rs b/tests/target/issue-1124.rs index d30f29461d8c..7f38fe75a8f9 100644 --- a/tests/target/issue-1124.rs +++ b/tests/target/issue-1124.rs @@ -12,10 +12,6 @@ mod a { } use a; - - - use x; - use y; use z; From 4bd03737570c6f07e92b1bd1116bc3e56c571216 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 6 Sep 2017 19:24:14 +0900 Subject: [PATCH 4/6] No cloning --- src/visitor.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index 76f6e4914e20..296c254ea87b 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -300,7 +300,8 @@ impl<'a> FmtVisitor<'a> { // complex in the module case. It is complex because the module could be // in a separate file and there might be attributes in both files, but // the AST lumps them all together. - let mut attrs = item.attrs.clone(); + let filterd_attrs; + let mut attrs = &item.attrs; match item.node { ast::ItemKind::Mod(ref m) => { let outer_file = self.codemap.lookup_char_pos(item.span.lo()).file; @@ -318,7 +319,7 @@ impl<'a> FmtVisitor<'a> { } else { // Module is not inline and should not be skipped. We want // to process only the attributes in the current file. - let filterd_attrs = item.attrs + filterd_attrs = item.attrs .iter() .filter_map(|a| { let attr_file = self.codemap.lookup_char_pos(a.span.lo()).file; @@ -332,7 +333,7 @@ impl<'a> FmtVisitor<'a> { // Assert because if we should skip it should be caught by // the above case. assert!(!self.visit_attrs(&filterd_attrs, ast::AttrStyle::Outer)); - attrs = filterd_attrs; + attrs = &filterd_attrs; } } _ => if self.visit_attrs(&item.attrs, ast::AttrStyle::Outer) { From 4dbda0662964e02e02c2b1891cd1332966fef658 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 6 Sep 2017 19:24:23 +0900 Subject: [PATCH 5/6] Use push_rewrite() to remove duplicates --- src/visitor.rs | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index 296c254ea87b..f4288d844623 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -345,25 +345,16 @@ impl<'a> FmtVisitor<'a> { match item.node { ast::ItemKind::Use(ref vp) => self.format_import(&item, vp), ast::ItemKind::Impl(..) => { - self.format_missing_with_indent(source!(self, item.span).lo()); let snippet = self.snippet(item.span); let where_span_end = snippet .find_uncommented("{") .map(|x| (BytePos(x as u32)) + source!(self, item.span).lo()); - if let Some(impl_str) = - format_impl(&self.get_context(), item, self.block_indent, where_span_end) - { - self.buffer.push_str(&impl_str); - self.last_pos = source!(self, item.span).hi(); - } + let rw = format_impl(&self.get_context(), item, self.block_indent, where_span_end); + self.push_rewrite(item.span, rw); } ast::ItemKind::Trait(..) => { - self.format_missing_with_indent(item.span.lo()); - if let Some(trait_str) = format_trait(&self.get_context(), item, self.block_indent) - { - self.buffer.push_str(&trait_str); - self.last_pos = source!(self, item.span).hi(); - } + let rw = format_trait(&self.get_context(), item, self.block_indent); + self.push_rewrite(item.span, rw); } ast::ItemKind::ExternCrate(_) => { let rw = rewrite_extern_crate(&self.get_context(), item); @@ -371,17 +362,15 @@ impl<'a> FmtVisitor<'a> { } ast::ItemKind::Struct(ref def, ref generics) => { let rewrite = { - let indent = self.block_indent; - let context = self.get_context(); ::items::format_struct( - &context, + &self.get_context(), "struct ", item.ident, &item.vis, def, Some(generics), item.span, - indent, + self.block_indent, None, ).map(|s| match *def { ast::VariantData::Tuple(..) => s + ";", From 903d815228d6d3a0afdce19defe5beafe9b039da Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 6 Sep 2017 19:30:51 +0900 Subject: [PATCH 6/6] Clean up --- src/visitor.rs | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index f4288d844623..aff768f450c0 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -22,8 +22,9 @@ use comment::{contains_comment, recover_missing_comment_in_span, CodeCharKind, C FindUncommented}; use comment::rewrite_comment; use config::{BraceStyle, Config}; -use items::{format_impl, format_trait, rewrite_associated_impl_type, rewrite_associated_type, - rewrite_static, rewrite_type_alias}; +use items::{format_impl, format_struct, format_struct_struct, format_trait, + rewrite_associated_impl_type, rewrite_associated_type, rewrite_static, + rewrite_type_alias}; use lists::{itemize_list, write_list, DefinitiveListTactic, ListFormatting, SeparatorPlace, SeparatorTactic}; use macros::{rewrite_macro, MacroPosition}; @@ -361,22 +362,20 @@ impl<'a> FmtVisitor<'a> { self.push_rewrite(item.span, rw); } ast::ItemKind::Struct(ref def, ref generics) => { - let rewrite = { - ::items::format_struct( - &self.get_context(), - "struct ", - item.ident, - &item.vis, - def, - Some(generics), - item.span, - self.block_indent, - None, - ).map(|s| match *def { - ast::VariantData::Tuple(..) => s + ";", - _ => s, - }) - }; + let rewrite = format_struct( + &self.get_context(), + "struct ", + item.ident, + &item.vis, + def, + Some(generics), + item.span, + self.block_indent, + None, + ).map(|s| match *def { + ast::VariantData::Tuple(..) => s + ";", + _ => s, + }); self.push_rewrite(item.span, rewrite); } ast::ItemKind::Enum(ref def, ref generics) => { @@ -457,7 +456,7 @@ impl<'a> FmtVisitor<'a> { self.push_rewrite(item.span, rewrite); } ast::ItemKind::Union(ref def, ref generics) => { - let rewrite = ::items::format_struct_struct( + let rewrite = format_struct_struct( &self.get_context(), "union ", item.ident,