From ed37335c012a66f5d028a160221b1ca152a61325 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 2 Sep 2020 15:21:10 +0200 Subject: Begin refactor of import insertion --- crates/assists/src/utils/insert_use.rs | 908 ++++++++++++++++----------------- 1 file changed, 440 insertions(+), 468 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 49096a67c..8ee5e0c9c 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -1,17 +1,13 @@ -//! Handle syntactic aspects of inserting a new `use`. -// FIXME: rewrite according to the plan, outlined in -// https://github.com/rust-analyzer/rust-analyzer/issues/3301#issuecomment-592931553 - -use std::iter::successors; +use std::iter::{self, successors}; +use algo::skip_trivia_token; +use ast::{edit::AstNodeEdit, PathSegmentKind, VisibilityOwner}; use either::Either; use syntax::{ - ast::{self, NameOwner, VisibilityOwner}, - AstNode, AstToken, Direction, SmolStr, - SyntaxKind::{PATH, PATH_SEGMENT}, - SyntaxNode, SyntaxToken, T, + algo, + ast::{self, make, AstNode}, + Direction, InsertPosition, SyntaxElement, SyntaxNode, T, }; -use text_edit::TextEditBuilder; use crate::assist_context::AssistContext; @@ -22,525 +18,501 @@ pub(crate) fn find_insert_use_container( ) -> Option> { ctx.sema.ancestors_with_macros(position.clone()).find_map(|n| { if let Some(module) = ast::Module::cast(n.clone()) { - return module.item_list().map(|it| Either::Left(it)); + return module.item_list().map(Either::Left); } Some(Either::Right(ast::SourceFile::cast(n)?)) }) } -/// Creates and inserts a use statement for the given path to import. -/// The use statement is inserted in the scope most appropriate to the -/// the cursor position given, additionally merged with the existing use imports. pub(crate) fn insert_use_statement( // Ideally the position of the cursor, used to position: &SyntaxNode, path_to_import: &str, - ctx: &AssistContext, - builder: &mut TextEditBuilder, + ctx: &crate::assist_context::AssistContext, + builder: &mut text_edit::TextEditBuilder, ) { - let target = path_to_import.split("::").map(SmolStr::new).collect::>(); - let container = find_insert_use_container(position, ctx); + insert_use(position.clone(), make::path_from_text(path_to_import), Some(MergeBehaviour::Full)); +} + +pub fn insert_use( + where_: SyntaxNode, + path: ast::Path, + merge_behaviour: Option, +) -> SyntaxNode { + let use_item = make::use_(make::use_tree(path.clone(), None, None, false)); + // merge into existing imports if possible + if let Some(mb) = merge_behaviour { + for existing_use in where_.children().filter_map(ast::Use::cast) { + if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { + let to_delete: SyntaxElement = existing_use.syntax().clone().into(); + let to_delete = to_delete.clone()..=to_delete; + let to_insert = iter::once(merged.syntax().clone().into()); + return algo::replace_children(&where_, to_delete, to_insert); + } + } + } + + // either we weren't allowed to merge or there is no import that fits the merge conditions + // so look for the place we have to insert to + let (insert_position, add_blank) = find_insert_position(&where_, path); + + let to_insert: Vec = { + let mut buf = Vec::new(); + + if add_blank == AddBlankLine::Before { + buf.push(make::tokens::single_newline().into()); + } + + buf.push(use_item.syntax().clone().into()); + + if add_blank == AddBlankLine::After { + buf.push(make::tokens::single_newline().into()); + } else if add_blank == AddBlankLine::AfterTwice { + buf.push(make::tokens::single_newline().into()); + buf.push(make::tokens::single_newline().into()); + } - if let Some(container) = container { - let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); - let action = best_action_for_target(syntax, position.clone(), &target); - make_assist(&action, &target, builder); + buf + }; + + algo::insert_children(&where_, insert_position, to_insert) +} + +fn try_merge_imports( + old: &ast::Use, + new: &ast::Use, + merge_behaviour: MergeBehaviour, +) -> Option { + // dont merge into re-exports + if old.visibility().map(|vis| vis.pub_token()).is_some() { + return None; + } + let old_tree = old.use_tree()?; + let new_tree = new.use_tree()?; + let merged = try_merge_trees(&old_tree, &new_tree, merge_behaviour)?; + Some(old.with_use_tree(merged)) +} + +/// Simple function that checks if a UseTreeList is deeper than one level +fn use_tree_list_is_nested(tl: &ast::UseTreeList) -> bool { + tl.use_trees().any(|use_tree| { + use_tree.use_tree_list().is_some() || use_tree.path().and_then(|p| p.qualifier()).is_some() + }) +} + +pub fn try_merge_trees( + old: &ast::UseTree, + new: &ast::UseTree, + merge_behaviour: MergeBehaviour, +) -> 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 lhs_tl = lhs.use_tree_list()?; + let rhs_tl = rhs.use_tree_list()?; + + // if we are only allowed to merge the last level check if the paths are only one level deep + // FIXME: This shouldn't work yet i think + if merge_behaviour == MergeBehaviour::Last && use_tree_list_is_nested(&lhs_tl) + || use_tree_list_is_nested(&rhs_tl) + { + return None; } + + let should_insert_comma = lhs_tl + .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_tl + .syntax() + .children_with_tokens() + .filter(|it| it.kind() != T!['{'] && it.kind() != T!['}']), + ); + let pos = InsertPosition::Before(lhs_tl.r_curly_token()?.into()); + let use_tree_list = lhs_tl.insert_children(pos, to_insert); + Some(lhs.with_use_tree_list(use_tree_list)) } -fn collect_path_segments_raw( - segments: &mut Vec, - mut path: ast::Path, -) -> Option { - let oldlen = segments.len(); +/// Traverses both paths until they differ, returning the common prefix of both. +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 { - let mut children = path.syntax().children_with_tokens(); - let (first, second, third) = ( - children.next().map(|n| (n.clone(), n.kind())), - children.next().map(|n| (n.clone(), n.kind())), - children.next().map(|n| (n.clone(), n.kind())), - ); - match (first, second, third) { - (Some((subpath, PATH)), Some((_, T![::])), Some((segment, PATH_SEGMENT))) => { - path = ast::Path::cast(subpath.as_node()?.clone())?; - segments.push(ast::PathSegment::cast(segment.as_node()?.clone())?); - } - (Some((segment, PATH_SEGMENT)), _, _) => { - segments.push(ast::PathSegment::cast(segment.as_node()?.clone())?); - break; + 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().zip(rhs_curr.parent_path()) { + Some((lhs, rhs)) => { + lhs_curr = lhs; + rhs_curr = rhs; } - (_, _, _) => return None, + _ => break, } } - // We need to reverse only the new added segments - let only_new_segments = segments.split_at_mut(oldlen).1; - only_new_segments.reverse(); - Some(segments.len() - oldlen) + + res } -fn fmt_segments_raw(segments: &[SmolStr], buf: &mut String) { - let mut iter = segments.iter(); - if let Some(s) = iter.next() { - buf.push_str(s); - } - for s in iter { - buf.push_str("::"); - buf.push_str(s); - } +/// What type of merges are allowed. +#[derive(Copy, Clone, PartialEq, Eq)] +pub enum MergeBehaviour { + /// Merge everything together creating deeply nested imports. + Full, + /// Only merge the last import level, doesn't allow import nesting. + Last, } -/// Returns the number of common segments. -fn compare_path_segments(left: &[SmolStr], right: &[ast::PathSegment]) -> usize { - left.iter().zip(right).take_while(|(l, r)| compare_path_segment(l, r)).count() +#[derive(Eq, PartialEq, PartialOrd, Ord)] +enum ImportGroup { + // the order here defines the order of new group inserts + Std, + ExternCrate, + ThisCrate, + ThisModule, + SuperModule, } -fn compare_path_segment(a: &SmolStr, b: &ast::PathSegment) -> bool { - if let Some(kb) = b.kind() { - match kb { - ast::PathSegmentKind::Name(nameref_b) => a == nameref_b.text(), - ast::PathSegmentKind::SelfKw => a == "self", - ast::PathSegmentKind::SuperKw => a == "super", - ast::PathSegmentKind::CrateKw => a == "crate", - ast::PathSegmentKind::Type { .. } => false, // not allowed in imports +impl ImportGroup { + fn new(path: &ast::Path) -> ImportGroup { + let default = ImportGroup::ExternCrate; + + let first_segment = match first_segment(path) { + Some(it) => it, + None => return default, + }; + + let kind = first_segment.kind().unwrap_or(PathSegmentKind::SelfKw); + match kind { + PathSegmentKind::SelfKw => ImportGroup::ThisModule, + PathSegmentKind::SuperKw => ImportGroup::SuperModule, + PathSegmentKind::CrateKw => ImportGroup::ThisCrate, + PathSegmentKind::Name(name) => match name.text().as_str() { + "std" => ImportGroup::Std, + "core" => ImportGroup::Std, + // FIXME: can be ThisModule as well + _ => ImportGroup::ExternCrate, + }, + PathSegmentKind::Type { .. } => unreachable!(), } - } else { - false } } -fn compare_path_segment_with_name(a: &SmolStr, b: &ast::Name) -> bool { - a == b.text() +fn first_segment(path: &ast::Path) -> Option { + first_path(path).segment() } -#[derive(Clone, Debug)] -enum ImportAction { - Nothing, - // Add a brand new use statement. - AddNewUse { - anchor: Option, // anchor node - add_after_anchor: bool, - }, - - // To split an existing use statement creating a nested import. - AddNestedImport { - // how may segments matched with the target path - common_segments: usize, - path_to_split: ast::Path, - // the first segment of path_to_split we want to add into the new nested list - first_segment_to_split: Option, - // Wether to add 'self' in addition to the target path - add_self: bool, - }, - // To add the target path to an existing nested import tree list. - AddInTreeList { - common_segments: usize, - // The UseTreeList where to add the target path - tree_list: ast::UseTreeList, - add_self: bool, - }, +fn first_path(path: &ast::Path) -> ast::Path { + successors(Some(path.clone()), ast::Path::qualifier).last().unwrap() } -impl ImportAction { - fn add_new_use(anchor: Option, add_after_anchor: bool) -> Self { - ImportAction::AddNewUse { anchor, add_after_anchor } +fn segment_iter(path: &ast::Path) -> impl Iterator + Clone { + path.syntax().children().flat_map(ast::PathSegment::cast) +} + +#[derive(PartialEq, Eq)] +enum AddBlankLine { + Before, + After, + AfterTwice, +} + +fn find_insert_position( + scope: &SyntaxNode, + insert_path: ast::Path, +) -> (InsertPosition, AddBlankLine) { + let group = ImportGroup::new(&insert_path); + let path_node_iter = scope + .children() + .filter_map(|node| ast::Use::cast(node.clone()).zip(Some(node))) + .flat_map(|(use_, node)| use_.use_tree().and_then(|tree| tree.path()).zip(Some(node))); + // Iterator that discards anything thats not in the required grouping + // This implementation allows the user to rearrange their import groups as this only takes the first group that fits + let group_iter = path_node_iter + .clone() + .skip_while(|(path, _)| ImportGroup::new(path) != group) + .take_while(|(path, _)| ImportGroup::new(path) == group); + + let segments = segment_iter(&insert_path); + // track the last element we iterated over, if this is still None after the iteration then that means we never iterated in the first place + let mut last = None; + // find the element that would come directly after our new import + let post_insert = + group_iter.inspect(|(_, node)| last = Some(node.clone())).find(|(path, _)| { + let check_segments = segment_iter(&path); + segments + .clone() + .zip(check_segments) + .flat_map(|(seg, seg2)| seg.name_ref().zip(seg2.name_ref())) + .all(|(l, r)| l.text() <= r.text()) + }); + match post_insert { + // insert our import before that element + Some((_, node)) => (InsertPosition::Before(node.into()), AddBlankLine::After), + // there is no element after our new import, so append it to the end of the group + None => match last { + Some(node) => (InsertPosition::After(node.into()), AddBlankLine::Before), + // the group we were looking for actually doesnt exist, so insert + None => { + // similar concept here to the `last` from above + let mut last = None; + // find the group that comes after where we want to insert + let post_group = path_node_iter + .inspect(|(_, node)| last = Some(node.clone())) + .find(|(p, _)| ImportGroup::new(p) > group); + match post_group { + Some((_, node)) => { + (InsertPosition::Before(node.into()), AddBlankLine::AfterTwice) + } + // there is no such group, so append after the last one + None => match last { + Some(node) => (InsertPosition::After(node.into()), AddBlankLine::Before), + // there are no imports in this file at all + None => (InsertPosition::First, AddBlankLine::AfterTwice), + }, + } + } + }, } +} - fn add_nested_import( - common_segments: usize, - path_to_split: ast::Path, - first_segment_to_split: Option, - add_self: bool, - ) -> Self { - ImportAction::AddNestedImport { - common_segments, - path_to_split, - first_segment_to_split, - add_self, - } +#[cfg(test)] +mod tests { + use super::*; + + use test_utils::assert_eq_text; + + #[test] + fn insert_start() { + check_none( + "std::bar::A", + r"use std::bar::B; +use std::bar::D; +use std::bar::F; +use std::bar::G;", + r"use std::bar::A; +use std::bar::B; +use std::bar::D; +use std::bar::F; +use std::bar::G;", + ) } - fn add_in_tree_list( - common_segments: usize, - tree_list: ast::UseTreeList, - add_self: bool, - ) -> Self { - ImportAction::AddInTreeList { common_segments, tree_list, add_self } + #[test] + fn insert_middle() { + check_none( + "std::bar::E", + r"use std::bar::A; +use std::bar::D; +use std::bar::F; +use std::bar::G;", + r"use std::bar::A; +use std::bar::D; +use std::bar::E; +use std::bar::F; +use std::bar::G;", + ) } - fn better(left: ImportAction, right: ImportAction) -> ImportAction { - if left.is_better(&right) { - left - } else { - right - } + #[test] + fn insert_end() { + check_none( + "std::bar::Z", + r"use std::bar::A; +use std::bar::D; +use std::bar::F; +use std::bar::G;", + r"use std::bar::A; +use std::bar::D; +use std::bar::F; +use std::bar::G; +use std::bar::Z;", + ) } - fn is_better(&self, other: &ImportAction) -> bool { - match (self, other) { - (ImportAction::Nothing, _) => true, - (ImportAction::AddInTreeList { .. }, ImportAction::Nothing) => false, - ( - ImportAction::AddNestedImport { common_segments: n, .. }, - ImportAction::AddInTreeList { common_segments: m, .. }, - ) - | ( - ImportAction::AddInTreeList { common_segments: n, .. }, - ImportAction::AddNestedImport { common_segments: m, .. }, - ) - | ( - ImportAction::AddInTreeList { common_segments: n, .. }, - ImportAction::AddInTreeList { common_segments: m, .. }, - ) - | ( - ImportAction::AddNestedImport { common_segments: n, .. }, - ImportAction::AddNestedImport { common_segments: m, .. }, - ) => n > m, - (ImportAction::AddInTreeList { .. }, _) => true, - (ImportAction::AddNestedImport { .. }, ImportAction::Nothing) => false, - (ImportAction::AddNestedImport { .. }, _) => true, - (ImportAction::AddNewUse { .. }, _) => false, - } + #[test] + fn insert_middle_pnested() { + check_none( + "std::bar::E", + r"use std::bar::A; +use std::bar::{D, Z}; // example of weird imports due to user +use std::bar::F; +use std::bar::G;", + r"use std::bar::A; +use std::bar::E; +use std::bar::{D, Z}; // example of weird imports due to user +use std::bar::F; +use std::bar::G;", + ) } -} -// Find out the best ImportAction to import target path against current_use_tree. -// If current_use_tree has a nested import the function gets called recursively on every UseTree inside a UseTreeList. -fn walk_use_tree_for_best_action( - current_path_segments: &mut Vec, // buffer containing path segments - current_parent_use_tree_list: Option, // will be Some value if we are in a nested import - current_use_tree: ast::UseTree, // the use tree we are currently examinating - target: &[SmolStr], // the path we want to import -) -> ImportAction { - // We save the number of segments in the buffer so we can restore the correct segments - // before returning. Recursive call will add segments so we need to delete them. - let prev_len = current_path_segments.len(); - - let tree_list = current_use_tree.use_tree_list(); - let alias = current_use_tree.rename(); - - let path = match current_use_tree.path() { - Some(path) => path, - None => { - // If the use item don't have a path, it means it's broken (syntax error) - return ImportAction::add_new_use( - current_use_tree - .syntax() - .ancestors() - .find_map(ast::Use::cast) - .map(|it| it.syntax().clone()), - true, - ); - } - }; + #[test] + fn insert_middle_groups() { + check_none( + "foo::bar::G", + r"use std::bar::A; +use std::bar::D; + +use foo::bar::F; +use foo::bar::H;", + r"use std::bar::A; +use std::bar::D; + +use foo::bar::F; +use foo::bar::G; +use foo::bar::H;", + ) + } - // This can happen only if current_use_tree is a direct child of a UseItem - if let Some(name) = alias.and_then(|it| it.name()) { - if compare_path_segment_with_name(&target[0], &name) { - return ImportAction::Nothing; - } + #[test] + fn insert_first_matching_group() { + check_none( + "foo::bar::G", + r"use foo::bar::A; +use foo::bar::D; + +use std; + +use foo::bar::F; +use foo::bar::H;", + r"use foo::bar::A; +use foo::bar::D; +use foo::bar::G; + +use std; + +use foo::bar::F; +use foo::bar::H;", + ) } - collect_path_segments_raw(current_path_segments, path.clone()); - - // We compare only the new segments added in the line just above. - // The first prev_len segments were already compared in 'parent' recursive calls. - let left = target.split_at(prev_len).1; - let right = current_path_segments.split_at(prev_len).1; - let common = compare_path_segments(left, &right); - let mut action = match common { - 0 => ImportAction::add_new_use( - // e.g: target is std::fmt and we can have - // use foo::bar - // We add a brand new use statement - current_use_tree - .syntax() - .ancestors() - .find_map(ast::Use::cast) - .map(|it| it.syntax().clone()), - true, - ), - common if common == left.len() && left.len() == right.len() => { - // e.g: target is std::fmt and we can have - // 1- use std::fmt; - // 2- use std::fmt::{ ... } - if let Some(list) = tree_list { - // In case 2 we need to add self to the nested list - // unless it's already there - let has_self = list.use_trees().map(|it| it.path()).any(|p| { - p.and_then(|it| it.segment()) - .and_then(|it| it.kind()) - .filter(|k| *k == ast::PathSegmentKind::SelfKw) - .is_some() - }); - - if has_self { - ImportAction::Nothing - } else { - ImportAction::add_in_tree_list(current_path_segments.len(), list, true) - } - } else { - // Case 1 - ImportAction::Nothing - } - } - common if common != left.len() && left.len() == right.len() => { - // e.g: target is std::fmt and we have - // use std::io; - // We need to split. - let segments_to_split = current_path_segments.split_at(prev_len + common).1; - ImportAction::add_nested_import( - prev_len + common, - path, - Some(segments_to_split[0].clone()), - false, - ) - } - common if common == right.len() && left.len() > right.len() => { - // e.g: target is std::fmt and we can have - // 1- use std; - // 2- use std::{ ... }; - - // fallback action - let mut better_action = ImportAction::add_new_use( - current_use_tree - .syntax() - .ancestors() - .find_map(ast::Use::cast) - .map(|it| it.syntax().clone()), - true, - ); - if let Some(list) = tree_list { - // Case 2, check recursively if the path is already imported in the nested list - for u in list.use_trees() { - let child_action = walk_use_tree_for_best_action( - current_path_segments, - Some(list.clone()), - u, - target, - ); - if child_action.is_better(&better_action) { - better_action = child_action; - if let ImportAction::Nothing = better_action { - return better_action; - } - } - } - } else { - // Case 1, split adding self - better_action = ImportAction::add_nested_import(prev_len + common, path, None, true) - } - better_action - } - common if common == left.len() && left.len() < right.len() => { - // e.g: target is std::fmt and we can have - // use std::fmt::Debug; - let segments_to_split = current_path_segments.split_at(prev_len + common).1; - ImportAction::add_nested_import( - prev_len + common, - path, - Some(segments_to_split[0].clone()), - true, - ) - } - common if common < left.len() && common < right.len() => { - // e.g: target is std::fmt::nested::Debug - // use std::fmt::Display - let segments_to_split = current_path_segments.split_at(prev_len + common).1; - ImportAction::add_nested_import( - prev_len + common, - path, - Some(segments_to_split[0].clone()), - false, - ) - } - _ => unreachable!(), - }; + #[test] + fn insert_missing_group() { + check_none( + "std::fmt", + r"use foo::bar::A; +use foo::bar::D;", + r"use std::fmt; + +use foo::bar::A; +use foo::bar::D;", + ) + } - // If we are inside a UseTreeList adding a use statement become adding to the existing - // tree list. - action = match (current_parent_use_tree_list, action.clone()) { - (Some(use_tree_list), ImportAction::AddNewUse { .. }) => { - ImportAction::add_in_tree_list(prev_len, use_tree_list, false) - } - (_, _) => action, - }; + #[test] + fn insert_no_imports() { + check_full( + "foo::bar", + "fn main() {}", + r"use foo::bar; - // We remove the segments added - current_path_segments.truncate(prev_len); - action -} +fn main() {}", + ) + } -fn best_action_for_target( - container: SyntaxNode, - anchor: SyntaxNode, - target: &[SmolStr], -) -> ImportAction { - let mut storage = Vec::with_capacity(16); // this should be the only allocation - let best_action = container - .children() - .filter_map(ast::Use::cast) - .filter(|u| u.visibility().is_none()) - .filter_map(|it| it.use_tree()) - .map(|u| walk_use_tree_for_best_action(&mut storage, None, u, target)) - .fold(None, |best, a| match best { - Some(best) => Some(ImportAction::better(best, a)), - None => Some(a), - }); + #[test] + fn insert_empty_file() { + // empty files will get two trailing newlines + // this is due to the test case insert_no_imports above + check_full( + "foo::bar", + "", + r"use foo::bar; + +", + ) + } - match best_action { - Some(action) => action, - None => { - // We have no action and no UseItem was found in container so we find - // another item and we use it as anchor. - // If there are no items above, we choose the target path itself as anchor. - // todo: we should include even whitespace blocks as anchor candidates - let anchor = container.children().next().or_else(|| Some(anchor)); + #[test] + fn adds_std_group() { + check_full( + "std::fmt::Debug", + r"use stdx;", + r"use std::fmt::Debug; - let add_after_anchor = anchor - .clone() - .and_then(ast::Attr::cast) - .map(|attr| attr.kind() == ast::AttrKind::Inner) - .unwrap_or(false); - ImportAction::add_new_use(anchor, add_after_anchor) - } +use stdx;", + ) } -} -fn make_assist(action: &ImportAction, target: &[SmolStr], edit: &mut TextEditBuilder) { - match action { - ImportAction::AddNewUse { anchor, add_after_anchor } => { - make_assist_add_new_use(anchor, *add_after_anchor, target, edit) - } - ImportAction::AddInTreeList { common_segments, tree_list, add_self } => { - // We know that the fist n segments already exists in the use statement we want - // to modify, so we want to add only the last target.len() - n segments. - let segments_to_add = target.split_at(*common_segments).1; - make_assist_add_in_tree_list(tree_list, segments_to_add, *add_self, edit) - } - ImportAction::AddNestedImport { - common_segments, - path_to_split, - first_segment_to_split, - add_self, - } => { - let segments_to_add = target.split_at(*common_segments).1; - make_assist_add_nested_import( - path_to_split, - first_segment_to_split, - segments_to_add, - *add_self, - edit, - ) - } - _ => {} + #[test] + fn merges_groups() { + check_last("std::io", r"use std::fmt;", r"use std::{fmt, io};") } -} -fn make_assist_add_new_use( - anchor: &Option, - after: bool, - target: &[SmolStr], - edit: &mut TextEditBuilder, -) { - if let Some(anchor) = anchor { - let indent = leading_indent(anchor); - let mut buf = String::new(); - if after { - buf.push_str("\n"); - if let Some(spaces) = &indent { - buf.push_str(spaces); - } - } - buf.push_str("use "); - fmt_segments_raw(target, &mut buf); - buf.push_str(";"); - if !after { - buf.push_str("\n\n"); - if let Some(spaces) = &indent { - buf.push_str(&spaces); - } - } - let position = if after { anchor.text_range().end() } else { anchor.text_range().start() }; - edit.insert(position, buf); + #[test] + fn merges_groups_last() { + check_last( + "std::io", + r"use std::fmt::{Result, Display};", + r"use std::fmt::{Result, Display}; +use std::io;", + ) } -} -fn make_assist_add_in_tree_list( - tree_list: &ast::UseTreeList, - target: &[SmolStr], - add_self: bool, - edit: &mut TextEditBuilder, -) { - let last = tree_list.use_trees().last(); - if let Some(last) = last { - let mut buf = String::new(); - let comma = last.syntax().siblings(Direction::Next).find(|n| n.kind() == T![,]); - let offset = if let Some(comma) = comma { - comma.text_range().end() - } else { - buf.push_str(","); - last.syntax().text_range().end() - }; - if add_self { - buf.push_str(" self") - } else { - buf.push_str(" "); - } - fmt_segments_raw(target, &mut buf); - edit.insert(offset, buf); - } else { + #[test] + fn merges_groups2() { + check_full( + "std::io", + r"use std::fmt::{Result, Display};", + r"use std::{fmt::{Result, Display}, io};", + ) } -} -fn make_assist_add_nested_import( - path: &ast::Path, - first_segment_to_split: &Option, - target: &[SmolStr], - add_self: bool, - edit: &mut TextEditBuilder, -) { - let use_tree = path.syntax().ancestors().find_map(ast::UseTree::cast); - if let Some(use_tree) = use_tree { - let (start, add_colon_colon) = if let Some(first_segment_to_split) = first_segment_to_split - { - (first_segment_to_split.syntax().text_range().start(), false) - } else { - (use_tree.syntax().text_range().end(), true) - }; - let end = use_tree.syntax().text_range().end(); + #[test] + fn skip_merges_groups_pub() { + check_full( + "std::io", + r"pub use std::fmt::{Result, Display};", + r"pub use std::fmt::{Result, Display}; +use std::io;", + ) + } - let mut buf = String::new(); - if add_colon_colon { - buf.push_str("::"); - } - buf.push_str("{"); - if add_self { - buf.push_str("self, "); - } - fmt_segments_raw(target, &mut buf); - if !target.is_empty() { - buf.push_str(", "); - } - edit.insert(start, buf); - edit.insert(end, "}".to_string()); + #[test] + fn merges_groups_self() { + check_full("std::fmt::Debug", r"use std::fmt;", r"use std::fmt::{self, Debug};") } -} -/// If the node is on the beginning of the line, calculate indent. -fn leading_indent(node: &SyntaxNode) -> Option { - for token in prev_tokens(node.first_token()?) { - if let Some(ws) = ast::Whitespace::cast(token.clone()) { - let ws_text = ws.text(); - if let Some(pos) = ws_text.rfind('\n') { - return Some(ws_text[pos + 1..].into()); - } - } - if token.text().contains('\n') { - break; - } + fn check( + path: &str, + ra_fixture_before: &str, + ra_fixture_after: &str, + mb: Option, + ) { + let file = ast::SourceFile::parse(ra_fixture_before).tree().syntax().clone(); + let path = ast::SourceFile::parse(&format!("use {};", path)) + .tree() + .syntax() + .descendants() + .find_map(ast::Path::cast) + .unwrap(); + + let result = insert_use(file, path, mb).to_string(); + assert_eq_text!(&result, ra_fixture_after); + } + + fn check_full(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { + check(path, ra_fixture_before, ra_fixture_after, Some(MergeBehaviour::Full)) } - return None; - fn prev_tokens(token: SyntaxToken) -> impl Iterator { - successors(token.prev_token(), |token| token.prev_token()) + + fn check_last(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { + check(path, ra_fixture_before, ra_fixture_after, Some(MergeBehaviour::Last)) + } + + fn check_none(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { + check(path, ra_fixture_before, ra_fixture_after, None) } } -- cgit v1.2.3 From 903c7eb2e55ba403f7174110dfdde81184d6ed25 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 2 Sep 2020 15:50:29 +0200 Subject: Add more import insertion tests --- crates/assists/src/utils/insert_use.rs | 71 ++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 8ee5e0c9c..8563b16ab 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -114,8 +114,7 @@ pub fn try_merge_trees( let lhs_tl = lhs.use_tree_list()?; let rhs_tl = rhs.use_tree_list()?; - // if we are only allowed to merge the last level check if the paths are only one level deep - // FIXME: This shouldn't work yet i think + // if we are only allowed to merge the last level check if the split off paths are only one level deep if merge_behaviour == MergeBehaviour::Last && use_tree_list_is_nested(&lhs_tl) || use_tree_list_is_nested(&rhs_tl) { @@ -463,7 +462,7 @@ use std::io;", } #[test] - fn merges_groups2() { + fn merges_groups_full() { check_full( "std::io", r"use std::fmt::{Result, Display};", @@ -471,6 +470,61 @@ use std::io;", ) } + #[test] + fn merges_groups_long_full() { + check_full( + "std::foo::bar::Baz", + r"use std::foo::bar::Qux;", + r"use std::foo::bar::{Baz, Qux};", + ) + } + + #[test] + fn merges_groups_long_last() { + check_last( + "std::foo::bar::Baz", + r"use std::foo::bar::Qux;", + r"use std::foo::bar::{Baz, Qux};", + ) + } + + #[test] + fn merges_groups_long_full_list() { + check_full( + "std::foo::bar::Baz", + r"use std::foo::bar::{Qux, Quux};", + r"use std::foo::bar::{Baz, Quux, Qux};", + ) + } + + #[test] + fn merges_groups_long_last_list() { + check_last( + "std::foo::bar::Baz", + r"use std::foo::bar::{Qux, Quux};", + r"use std::foo::bar::{Baz, Quux, Qux};", + ) + } + + #[test] + fn merges_groups_long_full_nested() { + check_full( + "std::foo::bar::Baz", + r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", + r"use std::foo::bar::{Baz, quux::{Fez, Fizz}, Qux};", + ) + } + + #[test] + fn merges_groups_long_last_nested() { + check_last( + "std::foo::bar::Baz", + r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", + r"use std::foo::bar::Baz; +use std::foo::bar::{quux::{Fez, Fizz}, Qux};", + ) + } + #[test] fn skip_merges_groups_pub() { check_full( @@ -481,6 +535,17 @@ use std::io;", ) } + // should this be a thing? + #[test] + fn split_merge() { + check_last( + "std::fmt::Result", + r"use std::{fmt, io};", + r"use std::fmt::Result; +use std::io;", + ) + } + #[test] fn merges_groups_self() { check_full("std::fmt::Debug", r"use std::fmt;", r"use std::fmt::{self, Debug};") -- cgit v1.2.3 From 74186d3ae7265b691f00b562f802ed5bdcda5c89 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 2 Sep 2020 19:22:23 +0200 Subject: Tidy up tests and apply suggested changes --- crates/assists/src/utils/insert_use.rs | 179 ++++++++++++++++++++------------- 1 file changed, 107 insertions(+), 72 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 8563b16ab..dbe2dfdcb 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -34,14 +34,15 @@ pub(crate) fn insert_use_statement( insert_use(position.clone(), make::path_from_text(path_to_import), Some(MergeBehaviour::Full)); } +/// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. pub fn insert_use( where_: SyntaxNode, path: ast::Path, - merge_behaviour: Option, + merge: Option, ) -> SyntaxNode { let use_item = make::use_(make::use_tree(path.clone(), None, None, false)); // merge into existing imports if possible - if let Some(mb) = merge_behaviour { + if let Some(mb) = merge { for existing_use in where_.children().filter_map(ast::Use::cast) { if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { let to_delete: SyntaxElement = existing_use.syntax().clone().into(); @@ -59,17 +60,24 @@ pub fn insert_use( let to_insert: Vec = { let mut buf = Vec::new(); - if add_blank == AddBlankLine::Before { - buf.push(make::tokens::single_newline().into()); + match add_blank { + AddBlankLine::Before => buf.push(make::tokens::single_newline().into()), + AddBlankLine::BeforeTwice => { + buf.push(make::tokens::single_newline().into()); + buf.push(make::tokens::single_newline().into()); + } + _ => (), } buf.push(use_item.syntax().clone().into()); - if add_blank == AddBlankLine::After { - buf.push(make::tokens::single_newline().into()); - } else if add_blank == AddBlankLine::AfterTwice { - buf.push(make::tokens::single_newline().into()); - buf.push(make::tokens::single_newline().into()); + match add_blank { + AddBlankLine::After => buf.push(make::tokens::single_newline().into()), + AddBlankLine::AfterTwice => { + buf.push(make::tokens::single_newline().into()); + buf.push(make::tokens::single_newline().into()); + } + _ => (), } buf @@ -83,8 +91,8 @@ fn try_merge_imports( new: &ast::Use, merge_behaviour: MergeBehaviour, ) -> Option { - // dont merge into re-exports - if old.visibility().map(|vis| vis.pub_token()).is_some() { + // don't merge into re-exports + if old.visibility().and_then(|vis| vis.pub_token()).is_some() { return None; } let old_tree = old.use_tree()?; @@ -115,8 +123,8 @@ pub fn try_merge_trees( let rhs_tl = rhs.use_tree_list()?; // if we are only allowed to merge the last level check if the split off paths are only one level deep - if merge_behaviour == MergeBehaviour::Last && use_tree_list_is_nested(&lhs_tl) - || use_tree_list_is_nested(&rhs_tl) + if merge_behaviour == MergeBehaviour::Last + && (use_tree_list_is_nested(&lhs_tl) || use_tree_list_is_nested(&rhs_tl)) { return None; } @@ -124,18 +132,15 @@ pub fn try_merge_trees( let should_insert_comma = lhs_tl .r_curly_token() .and_then(|it| skip_trivia_token(it.prev_token()?, Direction::Prev)) - .map(|it| it.kind() != T![,]) - .unwrap_or(true); + .map(|it| it.kind()) + != Some(T![,]); 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_tl - .syntax() - .children_with_tokens() - .filter(|it| it.kind() != T!['{'] && it.kind() != T!['}']), + rhs_tl.syntax().children_with_tokens().filter(|it| !matches!(it.kind(), T!['{'] | T!['}'])), ); let pos = InsertPosition::Before(lhs_tl.r_curly_token()?.into()); let use_tree_list = lhs_tl.insert_children(pos, to_insert); @@ -225,6 +230,7 @@ fn segment_iter(path: &ast::Path) -> impl Iterator + Cl #[derive(PartialEq, Eq)] enum AddBlankLine { Before, + BeforeTwice, After, AfterTwice, } @@ -278,7 +284,9 @@ fn find_insert_position( } // there is no such group, so append after the last one None => match last { - Some(node) => (InsertPosition::After(node.into()), AddBlankLine::Before), + Some(node) => { + (InsertPosition::After(node.into()), AddBlankLine::BeforeTwice) + } // there are no imports in this file at all None => (InsertPosition::First, AddBlankLine::AfterTwice), }, @@ -297,12 +305,14 @@ mod tests { #[test] fn insert_start() { check_none( - "std::bar::A", - r"use std::bar::B; + "std::bar::AA", + r" +use std::bar::B; use std::bar::D; use std::bar::F; use std::bar::G;", - r"use std::bar::A; + r" +use std::bar::AA; use std::bar::B; use std::bar::D; use std::bar::F; @@ -313,14 +323,16 @@ use std::bar::G;", #[test] fn insert_middle() { check_none( - "std::bar::E", - r"use std::bar::A; + "std::bar::EE", + r" +use std::bar::A; use std::bar::D; use std::bar::F; use std::bar::G;", - r"use std::bar::A; + r" +use std::bar::A; use std::bar::D; -use std::bar::E; +use std::bar::EE; use std::bar::F; use std::bar::G;", ) @@ -329,29 +341,33 @@ use std::bar::G;", #[test] fn insert_end() { check_none( - "std::bar::Z", - r"use std::bar::A; + "std::bar::ZZ", + r" +use std::bar::A; use std::bar::D; use std::bar::F; use std::bar::G;", - r"use std::bar::A; + r" +use std::bar::A; use std::bar::D; use std::bar::F; use std::bar::G; -use std::bar::Z;", +use std::bar::ZZ;", ) } #[test] - fn insert_middle_pnested() { + fn insert_middle_nested() { check_none( - "std::bar::E", - r"use std::bar::A; + "std::bar::EE", + r" +use std::bar::A; use std::bar::{D, Z}; // example of weird imports due to user use std::bar::F; use std::bar::G;", - r"use std::bar::A; -use std::bar::E; + r" +use std::bar::A; +use std::bar::EE; use std::bar::{D, Z}; // example of weird imports due to user use std::bar::F; use std::bar::G;", @@ -361,17 +377,19 @@ use std::bar::G;", #[test] fn insert_middle_groups() { check_none( - "foo::bar::G", - r"use std::bar::A; + "foo::bar::GG", + r" +use std::bar::A; use std::bar::D; use foo::bar::F; use foo::bar::H;", - r"use std::bar::A; + r" +use std::bar::A; use std::bar::D; use foo::bar::F; -use foo::bar::G; +use foo::bar::GG; use foo::bar::H;", ) } @@ -379,17 +397,19 @@ use foo::bar::H;", #[test] fn insert_first_matching_group() { check_none( - "foo::bar::G", - r"use foo::bar::A; + "foo::bar::GG", + r" +use foo::bar::A; use foo::bar::D; use std; use foo::bar::F; use foo::bar::H;", - r"use foo::bar::A; + r" +use foo::bar::A; use foo::bar::D; -use foo::bar::G; +use foo::bar::GG; use std; @@ -399,18 +419,35 @@ use foo::bar::H;", } #[test] - fn insert_missing_group() { + fn insert_missing_group_std() { check_none( "std::fmt", - r"use foo::bar::A; + r" +use foo::bar::A; use foo::bar::D;", - r"use std::fmt; + r" +use std::fmt; use foo::bar::A; use foo::bar::D;", ) } + #[test] + fn insert_missing_group_self() { + check_none( + "self::fmt", + r" +use foo::bar::A; +use foo::bar::D;", + r" +use foo::bar::A; +use foo::bar::D; + +use self::fmt;", + ) + } + #[test] fn insert_no_imports() { check_full( @@ -436,23 +473,12 @@ fn main() {}", } #[test] - fn adds_std_group() { - check_full( - "std::fmt::Debug", - r"use stdx;", - r"use std::fmt::Debug; - -use stdx;", - ) - } - - #[test] - fn merges_groups() { + fn merge_groups() { check_last("std::io", r"use std::fmt;", r"use std::{fmt, io};") } #[test] - fn merges_groups_last() { + fn merge_groups_last() { check_last( "std::io", r"use std::fmt::{Result, Display};", @@ -462,7 +488,7 @@ use std::io;", } #[test] - fn merges_groups_full() { + fn merge_groups_full() { check_full( "std::io", r"use std::fmt::{Result, Display};", @@ -471,7 +497,7 @@ use std::io;", } #[test] - fn merges_groups_long_full() { + fn merge_groups_long_full() { check_full( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", @@ -480,7 +506,7 @@ use std::io;", } #[test] - fn merges_groups_long_last() { + fn merge_groups_long_last() { check_last( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", @@ -489,7 +515,7 @@ use std::io;", } #[test] - fn merges_groups_long_full_list() { + fn merge_groups_long_full_list() { check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", @@ -498,7 +524,7 @@ use std::io;", } #[test] - fn merges_groups_long_last_list() { + fn merge_groups_long_last_list() { check_last( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", @@ -507,7 +533,7 @@ use std::io;", } #[test] - fn merges_groups_long_full_nested() { + fn merge_groups_long_full_nested() { check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", @@ -516,7 +542,7 @@ use std::io;", } #[test] - fn merges_groups_long_last_nested() { + fn merge_groups_long_last_nested() { check_last( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", @@ -526,7 +552,7 @@ use std::foo::bar::{quux::{Fez, Fizz}, Qux};", } #[test] - fn skip_merges_groups_pub() { + fn merge_groups_skip_pub() { check_full( "std::io", r"pub use std::fmt::{Result, Display};", @@ -535,22 +561,31 @@ use std::io;", ) } - // should this be a thing? #[test] - fn split_merge() { + #[ignore] // FIXME: Support this + fn split_out_merge() { check_last( "std::fmt::Result", r"use std::{fmt, io};", - r"use std::fmt::Result; + r"use std::{self, fmt::Result}; use std::io;", ) } #[test] - fn merges_groups_self() { + fn merge_groups_self() { check_full("std::fmt::Debug", r"use std::fmt;", r"use std::fmt::{self, Debug};") } + #[test] + fn merge_self_glob() { + check_full( + "token::TokenKind", + r"use token::TokenKind::*;", + r"use token::TokenKind::{self::*, self};", + ) + } + fn check( path: &str, ra_fixture_before: &str, -- cgit v1.2.3 From 07ff9eeca8136563e7d1c09ec82cff9a67ed975a Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 2 Sep 2020 19:34:01 +0200 Subject: Use mark to check that paths that are too long will not be merged --- crates/assists/src/utils/insert_use.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index dbe2dfdcb..8842068e6 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -10,6 +10,7 @@ use syntax::{ }; use crate::assist_context::AssistContext; +use test_utils::mark; /// Determines the containing syntax node in which to insert a `use` statement affecting `position`. pub(crate) fn find_insert_use_container( @@ -126,6 +127,7 @@ pub fn try_merge_trees( if merge_behaviour == MergeBehaviour::Last && (use_tree_list_is_nested(&lhs_tl) || use_tree_list_is_nested(&rhs_tl)) { + mark::hit!(test_last_merge_too_long); return None; } @@ -586,6 +588,17 @@ use std::io;", ) } + #[test] + fn merge_last_too_long() { + mark::check!(test_last_merge_too_long); + check_last( + "foo::bar", + r"use foo::bar::baz::Qux;", + r"use foo::bar::baz::Qux; +use foo::bar;", + ); + } + fn check( path: &str, ra_fixture_before: &str, -- cgit v1.2.3 From 952f3856822d471cf5062e82f544c901c385f3ae Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 3 Sep 2020 12:36:16 +0200 Subject: Impl make::blank_line --- crates/assists/src/utils.rs | 2 +- crates/assists/src/utils/insert_use.rs | 33 ++++++++------------------------- 2 files changed, 9 insertions(+), 26 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils.rs b/crates/assists/src/utils.rs index daa7b64f7..07afa64ff 100644 --- a/crates/assists/src/utils.rs +++ b/crates/assists/src/utils.rs @@ -16,7 +16,7 @@ use syntax::{ use crate::assist_config::SnippetCap; -pub(crate) use insert_use::{find_insert_use_container, insert_use_statement}; +pub(crate) use insert_use::{find_insert_use_container, insert_use, MergeBehaviour}; pub(crate) fn unwrap_trivial_block(block: ast::BlockExpr) -> ast::Expr { extract_trivial_expression(&block) diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 8842068e6..030a5a935 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -9,13 +9,12 @@ use syntax::{ Direction, InsertPosition, SyntaxElement, SyntaxNode, T, }; -use crate::assist_context::AssistContext; use test_utils::mark; /// Determines the containing syntax node in which to insert a `use` statement affecting `position`. pub(crate) fn find_insert_use_container( position: &SyntaxNode, - ctx: &AssistContext, + ctx: &crate::assist_context::AssistContext, ) -> Option> { ctx.sema.ancestors_with_macros(position.clone()).find_map(|n| { if let Some(module) = ast::Module::cast(n.clone()) { @@ -25,19 +24,9 @@ pub(crate) fn find_insert_use_container( }) } -pub(crate) fn insert_use_statement( - // Ideally the position of the cursor, used to - position: &SyntaxNode, - path_to_import: &str, - ctx: &crate::assist_context::AssistContext, - builder: &mut text_edit::TextEditBuilder, -) { - insert_use(position.clone(), make::path_from_text(path_to_import), Some(MergeBehaviour::Full)); -} - /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. pub fn insert_use( - where_: SyntaxNode, + where_: &SyntaxNode, path: ast::Path, merge: Option, ) -> SyntaxNode { @@ -49,24 +38,21 @@ pub fn insert_use( let to_delete: SyntaxElement = existing_use.syntax().clone().into(); let to_delete = to_delete.clone()..=to_delete; let to_insert = iter::once(merged.syntax().clone().into()); - return algo::replace_children(&where_, to_delete, to_insert); + return algo::replace_children(where_, to_delete, to_insert); } } } // either we weren't allowed to merge or there is no import that fits the merge conditions // so look for the place we have to insert to - let (insert_position, add_blank) = find_insert_position(&where_, path); + let (insert_position, add_blank) = find_insert_position(where_, path); let to_insert: Vec = { let mut buf = Vec::new(); match add_blank { AddBlankLine::Before => buf.push(make::tokens::single_newline().into()), - AddBlankLine::BeforeTwice => { - buf.push(make::tokens::single_newline().into()); - buf.push(make::tokens::single_newline().into()); - } + AddBlankLine::BeforeTwice => buf.push(make::tokens::blank_line().into()), _ => (), } @@ -74,17 +60,14 @@ pub fn insert_use( match add_blank { AddBlankLine::After => buf.push(make::tokens::single_newline().into()), - AddBlankLine::AfterTwice => { - buf.push(make::tokens::single_newline().into()); - buf.push(make::tokens::single_newline().into()); - } + AddBlankLine::AfterTwice => buf.push(make::tokens::blank_line().into()), _ => (), } buf }; - algo::insert_children(&where_, insert_position, to_insert) + algo::insert_children(where_, insert_position, to_insert) } fn try_merge_imports( @@ -613,7 +596,7 @@ use foo::bar;", .find_map(ast::Path::cast) .unwrap(); - let result = insert_use(file, path, mb).to_string(); + let result = insert_use(&file, path, mb).to_string(); assert_eq_text!(&result, ra_fixture_after); } -- cgit v1.2.3 From c1925df7fc91a171925ecb59b9f1895ee59ce72f Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 3 Sep 2020 14:22:22 +0200 Subject: Replace insert_use_statement with the new insert_use --- crates/assists/src/handlers/auto_import.rs | 19 ++++--- .../handlers/extract_struct_from_enum_variant.rs | 20 ++++--- .../handlers/replace_qualified_name_with_use.rs | 65 +++++++++++----------- crates/assists/src/utils/insert_use.rs | 18 +++--- 4 files changed, 68 insertions(+), 54 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/auto_import.rs b/crates/assists/src/handlers/auto_import.rs index c4770f336..f3b4c5708 100644 --- a/crates/assists/src/handlers/auto_import.rs +++ b/crates/assists/src/handlers/auto_import.rs @@ -13,8 +13,11 @@ use syntax::{ }; use crate::{ - utils::insert_use_statement, AssistContext, AssistId, AssistKind, Assists, GroupLabel, + utils::{insert_use, MergeBehaviour}, + AssistContext, AssistId, AssistKind, Assists, GroupLabel, }; +use ast::make; +use insert_use::find_insert_use_container; // Assist: auto_import // @@ -44,6 +47,8 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> let range = ctx.sema.original_range(&auto_import_assets.syntax_under_caret).range; let group = auto_import_assets.get_import_group_message(); + let container = find_insert_use_container(&auto_import_assets.syntax_under_caret, ctx)?; + let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); for import in proposed_imports { acc.add_group( &group, @@ -51,12 +56,12 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> format!("Import `{}`", &import), range, |builder| { - insert_use_statement( - &auto_import_assets.syntax_under_caret, - &import.to_string(), - ctx, - builder.text_edit_builder(), + let new_syntax = insert_use( + &syntax, + make::path_from_text(&import.to_string()), + Some(MergeBehaviour::Full), ); + builder.replace(syntax.text_range(), new_syntax.to_string()) }, ); } @@ -358,7 +363,7 @@ mod tests { } ", r" - use PubMod::{PubStruct2, PubStruct1}; + use PubMod::{PubStruct1, PubStruct2}; struct Test { test: PubStruct2, diff --git a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs index 8ac20210a..d9c50f25f 100644 --- a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs @@ -10,9 +10,12 @@ use syntax::{ }; use crate::{ - assist_context::AssistBuilder, utils::insert_use_statement, AssistContext, AssistId, - AssistKind, Assists, + assist_context::AssistBuilder, + utils::{insert_use, MergeBehaviour}, + AssistContext, AssistId, AssistKind, Assists, }; +use ast::make; +use insert_use::find_insert_use_container; // Assist: extract_struct_from_enum_variant // @@ -107,12 +110,15 @@ fn insert_import( if let Some(mut mod_path) = mod_path { mod_path.segments.pop(); mod_path.segments.push(variant_hir_name.clone()); - insert_use_statement( - path.syntax(), - &mod_path.to_string(), - ctx, - builder.text_edit_builder(), + let container = find_insert_use_container(path.syntax(), ctx)?; + let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); + + let new_syntax = insert_use( + &syntax, + make::path_from_text(&mod_path.to_string()), + Some(MergeBehaviour::Full), ); + builder.replace(syntax.text_range(), new_syntax.to_string()) } Some(()) } diff --git a/crates/assists/src/handlers/replace_qualified_name_with_use.rs b/crates/assists/src/handlers/replace_qualified_name_with_use.rs index 470e5f8ff..56e85125d 100644 --- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs @@ -2,9 +2,10 @@ use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode, TextRang use test_utils::mark; use crate::{ - utils::{find_insert_use_container, insert_use_statement}, + utils::{find_insert_use_container, insert_use, MergeBehaviour}, AssistContext, AssistId, AssistKind, Assists, }; +use ast::make; // Assist: replace_qualified_name_with_use // @@ -32,7 +33,7 @@ pub(crate) fn replace_qualified_name_with_use( mark::hit!(dont_import_trivial_paths); return None; } - let path_to_import = path.to_string().clone(); + let path_to_import = path.to_string(); let path_to_import = match path.segment()?.generic_arg_list() { Some(generic_args) => { let generic_args_start = @@ -43,28 +44,24 @@ pub(crate) fn replace_qualified_name_with_use( }; let target = path.syntax().text_range(); + let container = find_insert_use_container(path.syntax(), ctx)?; + let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); acc.add( AssistId("replace_qualified_name_with_use", AssistKind::RefactorRewrite), "Replace qualified path with use", target, |builder| { - let container = match find_insert_use_container(path.syntax(), ctx) { - Some(c) => c, - None => return, - }; - insert_use_statement( - path.syntax(), - &path_to_import.to_string(), - ctx, - builder.text_edit_builder(), - ); - // Now that we've brought the name into scope, re-qualify all paths that could be // affected (that is, all paths inside the node we added the `use` to). let mut rewriter = SyntaxRewriter::default(); - let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); - shorten_paths(&mut rewriter, syntax, path); - builder.rewrite(rewriter); + shorten_paths(&mut rewriter, syntax.clone(), path); + let rewritten_syntax = rewriter.rewrite(&syntax); + let new_syntax = insert_use( + &rewritten_syntax, + make::path_from_text(path_to_import), + Some(MergeBehaviour::Full), + ); + builder.replace(syntax.text_range(), new_syntax.to_string()) }, ) } @@ -220,9 +217,10 @@ impl std::fmt::Debug<|> for Foo { } ", r" -use stdx; use std::fmt::Debug; +use stdx; + impl Debug for Foo { } ", @@ -274,7 +272,7 @@ impl std::io<|> for Foo { } ", r" -use std::{io, fmt}; +use std::{fmt, io}; impl io for Foo { } @@ -293,7 +291,7 @@ impl std::fmt::Debug<|> for Foo { } ", r" -use std::fmt::{self, Debug, }; +use std::fmt::{self, Debug}; impl Debug for Foo { } @@ -312,7 +310,7 @@ impl std::fmt<|> for Foo { } ", r" -use std::fmt::{self, Debug}; +use std::fmt::{Debug, self}; impl fmt for Foo { } @@ -330,8 +328,9 @@ use std::fmt::{Debug, nested::{Display}}; impl std::fmt::nested<|> for Foo { } ", + // FIXME(veykril): should be nested::{self, Display} here r" -use std::fmt::{Debug, nested::{Display, self}}; +use std::fmt::{Debug, nested::{Display}, nested}; impl nested for Foo { } @@ -349,8 +348,9 @@ use std::fmt::{Debug, nested::{self, Display}}; impl std::fmt::nested<|> for Foo { } ", + // FIXME(veykril): self is being pulled out for some reason now r" -use std::fmt::{Debug, nested::{self, Display}}; +use std::fmt::{Debug, nested::{Display}, nested}; impl nested for Foo { } @@ -369,7 +369,7 @@ impl std::fmt::nested::Debug<|> for Foo { } ", r" -use std::fmt::{Debug, nested::{Display, Debug}}; +use std::fmt::{Debug, nested::{Display}, nested::Debug}; impl Debug for Foo { } @@ -388,7 +388,7 @@ impl std::fmt::nested::Display<|> for Foo { } ", r" -use std::fmt::{nested::Display, Debug}; +use std::fmt::{Debug, nested::Display}; impl Display for Foo { } @@ -407,7 +407,7 @@ impl std::fmt::Display<|> for Foo { } ", r" -use std::fmt::{Display, nested::Debug}; +use std::fmt::{nested::Debug, Display}; impl Display for Foo { } @@ -427,11 +427,12 @@ use crate::{ fn foo() { crate::ty::lower<|>::trait_env() } ", + // FIXME(veykril): formatting broke here r" use crate::{ - ty::{Substs, Ty, lower}, + ty::{Substs, Ty}, AssocItem, -}; +ty::lower}; fn foo() { lower::trait_env() } ", @@ -451,6 +452,8 @@ impl foo::Debug<|> for Foo { r" use std::fmt as foo; +use foo::Debug; + impl Debug for Foo { } ", @@ -627,7 +630,7 @@ fn main() { } ", r" -use std::fmt::{self, Display}; +use std::fmt::{Display, self}; fn main() { fmt; @@ -647,9 +650,8 @@ impl std::io<|> for Foo { } ", r" -use std::io; - pub use std::fmt; +use std::io; impl io for Foo { } @@ -668,9 +670,8 @@ impl std::io<|> for Foo { } ", r" -use std::io; - pub(crate) use std::fmt; +use std::io; impl io for Foo { } diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 030a5a935..f2acda6f3 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -18,9 +18,10 @@ pub(crate) fn find_insert_use_container( ) -> Option> { ctx.sema.ancestors_with_macros(position.clone()).find_map(|n| { if let Some(module) = ast::Module::cast(n.clone()) { - return module.item_list().map(Either::Left); + module.item_list().map(Either::Left) + } else { + Some(Either::Right(ast::SourceFile::cast(n)?)) } - Some(Either::Right(ast::SourceFile::cast(n)?)) }) } @@ -92,6 +93,7 @@ fn use_tree_list_is_nested(tl: &ast::UseTreeList) -> bool { }) } +// FIXME: currently this merely prepends the new tree into old, ideally it would insert the items in a sorted fashion pub fn try_merge_trees( old: &ast::UseTree, new: &ast::UseTree, @@ -486,7 +488,7 @@ use std::io;", check_full( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", - r"use std::foo::bar::{Baz, Qux};", + r"use std::foo::bar::{Qux, Baz};", ) } @@ -495,7 +497,7 @@ use std::io;", check_last( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", - r"use std::foo::bar::{Baz, Qux};", + r"use std::foo::bar::{Qux, Baz};", ) } @@ -504,7 +506,7 @@ use std::io;", check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", - r"use std::foo::bar::{Baz, Quux, Qux};", + r"use std::foo::bar::{Qux, Quux, Baz};", ) } @@ -513,7 +515,7 @@ use std::io;", check_last( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", - r"use std::foo::bar::{Baz, Quux, Qux};", + r"use std::foo::bar::{Qux, Quux, Baz};", ) } @@ -522,7 +524,7 @@ use std::io;", check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", - r"use std::foo::bar::{Baz, quux::{Fez, Fizz}, Qux};", + r"use std::foo::bar::{Qux, quux::{Fez, Fizz}, Baz};", ) } @@ -532,7 +534,7 @@ use std::io;", "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", r"use std::foo::bar::Baz; -use std::foo::bar::{quux::{Fez, Fizz}, Qux};", +use std::foo::bar::{Qux, quux::{Fez, Fizz}};", ) } -- cgit v1.2.3 From 98e2f674e9e736720d1cd0a5b8c24e1fb10f64a1 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 3 Sep 2020 14:38:34 +0200 Subject: Fix inserting imports in front of inner attributes --- .../handlers/replace_qualified_name_with_use.rs | 5 ++- crates/assists/src/utils/insert_use.rs | 52 +++++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/replace_qualified_name_with_use.rs b/crates/assists/src/handlers/replace_qualified_name_with_use.rs index 56e85125d..597bc268c 100644 --- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs @@ -348,9 +348,9 @@ use std::fmt::{Debug, nested::{self, Display}}; impl std::fmt::nested<|> for Foo { } ", - // FIXME(veykril): self is being pulled out for some reason now + // FIXME(veykril): nested is duplicated now r" -use std::fmt::{Debug, nested::{Display}, nested}; +use std::fmt::{Debug, nested::{self, Display}, nested}; impl nested for Foo { } @@ -518,6 +518,7 @@ fn main() { ", r" #![allow(dead_code)] + use std::fmt::Debug; fn main() { diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index f2acda6f3..1ddd72197 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -275,7 +275,27 @@ fn find_insert_position( (InsertPosition::After(node.into()), AddBlankLine::BeforeTwice) } // there are no imports in this file at all - None => (InsertPosition::First, AddBlankLine::AfterTwice), + None => { + // check if the scope has a inner attributes, we dont want to insert in front of it + match scope + .children() + // no flat_map here cause we want to short circuit the iterator + .map(ast::Attr::cast) + .take_while(|attr| { + attr.as_ref() + .map(|attr| attr.kind() == ast::AttrKind::Inner) + .unwrap_or(false) + }) + .last() + .flatten() + { + Some(attr) => ( + InsertPosition::After(attr.syntax().clone().into()), + AddBlankLine::BeforeTwice, + ), + None => (InsertPosition::First, AddBlankLine::AfterTwice), + } + } }, } } @@ -459,6 +479,36 @@ fn main() {}", ) } + #[test] + fn insert_after_inner_attr() { + // empty files will get two trailing newlines + // this is due to the test case insert_no_imports above + check_full( + "foo::bar", + r"#![allow(unused_imports)]", + r"#![allow(unused_imports)] + +use foo::bar;", + ) + } + + #[test] + fn insert_after_inner_attr2() { + // empty files will get two trailing newlines + // this is due to the test case insert_no_imports above + check_full( + "foo::bar", + r"#![allow(unused_imports)] + +fn main() {}", + r"#![allow(unused_imports)] + +use foo::bar; + +fn main() {}", + ) + } + #[test] fn merge_groups() { check_last("std::io", r"use std::fmt;", r"use std::{fmt, io};") -- cgit v1.2.3 From 7de2a30f4080d0e6d8790882238bed1cb6ccbc21 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 3 Sep 2020 16:23:33 +0200 Subject: Fix import insertion breaking nested modules --- crates/assists/src/handlers/auto_import.rs | 11 +- .../handlers/extract_struct_from_enum_variant.rs | 8 +- .../handlers/replace_qualified_name_with_use.rs | 20 +-- crates/assists/src/utils.rs | 2 +- crates/assists/src/utils/insert_use.rs | 138 ++++++++++++++------- 5 files changed, 117 insertions(+), 62 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/auto_import.rs b/crates/assists/src/handlers/auto_import.rs index f3b4c5708..66e819154 100644 --- a/crates/assists/src/handlers/auto_import.rs +++ b/crates/assists/src/handlers/auto_import.rs @@ -1,11 +1,13 @@ use std::collections::BTreeSet; +use ast::make; use either::Either; use hir::{ AsAssocItem, AssocItemContainer, ModPath, Module, ModuleDef, PathResolution, Semantics, Trait, Type, }; use ide_db::{imports_locator, RootDatabase}; +use insert_use::ImportScope; use rustc_hash::FxHashSet; use syntax::{ ast::{self, AstNode}, @@ -16,8 +18,6 @@ use crate::{ utils::{insert_use, MergeBehaviour}, AssistContext, AssistId, AssistKind, Assists, GroupLabel, }; -use ast::make; -use insert_use::find_insert_use_container; // Assist: auto_import // @@ -47,8 +47,9 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> let range = ctx.sema.original_range(&auto_import_assets.syntax_under_caret).range; let group = auto_import_assets.get_import_group_message(); - let container = find_insert_use_container(&auto_import_assets.syntax_under_caret, ctx)?; - let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); + let scope = + ImportScope::find_insert_use_container(&auto_import_assets.syntax_under_caret, ctx)?; + let syntax = scope.as_syntax_node(); for import in proposed_imports { acc.add_group( &group, @@ -57,7 +58,7 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> range, |builder| { let new_syntax = insert_use( - &syntax, + &scope, make::path_from_text(&import.to_string()), Some(MergeBehaviour::Full), ); diff --git a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs index d9c50f25f..a50a57f1a 100644 --- a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs @@ -15,7 +15,7 @@ use crate::{ AssistContext, AssistId, AssistKind, Assists, }; use ast::make; -use insert_use::find_insert_use_container; +use insert_use::ImportScope; // Assist: extract_struct_from_enum_variant // @@ -110,11 +110,11 @@ fn insert_import( if let Some(mut mod_path) = mod_path { mod_path.segments.pop(); mod_path.segments.push(variant_hir_name.clone()); - let container = find_insert_use_container(path.syntax(), ctx)?; - let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); + let scope = ImportScope::find_insert_use_container(path.syntax(), ctx)?; + let syntax = scope.as_syntax_node(); let new_syntax = insert_use( - &syntax, + &scope, make::path_from_text(&mod_path.to_string()), Some(MergeBehaviour::Full), ); diff --git a/crates/assists/src/handlers/replace_qualified_name_with_use.rs b/crates/assists/src/handlers/replace_qualified_name_with_use.rs index 597bc268c..85c70d16b 100644 --- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs @@ -2,7 +2,7 @@ use syntax::{algo::SyntaxRewriter, ast, match_ast, AstNode, SyntaxNode, TextRang use test_utils::mark; use crate::{ - utils::{find_insert_use_container, insert_use, MergeBehaviour}, + utils::{insert_use, ImportScope, MergeBehaviour}, AssistContext, AssistId, AssistKind, Assists, }; use ast::make; @@ -44,8 +44,8 @@ pub(crate) fn replace_qualified_name_with_use( }; let target = path.syntax().text_range(); - let container = find_insert_use_container(path.syntax(), ctx)?; - let syntax = container.either(|l| l.syntax().clone(), |r| r.syntax().clone()); + let scope = ImportScope::find_insert_use_container(path.syntax(), ctx)?; + let syntax = scope.as_syntax_node(); acc.add( AssistId("replace_qualified_name_with_use", AssistKind::RefactorRewrite), "Replace qualified path with use", @@ -56,12 +56,14 @@ pub(crate) fn replace_qualified_name_with_use( let mut rewriter = SyntaxRewriter::default(); shorten_paths(&mut rewriter, syntax.clone(), path); let rewritten_syntax = rewriter.rewrite(&syntax); - let new_syntax = insert_use( - &rewritten_syntax, - make::path_from_text(path_to_import), - Some(MergeBehaviour::Full), - ); - builder.replace(syntax.text_range(), new_syntax.to_string()) + if let Some(ref import_scope) = ImportScope::from(rewritten_syntax) { + let new_syntax = insert_use( + import_scope, + make::path_from_text(path_to_import), + Some(MergeBehaviour::Full), + ); + builder.replace(syntax.text_range(), new_syntax.to_string()) + } }, ) } diff --git a/crates/assists/src/utils.rs b/crates/assists/src/utils.rs index 07afa64ff..7559ddd63 100644 --- a/crates/assists/src/utils.rs +++ b/crates/assists/src/utils.rs @@ -16,7 +16,7 @@ use syntax::{ use crate::assist_config::SnippetCap; -pub(crate) use insert_use::{find_insert_use_container, insert_use, MergeBehaviour}; +pub(crate) use insert_use::{insert_use, ImportScope, MergeBehaviour}; pub(crate) fn unwrap_trivial_block(block: ast::BlockExpr) -> ast::Expr { extract_trivial_expression(&block) diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 1ddd72197..40ff31075 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -1,8 +1,10 @@ use std::iter::{self, successors}; use algo::skip_trivia_token; -use ast::{edit::AstNodeEdit, PathSegmentKind, VisibilityOwner}; -use either::Either; +use ast::{ + edit::{AstNodeEdit, IndentLevel}, + PathSegmentKind, VisibilityOwner, +}; use syntax::{ algo, ast::{self, make, AstNode}, @@ -11,56 +13,121 @@ use syntax::{ use test_utils::mark; -/// Determines the containing syntax node in which to insert a `use` statement affecting `position`. -pub(crate) fn find_insert_use_container( - position: &SyntaxNode, - ctx: &crate::assist_context::AssistContext, -) -> Option> { - ctx.sema.ancestors_with_macros(position.clone()).find_map(|n| { - if let Some(module) = ast::Module::cast(n.clone()) { - module.item_list().map(Either::Left) +#[derive(Debug)] +pub enum ImportScope { + File(ast::SourceFile), + Module(ast::ItemList), +} + +impl ImportScope { + pub fn from(syntax: SyntaxNode) -> Option { + if let Some(module) = ast::Module::cast(syntax.clone()) { + module.item_list().map(ImportScope::Module) + } else if let this @ Some(_) = ast::SourceFile::cast(syntax.clone()) { + this.map(ImportScope::File) } else { - Some(Either::Right(ast::SourceFile::cast(n)?)) + ast::ItemList::cast(syntax).map(ImportScope::Module) } - }) + } + + /// Determines the containing syntax node in which to insert a `use` statement affecting `position`. + pub(crate) fn find_insert_use_container( + position: &SyntaxNode, + ctx: &crate::assist_context::AssistContext, + ) -> Option { + ctx.sema.ancestors_with_macros(position.clone()).find_map(Self::from) + } + + pub(crate) fn as_syntax_node(&self) -> &SyntaxNode { + match self { + ImportScope::File(file) => file.syntax(), + ImportScope::Module(item_list) => item_list.syntax(), + } + } + + fn indent_level(&self) -> IndentLevel { + match self { + ImportScope::File(file) => file.indent_level(), + ImportScope::Module(item_list) => item_list.indent_level() + 1, + } + } + + fn first_insert_pos(&self) -> (InsertPosition, AddBlankLine) { + match self { + ImportScope::File(_) => (InsertPosition::First, AddBlankLine::AfterTwice), + // don't insert the impotrs before the item lists curly brace + ImportScope::Module(item_list) => item_list + .l_curly_token() + .map(|b| (InsertPosition::After(b.into()), AddBlankLine::Around)) + .unwrap_or((InsertPosition::First, AddBlankLine::AfterTwice)), + } + } + + fn insert_pos_after_inner_attribute(&self) -> (InsertPosition, AddBlankLine) { + // check if the scope has a inner attributes, we dont want to insert in front of it + match self + .as_syntax_node() + .children() + // no flat_map here cause we want to short circuit the iterator + .map(ast::Attr::cast) + .take_while(|attr| { + attr.as_ref().map(|attr| attr.kind() == ast::AttrKind::Inner).unwrap_or(false) + }) + .last() + .flatten() + { + Some(attr) => { + (InsertPosition::After(attr.syntax().clone().into()), AddBlankLine::BeforeTwice) + } + None => self.first_insert_pos(), + } + } } /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. -pub fn insert_use( - where_: &SyntaxNode, +pub(crate) fn insert_use( + scope: &ImportScope, path: ast::Path, merge: Option, ) -> SyntaxNode { let use_item = make::use_(make::use_tree(path.clone(), None, None, false)); // merge into existing imports if possible if let Some(mb) = merge { - for existing_use in where_.children().filter_map(ast::Use::cast) { + for existing_use in scope.as_syntax_node().children().filter_map(ast::Use::cast) { if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { let to_delete: SyntaxElement = existing_use.syntax().clone().into(); let to_delete = to_delete.clone()..=to_delete; let to_insert = iter::once(merged.syntax().clone().into()); - return algo::replace_children(where_, to_delete, to_insert); + return algo::replace_children(scope.as_syntax_node(), to_delete, to_insert); } } } // either we weren't allowed to merge or there is no import that fits the merge conditions // so look for the place we have to insert to - let (insert_position, add_blank) = find_insert_position(where_, path); + let (insert_position, add_blank) = find_insert_position(scope, path); let to_insert: Vec = { let mut buf = Vec::new(); match add_blank { - AddBlankLine::Before => buf.push(make::tokens::single_newline().into()), + AddBlankLine::Before | AddBlankLine::Around => { + buf.push(make::tokens::single_newline().into()) + } AddBlankLine::BeforeTwice => buf.push(make::tokens::blank_line().into()), _ => (), } + if let ident_level @ 1..=usize::MAX = scope.indent_level().0 as usize { + // TODO: 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()); match add_blank { - AddBlankLine::After => buf.push(make::tokens::single_newline().into()), + AddBlankLine::After | AddBlankLine::Around => { + buf.push(make::tokens::single_newline().into()) + } AddBlankLine::AfterTwice => buf.push(make::tokens::blank_line().into()), _ => (), } @@ -68,7 +135,7 @@ pub fn insert_use( buf }; - algo::insert_children(where_, insert_position, to_insert) + algo::insert_children(scope.as_syntax_node(), insert_position, to_insert) } fn try_merge_imports( @@ -218,16 +285,18 @@ fn segment_iter(path: &ast::Path) -> impl Iterator + Cl enum AddBlankLine { Before, BeforeTwice, + Around, After, AfterTwice, } fn find_insert_position( - scope: &SyntaxNode, + scope: &ImportScope, insert_path: ast::Path, ) -> (InsertPosition, AddBlankLine) { let group = ImportGroup::new(&insert_path); let path_node_iter = scope + .as_syntax_node() .children() .filter_map(|node| ast::Use::cast(node.clone()).zip(Some(node))) .flat_map(|(use_, node)| use_.use_tree().and_then(|tree| tree.path()).zip(Some(node))); @@ -275,27 +344,7 @@ fn find_insert_position( (InsertPosition::After(node.into()), AddBlankLine::BeforeTwice) } // there are no imports in this file at all - None => { - // check if the scope has a inner attributes, we dont want to insert in front of it - match scope - .children() - // no flat_map here cause we want to short circuit the iterator - .map(ast::Attr::cast) - .take_while(|attr| { - attr.as_ref() - .map(|attr| attr.kind() == ast::AttrKind::Inner) - .unwrap_or(false) - }) - .last() - .flatten() - { - Some(attr) => ( - InsertPosition::After(attr.syntax().clone().into()), - AddBlankLine::BeforeTwice, - ), - None => (InsertPosition::First, AddBlankLine::AfterTwice), - } - } + None => scope.insert_pos_after_inner_attribute(), }, } } @@ -640,7 +689,10 @@ use foo::bar;", ra_fixture_after: &str, mb: Option, ) { - let file = ast::SourceFile::parse(ra_fixture_before).tree().syntax().clone(); + let file = super::ImportScope::from( + ast::SourceFile::parse(ra_fixture_before).tree().syntax().clone(), + ) + .unwrap(); let path = ast::SourceFile::parse(&format!("use {};", path)) .tree() .syntax() -- cgit v1.2.3 From d29b69cbe61d20556c55e4f6a53da0784df8b6d0 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 3 Sep 2020 18:32:29 +0200 Subject: Disable insert_import in extract_struct_from_enum_variant until its fixed --- crates/assists/src/handlers/extract_struct_from_enum_variant.rs | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs index a50a57f1a..eb812c1c9 100644 --- a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs @@ -118,6 +118,7 @@ fn insert_import( make::path_from_text(&mod_path.to_string()), Some(MergeBehaviour::Full), ); + // FIXME: this will currently panic as multiple imports will have overlapping text ranges builder.replace(syntax.text_range(), new_syntax.to_string()) } Some(()) @@ -191,6 +192,7 @@ fn update_reference( list_range.end().checked_sub(TextSize::from(1))?, ); builder.edit_file(reference.file_range.file_id); + /* FIXME: this most likely requires AST-based editing, see `insert_import` if !visited_modules_set.contains(&module) { if insert_import(ctx, builder, &path_expr, &module, enum_module_def, variant_hir_name) .is_some() @@ -198,6 +200,7 @@ fn update_reference( visited_modules_set.insert(module); } } + */ builder.replace(inside_list_range, format!("{}{}", segment, list)); Some(()) } @@ -256,6 +259,7 @@ pub enum A { One(One) }"#, } #[test] + #[ignore] // FIXME: this currently panics if `insert_import` is used fn test_extract_struct_with_complex_imports() { check_assist( extract_struct_from_enum_variant, -- cgit v1.2.3 From 82f61e6629f709d7f347fd801ef5c31f476ff29e Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 3 Sep 2020 18:44:39 +0200 Subject: Add extra insert_use test for pub(crate) re-export handling --- .../handlers/extract_struct_from_enum_variant.rs | 9 +++++---- crates/assists/src/utils/insert_use.rs | 22 ++++++++++++++-------- 2 files changed, 19 insertions(+), 12 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs index eb812c1c9..80c62d8bb 100644 --- a/crates/assists/src/handlers/extract_struct_from_enum_variant.rs +++ b/crates/assists/src/handlers/extract_struct_from_enum_variant.rs @@ -97,6 +97,7 @@ fn existing_struct_def(db: &RootDatabase, variant_name: &str, variant: &EnumVari .any(|(name, _)| name.to_string() == variant_name.to_string()) } +#[allow(dead_code)] fn insert_import( ctx: &AssistContext, builder: &mut AssistBuilder, @@ -174,9 +175,9 @@ fn update_reference( builder: &mut AssistBuilder, reference: Reference, source_file: &SourceFile, - enum_module_def: &ModuleDef, - variant_hir_name: &Name, - visited_modules_set: &mut FxHashSet, + _enum_module_def: &ModuleDef, + _variant_hir_name: &Name, + _visited_modules_set: &mut FxHashSet, ) -> Option<()> { let path_expr: ast::PathExpr = find_node_at_offset::( source_file.syntax(), @@ -185,7 +186,7 @@ fn update_reference( let call = path_expr.syntax().parent().and_then(ast::CallExpr::cast)?; let list = call.arg_list()?; let segment = path_expr.path()?.segment()?; - let module = ctx.sema.scope(&path_expr.syntax()).module()?; + let _module = ctx.sema.scope(&path_expr.syntax()).module()?; let list_range = list.syntax().text_range(); let inside_list_range = TextRange::new( list_range.start().checked_add(TextSize::from(1))?, diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 40ff31075..8a4c8520d 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -1,3 +1,4 @@ +//! Handle syntactic aspects of inserting a new `use`. use std::iter::{self, successors}; use algo::skip_trivia_token; @@ -10,7 +11,6 @@ use syntax::{ ast::{self, make, AstNode}, Direction, InsertPosition, SyntaxElement, SyntaxNode, T, }; - use test_utils::mark; #[derive(Debug)] @@ -55,7 +55,7 @@ impl ImportScope { fn first_insert_pos(&self) -> (InsertPosition, AddBlankLine) { match self { ImportScope::File(_) => (InsertPosition::First, AddBlankLine::AfterTwice), - // don't insert the impotrs before the item lists curly brace + // don't insert the imports before the item list's opening curly brace ImportScope::Module(item_list) => item_list .l_curly_token() .map(|b| (InsertPosition::After(b.into()), AddBlankLine::Around)) @@ -64,7 +64,7 @@ impl ImportScope { } fn insert_pos_after_inner_attribute(&self) -> (InsertPosition, AddBlankLine) { - // check if the scope has a inner attributes, we dont want to insert in front of it + // check if the scope has inner attributes, we dont want to insert in front of them match self .as_syntax_node() .children() @@ -119,7 +119,7 @@ pub(crate) fn insert_use( } if let ident_level @ 1..=usize::MAX = scope.indent_level().0 as usize { - // TODO: this alone doesnt properly re-align all cases + // 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()); @@ -530,8 +530,6 @@ fn main() {}", #[test] fn insert_after_inner_attr() { - // empty files will get two trailing newlines - // this is due to the test case insert_no_imports above check_full( "foo::bar", r"#![allow(unused_imports)]", @@ -543,8 +541,6 @@ use foo::bar;", #[test] fn insert_after_inner_attr2() { - // empty files will get two trailing newlines - // this is due to the test case insert_no_imports above check_full( "foo::bar", r"#![allow(unused_imports)] @@ -647,6 +643,16 @@ use std::io;", ) } + #[test] + fn merge_groups_skip_pub_crate() { + check_full( + "std::io", + r"pub(crate) use std::fmt::{Result, Display};", + r"pub(crate) use std::fmt::{Result, Display}; +use std::io;", + ) + } + #[test] #[ignore] // FIXME: Support this fn split_out_merge() { -- cgit v1.2.3