From 27b65ec91d7c84908ba680da754e5af89ef57c41 Mon Sep 17 00:00:00 2001 From: TopGunSnake Date: Mon, 18 Jul 2022 19:55:33 -0500 Subject: [PATCH 1/5] Add test case and token finder to address 12790 --- .../src/handlers/extract_module.rs | 146 +++++++++++++++++- 1 file changed, 144 insertions(+), 2 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 11349b45d3c3..b54090b875f1 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -19,7 +19,7 @@ use syntax::{ make, HasName, HasVisibility, }, match_ast, ted, AstNode, SourceFile, - SyntaxKind::WHITESPACE, + SyntaxKind::{self, WHITESPACE}, SyntaxNode, TextRange, }; @@ -380,7 +380,24 @@ impl Module { } for (vis, syntax) in replacements { - add_change_vis(vis, syntax.first_child_or_token()); + let item = syntax.children_with_tokens().find(|node_or_token| { + match node_or_token.kind() { + // We're looking for the start of functions, impls, structs, traits, and other documentable/attribute + // macroable items that would have pub(crate) in front of it + SyntaxKind::FN_KW + | SyntaxKind::IMPL_KW + | SyntaxKind::STRUCT_KW + | SyntaxKind::TRAIT_KW + | SyntaxKind::TYPE_KW + | SyntaxKind::MOD_KW => true, + // If we didn't find a keyword, we want to cover the record fields + SyntaxKind::NAME => true, + // Otherwise, the token shouldn't have pub(crate) before it + _ => false, + } + }); + + add_change_vis(vis, item); } } @@ -1581,4 +1598,129 @@ mod modname { ", ) } + + #[test] + fn test_issue_12790() { + check_assist( + extract_module, + r" + $0/// A documented function + fn documented_fn() {} + + // A commented function with a #[] attribute macro + #[cfg(test)] + fn attribute_fn() {} + + // A normally commented function + fn normal_fn() {} + + /// A documented Struct + struct DocumentedStruct { + // Normal field + x: i32, + + /// Documented field + y: i32, + + // Macroed field + #[cfg(test)] + z: i32, + } + + // A macroed Struct + #[cfg(test)] + struct MacroedStruct { + // Normal field + x: i32, + + /// Documented field + y: i32, + + // Macroed field + #[cfg(test)] + z: i32, + } + + // A normal Struct + struct NormalStruct { + // Normal field + x: i32, + + /// Documented field + y: i32, + + // Macroed field + #[cfg(test)] + z: i32, + } + + /// A documented type + type DocumentedType = i32; + + // A macroed type + #[cfg(test)] + type MacroedType = i32;$0 + ", + r" + mod modname { + /// A documented function + pub(crate) fn documented_fn() {} + + // A commented function with a #[] attribute macro + #[cfg(test)] + pub(crate) fn attribute_fn() {} + + // A normally commented function + pub(crate) fn normal_fn() {} + + /// A documented Struct + pub(crate) struct DocumentedStruct { + // Normal field + pub(crate) x: i32, + + /// Documented field + pub(crate) y: i32, + + // Macroed field + #[cfg(test)] + pub(crate) z: i32, + } + + // A macroed Struct + #[cfg(test)] + pub(crate) struct MacroedStruct { + // Normal field + pub(crate) x: i32, + + /// Documented field + pub(crate) y: i32, + + // Macroed field + #[cfg(test)] + pub(crate) z: i32, + } + + // A normal Struct + pub(crate) struct NormalStruct { + // Normal field + pub(crate) x: i32, + + /// Documented field + pub(crate) y: i32, + + // Macroed field + #[cfg(test)] + pub(crate) z: i32, + } + + /// A documented type + pub(crate) type DocumentedType = i32; + + // A macroed type + #[cfg(test)] + pub(crate) type MacroedType = i32; + } + ", + ) + } } From 3203cb124ef48fa737901c59b40e752dc00a9ae0 Mon Sep 17 00:00:00 2001 From: TopGunSnake Date: Mon, 18 Jul 2022 20:17:42 -0500 Subject: [PATCH 2/5] Added coverage for trait, mod, impl, and enum cases. --- .../src/handlers/extract_module.rs | 52 +++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index b54090b875f1..3f93ebc7de8a 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -385,12 +385,12 @@ impl Module { // We're looking for the start of functions, impls, structs, traits, and other documentable/attribute // macroable items that would have pub(crate) in front of it SyntaxKind::FN_KW - | SyntaxKind::IMPL_KW | SyntaxKind::STRUCT_KW + | SyntaxKind::ENUM_KW | SyntaxKind::TRAIT_KW | SyntaxKind::TYPE_KW | SyntaxKind::MOD_KW => true, - // If we didn't find a keyword, we want to cover the record fields + // If we didn't find a keyword, we want to cover the record fields in a struct SyntaxKind::NAME => true, // Otherwise, the token shouldn't have pub(crate) before it _ => false, @@ -1659,7 +1659,30 @@ mod modname { // A macroed type #[cfg(test)] - type MacroedType = i32;$0 + type MacroedType = i32; + + /// A module to move + mod module {} + + /// An impl to move + impl NormalStruct { + /// A method + fn new() {} + } + + /// A documented trait + trait DocTrait { + /// Inner function + fn doc() {} + } + + /// An enum + enum DocumentedEnum { + /// A variant + A, + /// Another variant + B { x: i32, y: i32 } + }$0 ", r" mod modname { @@ -1719,6 +1742,29 @@ mod modname { // A macroed type #[cfg(test)] pub(crate) type MacroedType = i32; + + /// A module to move + pub(crate) mod module {} + + /// An impl to move + impl NormalStruct { + /// A method + pub(crate) fn new() {} + } + + /// A documented trait + pub(crate) trait DocTrait { + /// Inner function + fn doc() {} + } + + /// An enum + pub(crate) enum DocumentedEnum { + /// A variant + A, + /// Another variant + B { x: i32, y: i32 } + } } ", ) From 3cb78ffa82328a9a67cedc614da75092026d2890 Mon Sep 17 00:00:00 2001 From: TopGunSnake Date: Mon, 18 Jul 2022 20:29:13 -0500 Subject: [PATCH 3/5] Cleaned up trailing whitespace for tidy::files_are_tidy --- .../src/handlers/extract_module.rs | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 3f93ebc7de8a..cf7d35d673d1 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -1606,49 +1606,49 @@ mod modname { r" $0/// A documented function fn documented_fn() {} - + // A commented function with a #[] attribute macro #[cfg(test)] fn attribute_fn() {} - + // A normally commented function fn normal_fn() {} - + /// A documented Struct struct DocumentedStruct { // Normal field x: i32, - + /// Documented field y: i32, - + // Macroed field #[cfg(test)] z: i32, } - + // A macroed Struct #[cfg(test)] struct MacroedStruct { // Normal field x: i32, - + /// Documented field y: i32, - + // Macroed field #[cfg(test)] z: i32, } - + // A normal Struct struct NormalStruct { // Normal field x: i32, - + /// Documented field y: i32, - + // Macroed field #[cfg(test)] z: i32, @@ -1660,10 +1660,10 @@ mod modname { // A macroed type #[cfg(test)] type MacroedType = i32; - + /// A module to move mod module {} - + /// An impl to move impl NormalStruct { /// A method @@ -1688,49 +1688,49 @@ mod modname { mod modname { /// A documented function pub(crate) fn documented_fn() {} - + // A commented function with a #[] attribute macro #[cfg(test)] pub(crate) fn attribute_fn() {} - + // A normally commented function pub(crate) fn normal_fn() {} - + /// A documented Struct pub(crate) struct DocumentedStruct { // Normal field pub(crate) x: i32, - + /// Documented field pub(crate) y: i32, - + // Macroed field #[cfg(test)] pub(crate) z: i32, } - + // A macroed Struct #[cfg(test)] pub(crate) struct MacroedStruct { // Normal field pub(crate) x: i32, - + /// Documented field pub(crate) y: i32, - + // Macroed field #[cfg(test)] pub(crate) z: i32, } - + // A normal Struct pub(crate) struct NormalStruct { // Normal field pub(crate) x: i32, - + /// Documented field pub(crate) y: i32, - + // Macroed field #[cfg(test)] pub(crate) z: i32, From 09da74a35d7d037bc78f964c69982f8d3ffa03e6 Mon Sep 17 00:00:00 2001 From: TopGunSnake Date: Mon, 18 Jul 2022 20:48:01 -0500 Subject: [PATCH 4/5] Added case for const --- crates/ide-assists/src/handlers/extract_module.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index cf7d35d673d1..2b2e1e9bbd36 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -389,6 +389,7 @@ impl Module { | SyntaxKind::ENUM_KW | SyntaxKind::TRAIT_KW | SyntaxKind::TYPE_KW + | SyntaxKind::CONST_KW | SyntaxKind::MOD_KW => true, // If we didn't find a keyword, we want to cover the record fields in a struct SyntaxKind::NAME => true, @@ -1682,7 +1683,10 @@ mod modname { A, /// Another variant B { x: i32, y: i32 } - }$0 + } + + /// Documented const + const MY_CONST: i32 = 0;$0 ", r" mod modname { @@ -1765,6 +1769,9 @@ mod modname { /// Another variant B { x: i32, y: i32 } } + + /// Documented const + pub(crate) const MY_CONST: i32 = 0; } ", ) From 6df414faa25ffbf5379c18ff054c7cd1913df66b Mon Sep 17 00:00:00 2001 From: TopGunSnake Date: Tue, 19 Jul 2022 18:08:05 -0500 Subject: [PATCH 5/5] Inverted the match logic to skip comments, attribute macros, and whitespace before the appropriate keywords. --- .../ide-assists/src/handlers/extract_module.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/crates/ide-assists/src/handlers/extract_module.rs b/crates/ide-assists/src/handlers/extract_module.rs index 2b2e1e9bbd36..154856df5482 100644 --- a/crates/ide-assists/src/handlers/extract_module.rs +++ b/crates/ide-assists/src/handlers/extract_module.rs @@ -382,19 +382,10 @@ impl Module { for (vis, syntax) in replacements { let item = syntax.children_with_tokens().find(|node_or_token| { match node_or_token.kind() { - // We're looking for the start of functions, impls, structs, traits, and other documentable/attribute - // macroable items that would have pub(crate) in front of it - SyntaxKind::FN_KW - | SyntaxKind::STRUCT_KW - | SyntaxKind::ENUM_KW - | SyntaxKind::TRAIT_KW - | SyntaxKind::TYPE_KW - | SyntaxKind::CONST_KW - | SyntaxKind::MOD_KW => true, - // If we didn't find a keyword, we want to cover the record fields in a struct - SyntaxKind::NAME => true, - // Otherwise, the token shouldn't have pub(crate) before it - _ => false, + // We're skipping comments, doc comments, and attribute macros that may precede the keyword + // that the visibility should be placed before. + SyntaxKind::COMMENT | SyntaxKind::ATTR | SyntaxKind::WHITESPACE => false, + _ => true, } });