Use mutable syntax trees in merge_imports, split_imports

crates/ide_assists/src/handlers/merge_imports.rs

@@ -80,7 +80,7 @@ use std::fmt$0::{Display, Debug};
 use std::fmt::{Display, Debug};
 ",
             r"
-use std::fmt::{Debug, Display};
+use std::fmt::{Display, Debug};
 ",
         )
     }
@@ -108,7 +108,7 @@ use std::fmt::Debug;
 use std::fmt$0::Display;
 ",
             r"
-use std::fmt::{Debug, Display};
+use std::fmt::{Display, Debug};
 ",
         );
     }
@@ -135,7 +135,7 @@ use std::fmt::{self, Display};
 use std::{fmt, $0fmt::Display};
 ",
             r"
-use std::{fmt::{self, Display}};
+use std::{fmt::{Display, self}};
 ",
         );
     }
@@ -247,7 +247,7 @@ use std::{fmt::{Debug, Display}};
 use std::{fmt::Debug, fmt$0::Display};
 ",
             r"
-use std::{fmt::{Debug, Display}};
+use std::{fmt::{Display, Debug}};
 ",
         );
     }
@@ -341,7 +341,9 @@ use foo::$0{
 };
 ",
             r"
-use foo::{FooBar, bar::baz};
+use foo::{
+    FooBar, bar::baz,
+};
 ",
         )
     }

crates/ide_assists/src/handlers/replace_qualified_name_with_use.rs

@@ -299,7 +299,7 @@ fn main() {
     ",
             r"
 mod std { pub mod fmt { pub trait Display {} } }
-use std::fmt::{self, Display};
+use std::fmt::{Display, self};
 
 fn main() {
     fmt;

crates/ide_assists/src/handlers/split_import.rs

@@ -19,14 +19,20 @@ pub(crate) fn split_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()>
 
     let use_tree = path.top_path().syntax().ancestors().find_map(ast::UseTree::cast)?;
 
-    let new_tree = use_tree.split_prefix(&path);
+    let has_errors = use_tree
-    if new_tree == use_tree {
+        .syntax()
+        .descendants_with_tokens()
+        .any(|it| it.kind() == syntax::SyntaxKind::ERROR);
+    let last_segment = use_tree.path().and_then(|it| it.segment());
+    if has_errors || last_segment.is_none() {
         return None;
     }
 
     let target = colon_colon.text_range();
     acc.add(AssistId("split_import", AssistKind::RefactorRewrite), "Split import", target, |edit| {
-        edit.replace_ast(use_tree, new_tree);
+        let use_tree = edit.make_mut(use_tree.clone());
+        let path = edit.make_mut(path);
+        use_tree.split_prefix(&path);
     })
 }
 

crates/ide_db/src/helpers/insert_use/tests.rs

@@ -463,7 +463,7 @@ fn merge_groups_full() {
 
 #[test]
 fn merge_groups_long_full() {
-    check_crate("std::foo::bar::Baz", r"use std::foo::bar::Qux;", r"use std::foo::bar::{Baz, Qux};")
+    check_crate("std::foo::bar::Baz", r"use std::foo::bar::Qux;", r"use std::foo::bar::{Qux, Baz};")
 }
 
 #[test]
@@ -471,7 +471,7 @@ fn merge_groups_long_last() {
     check_module(
         "std::foo::bar::Baz",
         r"use std::foo::bar::Qux;",
-        r"use std::foo::bar::{Baz, Qux};",
+        r"use std::foo::bar::{Qux, Baz};",
     )
 }
 
@@ -480,7 +480,7 @@ fn merge_groups_long_full_list() {
     check_crate(
         "std::foo::bar::Baz",
         r"use std::foo::bar::{Qux, Quux};",
-        r"use std::foo::bar::{Baz, Quux, Qux};",
+        r"use std::foo::bar::{Qux, Quux, Baz};",
     )
 }
 
@@ -489,7 +489,7 @@ fn merge_groups_long_last_list() {
     check_module(
         "std::foo::bar::Baz",
         r"use std::foo::bar::{Qux, Quux};",
-        r"use std::foo::bar::{Baz, Quux, Qux};",
+        r"use std::foo::bar::{Qux, Quux, Baz};",
     )
 }
 
@@ -498,7 +498,7 @@ fn merge_groups_long_full_nested() {
     check_crate(
         "std::foo::bar::Baz",
         r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};",
-        r"use std::foo::bar::{Baz, Qux, quux::{Fez, Fizz}};",
+        r"use std::foo::bar::{Qux, quux::{Fez, Fizz}, Baz};",
     )
 }
 
@@ -517,7 +517,7 @@ fn merge_groups_full_nested_deep() {
     check_crate(
         "std::foo::bar::quux::Baz",
         r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};",
-        r"use std::foo::bar::{Qux, quux::{Baz, Fez, Fizz}};",
+        r"use std::foo::bar::{Qux, quux::{Fez, Fizz, Baz}};",
     )
 }
 
@@ -526,7 +526,7 @@ fn merge_groups_full_nested_long() {
     check_crate(
         "std::foo::bar::Baz",
         r"use std::{foo::bar::Qux};",
-        r"use std::{foo::bar::{Baz, Qux}};",
+        r"use std::{foo::bar::{Qux, Baz}};",
     );
 }
 
@@ -535,7 +535,7 @@ fn merge_groups_last_nested_long() {
     check_crate(
         "std::foo::bar::Baz",
         r"use std::{foo::bar::Qux};",
-        r"use std::{foo::bar::{Baz, Qux}};",
+        r"use std::{foo::bar::{Qux, Baz}};",
     );
 }
 
@@ -600,7 +600,7 @@ fn merge_mod_into_glob() {
     check_with_config(
         "token::TokenKind",
         r"use token::TokenKind::*;",
-        r"use token::TokenKind::{*, self};",
+        r"use token::TokenKind::{self, *};",
         &InsertUseConfig {
             granularity: ImportGranularity::Crate,
             enforce_granularity: true,
@@ -618,7 +618,7 @@ fn merge_self_glob() {
     check_with_config(
         "self",
         r"use self::*;",
-        r"use self::{*, self};",
+        r"use self::{self, *};",
         &InsertUseConfig {
             granularity: ImportGranularity::Crate,
             enforce_granularity: true,
@@ -637,7 +637,7 @@ fn merge_glob() {
         r"
 use syntax::{SyntaxKind::*};",
         r"
-use syntax::{SyntaxKind::{*, self}};",
+use syntax::{SyntaxKind::{self, *}};",
     )
 }
 

crates/ide_db/src/helpers/merge_imports.rs

@@ -44,10 +44,10 @@ pub fn try_merge_imports(
     }
 
     let lhs = lhs.clone_subtree().clone_for_update();
+    let rhs = rhs.clone_subtree().clone_for_update();
     let lhs_tree = lhs.use_tree()?;
     let rhs_tree = rhs.use_tree()?;
-    let merged = try_merge_trees(&lhs_tree, &rhs_tree, merge_behavior)?;
+    try_merge_trees_mut(&lhs_tree, &rhs_tree, merge_behavior)?;
-    ted::replace(lhs_tree.syntax(), merged.syntax());
     Some(lhs)
 }
 
@@ -56,57 +56,49 @@ pub fn try_merge_trees(
     rhs: &ast::UseTree,
     merge: MergeBehavior,
 ) -> Option<ast::UseTree> {
+    let lhs = lhs.clone_subtree().clone_for_update();
+    let rhs = rhs.clone_subtree().clone_for_update();
+    try_merge_trees_mut(&lhs, &rhs, merge)?;
+    Some(lhs)
+}
+
+fn try_merge_trees_mut(lhs: &ast::UseTree, rhs: &ast::UseTree, merge: MergeBehavior) -> Option<()> {
     let lhs_path = lhs.path()?;
     let rhs_path = rhs.path()?;
 
     let (lhs_prefix, rhs_prefix) = common_prefix(&lhs_path, &rhs_path)?;
-    let (lhs, rhs) = if lhs.is_simple_path()
+    if !(lhs.is_simple_path()
         && rhs.is_simple_path()
         && lhs_path == lhs_prefix
-        && rhs_path == rhs_prefix
+        && rhs_path == rhs_prefix)
     {
-        (lhs.clone(), rhs.clone())
+        lhs.split_prefix(&lhs_prefix);
-    } else {
+        rhs.split_prefix(&rhs_prefix);
-        (lhs.split_prefix(&lhs_prefix), rhs.split_prefix(&rhs_prefix))
+    }
-    };
+    recursive_merge(&lhs, &rhs, merge)
-    recursive_merge(&lhs, &rhs, merge).map(|it| it.clone_for_update())
 }
 
-/// Recursively "zips" together lhs and rhs.
+/// Recursively merges rhs to lhs
-fn recursive_merge(
+#[must_use]
-    lhs: &ast::UseTree,
+fn recursive_merge(lhs: &ast::UseTree, rhs: &ast::UseTree, merge: MergeBehavior) -> Option<()> {
-    rhs: &ast::UseTree,
+    let mut use_trees: Vec<ast::UseTree> = lhs
-    merge: MergeBehavior,
-) -> Option<ast::UseTree> {
-    let mut use_trees = lhs
         .use_tree_list()
         .into_iter()
         .flat_map(|list| list.use_trees())
         // We use Option here to early return from this function(this is not the
         // same as a `filter` op).
-        .map(|tree| match merge.is_tree_allowed(&tree) {
+        .map(|tree| merge.is_tree_allowed(&tree).then(|| tree))
-            true => Some(tree),
+        .collect::<Option<_>>()?;
-            false => None,
-        })
-        .collect::<Option<Vec<_>>>()?;
     use_trees.sort_unstable_by(|a, b| path_cmp_for_sort(a.path(), b.path()));
     for rhs_t in rhs.use_tree_list().into_iter().flat_map(|list| list.use_trees()) {
         if !merge.is_tree_allowed(&rhs_t) {
             return None;
         }
         let rhs_path = rhs_t.path();
-        match use_trees.binary_search_by(|lhs_t| {
-            let (lhs_t, rhs_t) = match lhs_t
-                .path()
-                .zip(rhs_path.clone())
-                .and_then(|(lhs, rhs)| common_prefix(&lhs, &rhs))
-            {
-                Some((lhs_p, rhs_p)) => (lhs_t.split_prefix(&lhs_p), rhs_t.split_prefix(&rhs_p)),
-                None => (lhs_t.clone(), rhs_t.clone()),
-            };
 
-            path_cmp_bin_search(lhs_t.path(), rhs_t.path())
+        match use_trees
-        }) {
+            .binary_search_by(|lhs_t| path_cmp_bin_search(lhs_t.path(), rhs_path.as_ref()))
+        {
             Ok(idx) => {
                 let lhs_t = &mut use_trees[idx];
                 let lhs_path = lhs_t.path()?;
@@ -128,6 +120,7 @@ fn recursive_merge(
                     match (tree_contains_self(lhs_t), tree_contains_self(&rhs_t)) {
                         (true, false) => continue,
                         (false, true) => {
+                            ted::replace(lhs_t.syntax(), rhs_t.syntax());
                             *lhs_t = rhs_t;
                             continue;
                         }
@@ -142,25 +135,27 @@ fn recursive_merge(
                     if lhs_t.star_token().is_some() || rhs_t.star_token().is_some() {
                         if tree_is_self(lhs_t) || tree_is_self(&rhs_t) {
                             cov_mark::hit!(merge_self_glob);
-                            *lhs_t = make::use_tree(
+                            let self_tree = make::use_tree(
                                 make::path_unqualified(make::path_segment_self()),
                                 None,
                                 None,
                                 false,
-                            );
+                            )
-                            use_trees.insert(idx, make::use_tree_glob());
+                            .clone_for_update();
+                            ted::replace(lhs_t.syntax(), self_tree.syntax());
+                            *lhs_t = self_tree;
+                            let glob = make::use_tree_glob().clone_for_update();
+                            use_trees.insert(idx, glob.clone());
+                            lhs.get_or_create_use_tree_list().add_use_tree(glob);
                             continue;
                         }
                     } else if lhs_t.use_tree_list().is_none() && rhs_t.use_tree_list().is_none() {
                         continue;
                     }
                 }
-                let lhs = lhs_t.split_prefix(&lhs_prefix);
+                lhs_t.split_prefix(&lhs_prefix);
-                let rhs = rhs_t.split_prefix(&rhs_prefix);
+                rhs_t.split_prefix(&rhs_prefix);
-                match recursive_merge(&lhs, &rhs, merge) {
+                recursive_merge(&lhs_t, &rhs_t, merge)?;
-                    Some(use_tree) => use_trees[idx] = use_tree,
-                    None => return None,
-                }
             }
             Err(_)
                 if merge == MergeBehavior::Module
@@ -170,16 +165,12 @@ fn recursive_merge(
                 return None
             }
             Err(idx) => {
-                use_trees.insert(idx, rhs_t);
+                use_trees.insert(idx, rhs_t.clone());
+                lhs.get_or_create_use_tree_list().add_use_tree(rhs_t);
             }
         }
     }
-
+    Some(())
-    let lhs = lhs.clone_subtree().clone_for_update();
-    if let Some(old) = lhs.use_tree_list() {
-        ted::replace(old.syntax(), make::use_tree_list(use_trees).syntax().clone_for_update());
-    }
-    ast::UseTree::cast(lhs.syntax().clone_subtree())
 }
 
 /// Traverses both paths until they differ, returning the common prefix of both.
@@ -224,11 +215,9 @@ fn path_cmp_for_sort(a: Option<ast::Path>, b: Option<ast::Path>) -> Ordering {
 }
 
 /// Path comparison func for binary searching for merging.
-fn path_cmp_bin_search(lhs: Option<ast::Path>, rhs: Option<ast::Path>) -> Ordering {
+fn path_cmp_bin_search(lhs: Option<ast::Path>, rhs: Option<&ast::Path>) -> Ordering {
-    match (
+    match (lhs.as_ref().and_then(ast::Path::first_segment), rhs.and_then(ast::Path::first_segment))
-        lhs.as_ref().and_then(ast::Path::first_segment),
+    {
-        rhs.as_ref().and_then(ast::Path::first_segment),
-    ) {
         (None, None) => Ordering::Equal,
         (None, Some(_)) => Ordering::Less,
         (Some(_), None) => Ordering::Greater,

crates/syntax/src/ast/edit.rs

@@ -4,48 +4,10 @@
 use std::{fmt, iter, ops};
 
 use crate::{
-    algo,
     ast::{self, make, AstNode},
     ted, AstToken, NodeOrToken, SyntaxElement, SyntaxNode, SyntaxToken,
 };
 
-impl ast::UseTree {
-    /// 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() {
-            make::path_unqualified(make::path_segment_self())
-        } else {
-            match split_path_prefix(prefix) {
-                Some(it) => it,
-                None => return self.clone(),
-            }
-        };
-
-        let use_tree = make::use_tree(
-            suffix,
-            self.use_tree_list(),
-            self.rename(),
-            self.star_token().is_some(),
-        );
-        let nested = make::use_tree_list(iter::once(use_tree));
-        return make::use_tree(prefix.clone(), Some(nested), None, false);
-
-        fn split_path_prefix(prefix: &ast::Path) -> Option<ast::Path> {
-            let parent = prefix.parent_path()?;
-            let segment = parent.segment()?;
-            if algo::has_errors(segment.syntax()) {
-                return None;
-            }
-            let mut res = make::path_unqualified(segment);
-            for p in iter::successors(parent.parent_path(), |it| it.parent_path()) {
-                res = make::path_qualified(res, p.segment()?);
-            }
-            Some(res)
-        }
-    }
-}
-
 #[derive(Debug, Clone, Copy)]
 pub struct IndentLevel(pub u8);
 

crates/syntax/src/ast/edit_in_place.rs

@@ -1,12 +1,12 @@
 //! Structural editing for ast.
 
-use std::iter::empty;
+use std::iter::{empty, successors};
 
 use parser::{SyntaxKind, T};
 use rowan::SyntaxElement;
 
 use crate::{
-    algo::neighbor,
+    algo::{self, neighbor},
     ast::{self, edit::IndentLevel, make, HasGenericParams},
     ted::{self, Position},
     AstNode, AstToken, Direction,
@@ -282,6 +282,78 @@ impl ast::UseTree {
         }
         ted::remove(self.syntax());
     }
+
+    pub fn get_or_create_use_tree_list(&self) -> ast::UseTreeList {
+        match self.use_tree_list() {
+            Some(it) => it,
+            None => {
+                let position = Position::last_child_of(self.syntax());
+                let use_tree_list = make::use_tree_list(empty()).clone_for_update();
+                let mut elements = Vec::with_capacity(2);
+                if self.coloncolon_token().is_none() {
+                    elements.push(make::token(T![::]).into());
+                }
+                elements.push(use_tree_list.syntax().clone().into());
+                ted::insert_all_raw(position, elements);
+                use_tree_list
+            }
+        }
+    }
+
+    /// Splits off the given prefix, making it the path component of the use tree,
+    /// appending the rest of the path to all UseTreeList items.
+    pub fn split_prefix(&self, prefix: &ast::Path) {
+        debug_assert_eq!(self.path(), Some(prefix.top_path()));
+        let path = self.path().unwrap();
+        if &path == prefix && self.use_tree_list().is_none() {
+            let self_suffix = make::path_unqualified(make::path_segment_self()).clone_for_update();
+            ted::replace(path.syntax(), self_suffix.syntax());
+        } else if split_path_prefix(prefix).is_none() {
+            return;
+        }
+
+        let subtree = self.clone_subtree().clone_for_update();
+        ted::remove_all_iter(self.syntax().children_with_tokens());
+        ted::insert(Position::first_child_of(self.syntax()), prefix.syntax());
+        self.get_or_create_use_tree_list().add_use_tree(subtree);
+
+        fn split_path_prefix(prefix: &ast::Path) -> Option<()> {
+            let parent = prefix.parent_path()?;
+            let segment = parent.segment()?;
+            if algo::has_errors(segment.syntax()) {
+                return None;
+            }
+            for p in successors(parent.parent_path(), |it| it.parent_path()) {
+                p.segment()?;
+            }
+            prefix.parent_path().and_then(|p| p.coloncolon_token()).map(ted::remove);
+            ted::remove(prefix.syntax());
+            Some(())
+        }
+    }
+}
+
+impl ast::UseTreeList {
+    pub fn add_use_tree(&self, use_tree: ast::UseTree) {
+        let (position, elements) = match self.use_trees().last() {
+            Some(last_tree) => (
+                Position::after(last_tree.syntax()),
+                vec![
+                    make::token(T![,]).into(),
+                    make::tokens::single_space().into(),
+                    use_tree.syntax.into(),
+                ],
+            ),
+            None => {
+                let position = match self.l_curly_token() {
+                    Some(l_curly) => Position::after(l_curly),
+                    None => Position::last_child_of(self.syntax()),
+                };
+                (position, vec![use_tree.syntax.into()])
+            }
+        };
+        ted::insert_all_raw(position, elements);
+    }
 }
 
 impl ast::Use {