From 7ccb198af81d8f33ccad66a417ae6529f91df625 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sat, 5 Sep 2020 15:51:26 +0200 Subject: Remove duplicated import merge logic --- crates/assists/src/handlers/merge_imports.rs | 111 +++++++++------------------ crates/assists/src/utils/insert_use.rs | 9 ++- 2 files changed, 43 insertions(+), 77 deletions(-) diff --git a/crates/assists/src/handlers/merge_imports.rs b/crates/assists/src/handlers/merge_imports.rs index 35b884206..da084d5fb 100644 --- a/crates/assists/src/handlers/merge_imports.rs +++ b/crates/assists/src/handlers/merge_imports.rs @@ -1,14 +1,14 @@ -use std::iter::successors; - use syntax::{ - algo::{neighbor, skip_trivia_token, SyntaxRewriter}, - ast::{self, edit::AstNodeEdit, make}, - AstNode, Direction, InsertPosition, SyntaxElement, T, + algo::{neighbor, SyntaxRewriter}, + ast, AstNode, }; use crate::{ assist_context::{AssistContext, Assists}, - utils::next_prev, + utils::{ + insert_use::{try_merge_imports, try_merge_trees}, + next_prev, MergeBehaviour, + }, AssistId, AssistKind, }; @@ -30,23 +30,22 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext) -> Option<() let mut offset = ctx.offset(); if let Some(use_item) = tree.syntax().parent().and_then(ast::Use::cast) { - let (merged, to_delete) = next_prev() - .filter_map(|dir| neighbor(&use_item, dir)) - .filter_map(|it| Some((it.clone(), it.use_tree()?))) - .find_map(|(use_item, use_tree)| { - Some((try_merge_trees(&tree, &use_tree)?, use_item)) + let (merged, to_delete) = + next_prev().filter_map(|dir| neighbor(&use_item, dir)).find_map(|use_item2| { + try_merge_imports(&use_item, &use_item2, MergeBehaviour::Full).zip(Some(use_item2)) })?; - rewriter.replace_ast(&tree, &merged); + rewriter.replace_ast(&use_item, &merged); rewriter += to_delete.remove(); if to_delete.syntax().text_range().end() < offset { offset -= to_delete.syntax().text_range().len(); } } else { - let (merged, to_delete) = next_prev() - .filter_map(|dir| neighbor(&tree, dir)) - .find_map(|use_tree| Some((try_merge_trees(&tree, &use_tree)?, use_tree.clone())))?; + let (merged, to_delete) = + next_prev().filter_map(|dir| neighbor(&tree, dir)).find_map(|use_tree| { + try_merge_trees(&tree, &use_tree, MergeBehaviour::Full).zip(Some(use_tree)) + })?; rewriter.replace_ast(&tree, &merged); rewriter += to_delete.remove(); @@ -67,66 +66,6 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext) -> Option<() ) } -fn try_merge_trees(old: &ast::UseTree, new: &ast::UseTree) -> Option { - let lhs_path = old.path()?; - let rhs_path = new.path()?; - - let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?; - - let lhs = old.split_prefix(&lhs_prefix); - let rhs = new.split_prefix(&rhs_prefix); - - let should_insert_comma = lhs - .use_tree_list()? - .r_curly_token() - .and_then(|it| skip_trivia_token(it.prev_token()?, Direction::Prev)) - .map(|it| it.kind() != T![,]) - .unwrap_or(true); - - let mut to_insert: Vec = Vec::new(); - if should_insert_comma { - to_insert.push(make::token(T![,]).into()); - to_insert.push(make::tokens::single_space().into()); - } - to_insert.extend( - rhs.use_tree_list()? - .syntax() - .children_with_tokens() - .filter(|it| it.kind() != T!['{'] && it.kind() != T!['}']), - ); - let use_tree_list = lhs.use_tree_list()?; - let pos = InsertPosition::Before(use_tree_list.r_curly_token()?.into()); - let use_tree_list = use_tree_list.insert_children(pos, to_insert); - Some(lhs.with_use_tree_list(use_tree_list)) -} - -fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Path)> { - let mut res = None; - let mut lhs_curr = first_path(&lhs); - let mut rhs_curr = first_path(&rhs); - loop { - match (lhs_curr.segment(), rhs_curr.segment()) { - (Some(lhs), Some(rhs)) if lhs.syntax().text() == rhs.syntax().text() => (), - _ => break, - } - res = Some((lhs_curr.clone(), rhs_curr.clone())); - - match (lhs_curr.parent_path(), rhs_curr.parent_path()) { - (Some(lhs), Some(rhs)) => { - lhs_curr = lhs; - rhs_curr = rhs; - } - _ => break, - } - } - - res -} - -fn first_path(path: &ast::Path) -> ast::Path { - successors(Some(path.clone()), |it| it.qualifier()).last().unwrap() -} - #[cfg(test)] mod tests { use crate::tests::{check_assist, check_assist_not_applicable}; @@ -188,6 +127,28 @@ use std::{fmt::{Display, self}}; ); } + #[test] + fn skip_pub1() { + check_assist_not_applicable( + merge_imports, + r" +pub use std::fmt<|>::Debug; +use std::fmt::Display; +", + ); + } + + #[test] + fn skip_pub_last() { + check_assist_not_applicable( + merge_imports, + r" +use std::fmt<|>::Debug; +pub use std::fmt::Display; +", + ); + } + #[test] fn test_merge_nested() { check_assist( diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 1cb52318a..a920e12c5 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -143,8 +143,13 @@ pub(crate) fn try_merge_imports( new: &ast::Use, merge_behaviour: MergeBehaviour, ) -> Option { - // don't merge into re-exports - if old.visibility().and_then(|vis| vis.pub_token()).is_some() { + // don't merge imports with different visibilities + if old + .visibility() + .and_then(|vis| vis.pub_token()) + .or_else(|| new.visibility().and_then(|vis| vis.pub_token())) + .is_some() + { return None; } let old_tree = old.use_tree()?; -- cgit v1.2.3