From d7ed3515730ef9a0c58af0af23a044c1d92bc87a Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 3 May 2022 19:41:07 +0400 Subject: [PATCH 1/3] Fix some typos in `ide-assists/src/lib.rs` --- crates/ide-assists/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ide-assists/src/lib.rs b/crates/ide-assists/src/lib.rs index ef4aa1c62bdf..93a9f3858ffd 100644 --- a/crates/ide-assists/src/lib.rs +++ b/crates/ide-assists/src/lib.rs @@ -37,7 +37,7 @@ //! should be available more or less everywhere, which isn't useful. So //! instead we only show it if the user *selects* the items they want to sort. //! * Consider grouping related assists together (see [`Assists::add_group`]). -//! * Make assists robust. If the assist depends on results of type-inference to +//! * Make assists robust. If the assist depends on results of type-inference too //! much, it might only fire in fully-correct code. This makes assist less //! useful and (worse) less predictable. The user should have a clear //! intuition when each particular assist is available. @@ -54,7 +54,6 @@ //! something. If something *could* be a diagnostic, it should be a //! diagnostic. Conversely, it might be valuable to turn a diagnostic with a //! lot of false errors into an assist. -//! * //! //! See also this post: //! From 2b20a05fc6cdd6223793bffe498e1b1fb2e5b09e Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Tue, 3 May 2022 19:41:33 +0400 Subject: [PATCH 2/3] Add "Sort items by trait definition" --- .../src/handlers/reorder_impl_items.rs | 270 ++++++++++++++++++ crates/ide-assists/src/lib.rs | 2 + crates/ide-assists/src/tests/generated.rs | 35 +++ 3 files changed, 307 insertions(+) create mode 100644 crates/ide-assists/src/handlers/reorder_impl_items.rs diff --git a/crates/ide-assists/src/handlers/reorder_impl_items.rs b/crates/ide-assists/src/handlers/reorder_impl_items.rs new file mode 100644 index 000000000000..87bee6c12158 --- /dev/null +++ b/crates/ide-assists/src/handlers/reorder_impl_items.rs @@ -0,0 +1,270 @@ +use hir::{PathResolution, Semantics}; +use ide_db::{FxHashMap, RootDatabase}; +use itertools::Itertools; +use syntax::{ + ast::{self, HasName}, + ted, AstNode, +}; + +use crate::{AssistContext, AssistId, AssistKind, Assists}; + +// Assist: reorder_impl_items +// +// Reorder the items of an `impl Trait`. The items will be ordered +// in the same order as in the trait definition. +// +// ``` +// trait Foo { +// type A; +// const B: u8; +// fn c(); +// } +// +// struct Bar; +// $0impl Foo for Bar { +// const B: u8 = 17; +// fn c() {} +// type A = String; +// } +// ``` +// -> +// ``` +// trait Foo { +// type A; +// const B: u8; +// fn c(); +// } +// +// struct Bar; +// impl Foo for Bar { +// type A = String; +// const B: u8 = 17; +// fn c() {} +// } +// ``` +pub(crate) fn reorder_impl_items(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { + let impl_ast = ctx.find_node_at_offset::()?; + let items = impl_ast.assoc_item_list()?; + let assoc_items = items.assoc_items().collect::>(); + + // If all items are either function or macro calls, then reorder_impl assist can be used + if assoc_items.iter().all(|i| matches!(i, ast::AssocItem::Fn(_) | ast::AssocItem::MacroCall(_))) + { + cov_mark::hit!(not_applicable_if_all_functions); + return None; + } + + let path = impl_ast + .trait_() + .and_then(|t| match t { + ast::Type::PathType(path) => Some(path), + _ => None, + })? + .path()?; + + let ranks = compute_item_ranks(&path, ctx)?; + let sorted: Vec<_> = assoc_items + .iter() + .cloned() + .sorted_by_key(|i| { + let name = match i { + ast::AssocItem::Const(c) => c.name(), + ast::AssocItem::Fn(f) => f.name(), + ast::AssocItem::TypeAlias(t) => t.name(), + ast::AssocItem::MacroCall(_) => None, + }; + + name.and_then(|n| ranks.get(&n.to_string()).copied()).unwrap_or(usize::max_value()) + }) + .collect(); + + // Don't edit already sorted methods: + if assoc_items == sorted { + cov_mark::hit!(not_applicable_if_sorted); + return None; + } + + let target = items.syntax().text_range(); + acc.add( + AssistId("reorder_impl_items", AssistKind::RefactorRewrite), + "Sort items by trait definition", + target, + |builder| { + let assoc_items = + assoc_items.into_iter().map(|item| builder.make_mut(item)).collect::>(); + assoc_items + .into_iter() + .zip(sorted) + .for_each(|(old, new)| ted::replace(old.syntax(), new.clone_for_update().syntax())); + }, + ) +} + +fn compute_item_ranks(path: &ast::Path, ctx: &AssistContext) -> Option> { + let td = trait_definition(path, &ctx.sema)?; + + Some( + td.items(ctx.db()) + .iter() + .flat_map(|i| i.name(ctx.db())) + .enumerate() + .map(|(idx, name)| (name.to_string(), idx)) + .collect(), + ) +} + +fn trait_definition(path: &ast::Path, sema: &Semantics) -> Option { + match sema.resolve_path(path)? { + PathResolution::Def(hir::ModuleDef::Trait(trait_)) => Some(trait_), + _ => None, + } +} + +#[cfg(test)] +mod tests { + use crate::tests::{check_assist, check_assist_not_applicable}; + + use super::*; + + #[test] + fn not_applicable_if_sorted() { + cov_mark::check!(not_applicable_if_sorted); + check_assist_not_applicable( + reorder_impl_items, + r#" +trait Bar { + type T; + const C: (); + fn a() {} + fn z() {} + fn b() {} +} +struct Foo; +$0impl Bar for Foo { + type T = (); + const C: () = (); + fn a() {} + fn z() {} + fn b() {} +} + "#, + ) + } + + #[test] + fn not_applicable_if_all_functions() { + cov_mark::check!(not_applicable_if_all_functions); + check_assist_not_applicable( + reorder_impl_items, + r#" +trait Bar { + fn a() {} + fn z() {} + fn b() {} +} +struct Foo; +$0impl Bar for Foo { + fn a() {} + fn z() {} + fn b() {} +} + "#, + ) + } + + #[test] + fn not_applicable_if_empty() { + check_assist_not_applicable( + reorder_impl_items, + r#" +trait Bar {}; +struct Foo; +$0impl Bar for Foo {} + "#, + ) + } + + #[test] + fn reorder_impl_trait_items() { + check_assist( + reorder_impl_items, + r#" +trait Bar { + fn a() {} + type T0; + fn c() {} + const C1: (); + fn b() {} + type T1; + fn d() {} + const C0: (); +} + +struct Foo; +$0impl Bar for Foo { + type T1 = (); + fn d() {} + fn b() {} + fn c() {} + const C1: () = (); + fn a() {} + type T0 = (); + const C0: () = (); +} + "#, + r#" +trait Bar { + fn a() {} + type T0; + fn c() {} + const C1: (); + fn b() {} + type T1; + fn d() {} + const C0: (); +} + +struct Foo; +impl Bar for Foo { + fn a() {} + type T0 = (); + fn c() {} + const C1: () = (); + fn b() {} + type T1 = (); + fn d() {} + const C0: () = (); +} + "#, + ) + } + + #[test] + fn reorder_impl_trait_items_uneven_ident_lengths() { + check_assist( + reorder_impl_items, + r#" +trait Bar { + type Foo; + type Fooo; +} + +struct Foo; +impl Bar for Foo { + type Fooo = (); + type Foo = ();$0 +}"#, + r#" +trait Bar { + type Foo; + type Fooo; +} + +struct Foo; +impl Bar for Foo { + type Foo = (); + type Fooo = (); +}"#, + ) + } +} diff --git a/crates/ide-assists/src/lib.rs b/crates/ide-assists/src/lib.rs index 93a9f3858ffd..7b2f733567c8 100644 --- a/crates/ide-assists/src/lib.rs +++ b/crates/ide-assists/src/lib.rs @@ -170,6 +170,7 @@ mod handlers { mod remove_unused_param; mod reorder_fields; mod reorder_impl; + mod reorder_impl_items; mod replace_try_expr_with_match; mod replace_derive_with_manual_impl; mod replace_if_let_with_match; @@ -257,6 +258,7 @@ mod handlers { remove_unused_param::remove_unused_param, reorder_fields::reorder_fields, reorder_impl::reorder_impl, + reorder_impl_items::reorder_impl_items, replace_try_expr_with_match::replace_try_expr_with_match, replace_derive_with_manual_impl::replace_derive_with_manual_impl, replace_if_let_with_match::replace_if_let_with_match, diff --git a/crates/ide-assists/src/tests/generated.rs b/crates/ide-assists/src/tests/generated.rs index 8a1e95d8947b..b629364b880a 100644 --- a/crates/ide-assists/src/tests/generated.rs +++ b/crates/ide-assists/src/tests/generated.rs @@ -1771,6 +1771,41 @@ impl Foo for Bar { ) } +#[test] +fn doctest_reorder_impl_items() { + check_doc_test( + "reorder_impl_items", + r#####" +trait Foo { + type A; + const B: u8; + fn c(); +} + +struct Bar; +$0impl Foo for Bar { + const B: u8 = 17; + fn c() {} + type A = String; +} +"#####, + r#####" +trait Foo { + type A; + const B: u8; + fn c(); +} + +struct Bar; +impl Foo for Bar { + type A = String; + const B: u8 = 17; + fn c() {} +} +"#####, + ) +} + #[test] fn doctest_replace_char_with_string() { check_doc_test( From e3151247982f47a7fdcf225c066c18acea229d30 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Wed, 4 May 2022 00:55:00 +0400 Subject: [PATCH 3/3] Remove "Sort methods by trait definition" assist It was replaced by the "Sort items by trait definition" assist. --- .../ide-assists/src/handlers/reorder_impl.rs | 217 ------------------ .../src/handlers/reorder_impl_items.rs | 39 ++-- crates/ide-assists/src/lib.rs | 2 - crates/ide-assists/src/tests/generated.rs | 35 --- 4 files changed, 25 insertions(+), 268 deletions(-) delete mode 100644 crates/ide-assists/src/handlers/reorder_impl.rs diff --git a/crates/ide-assists/src/handlers/reorder_impl.rs b/crates/ide-assists/src/handlers/reorder_impl.rs deleted file mode 100644 index c0991c6080de..000000000000 --- a/crates/ide-assists/src/handlers/reorder_impl.rs +++ /dev/null @@ -1,217 +0,0 @@ -use hir::{PathResolution, Semantics}; -use ide_db::{FxHashMap, RootDatabase}; -use itertools::Itertools; -use syntax::{ - ast::{self, HasName}, - ted, AstNode, -}; - -use crate::{utils::get_methods, AssistContext, AssistId, AssistKind, Assists}; - -// Assist: reorder_impl -// -// Reorder the methods of an `impl Trait`. The methods will be ordered -// in the same order as in the trait definition. -// -// ``` -// trait Foo { -// fn a() {} -// fn b() {} -// fn c() {} -// } -// -// struct Bar; -// $0impl Foo for Bar { -// fn b() {} -// fn c() {} -// fn a() {} -// } -// ``` -// -> -// ``` -// trait Foo { -// fn a() {} -// fn b() {} -// fn c() {} -// } -// -// struct Bar; -// impl Foo for Bar { -// fn a() {} -// fn b() {} -// fn c() {} -// } -// ``` -pub(crate) fn reorder_impl(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { - let impl_ast = ctx.find_node_at_offset::()?; - let items = impl_ast.assoc_item_list()?; - let methods = get_methods(&items); - - let path = impl_ast - .trait_() - .and_then(|t| match t { - ast::Type::PathType(path) => Some(path), - _ => None, - })? - .path()?; - - let ranks = compute_method_ranks(&path, ctx)?; - let sorted: Vec<_> = methods - .iter() - .cloned() - .sorted_by_key(|f| { - f.name().and_then(|n| ranks.get(&n.to_string()).copied()).unwrap_or(usize::max_value()) - }) - .collect(); - - // Don't edit already sorted methods: - if methods == sorted { - cov_mark::hit!(not_applicable_if_sorted); - return None; - } - - let target = items.syntax().text_range(); - acc.add( - AssistId("reorder_impl", AssistKind::RefactorRewrite), - "Sort methods by trait definition", - target, - |builder| { - let methods = methods.into_iter().map(|fn_| builder.make_mut(fn_)).collect::>(); - methods - .into_iter() - .zip(sorted) - .for_each(|(old, new)| ted::replace(old.syntax(), new.clone_for_update().syntax())); - }, - ) -} - -fn compute_method_ranks(path: &ast::Path, ctx: &AssistContext) -> Option> { - let td = trait_definition(path, &ctx.sema)?; - - Some( - td.items(ctx.db()) - .iter() - .flat_map(|i| match i { - hir::AssocItem::Function(f) => Some(f), - _ => None, - }) - .enumerate() - .map(|(idx, func)| (func.name(ctx.db()).to_string(), idx)) - .collect(), - ) -} - -fn trait_definition(path: &ast::Path, sema: &Semantics) -> Option { - match sema.resolve_path(path)? { - PathResolution::Def(hir::ModuleDef::Trait(trait_)) => Some(trait_), - _ => None, - } -} - -#[cfg(test)] -mod tests { - use crate::tests::{check_assist, check_assist_not_applicable}; - - use super::*; - - #[test] - fn not_applicable_if_sorted() { - cov_mark::check!(not_applicable_if_sorted); - check_assist_not_applicable( - reorder_impl, - r#" -trait Bar { - fn a() {} - fn z() {} - fn b() {} -} -struct Foo; -$0impl Bar for Foo { - fn a() {} - fn z() {} - fn b() {} -} - "#, - ) - } - - #[test] - fn not_applicable_if_empty() { - check_assist_not_applicable( - reorder_impl, - r#" -trait Bar {}; -struct Foo; -$0impl Bar for Foo {} - "#, - ) - } - - #[test] - fn reorder_impl_trait_functions() { - check_assist( - reorder_impl, - r#" -trait Bar { - fn a() {} - fn c() {} - fn b() {} - fn d() {} -} - -struct Foo; -$0impl Bar for Foo { - fn d() {} - fn b() {} - fn c() {} - fn a() {} -} - "#, - r#" -trait Bar { - fn a() {} - fn c() {} - fn b() {} - fn d() {} -} - -struct Foo; -impl Bar for Foo { - fn a() {} - fn c() {} - fn b() {} - fn d() {} -} - "#, - ) - } - - #[test] - fn reorder_impl_trait_methods_uneven_ident_lengths() { - check_assist( - reorder_impl, - r#" -trait Bar { - fn foo(&mut self) {} - fn fooo(&mut self) {} -} - -struct Foo; -impl Bar for Foo { - fn fooo(&mut self) {} - fn foo(&mut self) {$0} -}"#, - r#" -trait Bar { - fn foo(&mut self) {} - fn fooo(&mut self) {} -} - -struct Foo; -impl Bar for Foo { - fn foo(&mut self) {} - fn fooo(&mut self) {} -}"#, - ) - } -} diff --git a/crates/ide-assists/src/handlers/reorder_impl_items.rs b/crates/ide-assists/src/handlers/reorder_impl_items.rs index 87bee6c12158..2bda3a181707 100644 --- a/crates/ide-assists/src/handlers/reorder_impl_items.rs +++ b/crates/ide-assists/src/handlers/reorder_impl_items.rs @@ -47,13 +47,6 @@ pub(crate) fn reorder_impl_items(acc: &mut Assists, ctx: &AssistContext) -> Opti let items = impl_ast.assoc_item_list()?; let assoc_items = items.assoc_items().collect::>(); - // If all items are either function or macro calls, then reorder_impl assist can be used - if assoc_items.iter().all(|i| matches!(i, ast::AssocItem::Fn(_) | ast::AssocItem::MacroCall(_))) - { - cov_mark::hit!(not_applicable_if_all_functions); - return None; - } - let path = impl_ast .trait_() .and_then(|t| match t { @@ -152,23 +145,41 @@ $0impl Bar for Foo { } #[test] - fn not_applicable_if_all_functions() { - cov_mark::check!(not_applicable_if_all_functions); - check_assist_not_applicable( + fn reorder_impl_trait_functions() { + check_assist( reorder_impl_items, r#" trait Bar { fn a() {} - fn z() {} + fn c() {} fn b() {} + fn d() {} } + struct Foo; $0impl Bar for Foo { - fn a() {} - fn z() {} + fn d() {} fn b() {} + fn c() {} + fn a() {} } - "#, +"#, + r#" +trait Bar { + fn a() {} + fn c() {} + fn b() {} + fn d() {} +} + +struct Foo; +impl Bar for Foo { + fn a() {} + fn c() {} + fn b() {} + fn d() {} +} +"#, ) } diff --git a/crates/ide-assists/src/lib.rs b/crates/ide-assists/src/lib.rs index 7b2f733567c8..42bbc70b5328 100644 --- a/crates/ide-assists/src/lib.rs +++ b/crates/ide-assists/src/lib.rs @@ -169,7 +169,6 @@ mod handlers { mod remove_mut; mod remove_unused_param; mod reorder_fields; - mod reorder_impl; mod reorder_impl_items; mod replace_try_expr_with_match; mod replace_derive_with_manual_impl; @@ -257,7 +256,6 @@ mod handlers { remove_mut::remove_mut, remove_unused_param::remove_unused_param, reorder_fields::reorder_fields, - reorder_impl::reorder_impl, reorder_impl_items::reorder_impl_items, replace_try_expr_with_match::replace_try_expr_with_match, replace_derive_with_manual_impl::replace_derive_with_manual_impl, diff --git a/crates/ide-assists/src/tests/generated.rs b/crates/ide-assists/src/tests/generated.rs index b629364b880a..eed50a8562db 100644 --- a/crates/ide-assists/src/tests/generated.rs +++ b/crates/ide-assists/src/tests/generated.rs @@ -1736,41 +1736,6 @@ const test: Foo = Foo {foo: 1, bar: 0} ) } -#[test] -fn doctest_reorder_impl() { - check_doc_test( - "reorder_impl", - r#####" -trait Foo { - fn a() {} - fn b() {} - fn c() {} -} - -struct Bar; -$0impl Foo for Bar { - fn b() {} - fn c() {} - fn a() {} -} -"#####, - r#####" -trait Foo { - fn a() {} - fn b() {} - fn c() {} -} - -struct Bar; -impl Foo for Bar { - fn a() {} - fn b() {} - fn c() {} -} -"#####, - ) -} - #[test] fn doctest_reorder_impl_items() { check_doc_test(