mathieu/granite-rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Merge #5955

Browse source 
5955: Remove merge import code duplication r=jonas-schievink a=Veykril

This removes the code duplication caused by #5935, this also allows the assist to merge imports that have equal visibility and prevents merges of unequal visibility. This PR also fixes an iteration mistake in the mentioned PR:

Turns out I made a mistake when writing the `segment_iter` function, I was assuming that the `children` of a path will just be the segments, which is obviously not the case. This also brings insertion order of shorter paths in line with how `rustfmt` orders them.

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
This commit is contained in:
bors[bot] 2020-09-11 14:01:20 +00:00 • committed by GitHub
commit 568dc38b7b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 114 additions and 82 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -1,14 +1,14 @@
use std::iter::successors;
use syntax::{
algo::{neighbor, skip_trivia_token, SyntaxRewriter},
ast::{self, edit::AstNodeEdit, make},
AstNode, Direction, InsertPosition, SyntaxElement, T,
algo::{neighbor, SyntaxRewriter},
ast, AstNode,
};
use crate::{
assist_context::{AssistContext, Assists},
utils::next_prev,
utils::{
insert_use::{try_merge_imports, try_merge_trees},
next_prev, MergeBehaviour,
},
AssistId, AssistKind,
};
@ -30,23 +30,22 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext) -> Option<()
let mut offset = ctx.offset();
if let Some(use_item) = tree.syntax().parent().and_then(ast::Use::cast) {
let (merged, to_delete) = next_prev()
.filter_map(|dir| neighbor(&use_item, dir))
.filter_map(|it| Some((it.clone(), it.use_tree()?)))
.find_map(|(use_item, use_tree)| {
Some((try_merge_trees(&tree, &use_tree)?, use_item))
let (merged, to_delete) =
next_prev().filter_map(|dir| neighbor(&use_item, dir)).find_map(|use_item2| {
try_merge_imports(&use_item, &use_item2, MergeBehaviour::Full).zip(Some(use_item2))
})?;
rewriter.replace_ast(&tree, &merged);
rewriter.replace_ast(&use_item, &merged);
rewriter += to_delete.remove();
if to_delete.syntax().text_range().end() < offset {
offset -= to_delete.syntax().text_range().len();
}
} else {
let (merged, to_delete) = next_prev()
.filter_map(|dir| neighbor(&tree, dir))
.find_map(|use_tree| Some((try_merge_trees(&tree, &use_tree)?, use_tree.clone())))?;
let (merged, to_delete) =
next_prev().filter_map(|dir| neighbor(&tree, dir)).find_map(|use_tree| {
try_merge_trees(&tree, &use_tree, MergeBehaviour::Full).zip(Some(use_tree))
})?;
rewriter.replace_ast(&tree, &merged);
rewriter += to_delete.remove();
@ -67,66 +66,6 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext) -> Option<()
)
}
fn try_merge_trees(old: &ast::UseTree, new: &ast::UseTree) -> Option<ast::UseTree> {
let lhs_path = old.path()?;
let rhs_path = new.path()?;
let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?;
let lhs = old.split_prefix(&lhs_prefix);
let rhs = new.split_prefix(&rhs_prefix);
let should_insert_comma = lhs
.use_tree_list()?
.r_curly_token()
.and_then(|it| skip_trivia_token(it.prev_token()?, Direction::Prev))
.map(|it| it.kind() != T![,])
.unwrap_or(true);
let mut to_insert: Vec<SyntaxElement> = Vec::new();
if should_insert_comma {
to_insert.push(make::token(T![,]).into());
to_insert.push(make::tokens::single_space().into());
}
to_insert.extend(
rhs.use_tree_list()?
.syntax()
.children_with_tokens()
.filter(|it| it.kind() != T!['{'] && it.kind() != T!['}']),
);
let use_tree_list = lhs.use_tree_list()?;
let pos = InsertPosition::Before(use_tree_list.r_curly_token()?.into());
let use_tree_list = use_tree_list.insert_children(pos, to_insert);
Some(lhs.with_use_tree_list(use_tree_list))
}
fn common_prefix(lhs: &ast::Path, rhs: &ast::Path) -> Option<(ast::Path, ast::Path)> {
let mut res = None;
let mut lhs_curr = first_path(&lhs);
let mut rhs_curr = first_path(&rhs);
loop {
match (lhs_curr.segment(), rhs_curr.segment()) {
(Some(lhs), Some(rhs)) if lhs.syntax().text() == rhs.syntax().text() => (),
_ => break,
}
res = Some((lhs_curr.clone(), rhs_curr.clone()));
match (lhs_curr.parent_path(), rhs_curr.parent_path()) {
(Some(lhs), Some(rhs)) => {
lhs_curr = lhs;
rhs_curr = rhs;
}
_ => break,
}
}
res
}
fn first_path(path: &ast::Path) -> ast::Path {
successors(Some(path.clone()), |it| it.qualifier()).last().unwrap()
}
#[cfg(test)]
mod tests {
use crate::tests::{check_assist, check_assist_not_applicable};
@ -188,6 +127,78 @@ use std::{fmt::{Display, self}};
);
}
#[test]
fn skip_pub1() {
check_assist_not_applicable(
merge_imports,
r"
pub use std::fmt<|>::Debug;
use std::fmt::Display;
",
);
}
#[test]
fn skip_pub_last() {
check_assist_not_applicable(
merge_imports,
r"
use std::fmt<|>::Debug;
pub use std::fmt::Display;
",
);
}
#[test]
fn skip_pub_crate_pub() {
check_assist_not_applicable(
merge_imports,
r"
pub(crate) use std::fmt<|>::Debug;
pub use std::fmt::Display;
",
);
}
#[test]
fn skip_pub_pub_crate() {
check_assist_not_applicable(
merge_imports,
r"
pub use std::fmt<|>::Debug;
pub(crate) use std::fmt::Display;
",
);
}
#[test]
fn merge_pub() {
check_assist(
merge_imports,
r"
pub use std::fmt<|>::Debug;
pub use std::fmt::Display;
",
r"
pub use std::fmt::{Debug, Display};
",
)
}
#[test]
fn merge_pub_crate() {
check_assist(
merge_imports,
r"
pub(crate) use std::fmt<|>::Debug;
pub(crate) use std::fmt::Display;
",
r"
pub(crate) use std::fmt::{Debug, Display};
",
)
}
#[test]
fn test_merge_nested() {
check_assist(

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

@ -138,13 +138,23 @@ pub(crate) fn insert_use(
algo::insert_children(scope.as_syntax_node(), insert_position, to_insert)
}
fn try_merge_imports(
fn eq_visibility(vis0: Option<ast::Visibility>, vis1: Option<ast::Visibility>) -> bool {
match (vis0, vis1) {
(None, None) => true,
// FIXME: Don't use the string representation to check for equality
// spaces inside of the node would break this comparison
(Some(vis0), Some(vis1)) => vis0.to_string() == vis1.to_string(),
_ => false,
}
}
pub(crate) fn try_merge_imports(
old: &ast::Use,
new: &ast::Use,
merge_behaviour: MergeBehaviour,
) -> Option<ast::Use> {
// don't merge into re-exports
if old.visibility().and_then(|vis| vis.pub_token()).is_some() {
// don't merge imports with different visibilities
if !eq_visibility(old.visibility(), new.visibility()) {
return None;
}
let old_tree = old.use_tree()?;
@ -161,7 +171,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(
pub(crate) fn try_merge_trees(
old: &ast::UseTree,
new: &ast::UseTree,
merge_behaviour: MergeBehaviour,
@ -278,7 +288,8 @@ fn first_path(path: &ast::Path) -> ast::Path {
}
fn segment_iter(path: &ast::Path) -> impl Iterator<Item = ast::PathSegment> + Clone {
path.syntax().children().flat_map(ast::PathSegment::cast)
// 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)]
@ -684,8 +695,18 @@ use std::io;",
check_last(
"foo::bar",
r"use foo::bar::baz::Qux;",
r"use foo::bar::baz::Qux;
use foo::bar;",
r"use foo::bar;
use foo::bar::baz::Qux;",
);
}
#[test]
fn insert_short_before_long() {
check_none(
"foo::bar",
r"use foo::bar::baz::Qux;",
r"use foo::bar;
use foo::bar::baz::Qux;",
);
}