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