From 335edf87bc84bdfcc48ab23f12f606ced09dbac7 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 11 Nov 2020 11:37:41 +0200 Subject: Do not insert imports before inner comments --- crates/assists/src/utils/insert_use.rs | 110 +++++++++++++++++++++++++++------ 1 file changed, 92 insertions(+), 18 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 84a0dffdd..c4de83f77 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -9,7 +9,7 @@ use syntax::{ edit::{AstNodeEdit, IndentLevel}, make, AstNode, PathSegmentKind, VisibilityOwner, }, - InsertPosition, SyntaxElement, SyntaxNode, + AstToken, InsertPosition, NodeOrToken, SyntaxElement, SyntaxNode, SyntaxToken, }; use test_utils::mark; @@ -63,27 +63,46 @@ impl ImportScope { } } - fn insert_pos_after_inner_attribute(&self) -> (InsertPosition, 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, 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 @@ use foo::bar;", "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() {}"#, ) } -- cgit v1.2.3 From 3481ea96bdf530e70ce0c3568918421564845b0d Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 11 Nov 2020 14:04:31 +0200 Subject: Add a FIXME for non-unified inner attributes --- crates/assists/src/utils/insert_use.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index c4de83f77..5d726370a 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -72,6 +72,11 @@ impl ImportScope { if is_inner_node(maybe_inner_node.clone()) { last_inner_element = Some(NodeOrToken::Node(maybe_inner_node)) } else { + // FIXME: https://doc.rust-lang.org/reference/comments.html#doc-comments + // states that inner comments (`//!` and `/*!`) are equal to inner attribute `#![doc="..."]` + // yet RA treats them differently now: inner attributes never belong to child nodes, + // but inner comments can, ergo this check. + // We need to align this and treat both cases the same way. 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)) @@ -877,11 +882,11 @@ use foo::bar::Baz;"#, "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. */ +/*! Still an inner comment, cannot place any code before. */ 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. */ +/*! Still an inner comment, cannot place any code before. */ use foo::bar::Baz; fn main() {}"#, -- cgit v1.2.3 From 07e633ef0adda5f3ce608762267d67b6359358a7 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 12 Nov 2020 13:52:24 +0200 Subject: Remove the fixme --- crates/assists/src/utils/insert_use.rs | 51 ++++++++++------------------------ 1 file changed, 15 insertions(+), 36 deletions(-) (limited to 'crates/assists') diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 5d726370a..af3fc96b6 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -63,47 +63,26 @@ impl ImportScope { } } - fn insert_pos_after_inner_elements(&self) -> (InsertPosition, 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 { - // FIXME: https://doc.rust-lang.org/reference/comments.html#doc-comments - // states that inner comments (`//!` and `/*!`) are equal to inner attribute `#![doc="..."]` - // yet RA treats them differently now: inner attributes never belong to child nodes, - // but inner comments can, ergo this check. - // We need to align this and treat both cases the same way. - 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 insert_pos_after_last_inner_element(&self) -> (InsertPosition, AddBlankLine) { + self.as_syntax_node() + .children_with_tokens() + .filter(|child| match child { + NodeOrToken::Node(node) => is_inner_attribute(node.clone()), + NodeOrToken::Token(token) => is_inner_comment(token.clone()), + }) + .last() + .map(|last_inner_element| { + (InsertPosition::After(last_inner_element.into()), AddBlankLine::BeforeTwice) + }) + .unwrap_or_else(|| self.first_insert_pos()) } } -fn is_inner_node(node: SyntaxNode) -> bool { +fn is_inner_attribute(node: SyntaxNode) -> bool { ast::Attr::cast(node).map(|attr| attr.kind()) == Some(ast::AttrKind::Inner) } -fn is_inner_token(token: SyntaxToken) -> bool { +fn is_inner_comment(token: SyntaxToken) -> bool { ast::Comment::cast(token).and_then(|comment| comment.kind().doc) == Some(ast::CommentPlacement::Inner) } @@ -582,7 +561,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_elements(), + None => scope.insert_pos_after_last_inner_element(), }, } } -- cgit v1.2.3