From 02c7b8b9ba184850d1857cf5b3cf534c9ea95179 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Mon, 10 Jul 2023 23:56:59 -0400 Subject: [PATCH 01/10] Add `MethodCallExpr::get_or_create_generic_arg_list` Mirrors `PathSegment's` version, except that it always generates a turbofish --- crates/syntax/src/ast/edit_in_place.rs | 20 +++++++++++++++++++- crates/syntax/src/ast/make.rs | 7 +++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index a85e1d1d9d..097c444797 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -14,7 +14,7 @@ use crate::{ SyntaxNode, SyntaxToken, }; -use super::HasName; +use super::{HasArgList, HasName}; pub trait GenericParamsOwnerEdit: ast::HasGenericParams { fn get_or_create_generic_param_list(&self) -> ast::GenericParamList; @@ -362,6 +362,24 @@ impl ast::PathSegment { } } +impl ast::MethodCallExpr { + pub fn get_or_create_generic_arg_list(&self) -> ast::GenericArgList { + if self.generic_arg_list().is_none() { + let generic_arg_list = make::turbofish_generic_arg_list(empty()).clone_for_update(); + + if let Some(arg_list) = self.arg_list() { + ted::insert_raw( + ted::Position::before(arg_list.syntax()), + generic_arg_list.syntax(), + ); + } else { + ted::append_child(self.syntax(), generic_arg_list.syntax()); + } + } + self.generic_arg_list().unwrap() + } +} + impl Removable for ast::UseTree { fn remove(&self) { for dir in [Direction::Next, Direction::Prev] { diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 31a858b91a..8a701f6292 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -941,6 +941,13 @@ pub fn lifetime_arg(lifetime: ast::Lifetime) -> ast::LifetimeArg { ast_from_text(&format!("const S: T<{lifetime}> = ();")) } +pub fn turbofish_generic_arg_list( + args: impl IntoIterator, +) -> ast::GenericArgList { + let args = args.into_iter().join(", "); + ast_from_text(&format!("const S: T::<{args}> = ();")) +} + pub(crate) fn generic_arg_list( args: impl IntoIterator, ) -> ast::GenericArgList { From cc4e06f04bf68284f9152e77312ddfc14de613b4 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Tue, 11 Jul 2023 00:02:46 -0400 Subject: [PATCH 02/10] Migrate `add_turbo_fish` to mutable ast `add_type_ascription` is still left as-is since it's a different assist --- .../src/handlers/add_turbo_fish.rs | 100 ++++++++++++------ 1 file changed, 66 insertions(+), 34 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_turbo_fish.rs b/crates/ide-assists/src/handlers/add_turbo_fish.rs index 36f68d1767..6d973a24c4 100644 --- a/crates/ide-assists/src/handlers/add_turbo_fish.rs +++ b/crates/ide-assists/src/handlers/add_turbo_fish.rs @@ -1,6 +1,9 @@ +use either::Either; use ide_db::defs::{Definition, NameRefClass}; -use itertools::Itertools; -use syntax::{ast, AstNode, SyntaxKind, T}; +use syntax::{ + ast::{self, make, HasArgList}, + ted, AstNode, +}; use crate::{ assist_context::{AssistContext, Assists}, @@ -25,21 +28,45 @@ use crate::{ // } // ``` pub(crate) fn add_turbo_fish(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { - let ident = ctx.find_token_syntax_at_offset(SyntaxKind::IDENT).or_else(|| { - let arg_list = ctx.find_node_at_offset::()?; - if arg_list.args().next().is_some() { - return None; - } - cov_mark::hit!(add_turbo_fish_after_call); - cov_mark::hit!(add_type_ascription_after_call); - arg_list.l_paren_token()?.prev_token().filter(|it| it.kind() == SyntaxKind::IDENT) - })?; - let next_token = ident.next_token()?; - if next_token.kind() == T![::] { + let turbofish_target = + ctx.find_node_at_offset::().map(Either::Left).or_else(|| { + let callable_expr = ctx.find_node_at_offset::()?; + + if callable_expr.arg_list()?.args().next().is_some() { + return None; + } + + cov_mark::hit!(add_turbo_fish_after_call); + cov_mark::hit!(add_type_ascription_after_call); + + match callable_expr { + ast::CallableExpr::Call(it) => { + let ast::Expr::PathExpr(path) = it.expr()? else { + return None; + }; + + Some(Either::Left(path.path()?.segment()?)) + } + ast::CallableExpr::MethodCall(it) => Some(Either::Right(it)), + } + })?; + + let already_has_turbofish = match &turbofish_target { + Either::Left(path_segment) => path_segment.generic_arg_list().is_some(), + Either::Right(method_call) => method_call.generic_arg_list().is_some(), + }; + + if already_has_turbofish { cov_mark::hit!(add_turbo_fish_one_fish_is_enough); return None; } - let name_ref = ast::NameRef::cast(ident.parent()?)?; + + let name_ref = match &turbofish_target { + Either::Left(path_segment) => path_segment.name_ref()?, + Either::Right(method_call) => method_call.name_ref()?, + }; + let ident = name_ref.ident_token()?; + let def = match NameRefClass::classify(&ctx.sema, &name_ref)? { NameRefClass::Definition(def) => def, NameRefClass::FieldShorthand { .. } | NameRefClass::ExternCrateShorthand { .. } => { @@ -91,33 +118,38 @@ pub(crate) fn add_turbo_fish(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti AssistId("add_turbo_fish", AssistKind::RefactorRewrite), "Add `::<>`", ident.text_range(), - |builder| { - builder.trigger_signature_help(); - match ctx.config.snippet_cap { - Some(cap) => { - let fish_head = get_snippet_fish_head(number_of_arguments); - let snip = format!("::<{fish_head}>"); - builder.insert_snippet(cap, ident.text_range().end(), snip) + |edit| { + edit.trigger_signature_help(); + + let new_arg_list = match turbofish_target { + Either::Left(path_segment) => { + edit.make_mut(path_segment).get_or_create_generic_arg_list() } - None => { - let fish_head = std::iter::repeat("_").take(number_of_arguments).format(", "); - let snip = format!("::<{fish_head}>"); - builder.insert(ident.text_range().end(), snip); + Either::Right(method_call) => { + edit.make_mut(method_call).get_or_create_generic_arg_list() + } + }; + + let fish_head = get_fish_head(number_of_arguments).clone_for_update(); + + // Note: we need to replace the `new_arg_list` instead of being able to use something like + // `GenericArgList::add_generic_arg` as `PathSegment::get_or_create_generic_arg_list` + // always creates a non-turbofish form generic arg list. + ted::replace(new_arg_list.syntax(), fish_head.syntax()); + + if let Some(cap) = ctx.config.snippet_cap { + for arg in fish_head.generic_args() { + edit.add_placeholder_snippet(cap, arg) } } }, ) } -/// This will create a snippet string with tabstops marked -fn get_snippet_fish_head(number_of_arguments: usize) -> String { - let mut fish_head = (1..number_of_arguments) - .format_with("", |i, f| f(&format_args!("${{{i}:_}}, "))) - .to_string(); - - // tabstop 0 is a special case and always the last one - fish_head.push_str("${0:_}"); - fish_head +/// This will create a turbofish generic arg list corresponding to the number of arguments +fn get_fish_head(number_of_arguments: usize) -> ast::GenericArgList { + let args = (0..number_of_arguments).map(|_| make::type_arg(make::ty_placeholder()).into()); + make::turbofish_generic_arg_list(args) } #[cfg(test)] From 92422f7488888c3868c9f5ddb080f26566f9f382 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Sun, 16 Jul 2023 16:21:49 -0400 Subject: [PATCH 03/10] Use syntax's version of `SyntaxElement` --- crates/syntax/src/ast/edit_in_place.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index 097c444797..6c6a2bc71e 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -3,13 +3,12 @@ use std::iter::{empty, successors}; use parser::{SyntaxKind, T}; -use rowan::SyntaxElement; use crate::{ algo::{self, neighbor}, ast::{self, edit::IndentLevel, make, HasGenericParams}, ted::{self, Position}, - AstNode, AstToken, Direction, + AstNode, AstToken, Direction, SyntaxElement, SyntaxKind::{ATTR, COMMENT, WHITESPACE}, SyntaxNode, SyntaxToken, }; @@ -577,7 +576,7 @@ impl ast::AssocItemList { None => (IndentLevel::single(), Position::last_child_of(self.syntax()), "\n"), }, }; - let elements: Vec> = vec![ + let elements: Vec = vec![ make::tokens::whitespace(&format!("{whitespace}{indent}")).into(), item.syntax().clone().into(), ]; @@ -771,7 +770,7 @@ impl ast::VariantList { None => (IndentLevel::single(), Position::last_child_of(self.syntax())), }, }; - let elements: Vec> = vec![ + let elements: Vec = vec![ make::tokens::whitespace(&format!("{}{indent}", "\n")).into(), variant.syntax().clone().into(), ast::make::token(T![,]).into(), From 5fc8cc52e2ebc30d720873236293f5469bf5cfd8 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Sun, 16 Jul 2023 16:22:29 -0400 Subject: [PATCH 04/10] Add `LetStmt::set_ty` Way for setting and removing the type ascription of a let stmt --- crates/syntax/src/ast/edit_in_place.rs | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index 6c6a2bc71e..b9059a527d 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -646,6 +646,47 @@ impl ast::MatchArmList { } } +impl ast::LetStmt { + pub fn set_ty(&self, ty: Option) { + match ty { + None => { + if let Some(colon_token) = self.colon_token() { + ted::remove(colon_token); + } + + if let Some(existing_ty) = self.ty() { + if let Some(sibling) = existing_ty.syntax().prev_sibling_or_token() { + if sibling.kind() == SyntaxKind::WHITESPACE { + ted::remove(sibling); + } + } + + ted::remove(existing_ty.syntax()); + } + } + Some(new_ty) => { + if self.colon_token().is_none() { + let mut to_insert: Vec = vec![]; + + let position = match self.pat() { + Some(pat) => Position::after(pat.syntax()), + None => { + to_insert.push(make::tokens::single_space().into()); + Position::after(self.let_token().unwrap()) + } + }; + + to_insert.push(make::token(T![:]).into()); + + ted::insert_all_raw(position, to_insert); + } + + ted::insert(Position::after(self.colon_token().unwrap()), new_ty.syntax()); + } + } + } +} + impl ast::RecordExprFieldList { pub fn add_field(&self, field: ast::RecordExprField) { let is_multiline = self.syntax().text().contains_char('\n'); From f3dcc67dfa62ac0950352c719e657687428f79f6 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Sun, 16 Jul 2023 16:24:13 -0400 Subject: [PATCH 05/10] Migrate `add_type_ascription` --- .../src/handlers/add_turbo_fish.rs | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_turbo_fish.rs b/crates/ide-assists/src/handlers/add_turbo_fish.rs index 6d973a24c4..6afc1693fa 100644 --- a/crates/ide-assists/src/handlers/add_turbo_fish.rs +++ b/crates/ide-assists/src/handlers/add_turbo_fish.rs @@ -85,20 +85,23 @@ pub(crate) fn add_turbo_fish(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti if let Some(let_stmt) = ctx.find_node_at_offset::() { if let_stmt.colon_token().is_none() { - let type_pos = let_stmt.pat()?.syntax().last_token()?.text_range().end(); - let semi_pos = let_stmt.syntax().last_token()?.text_range().end(); - acc.add( AssistId("add_type_ascription", AssistKind::RefactorRewrite), "Add `: _` before assignment operator", ident.text_range(), - |builder| { + |edit| { + let let_stmt = edit.make_mut(let_stmt); + if let_stmt.semicolon_token().is_none() { - builder.insert(semi_pos, ";"); + ted::append_child(let_stmt.syntax(), make::tokens::semicolon()); } - match ctx.config.snippet_cap { - Some(cap) => builder.insert_snippet(cap, type_pos, ": ${0:_}"), - None => builder.insert(type_pos, ": _"), + + let placeholder_ty = make::ty_placeholder().clone_for_update(); + + let_stmt.set_ty(Some(placeholder_ty.clone())); + + if let Some(cap) = ctx.config.snippet_cap { + edit.add_placeholder_snippet(cap, placeholder_ty); } }, )? @@ -395,6 +398,26 @@ fn main() { ); } + #[test] + fn add_type_ascription_missing_pattern() { + check_assist_by_label( + add_turbo_fish, + r#" +fn make() -> T {} +fn main() { + let = make$0() +} +"#, + r#" +fn make() -> T {} +fn main() { + let : ${0:_} = make(); +} +"#, + "Add `: _` before assignment operator", + ); + } + #[test] fn add_turbo_fish_function_lifetime_parameter() { check_assist( From 4aaa592a9a7461cf844e70bcc39a16173affab26 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Sun, 16 Jul 2023 23:03:39 -0400 Subject: [PATCH 06/10] Migrate `destructure_tuple_binding` to mutable ast Due to the way the current tree mutation api works, we need to collect changes before we can apply them to the real syntax tree, and also can only switch to a file once. `destructure_tuple_binding_in_sub_pattern` also gets migrated even though can't be used. --- .../src/handlers/destructure_tuple_binding.rs | 249 +++++++++++------- crates/syntax/src/ast/make.rs | 2 +- 2 files changed, 157 insertions(+), 94 deletions(-) diff --git a/crates/ide-assists/src/handlers/destructure_tuple_binding.rs b/crates/ide-assists/src/handlers/destructure_tuple_binding.rs index f30ca2552d..6a012d30bf 100644 --- a/crates/ide-assists/src/handlers/destructure_tuple_binding.rs +++ b/crates/ide-assists/src/handlers/destructure_tuple_binding.rs @@ -3,10 +3,12 @@ use ide_db::{ defs::Definition, search::{FileReference, SearchScope, UsageSearchResult}, }; +use itertools::Itertools; use syntax::{ - ast::{self, AstNode, FieldExpr, HasName, IdentPat, MethodCallExpr}, - TextRange, + ast::{self, make, AstNode, FieldExpr, HasName, IdentPat, MethodCallExpr}, + ted, T, }; +use text_edit::TextRange; use crate::assist_context::{AssistContext, Assists, SourceChangeBuilder}; @@ -61,27 +63,36 @@ pub(crate) fn destructure_tuple_binding_impl( acc.add( AssistId("destructure_tuple_binding_in_sub_pattern", AssistKind::RefactorRewrite), "Destructure tuple in sub-pattern", - data.range, - |builder| { - edit_tuple_assignment(ctx, builder, &data, true); - edit_tuple_usages(&data, builder, ctx, true); - }, + data.ident_pat.syntax().text_range(), + |edit| destructure_tuple_edit_impl(ctx, edit, &data, true), ); } acc.add( AssistId("destructure_tuple_binding", AssistKind::RefactorRewrite), if with_sub_pattern { "Destructure tuple in place" } else { "Destructure tuple" }, - data.range, - |builder| { - edit_tuple_assignment(ctx, builder, &data, false); - edit_tuple_usages(&data, builder, ctx, false); - }, + data.ident_pat.syntax().text_range(), + |edit| destructure_tuple_edit_impl(ctx, edit, &data, false), ); Some(()) } +fn destructure_tuple_edit_impl( + ctx: &AssistContext<'_>, + edit: &mut SourceChangeBuilder, + data: &TupleData, + in_sub_pattern: bool, +) { + let assignment_edit = edit_tuple_assignment(ctx, edit, &data, in_sub_pattern); + let current_file_usages_edit = edit_tuple_usages(&data, edit, ctx, in_sub_pattern); + + assignment_edit.apply(); + if let Some(usages_edit) = current_file_usages_edit { + usages_edit.into_iter().for_each(|usage_edit| usage_edit.apply(edit)) + } +} + fn collect_data(ident_pat: IdentPat, ctx: &AssistContext<'_>) -> Option { if ident_pat.at_token().is_some() { // Cannot destructure pattern with sub-pattern: @@ -109,7 +120,6 @@ fn collect_data(ident_pat: IdentPat, ctx: &AssistContext<'_>) -> Option) -> Option>(); - Some(TupleData { ident_pat, range, ref_type, field_names, usages }) + Some(TupleData { ident_pat, ref_type, field_names, usages }) } fn generate_name( @@ -142,72 +152,106 @@ enum RefType { } struct TupleData { ident_pat: IdentPat, - // name: String, - range: TextRange, ref_type: Option, field_names: Vec, - // field_types: Vec, usages: Option, } fn edit_tuple_assignment( ctx: &AssistContext<'_>, - builder: &mut SourceChangeBuilder, + edit: &mut SourceChangeBuilder, data: &TupleData, in_sub_pattern: bool, -) { +) -> AssignmentEdit { + let ident_pat = edit.make_mut(data.ident_pat.clone()); + let tuple_pat = { let original = &data.ident_pat; let is_ref = original.ref_token().is_some(); let is_mut = original.mut_token().is_some(); - let fields = data.field_names.iter().map(|name| { - ast::Pat::from(ast::make::ident_pat(is_ref, is_mut, ast::make::name(name))) - }); - ast::make::tuple_pat(fields) + let fields = data + .field_names + .iter() + .map(|name| ast::Pat::from(make::ident_pat(is_ref, is_mut, make::name(name)))); + make::tuple_pat(fields).clone_for_update() }; - let add_cursor = |text: &str| { - // place cursor on first tuple item - let first_tuple = &data.field_names[0]; - text.replacen(first_tuple, &format!("$0{first_tuple}"), 1) - }; + if let Some(cap) = ctx.config.snippet_cap { + // place cursor on first tuple name + let ast::Pat::IdentPat(first_pat) = tuple_pat.fields().next().unwrap() else { + unreachable!() + }; + edit.add_tabstop_before(cap, first_pat.name().unwrap()) + } - // with sub_pattern: keep original tuple and add subpattern: `tup @ (_0, _1)` - if in_sub_pattern { - let text = format!(" @ {tuple_pat}"); - match ctx.config.snippet_cap { - Some(cap) => { - let snip = add_cursor(&text); - builder.insert_snippet(cap, data.range.end(), snip); - } - None => builder.insert(data.range.end(), text), - }; - } else { - let text = tuple_pat.to_string(); - match ctx.config.snippet_cap { - Some(cap) => { - let snip = add_cursor(&text); - builder.replace_snippet(cap, data.range, snip); - } - None => builder.replace(data.range, text), - }; + AssignmentEdit { ident_pat, tuple_pat, in_sub_pattern } +} +struct AssignmentEdit { + ident_pat: ast::IdentPat, + tuple_pat: ast::TuplePat, + in_sub_pattern: bool, +} + +impl AssignmentEdit { + fn apply(self) { + // with sub_pattern: keep original tuple and add subpattern: `tup @ (_0, _1)` + if self.in_sub_pattern { + ted::insert_all_raw( + ted::Position::after(self.ident_pat.syntax()), + vec![ + make::tokens::single_space().into(), + make::token(T![@]).into(), + make::tokens::single_space().into(), + self.tuple_pat.syntax().clone().into(), + ], + ) + } else { + ted::replace(self.ident_pat.syntax(), self.tuple_pat.syntax()) + } } } fn edit_tuple_usages( data: &TupleData, - builder: &mut SourceChangeBuilder, + edit: &mut SourceChangeBuilder, ctx: &AssistContext<'_>, in_sub_pattern: bool, -) { - if let Some(usages) = data.usages.as_ref() { - for (file_id, refs) in usages.iter() { - builder.edit_file(*file_id); +) -> Option> { + let mut current_file_usages = None; - for r in refs { - edit_tuple_usage(ctx, builder, r, data, in_sub_pattern); + if let Some(usages) = data.usages.as_ref() { + // We need to collect edits first before actually applying them + // as mapping nodes to their mutable node versions requires an + // unmodified syntax tree. + // + // We also defer editing usages in the current file first since + // tree mutation in the same file breaks when `builder.edit_file` + // is called + + if let Some((_, refs)) = usages.iter().find(|(file_id, _)| **file_id == ctx.file_id()) { + current_file_usages = Some( + refs.iter() + .filter_map(|r| edit_tuple_usage(ctx, edit, r, data, in_sub_pattern)) + .collect_vec(), + ); + } + + for (file_id, refs) in usages.iter() { + if *file_id == ctx.file_id() { + continue; } + + edit.edit_file(*file_id); + + let tuple_edits = refs + .iter() + .filter_map(|r| edit_tuple_usage(ctx, edit, r, data, in_sub_pattern)) + .collect_vec(); + + tuple_edits.into_iter().for_each(|tuple_edit| tuple_edit.apply(edit)) } } + + current_file_usages } fn edit_tuple_usage( ctx: &AssistContext<'_>, @@ -215,25 +259,14 @@ fn edit_tuple_usage( usage: &FileReference, data: &TupleData, in_sub_pattern: bool, -) { +) -> Option { match detect_tuple_index(usage, data) { - Some(index) => edit_tuple_field_usage(ctx, builder, data, index), - None => { - if in_sub_pattern { - cov_mark::hit!(destructure_tuple_call_with_subpattern); - return; - } - - // no index access -> make invalid -> requires handling by user - // -> put usage in block comment - // - // Note: For macro invocations this might result in still valid code: - // When a macro accepts the tuple as argument, as well as no arguments at all, - // uncommenting the tuple still leaves the macro call working (see `tests::in_macro_call::empty_macro`). - // But this is an unlikely case. Usually the resulting macro call will become erroneous. - builder.insert(usage.range.start(), "/*"); - builder.insert(usage.range.end(), "*/"); + Some(index) => Some(edit_tuple_field_usage(ctx, builder, data, index)), + None if in_sub_pattern => { + cov_mark::hit!(destructure_tuple_call_with_subpattern); + return None; } + None => Some(EditTupleUsage::NoIndex(usage.range)), } } @@ -242,19 +275,47 @@ fn edit_tuple_field_usage( builder: &mut SourceChangeBuilder, data: &TupleData, index: TupleIndex, -) { +) -> EditTupleUsage { let field_name = &data.field_names[index.index]; + let field_name = make::expr_path(make::ext::ident_path(field_name)); if data.ref_type.is_some() { - let ref_data = handle_ref_field_usage(ctx, &index.field_expr); - builder.replace(ref_data.range, ref_data.format(field_name)); + let (replace_expr, ref_data) = handle_ref_field_usage(ctx, &index.field_expr); + let replace_expr = builder.make_mut(replace_expr); + EditTupleUsage::ReplaceExpr(replace_expr, ref_data.wrap_expr(field_name)) } else { - builder.replace(index.range, field_name); + let field_expr = builder.make_mut(index.field_expr); + EditTupleUsage::ReplaceExpr(field_expr.into(), field_name) } } +enum EditTupleUsage { + /// no index access -> make invalid -> requires handling by user + /// -> put usage in block comment + /// + /// Note: For macro invocations this might result in still valid code: + /// When a macro accepts the tuple as argument, as well as no arguments at all, + /// uncommenting the tuple still leaves the macro call working (see `tests::in_macro_call::empty_macro`). + /// But this is an unlikely case. Usually the resulting macro call will become erroneous. + NoIndex(TextRange), + ReplaceExpr(ast::Expr, ast::Expr), +} + +impl EditTupleUsage { + fn apply(self, edit: &mut SourceChangeBuilder) { + match self { + EditTupleUsage::NoIndex(range) => { + edit.insert(range.start(), "/*"); + edit.insert(range.end(), "*/"); + } + EditTupleUsage::ReplaceExpr(target_expr, replace_with) => { + ted::replace(target_expr.syntax(), replace_with.clone_for_update().syntax()) + } + } + } +} + struct TupleIndex { index: usize, - range: TextRange, field_expr: FieldExpr, } fn detect_tuple_index(usage: &FileReference, data: &TupleData) -> Option { @@ -296,7 +357,7 @@ fn detect_tuple_index(usage: &FileReference, data: &TupleData) -> Option Option String { - match (self.needs_deref, self.needs_parentheses) { - (true, true) => format!("(*{field_name})"), - (true, false) => format!("*{field_name}"), - (false, true) => format!("({field_name})"), - (false, false) => field_name.to_string(), + fn wrap_expr(&self, mut expr: ast::Expr) -> ast::Expr { + if self.needs_deref { + expr = make::expr_prefix(T![*], expr); } + + if self.needs_parentheses { + expr = make::expr_paren(expr); + } + + return expr; } } -fn handle_ref_field_usage(ctx: &AssistContext<'_>, field_expr: &FieldExpr) -> RefData { +fn handle_ref_field_usage(ctx: &AssistContext<'_>, field_expr: &FieldExpr) -> (ast::Expr, RefData) { let s = field_expr.syntax(); - let mut ref_data = - RefData { range: s.text_range(), needs_deref: true, needs_parentheses: true }; + let mut ref_data = RefData { needs_deref: true, needs_parentheses: true }; + let mut target_node = field_expr.clone().into(); let parent = match s.parent().map(ast::Expr::cast) { Some(Some(parent)) => parent, Some(None) => { ref_data.needs_parentheses = false; - return ref_data; + return (target_node, ref_data); } - None => return ref_data, + None => return (target_node, ref_data), }; match parent { @@ -342,7 +405,7 @@ fn handle_ref_field_usage(ctx: &AssistContext<'_>, field_expr: &FieldExpr) -> Re // there might be a ref outside: `&(t.0)` -> can be removed if let Some(it) = it.syntax().parent().and_then(ast::RefExpr::cast) { ref_data.needs_deref = false; - ref_data.range = it.syntax().text_range(); + target_node = it.into(); } } ast::Expr::RefExpr(it) => { @@ -351,8 +414,8 @@ fn handle_ref_field_usage(ctx: &AssistContext<'_>, field_expr: &FieldExpr) -> Re ref_data.needs_parentheses = false; // might be surrounded by parens -> can be removed too match it.syntax().parent().and_then(ast::ParenExpr::cast) { - Some(parent) => ref_data.range = parent.syntax().text_range(), - None => ref_data.range = it.syntax().text_range(), + Some(parent) => target_node = parent.into(), + None => target_node = it.into(), }; } // higher precedence than deref `*` @@ -414,7 +477,7 @@ fn handle_ref_field_usage(ctx: &AssistContext<'_>, field_expr: &FieldExpr) -> Re } }; - ref_data + (target_node, ref_data) } #[cfg(test)] diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 8a701f6292..ad63cc5586 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -1133,7 +1133,7 @@ pub mod tokens { pub(super) static SOURCE_FILE: Lazy> = Lazy::new(|| { SourceFile::parse( - "const C: <()>::Item = ( true && true , true || true , 1 != 1, 2 == 2, 3 < 3, 4 <= 4, 5 > 5, 6 >= 6, !true, *p, &p , &mut p)\n;\n\n", + "const C: <()>::Item = ( true && true , true || true , 1 != 1, 2 == 2, 3 < 3, 4 <= 4, 5 > 5, 6 >= 6, !true, *p, &p , &mut p, { let a @ [] })\n;\n\n", ) }); From 6f68cd33947c3f361e47c24904605e49e5637eba Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Mon, 13 Nov 2023 18:42:58 -0500 Subject: [PATCH 07/10] Remove unwraps from `destructure_tuple_binding` --- .../src/handlers/destructure_tuple_binding.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/ide-assists/src/handlers/destructure_tuple_binding.rs b/crates/ide-assists/src/handlers/destructure_tuple_binding.rs index 6a012d30bf..2dc30e685a 100644 --- a/crates/ide-assists/src/handlers/destructure_tuple_binding.rs +++ b/crates/ide-assists/src/handlers/destructure_tuple_binding.rs @@ -177,10 +177,12 @@ fn edit_tuple_assignment( if let Some(cap) = ctx.config.snippet_cap { // place cursor on first tuple name - let ast::Pat::IdentPat(first_pat) = tuple_pat.fields().next().unwrap() else { - unreachable!() - }; - edit.add_tabstop_before(cap, first_pat.name().unwrap()) + if let Some(ast::Pat::IdentPat(first_pat)) = tuple_pat.fields().next() { + edit.add_tabstop_before( + cap, + first_pat.name().expect("first ident pattern should have a name"), + ) + } } AssignmentEdit { ident_pat, tuple_pat, in_sub_pattern } From 787ca888e35eaa351c9755f4f4de75acca8bf836 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Mon, 13 Nov 2023 20:30:58 -0500 Subject: [PATCH 08/10] Add `IdentPat::set_pat` Needed so that the `tuple_pat` node gets added to the syntax tree, which is required as we're using structured snippets. --- .../src/handlers/destructure_tuple_binding.rs | 10 +-- crates/syntax/src/ast/edit_in_place.rs | 75 +++++++++++++++++++ 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/crates/ide-assists/src/handlers/destructure_tuple_binding.rs b/crates/ide-assists/src/handlers/destructure_tuple_binding.rs index 2dc30e685a..65b497e83a 100644 --- a/crates/ide-assists/src/handlers/destructure_tuple_binding.rs +++ b/crates/ide-assists/src/handlers/destructure_tuple_binding.rs @@ -197,15 +197,7 @@ impl AssignmentEdit { fn apply(self) { // with sub_pattern: keep original tuple and add subpattern: `tup @ (_0, _1)` if self.in_sub_pattern { - ted::insert_all_raw( - ted::Position::after(self.ident_pat.syntax()), - vec![ - make::tokens::single_space().into(), - make::token(T![@]).into(), - make::tokens::single_space().into(), - self.tuple_pat.syntax().clone().into(), - ], - ) + self.ident_pat.set_pat(Some(self.tuple_pat.into())) } else { ted::replace(self.ident_pat.syntax(), self.tuple_pat.syntax()) } diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index b9059a527d..edb55a2f13 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -846,6 +846,53 @@ fn normalize_ws_between_braces(node: &SyntaxNode) -> Option<()> { Some(()) } +impl ast::IdentPat { + pub fn set_pat(&self, pat: Option) { + match pat { + None => { + if let Some(at_token) = self.at_token() { + // Remove `@ Pat` + let start = at_token.clone().into(); + let end = self + .pat() + .map(|it| it.syntax().clone().into()) + .unwrap_or_else(|| at_token.into()); + + ted::remove_all(start..=end); + + // Remove any trailing ws + if let Some(last) = + self.syntax().last_token().filter(|it| it.kind() == WHITESPACE) + { + last.detach(); + } + } + } + Some(pat) => { + if let Some(old_pat) = self.pat() { + // Replace existing pattern + ted::replace(old_pat.syntax(), pat.syntax()) + } else if let Some(at_token) = self.at_token() { + // Have an `@` token but not a pattern yet + ted::insert(ted::Position::after(at_token), pat.syntax()); + } else { + // Don't have an `@`, should have a name + let name = self.name().unwrap(); + + ted::insert_all( + ted::Position::after(name.syntax()), + vec![ + make::token(T![@]).into(), + make::tokens::single_space().into(), + pat.syntax().clone().into(), + ], + ) + } + } + } + } +} + pub trait HasVisibilityEdit: ast::HasVisibility { fn set_visibility(&self, visbility: ast::Visibility) { match self.visibility() { @@ -947,6 +994,34 @@ mod tests { ); } + #[test] + fn test_ident_pat_set_pat() { + #[track_caller] + fn check(before: &str, expected: &str, pat: Option) { + let pat = pat.map(|it| it.clone_for_update()); + + let ident_pat = ast_mut_from_text::(&format!("fn f() {{ {before} }}")); + ident_pat.set_pat(pat); + + let after = ast_mut_from_text::(&format!("fn f() {{ {expected} }}")); + assert_eq!(ident_pat.to_string(), after.to_string()); + } + + // replacing + check("let a @ _;", "let a @ ();", Some(make::tuple_pat([]).into())); + + // note: no trailing semicolon is added for the below tests since it + // seems to be picked up by the ident pat during error recovery? + + // adding + check("let a ", "let a @ ()", Some(make::tuple_pat([]).into())); + check("let a @ ", "let a @ ()", Some(make::tuple_pat([]).into())); + + // removing + check("let a @ ()", "let a", None); + check("let a @ ", "let a", None); + } + #[test] fn add_variant_to_empty_enum() { let variant = make::variant(make::name("Bar"), None).clone_for_update(); From df629627c5bd63ea8f44eca1751f2026baf65e5b Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Tue, 14 Nov 2023 17:35:24 -0500 Subject: [PATCH 09/10] Add tests for `LetStmt::set_ty` --- crates/syntax/src/ast/edit_in_place.rs | 62 ++++++++++++++++++++------ 1 file changed, 48 insertions(+), 14 deletions(-) diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index edb55a2f13..37d8212042 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -663,25 +663,28 @@ impl ast::LetStmt { ted::remove(existing_ty.syntax()); } + + // Remove any trailing ws + if let Some(last) = self.syntax().last_token().filter(|it| it.kind() == WHITESPACE) + { + last.detach(); + } } Some(new_ty) => { if self.colon_token().is_none() { - let mut to_insert: Vec = vec![]; - - let position = match self.pat() { - Some(pat) => Position::after(pat.syntax()), - None => { - to_insert.push(make::tokens::single_space().into()); - Position::after(self.let_token().unwrap()) - } - }; - - to_insert.push(make::token(T![:]).into()); - - ted::insert_all_raw(position, to_insert); + ted::insert_raw( + Position::after( + self.pat().expect("let stmt should have a pattern").syntax(), + ), + make::token(T![:]), + ); } - ted::insert(Position::after(self.colon_token().unwrap()), new_ty.syntax()); + if let Some(old_ty) = self.ty() { + ted::replace(old_ty.syntax(), new_ty.syntax()); + } else { + ted::insert(Position::after(self.colon_token().unwrap()), new_ty.syntax()); + } } } } @@ -1022,6 +1025,37 @@ mod tests { check("let a @ ", "let a", None); } + #[test] + fn test_let_stmt_set_ty() { + #[track_caller] + fn check(before: &str, expected: &str, ty: Option) { + let ty = ty.map(|it| it.clone_for_update()); + + let let_stmt = ast_mut_from_text::(&format!("fn f() {{ {before} }}")); + let_stmt.set_ty(ty); + + let after = ast_mut_from_text::(&format!("fn f() {{ {expected} }}")); + assert_eq!(let_stmt.to_string(), after.to_string(), "{let_stmt:#?}\n!=\n{after:#?}"); + } + + // adding + check("let a;", "let a: ();", Some(make::ty_tuple([]))); + // no semicolon due to it being eaten during error recovery + check("let a:", "let a: ()", Some(make::ty_tuple([]))); + + // replacing + check("let a: u8;", "let a: ();", Some(make::ty_tuple([]))); + check("let a: u8 = 3;", "let a: () = 3;", Some(make::ty_tuple([]))); + check("let a: = 3;", "let a: () = 3;", Some(make::ty_tuple([]))); + + // removing + check("let a: u8;", "let a;", None); + check("let a:;", "let a;", None); + + check("let a: u8 = 3;", "let a = 3;", None); + check("let a: = 3;", "let a = 3;", None); + } + #[test] fn add_variant_to_empty_enum() { let variant = make::variant(make::name("Bar"), None).clone_for_update(); From 3f99a56faef063b236744b5689d0918dd62b6fa0 Mon Sep 17 00:00:00 2001 From: DropDemBits Date: Tue, 14 Nov 2023 17:50:33 -0500 Subject: [PATCH 10/10] Fix panic in `add_type_ascription` Assist wasn't applicable when the let statement was missing a pattern before, so we should do the same now. --- .../ide-assists/src/handlers/add_turbo_fish.rs | 17 +++++++++-------- crates/ide-assists/src/tests.rs | 5 +++++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_turbo_fish.rs b/crates/ide-assists/src/handlers/add_turbo_fish.rs index 6afc1693fa..88fd0b1b73 100644 --- a/crates/ide-assists/src/handlers/add_turbo_fish.rs +++ b/crates/ide-assists/src/handlers/add_turbo_fish.rs @@ -85,6 +85,10 @@ pub(crate) fn add_turbo_fish(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti if let Some(let_stmt) = ctx.find_node_at_offset::() { if let_stmt.colon_token().is_none() { + if let_stmt.pat().is_none() { + return None; + } + acc.add( AssistId("add_type_ascription", AssistKind::RefactorRewrite), "Add `: _` before assignment operator", @@ -157,7 +161,10 @@ fn get_fish_head(number_of_arguments: usize) -> ast::GenericArgList { #[cfg(test)] mod tests { - use crate::tests::{check_assist, check_assist_by_label, check_assist_not_applicable}; + use crate::tests::{ + check_assist, check_assist_by_label, check_assist_not_applicable, + check_assist_not_applicable_by_label, + }; use super::*; @@ -400,19 +407,13 @@ fn main() { #[test] fn add_type_ascription_missing_pattern() { - check_assist_by_label( + check_assist_not_applicable_by_label( add_turbo_fish, r#" fn make() -> T {} fn main() { let = make$0() } -"#, - r#" -fn make() -> T {} -fn main() { - let : ${0:_} = make(); -} "#, "Add `: _` before assignment operator", ); diff --git a/crates/ide-assists/src/tests.rs b/crates/ide-assists/src/tests.rs index cc3e251a8f..1f72e4f03e 100644 --- a/crates/ide-assists/src/tests.rs +++ b/crates/ide-assists/src/tests.rs @@ -98,6 +98,11 @@ pub(crate) fn check_assist_not_applicable(assist: Handler, ra_fixture: &str) { check(assist, ra_fixture, ExpectedResult::NotApplicable, None); } +#[track_caller] +pub(crate) fn check_assist_not_applicable_by_label(assist: Handler, ra_fixture: &str, label: &str) { + check(assist, ra_fixture, ExpectedResult::NotApplicable, Some(label)); +} + /// Check assist in unresolved state. Useful to check assists for lazy computation. #[track_caller] pub(crate) fn check_assist_unresolved(assist: Handler, ra_fixture: &str) {