From cd6cd91bf345d47cbf22e00fc4cddced4ae67ae6 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sat, 12 Sep 2020 23:27:01 +0200 Subject: Tidy up `recursive_merge` implementation --- crates/assists/src/utils/insert_use.rs | 120 ++++++++++++++++----------------- 1 file changed, 60 insertions(+), 60 deletions(-) (limited to 'crates') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 4f5fd317a..99f0b5b75 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -120,7 +120,6 @@ pub(crate) fn insert_use( } if let ident_level @ 1..=usize::MAX = scope.indent_level().0 as usize { - // FIXME: this alone doesnt properly re-align all cases buf.push(make::tokens::whitespace(&" ".repeat(4 * ident_level)).into()); } buf.push(use_item.syntax().clone().into()); @@ -164,44 +163,6 @@ pub(crate) fn try_merge_imports( Some(old.with_use_tree(merged)) } -/// Orders paths in the following way: -/// the sole self token comes first, after that come uppercase identifiers, then lowercase identifiers -/// FIXME: rustfmt sort lowercase idents before uppercase -fn path_cmp(a: &ast::Path, b: &ast::Path) -> Ordering { - let a = segment_iter(a); - let b = segment_iter(b); - let mut a_clone = a.clone(); - let mut b_clone = b.clone(); - match ( - a_clone.next().and_then(|ps| ps.self_token()).is_some() && a_clone.next().is_none(), - b_clone.next().and_then(|ps| ps.self_token()).is_some() && b_clone.next().is_none(), - ) { - (true, true) => Ordering::Equal, - (true, false) => Ordering::Less, - (false, true) => Ordering::Greater, - (false, false) => { - // cmp_by would be useful for us here but that is currently unstable - // cmp doesnt work due the lifetimes on text's return type - a.zip(b) - .flat_map(|(seg, seg2)| seg.name_ref().zip(seg2.name_ref())) - .find_map(|(a, b)| match a.text().cmp(b.text()) { - ord @ Ordering::Greater | ord @ Ordering::Less => Some(ord), - Ordering::Equal => None, - }) - .unwrap_or(Ordering::Equal) - } - } -} - -fn path_cmp_opt(a: Option, b: Option) -> Ordering { - match (a, b) { - (None, None) => Ordering::Equal, - (None, Some(_)) => Ordering::Less, - (Some(_), None) => Ordering::Greater, - (Some(a), Some(b)) => path_cmp(&a, &b), - } -} - pub(crate) fn try_merge_trees( old: &ast::UseTree, new: &ast::UseTree, @@ -216,17 +177,18 @@ pub(crate) fn try_merge_trees( recursive_merge(&lhs, &rhs, merge).map(|(merged, _)| merged) } -/// Returns the merged tree and the number of children it has, which is required to check if the tree is allowed to be used for MergeBehaviour::Last +/// Recursively "zips" together lhs and rhs. fn recursive_merge( lhs: &ast::UseTree, rhs: &ast::UseTree, merge: MergeBehaviour, -) -> Option<(ast::UseTree, usize)> { +) -> Option<(ast::UseTree, bool)> { let mut use_trees = lhs .use_tree_list() .into_iter() .flat_map(|list| list.use_trees()) - // check if any of the use trees are nested, if they are and the behaviour is last only we are not allowed to merge this + // check if any of the use trees are nested, if they are and the behaviour is `last` we are not allowed to merge this + // so early exit the iterator by using Option's Intoiterator impl .map(|tree| match merge == MergeBehaviour::Last && tree.use_tree_list().is_some() { true => None, false => Some(tree), @@ -237,15 +199,17 @@ fn recursive_merge( let rhs_path = rhs_t.path(); match use_trees.binary_search_by(|p| path_cmp_opt(p.path(), rhs_path.clone())) { Ok(idx) => { - let lhs_path = use_trees[idx].path()?; + let lhs_t = &mut use_trees[idx]; + let lhs_path = lhs_t.path()?; let rhs_path = rhs_path?; let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?; if lhs_prefix == lhs_path && rhs_prefix == rhs_path { - let tree_is_self = - |tree: ast::UseTree| tree.path().map(path_is_self).unwrap_or(false); + let tree_is_self = |tree: ast::UseTree| { + tree.path().as_ref().map(path_is_self).unwrap_or(false) + }; // check if only one of the two trees has a tree list, and whether that then contains `self` or not. // If this is the case we can skip this iteration since the path without the list is already included in the other one via `self` - if use_trees[idx] + if lhs_t .use_tree_list() .xor(rhs_t.use_tree_list()) .map(|tree_list| tree_list.use_trees().any(tree_is_self)) @@ -257,8 +221,8 @@ fn recursive_merge( // glob imports arent part of the use-tree lists so we need to special handle them here as well // this special handling is only required for when we merge a module import into a glob import of said module // see the `merge_self_glob` or `merge_mod_into_glob` tests - if use_trees[idx].star_token().is_some() || rhs_t.star_token().is_some() { - use_trees[idx] = make::use_tree( + if lhs_t.star_token().is_some() || rhs_t.star_token().is_some() { + *lhs_t = make::use_tree( make::path_unqualified(make::path_segment_self()), None, None, @@ -276,11 +240,14 @@ fn recursive_merge( continue; } } - let lhs = use_trees[idx].split_prefix(&lhs_prefix); + let lhs = lhs_t.split_prefix(&lhs_prefix); let rhs = rhs_t.split_prefix(&rhs_prefix); + let this_has_children = use_trees.len() > 0; match recursive_merge(&lhs, &rhs, merge) { - Some((_, count)) - if merge == MergeBehaviour::Last && use_trees.len() > 1 && count > 1 => + Some((_, has_multiple_children)) + if merge == MergeBehaviour::Last + && this_has_children + && has_multiple_children => { return None } @@ -293,9 +260,8 @@ fn recursive_merge( } } } - let count = use_trees.len(); - let tl = make::use_tree_list(use_trees); - Some((lhs.with_use_tree_list(tl), count)) + let has_multiple_children = use_trees.len() > 1; + Some((lhs.with_use_tree_list(make::use_tree_list(use_trees)), has_multiple_children)) } /// Traverses both paths until they differ, returning the common prefix of both. @@ -306,7 +272,7 @@ fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Pa loop { match (lhs_curr.segment(), rhs_curr.segment()) { (Some(lhs), Some(rhs)) if lhs.syntax().text() == rhs.syntax().text() => (), - _ => break, + _ => break res, } res = Some((lhs_curr.clone(), rhs_curr.clone())); @@ -315,17 +281,16 @@ fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Pa lhs_curr = lhs; rhs_curr = rhs; } - _ => break, + _ => break res, } } - - res } -fn path_is_self(path: ast::Path) -> bool { +fn path_is_self(path: &ast::Path) -> bool { path.segment().and_then(|seg| seg.self_token()).is_some() && path.qualifier().is_none() } +#[inline] fn first_segment(path: &ast::Path) -> Option { first_path(path).segment() } @@ -339,6 +304,41 @@ fn segment_iter(path: &ast::Path) -> impl Iterator + Cl successors(first_segment(path), |p| p.parent_path().parent_path().and_then(|p| p.segment())) } +/// Orders paths in the following way: +/// the sole self token comes first, after that come uppercase identifiers, then lowercase identifiers +// FIXME: rustfmt sort lowercase idents before uppercase, in general we want to have the same ordering rustfmt has +// which is `self` and `super` first, then identifier imports with lowercase ones first, then glob imports and at last list imports. +// Example foo::{self, foo, baz, Baz, Qux, *, {Bar}} +fn path_cmp(a: &ast::Path, b: &ast::Path) -> Ordering { + match (path_is_self(a), path_is_self(b)) { + (true, true) => Ordering::Equal, + (true, false) => Ordering::Less, + (false, true) => Ordering::Greater, + (false, false) => { + let a = segment_iter(a); + let b = segment_iter(b); + // cmp_by would be useful for us here but that is currently unstable + // cmp doesnt work due the lifetimes on text's return type + a.zip(b) + .flat_map(|(seg, seg2)| seg.name_ref().zip(seg2.name_ref())) + .find_map(|(a, b)| match a.text().cmp(b.text()) { + ord @ Ordering::Greater | ord @ Ordering::Less => Some(ord), + Ordering::Equal => None, + }) + .unwrap_or(Ordering::Equal) + } + } +} + +fn path_cmp_opt(a: Option, b: Option) -> Ordering { + match (a, b) { + (None, None) => Ordering::Equal, + (None, Some(_)) => Ordering::Less, + (Some(_), None) => Ordering::Greater, + (Some(a), Some(b)) => path_cmp(&a, &b), + } +} + /// What type of merges are allowed. #[derive(Copy, Clone, PartialEq, Eq)] pub enum MergeBehaviour { @@ -924,6 +924,6 @@ use foo::bar::baz::Qux;", .unwrap(); let result = try_merge_imports(&use0, &use1, mb); - assert_eq!(result, None); + assert_eq!(result.map(|u| u.to_string()), None); } } -- cgit v1.2.3