From 0df1153277cc581533c97ccf826231898eb214ed Mon Sep 17 00:00:00 2001 From: Ali Bektas Date: Tue, 13 Jun 2023 14:42:33 +0200 Subject: [PATCH 1/4] bugfix : skip doc(hidden) default members fixes #14957 --- .../src/handlers/add_missing_impl_members.rs | 39 +++++++++++++++++++ .../replace_derive_with_manual_impl.rs | 2 +- crates/ide-assists/src/utils.rs | 36 +++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_missing_impl_members.rs b/crates/ide-assists/src/handlers/add_missing_impl_members.rs index d07c637262..ecf1abc277 100644 --- a/crates/ide-assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide-assists/src/handlers/add_missing_impl_members.rs @@ -43,6 +43,7 @@ pub(crate) fn add_missing_impl_members(acc: &mut Assists, ctx: &AssistContext<'_ acc, ctx, DefaultMethods::No, + true, "add_impl_missing_members", "Implement missing members", ) @@ -87,6 +88,7 @@ pub(crate) fn add_missing_default_members( acc, ctx, DefaultMethods::Only, + true, "add_impl_default_members", "Implement default members", ) @@ -96,6 +98,7 @@ fn add_missing_impl_members_inner( acc: &mut Assists, ctx: &AssistContext<'_>, mode: DefaultMethods, + ignore_hidden: bool, assist_id: &'static str, label: &'static str, ) -> Option<()> { @@ -119,6 +122,7 @@ fn add_missing_impl_members_inner( &ctx.sema, &ide_db::traits::get_missing_assoc_items(&ctx.sema, &impl_def), mode, + ignore_hidden, ); if missing_items.is_empty() { @@ -1966,4 +1970,39 @@ impl AnotherTrait for () { "#, ); } + + #[test] + fn doc_hidden_default_impls_ignored() { + check_assist( + add_missing_default_members, + r#" +struct Foo; +trait Trait { + #[doc(hidden)] + fn func_with_default_impl() -> u32 { + 42 + } + fn another_default_impl() -> u32 { + 43 + } +} +impl Tra$0it for Foo {}"#, + r#" +struct Foo; +trait Trait { + #[doc(hidden)] + fn func_with_default_impl() -> u32 { + 42 + } + fn another_default_impl() -> u32 { + 43 + } +} +impl Trait for Foo { + $0fn another_default_impl() -> u32 { + 43 + } +}"#, + ) + } } diff --git a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs index c03bc2f41d..4fa3394233 100644 --- a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs +++ b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs @@ -172,7 +172,7 @@ fn impl_def_from_trait( ) -> Option<(ast::Impl, ast::AssocItem)> { let trait_ = trait_?; let target_scope = sema.scope(annotated_name.syntax())?; - let trait_items = filter_assoc_items(sema, &trait_.items(sema.db), DefaultMethods::No); + let trait_items = filter_assoc_items(sema, &trait_.items(sema.db), DefaultMethods::No, true); if trait_items.is_empty() { return None; } diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index 03d8553506..e4d242815e 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -3,7 +3,7 @@ use std::ops; pub(crate) use gen_trait_fn_body::gen_trait_fn_body; -use hir::{db::HirDatabase, HirDisplay, InFile, Semantics}; +use hir::{db::HirDatabase, HasAttrs as HirHasAttrs, HirDisplay, InFile, Semantics}; use ide_db::{ famous_defs::FamousDefs, path_transform::PathTransform, syntax_helpers::insert_whitespace_into_node::insert_ws_into, RootDatabase, SnippetCap, @@ -94,16 +94,44 @@ pub fn filter_assoc_items( sema: &Semantics<'_, RootDatabase>, items: &[hir::AssocItem], default_methods: DefaultMethods, + ignore_hidden: bool, ) -> Vec> { + // FIXME : How to use the closure below this so I can write sth like + // sema.source(hide(it)?)?... + + // let hide = |it: impl HasAttrs| { + // if default_methods == DefaultMethods::IgnoreHidden && it.attrs(sema.db).has_doc_hidden() { + // None; + // } + // Some(it) + // }; + return items .iter() // Note: This throws away items with no source. .copied() .filter_map(|assoc_item| { let item = match assoc_item { - hir::AssocItem::Function(it) => sema.source(it)?.map(ast::AssocItem::Fn), - hir::AssocItem::TypeAlias(it) => sema.source(it)?.map(ast::AssocItem::TypeAlias), - hir::AssocItem::Const(it) => sema.source(it)?.map(ast::AssocItem::Const), + hir::AssocItem::Function(it) => { + if ignore_hidden && it.attrs(sema.db).has_doc_hidden() { + return None; + } + sema.source(it)?.map(ast::AssocItem::Fn) + } + hir::AssocItem::TypeAlias(it) => { + if ignore_hidden && it.attrs(sema.db).has_doc_hidden() { + return None; + } + + sema.source(it)?.map(ast::AssocItem::TypeAlias) + } + hir::AssocItem::Const(it) => { + if ignore_hidden && it.attrs(sema.db).has_doc_hidden() { + return None; + } + + sema.source(it)?.map(ast::AssocItem::Const) + } }; Some(item) }) From 8a2c5d215bbef35688d47d851569843e61c1a0ba Mon Sep 17 00:00:00 2001 From: Ali Bektas Date: Fri, 23 Jun 2023 01:27:19 +0200 Subject: [PATCH 2/4] Still in need of more test cases --- .../src/handlers/add_missing_impl_members.rs | 44 ++++++++++++++--- .../replace_derive_with_manual_impl.rs | 14 +++++- crates/ide-assists/src/utils.rs | 47 ++++++------------- 3 files changed, 65 insertions(+), 40 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_missing_impl_members.rs b/crates/ide-assists/src/handlers/add_missing_impl_members.rs index ecf1abc277..818ec3de33 100644 --- a/crates/ide-assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide-assists/src/handlers/add_missing_impl_members.rs @@ -3,7 +3,10 @@ use syntax::ast::{self, make, AstNode}; use crate::{ assist_context::{AssistContext, Assists}, - utils::{add_trait_assoc_items_to_impl, filter_assoc_items, gen_trait_fn_body, DefaultMethods}, + utils::{ + add_trait_assoc_items_to_impl, filter_assoc_items, gen_trait_fn_body, DefaultMethods, + IgnoreAssocItems, + }, AssistId, AssistKind, }; @@ -43,7 +46,7 @@ pub(crate) fn add_missing_impl_members(acc: &mut Assists, ctx: &AssistContext<'_ acc, ctx, DefaultMethods::No, - true, + IgnoreAssocItems::HiddenDocAttrPresent, "add_impl_missing_members", "Implement missing members", ) @@ -88,7 +91,7 @@ pub(crate) fn add_missing_default_members( acc, ctx, DefaultMethods::Only, - true, + IgnoreAssocItems::HiddenDocAttrPresent, "add_impl_default_members", "Implement default members", ) @@ -98,7 +101,7 @@ fn add_missing_impl_members_inner( acc: &mut Assists, ctx: &AssistContext<'_>, mode: DefaultMethods, - ignore_hidden: bool, + ignore_items: IgnoreAssocItems, assist_id: &'static str, label: &'static str, ) -> Option<()> { @@ -118,11 +121,22 @@ fn add_missing_impl_members_inner( let trait_ref = impl_.trait_ref(ctx.db())?; let trait_ = trait_ref.trait_(); + let mut ign_item = ignore_items; + + if let IgnoreAssocItems::HiddenDocAttrPresent = ignore_items { + // Relax condition for local crates. + + let db = ctx.db(); + if trait_.module(db).krate().origin(db).is_local() { + ign_item = IgnoreAssocItems::No; + } + } + let missing_items = filter_assoc_items( &ctx.sema, &ide_db::traits::get_missing_assoc_items(&ctx.sema, &impl_def), mode, - ignore_hidden, + ign_item, ); if missing_items.is_empty() { @@ -1999,10 +2013,28 @@ trait Trait { } } impl Trait for Foo { - $0fn another_default_impl() -> u32 { + $0fn func_with_default_impl() -> u32 { + 42 + } + + fn another_default_impl() -> u32 { 43 } }"#, ) } + + #[test] + fn doc_hidden_default_impls_extern_crates() { + // Not applicable because Eq has a single method and this has a #[doc(hidden)] attr set. + check_assist_not_applicable( + add_missing_default_members, + r#" +//- minicore: eq +use core::cmp::Eq; +struct Foo; +impl E$0q for Foo { /* $0 */ } +"#, + ) + } } diff --git a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs index 4fa3394233..48a68b6bea 100644 --- a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs +++ b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs @@ -10,7 +10,7 @@ use crate::{ assist_context::{AssistContext, Assists, SourceChangeBuilder}, utils::{ add_trait_assoc_items_to_impl, filter_assoc_items, gen_trait_fn_body, - generate_trait_impl_text, render_snippet, Cursor, DefaultMethods, + generate_trait_impl_text, render_snippet, Cursor, DefaultMethods, IgnoreAssocItems, }, AssistId, AssistKind, }; @@ -172,7 +172,17 @@ fn impl_def_from_trait( ) -> Option<(ast::Impl, ast::AssocItem)> { let trait_ = trait_?; let target_scope = sema.scope(annotated_name.syntax())?; - let trait_items = filter_assoc_items(sema, &trait_.items(sema.db), DefaultMethods::No, true); + + // Keep assoc items of local crates even if they have #[doc(hidden)] attr. + let ignore_items = if trait_.module(sema.db).krate().origin(sema.db).is_local() { + IgnoreAssocItems::No + } else { + IgnoreAssocItems::HiddenDocAttrPresent + }; + + let trait_items = + filter_assoc_items(sema, &trait_.items(sema.db), DefaultMethods::No, ignore_items); + if trait_items.is_empty() { return None; } diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index e4d242815e..e09af5ce30 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -84,6 +84,12 @@ pub fn test_related_attribute(fn_def: &ast::Fn) -> Option { }) } +#[derive(Clone, Copy, PartialEq)] +pub enum IgnoreAssocItems { + HiddenDocAttrPresent, + No, +} + #[derive(Copy, Clone, PartialEq)] pub enum DefaultMethods { Only, @@ -94,48 +100,25 @@ pub fn filter_assoc_items( sema: &Semantics<'_, RootDatabase>, items: &[hir::AssocItem], default_methods: DefaultMethods, - ignore_hidden: bool, + ignore_items: IgnoreAssocItems, ) -> Vec> { - // FIXME : How to use the closure below this so I can write sth like - // sema.source(hide(it)?)?... - - // let hide = |it: impl HasAttrs| { - // if default_methods == DefaultMethods::IgnoreHidden && it.attrs(sema.db).has_doc_hidden() { - // None; - // } - // Some(it) - // }; - return items .iter() - // Note: This throws away items with no source. .copied() + .filter(|assoc_item| { + !(ignore_items == IgnoreAssocItems::HiddenDocAttrPresent + && assoc_item.attrs(sema.db).has_doc_hidden()) + }) .filter_map(|assoc_item| { let item = match assoc_item { - hir::AssocItem::Function(it) => { - if ignore_hidden && it.attrs(sema.db).has_doc_hidden() { - return None; - } - sema.source(it)?.map(ast::AssocItem::Fn) - } - hir::AssocItem::TypeAlias(it) => { - if ignore_hidden && it.attrs(sema.db).has_doc_hidden() { - return None; - } - - sema.source(it)?.map(ast::AssocItem::TypeAlias) - } - hir::AssocItem::Const(it) => { - if ignore_hidden && it.attrs(sema.db).has_doc_hidden() { - return None; - } - - sema.source(it)?.map(ast::AssocItem::Const) - } + hir::AssocItem::Function(it) => sema.source(it)?.map(ast::AssocItem::Fn), + hir::AssocItem::TypeAlias(it) => sema.source(it)?.map(ast::AssocItem::TypeAlias), + hir::AssocItem::Const(it) => sema.source(it)?.map(ast::AssocItem::Const), }; Some(item) }) .filter(has_def_name) + // Note: This throws away items with no source. .filter(|it| match &it.value { ast::AssocItem::Fn(def) => matches!( (default_methods, def.body()), From 18ea9245c6c1b73dabaff1fe5be286a6e39a68dc Mon Sep 17 00:00:00 2001 From: Ali Bektas Date: Fri, 23 Jun 2023 01:32:06 +0200 Subject: [PATCH 3/4] v2 --- .../src/handlers/add_missing_impl_members.rs | 115 +++++++++++++++++- crates/ide-assists/src/utils.rs | 2 +- 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_missing_impl_members.rs b/crates/ide-assists/src/handlers/add_missing_impl_members.rs index 818ec3de33..e5c18f362b 100644 --- a/crates/ide-assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide-assists/src/handlers/add_missing_impl_members.rs @@ -125,7 +125,6 @@ fn add_missing_impl_members_inner( if let IgnoreAssocItems::HiddenDocAttrPresent = ignore_items { // Relax condition for local crates. - let db = ctx.db(); if trait_.module(db).krate().origin(db).is_local() { ign_item = IgnoreAssocItems::No; @@ -1987,6 +1986,7 @@ impl AnotherTrait for () { #[test] fn doc_hidden_default_impls_ignored() { + // doc(hidden) attr is ignored trait and impl both belong to the local crate. check_assist( add_missing_default_members, r#" @@ -2025,7 +2025,7 @@ impl Trait for Foo { } #[test] - fn doc_hidden_default_impls_extern_crates() { + fn doc_hidden_default_impls_lang_crates() { // Not applicable because Eq has a single method and this has a #[doc(hidden)] attr set. check_assist_not_applicable( add_missing_default_members, @@ -2037,4 +2037,115 @@ impl E$0q for Foo { /* $0 */ } "#, ) } + + #[test] + fn doc_hidden_default_impls_lib_crates() { + check_assist( + add_missing_default_members, + r#" + //- /main.rs crate:a deps:b + struct B; + impl b::Exte$0rnTrait for B {} + //- /lib.rs crate:b new_source_root:library + pub trait ExternTrait { + #[doc(hidden)] + fn hidden_default() -> Option<()> { + todo!() + } + + fn unhidden_default() -> Option<()> { + todo!() + } + + fn unhidden_nondefault() -> Option<()>; + } + "#, + r#" + struct B; + impl b::ExternTrait for B { + $0fn unhidden_default() -> Option<()> { + todo!() + } + } + "#, + ) + } + + #[test] + fn doc_hidden_default_impls_local_crates() { + check_assist( + add_missing_default_members, + r#" +trait LocalTrait { + #[doc(hidden)] + fn no_skip_default() -> Option<()> { + todo!() + } + fn no_skip_default_2() -> Option<()> { + todo!() + } +} + +struct B; +impl Loc$0alTrait for B {} + "#, + r#" +trait LocalTrait { + #[doc(hidden)] + fn no_skip_default() -> Option<()> { + todo!() + } + fn no_skip_default_2() -> Option<()> { + todo!() + } +} + +struct B; +impl LocalTrait for B { + $0fn no_skip_default() -> Option<()> { + todo!() + } + + fn no_skip_default_2() -> Option<()> { + todo!() + } +} + "#, + ) + } + + #[test] + fn doc_hidden_default_impls_workspace_crates() { + check_assist( + add_missing_default_members, + r#" +//- /lib.rs crate:b new_source_root:local +trait LocalTrait { + #[doc(hidden)] + fn no_skip_default() -> Option<()> { + todo!() + } + fn no_skip_default_2() -> Option<()> { + todo!() + } +} + +//- /main.rs crate:a deps:b +struct B; +impl b::Loc$0alTrait for B {} + "#, + r#" +struct B; +impl b::LocalTrait for B { + $0fn no_skip_default() -> Option<()> { + todo!() + } + + fn no_skip_default_2() -> Option<()> { + todo!() + } +} + "#, + ) + } } diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index e09af5ce30..aab4f15f14 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -109,6 +109,7 @@ pub fn filter_assoc_items( !(ignore_items == IgnoreAssocItems::HiddenDocAttrPresent && assoc_item.attrs(sema.db).has_doc_hidden()) }) + // Note: This throws away items with no source. .filter_map(|assoc_item| { let item = match assoc_item { hir::AssocItem::Function(it) => sema.source(it)?.map(ast::AssocItem::Fn), @@ -118,7 +119,6 @@ pub fn filter_assoc_items( Some(item) }) .filter(has_def_name) - // Note: This throws away items with no source. .filter(|it| match &it.value { ast::AssocItem::Fn(def) => matches!( (default_methods, def.body()), From 915ddb05fa78dfe1cfcda2b68c1013f218d729d4 Mon Sep 17 00:00:00 2001 From: Ali Bektas Date: Fri, 7 Jul 2023 14:15:15 +0200 Subject: [PATCH 4/4] HiddenDocAttr becomes DocHiddenAttr --- crates/ide-assists/src/handlers/add_missing_impl_members.rs | 6 +++--- .../src/handlers/replace_derive_with_manual_impl.rs | 2 +- crates/ide-assists/src/utils.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ide-assists/src/handlers/add_missing_impl_members.rs b/crates/ide-assists/src/handlers/add_missing_impl_members.rs index e5c18f362b..6aca716bb6 100644 --- a/crates/ide-assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide-assists/src/handlers/add_missing_impl_members.rs @@ -46,7 +46,7 @@ pub(crate) fn add_missing_impl_members(acc: &mut Assists, ctx: &AssistContext<'_ acc, ctx, DefaultMethods::No, - IgnoreAssocItems::HiddenDocAttrPresent, + IgnoreAssocItems::DocHiddenAttrPresent, "add_impl_missing_members", "Implement missing members", ) @@ -91,7 +91,7 @@ pub(crate) fn add_missing_default_members( acc, ctx, DefaultMethods::Only, - IgnoreAssocItems::HiddenDocAttrPresent, + IgnoreAssocItems::DocHiddenAttrPresent, "add_impl_default_members", "Implement default members", ) @@ -123,7 +123,7 @@ fn add_missing_impl_members_inner( let mut ign_item = ignore_items; - if let IgnoreAssocItems::HiddenDocAttrPresent = ignore_items { + if let IgnoreAssocItems::DocHiddenAttrPresent = ignore_items { // Relax condition for local crates. let db = ctx.db(); if trait_.module(db).krate().origin(db).is_local() { diff --git a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs index 48a68b6bea..ac45581b7b 100644 --- a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs +++ b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs @@ -177,7 +177,7 @@ fn impl_def_from_trait( let ignore_items = if trait_.module(sema.db).krate().origin(sema.db).is_local() { IgnoreAssocItems::No } else { - IgnoreAssocItems::HiddenDocAttrPresent + IgnoreAssocItems::DocHiddenAttrPresent }; let trait_items = diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index aab4f15f14..a262570d94 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -86,7 +86,7 @@ pub fn test_related_attribute(fn_def: &ast::Fn) -> Option { #[derive(Clone, Copy, PartialEq)] pub enum IgnoreAssocItems { - HiddenDocAttrPresent, + DocHiddenAttrPresent, No, } @@ -106,7 +106,7 @@ pub fn filter_assoc_items( .iter() .copied() .filter(|assoc_item| { - !(ignore_items == IgnoreAssocItems::HiddenDocAttrPresent + !(ignore_items == IgnoreAssocItems::DocHiddenAttrPresent && assoc_item.attrs(sema.db).has_doc_hidden()) }) // Note: This throws away items with no source.