squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Reimplement import merging by making it recursive properly nesting all levels

Browse source
This commit is contained in:
Lukas Wirth 2020-09-12 19:18:14 +02:00
parent c8623461a5
commit a898752881
4 changed files with 262 additions and 81 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
crates/assists/src/handlers/merge_imports.rs
View file

@ -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};
",
)
}

20
crates/assists/src/handlers/replace_qualified_name_with_use.rs
View file

@ -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;

308
crates/assists/src/utils/insert_use.rs
View file

@ -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<ast::Path>, b: Option<ast::Path>) -> 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<ast::UseTree> {
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()?;
recursive_merge(&lhs, &rhs, merge).map(|(merged, _)| merged)
}
// 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;
}
/// 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::<Option<Vec<_>>>()?;
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;
}
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<SyntaxElement> = Vec::new();
if should_insert_comma {
to_insert.push(make::token(T![,]).into());
to_insert.push(make::tokens::single_space().into());
// 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);
}
}
}
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))
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<ast::PathSegment> {
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<Item = ast::PathSegment> + 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<ast::PathSegment> {
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<Item = ast::PathSegment> + 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);
}
}

1
crates/syntax/src/ast/edit.rs
View file

@ -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() {