Auto merge of #15260 - DropDemBits:structured-snippet-migrate-3, r=lowr

internal: Migrate assists to the structured snippet API, part 3

Continuing from #15231

Migrates the following assists:
- `add_missing_match_arms`
- `fix_visibility`
- `promote_local_to_const`

The `add_missing_match_arms` changes are best reviewed commit-by-commit since they're relatively big changes compared to the rest of the commits.

crates/ide-assists/src/handlers/add_missing_match_arms.rs

@@ -8,10 +8,7 @@
 use syntax::ast::edit_in_place::Removable;
 use syntax::ast::{self, make, AstNode, HasName, MatchArmList, MatchExpr, Pat};
 
-use crate::{
-    utils::{self, render_snippet, Cursor},
-    AssistContext, AssistId, AssistKind, Assists,
-};
+use crate::{utils, AssistContext, AssistId, AssistKind, Assists};
 
 // Assist: add_missing_match_arms
 //
@@ -75,14 +72,18 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
         .collect();
 
     let module = ctx.sema.scope(expr.syntax())?.module();
-    let (mut missing_pats, is_non_exhaustive): (
+    let (mut missing_pats, is_non_exhaustive, has_hidden_variants): (
         Peekable<Box<dyn Iterator<Item = (ast::Pat, bool)>>>,
         bool,
+        bool,
     ) = if let Some(enum_def) = resolve_enum_def(&ctx.sema, &expr) {
         let is_non_exhaustive = enum_def.is_non_exhaustive(ctx.db(), module.krate());
 
         let variants = enum_def.variants(ctx.db());
 
+        let has_hidden_variants =
+            variants.iter().any(|variant| variant.should_be_hidden(ctx.db(), module.krate()));
+
         let missing_pats = variants
             .into_iter()
             .filter_map(|variant| {
@@ -101,7 +102,7 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
         } else {
             Box::new(missing_pats)
         };
-        (missing_pats.peekable(), is_non_exhaustive)
+        (missing_pats.peekable(), is_non_exhaustive, has_hidden_variants)
     } else if let Some(enum_defs) = resolve_tuple_of_enum_def(&ctx.sema, &expr) {
         let is_non_exhaustive =
             enum_defs.iter().any(|enum_def| enum_def.is_non_exhaustive(ctx.db(), module.krate()));
@@ -124,6 +125,12 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
         if n_arms > 256 {
             return None;
         }
+
+        let has_hidden_variants = variants_of_enums
+            .iter()
+            .flatten()
+            .any(|variant| variant.should_be_hidden(ctx.db(), module.krate()));
+
         let missing_pats = variants_of_enums
             .into_iter()
             .multi_cartesian_product()
@@ -139,7 +146,11 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
                 (ast::Pat::from(make::tuple_pat(patterns)), is_hidden)
             })
             .filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
-        ((Box::new(missing_pats) as Box<dyn Iterator<Item = _>>).peekable(), is_non_exhaustive)
+        (
+            (Box::new(missing_pats) as Box<dyn Iterator<Item = _>>).peekable(),
+            is_non_exhaustive,
+            has_hidden_variants,
+        )
     } else if let Some((enum_def, len)) = resolve_array_of_enum_def(&ctx.sema, &expr) {
         let is_non_exhaustive = enum_def.is_non_exhaustive(ctx.db(), module.krate());
         let variants = enum_def.variants(ctx.db());
@@ -148,6 +159,9 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
             return None;
         }
 
+        let has_hidden_variants =
+            variants.iter().any(|variant| variant.should_be_hidden(ctx.db(), module.krate()));
+
         let variants_of_enums = vec![variants; len];
 
         let missing_pats = variants_of_enums
@@ -164,14 +178,20 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
                 (ast::Pat::from(make::slice_pat(patterns)), is_hidden)
             })
             .filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
-        ((Box::new(missing_pats) as Box<dyn Iterator<Item = _>>).peekable(), is_non_exhaustive)
+        (
+            (Box::new(missing_pats) as Box<dyn Iterator<Item = _>>).peekable(),
+            is_non_exhaustive,
+            has_hidden_variants,
+        )
     } else {
         return None;
     };
 
     let mut needs_catch_all_arm = is_non_exhaustive && !has_catch_all_arm;
 
-    if !needs_catch_all_arm && missing_pats.peek().is_none() {
+    if !needs_catch_all_arm
+        && ((has_hidden_variants && has_catch_all_arm) || missing_pats.peek().is_none())
+    {
         return None;
     }
 
@@ -179,13 +199,21 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
         AssistId("add_missing_match_arms", AssistKind::QuickFix),
         "Fill match arms",
         target_range,
-        |builder| {
+        |edit| {
             let new_match_arm_list = match_arm_list.clone_for_update();
+
+            // having any hidden variants means that we need a catch-all arm
+            needs_catch_all_arm |= has_hidden_variants;
+
             let missing_arms = missing_pats
-                .map(|(pat, hidden)| {
-                    (make::match_arm(iter::once(pat), None, make::ext::expr_todo()), hidden)
+                .filter(|(_, hidden)| {
+                    // filter out hidden patterns because they're handled by the catch-all arm
+                    !hidden
                 })
-                .map(|(it, hidden)| (it.clone_for_update(), hidden));
+                .map(|(pat, _)| {
+                    make::match_arm(iter::once(pat), None, make::ext::expr_todo())
+                        .clone_for_update()
+                });
 
             let catch_all_arm = new_match_arm_list
                 .arms()
@@ -204,15 +232,13 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
                     cov_mark::hit!(add_missing_match_arms_empty_expr);
                 }
             }
+
             let mut first_new_arm = None;
-            for (arm, hidden) in missing_arms {
-                if hidden {
-                    needs_catch_all_arm = !has_catch_all_arm;
-                } else {
+            for arm in missing_arms {
                 first_new_arm.get_or_insert_with(|| arm.clone());
                 new_match_arm_list.add_arm(arm);
             }
-            }
+
             if needs_catch_all_arm && !has_catch_all_arm {
                 cov_mark::hit!(added_wildcard_pattern);
                 let arm = make::match_arm(
@@ -225,24 +251,39 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
                 new_match_arm_list.add_arm(arm);
             }
 
-            let old_range = ctx.sema.original_range(match_arm_list.syntax()).range;
-            match (first_new_arm, ctx.config.snippet_cap) {
-                (Some(first_new_arm), Some(cap)) => {
-                    let extend_lifetime;
-                    let cursor =
-                        match first_new_arm.syntax().descendants().find_map(ast::WildcardPat::cast)
-                        {
-                            Some(it) => {
-                                extend_lifetime = it.syntax().clone();
-                                Cursor::Replace(&extend_lifetime)
+            if let (Some(first_new_arm), Some(cap)) = (first_new_arm, ctx.config.snippet_cap) {
+                match first_new_arm.syntax().descendants().find_map(ast::WildcardPat::cast) {
+                    Some(it) => edit.add_placeholder_snippet(cap, it),
+                    None => edit.add_tabstop_before(cap, first_new_arm),
+                }
+            }
+
+            // FIXME: Hack for mutable syntax trees not having great support for macros
+            // Just replace the element that the original range came from
+            let old_place = {
+                // Find the original element
+                let old_file_range = ctx.sema.original_range(match_arm_list.syntax());
+                let file = ctx.sema.parse(old_file_range.file_id);
+                let old_place = file.syntax().covering_element(old_file_range.range);
+
+                // Make `old_place` mut
+                match old_place {
+                    syntax::SyntaxElement::Node(it) => {
+                        syntax::SyntaxElement::from(edit.make_syntax_mut(it))
+                    }
+                    syntax::SyntaxElement::Token(it) => {
+                        // Don't have a way to make tokens mut, so instead make the parent mut
+                        // and find the token again
+                        let parent = edit.make_syntax_mut(it.parent().unwrap());
+                        let mut_token =
+                            parent.covering_element(it.text_range()).into_token().unwrap();
+
+                        syntax::SyntaxElement::from(mut_token)
+                    }
                 }
-                            None => Cursor::Before(first_new_arm.syntax()),
             };
-                    let snippet = render_snippet(cap, new_match_arm_list.syntax(), cursor);
-                    builder.replace_snippet(cap, old_range, snippet);
-                }
-                _ => builder.replace(old_range, new_match_arm_list.to_string()),
-            }
+
+            syntax::ted::replace(old_place, new_match_arm_list.syntax());
         },
     )
 }
@@ -1621,10 +1662,9 @@ pub enum E { #[doc(hidden)] A, }
         );
     }
 
-    // FIXME: I don't think the assist should be applicable in this case
     #[test]
     fn does_not_fill_wildcard_with_wildcard() {
-        check_assist(
+        check_assist_not_applicable(
             add_missing_match_arms,
             r#"
 //- /main.rs crate:main deps:e
@@ -1635,13 +1675,6 @@ fn foo(t: ::e::E) {
 }
 //- /e.rs crate:e
 pub enum E { #[doc(hidden)] A, }
-"#,
-            r#"
-fn foo(t: ::e::E) {
-    match t {
-        _ => todo!(),
-    }
-}
 "#,
         );
     }
@@ -1777,7 +1810,7 @@ fn foo(t: ::e::E, b: bool) {
 
     #[test]
     fn does_not_fill_wildcard_with_partial_wildcard_and_wildcard() {
-        check_assist(
+        check_assist_not_applicable(
             add_missing_match_arms,
             r#"
 //- /main.rs crate:main deps:e
@@ -1789,14 +1822,6 @@ fn foo(t: ::e::E, b: bool) {
 }
 //- /e.rs crate:e
 pub enum E { #[doc(hidden)] A, }"#,
-            r#"
-fn foo(t: ::e::E, b: bool) {
-    match t {
-        _ if b => todo!(),
-        _ => todo!(),
-    }
-}
-"#,
         );
     }
 

crates/ide-assists/src/handlers/fix_visibility.rs

@@ -1,11 +1,11 @@
 use hir::{db::HirDatabase, HasSource, HasVisibility, ModuleDef, PathResolution, ScopeDef};
 use ide_db::base_db::FileId;
 use syntax::{
-    ast::{self, HasVisibility as _},
-    AstNode, TextRange, TextSize,
+    ast::{self, edit_in_place::HasVisibilityEdit, make, HasVisibility as _},
+    AstNode, TextRange,
 };
 
-use crate::{utils::vis_offset, AssistContext, AssistId, AssistKind, Assists};
+use crate::{AssistContext, AssistId, AssistKind, Assists};
 
 // FIXME: this really should be a fix for diagnostic, rather than an assist.
 
@@ -58,11 +58,13 @@ fn add_vis_to_referenced_module_def(acc: &mut Assists, ctx: &AssistContext<'_>)
         return None;
     };
 
-    let (offset, current_visibility, target, target_file, target_name) =
-        target_data_for_def(ctx.db(), def)?;
+    let (vis_owner, 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" };
+    let missing_visibility = if current_module.krate() == target_module.krate() {
+        make::visibility_pub_crate()
+    } else {
+        make::visibility_pub()
+    };
 
     let assist_label = match target_name {
         None => format!("Change visibility to {missing_visibility}"),
@@ -71,23 +73,14 @@ fn add_vis_to_referenced_module_def(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) => 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} ")),
-            },
+    acc.add(AssistId("fix_visibility", AssistKind::QuickFix), assist_label, target, |edit| {
+        edit.edit_file(target_file);
+
+        let vis_owner = edit.make_mut(vis_owner);
+        vis_owner.set_visibility(missing_visibility.clone_for_update());
+
+        if let Some((cap, vis)) = ctx.config.snippet_cap.zip(vis_owner.visibility()) {
+            edit.add_tabstop_before(cap, vis);
         }
     })
 }
@@ -107,19 +100,22 @@ 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, current_visibility, target) = match in_file_source.value {
+    let (vis_owner, target) = match in_file_source.value {
         hir::FieldSource::Named(it) => {
-            let s = it.syntax();
-            (vis_offset(s), it.visibility(), s.text_range())
+            let range = it.syntax().text_range();
+            (ast::AnyHasVisibility::new(it), range)
         }
         hir::FieldSource::Pos(it) => {
-            let s = it.syntax();
-            (vis_offset(s), it.visibility(), s.text_range())
+            let range = it.syntax().text_range();
+            (ast::AnyHasVisibility::new(it), range)
         }
     };
 
-    let missing_visibility =
-        if current_module.krate() == target_module.krate() { "pub(crate)" } else { "pub" };
+    let missing_visibility = if current_module.krate() == target_module.krate() {
+        make::visibility_pub_crate()
+    } else {
+        make::visibility_pub()
+    };
     let target_file = in_file_source.file_id.original_file(ctx.db());
 
     let target_name = record_field_def.name(ctx.db());
@@ -129,23 +125,14 @@ fn add_vis_to_referenced_record_field(acc: &mut Assists, ctx: &AssistContext<'_>
         target_name.display(ctx.db())
     );
 
-    acc.add(AssistId("fix_visibility", AssistKind::QuickFix), assist_label, target, |builder| {
-        builder.edit_file(target_file);
-        match ctx.config.snippet_cap {
-            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} ")),
-            },
+    acc.add(AssistId("fix_visibility", AssistKind::QuickFix), assist_label, target, |edit| {
+        edit.edit_file(target_file);
+
+        let vis_owner = edit.make_mut(vis_owner);
+        vis_owner.set_visibility(missing_visibility.clone_for_update());
+
+        if let Some((cap, vis)) = ctx.config.snippet_cap.zip(vis_owner.visibility()) {
+            edit.add_tabstop_before(cap, vis);
         }
     })
 }
@@ -153,11 +140,11 @@ 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, Option<ast::Visibility>, TextRange, FileId, Option<hir::Name>)> {
+) -> Option<(ast::AnyHasVisibility, TextRange, FileId, Option<hir::Name>)> {
     fn offset_target_and_file_id<S, Ast>(
         db: &dyn HirDatabase,
         x: S,
-    ) -> Option<(TextSize, Option<ast::Visibility>, TextRange, FileId)>
+    ) -> Option<(ast::AnyHasVisibility, TextRange, FileId)>
     where
         S: HasSource<Ast = Ast>,
         Ast: AstNode + ast::HasVisibility,
@@ -165,18 +152,12 @@ fn offset_target_and_file_id<S, Ast>(
         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;
-        let current_visibility = source.value.visibility();
-        Some((
-            vis_offset(syntax),
-            current_visibility,
-            syntax.text_range(),
-            file_id.original_file(db.upcast()),
-        ))
+        let range = in_file_syntax.value.text_range();
+        Some((ast::AnyHasVisibility::new(source.value), range, file_id.original_file(db.upcast())))
     }
 
     let target_name;
-    let (offset, current_visibility, target, target_file) = match def {
+    let (offset, target, target_file) = match def {
         hir::ModuleDef::Function(f) => {
             target_name = Some(f.name(db));
             offset_target_and_file_id(db, f)?
@@ -213,8 +194,8 @@ fn offset_target_and_file_id<S, Ast>(
             target_name = m.name(db);
             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), in_file_source.value.visibility(), syntax.text_range(), file_id)
+            let range = in_file_source.value.syntax().text_range();
+            (ast::AnyHasVisibility::new(in_file_source.value), range, file_id)
         }
         // FIXME
         hir::ModuleDef::Macro(_) => return None,
@@ -222,7 +203,7 @@ fn offset_target_and_file_id<S, Ast>(
         hir::ModuleDef::Variant(_) | hir::ModuleDef::BuiltinType(_) => return None,
     };
 
-    Some((offset, current_visibility, target, target_file, target_name))
+    Some((offset, target, target_file, target_name))
 }
 
 #[cfg(test)]

crates/ide-assists/src/handlers/promote_local_to_const.rs

@@ -8,13 +8,10 @@
 use stdx::to_upper_snake_case;
 use syntax::{
     ast::{self, make, HasName},
-    AstNode, WalkEvent,
+    ted, AstNode, WalkEvent,
 };
 
-use crate::{
-    assist_context::{AssistContext, Assists},
-    utils::{render_snippet, Cursor},
-};
+use crate::assist_context::{AssistContext, Assists};
 
 // Assist: promote_local_to_const
 //
@@ -70,29 +67,33 @@ pub(crate) fn promote_local_to_const(acc: &mut Assists, ctx: &AssistContext<'_>)
         cov_mark::hit!(promote_local_non_const);
         return None;
     }
-    let target = let_stmt.syntax().text_range();
+
     acc.add(
         AssistId("promote_local_to_const", AssistKind::Refactor),
         "Promote local to constant",
-        target,
-        |builder| {
+        let_stmt.syntax().text_range(),
+        |edit| {
             let name = to_upper_snake_case(&name.to_string());
             let usages = Definition::Local(local).usages(&ctx.sema).all();
             if let Some(usages) = usages.references.get(&ctx.file_id()) {
+                let name = make::name_ref(&name);
+
                 for usage in usages {
-                    builder.replace(usage.range, &name);
+                    let Some(usage) = usage.name.as_name_ref().cloned() else { continue };
+                    let usage = edit.make_mut(usage);
+                    ted::replace(usage.syntax(), name.clone_for_update().syntax());
                 }
             }
 
-            let item = make::item_const(None, make::name(&name), make::ty(&ty), initializer);
-            match ctx.config.snippet_cap.zip(item.name()) {
-                Some((cap, name)) => builder.replace_snippet(
-                    cap,
-                    target,
-                    render_snippet(cap, item.syntax(), Cursor::Before(name.syntax())),
-                ),
-                None => builder.replace(target, item.to_string()),
+            let item = make::item_const(None, make::name(&name), make::ty(&ty), initializer)
+                .clone_for_update();
+            let let_stmt = edit.make_mut(let_stmt);
+
+            if let Some((cap, name)) = ctx.config.snippet_cap.zip(item.name()) {
+                edit.add_tabstop_before(cap, name);
             }
+
+            ted::replace(let_stmt.syntax(), item.syntax());
         },
     )
 }
@@ -157,6 +158,27 @@ fn foo() {
         );
     }
 
+    #[test]
+    fn multiple_uses() {
+        check_assist(
+            promote_local_to_const,
+            r"
+fn foo() {
+    let x$0 = 0;
+    let y = x;
+    let z = (x, x, x, x);
+}
+",
+            r"
+fn foo() {
+    const $0X: i32 = 0;
+    let y = X;
+    let z = (X, X, X, X);
+}
+",
+        );
+    }
+
     #[test]
     fn not_applicable_non_const_meth_call() {
         cov_mark::check!(promote_local_non_const);

crates/syntax/src/ast/edit_in_place.rs

@@ -748,6 +748,27 @@ fn normalize_ws_between_braces(node: &SyntaxNode) -> Option<()> {
     Some(())
 }
 
+pub trait HasVisibilityEdit: ast::HasVisibility {
+    fn set_visibility(&self, visbility: ast::Visibility) {
+        match self.visibility() {
+            Some(current_visibility) => {
+                ted::replace(current_visibility.syntax(), visbility.syntax())
+            }
+            None => {
+                let vis_before = self
+                    .syntax()
+                    .children_with_tokens()
+                    .find(|it| !matches!(it.kind(), WHITESPACE | COMMENT | ATTR))
+                    .unwrap_or_else(|| self.syntax().first_child_or_token().unwrap());
+
+                ted::insert(ted::Position::before(vis_before), visbility.syntax());
+            }
+        }
+    }
+}
+
+impl<T: ast::HasVisibility> HasVisibilityEdit for T {}
+
 pub trait Indent: AstNode + Clone + Sized {
     fn indent_level(&self) -> IndentLevel {
         IndentLevel::from_node(self.syntax())