Repository: intellij-rust/intellij-rust
Issue: Move refactoring jumbles up mods/uses

<!--
Hello and thank you for the issue!
If you would like to report a bug, we have added some points below that you can fill out.
Consider using "Help | Create New Issue" action that fills out all necessary information automatically.
Feel free to remove all the irrelevant text to request a new feature.
-->

## Environment

* **IntelliJ Rust plugin version:** 0.4.152.4026-212
* **Rust toolchain version:** 1.54
* **IDE name and version:** CLion 2021.2
* **Operating system:** Linux

## Problem description
When performing the move refactoring, files that need to be modified by it end up with all the mod/use statements jumbled around. For example, I usually have `mod` above `use`, the refactoring will reverse that among other changes. It'd be really desirable if it made the minimum amount of changes necessary rather than reorganizing everything

## Steps to reproduce
Use move refactoring to, ex, move a struct definition from one module to another.

Changed files:
src/main/kotlin/org/rust/ide/refactoring/move/common/RsMoveRetargetReferencesProcessor.kt
src/test/kotlin/org/rust/ide/refactoring/move/RsMoveTopLevelItemsTest.kt
src/test/kotlin/org/rust/ide/refactoring/move/RsMoveFileReexportTest.kt
src/main/kotlin/org/rust/ide/refactoring/RsImportOptimizer.kt

diff --git a/src/main/kotlin/org/rust/ide/refactoring/RsImportOptimizer.kt b/src/main/kotlin/org/rust/ide/refactoring/RsImportOptimizer.kt
index cacf4b454..b636417f7 100644
--- a/src/main/kotlin/org/rust/ide/refactoring/RsImportOptimizer.kt
+++ b/src/main/kotlin/org/rust/ide/refactoring/RsImportOptimizer.kt
@@ -26,11 +26,11 @@ class RsImportOptimizer : ImportOptimizer {
         if (document != null) {
             documentManager.commitDocument(document)
         }
-        executeForUseItem(file as RsFile)
-        executeForExternCrate(file)
+        optimizeAndReorderUseItems(file as RsFile)
+        reorderExternCrates(file)
     }
 
-    private fun executeForExternCrate(file: RsFile) {
+    private fun reorderExternCrates(file: RsFile) {
         val first = file.childrenOfType<RsElement>()
             .firstOrNull { it !is RsInnerAttr } ?: return
         val externCrateItems = file.childrenOfType<RsExternCrateItem>()
@@ -42,27 +42,39 @@ class RsImportOptimizer : ImportOptimizer {
         externCrateItems.forEach { it.delete() }
     }
 
-    fun executeForUseItem(mod: RsMod) {
+    private fun optimizeAndReorderUseItems(mod: RsMod) {
         val uses = mod.childrenOfType<RsUseItem>()
         if (uses.isNotEmpty()) {
             replaceOrderOfUseItems(mod, uses)
         }
         val mods = mod.childrenOfType<RsMod>()
-        mods.forEach { executeForUseItem(it) }
+        mods.forEach { optimizeAndReorderUseItems(it) }
     }
 
     companion object {
 
-        /** Returns false if [useSpeck] is empty and should be removed */
-        fun optimizeUseSpeck(psiFactory: RsPsiFactory, useSpeck: RsUseSpeck): Boolean {
-            val checkUnusedImports = RsUnusedImportInspection.isEnabled(useSpeck.project)
-            return optimizeUseSpeck(psiFactory, useSpeck, checkUnusedImports)
+        fun optimizeUseItems(mod: RsMod) {
+            val psiFactory = RsPsiFactory(mod.project)
+            val uses = mod.childrenOfType<RsUseItem>()
+            uses.forEach { optimizeUseItem(psiFactory, it) }
+            val mods = mod.childrenOfType<RsMod>()
+            mods.forEach { optimizeUseItems(it) }
+        }
+
+        private fun optimizeUseItem(psiFactory: RsPsiFactory, useItem: RsUseItem) {
+            val useSpeck = useItem.useSpeck ?: return
+            val used = optimizeUseSpeck(psiFactory, useSpeck)
+            if (!used) {
+                (useItem.nextSibling as? PsiWhiteSpace)?.delete()
+                useItem.delete()
+            }
         }
 
+        /** Returns false if [useSpeck] is empty and should be removed */
         private fun optimizeUseSpeck(
             psiFactory: RsPsiFactory,
             useSpeck: RsUseSpeck,
-            checkUnusedImports: Boolean
+            checkUnusedImports: Boolean = RsUnusedImportInspection.isEnabled(useSpeck.project),
         ): Boolean {
             val useGroup = useSpeck.useGroup
             if (useGroup == null) {
diff --git a/src/main/kotlin/org/rust/ide/refactoring/move/common/RsMoveRetargetReferencesProcessor.kt b/src/main/kotlin/org/rust/ide/refactoring/move/common/RsMoveRetargetReferencesProcessor.kt
index 3f8fd8e3d..6b12182e3 100644
--- a/src/main/kotlin/org/rust/ide/refactoring/move/common/RsMoveRetargetReferencesProcessor.kt
+++ b/src/main/kotlin/org/rust/ide/refactoring/move/common/RsMoveRetargetReferencesProcessor.kt
@@ -27,7 +27,6 @@ class RsMoveRetargetReferencesProcessor(
     private val psiFactory: RsPsiFactory = RsPsiFactory(project)
     private val codeFragmentFactory: RsCodeFragmentFactory = RsCodeFragmentFactory(project)
 
-    private val useSpecksToOptimize: MutableList<RsUseSpeck> = mutableListOf()
     private val filesToOptimizeImports: MutableSet<RsFile> =
         mutableSetOf(sourceMod.containingFile as RsFile, targetMod.containingFile as RsFile)
 
@@ -223,7 +222,6 @@ class RsMoveRetargetReferencesProcessor(
         // we can't optimize use speck right now,
         // because otherwise `use mod1::{foo2};` becomes `use mod1::foo2;` which destroys `foo2` reference
         useGroup.parentUseSpeck.let {
-            useSpecksToOptimize += it
             filesToOptimizeImports += it.containingFile as RsFile
         }
         return true
@@ -231,14 +229,11 @@ class RsMoveRetargetReferencesProcessor(
 
     private fun insertUseItemAndCopyAttributes(useSpeckText: String, existingUseItem: RsUseItem) {
         val containingMod = existingUseItem.containingMod
-        if (existingUseItem.outerAttrList.isEmpty() && existingUseItem.vis == null) {
-            containingMod.insertUseItem(psiFactory, useSpeckText)
-        } else {
-            val useItem = existingUseItem.copy() as RsUseItem
-            val useSpeck = psiFactory.createUseSpeck(useSpeckText)
-            useItem.useSpeck!!.replace(useSpeck)
-            containingMod.addAfter(useItem, existingUseItem)
-        }
+        val useItem = existingUseItem.copy() as RsUseItem
+        val useSpeck = psiFactory.createUseSpeck(useSpeckText)
+        useItem.useSpeck!!.replace(useSpeck)
+        containingMod.insertUseItem(psiFactory, useItem)
+
         filesToOptimizeImports.add(containingMod.containingFile as RsFile)
     }
 
@@ -248,7 +243,6 @@ class RsMoveRetargetReferencesProcessor(
     }
 
     fun optimizeImports() {
-        useSpecksToOptimize.forEach { RsImportOptimizer.optimizeUseSpeck(psiFactory, it) }
-        filesToOptimizeImports.forEach { RsImportOptimizer().executeForUseItem(it) }
+        filesToOptimizeImports.forEach { RsImportOptimizer.optimizeUseItems(it) }
     }
 }
diff --git a/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveFileReexportTest.kt b/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveFileReexportTest.kt
index 681f8cba7..861020572 100644
--- a/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveFileReexportTest.kt
+++ b/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveFileReexportTest.kt
@@ -43,7 +43,6 @@ class RsMoveFileReexportTest : RsMoveFileTestBase() {
         mod mod2;
     //- mod1/mod.rs
         pub use mod1_inner::mod1_func;
-
         mod mod1_inner {  // private
             pub fn mod1_func() {}
         }
@@ -226,7 +225,6 @@ class RsMoveFileReexportTest : RsMoveFileTestBase() {
     //- mod1/mod.rs
         pub mod inner1 {
             pub use inner2::*;
-
             mod inner2 {
                 pub fn inner2_func() {}
             }
@@ -291,7 +289,6 @@ class RsMoveFileReexportTest : RsMoveFileTestBase() {
 
         pub mod inner1 {
             use inner2::*;
-
             mod inner2 {
                 pub fn inner2_func() {}
             }
diff --git a/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveTopLevelItemsTest.kt b/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveTopLevelItemsTest.kt
index 8722d396c..66f0a8766 100644
--- a/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveTopLevelItemsTest.kt
+++ b/src/test/kotlin/org/rust/ide/refactoring/move/RsMoveTopLevelItemsTest.kt
@@ -1452,7 +1452,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
         mod mod2 {
             pub mod inner1 {
                 pub use inner2::*;
-
                 mod inner2 { pub fn bar2() {} }
                 pub fn bar1() {}
             }
@@ -1706,7 +1705,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
         mod mod2 {
             mod foo1 {
                 use crate::mod2;
-
                 fn test() { mod2::bar1(); }
             }
 
@@ -1744,7 +1742,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
         mod mod2 {
             mod foo1 {
                 use crate::mod2::Bar1;
-
                 fn test() { let _ = Bar1 {}; }
             }
 
@@ -1851,7 +1848,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
 
             pub mod inner1 {
                 pub use inner2::*;
-
                 mod inner2 { pub fn foo3() {} }
                 pub fn foo2() {}
             }
@@ -2038,7 +2034,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
         mod usage {
             use crate::mod2::foo1;
             use crate::mod2::Foo2;
-
             fn test() {
                 foo1();
                 let _ = Foo2 {};
@@ -2072,7 +2067,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
 
         mod usage {
             use crate::mod2;
-
             fn test() {
                 mod2::foo1();
                 let _ = mod2::Foo2 {};
@@ -2151,7 +2145,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
         mod inner {
             // can't use this reexport for foo
             pub use mod2::bar;
-
             pub mod mod2 {
                 pub fn bar() {}
 
@@ -2191,7 +2184,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod inner1 {
             pub use bar::*;
-
             mod mod1 {}
             // private
             mod bar { pub fn bar_func() {} }
@@ -2215,7 +2207,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod mod1 {
             pub use mod1_inner::bar;
-
             mod mod1_inner {
                 pub fn bar() {}
             }
@@ -2244,7 +2235,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod inner {
             pub use mod1::*;
-
             // private
             mod mod1 {}
             pub mod mod2 {
@@ -2275,7 +2265,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod inner {
             pub use mod2::*;
-
             pub mod mod1 {}
             // private
             mod mod2 {
@@ -2307,7 +2296,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod inner1 {
             pub use inner2::*;
-
             mod inner2 {
                 pub mod mod1 {}
                 pub mod mod2 {
@@ -2340,7 +2328,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod inner1 {
             pub use inner2::*;
-
             mod inner2 {  // private
                 pub mod mod1 {}
                 pub mod mod2 {
@@ -2632,7 +2619,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod mod1 {
             use Bar::*;
-
             pub enum Bar { Bar1, Bar2 }
         }
         mod mod2 {
@@ -2689,7 +2675,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
     //- lib.rs
         mod mod1 {
             use Bar::*;
-
             pub enum Bar { Bar1, Bar2 }
         }
         mod mod2 {
@@ -2872,7 +2857,6 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
 
             mod inner2 {
                 use crate::mod2::inner1::Foo as Bar;
-
                 fn test(bar: Bar) {}
             }
         }
@@ -2898,13 +2882,58 @@ class RsMoveTopLevelItemsTest : RsMoveTopLevelItemsTestBase() {
         }
         mod usage {
             use crate::mod2::foo as bar;
-
             fn test() {
                 bar();
             }
         }
     """)
 
+    fun `test don't reorder use items`() = doTest("""
+    //- lib.rs
+        mod mod1 {
+            mod inner {}
+            use aaa;
+            fn foo/*caret*/() {}
+        }
+        mod mod2/*target*/ {}
+    """, """
+    //- lib.rs
+        mod mod1 {
+            mod inner {}
+            use aaa;
+        }
+        mod mod2 {
+            fn foo() {}
+        }
+    """)
+
+    fun `test insert import at correct location`() = doTest("""
+    //- lib.rs
+        mod mod1 {
+            use crate::mod1::A;
+            use crate::mod3::C;
+            fn foo/*caret*/() {}
+            fn bar() {
+                foo();
+            }
+        }
+        mod mod2/*target*/ {}
+    """, """
+    //- lib.rs
+        mod mod1 {
+            use crate::mod1::A;
+            use crate::mod2;
+            use crate::mod3::C;
+
+            fn bar() {
+                mod2::foo();
+            }
+        }
+        mod mod2 {
+            pub fn foo() {}
+        }
+    """)
+
     fun `test move create file 1`() = doTestCreateFile("foo.rs", """
     //- lib.rs
         fn func/*caret*/() {}