From c2c7fcc52e2adf31d384440d97d03012f5f61fea Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Mon, 14 Dec 2020 13:04:36 +0200 Subject: [PATCH 1/8] Fix for issue #3943 - rordering of imports --- src/formatting/imports.rs | 133 +++++++++++++++++++++++++++++++++++-- tests/source/issue-3943.rs | 17 +++++ tests/target/issue-3943.rs | 17 +++++ 3 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 tests/source/issue-3943.rs create mode 100644 tests/target/issue-3943.rs diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index 72aff70987a..9d690ae4ac9 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -118,6 +118,7 @@ impl Eq for UseTree {} impl Spanned for UseTree { fn span(&self) -> Span { + debug!("** [DBO] Spanned for UseTree: span: enter ;\n"); let lo = if let Some(ref attrs) = self.attrs { attrs.iter().next().map_or(self.span.lo(), |a| a.span.lo()) } else { @@ -127,6 +128,15 @@ impl Spanned for UseTree { } } +/* >>>>>>> [DBO] ADD */ +macro_rules! to_use_segment_ident { + ($seg:ident, $modsep:ident) => {{ + let mod_sep = if $modsep { "::" } else { "" }; + UseSegment::Ident(format!("{}{}", mod_sep, $seg), None) + }}; +} +/* <<<<<<< [DBO] ADD */ + impl UseSegment { // Clone a version of self with any top-level alias removed. fn remove_alias(&self) -> UseSegment { @@ -153,11 +163,21 @@ impl UseSegment { "super" => UseSegment::Super(None), "crate" => UseSegment::Crate(None), _ => { + /* >>>>> [DBO] REPLACE ******** let mod_sep = if modsep { "::" } else { "" }; UseSegment::Ident(format!("{}{}", mod_sep, name), None) + ********** [DBO] REPLACE *********/ + to_use_segment_ident!(name, modsep) + /* <<<<<<< [DBO] REPLACE */ } }) } + + /* >>>>>>> [DBO] ADD */ + fn from_use_segment(use_seg: &UseSegment, modsep: bool) -> UseSegment { + to_use_segment_ident!(use_seg, modsep) + } + /* <<<<<<< [DBO] ADD */ } pub(crate) fn merge_use_trees(use_trees: Vec) -> Vec { @@ -419,6 +439,10 @@ impl UseTree { // Do the adjustments that rustfmt does elsewhere to use paths. pub(crate) fn normalize(mut self) -> UseTree { + debug!( + "** [DBO] UseTree: normalize: enter self={:?}, self.path={:?}\n", + self, self.path + ); let mut last = self.path.pop().expect("Empty use tree?"); // Hack around borrow checker. let mut normalize_sole_list = false; @@ -429,10 +453,12 @@ impl UseTree { _ if self.attrs.is_some() => {} UseSegment::List(ref list) if list.is_empty() => { self.path = vec![]; + debug!("** [DBO] UseTree: normalize: retE1={:?}\n", self); return self; } UseSegment::Slf(None) if self.path.is_empty() && self.visibility.is_some() => { self.path = vec![]; + debug!("** [DBO] UseTree: normalize: retE2={:?}\n", self); return self; } _ => {} @@ -441,6 +467,7 @@ impl UseTree { // Normalise foo::self -> foo. if let UseSegment::Slf(None) = last { if !self.path.is_empty() { + debug!("** [DBO] UseTree: normalize: retE3={:?}\n", self); return self; } } @@ -467,6 +494,7 @@ impl UseTree { } if done { + debug!("** [DBO] UseTree: normalize: retE4={:?}\n", self); return self; } @@ -480,9 +508,30 @@ impl UseTree { if normalize_sole_list { match last { UseSegment::List(list) => { + debug!("** [DBO] UseTree: normalize: list={:?}\n", list); for seg in &list[0].path { + debug!( + "** [DBO] UseTree: normalize: seg={:?}, self.path[0]={:?};\n", + seg, self.path[0] + ); + + /****** >>>>>>> [DBO] REPLACE next ******** self.path.push(seg.clone()); + *************************************************/ + if !self.path[0].to_string().is_empty() { + self.path.push(seg.clone()); + } else { + self.path = vec![]; + let useseg = UseSegment::from_use_segment(seg, true); + debug!("** [DBO] UseTree: normalize: useseg={:?}\n", useseg); + self.path.push(useseg); + } + /****** <<<<<<<<< [DBO] REPLACE next ********/ } + debug!( + "** [DBO] UseTree: normalize: retE5 before self.normalize() self={:?}, self.path={:?}\n", + self, self.path + ); return self.normalize(); } _ => unreachable!(), @@ -493,9 +542,14 @@ impl UseTree { if let UseSegment::List(list) = last { let mut list = list.into_iter().map(UseTree::normalize).collect::>(); list.sort(); + //debug!("** [DBO] UseTree: normalize: after sort list={:?}\n", list); last = UseSegment::List(list); } + debug!( + "** [DBO] UseTree: normalize: retEE (almost)={:?}, last={:?}\n", + self, last + ); self.path.push(last); self } @@ -539,6 +593,7 @@ impl UseTree { } fn flatten(self) -> Vec { + debug!("** [DBO] UseTree: flatten: enter self={:?};\n", self); match self.path.last() { Some(UseSegment::List(list)) => { if list.len() == 1 && list[0].path.len() == 1 { @@ -569,6 +624,7 @@ impl UseTree { } fn merge(&mut self, other: &UseTree) { + debug!("** [DBO] UseTree: merge: enter self={:?};\n", self); let mut prefix = 0; for (a, b) in self.path.iter().zip(other.path.iter()) { if *a == *b { @@ -585,6 +641,7 @@ impl UseTree { } fn merge_rest(a: &[UseSegment], b: &[UseSegment], mut len: usize) -> Option> { + debug!("** [DBO] merge_rest: enter ;\n"); if a.len() == len && b.len() == len { return None; } @@ -618,6 +675,7 @@ fn merge_rest(a: &[UseSegment], b: &[UseSegment], mut len: usize) -> Option, use_tree: UseTree) { + debug!("** [DBO] merge_use_trees_inner: enter ;\n"); let similar_trees = trees.iter_mut().filter(|tree| tree.share_prefix(&use_tree)); if use_tree.path.len() == 1 { if let Some(tree) = similar_trees.min_by_key(|tree| tree.path.len()) { @@ -637,16 +695,30 @@ fn merge_use_trees_inner(trees: &mut Vec, use_tree: UseTree) { impl PartialOrd for UseSegment { fn partial_cmp(&self, other: &UseSegment) -> Option { + debug!("** [DBO] PartialOrd for UseSegment: partial_cmp: enter ;\n"); Some(self.cmp(other)) } } impl PartialOrd for UseTree { fn partial_cmp(&self, other: &UseTree) -> Option { - Some(self.cmp(other)) + debug!( + "** [DBO] PartialOrd for UseTree: partial_cmp: enter self={:?}, other={:?};\n", + self, other + ); + let ret = Some(self.cmp(other)); + debug!( + "** [DBO] PartialOrd for UseTree: partial_cmp: retE={:?};\n", + ret + ); + ret } } impl Ord for UseSegment { fn cmp(&self, other: &UseSegment) -> Ordering { + debug!( + "** [DBO] Ord for UseSegment: cmp: enter self={:?}, other={:?};\n", + self, other + ); use self::UseSegment::*; fn is_upper_snake_case(s: &str) -> bool { @@ -654,7 +726,7 @@ impl Ord for UseSegment { .all(|c| c.is_uppercase() || c == '_' || c.is_numeric()) } - match (self, other) { + let ret = match (self, other) { (&Slf(ref a), &Slf(ref b)) | (&Super(ref a), &Super(ref b)) | (&Crate(ref a), &Crate(ref b)) => compare_opt_ident_as_versions(&a, &b), @@ -662,17 +734,37 @@ impl Ord for UseSegment { (&Ident(ref pia, ref aa), &Ident(ref pib, ref ab)) => { let ia = pia.trim_start_matches("r#"); let ib = pib.trim_start_matches("r#"); + debug!( + "** [DBO] Ord for UseSegment: cmp: ia={:?}, ib={:?};\n", + ia, ib + ); // snake_case < CamelCase < UPPER_SNAKE_CASE + + /*********** >>>>>>> [DBO] ADD ******************** + if !ia.is_empty() && ib.is_empty() { + debug!("** [DBO] Ord for UseSegment: cmp: retE11=Greater;\n"); + return Ordering::Greater; + } + if ia.is_empty() && !ib.is_empty() { + debug!("** [DBO] Ord for UseSegment: cmp: retE12=Less;\n"); + return Ordering::Less; + } + ***************** <<<<<<< [DBO] ADD ***************/ + if ia.starts_with(char::is_uppercase) && ib.starts_with(char::is_lowercase) { + debug!("** [DBO] Ord for UseSegment: cmp: retE1=Greater;\n"); return Ordering::Greater; } if ia.starts_with(char::is_lowercase) && ib.starts_with(char::is_uppercase) { + debug!("** [DBO] Ord for UseSegment: cmp: retE2=Less;\n"); return Ordering::Less; } if is_upper_snake_case(ia) && !is_upper_snake_case(ib) { + debug!("** [DBO] Ord for UseSegment: cmp: retE3=Greater;\n"); return Ordering::Greater; } if !is_upper_snake_case(ia) && is_upper_snake_case(ib) { + debug!("** [DBO] Ord for UseSegment: cmp: retE4=Less;\n"); return Ordering::Less; } @@ -682,6 +774,7 @@ impl Ord for UseSegment { for (a, b) in a.iter().zip(b.iter()) { let ord = a.cmp(b); if ord != Ordering::Equal { + debug!("** [DBO] Ord for UseSegment: cmp: ord=retE5={:?};\n", ord); return ord; } } @@ -698,23 +791,50 @@ impl Ord for UseSegment { (_, &Ident(..)) => Ordering::Greater, (&Glob, _) => Ordering::Less, (_, &Glob) => Ordering::Greater, - } + }; + debug!("** [DBO] Ord for UseSegment: cmp: retEE={:?};\n", ret); + ret } } impl Ord for UseTree { fn cmp(&self, other: &UseTree) -> Ordering { + debug!( + "** [DBO] Ord for UseTree: cmp: enter self={:?}, other={:?}, self.path.iter={:?}, other.path.iter={:?};\n", + self, + other, + self.path.iter(), + other.path.iter(), + ); for (a, b) in self.path.iter().zip(other.path.iter()) { // The comparison without aliases is a hack to avoid situations like // comparing `a::b` to `a as c` - where the latter should be ordered // first since it is shorter. + debug!( + "** [DBO] Ord for UseTree: cmp: a={:?}, a.remove_alias={:?}, b={:?}, b.remove_alias={:?};\n", + a, + a.remove_alias(), + b, + b.remove_alias(), + ); + let ord = a.remove_alias().cmp(&b.remove_alias()); if ord != Ordering::Equal { + debug!("** [DBO] Ord for UseTree: cmp: ord=retE1={:?};\n", ord); return ord; } } - Ord::cmp(&self.path.len(), &other.path.len()) - .then(Ord::cmp(&self.path.last(), &other.path.last())) + debug!( + "** [DBO] Ord for UseTree: cmp: self.path.len={:?}, other.path.len={:?}, self.path.last={:?}, other.path.last()={:?};\n", + self.path.len(), + other.path.len(), + self.path.last(), + other.path.last(), + ); + let ret = Ord::cmp(&self.path.len(), &other.path.len()) + .then(Ord::cmp(&self.path.last(), &other.path.last())); + debug!("** [DBO] Ord for UseTree: cmp: retEE={:?};\n", ret); + ret } } @@ -723,6 +843,7 @@ fn rewrite_nested_use_tree( use_tree_list: &[UseTree], shape: Shape, ) -> Option { + debug!("** [DBO] rewrite_nested_use_tree: enter ;\n"); let mut list_items = Vec::with_capacity(use_tree_list.len()); let nested_shape = match context.config.imports_indent() { IndentStyle::Block => shape @@ -792,6 +913,7 @@ fn rewrite_nested_use_tree( impl Rewrite for UseSegment { fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option { + debug!("** [DBO] Rewrite for UseSegment: rewrite: enter ;\n"); Some(match self { UseSegment::Ident(ref ident, Some(ref rename)) => format!("{} as {}", ident, rename), UseSegment::Ident(ref ident, None) => ident.clone(), @@ -815,6 +937,7 @@ impl Rewrite for UseSegment { impl Rewrite for UseTree { // This does NOT format attributes and visibility or add a trailing `;`. fn rewrite(&self, context: &RewriteContext<'_>, mut shape: Shape) -> Option { + debug!("** [DBO] Rewrite for UseTree: rewrite: enter ;\n"); let mut result = String::with_capacity(256); let mut iter = self.path.iter().peekable(); while let Some(ref segment) = iter.next() { diff --git a/tests/source/issue-3943.rs b/tests/source/issue-3943.rs new file mode 100644 index 00000000000..01228f20d12 --- /dev/null +++ b/tests/source/issue-3943.rs @@ -0,0 +1,17 @@ +use ::foo; +use ::foo::{Bar1}; +use ::foo::{Bar2, Baz2}; +use ::{Foo}; +use ::{Bar3, Baz3}; + +use ::foo; +use ::foo::Bar; +use ::foo::{Bar, Baz}; +use ::Foo; +use ::{Bar, Baz}; + +use ::Foo; +use ::foo; +use ::foo::Bar; +use ::foo::{Bar, Baz}; +use ::{Bar, Baz}; diff --git a/tests/target/issue-3943.rs b/tests/target/issue-3943.rs new file mode 100644 index 00000000000..41051a93428 --- /dev/null +++ b/tests/target/issue-3943.rs @@ -0,0 +1,17 @@ +use ::Foo; +use ::foo; +use ::foo::Bar1; +use ::foo::{Bar2, Baz2}; +use ::{Bar3, Baz3}; + +use ::Foo; +use ::foo; +use ::foo::Bar; +use ::foo::{Bar, Baz}; +use ::{Bar, Baz}; + +use ::Foo; +use ::foo; +use ::foo::Bar; +use ::foo::{Bar, Baz}; +use ::{Bar, Baz}; From 273733f517556e7633b5d4e236e52fbfcd968bce Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 15 Dec 2020 11:26:14 +0200 Subject: [PATCH 2/8] change and debug info removal --- src/formatting/imports.rs | 122 +++----------------------------------- 1 file changed, 9 insertions(+), 113 deletions(-) diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index 9d690ae4ac9..d5131fdf6fc 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -118,7 +118,6 @@ impl Eq for UseTree {} impl Spanned for UseTree { fn span(&self) -> Span { - debug!("** [DBO] Spanned for UseTree: span: enter ;\n"); let lo = if let Some(ref attrs) = self.attrs { attrs.iter().next().map_or(self.span.lo(), |a| a.span.lo()) } else { @@ -128,14 +127,12 @@ impl Spanned for UseTree { } } -/* >>>>>>> [DBO] ADD */ macro_rules! to_use_segment_ident { ($seg:ident, $modsep:ident) => {{ let mod_sep = if $modsep { "::" } else { "" }; UseSegment::Ident(format!("{}{}", mod_sep, $seg), None) }}; } -/* <<<<<<< [DBO] ADD */ impl UseSegment { // Clone a version of self with any top-level alias removed. @@ -163,21 +160,14 @@ impl UseSegment { "super" => UseSegment::Super(None), "crate" => UseSegment::Crate(None), _ => { - /* >>>>> [DBO] REPLACE ******** - let mod_sep = if modsep { "::" } else { "" }; - UseSegment::Ident(format!("{}{}", mod_sep, name), None) - ********** [DBO] REPLACE *********/ to_use_segment_ident!(name, modsep) - /* <<<<<<< [DBO] REPLACE */ } }) } - /* >>>>>>> [DBO] ADD */ fn from_use_segment(use_seg: &UseSegment, modsep: bool) -> UseSegment { to_use_segment_ident!(use_seg, modsep) } - /* <<<<<<< [DBO] ADD */ } pub(crate) fn merge_use_trees(use_trees: Vec) -> Vec { @@ -439,10 +429,6 @@ impl UseTree { // Do the adjustments that rustfmt does elsewhere to use paths. pub(crate) fn normalize(mut self) -> UseTree { - debug!( - "** [DBO] UseTree: normalize: enter self={:?}, self.path={:?}\n", - self, self.path - ); let mut last = self.path.pop().expect("Empty use tree?"); // Hack around borrow checker. let mut normalize_sole_list = false; @@ -453,12 +439,10 @@ impl UseTree { _ if self.attrs.is_some() => {} UseSegment::List(ref list) if list.is_empty() => { self.path = vec![]; - debug!("** [DBO] UseTree: normalize: retE1={:?}\n", self); return self; } UseSegment::Slf(None) if self.path.is_empty() && self.visibility.is_some() => { self.path = vec![]; - debug!("** [DBO] UseTree: normalize: retE2={:?}\n", self); return self; } _ => {} @@ -467,7 +451,6 @@ impl UseTree { // Normalise foo::self -> foo. if let UseSegment::Slf(None) = last { if !self.path.is_empty() { - debug!("** [DBO] UseTree: normalize: retE3={:?}\n", self); return self; } } @@ -494,7 +477,6 @@ impl UseTree { } if done { - debug!("** [DBO] UseTree: normalize: retE4={:?}\n", self); return self; } @@ -508,30 +490,18 @@ impl UseTree { if normalize_sole_list { match last { UseSegment::List(list) => { - debug!("** [DBO] UseTree: normalize: list={:?}\n", list); for seg in &list[0].path { - debug!( - "** [DBO] UseTree: normalize: seg={:?}, self.path[0]={:?};\n", - seg, self.path[0] - ); - - /****** >>>>>>> [DBO] REPLACE next ******** - self.path.push(seg.clone()); - *************************************************/ - if !self.path[0].to_string().is_empty() { + if self.path.len() == 0 || !self.path[0].to_string().is_empty() { self.path.push(seg.clone()); } else { + // In case of single item list, when the `{...}` are removed, + // path vector should include single item, like [::Foo], + // and not two items like [, Foo] self.path = vec![]; let useseg = UseSegment::from_use_segment(seg, true); - debug!("** [DBO] UseTree: normalize: useseg={:?}\n", useseg); self.path.push(useseg); } - /****** <<<<<<<<< [DBO] REPLACE next ********/ } - debug!( - "** [DBO] UseTree: normalize: retE5 before self.normalize() self={:?}, self.path={:?}\n", - self, self.path - ); return self.normalize(); } _ => unreachable!(), @@ -542,14 +512,9 @@ impl UseTree { if let UseSegment::List(list) = last { let mut list = list.into_iter().map(UseTree::normalize).collect::>(); list.sort(); - //debug!("** [DBO] UseTree: normalize: after sort list={:?}\n", list); last = UseSegment::List(list); } - debug!( - "** [DBO] UseTree: normalize: retEE (almost)={:?}, last={:?}\n", - self, last - ); self.path.push(last); self } @@ -593,7 +558,6 @@ impl UseTree { } fn flatten(self) -> Vec { - debug!("** [DBO] UseTree: flatten: enter self={:?};\n", self); match self.path.last() { Some(UseSegment::List(list)) => { if list.len() == 1 && list[0].path.len() == 1 { @@ -624,7 +588,6 @@ impl UseTree { } fn merge(&mut self, other: &UseTree) { - debug!("** [DBO] UseTree: merge: enter self={:?};\n", self); let mut prefix = 0; for (a, b) in self.path.iter().zip(other.path.iter()) { if *a == *b { @@ -641,7 +604,6 @@ impl UseTree { } fn merge_rest(a: &[UseSegment], b: &[UseSegment], mut len: usize) -> Option> { - debug!("** [DBO] merge_rest: enter ;\n"); if a.len() == len && b.len() == len { return None; } @@ -675,7 +637,6 @@ fn merge_rest(a: &[UseSegment], b: &[UseSegment], mut len: usize) -> Option, use_tree: UseTree) { - debug!("** [DBO] merge_use_trees_inner: enter ;\n"); let similar_trees = trees.iter_mut().filter(|tree| tree.share_prefix(&use_tree)); if use_tree.path.len() == 1 { if let Some(tree) = similar_trees.min_by_key(|tree| tree.path.len()) { @@ -695,30 +656,16 @@ fn merge_use_trees_inner(trees: &mut Vec, use_tree: UseTree) { impl PartialOrd for UseSegment { fn partial_cmp(&self, other: &UseSegment) -> Option { - debug!("** [DBO] PartialOrd for UseSegment: partial_cmp: enter ;\n"); Some(self.cmp(other)) } } impl PartialOrd for UseTree { fn partial_cmp(&self, other: &UseTree) -> Option { - debug!( - "** [DBO] PartialOrd for UseTree: partial_cmp: enter self={:?}, other={:?};\n", - self, other - ); - let ret = Some(self.cmp(other)); - debug!( - "** [DBO] PartialOrd for UseTree: partial_cmp: retE={:?};\n", - ret - ); - ret + Some(self.cmp(other)) } } impl Ord for UseSegment { fn cmp(&self, other: &UseSegment) -> Ordering { - debug!( - "** [DBO] Ord for UseSegment: cmp: enter self={:?}, other={:?};\n", - self, other - ); use self::UseSegment::*; fn is_upper_snake_case(s: &str) -> bool { @@ -726,7 +673,7 @@ impl Ord for UseSegment { .all(|c| c.is_uppercase() || c == '_' || c.is_numeric()) } - let ret = match (self, other) { + match (self, other) { (&Slf(ref a), &Slf(ref b)) | (&Super(ref a), &Super(ref b)) | (&Crate(ref a), &Crate(ref b)) => compare_opt_ident_as_versions(&a, &b), @@ -734,37 +681,17 @@ impl Ord for UseSegment { (&Ident(ref pia, ref aa), &Ident(ref pib, ref ab)) => { let ia = pia.trim_start_matches("r#"); let ib = pib.trim_start_matches("r#"); - debug!( - "** [DBO] Ord for UseSegment: cmp: ia={:?}, ib={:?};\n", - ia, ib - ); // snake_case < CamelCase < UPPER_SNAKE_CASE - - /*********** >>>>>>> [DBO] ADD ******************** - if !ia.is_empty() && ib.is_empty() { - debug!("** [DBO] Ord for UseSegment: cmp: retE11=Greater;\n"); - return Ordering::Greater; - } - if ia.is_empty() && !ib.is_empty() { - debug!("** [DBO] Ord for UseSegment: cmp: retE12=Less;\n"); - return Ordering::Less; - } - ***************** <<<<<<< [DBO] ADD ***************/ - if ia.starts_with(char::is_uppercase) && ib.starts_with(char::is_lowercase) { - debug!("** [DBO] Ord for UseSegment: cmp: retE1=Greater;\n"); return Ordering::Greater; } if ia.starts_with(char::is_lowercase) && ib.starts_with(char::is_uppercase) { - debug!("** [DBO] Ord for UseSegment: cmp: retE2=Less;\n"); return Ordering::Less; } if is_upper_snake_case(ia) && !is_upper_snake_case(ib) { - debug!("** [DBO] Ord for UseSegment: cmp: retE3=Greater;\n"); return Ordering::Greater; } if !is_upper_snake_case(ia) && is_upper_snake_case(ib) { - debug!("** [DBO] Ord for UseSegment: cmp: retE4=Less;\n"); return Ordering::Less; } @@ -774,7 +701,6 @@ impl Ord for UseSegment { for (a, b) in a.iter().zip(b.iter()) { let ord = a.cmp(b); if ord != Ordering::Equal { - debug!("** [DBO] Ord for UseSegment: cmp: ord=retE5={:?};\n", ord); return ord; } } @@ -791,50 +717,23 @@ impl Ord for UseSegment { (_, &Ident(..)) => Ordering::Greater, (&Glob, _) => Ordering::Less, (_, &Glob) => Ordering::Greater, - }; - debug!("** [DBO] Ord for UseSegment: cmp: retEE={:?};\n", ret); - ret + } } } impl Ord for UseTree { fn cmp(&self, other: &UseTree) -> Ordering { - debug!( - "** [DBO] Ord for UseTree: cmp: enter self={:?}, other={:?}, self.path.iter={:?}, other.path.iter={:?};\n", - self, - other, - self.path.iter(), - other.path.iter(), - ); for (a, b) in self.path.iter().zip(other.path.iter()) { // The comparison without aliases is a hack to avoid situations like // comparing `a::b` to `a as c` - where the latter should be ordered // first since it is shorter. - debug!( - "** [DBO] Ord for UseTree: cmp: a={:?}, a.remove_alias={:?}, b={:?}, b.remove_alias={:?};\n", - a, - a.remove_alias(), - b, - b.remove_alias(), - ); - let ord = a.remove_alias().cmp(&b.remove_alias()); if ord != Ordering::Equal { - debug!("** [DBO] Ord for UseTree: cmp: ord=retE1={:?};\n", ord); return ord; } } - debug!( - "** [DBO] Ord for UseTree: cmp: self.path.len={:?}, other.path.len={:?}, self.path.last={:?}, other.path.last()={:?};\n", - self.path.len(), - other.path.len(), - self.path.last(), - other.path.last(), - ); - let ret = Ord::cmp(&self.path.len(), &other.path.len()) - .then(Ord::cmp(&self.path.last(), &other.path.last())); - debug!("** [DBO] Ord for UseTree: cmp: retEE={:?};\n", ret); - ret + Ord::cmp(&self.path.len(), &other.path.len()) + .then(Ord::cmp(&self.path.last(), &other.path.last())) } } @@ -843,7 +742,6 @@ fn rewrite_nested_use_tree( use_tree_list: &[UseTree], shape: Shape, ) -> Option { - debug!("** [DBO] rewrite_nested_use_tree: enter ;\n"); let mut list_items = Vec::with_capacity(use_tree_list.len()); let nested_shape = match context.config.imports_indent() { IndentStyle::Block => shape @@ -913,7 +811,6 @@ fn rewrite_nested_use_tree( impl Rewrite for UseSegment { fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option { - debug!("** [DBO] Rewrite for UseSegment: rewrite: enter ;\n"); Some(match self { UseSegment::Ident(ref ident, Some(ref rename)) => format!("{} as {}", ident, rename), UseSegment::Ident(ref ident, None) => ident.clone(), @@ -937,7 +834,6 @@ impl Rewrite for UseSegment { impl Rewrite for UseTree { // This does NOT format attributes and visibility or add a trailing `;`. fn rewrite(&self, context: &RewriteContext<'_>, mut shape: Shape) -> Option { - debug!("** [DBO] Rewrite for UseTree: rewrite: enter ;\n"); let mut result = String::with_capacity(256); let mut iter = self.path.iter().peekable(); while let Some(ref segment) = iter.next() { From 0a0a5771f4023c92f31f42a9a6215ed74cf41626 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 22 Dec 2020 22:17:50 +0200 Subject: [PATCH 3/8] New fix approach with debug information --- src/formatting/imports.rs | 129 +++++++++++++++++++++++++++---------- tests/source/issue-3943.rs | 14 ++++ tests/target/issue-3943.rs | 14 ++++ 3 files changed, 122 insertions(+), 35 deletions(-) diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index d5131fdf6fc..b7f4b9aee67 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -127,13 +127,6 @@ impl Spanned for UseTree { } } -macro_rules! to_use_segment_ident { - ($seg:ident, $modsep:ident) => {{ - let mod_sep = if $modsep { "::" } else { "" }; - UseSegment::Ident(format!("{}{}", mod_sep, $seg), None) - }}; -} - impl UseSegment { // Clone a version of self with any top-level alias removed. fn remove_alias(&self) -> UseSegment { @@ -160,14 +153,11 @@ impl UseSegment { "super" => UseSegment::Super(None), "crate" => UseSegment::Crate(None), _ => { - to_use_segment_ident!(name, modsep) + let mod_sep = if modsep { "::" } else { "" }; + UseSegment::Ident(format!("{}{}", mod_sep, name), None) } }) } - - fn from_use_segment(use_seg: &UseSegment, modsep: bool) -> UseSegment { - to_use_segment_ident!(use_seg, modsep) - } } pub(crate) fn merge_use_trees(use_trees: Vec) -> Vec { @@ -329,6 +319,11 @@ impl UseTree { opt_lo: Option, attrs: Option>, ) -> UseTree { + debug!( + "** [DBO] from_ast: enter a={:?}, list_item={:?}, opt_lo={:?}, attrs={:?};", + a, list_item, opt_lo, attrs + ); + let span = if let Some(lo) = opt_lo { mk_sp(lo, a.span.hi()) } else { @@ -346,6 +341,10 @@ impl UseTree { context.config.edition() == Edition::Edition2018 && a.prefix.is_global(); let mut modsep = leading_modsep; + debug!( + "** [DBO] from_ast: leading_modsep={:?}, init modsep={:?}, span={:?};", + leading_modsep, modsep, span + ); for p in &a.prefix.segments { if let Some(use_segment) = UseSegment::from_path_segment(context, p, modsep) { @@ -353,6 +352,7 @@ impl UseTree { modsep = false; } } + debug!("** [DBO] from_ast: modified modsep={:?};", modsep); match a.kind { UseTreeKind::Glob => { @@ -378,19 +378,75 @@ impl UseTree { false, ) .collect(); - // in case of a global path and the nested list starts at the root, - // e.g., "::{foo, bar}" - if a.prefix.segments.len() == 1 && leading_modsep { - result.path.push(UseSegment::Ident("".to_owned(), None)); - } - result.path.push(UseSegment::List( - list.iter() - .zip(items.into_iter()) - .map(|(t, list_item)| { - Self::from_ast(context, &t.0, Some(list_item), None, None, None) - }) - .collect(), - )); + debug!( + "** [DBO] from_ast: Nested: len={:?}, list={:?}, items={:?}, result={:?};", + list.len(), + list, + items, + result, + ); + + /* >>>>>> [DBO] ADD */ + // find whether a case of a global path and the nested list starts at the root + // with one item, e.g., "::{foo}", and does not include comments or "as". + let first_item = if a.prefix.segments.len() == 1 + && list.len() == 1 + && result.to_string().is_empty() + { + let first = &list[0].0; + debug!( + "** [DBO] from_ast: Nested: first.span={:?}, item_span={:?};", + first.span, + context.snippet(mk_sp(first.span.lo(), span.hi() - BytePos(1))), + ); + match first.kind { + UseTreeKind::Simple(..) => { + // "-1" for the "}" + let snippet = context + .snippet(mk_sp(first.span.lo(), span.hi() - BytePos(1))) + .trim(); + // Ensure that indent includes only the name and not + // "as" clause, comments, etc. + if snippet.eq(&format!("{}", first.prefix.segments[0].ident)) { + Some(first) + } else { + None + } + } + _ => None, + } + } else { + None + }; + debug!( + "** [DBO] from_ast: Nested: modsep={:?}, first_item={:?};", + modsep, first_item + ); + if let Some(first) = first_item { + // in case of a global path and the nested list starts at the root + // with one item, e.g., "::{foo}" + let tree = Self::from_ast(context, first, None, None, None, None); + let mod_sep = if leading_modsep { "::" } else { "" }; + let seg = UseSegment::Ident(format!("{}{}", mod_sep, tree), None); + debug!("** [DBO] from_ast: Nested: tree={:?}, seg={:?};", tree, seg); + result.path.pop(); + result.path.push(seg); + } else { + /******* <<<<<<<<< [DBO] ADD */ + // in case of a global path and the nested list starts at the root, + // e.g., "::{foo, bar}" + if a.prefix.segments.len() == 1 && leading_modsep { + result.path.push(UseSegment::Ident("".to_owned(), None)); + } + result.path.push(UseSegment::List( + list.iter() + .zip(items.into_iter()) + .map(|(t, list_item)| { + Self::from_ast(context, &t.0, Some(list_item), None, None, None) + }) + .collect(), + )); + }; // >>>>> [DBO] ADD <<<<<< */ } UseTreeKind::Simple(ref rename, ..) => { // If the path has leading double colons and is composed of only 2 segments, then we @@ -412,18 +468,30 @@ impl UseTree { Some(rewrite_ident(context, ident).to_owned()) } }); + debug!( + "** [DBO] from_ast: Simple: seg.len={:?}, rename={:?}, name={:?}, alias={:?};", + a.prefix.segments.len(), + rename, + name, + alias, + ); let segment = match name.as_ref() { "self" => UseSegment::Slf(alias), "super" => UseSegment::Super(alias), "crate" => UseSegment::Crate(alias), _ => UseSegment::Ident(name, alias), }; + debug!( + "** [DBO] from_ast: UseTreeKind::Simple: segment={:?}, result={:?};", + segment, result + ); // `name` is already in result. result.path.pop(); result.path.push(segment); } } + debug!("** [DBO] from_ast: resltEE={:?};", result); result } @@ -491,16 +559,7 @@ impl UseTree { match last { UseSegment::List(list) => { for seg in &list[0].path { - if self.path.len() == 0 || !self.path[0].to_string().is_empty() { - self.path.push(seg.clone()); - } else { - // In case of single item list, when the `{...}` are removed, - // path vector should include single item, like [::Foo], - // and not two items like [, Foo] - self.path = vec![]; - let useseg = UseSegment::from_use_segment(seg, true); - self.path.push(useseg); - } + self.path.push(seg.clone()); } return self.normalize(); } diff --git a/tests/source/issue-3943.rs b/tests/source/issue-3943.rs index 01228f20d12..99169f05813 100644 --- a/tests/source/issue-3943.rs +++ b/tests/source/issue-3943.rs @@ -1,3 +1,4 @@ +// Tests for original #3943 issue use ::foo; use ::foo::{Bar1}; use ::foo::{Bar2, Baz2}; @@ -15,3 +16,16 @@ use ::foo; use ::foo::Bar; use ::foo::{Bar, Baz}; use ::{Bar, Baz}; + +// Additional tests for signle item `{}` handling +use ::AAAA; +use bbbbb::AAAA; +use ::{BBBB}; +use aaaa::{BBBB}; +use crate::detect::{Feature, cache}; +use super::{auxvec}; + +// Tests with comments and "as" +use a::{/* pre-comment */ item}; +use a::{ item /* post-comment */}; +use a::{/* pre-comment */ item /* post-comment */ }; diff --git a/tests/target/issue-3943.rs b/tests/target/issue-3943.rs index 41051a93428..87958a8a22f 100644 --- a/tests/target/issue-3943.rs +++ b/tests/target/issue-3943.rs @@ -1,3 +1,4 @@ +// Tests for original #3943 issue use ::Foo; use ::foo; use ::foo::Bar1; @@ -15,3 +16,16 @@ use ::foo; use ::foo::Bar; use ::foo::{Bar, Baz}; use ::{Bar, Baz}; + +// Additional tests for signle item `{}` handling +use super::auxvec; +use crate::detect::{cache, Feature}; +use ::AAAA; +use ::BBBB; +use aaaa::BBBB; +use bbbbb::AAAA; + +// Tests with comments and "as" +use a::{/* pre-comment */ item}; +use a::{item /* post-comment */}; +use a::{/* pre-comment */ item /* post-comment */}; From d5bd26f458c824fdf800920d17284be850b89de8 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 22 Dec 2020 22:26:31 +0200 Subject: [PATCH 4/8] Remove debug info --- src/formatting/imports.rs | 45 +++------------------------------------ 1 file changed, 3 insertions(+), 42 deletions(-) diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index b7f4b9aee67..d50dc8ced6c 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -319,11 +319,6 @@ impl UseTree { opt_lo: Option, attrs: Option>, ) -> UseTree { - debug!( - "** [DBO] from_ast: enter a={:?}, list_item={:?}, opt_lo={:?}, attrs={:?};", - a, list_item, opt_lo, attrs - ); - let span = if let Some(lo) = opt_lo { mk_sp(lo, a.span.hi()) } else { @@ -341,10 +336,6 @@ impl UseTree { context.config.edition() == Edition::Edition2018 && a.prefix.is_global(); let mut modsep = leading_modsep; - debug!( - "** [DBO] from_ast: leading_modsep={:?}, init modsep={:?}, span={:?};", - leading_modsep, modsep, span - ); for p in &a.prefix.segments { if let Some(use_segment) = UseSegment::from_path_segment(context, p, modsep) { @@ -352,7 +343,6 @@ impl UseTree { modsep = false; } } - debug!("** [DBO] from_ast: modified modsep={:?};", modsep); match a.kind { UseTreeKind::Glob => { @@ -378,15 +368,7 @@ impl UseTree { false, ) .collect(); - debug!( - "** [DBO] from_ast: Nested: len={:?}, list={:?}, items={:?}, result={:?};", - list.len(), - list, - items, - result, - ); - /* >>>>>> [DBO] ADD */ // find whether a case of a global path and the nested list starts at the root // with one item, e.g., "::{foo}", and does not include comments or "as". let first_item = if a.prefix.segments.len() == 1 @@ -394,11 +376,6 @@ impl UseTree { && result.to_string().is_empty() { let first = &list[0].0; - debug!( - "** [DBO] from_ast: Nested: first.span={:?}, item_span={:?};", - first.span, - context.snippet(mk_sp(first.span.lo(), span.hi() - BytePos(1))), - ); match first.kind { UseTreeKind::Simple(..) => { // "-1" for the "}" @@ -418,21 +395,16 @@ impl UseTree { } else { None }; - debug!( - "** [DBO] from_ast: Nested: modsep={:?}, first_item={:?};", - modsep, first_item - ); + if let Some(first) = first_item { // in case of a global path and the nested list starts at the root // with one item, e.g., "::{foo}" let tree = Self::from_ast(context, first, None, None, None, None); let mod_sep = if leading_modsep { "::" } else { "" }; let seg = UseSegment::Ident(format!("{}{}", mod_sep, tree), None); - debug!("** [DBO] from_ast: Nested: tree={:?}, seg={:?};", tree, seg); result.path.pop(); result.path.push(seg); } else { - /******* <<<<<<<<< [DBO] ADD */ // in case of a global path and the nested list starts at the root, // e.g., "::{foo, bar}" if a.prefix.segments.len() == 1 && leading_modsep { @@ -446,7 +418,7 @@ impl UseTree { }) .collect(), )); - }; // >>>>> [DBO] ADD <<<<<< */ + }; } UseTreeKind::Simple(ref rename, ..) => { // If the path has leading double colons and is composed of only 2 segments, then we @@ -468,30 +440,19 @@ impl UseTree { Some(rewrite_ident(context, ident).to_owned()) } }); - debug!( - "** [DBO] from_ast: Simple: seg.len={:?}, rename={:?}, name={:?}, alias={:?};", - a.prefix.segments.len(), - rename, - name, - alias, - ); + let segment = match name.as_ref() { "self" => UseSegment::Slf(alias), "super" => UseSegment::Super(alias), "crate" => UseSegment::Crate(alias), _ => UseSegment::Ident(name, alias), }; - debug!( - "** [DBO] from_ast: UseTreeKind::Simple: segment={:?}, result={:?};", - segment, result - ); // `name` is already in result. result.path.pop(); result.path.push(segment); } } - debug!("** [DBO] from_ast: resltEE={:?};", result); result } From 9e200dc1e4ba2b87c0562cd977d0e6c2087ea482 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 5 Jan 2021 16:38:04 +0200 Subject: [PATCH 5/8] Add UseSegment:Empty with some refactoring and debug info --- src/formatting/imports.rs | 188 ++++++++++++++++++++++++++++++++++--- src/formatting/reorder.rs | 4 +- tests/source/issue-3943.rs | 13 +++ tests/target/issue-3943.rs | 19 +++- 4 files changed, 207 insertions(+), 17 deletions(-) diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index d50dc8ced6c..7ab2a9eaed4 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -25,6 +25,11 @@ use crate::formatting::{ /// Returns a name imported by a `use` declaration. /// E.g., returns `Ordering` for `std::cmp::Ordering` and `self` for `std::cmp::self`. pub(crate) fn path_to_imported_ident(path: &ast::Path) -> symbol::Ident { + debug!( + "** [DBO] path_to_imported_ident: enter last segment={:?}, path={:?};", + path.segments.last(), + path, + ); path.segments.last().unwrap().ident } @@ -94,6 +99,7 @@ pub(crate) enum UseSegment { Super(Option), Crate(Option), Glob, + Empty, // >>>> [DBO] ADD <<<<<<< List(Vec), } @@ -145,16 +151,32 @@ impl UseSegment { modsep: bool, ) -> Option { let name = rewrite_ident(context, path_seg.ident); + debug!( + "** [DBO]: from_path_segment: enter name={:?}, modsep={:?};", + name, modsep + ); if name.is_empty() || name == "{{root}}" { - return None; + /* >>>>> [DBO] REPLACE next */ + //return None; + debug!("** [DBO]: from_path_segment: replacing {{root}} empty sengment on None;"); + if modsep { + return Some(UseSegment::Empty); + } else { + return None; + } + /* <<<<< [DBO] REPLACE next */ } Some(match name { "self" => UseSegment::Slf(None), "super" => UseSegment::Super(None), "crate" => UseSegment::Crate(None), _ => { - let mod_sep = if modsep { "::" } else { "" }; - UseSegment::Ident(format!("{}{}", mod_sep, name), None) + //let mod_sep = if modsep { "::" } else { "" }; // >>>> [DBO] DELETE <<<< + /* >>>>> [DBO] REPLACE next */ + //UseSegment::Ident(format!("{}{}", mod_sep, name), None) + // ????????!!!!!!! ADD here and empty segment ??????? + UseSegment::Ident(name.to_string(), None) + /* <<<<<< [DBO] REPLACE next */ } }) } @@ -195,7 +217,15 @@ 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::Empty => write!(f, ""), // >>>>> [DBO] ADD <<<<<< + //UseSegment::Ident(ref s, _) => write!(f, "{}", s), >>>>> [DBO] without debug <<<<< + UseSegment::Ident(ref s, ref s1) => { + debug!( + "** [DBO] fmt::Display for UseSegment: Ident s={:?}, s1={:?};", + s, s1 + ); + write!(f, "{}", s) + } UseSegment::Slf(..) => write!(f, "self"), UseSegment::Super(..) => write!(f, "super"), UseSegment::Crate(..) => write!(f, "crate"), @@ -319,6 +349,8 @@ impl UseTree { opt_lo: Option, attrs: Option>, ) -> UseTree { + debug!("** [DBO] from_ast: enter a={:?};", a); + let span = if let Some(lo) = opt_lo { mk_sp(lo, a.span.hi()) } else { @@ -344,13 +376,50 @@ impl UseTree { } } + /* >>>>>>>> [DBO] ADD */ + let segment_from_simple = |path: &ast::Path, rename: &Option| { + let name = rewrite_ident(context, path_to_imported_ident(&path)).to_owned(); + let alias = rename.and_then(|ident| { + if ident.name == sym::underscore_imports { + // for impl-only-use + Some("_".to_owned()) + } else if ident == path_to_imported_ident(&path) { + None + } else { + Some(rewrite_ident(context, ident).to_owned()) + } + }); + debug!( + "** [DBO] from_ast: segment_from_simple: name={:?}, alias={:?}, rename={:?};", + name, alias, rename + ); + match name.as_ref() { + "self" => UseSegment::Slf(alias), + "super" => UseSegment::Super(alias), + "crate" => UseSegment::Crate(alias), + _ => UseSegment::Ident(name, alias), + } + }; + /* <<<<<<< [DBO] ADD */ + + debug!( + "** [DBO] from_ast: before match result.path={:?};", + result.path + ); + match a.kind { UseTreeKind::Glob => { + /* >>>>>>>> ????!!!! [DBO] DELETE ********** // in case of a global path and the glob starts at the root, e.g., "::*" if a.prefix.segments.len() == 1 && leading_modsep { - result.path.push(UseSegment::Ident("".to_owned(), None)); + /* >>>>>> [DBO] REPLACE next <<<<< */ + //result.path.push(UseSegment::Ident("".to_owned(), None)); + result.path.push(UseSegment::Empty); + debug!("** [DBO] from_ast: Glob result.path1={:?};", result.path); } + ******** <<<<<<<<<<<< [DBO] DELETE */ result.path.push(UseSegment::Glob); + debug!("** [DBO] from_ast: Glob result.pathE={:?};", result.path); } UseTreeKind::Nested(ref list) => { // Extract comments between nested use items. @@ -368,9 +437,17 @@ impl UseTree { false, ) .collect(); + debug!( + "** [DBO] from_ast: Nested: len={:?}, list={:?}, items={:?}, result={:?};", + list.len(), + list, + items, + result, + ); // find whether a case of a global path and the nested list starts at the root // with one item, e.g., "::{foo}", and does not include comments or "as". + /******** >>>>>>> REPLACE PREVIOUS CHANGE ************************** let first_item = if a.prefix.segments.len() == 1 && list.len() == 1 && result.to_string().is_empty() @@ -400,16 +477,53 @@ impl UseTree { // in case of a global path and the nested list starts at the root // with one item, e.g., "::{foo}" let tree = Self::from_ast(context, first, None, None, None, None); - let mod_sep = if leading_modsep { "::" } else { "" }; - let seg = UseSegment::Ident(format!("{}{}", mod_sep, tree), None); + //let mod_sep = if leading_modsep { "::" } else { "" }; //>>>> [DBO] DEL <<<< + /* >>>>>> [DBO] REPLACE next <<<<< */ + //let seg = UseSegment::Ident(format!("{}{}", mod_sep, tree), None); + let seg = UseSegment::Ident(format!("{}", tree), None); result.path.pop(); + /* >>>>> [DBO] ADD */ + if leading_modsep { + result.path.push(UseSegment::Empty); + } + /* <<<<<< [DBO] ADD */ result.path.push(seg); + ***************** REPLACE PREVIOUS CHANGE **************************/ + let mut path = None; + let mut rename = None; + if a.prefix.segments.len() == 1 && list.len() == 1 && result.to_string().is_empty() + { + let first = &list[0].0; + match first.kind { + UseTreeKind::Simple(ref ren, ..) => { + // "-1" for the "}" + let snippet = context + .snippet(mk_sp(first.span.lo(), span.hi() - BytePos(1))) + .trim(); + // Ensure that indent includes only the name and not + // "as" clause, comments, etc. + if snippet.eq(&format!("{}", first.prefix.segments[0].ident)) { + path = Some(&first.prefix); + rename = Some(ren); + } + } + _ => {} + } + }; + + if let (Some(path), Some(rename)) = (path, rename) { + result.path.push(segment_from_simple(path, rename)); + /******** <<<<<<<< REPLACE PREVIOUS CHANGE **************************/ } else { + /******* >>>>>>>> [DBO] DELETE *********** // in case of a global path and the nested list starts at the root, // e.g., "::{foo, bar}" if a.prefix.segments.len() == 1 && leading_modsep { - result.path.push(UseSegment::Ident("".to_owned(), None)); + /* >>>>>> [DBO] REPLACE next <<<<< */ + //result.path.push(UseSegment::Ident("".to_owned(), None)); + result.path.push(UseSegment::Empty); } + ********* <<<<<<< [DBO] DELETE *********/ result.path.push(UseSegment::List( list.iter() .zip(items.into_iter()) @@ -419,17 +533,27 @@ impl UseTree { .collect(), )); }; + debug!("** [DBO] from_ast: Nested result.path={:?};", result.path); } UseTreeKind::Simple(ref rename, ..) => { // If the path has leading double colons and is composed of only 2 segments, then we // bypass the call to path_to_imported_ident which would get only the ident and // lose the path root, e.g., `that` in `::that`. // The span of `a.prefix` contains the leading colons. + /******* >>>>>>>> [DBO] REAPLCE *********** let name = if a.prefix.segments.len() == 2 && leading_modsep { + debug!( + "** [DBO] from_ast: snippet(a.prefix.span)={:?}, a.prefix.segments={:?}", + context.snippet(a.prefix.span), a.prefix.segments + ); context.snippet(a.prefix.span).to_owned() } else { rewrite_ident(context, path_to_imported_ident(&a.prefix)).to_owned() }; + *************** [DBO] REPLACE ***********/ + /********* >>>>>> [DBO] MOVE into closure simple_to_segment ********** + let name = rewrite_ident(context, path_to_imported_ident(&a.prefix)).to_owned(); + /* <<<<<<<< [DBO] REPLACE *********/ let alias = rename.and_then(|ident| { if ident.name == sym::underscore_imports { // for impl-only-use @@ -441,18 +565,23 @@ impl UseTree { } }); + /* >>>>>> [DBO] REPLACE ********************/ let segment = match name.as_ref() { "self" => UseSegment::Slf(alias), "super" => UseSegment::Super(alias), "crate" => UseSegment::Crate(alias), _ => UseSegment::Ident(name, alias), }; - + ************* [DBO] MOVE into closure simple_to_segment **********/ + let segment = segment_from_simple(&a.prefix, rename); // `name` is already in result. result.path.pop(); result.path.push(segment); + debug!("** [DBO] from_ast: Simple result.path={:?};", result.path); + /********* >>>>>> [DBO] MOVE into closure simple_to_segment **********/ } } + debug!("** [DBO] from_ast: resltEE.path={:?};", result.path); result } @@ -693,7 +822,7 @@ impl Ord for UseSegment { .all(|c| c.is_uppercase() || c == '_' || c.is_numeric()) } - match (self, other) { + let ret = match (self, other) { (&Slf(ref a), &Slf(ref b)) | (&Super(ref a), &Super(ref b)) | (&Crate(ref a), &Crate(ref b)) => compare_opt_ident_as_versions(&a, &b), @@ -701,6 +830,7 @@ impl Ord for UseSegment { (&Ident(ref pia, ref aa), &Ident(ref pib, ref ab)) => { let ia = pia.trim_start_matches("r#"); let ib = pib.trim_start_matches("r#"); + debug!("** [DBO] cmp for UseSegment: ia={:?}, ib={:?};", ia, ib); // snake_case < CamelCase < UPPER_SNAKE_CASE if ia.starts_with(char::is_uppercase) && ib.starts_with(char::is_lowercase) { return Ordering::Greater; @@ -733,27 +863,58 @@ impl Ord for UseSegment { (_, &Super(_)) => Ordering::Greater, (&Crate(_), _) => Ordering::Less, (_, &Crate(_)) => Ordering::Greater, + /* >>>>>>> [DBO] ADD */ + (&Empty, &Empty) => Ordering::Equal, + (&Empty, _) => Ordering::Less, + (_, &Empty) => Ordering::Greater, + /* <<<<<<< [DBO] ADD */ (&Ident(..), _) => Ordering::Less, (_, &Ident(..)) => Ordering::Greater, (&Glob, _) => Ordering::Less, (_, &Glob) => Ordering::Greater, - } + }; + debug!( + "** [DBO] cmp for UseSegment: retEE={:?}, self={:?}, other={:?};", + ret, self, other + ); + ret } } impl Ord for UseTree { fn cmp(&self, other: &UseTree) -> Ordering { + debug!( + "** [DBO] cmp for UseTree: enter self={:?}, other={:?};", + self, other + ); for (a, b) in self.path.iter().zip(other.path.iter()) { // The comparison without aliases is a hack to avoid situations like // comparing `a::b` to `a as c` - where the latter should be ordered // first since it is shorter. + //??????!!!!!!! let ord = a.remove_alias().cmp(&b.remove_alias()); + debug!( + "** [DBO] cmp for UseTree: next loop ord={:?}, self={:?}, other={:?};", + ord, a, b + ); if ord != Ordering::Equal { return ord; } } - Ord::cmp(&self.path.len(), &other.path.len()) - .then(Ord::cmp(&self.path.last(), &other.path.last())) + /* >>>>>> [DBO] REPLACE next */ + //Ord::cmp(&self.path.len(), &other.path.len()) + // .then(Ord::cmp(&self.path.last(), &other.path.last())) + //????!!!!!!! + let ret = Ord::cmp(&self.path.len(), &other.path.len()) + .then(Ord::cmp(&self.path.last(), &other.path.last())); + debug!( + "** [DBO] cmp for UseTree: end cmp ord={:?}, self.len={:?}, other.len={:?};", + ret, + self.path.len(), + other.path.len(), + ); + ret + /* <<<<<<< [DBO] REPLACE next */ } } @@ -841,6 +1002,7 @@ impl Rewrite for UseSegment { UseSegment::Crate(Some(ref rename)) => format!("crate as {}", rename), UseSegment::Crate(None) => "crate".to_owned(), UseSegment::Glob => "*".to_owned(), + UseSegment::Empty => "".to_owned(), // >>>>>> [DBO] ADD <<<<<<<< UseSegment::List(ref use_tree_list) => rewrite_nested_use_tree( context, use_tree_list, diff --git a/src/formatting/reorder.rs b/src/formatting/reorder.rs index 379aa109b14..5ae80b52a09 100644 --- a/src/formatting/reorder.rs +++ b/src/formatting/reorder.rs @@ -307,7 +307,9 @@ fn group_imports(uts: Vec) -> Vec> { local_imports.push(ut) } // These are probably illegal here - UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), + /* >>>>>> [DBO] REPLACE next <<<<<< */ + //UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), + UseSegment::Empty | UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), } } diff --git a/tests/source/issue-3943.rs b/tests/source/issue-3943.rs index 99169f05813..3918b75be6b 100644 --- a/tests/source/issue-3943.rs +++ b/tests/source/issue-3943.rs @@ -29,3 +29,16 @@ use super::{auxvec}; use a::{/* pre-comment */ item}; use a::{ item /* post-comment */}; use a::{/* pre-comment */ item /* post-comment */ }; + +// Misc +use ::{Foo}; +use ::foo; +use ::{Foo1}; +use std; +use dummy; +use Super::foo; +use ::*; +use ::foo::{Foo, foo}; +use self::std::fs as self_fs; +use ::foo as bar; +use ::{Foo as baz}; diff --git a/tests/target/issue-3943.rs b/tests/target/issue-3943.rs index 87958a8a22f..e46fafd910a 100644 --- a/tests/target/issue-3943.rs +++ b/tests/target/issue-3943.rs @@ -1,20 +1,20 @@ // Tests for original #3943 issue -use ::Foo; use ::foo; use ::foo::Bar1; use ::foo::{Bar2, Baz2}; +use ::Foo; use ::{Bar3, Baz3}; -use ::Foo; use ::foo; use ::foo::Bar; use ::foo::{Bar, Baz}; +use ::Foo; use ::{Bar, Baz}; -use ::Foo; use ::foo; use ::foo::Bar; use ::foo::{Bar, Baz}; +use ::Foo; use ::{Bar, Baz}; // Additional tests for signle item `{}` handling @@ -29,3 +29,16 @@ use bbbbb::AAAA; use a::{/* pre-comment */ item}; use a::{item /* post-comment */}; use a::{/* pre-comment */ item /* post-comment */}; + +// Misc +use self::std::fs as self_fs; +use ::foo; +use ::foo as bar; +use ::foo::{foo, Foo}; +use ::Foo; +use ::Foo as baz; +use ::Foo1; +use ::*; +use dummy; +use std; +use Super::foo; From 2d52e7107d7a2265f27290e8478db666abd61e3d Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 5 Jan 2021 16:53:25 +0200 Subject: [PATCH 6/8] Remove debug info --- src/formatting/imports.rs | 190 ++------------------------------------ src/formatting/reorder.rs | 2 - 2 files changed, 9 insertions(+), 183 deletions(-) diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index 7ab2a9eaed4..8a25db30dff 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -25,11 +25,6 @@ use crate::formatting::{ /// Returns a name imported by a `use` declaration. /// E.g., returns `Ordering` for `std::cmp::Ordering` and `self` for `std::cmp::self`. pub(crate) fn path_to_imported_ident(path: &ast::Path) -> symbol::Ident { - debug!( - "** [DBO] path_to_imported_ident: enter last segment={:?}, path={:?};", - path.segments.last(), - path, - ); path.segments.last().unwrap().ident } @@ -99,7 +94,7 @@ pub(crate) enum UseSegment { Super(Option), Crate(Option), Glob, - Empty, // >>>> [DBO] ADD <<<<<<< + Empty, List(Vec), } @@ -151,33 +146,19 @@ impl UseSegment { modsep: bool, ) -> Option { let name = rewrite_ident(context, path_seg.ident); - debug!( - "** [DBO]: from_path_segment: enter name={:?}, modsep={:?};", - name, modsep - ); if name.is_empty() || name == "{{root}}" { - /* >>>>> [DBO] REPLACE next */ //return None; - debug!("** [DBO]: from_path_segment: replacing {{root}} empty sengment on None;"); if modsep { return Some(UseSegment::Empty); } else { return None; } - /* <<<<< [DBO] REPLACE next */ } Some(match name { "self" => UseSegment::Slf(None), "super" => UseSegment::Super(None), "crate" => UseSegment::Crate(None), - _ => { - //let mod_sep = if modsep { "::" } else { "" }; // >>>> [DBO] DELETE <<<< - /* >>>>> [DBO] REPLACE next */ - //UseSegment::Ident(format!("{}{}", mod_sep, name), None) - // ????????!!!!!!! ADD here and empty segment ??????? - UseSegment::Ident(name.to_string(), None) - /* <<<<<< [DBO] REPLACE next */ - } + _ => UseSegment::Ident(name.to_string(), None), }) } } @@ -217,13 +198,8 @@ impl fmt::Display for UseSegment { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match *self { UseSegment::Glob => write!(f, "*"), - UseSegment::Empty => write!(f, ""), // >>>>> [DBO] ADD <<<<<< - //UseSegment::Ident(ref s, _) => write!(f, "{}", s), >>>>> [DBO] without debug <<<<< - UseSegment::Ident(ref s, ref s1) => { - debug!( - "** [DBO] fmt::Display for UseSegment: Ident s={:?}, s1={:?};", - s, s1 - ); + UseSegment::Empty => write!(f, ""), + UseSegment::Ident(ref s, _) => { write!(f, "{}", s) } UseSegment::Slf(..) => write!(f, "self"), @@ -349,8 +325,6 @@ impl UseTree { opt_lo: Option, attrs: Option>, ) -> UseTree { - debug!("** [DBO] from_ast: enter a={:?};", a); - let span = if let Some(lo) = opt_lo { mk_sp(lo, a.span.hi()) } else { @@ -376,7 +350,6 @@ impl UseTree { } } - /* >>>>>>>> [DBO] ADD */ let segment_from_simple = |path: &ast::Path, rename: &Option| { let name = rewrite_ident(context, path_to_imported_ident(&path)).to_owned(); let alias = rename.and_then(|ident| { @@ -389,10 +362,6 @@ impl UseTree { Some(rewrite_ident(context, ident).to_owned()) } }); - debug!( - "** [DBO] from_ast: segment_from_simple: name={:?}, alias={:?}, rename={:?};", - name, alias, rename - ); match name.as_ref() { "self" => UseSegment::Slf(alias), "super" => UseSegment::Super(alias), @@ -400,26 +369,10 @@ impl UseTree { _ => UseSegment::Ident(name, alias), } }; - /* <<<<<<< [DBO] ADD */ - - debug!( - "** [DBO] from_ast: before match result.path={:?};", - result.path - ); match a.kind { UseTreeKind::Glob => { - /* >>>>>>>> ????!!!! [DBO] DELETE ********** - // in case of a global path and the glob starts at the root, e.g., "::*" - if a.prefix.segments.len() == 1 && leading_modsep { - /* >>>>>> [DBO] REPLACE next <<<<< */ - //result.path.push(UseSegment::Ident("".to_owned(), None)); - result.path.push(UseSegment::Empty); - debug!("** [DBO] from_ast: Glob result.path1={:?};", result.path); - } - ******** <<<<<<<<<<<< [DBO] DELETE */ result.path.push(UseSegment::Glob); - debug!("** [DBO] from_ast: Glob result.pathE={:?};", result.path); } UseTreeKind::Nested(ref list) => { // Extract comments between nested use items. @@ -437,58 +390,9 @@ impl UseTree { false, ) .collect(); - debug!( - "** [DBO] from_ast: Nested: len={:?}, list={:?}, items={:?}, result={:?};", - list.len(), - list, - items, - result, - ); // find whether a case of a global path and the nested list starts at the root // with one item, e.g., "::{foo}", and does not include comments or "as". - /******** >>>>>>> REPLACE PREVIOUS CHANGE ************************** - let first_item = if a.prefix.segments.len() == 1 - && list.len() == 1 - && result.to_string().is_empty() - { - let first = &list[0].0; - match first.kind { - UseTreeKind::Simple(..) => { - // "-1" for the "}" - let snippet = context - .snippet(mk_sp(first.span.lo(), span.hi() - BytePos(1))) - .trim(); - // Ensure that indent includes only the name and not - // "as" clause, comments, etc. - if snippet.eq(&format!("{}", first.prefix.segments[0].ident)) { - Some(first) - } else { - None - } - } - _ => None, - } - } else { - None - }; - - if let Some(first) = first_item { - // in case of a global path and the nested list starts at the root - // with one item, e.g., "::{foo}" - let tree = Self::from_ast(context, first, None, None, None, None); - //let mod_sep = if leading_modsep { "::" } else { "" }; //>>>> [DBO] DEL <<<< - /* >>>>>> [DBO] REPLACE next <<<<< */ - //let seg = UseSegment::Ident(format!("{}{}", mod_sep, tree), None); - let seg = UseSegment::Ident(format!("{}", tree), None); - result.path.pop(); - /* >>>>> [DBO] ADD */ - if leading_modsep { - result.path.push(UseSegment::Empty); - } - /* <<<<<< [DBO] ADD */ - result.path.push(seg); - ***************** REPLACE PREVIOUS CHANGE **************************/ let mut path = None; let mut rename = None; if a.prefix.segments.len() == 1 && list.len() == 1 && result.to_string().is_empty() @@ -513,17 +417,7 @@ impl UseTree { if let (Some(path), Some(rename)) = (path, rename) { result.path.push(segment_from_simple(path, rename)); - /******** <<<<<<<< REPLACE PREVIOUS CHANGE **************************/ } else { - /******* >>>>>>>> [DBO] DELETE *********** - // in case of a global path and the nested list starts at the root, - // e.g., "::{foo, bar}" - if a.prefix.segments.len() == 1 && leading_modsep { - /* >>>>>> [DBO] REPLACE next <<<<< */ - //result.path.push(UseSegment::Ident("".to_owned(), None)); - result.path.push(UseSegment::Empty); - } - ********* <<<<<<< [DBO] DELETE *********/ result.path.push(UseSegment::List( list.iter() .zip(items.into_iter()) @@ -533,55 +427,18 @@ impl UseTree { .collect(), )); }; - debug!("** [DBO] from_ast: Nested result.path={:?};", result.path); } UseTreeKind::Simple(ref rename, ..) => { // If the path has leading double colons and is composed of only 2 segments, then we // bypass the call to path_to_imported_ident which would get only the ident and // lose the path root, e.g., `that` in `::that`. // The span of `a.prefix` contains the leading colons. - /******* >>>>>>>> [DBO] REAPLCE *********** - let name = if a.prefix.segments.len() == 2 && leading_modsep { - debug!( - "** [DBO] from_ast: snippet(a.prefix.span)={:?}, a.prefix.segments={:?}", - context.snippet(a.prefix.span), a.prefix.segments - ); - context.snippet(a.prefix.span).to_owned() - } else { - rewrite_ident(context, path_to_imported_ident(&a.prefix)).to_owned() - }; - *************** [DBO] REPLACE ***********/ - /********* >>>>>> [DBO] MOVE into closure simple_to_segment ********** - let name = rewrite_ident(context, path_to_imported_ident(&a.prefix)).to_owned(); - /* <<<<<<<< [DBO] REPLACE *********/ - let alias = rename.and_then(|ident| { - if ident.name == sym::underscore_imports { - // for impl-only-use - Some("_".to_owned()) - } else if ident == path_to_imported_ident(&a.prefix) { - None - } else { - Some(rewrite_ident(context, ident).to_owned()) - } - }); - - /* >>>>>> [DBO] REPLACE ********************/ - let segment = match name.as_ref() { - "self" => UseSegment::Slf(alias), - "super" => UseSegment::Super(alias), - "crate" => UseSegment::Crate(alias), - _ => UseSegment::Ident(name, alias), - }; - ************* [DBO] MOVE into closure simple_to_segment **********/ let segment = segment_from_simple(&a.prefix, rename); // `name` is already in result. result.path.pop(); result.path.push(segment); - debug!("** [DBO] from_ast: Simple result.path={:?};", result.path); - /********* >>>>>> [DBO] MOVE into closure simple_to_segment **********/ } } - debug!("** [DBO] from_ast: resltEE.path={:?};", result.path); result } @@ -822,7 +679,7 @@ impl Ord for UseSegment { .all(|c| c.is_uppercase() || c == '_' || c.is_numeric()) } - let ret = match (self, other) { + match (self, other) { (&Slf(ref a), &Slf(ref b)) | (&Super(ref a), &Super(ref b)) | (&Crate(ref a), &Crate(ref b)) => compare_opt_ident_as_versions(&a, &b), @@ -830,7 +687,6 @@ impl Ord for UseSegment { (&Ident(ref pia, ref aa), &Ident(ref pib, ref ab)) => { let ia = pia.trim_start_matches("r#"); let ib = pib.trim_start_matches("r#"); - debug!("** [DBO] cmp for UseSegment: ia={:?}, ib={:?};", ia, ib); // snake_case < CamelCase < UPPER_SNAKE_CASE if ia.starts_with(char::is_uppercase) && ib.starts_with(char::is_lowercase) { return Ordering::Greater; @@ -863,58 +719,30 @@ impl Ord for UseSegment { (_, &Super(_)) => Ordering::Greater, (&Crate(_), _) => Ordering::Less, (_, &Crate(_)) => Ordering::Greater, - /* >>>>>>> [DBO] ADD */ (&Empty, &Empty) => Ordering::Equal, (&Empty, _) => Ordering::Less, (_, &Empty) => Ordering::Greater, - /* <<<<<<< [DBO] ADD */ (&Ident(..), _) => Ordering::Less, (_, &Ident(..)) => Ordering::Greater, (&Glob, _) => Ordering::Less, (_, &Glob) => Ordering::Greater, - }; - debug!( - "** [DBO] cmp for UseSegment: retEE={:?}, self={:?}, other={:?};", - ret, self, other - ); - ret + } } } impl Ord for UseTree { fn cmp(&self, other: &UseTree) -> Ordering { - debug!( - "** [DBO] cmp for UseTree: enter self={:?}, other={:?};", - self, other - ); for (a, b) in self.path.iter().zip(other.path.iter()) { // The comparison without aliases is a hack to avoid situations like // comparing `a::b` to `a as c` - where the latter should be ordered // first since it is shorter. - //??????!!!!!!! let ord = a.remove_alias().cmp(&b.remove_alias()); - debug!( - "** [DBO] cmp for UseTree: next loop ord={:?}, self={:?}, other={:?};", - ord, a, b - ); if ord != Ordering::Equal { return ord; } } - /* >>>>>> [DBO] REPLACE next */ - //Ord::cmp(&self.path.len(), &other.path.len()) - // .then(Ord::cmp(&self.path.last(), &other.path.last())) - //????!!!!!!! - let ret = Ord::cmp(&self.path.len(), &other.path.len()) - .then(Ord::cmp(&self.path.last(), &other.path.last())); - debug!( - "** [DBO] cmp for UseTree: end cmp ord={:?}, self.len={:?}, other.len={:?};", - ret, - self.path.len(), - other.path.len(), - ); - ret - /* <<<<<<< [DBO] REPLACE next */ + Ord::cmp(&self.path.len(), &other.path.len()) + .then(Ord::cmp(&self.path.last(), &other.path.last())) } } @@ -1002,7 +830,7 @@ impl Rewrite for UseSegment { UseSegment::Crate(Some(ref rename)) => format!("crate as {}", rename), UseSegment::Crate(None) => "crate".to_owned(), UseSegment::Glob => "*".to_owned(), - UseSegment::Empty => "".to_owned(), // >>>>>> [DBO] ADD <<<<<<<< + UseSegment::Empty => "".to_owned(), UseSegment::List(ref use_tree_list) => rewrite_nested_use_tree( context, use_tree_list, diff --git a/src/formatting/reorder.rs b/src/formatting/reorder.rs index 5ae80b52a09..5ffd6f299b5 100644 --- a/src/formatting/reorder.rs +++ b/src/formatting/reorder.rs @@ -307,8 +307,6 @@ fn group_imports(uts: Vec) -> Vec> { local_imports.push(ut) } // These are probably illegal here - /* >>>>>> [DBO] REPLACE next <<<<<< */ - //UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), UseSegment::Empty | UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), } } From a5b332885510d667e10f06c759e8f74d17acbc35 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Sat, 16 Jan 2021 18:34:14 +0200 Subject: [PATCH 7/8] Revert to fix in from_ast() with the root :: prefix the name - with as handling enhancement --- src/formatting/imports.rs | 89 +++++++++++++++++++++----------------- src/formatting/reorder.rs | 2 +- tests/source/issue-3943.rs | 2 +- tests/target/issue-3943.rs | 16 +++---- 4 files changed, 59 insertions(+), 50 deletions(-) diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index 8a25db30dff..54362924b5e 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -11,7 +11,7 @@ use rustc_span::{ use crate::config::lists::*; use crate::config::{Edition, IndentStyle}; use crate::formatting::{ - comment::combine_strs_with_missing_comments, + comment::{combine_strs_with_missing_comments, contains_comment}, lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListItem, Separator}, reorder::{compare_as_versions, compare_opt_ident_as_versions}, rewrite::{Rewrite, RewriteContext}, @@ -94,7 +94,6 @@ pub(crate) enum UseSegment { Super(Option), Crate(Option), Glob, - Empty, List(Vec), } @@ -147,18 +146,16 @@ impl UseSegment { ) -> Option { let name = rewrite_ident(context, path_seg.ident); if name.is_empty() || name == "{{root}}" { - //return None; - if modsep { - return Some(UseSegment::Empty); - } else { - return None; - } + return None; } Some(match name { "self" => UseSegment::Slf(None), "super" => UseSegment::Super(None), "crate" => UseSegment::Crate(None), - _ => UseSegment::Ident(name.to_string(), None), + _ => { + let mod_sep = if modsep { "::" } else { "" }; + UseSegment::Ident(format!("{}{}", mod_sep, name), None) + } }) } } @@ -198,10 +195,7 @@ impl fmt::Display for UseSegment { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match *self { UseSegment::Glob => write!(f, "*"), - UseSegment::Empty => write!(f, ""), - UseSegment::Ident(ref s, _) => { - write!(f, "{}", s) - } + UseSegment::Ident(ref s, _) => write!(f, "{}", s), UseSegment::Slf(..) => write!(f, "self"), UseSegment::Super(..) => write!(f, "super"), UseSegment::Crate(..) => write!(f, "crate"), @@ -350,28 +344,25 @@ impl UseTree { } } - let segment_from_simple = |path: &ast::Path, rename: &Option| { - let name = rewrite_ident(context, path_to_imported_ident(&path)).to_owned(); - let alias = rename.and_then(|ident| { + let rename_to_alias = |rename: &Option| { + rename.and_then(|ident| { if ident.name == sym::underscore_imports { // for impl-only-use Some("_".to_owned()) - } else if ident == path_to_imported_ident(&path) { + } else if ident == path_to_imported_ident(&a.prefix) { None } else { Some(rewrite_ident(context, ident).to_owned()) } - }); - match name.as_ref() { - "self" => UseSegment::Slf(alias), - "super" => UseSegment::Super(alias), - "crate" => UseSegment::Crate(alias), - _ => UseSegment::Ident(name, alias), - } + }) }; match a.kind { UseTreeKind::Glob => { + // in case of a global path and the glob starts at the root, e.g., "::*" + if a.prefix.segments.len() == 1 && leading_modsep { + result.path.push(UseSegment::Ident("".to_owned(), None)); + } result.path.push(UseSegment::Glob); } UseTreeKind::Nested(ref list) => { @@ -392,32 +383,42 @@ impl UseTree { .collect(); // find whether a case of a global path and the nested list starts at the root - // with one item, e.g., "::{foo}", and does not include comments or "as". - let mut path = None; - let mut rename = None; + // with one item, e.g., "::{foo as bar}", and does not include comments. + let mut first_item = None; + let mut first_alias = None; if a.prefix.segments.len() == 1 && list.len() == 1 && result.to_string().is_empty() { let first = &list[0].0; match first.kind { - UseTreeKind::Simple(ref ren, ..) => { + UseTreeKind::Simple(ref rename, ..) => { // "-1" for the "}" let snippet = context .snippet(mk_sp(first.span.lo(), span.hi() - BytePos(1))) .trim(); - // Ensure that indent includes only the name and not - // "as" clause, comments, etc. - if snippet.eq(&format!("{}", first.prefix.segments[0].ident)) { - path = Some(&first.prefix); - rename = Some(ren); + // Ensure that indent does not include comments + if !contains_comment(&snippet) { + first_item = Some(first); + first_alias = rename_to_alias(rename); } } _ => {} } }; - if let (Some(path), Some(rename)) = (path, rename) { - result.path.push(segment_from_simple(path, rename)); + if let Some(first) = first_item { + // in case of a global path and the nested list starts at the root + // with one item, e.g., "::{foo as bar}" + let tree = Self::from_ast(context, first, None, None, None, None); + let mod_sep = if leading_modsep { "::" } else { "" }; + let seg = UseSegment::Ident(format!("{}{}", mod_sep, tree), first_alias); + result.path.pop(); + result.path.push(seg); } else { + // in case of a global path and the nested list starts at the root, + // e.g., "::{foo, bar}" + if a.prefix.segments.len() == 1 && leading_modsep { + result.path.push(UseSegment::Ident("".to_owned(), None)); + } result.path.push(UseSegment::List( list.iter() .zip(items.into_iter()) @@ -433,7 +434,19 @@ impl UseTree { // bypass the call to path_to_imported_ident which would get only the ident and // lose the path root, e.g., `that` in `::that`. // The span of `a.prefix` contains the leading colons. - let segment = segment_from_simple(&a.prefix, rename); + let name = if a.prefix.segments.len() == 2 && leading_modsep { + context.snippet(a.prefix.span).to_owned() + } else { + rewrite_ident(context, path_to_imported_ident(&a.prefix)).to_owned() + }; + let alias = rename_to_alias(rename); + let segment = match name.as_ref() { + "self" => UseSegment::Slf(alias), + "super" => UseSegment::Super(alias), + "crate" => UseSegment::Crate(alias), + _ => UseSegment::Ident(name, alias), + }; + // `name` is already in result. result.path.pop(); result.path.push(segment); @@ -719,9 +732,6 @@ impl Ord for UseSegment { (_, &Super(_)) => Ordering::Greater, (&Crate(_), _) => Ordering::Less, (_, &Crate(_)) => Ordering::Greater, - (&Empty, &Empty) => Ordering::Equal, - (&Empty, _) => Ordering::Less, - (_, &Empty) => Ordering::Greater, (&Ident(..), _) => Ordering::Less, (_, &Ident(..)) => Ordering::Greater, (&Glob, _) => Ordering::Less, @@ -830,7 +840,6 @@ impl Rewrite for UseSegment { UseSegment::Crate(Some(ref rename)) => format!("crate as {}", rename), UseSegment::Crate(None) => "crate".to_owned(), UseSegment::Glob => "*".to_owned(), - UseSegment::Empty => "".to_owned(), UseSegment::List(ref use_tree_list) => rewrite_nested_use_tree( context, use_tree_list, diff --git a/src/formatting/reorder.rs b/src/formatting/reorder.rs index 5ffd6f299b5..379aa109b14 100644 --- a/src/formatting/reorder.rs +++ b/src/formatting/reorder.rs @@ -307,7 +307,7 @@ fn group_imports(uts: Vec) -> Vec> { local_imports.push(ut) } // These are probably illegal here - UseSegment::Empty | UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), + UseSegment::Glob | UseSegment::List(_) => external_imports.push(ut), } } diff --git a/tests/source/issue-3943.rs b/tests/source/issue-3943.rs index 3918b75be6b..e9224e6436f 100644 --- a/tests/source/issue-3943.rs +++ b/tests/source/issue-3943.rs @@ -25,7 +25,7 @@ use aaaa::{BBBB}; use crate::detect::{Feature, cache}; use super::{auxvec}; -// Tests with comments and "as" +// Tests with comments use a::{/* pre-comment */ item}; use a::{ item /* post-comment */}; use a::{/* pre-comment */ item /* post-comment */ }; diff --git a/tests/target/issue-3943.rs b/tests/target/issue-3943.rs index e46fafd910a..90c088ebf7a 100644 --- a/tests/target/issue-3943.rs +++ b/tests/target/issue-3943.rs @@ -1,20 +1,20 @@ // Tests for original #3943 issue +use ::Foo; use ::foo; use ::foo::Bar1; use ::foo::{Bar2, Baz2}; -use ::Foo; use ::{Bar3, Baz3}; +use ::Foo; use ::foo; use ::foo::Bar; use ::foo::{Bar, Baz}; -use ::Foo; use ::{Bar, Baz}; +use ::Foo; use ::foo; use ::foo::Bar; use ::foo::{Bar, Baz}; -use ::Foo; use ::{Bar, Baz}; // Additional tests for signle item `{}` handling @@ -25,20 +25,20 @@ use ::BBBB; use aaaa::BBBB; use bbbbb::AAAA; -// Tests with comments and "as" +// Tests with comments use a::{/* pre-comment */ item}; use a::{item /* post-comment */}; use a::{/* pre-comment */ item /* post-comment */}; // Misc use self::std::fs as self_fs; -use ::foo; -use ::foo as bar; -use ::foo::{foo, Foo}; use ::Foo; use ::Foo as baz; use ::Foo1; -use ::*; +use ::foo; +use ::foo as bar; +use ::foo::{foo, Foo}; use dummy; use std; use Super::foo; +use ::*; From a13b2367a5132230e7ac7347a0016cf76093e559 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Thu, 21 Jan 2021 12:41:06 +0200 Subject: [PATCH 8/8] Merge import.rs changes from latest master --- src/formatting/imports.rs | 149 ++++++++++++++++++----- tests/source/imports_granularity_item.rs | 6 + tests/target/imports_granularity_item.rs | 13 ++ 3 files changed, 139 insertions(+), 29 deletions(-) create mode 100644 tests/source/imports_granularity_item.rs create mode 100644 tests/target/imports_granularity_item.rs diff --git a/src/formatting/imports.rs b/src/formatting/imports.rs index 54362924b5e..aa507d862be 100644 --- a/src/formatting/imports.rs +++ b/src/formatting/imports.rs @@ -160,7 +160,7 @@ impl UseSegment { } } -pub(crate) fn merge_use_trees(use_trees: Vec) -> Vec { +pub(crate) fn merge_use_trees(use_trees: Vec, merge_by: SharedPrefix) -> 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() { @@ -169,8 +169,11 @@ pub(crate) fn merge_use_trees(use_trees: Vec) -> Vec { } for flattened in use_tree.flatten() { - if let Some(tree) = result.iter_mut().find(|tree| tree.share_prefix(&flattened)) { - tree.merge(&flattened); + if let Some(tree) = result + .iter_mut() + .find(|tree| tree.share_prefix(&flattened, merge_by)) + { + tree.merge(&flattened, merge_by); } else { result.push(flattened); } @@ -179,6 +182,24 @@ pub(crate) fn merge_use_trees(use_trees: Vec) -> Vec { result } +pub(crate) fn flatten_use_trees(use_trees: Vec) -> Vec { + use_trees + .into_iter() + .flat_map(UseTree::flatten) + .map(|mut tree| { + // If a path ends in `::self`, rewrite it to `::{self}`. + if let Some(UseSegment::Slf(..)) = tree.path.last() { + let self_segment = tree.path.pop().unwrap(); + tree.path.push(UseSegment::List(vec![UseTree::from_path( + vec![self_segment], + DUMMY_SP, + )])); + } + tree + }) + .collect() +} + impl fmt::Debug for UseTree { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(self, f) @@ -333,7 +354,7 @@ impl UseTree { }; let leading_modsep = - context.config.edition() == Edition::Edition2018 && a.prefix.is_global(); + context.config.edition() >= Edition::Edition2018 && a.prefix.is_global(); let mut modsep = leading_modsep; @@ -564,7 +585,7 @@ impl UseTree { } } - fn share_prefix(&self, other: &UseTree) -> bool { + fn share_prefix(&self, other: &UseTree, shared_prefix: SharedPrefix) -> bool { if self.path.is_empty() || other.path.is_empty() || self.attrs.is_some() @@ -572,7 +593,12 @@ impl UseTree { { false } else { - self.path[0] == other.path[0] + match shared_prefix { + SharedPrefix::Crate => self.path[0] == other.path[0], + SharedPrefix::Module => { + self.path[..self.path.len() - 1] == other.path[..other.path.len() - 1] + } + } } } @@ -606,7 +632,7 @@ impl UseTree { } } - fn merge(&mut self, other: &UseTree) { + fn merge(&mut self, other: &UseTree, merge_by: SharedPrefix) { let mut prefix = 0; for (a, b) in self.path.iter().zip(other.path.iter()) { if *a == *b { @@ -615,20 +641,30 @@ impl UseTree { break; } } - if let Some(new_path) = merge_rest(&self.path, &other.path, prefix) { + if let Some(new_path) = merge_rest(&self.path, &other.path, prefix, merge_by) { self.path = new_path; self.span = self.span.to(other.span); } } } -fn merge_rest(a: &[UseSegment], b: &[UseSegment], mut len: usize) -> Option> { +fn merge_rest( + a: &[UseSegment], + b: &[UseSegment], + mut len: usize, + merge_by: SharedPrefix, +) -> Option> { if a.len() == len && b.len() == len { return None; } if a.len() != len && b.len() != len { - if let UseSegment::List(mut list) = a[len].clone() { - merge_use_trees_inner(&mut list, UseTree::from_path(b[len..].to_vec(), DUMMY_SP)); + if let UseSegment::List(ref list) = a[len] { + let mut list = list.clone(); + merge_use_trees_inner( + &mut list, + UseTree::from_path(b[len..].to_vec(), DUMMY_SP), + merge_by, + ); let mut new_path = b[..len].to_vec(); new_path.push(UseSegment::List(list)); return Some(new_path); @@ -655,9 +691,11 @@ fn merge_rest(a: &[UseSegment], b: &[UseSegment], mut len: usize) -> Option, use_tree: UseTree) { - let similar_trees = trees.iter_mut().filter(|tree| tree.share_prefix(&use_tree)); - if use_tree.path.len() == 1 { +fn merge_use_trees_inner(trees: &mut Vec, use_tree: UseTree, merge_by: SharedPrefix) { + let similar_trees = trees + .iter_mut() + .filter(|tree| tree.share_prefix(&use_tree, merge_by)); + if use_tree.path.len() == 1 && merge_by == SharedPrefix::Crate { if let Some(tree) = similar_trees.min_by_key(|tree| tree.path.len()) { if tree.path.len() == 1 { return; @@ -665,7 +703,7 @@ fn merge_use_trees_inner(trees: &mut Vec, use_tree: UseTree) { } } else if let Some(tree) = similar_trees.max_by_key(|tree| tree.path.len()) { if tree.path.len() > 1 { - tree.merge(&use_tree); + tree.merge(&use_tree, merge_by); return; } } @@ -868,6 +906,12 @@ impl Rewrite for UseTree { } } +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub(crate) enum SharedPrefix { + Crate, + Module, +} + #[cfg(test)] mod test { use super::*; @@ -1014,44 +1058,91 @@ mod test { } } - #[test] - fn test_use_tree_merge() { - macro_rules! test_merge { - ([$($input:expr),* $(,)*], [$($output:expr),* $(,)*]) => { - assert_eq!( - merge_use_trees(parse_use_trees!($($input,)*)), - parse_use_trees!($($output,)*), - ); - } + macro_rules! test_merge { + ($by:ident, [$($input:expr),* $(,)*], [$($output:expr),* $(,)*]) => { + assert_eq!( + merge_use_trees(parse_use_trees!($($input,)*), SharedPrefix::$by), + 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, b::c}"]); - test_merge!(["a::b", "a::b"], ["a::b"]); - test_merge!(["a", "a::b", "a::b::c"], ["a::{self, b, b::c}"]); + #[test] + fn test_use_tree_merge_crate() { + test_merge!( + Crate, + ["a::b::{c, d}", "a::b::{e, f}"], + ["a::b::{c, d, e, f}"] + ); + test_merge!(Crate, ["a::b::c", "a::b"], ["a::{b, b::c}"]); + test_merge!(Crate, ["a::b", "a::b"], ["a::b"]); + test_merge!(Crate, ["a", "a::b", "a::b::c"], ["a::{self, b, b::c}"]); test_merge!( + Crate, ["a", "a::b", "a::b::c", "a::b::c::d"], ["a::{self, b, b::{c, c::d}}"] ); - test_merge!(["a", "a::b", "a::b::c", "a::b"], ["a::{self, b, b::c}"]); test_merge!( + Crate, + ["a", "a::b", "a::b::c", "a::b"], + ["a::{self, b, b::c}"] + ); + test_merge!( + Crate, ["a::{b::{self, c}, d::e}", "a::d::f"], ["a::{b::{self, c}, d::{e, f}}"] ); test_merge!( + Crate, ["a::d::f", "a::{b::{self, c}, d::e}"], ["a::{b::{self, c}, d::{e, f}}"] ); test_merge!( + Crate, ["a::{c, d, b}", "a::{d, e, b, a, f}", "a::{f, g, c}"], ["a::{a, b, c, d, e, f, g}"] ); test_merge!( + Crate, ["a::{self}", "b::{self as foo}"], ["a::{self}", "b::{self as foo}"] ); } + #[test] + fn test_use_tree_merge_module() { + test_merge!( + Module, + ["foo::b", "foo::{a, c, d::e}"], + ["foo::{a, b, c}", "foo::d::e"] + ); + + test_merge!( + Module, + ["foo::{a::b, a::c, d::e, d::f}"], + ["foo::a::{b, c}", "foo::d::{e, f}"] + ); + } + + #[test] + fn test_flatten_use_trees() { + assert_eq!( + flatten_use_trees(parse_use_trees!["foo::{a::{b, c}, d::e}"]), + parse_use_trees!["foo::a::b", "foo::a::c", "foo::d::e"] + ); + + assert_eq!( + flatten_use_trees(parse_use_trees!["foo::{self, a, b::{c, d}, e::*}"]), + parse_use_trees![ + "foo::{self}", + "foo::a", + "foo::b::c", + "foo::b::d", + "foo::e::*" + ] + ); + } + #[test] fn test_use_tree_flatten() { assert_eq!( diff --git a/tests/source/imports_granularity_item.rs b/tests/source/imports_granularity_item.rs new file mode 100644 index 00000000000..d0e94df66ae --- /dev/null +++ b/tests/source/imports_granularity_item.rs @@ -0,0 +1,6 @@ +// rustfmt-imports_granularity: Item + +use a::{b, c, d}; +use a::{f::g, h::{i, j}}; +use a::{l::{self, m, n::o, p::*}}; +use a::q::{self}; diff --git a/tests/target/imports_granularity_item.rs b/tests/target/imports_granularity_item.rs new file mode 100644 index 00000000000..eace785e670 --- /dev/null +++ b/tests/target/imports_granularity_item.rs @@ -0,0 +1,13 @@ +// rustfmt-imports_granularity: Item + +use a::b; +use a::c; +use a::d; +use a::f::g; +use a::h::i; +use a::h::j; +use a::l::m; +use a::l::n::o; +use a::l::p::*; +use a::l::{self}; +use a::q::{self};