Merge #8781

8781: internal: rewrite **Repalce impl Trait** assist to mutable syntax trees r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/ide/src/join_lines.rs

@@ -1,3 +1,5 @@
+use std::convert::TryFrom;
+
 use ide_assists::utils::extract_trivial_expression;
 use itertools::Itertools;
 use syntax::{
@@ -65,6 +67,14 @@ fn remove_newlines(edit: &mut TextEditBuilder, token: &SyntaxToken, range: TextR
 
 fn remove_newline(edit: &mut TextEditBuilder, token: &SyntaxToken, offset: TextSize) {
     if token.kind() != WHITESPACE || token.text().bytes().filter(|&b| b == b'\n').count() != 1 {
+        let n_spaces_after_line_break = {
+            let suff = &token.text()[TextRange::new(
+                offset - token.text_range().start() + TextSize::of('\n'),
+                TextSize::of(token.text()),
+            )];
+            suff.bytes().take_while(|&b| b == b' ').count()
+        };
+
         let mut no_space = false;
         if let Some(string) = ast::String::cast(token.clone()) {
             if let Some(range) = string.open_quote_text_range() {
@@ -73,18 +83,13 @@ fn remove_newline(edit: &mut TextEditBuilder, token: &SyntaxToken, offset: TextS
             }
             if let Some(range) = string.close_quote_text_range() {
                 cov_mark::hit!(join_string_literal_close_quote);
-                no_space |= range.start() == offset + TextSize::of('\n');
+                no_space |= range.start()
+                    == offset
+                        + TextSize::of('\n')
+                        + TextSize::try_from(n_spaces_after_line_break).unwrap();
             }
         }
 
-        let n_spaces_after_line_break = {
-            let suff = &token.text()[TextRange::new(
-                offset - token.text_range().start() + TextSize::of('\n'),
-                TextSize::of(token.text()),
-            )];
-            suff.bytes().take_while(|&b| b == b' ').count()
-        };
-
         let range = TextRange::at(offset, ((n_spaces_after_line_break + 1) as u32).into());
         let replace_with = if no_space { "" } else { " " };
         edit.replace(range, replace_with.to_string());
@@ -833,6 +838,19 @@ fn main() {
 fn main() {
     $0"hello";
 }
+"#,
+            );
+            check_join_lines(
+                r#"
+fn main() {
+    $0r"hello
+    ";
+}
+"#,
+                r#"
+fn main() {
+    $0r"hello";
+}
 "#,
             );
         }

crates/ide_assists/src/handlers/extract_variable.rs

@@ -54,7 +54,7 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext) -> Option
 
             let var_name = match &field_shorthand {
                 Some(it) => it.to_string(),
-                None => suggest_name::variable(&to_extract, &ctx.sema),
+                None => suggest_name::for_variable(&to_extract, &ctx.sema),
             };
             let expr_range = match &field_shorthand {
                 Some(it) => it.syntax().text_range().cover(to_extract.syntax().text_range()),

crates/ide_assists/src/handlers/introduce_named_lifetime.rs

@@ -139,12 +139,12 @@ fn generate_unique_lifetime_param_name(
 
 fn add_lifetime_param(type_params: ast::GenericParamList, new_lifetime_param: char) {
     let generic_param =
-        make::generic_param(format!("'{}", new_lifetime_param), None).clone_for_update();
+        make::generic_param(&format!("'{}", new_lifetime_param), None).clone_for_update();
     type_params.add_generic_param(generic_param);
 }
 
 fn make_ast_lifetime(new_lifetime_param: char) -> ast::Lifetime {
-    make::generic_param(format!("'{}", new_lifetime_param), None)
+    make::generic_param(&format!("'{}", new_lifetime_param), None)
         .syntax()
         .descendants()
         .find_map(ast::Lifetime::cast)

crates/ide_assists/src/handlers/replace_impl_trait_with_generic.rs

@@ -1,6 +1,9 @@
-use syntax::ast::{self, edit::AstNodeEdit, make, AstNode, GenericParamsOwner};
+use syntax::{
+    ast::{self, edit_in_place::GenericParamsOwnerEdit, make, AstNode},
+    ted,
+};
 
-use crate::{AssistContext, AssistId, AssistKind, Assists};
+use crate::{utils::suggest_name, AssistContext, AssistId, AssistKind, Assists};
 
 // Assist: replace_impl_trait_with_generic
 //
@@ -17,30 +20,29 @@ pub(crate) fn replace_impl_trait_with_generic(
     acc: &mut Assists,
     ctx: &AssistContext,
 ) -> Option<()> {
-    let type_impl_trait = ctx.find_node_at_offset::<ast::ImplTraitType>()?;
+    let impl_trait_type = ctx.find_node_at_offset::<ast::ImplTraitType>()?;
-    let type_param = type_impl_trait.syntax().parent().and_then(ast::Param::cast)?;
+    let param = impl_trait_type.syntax().parent().and_then(ast::Param::cast)?;
-    let type_fn = type_param.syntax().ancestors().find_map(ast::Fn::cast)?;
+    let fn_ = param.syntax().ancestors().find_map(ast::Fn::cast)?;
 
-    let impl_trait_ty = type_impl_trait.type_bound_list()?;
+    let type_bound_list = impl_trait_type.type_bound_list()?;
 
-    let target = type_fn.syntax().text_range();
+    let target = fn_.syntax().text_range();
     acc.add(
         AssistId("replace_impl_trait_with_generic", AssistKind::RefactorRewrite),
         "Replace impl trait with generic",
         target,
         |edit| {
-            let generic_letter = impl_trait_ty.to_string().chars().next().unwrap().to_string();
+            let impl_trait_type = edit.make_ast_mut(impl_trait_type);
+            let fn_ = edit.make_ast_mut(fn_);
 
-            let generic_param_list = type_fn
+            let type_param_name = suggest_name::for_generic_parameter(&impl_trait_type);
-                .generic_param_list()
-                .unwrap_or_else(|| make::generic_param_list(None))
-                .append_param(make::generic_param(generic_letter.clone(), Some(impl_trait_ty)));
 
-            let new_type_fn = type_fn
+            let type_param =
-                .replace_descendant::<ast::Type>(type_impl_trait.into(), make::ty(&generic_letter))
+                make::generic_param(&type_param_name, Some(type_bound_list)).clone_for_update();
-                .with_generic_param_list(generic_param_list);
+            let new_ty = make::ty(&type_param_name).clone_for_update();
 
-            edit.replace_ast(type_fn.clone(), new_type_fn);
+            ted::replace(impl_trait_type.syntax(), new_ty.syntax());
+            fn_.get_or_create_generic_param_list().add_generic_param(type_param)
         },
     )
 }
@@ -55,12 +57,8 @@ mod tests {
     fn replace_impl_trait_with_generic_params() {
         check_assist(
             replace_impl_trait_with_generic,
-            r#"
+            r#"fn foo<G>(bar: $0impl Bar) {}"#,
-            fn foo<G>(bar: $0impl Bar) {}
+            r#"fn foo<G, B: Bar>(bar: B) {}"#,
-            "#,
-            r#"
-            fn foo<G, B: Bar>(bar: B) {}
-            "#,
         );
     }
 
@@ -68,12 +66,8 @@ mod tests {
     fn replace_impl_trait_without_generic_params() {
         check_assist(
             replace_impl_trait_with_generic,
-            r#"
+            r#"fn foo(bar: $0impl Bar) {}"#,
-            fn foo(bar: $0impl Bar) {}
+            r#"fn foo<B: Bar>(bar: B) {}"#,
-            "#,
-            r#"
-            fn foo<B: Bar>(bar: B) {}
-            "#,
         );
     }
 
@@ -81,12 +75,8 @@ mod tests {
     fn replace_two_impl_trait_with_generic_params() {
         check_assist(
             replace_impl_trait_with_generic,
-            r#"
+            r#"fn foo<G>(foo: impl Foo, bar: $0impl Bar) {}"#,
-            fn foo<G>(foo: impl Foo, bar: $0impl Bar) {}
+            r#"fn foo<G, B: Bar>(foo: impl Foo, bar: B) {}"#,
-            "#,
-            r#"
-            fn foo<G, B: Bar>(foo: impl Foo, bar: B) {}
-            "#,
         );
     }
 
@@ -94,12 +84,8 @@ mod tests {
     fn replace_impl_trait_with_empty_generic_params() {
         check_assist(
             replace_impl_trait_with_generic,
-            r#"
+            r#"fn foo<>(bar: $0impl Bar) {}"#,
-            fn foo<>(bar: $0impl Bar) {}
+            r#"fn foo<B: Bar>(bar: B) {}"#,
-            "#,
-            r#"
-            fn foo<B: Bar>(bar: B) {}
-            "#,
         );
     }
 
@@ -108,13 +94,13 @@ mod tests {
         check_assist(
             replace_impl_trait_with_generic,
             r#"
-            fn foo<
+fn foo<
-            >(bar: $0impl Bar) {}
+>(bar: $0impl Bar) {}
-            "#,
+"#,
             r#"
-            fn foo<B: Bar
+fn foo<B: Bar
-            >(bar: B) {}
+>(bar: B) {}
-            "#,
+"#,
         );
     }
 
@@ -123,12 +109,8 @@ mod tests {
     fn replace_impl_trait_with_exist_generic_letter() {
         check_assist(
             replace_impl_trait_with_generic,
-            r#"
+            r#"fn foo<B>(bar: $0impl Bar) {}"#,
-            fn foo<B>(bar: $0impl Bar) {}
+            r#"fn foo<B, C: Bar>(bar: C) {}"#,
-            "#,
-            r#"
-            fn foo<B, C: Bar>(bar: C) {}
-            "#,
         );
     }
 
@@ -137,19 +119,19 @@ mod tests {
         check_assist(
             replace_impl_trait_with_generic,
             r#"
-            fn foo<
+fn foo<
-                G: Foo,
+    G: Foo,
-                F,
+    F,
-                H,
+    H,
-            >(bar: $0impl Bar) {}
+>(bar: $0impl Bar) {}
-            "#,
+"#,
             r#"
-            fn foo<
+fn foo<
-                G: Foo,
+    G: Foo,
-                F,
+    F,
-                H, B: Bar
+    H, B: Bar,
-            >(bar: B) {}
+>(bar: B) {}
-            "#,
+"#,
         );
     }
 
@@ -157,12 +139,8 @@ mod tests {
     fn replace_impl_trait_multiple() {
         check_assist(
             replace_impl_trait_with_generic,
-            r#"
+            r#"fn foo(bar: $0impl Foo + Bar) {}"#,
-            fn foo(bar: $0impl Foo + Bar) {}
+            r#"fn foo<F: Foo + Bar>(bar: F) {}"#,
-            "#,
-            r#"
-            fn foo<F: Foo + Bar>(bar: F) {}
-            "#,
         );
     }
 }

crates/ide_assists/src/utils/suggest_name.rs

@@ -6,7 +6,7 @@ use itertools::Itertools;
 use stdx::to_lower_snake_case;
 use syntax::{
     ast::{self, NameOwner},
-    match_ast, AstNode,
+    match_ast, AstNode, SmolStr,
 };
 
 /// Trait names, that will be ignored when in `impl Trait` and `dyn Trait`
@@ -57,6 +57,14 @@ const USELESS_METHODS: &[&str] = &[
     "iter_mut",
 ];
 
+pub(crate) fn for_generic_parameter(ty: &ast::ImplTraitType) -> SmolStr {
+    let c = ty
+        .type_bound_list()
+        .and_then(|bounds| bounds.syntax().text().char_at(0.into()))
+        .unwrap_or('T');
+    c.encode_utf8(&mut [0; 4]).into()
+}
+
 /// Suggest name of variable for given expression
 ///
 /// **NOTE**: it is caller's responsibility to guarantee uniqueness of the name.
@@ -75,7 +83,8 @@ const USELESS_METHODS: &[&str] = &[
 /// It also applies heuristics to filter out less informative names
 ///
 /// Currently it sticks to the first name found.
-pub(crate) fn variable(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> String {
+// FIXME: Microoptimize and return a `SmolStr` here.
+pub(crate) fn for_variable(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> String {
     // `from_param` does not benifit from stripping
     // it need the largest context possible
     // so we check firstmost
@@ -276,7 +285,7 @@ mod tests {
             frange.range,
             "selection is not an expression(yet contained in one)"
         );
-        let name = variable(&expr, &sema);
+        let name = for_variable(&expr, &sema);
         assert_eq!(&name, expected);
     }
 

crates/syntax/src/ast/edit_in_place.rs

@@ -195,18 +195,13 @@ impl ast::GenericParamList {
     pub fn add_generic_param(&self, generic_param: ast::GenericParam) {
         match self.generic_params().last() {
             Some(last_param) => {
-                let mut elems = Vec::new();
+                let position = Position::after(last_param.syntax());
-                if !last_param
+                let elements = vec![
-                    .syntax()
+                    make::token(T![,]).into(),
-                    .siblings_with_tokens(Direction::Next)
+                    make::tokens::single_space().into(),
-                    .any(|it| it.kind() == T![,])
+                    generic_param.syntax().clone().into(),
-                {
+                ];
-                    elems.push(make::token(T![,]).into());
+                ted::insert_all(position, elements);
-                    elems.push(make::tokens::single_space().into());
-                };
-                elems.push(generic_param.syntax().clone().into());
-                let after_last_param = Position::after(last_param.syntax());
-                ted::insert_all(after_last_param, elems);
             }
             None => {
                 let after_l_angle = Position::after(self.l_angle_token().unwrap());

crates/syntax/src/ast/make.rs

@@ -475,8 +475,8 @@ pub fn param_list(
     };
     ast_from_text(&list)
 }
-
+// FIXME: s/&str/ast:Name
-pub fn generic_param(name: String, ty: Option<ast::TypeBoundList>) -> ast::GenericParam {
+pub fn generic_param(name: &str, ty: Option<ast::TypeBoundList>) -> ast::GenericParam {
     let bound = match ty {
         Some(it) => format!(": {}", it),
         None => String::new(),