Merge #5955

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 <[email protected]>

Author: bors[bot]

crates/assists/src/handlers/merge_imports.rs

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

crates/assists/src/utils/insert_use.rs

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