2 files changed, 130 insertions, 63 deletions

diff --git a/crates/ra_assists/src/handlers/extract_variable.rs b/crates/ra_assists/src/handlers/extract_variable.rs
index 481baf1a4..098adf078 100644
--- a/crates/ra_assists/src/handlers/extract_variable.rs
+++ b/crates/ra_assists/src/handlers/extract_variable.rs
@@ -2,7 +2,6 @@ use ra_syntax::{
     ast::{self, AstNode},
     SyntaxKind::{
         BLOCK_EXPR, BREAK_EXPR, COMMENT, LAMBDA_EXPR, LOOP_EXPR, MATCH_ARM, PATH_EXPR, RETURN_EXPR,
-        WHITESPACE,
     },
     SyntaxNode,
 };
@@ -36,22 +35,20 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext) -> Option
         mark::hit!(extract_var_in_comment_is_not_applicable);
         return None;
     }
-    let expr = node.ancestors().find_map(valid_target_expr)?;
-    let (anchor_stmt, wrap_in_block) = anchor_stmt(expr.clone())?;
-    let indent = anchor_stmt.prev_sibling_or_token()?.as_token()?.clone();
-    if indent.kind() != WHITESPACE {
-        return None;
-    }
-    let target = expr.syntax().text_range();
+    let to_extract = node.ancestors().find_map(valid_target_expr)?;
+    let anchor = Anchor::from(&to_extract)?;
+    let indent = anchor.syntax().prev_sibling_or_token()?.as_token()?.clone();
+    let target = to_extract.syntax().text_range();
     acc.add(
         AssistId("extract_variable", AssistKind::RefactorExtract),
         "Extract into variable",
         target,
         move |edit| {
-            let field_shorthand = match expr.syntax().parent().and_then(ast::RecordField::cast) {
-                Some(field) => field.name_ref(),
-                None => None,
-            };
+            let field_shorthand =
+                match to_extract.syntax().parent().and_then(ast::RecordField::cast) {
+                    Some(field) => field.name_ref(),
+                    None => None,
+                };
 
             let mut buf = String::new();
 
@@ -60,26 +57,20 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext) -> Option
                 None => "var_name".to_string(),
             };
             let expr_range = match &field_shorthand {
-                Some(it) => it.syntax().text_range().cover(expr.syntax().text_range()),
-                None => expr.syntax().text_range(),
+                Some(it) => it.syntax().text_range().cover(to_extract.syntax().text_range()),
+                None => to_extract.syntax().text_range(),
             };
 
-            if wrap_in_block {
+            if let Anchor::WrapInBlock(_) = anchor {
                 format_to!(buf, "{{ let {} = ", var_name);
             } else {
                 format_to!(buf, "let {} = ", var_name);
             };
-            format_to!(buf, "{}", expr.syntax());
+            format_to!(buf, "{}", to_extract.syntax());
 
-            let full_stmt = ast::ExprStmt::cast(anchor_stmt.clone());
-            let is_full_stmt = if let Some(expr_stmt) = &full_stmt {
-                Some(expr.syntax().clone()) == expr_stmt.expr().map(|e| e.syntax().clone())
-            } else {
-                false
-            };
-            if is_full_stmt {
+            if let Anchor::Replace(stmt) = anchor {
                 mark::hit!(test_extract_var_expr_stmt);
-                if full_stmt.unwrap().semicolon_token().is_none() {
+                if stmt.semicolon_token().is_none() {
                     buf.push_str(";");
                 }
                 match ctx.config.snippet_cap {
@@ -107,7 +98,7 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext) -> Option
             }
 
             edit.replace(expr_range, var_name.clone());
-            let offset = anchor_stmt.text_range().start();
+            let offset = anchor.syntax().text_range().start();
             match ctx.config.snippet_cap {
                 Some(cap) => {
                     let snip =
@@ -117,8 +108,8 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext) -> Option
                 None => edit.insert(offset, buf),
             }
 
-            if wrap_in_block {
-                edit.insert(anchor_stmt.text_range().end(), " }");
+            if let Anchor::WrapInBlock(_) = anchor {
+                edit.insert(anchor.syntax().text_range().end(), " }");
             }
         },
     )
@@ -138,32 +129,48 @@ fn valid_target_expr(node: SyntaxNode) -> Option<ast::Expr> {
     }
 }
 
-/// Returns the syntax node which will follow the freshly extractd var
-/// and a boolean indicating whether we have to wrap it within a { } block
-/// to produce correct code.
-/// It can be a statement, the last in a block expression or a wanna be block
-/// expression like a lambda or match arm.
-fn anchor_stmt(expr: ast::Expr) -> Option<(SyntaxNode, bool)> {
-    expr.syntax().ancestors().find_map(|node| {
-        if let Some(expr) = node.parent().and_then(ast::BlockExpr::cast).and_then(|it| it.expr()) {
-            if expr.syntax() == &node {
-                mark::hit!(test_extract_var_last_expr);
-                return Some((node, false));
+enum Anchor {
+    Before(SyntaxNode),
+    Replace(ast::ExprStmt),
+    WrapInBlock(SyntaxNode),
+}
+
+impl Anchor {
+    fn from(to_extract: &ast::Expr) -> Option<Anchor> {
+        to_extract.syntax().ancestors().find_map(|node| {
+            if let Some(expr) =
+                node.parent().and_then(ast::BlockExpr::cast).and_then(|it| it.expr())
+            {
+                if expr.syntax() == &node {
+                    mark::hit!(test_extract_var_last_expr);
+                    return Some(Anchor::Before(node));
+                }
             }
-        }
 
-        if let Some(parent) = node.parent() {
-            if parent.kind() == MATCH_ARM || parent.kind() == LAMBDA_EXPR {
-                return Some((node, true));
+            if let Some(parent) = node.parent() {
+                if parent.kind() == MATCH_ARM || parent.kind() == LAMBDA_EXPR {
+                    return Some(Anchor::WrapInBlock(node));
+                }
             }
-        }
 
-        if ast::Stmt::cast(node.clone()).is_some() {
-            return Some((node, false));
-        }
+            if let Some(stmt) = ast::Stmt::cast(node.clone()) {
+                if let ast::Stmt::ExprStmt(stmt) = stmt {
+                    if stmt.expr().as_ref() == Some(to_extract) {
+                        return Some(Anchor::Replace(stmt));
+                    }
+                }
+                return Some(Anchor::Before(node));
+            }
+            None
+        })
+    }
 
-        None
-    })
+    fn syntax(&self) -> &SyntaxNode {
+        match self {
+            Anchor::Before(it) | Anchor::WrapInBlock(it) => it,
+            Anchor::Replace(stmt) => stmt.syntax(),
+        }
+    }
 }
 
 #[cfg(test)]
diff --git a/crates/ra_assists/src/handlers/fix_visibility.rs b/crates/ra_assists/src/handlers/fix_visibility.rs
index e212557c8..1d3ed3c6a 100644
--- a/crates/ra_assists/src/handlers/fix_visibility.rs
+++ b/crates/ra_assists/src/handlers/fix_visibility.rs
@@ -3,6 +3,7 @@ use ra_db::FileId;
 use ra_syntax::{ast, AstNode, TextRange, TextSize};
 
 use crate::{utils::vis_offset, AssistContext, AssistId, AssistKind, Assists};
+use ast::VisibilityOwner;
 
 // FIXME: this really should be a fix for diagnostic, rather than an assist.
 
@@ -48,7 +49,8 @@ fn add_vis_to_referenced_module_def(acc: &mut Assists, ctx: &AssistContext) -> O
         return None;
     };
 
-    let (offset, target, target_file, target_name) = target_data_for_def(ctx.db(), def)?;
+    let (offset, current_visibility, target, target_file, target_name) =
+        target_data_for_def(ctx.db(), def)?;
 
     let missing_visibility =
         if current_module.krate() == target_module.krate() { "pub(crate)" } else { "pub" };
@@ -61,8 +63,20 @@ fn add_vis_to_referenced_module_def(acc: &mut Assists, ctx: &AssistContext) -> O
     acc.add(AssistId("fix_visibility", AssistKind::QuickFix), assist_label, target, |builder| {
         builder.edit_file(target_file);
         match ctx.config.snippet_cap {
-            Some(cap) => builder.insert_snippet(cap, offset, format!("$0{} ", missing_visibility)),
-            None => builder.insert(offset, format!("{} ", missing_visibility)),
+            Some(cap) => match current_visibility {
+                Some(current_visibility) => builder.replace_snippet(
+                    cap,
+                    current_visibility.syntax().text_range(),
+                    format!("$0{}", missing_visibility),
+                ),
+                None => builder.insert_snippet(cap, offset, format!("$0{} ", missing_visibility)),
+            },
+            None => match current_visibility {
+                Some(current_visibility) => {
+                    builder.replace(current_visibility.syntax().text_range(), missing_visibility)
+                }
+                None => builder.insert(offset, format!("{} ", missing_visibility)),
+            },
         }
     })
 }
@@ -82,14 +96,14 @@ fn add_vis_to_referenced_record_field(acc: &mut Assists, ctx: &AssistContext) ->
     let target_module = parent.module(ctx.db());
 
     let in_file_source = record_field_def.source(ctx.db());
-    let (offset, target) = match in_file_source.value {
+    let (offset, current_visibility, target) = match in_file_source.value {
         hir::FieldSource::Named(it) => {
             let s = it.syntax();
-            (vis_offset(s), s.text_range())
+            (vis_offset(s), it.visibility(), s.text_range())
         }
         hir::FieldSource::Pos(it) => {
             let s = it.syntax();
-            (vis_offset(s), s.text_range())
+            (vis_offset(s), it.visibility(), s.text_range())
         }
     };
 
@@ -104,8 +118,20 @@ fn add_vis_to_referenced_record_field(acc: &mut Assists, ctx: &AssistContext) ->
     acc.add(AssistId("fix_visibility", AssistKind::QuickFix), assist_label, target, |builder| {
         builder.edit_file(target_file);
         match ctx.config.snippet_cap {
-            Some(cap) => builder.insert_snippet(cap, offset, format!("$0{} ", missing_visibility)),
-            None => builder.insert(offset, format!("{} ", missing_visibility)),
+            Some(cap) => match current_visibility {
+                Some(current_visibility) => builder.replace_snippet(
+                    cap,
+                    dbg!(current_visibility.syntax()).text_range(),
+                    format!("$0{}", missing_visibility),
+                ),
+                None => builder.insert_snippet(cap, offset, format!("$0{} ", missing_visibility)),
+            },
+            None => match current_visibility {
+                Some(current_visibility) => {
+                    builder.replace(current_visibility.syntax().text_range(), missing_visibility)
+                }
+                None => builder.insert(offset, format!("{} ", missing_visibility)),
+            },
         }
     })
 }
@@ -113,24 +139,30 @@ fn add_vis_to_referenced_record_field(acc: &mut Assists, ctx: &AssistContext) ->
 fn target_data_for_def(
     db: &dyn HirDatabase,
     def: hir::ModuleDef,
-) -> Option<(TextSize, TextRange, FileId, Option<hir::Name>)> {
+) -> Option<(TextSize, Option<ast::Visibility>, TextRange, FileId, Option<hir::Name>)> {
     fn offset_target_and_file_id<S, Ast>(
         db: &dyn HirDatabase,
         x: S,
-    ) -> (TextSize, TextRange, FileId)
+    ) -> (TextSize, Option<ast::Visibility>, TextRange, FileId)
     where
         S: HasSource<Ast = Ast>,
-        Ast: AstNode,
+        Ast: AstNode + ast::VisibilityOwner,
     {
         let source = x.source(db);
         let in_file_syntax = source.syntax();
         let file_id = in_file_syntax.file_id;
         let syntax = in_file_syntax.value;
-        (vis_offset(syntax), syntax.text_range(), file_id.original_file(db.upcast()))
+        let current_visibility = source.value.visibility();
+        (
+            vis_offset(syntax),
+            current_visibility,
+            syntax.text_range(),
+            file_id.original_file(db.upcast()),
+        )
     }
 
     let target_name;
-    let (offset, target, target_file) = match def {
+    let (offset, current_visibility, target, target_file) = match def {
         hir::ModuleDef::Function(f) => {
             target_name = Some(f.name(db));
             offset_target_and_file_id(db, f)
@@ -164,13 +196,13 @@ fn target_data_for_def(
             let in_file_source = m.declaration_source(db)?;
             let file_id = in_file_source.file_id.original_file(db.upcast());
             let syntax = in_file_source.value.syntax();
-            (vis_offset(syntax), syntax.text_range(), file_id)
+            (vis_offset(syntax), in_file_source.value.visibility(), syntax.text_range(), file_id)
         }
         // Enum variants can't be private, we can't modify builtin types
         hir::ModuleDef::EnumVariant(_) | hir::ModuleDef::BuiltinType(_) => return None,
     };
 
-    Some((offset, target, target_file, target_name))
+    Some((offset, current_visibility, target, target_file, target_name))
 }
 
 #[cfg(test)]
@@ -523,6 +555,34 @@ struct Bar;
     }
 
     #[test]
+    fn replaces_pub_crate_with_pub() {
+        check_assist(
+            fix_visibility,
+            r"
+//- /main.rs crate:a deps:foo
+foo::Bar<|>
+//- /lib.rs crate:foo
+pub(crate) struct Bar;
+",
+            r"$0pub struct Bar;
+",
+        );
+        check_assist(
+            fix_visibility,
+            r"
+//- /main.rs crate:a deps:foo
+fn main() {
+    foo::Foo { <|>bar: () };
+}
+//- /lib.rs crate:foo
+pub struct Foo { pub(crate) bar: () }
+",
+            r"pub struct Foo { $0pub bar: () }
+",
+        );
+    }
+
+    #[test]
     #[ignore]
     // FIXME handle reexports properly
     fn fix_visibility_of_reexport() {