From 335edf87bc84bdfcc48ab23f12f606ced09dbac7 Mon Sep 17 00:00:00 2001
From: Kirill Bulatov <mail4score@gmail.com>
Date: Wed, 11 Nov 2020 11:37:41 +0200
Subject: [PATCH] Do not insert imports before inner comments

---
 crates/assists/src/utils/insert_use.rs | 110 +++++++++++++++++++++----
 1 file changed, 92 insertions(+), 18 deletions(-)

diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs
index 84a0dffdd44..c4de83f773a 100644
--- a/crates/assists/src/utils/insert_use.rs
+++ b/crates/assists/src/utils/insert_use.rs
@@ -9,7 +9,7 @@
         edit::{AstNodeEdit, IndentLevel},
         make, AstNode, PathSegmentKind, VisibilityOwner,
     },
-    InsertPosition, SyntaxElement, SyntaxNode,
+    AstToken, InsertPosition, NodeOrToken, SyntaxElement, SyntaxNode, SyntaxToken,
 };
 use test_utils::mark;
 
@@ -63,27 +63,46 @@ fn first_insert_pos(&self) -> (InsertPosition<SyntaxElement>, AddBlankLine) {
         }
     }
 
-    fn insert_pos_after_inner_attribute(&self) -> (InsertPosition<SyntaxElement>, AddBlankLine) {
-        // check if the scope has inner attributes, we dont want to insert in front of them
-        match self
-            .as_syntax_node()
-            .children()
-            // no flat_map here cause we want to short circuit the iterator
-            .map(ast::Attr::cast)
-            .take_while(|attr| {
-                attr.as_ref().map(|attr| attr.kind() == ast::AttrKind::Inner).unwrap_or(false)
-            })
-            .last()
-            .flatten()
-        {
-            Some(attr) => {
-                (InsertPosition::After(attr.syntax().clone().into()), AddBlankLine::BeforeTwice)
+    fn insert_pos_after_inner_elements(&self) -> (InsertPosition<SyntaxElement>, AddBlankLine) {
+        let mut last_inner_element = None;
+
+        for maybe_inner_element in self.as_syntax_node().children_with_tokens() {
+            match maybe_inner_element {
+                NodeOrToken::Node(maybe_inner_node) => {
+                    if is_inner_node(maybe_inner_node.clone()) {
+                        last_inner_element = Some(NodeOrToken::Node(maybe_inner_node))
+                    } else {
+                        if let Some(maybe_inner_token) = maybe_inner_node.first_token() {
+                            if is_inner_token(maybe_inner_token.clone()) {
+                                last_inner_element = Some(NodeOrToken::Token(maybe_inner_token))
+                            }
+                        }
+                    };
+                }
+                NodeOrToken::Token(maybe_inner_token) => {
+                    if is_inner_token(maybe_inner_token.clone()) {
+                        last_inner_element = Some(NodeOrToken::Token(maybe_inner_token))
+                    }
+                }
             }
+        }
+
+        match last_inner_element {
+            Some(element) => (InsertPosition::After(element.into()), AddBlankLine::BeforeTwice),
             None => self.first_insert_pos(),
         }
     }
 }
 
+fn is_inner_node(node: SyntaxNode) -> bool {
+    ast::Attr::cast(node).map(|attr| attr.kind()) == Some(ast::AttrKind::Inner)
+}
+
+fn is_inner_token(token: SyntaxToken) -> bool {
+    ast::Comment::cast(token).and_then(|comment| comment.kind().doc)
+        == Some(ast::CommentPlacement::Inner)
+}
+
 /// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur.
 pub(crate) fn insert_use<'a>(
     scope: &ImportScope,
@@ -558,7 +577,7 @@ fn find_insert_position(
                             (InsertPosition::After(node.into()), AddBlankLine::BeforeTwice)
                         }
                         // there are no imports in this file at all
-                        None => scope.insert_pos_after_inner_attribute(),
+                        None => scope.insert_pos_after_inner_elements(),
                     },
                 }
             }
@@ -830,12 +849,67 @@ fn insert_after_inner_attr2() {
             "foo::bar",
             r"#![allow(unused_imports)]
 
+#![no_std]
 fn main() {}",
             r"#![allow(unused_imports)]
 
-use foo::bar;
+#![no_std]
 
+use foo::bar;
 fn main() {}",
+        );
+    }
+
+    #[test]
+    fn inserts_after_single_line_inner_comments() {
+        check_none(
+            "foo::bar::Baz",
+            "//! Single line inner comments do not allow any code before them.",
+            r#"//! Single line inner comments do not allow any code before them.
+
+use foo::bar::Baz;"#,
+        );
+    }
+
+    #[test]
+    fn inserts_after_multiline_inner_comments() {
+        check_none(
+            "foo::bar::Baz",
+            r#"/*! Multiline inner comments do not allow any code before them. */
+
+/*! RA considers this inner comment belonging to the function, yet we still cannot place the code before it. */
+fn main() {}"#,
+            r#"/*! Multiline inner comments do not allow any code before them. */
+
+/*! RA considers this inner comment belonging to the function, yet we still cannot place the code before it. */
+
+use foo::bar::Baz;
+fn main() {}"#,
+        )
+    }
+
+    #[test]
+    fn inserts_after_all_inner_items() {
+        check_none(
+            "foo::bar::Baz",
+            r#"#![allow(unused_imports)]
+/*! Multiline line comment 2 */
+
+
+//! Single line comment 1
+#![no_std]
+//! Single line comment 2
+fn main() {}"#,
+            r#"#![allow(unused_imports)]
+/*! Multiline line comment 2 */
+
+
+//! Single line comment 1
+#![no_std]
+//! Single line comment 2
+
+use foo::bar::Baz;
+fn main() {}"#,
         )
     }