From a898752881779a328462ad9f2db291073f2f134f Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sat, 12 Sep 2020 19:18:14 +0200 Subject: Reimplement import merging by making it recursive properly nesting all levels --- crates/assists/src/handlers/merge_imports.rs | 14 +- .../handlers/replace_qualified_name_with_use.rs | 20 +- crates/assists/src/utils/insert_use.rs | 312 ++++++++++++++++----- crates/syntax/src/ast/edit.rs | 1 + 4 files changed, 264 insertions(+), 83 deletions(-) (limited to 'crates') diff --git a/crates/assists/src/handlers/merge_imports.rs b/crates/assists/src/handlers/merge_imports.rs index 0bd679260..fe33cee53 100644 --- a/crates/assists/src/handlers/merge_imports.rs +++ b/crates/assists/src/handlers/merge_imports.rs @@ -95,7 +95,7 @@ use std::fmt::Debug; use std::fmt<|>::Display; ", r" -use std::fmt::{Display, Debug}; +use std::fmt::{Debug, Display}; ", ); } @@ -122,7 +122,7 @@ use std::fmt::{self, Display}; use std::{fmt, <|>fmt::Display}; ", r" -use std::{fmt::{Display, self}}; +use std::{fmt::{self, Display}}; ", ); } @@ -210,13 +210,17 @@ use std::{fmt<|>::Debug, fmt::Display}; use std::{fmt::{Debug, Display}}; ", ); + } + + #[test] + fn test_merge_nested2() { check_assist( merge_imports, r" use std::{fmt::Debug, fmt<|>::Display}; ", r" -use std::{fmt::{Display, Debug}}; +use std::{fmt::{Debug, Display}}; ", ); } @@ -310,9 +314,7 @@ use foo::<|>{ }; ", r" -use foo::{ - FooBar, -bar::baz}; +use foo::{FooBar, bar::baz}; ", ) } 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 85c70d16b..093c3b101 100644 --- a/crates/assists/src/handlers/replace_qualified_name_with_use.rs +++ b/crates/assists/src/handlers/replace_qualified_name_with_use.rs @@ -312,7 +312,7 @@ impl std::fmt<|> for Foo { } ", r" -use std::fmt::{Debug, self}; +use std::fmt::{self, Debug}; impl fmt for Foo { } @@ -330,9 +330,8 @@ 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}, nested}; +use std::fmt::{Debug, nested::{self, Display}}; impl nested for Foo { } @@ -350,9 +349,8 @@ use std::fmt::{Debug, nested::{self, Display}}; impl std::fmt::nested<|> for Foo { } ", - // FIXME(veykril): nested is duplicated now r" -use std::fmt::{Debug, nested::{self, Display}, nested}; +use std::fmt::{Debug, nested::{self, Display}}; impl nested for Foo { } @@ -371,7 +369,7 @@ impl std::fmt::nested::Debug<|> for Foo { } ", r" -use std::fmt::{Debug, nested::{Display}, nested::Debug}; +use std::fmt::{Debug, nested::{Debug, Display}}; impl Debug for Foo { } @@ -409,7 +407,7 @@ impl std::fmt::Display<|> for Foo { } ", r" -use std::fmt::{nested::Debug, Display}; +use std::fmt::{Display, nested::Debug}; impl Display for Foo { } @@ -429,12 +427,8 @@ use crate::{ fn foo() { crate::ty::lower<|>::trait_env() } ", - // FIXME(veykril): formatting broke here r" -use crate::{ - ty::{Substs, Ty}, - AssocItem, -ty::lower}; +use crate::{AssocItem, ty::{Substs, Ty, lower}}; fn foo() { lower::trait_env() } ", @@ -633,7 +627,7 @@ fn main() { } ", r" -use std::fmt::{Display, self}; +use std::fmt::{self, Display}; fn main() { fmt; diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 98553b2e0..4f5fd317a 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -1,7 +1,9 @@ //! Handle syntactic aspects of inserting a new `use`. -use std::iter::{self, successors}; +use std::{ + cmp::Ordering, + iter::{self, successors}, +}; -use algo::skip_trivia_token; use ast::{ edit::{AstNodeEdit, IndentLevel}, PathSegmentKind, VisibilityOwner, @@ -9,9 +11,8 @@ use ast::{ use syntax::{ algo, ast::{self, make, AstNode}, - Direction, InsertPosition, SyntaxElement, SyntaxNode, T, + InsertPosition, SyntaxElement, SyntaxNode, }; -use test_utils::mark; #[derive(Debug)] pub enum ImportScope { @@ -163,18 +164,48 @@ pub(crate) fn try_merge_imports( 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() - }) +/// 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), + } } -// FIXME: currently this merely prepends the new tree into old, ideally it would insert the items in a sorted fashion pub(crate) fn try_merge_trees( old: &ast::UseTree, new: &ast::UseTree, - merge_behaviour: MergeBehaviour, + merge: MergeBehaviour, ) -> Option { let lhs_path = old.path()?; let rhs_path = new.path()?; @@ -182,33 +213,89 @@ pub(crate) fn try_merge_trees( 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 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)) - { - mark::hit!(test_last_merge_too_long); - return None; - } + recursive_merge(&lhs, &rhs, merge).map(|(merged, _)| merged) +} - let should_insert_comma = lhs_tl - .r_curly_token() - .and_then(|it| skip_trivia_token(it.prev_token()?, Direction::Prev)) - .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| !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); - Some(lhs.with_use_tree_list(use_tree_list)) +/// 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 +fn recursive_merge( + lhs: &ast::UseTree, + rhs: &ast::UseTree, + merge: MergeBehaviour, +) -> Option<(ast::UseTree, usize)> { + 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 + .map(|tree| match merge == MergeBehaviour::Last && tree.use_tree_list().is_some() { + true => None, + false => Some(tree), + }) + .collect::>>()?; + use_trees.sort_unstable_by(|a, b| path_cmp_opt(a.path(), b.path())); + for rhs_t in rhs.use_tree_list().into_iter().flat_map(|list| list.use_trees()) { + 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 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); + // 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] + .use_tree_list() + .xor(rhs_t.use_tree_list()) + .map(|tree_list| tree_list.use_trees().any(tree_is_self)) + .unwrap_or(false) + { + continue; + } + + // 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( + make::path_unqualified(make::path_segment_self()), + None, + None, + false, + ); + use_trees.insert( + idx, + make::use_tree( + make::path_unqualified(make::path_segment_self()), + None, + None, + true, + ), + ); + continue; + } + } + let lhs = use_trees[idx].split_prefix(&lhs_prefix); + let rhs = rhs_t.split_prefix(&rhs_prefix); + match recursive_merge(&lhs, &rhs, merge) { + Some((_, count)) + if merge == MergeBehaviour::Last && use_trees.len() > 1 && count > 1 => + { + return None + } + Some((use_tree, _)) => use_trees[idx] = use_tree, + None => use_trees.insert(idx, rhs_t), + } + } + Err(idx) => { + use_trees.insert(idx, rhs_t); + } + } + } + let count = use_trees.len(); + let tl = make::use_tree_list(use_trees); + Some((lhs.with_use_tree_list(tl), count)) } /// Traverses both paths until they differ, returning the common prefix of both. @@ -235,6 +322,23 @@ fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Pa res } +fn path_is_self(path: ast::Path) -> bool { + path.segment().and_then(|seg| seg.self_token()).is_some() && path.qualifier().is_none() +} + +fn first_segment(path: &ast::Path) -> Option { + first_path(path).segment() +} + +fn first_path(path: &ast::Path) -> ast::Path { + successors(Some(path.clone()), ast::Path::qualifier).last().unwrap() +} + +fn segment_iter(path: &ast::Path) -> impl Iterator + Clone { + // cant make use of SyntaxNode::siblings, because the returned Iterator is not clone + successors(first_segment(path), |p| p.parent_path().parent_path().and_then(|p| p.segment())) +} + /// What type of merges are allowed. #[derive(Copy, Clone, PartialEq, Eq)] pub enum MergeBehaviour { @@ -279,19 +383,6 @@ impl ImportGroup { } } -fn first_segment(path: &ast::Path) -> Option { - first_path(path).segment() -} - -fn first_path(path: &ast::Path) -> ast::Path { - successors(Some(path.clone()), ast::Path::qualifier).last().unwrap() -} - -fn segment_iter(path: &ast::Path) -> impl Iterator + Clone { - // cant make use of SyntaxNode::siblings, because the returned Iterator is not clone - successors(first_segment(path), |p| p.parent_path().parent_path().and_then(|p| p.segment())) -} - #[derive(PartialEq, Eq)] enum AddBlankLine { Before, @@ -594,7 +685,7 @@ use std::io;", check_full( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", - r"use std::foo::bar::{Qux, Baz};", + r"use std::foo::bar::{Baz, Qux};", ) } @@ -603,7 +694,7 @@ use std::io;", check_last( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", - r"use std::foo::bar::{Qux, Baz};", + r"use std::foo::bar::{Baz, Qux};", ) } @@ -612,7 +703,7 @@ use std::io;", check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", - r"use std::foo::bar::{Qux, Quux, Baz};", + r"use std::foo::bar::{Baz, Quux, Qux};", ) } @@ -621,7 +712,7 @@ use std::io;", check_last( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", - r"use std::foo::bar::{Qux, Quux, Baz};", + r"use std::foo::bar::{Baz, Quux, Qux};", ) } @@ -630,7 +721,7 @@ use std::io;", check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", - r"use std::foo::bar::{Qux, quux::{Fez, Fizz}, Baz};", + r"use std::foo::bar::{Baz, Qux, quux::{Fez, Fizz}};", ) } @@ -644,6 +735,15 @@ use std::foo::bar::{Qux, quux::{Fez, Fizz}};", ) } + #[test] + fn merge_groups_full_nested_deep() { + check_full( + "std::foo::bar::quux::Baz", + r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", + r"use std::foo::bar::{Qux, quux::{Baz, Fez, Fizz}};", + ) + } + #[test] fn merge_groups_skip_pub() { check_full( @@ -670,34 +770,63 @@ use std::io;", check_last( "std::fmt::Result", r"use std::{fmt, io};", - r"use std::{self, fmt::Result}; + r"use std::fmt::{self, Result}; use std::io;", ) } + #[test] + fn merge_into_module_import() { + check_full( + "std::fmt::Result", + r"use std::{fmt, io};", + r"use std::{fmt::{self, Result}, io};", + ) + } + #[test] fn merge_groups_self() { check_full("std::fmt::Debug", r"use std::fmt;", r"use std::fmt::{self, Debug};") } #[test] - fn merge_self_glob() { + fn merge_mod_into_glob() { check_full( "token::TokenKind", r"use token::TokenKind::*;", r"use token::TokenKind::{self::*, self};", ) + // FIXME: have it emit `use token::TokenKind::{self, *}`? + } + + #[test] + fn merge_self_glob() { + check_full("self", r"use self::*;", r"use self::{self::*, self};") + // FIXME: have it emit `use {self, *}`? + } + + #[test] + #[ignore] // FIXME: Support this + fn merge_partial_path() { + check_full( + "ast::Foo", + r"use syntax::{ast, algo};", + r"use syntax::{ast::{self, Foo}, algo};", + ) + } + + #[test] + fn merge_glob_nested() { + check_full( + "foo::bar::quux::Fez", + r"use foo::bar::{Baz, quux::*;", + r"use foo::bar::{Baz, quux::{self::*, Fez}}", + ) } #[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; -use foo::bar::baz::Qux;", - ); + check_last("foo::bar", r"use foo::bar::baz::Qux;", r"use foo::bar::{self, baz::Qux};"); } #[test] @@ -710,6 +839,42 @@ use foo::bar::baz::Qux;", ); } + #[test] + fn merge_last_fail() { + check_merge_only_fail( + r"use foo::bar::{baz::{Qux, Fez}};", + r"use foo::bar::{baaz::{Quux, Feez}};", + MergeBehaviour::Last, + ); + } + + #[test] + fn merge_last_fail1() { + check_merge_only_fail( + r"use foo::bar::{baz::{Qux, Fez}};", + r"use foo::bar::baaz::{Quux, Feez};", + MergeBehaviour::Last, + ); + } + + #[test] + fn merge_last_fail2() { + check_merge_only_fail( + r"use foo::bar::baz::{Qux, Fez};", + r"use foo::bar::{baaz::{Quux, Feez}};", + MergeBehaviour::Last, + ); + } + + #[test] + fn merge_last_fail3() { + check_merge_only_fail( + r"use foo::bar::baz::{Qux, Fez};", + r"use foo::bar::baaz::{Quux, Feez};", + MergeBehaviour::Last, + ); + } + fn check( path: &str, ra_fixture_before: &str, @@ -742,4 +907,23 @@ use foo::bar::baz::Qux;", fn check_none(path: &str, ra_fixture_before: &str, ra_fixture_after: &str) { check(path, ra_fixture_before, ra_fixture_after, None) } + + fn check_merge_only_fail(ra_fixture0: &str, ra_fixture1: &str, mb: MergeBehaviour) { + let use0 = ast::SourceFile::parse(ra_fixture0) + .tree() + .syntax() + .descendants() + .find_map(ast::Use::cast) + .unwrap(); + + let use1 = ast::SourceFile::parse(ra_fixture1) + .tree() + .syntax() + .descendants() + .find_map(ast::Use::cast) + .unwrap(); + + let result = try_merge_imports(&use0, &use1, mb); + assert_eq!(result, None); + } } diff --git a/crates/syntax/src/ast/edit.rs b/crates/syntax/src/ast/edit.rs index 8b1c65dd6..45cf31f13 100644 --- a/crates/syntax/src/ast/edit.rs +++ b/crates/syntax/src/ast/edit.rs @@ -347,6 +347,7 @@ impl ast::UseTree { self.clone() } + /// Splits off the given prefix, making it the path component of the use tree, appending the rest of the path to all UseTreeList items. #[must_use] pub fn split_prefix(&self, prefix: &ast::Path) -> ast::UseTree { let suffix = if self.path().as_ref() == Some(prefix) && self.use_tree_list().is_none() { -- cgit v1.2.3