2 files changed, 160 insertions, 25 deletions

diff --git a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs
index 66f274fa7..8e2178391 100644
--- a/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs
+++ b/crates/ide_assists/src/handlers/extract_struct_from_enum_variant.rs
@@ -151,20 +151,37 @@ fn create_struct_def(
     field_list: &Either<ast::RecordFieldList, ast::TupleFieldList>,
     visibility: Option<ast::Visibility>,
 ) -> ast::Struct {
-    let pub_vis = Some(make::visibility_pub());
+    let pub_vis = make::visibility_pub();
+
+    let insert_pub = |node: &'_ SyntaxNode| {
+        let pub_vis = pub_vis.clone_for_update();
+        ted::insert(ted::Position::before(node), pub_vis.syntax());
+    };
+
+    // for fields without any existing visibility, use pub visibility
     let field_list = match field_list {
         Either::Left(field_list) => {
-            make::record_field_list(field_list.fields().flat_map(|field| {
-                Some(make::record_field(pub_vis.clone(), field.name()?, field.ty()?))
-            }))
-            .into()
+            let field_list = field_list.clone_for_update();
+
+            field_list
+                .fields()
+                .filter(|field| field.visibility().is_none())
+                .filter_map(|field| field.name())
+                .for_each(|it| insert_pub(it.syntax()));
+
+            field_list.into()
         }
-        Either::Right(field_list) => make::tuple_field_list(
+        Either::Right(field_list) => {
+            let field_list = field_list.clone_for_update();
+
             field_list
                 .fields()
-                .flat_map(|field| Some(make::tuple_field(pub_vis.clone(), field.ty()?))),
-        )
-        .into(),
+                .filter(|field| field.visibility().is_none())
+                .filter_map(|field| field.ty())
+                .for_each(|it| insert_pub(it.syntax()));
+
+            field_list.into()
+        }
     };
 
     make::struct_(visibility, variant_name, None, field_list).clone_for_update()
@@ -295,6 +312,106 @@ enum A { One(One) }"#,
     }
 
     #[test]
+    fn test_extract_struct_keep_comments_and_attrs_one_field_named() {
+        check_assist(
+            extract_struct_from_enum_variant,
+            r#"
+enum A {
+    $0One {
+        // leading comment
+        /// doc comment
+        #[an_attr]
+        foo: u32
+        // trailing comment
+    }
+}"#,
+            r#"
+struct One{
+        // leading comment
+        /// doc comment
+        #[an_attr]
+        pub foo: u32
+        // trailing comment
+    }
+
+enum A {
+    One(One)
+}"#,
+        );
+    }
+
+    #[test]
+    fn test_extract_struct_keep_comments_and_attrs_several_fields_named() {
+        check_assist(
+            extract_struct_from_enum_variant,
+            r#"
+enum A {
+    $0One {
+        // comment
+        /// doc
+        #[attr]
+        foo: u32,
+        // comment
+        #[attr]
+        /// doc
+        bar: u32
+    }
+}"#,
+            r#"
+struct One{
+        // comment
+        /// doc
+        #[attr]
+        pub foo: u32,
+        // comment
+        #[attr]
+        /// doc
+        pub bar: u32
+    }
+
+enum A {
+    One(One)
+}"#,
+        );
+    }
+
+    #[test]
+    fn test_extract_struct_keep_comments_and_attrs_several_fields_tuple() {
+        check_assist(
+            extract_struct_from_enum_variant,
+            "enum A { $0One(/* comment */ #[attr] u32, /* another */ u32 /* tail */) }",
+            r#"
+struct One(/* comment */ #[attr] pub u32, /* another */ pub u32 /* tail */);
+
+enum A { One(One) }"#,
+        );
+    }
+
+    #[test]
+    fn test_extract_struct_keep_existing_visibility_named() {
+        check_assist(
+            extract_struct_from_enum_variant,
+            "enum A { $0One{ pub a: u32, pub(crate) b: u32, pub(super) c: u32, d: u32 } }",
+            r#"
+struct One{ pub a: u32, pub(crate) b: u32, pub(super) c: u32, pub d: u32 }
+
+enum A { One(One) }"#,
+        );
+    }
+
+    #[test]
+    fn test_extract_struct_keep_existing_visibility_tuple() {
+        check_assist(
+            extract_struct_from_enum_variant,
+            "enum A { $0One(pub u32, pub(crate) u32, pub(super) u32, u32) }",
+            r#"
+struct One(pub u32, pub(crate) u32, pub(super) u32, pub u32);
+
+enum A { One(One) }"#,
+        );
+    }
+
+    #[test]
     fn test_extract_enum_variant_name_value_namespace() {
         check_assist(
             extract_struct_from_enum_variant,
diff --git a/crates/ide_assists/src/handlers/pull_assignment_up.rs b/crates/ide_assists/src/handlers/pull_assignment_up.rs
index 28d14b9c3..3128faa68 100644
--- a/crates/ide_assists/src/handlers/pull_assignment_up.rs
+++ b/crates/ide_assists/src/handlers/pull_assignment_up.rs
@@ -60,6 +60,12 @@ pub(crate) fn pull_assignment_up(acc: &mut Assists, ctx: &AssistContext) -> Opti
         return None;
     };
 
+    if let Some(parent) = tgt.syntax().parent() {
+        if matches!(parent.kind(), syntax::SyntaxKind::BIN_EXPR | syntax::SyntaxKind::LET_STMT) {
+            return None;
+        }
+    }
+
     acc.add(
         AssistId("pull_assignment_up", AssistKind::RefactorExtract),
         "Pull assignment up",
@@ -74,7 +80,13 @@ pub(crate) fn pull_assignment_up(acc: &mut Assists, ctx: &AssistContext) -> Opti
             let tgt = edit.make_ast_mut(tgt);
 
             for (stmt, rhs) in assignments {
-                ted::replace(stmt.syntax(), rhs.syntax());
+                let mut stmt = stmt.syntax().clone();
+                if let Some(parent) = stmt.parent() {
+                    if ast::ExprStmt::cast(parent.clone()).is_some() {
+                        stmt = parent.clone();
+                    }
+                }
+                ted::replace(stmt, rhs.syntax());
             }
             let assign_expr = make::expr_assignment(collector.common_lhs, tgt.clone());
             let assign_stmt = make::expr_stmt(assign_expr);
@@ -87,7 +99,7 @@ pub(crate) fn pull_assignment_up(acc: &mut Assists, ctx: &AssistContext) -> Opti
 struct AssignmentsCollector<'a> {
     sema: &'a hir::Semantics<'a, ide_db::RootDatabase>,
     common_lhs: ast::Expr,
-    assignments: Vec<(ast::ExprStmt, ast::Expr)>,
+    assignments: Vec<(ast::BinExpr, ast::Expr)>,
 }
 
 impl<'a> AssignmentsCollector<'a> {
@@ -95,6 +107,7 @@ impl<'a> AssignmentsCollector<'a> {
         for arm in match_expr.match_arm_list()?.arms() {
             match arm.expr()? {
                 ast::Expr::BlockExpr(block) => self.collect_block(&block)?,
+                ast::Expr::BinExpr(expr) => self.collect_expr(&expr)?,
                 _ => return None,
             }
         }
@@ -114,24 +127,30 @@ impl<'a> AssignmentsCollector<'a> {
         }
     }
     fn collect_block(&mut self, block: &ast::BlockExpr) -> Option<()> {
-        if block.tail_expr().is_some() {
-            return None;
-        }
-
-        let last_stmt = block.statements().last()?;
-        if let ast::Stmt::ExprStmt(stmt) = last_stmt {
-            if let ast::Expr::BinExpr(expr) = stmt.expr()? {
-                if expr.op_kind()? == ast::BinOp::Assignment
-                    && is_equivalent(self.sema, &expr.lhs()?, &self.common_lhs)
-                {
-                    self.assignments.push((stmt, expr.rhs()?));
-                    return Some(());
-                }
+        let last_expr = block.tail_expr().or_else(|| {
+            if let ast::Stmt::ExprStmt(stmt) = block.statements().last()? {
+                stmt.expr()
+            } else {
+                None
             }
+        })?;
+
+        if let ast::Expr::BinExpr(expr) = last_expr {
+            return self.collect_expr(&expr);
         }
 
         None
     }
+
+    fn collect_expr(&mut self, expr: &ast::BinExpr) -> Option<()> {
+        if expr.op_kind()? == ast::BinOp::Assignment
+            && is_equivalent(self.sema, &expr.lhs()?, &self.common_lhs)
+        {
+            self.assignments.push((expr.clone(), expr.rhs()?));
+            return Some(());
+        }
+        None
+    }
 }
 
 fn is_equivalent(
@@ -241,7 +260,6 @@ fn foo() {
     }
 
     #[test]
-    #[ignore]
     fn test_pull_assignment_up_assignment_expressions() {
         check_assist(
             pull_assignment_up,