Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Diagnose missing assoc items in trait impls #15895

Merged
merged 1 commit into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/hir/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ diagnostics![
PrivateField,
ReplaceFilterMapNextWithFindMap,
TraitImplIncorrectSafety,
TraitImplMissingAssocItems,
TraitImplOrphan,
TypedHole,
TypeMismatch,
Expand Down Expand Up @@ -302,3 +303,10 @@ pub struct TraitImplIncorrectSafety {
pub impl_: AstPtr<ast::Impl>,
pub should_be_safe: bool,
}

#[derive(Debug, PartialEq, Eq)]
pub struct TraitImplMissingAssocItems {
pub file_id: HirFileId,
pub impl_: AstPtr<ast::Impl>,
pub missing: Vec<(Name, AssocItem)>,
}
50 changes: 47 additions & 3 deletions crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub mod symbols;

mod display;

use std::{iter, ops::ControlFlow};
use std::{iter, mem::discriminant, ops::ControlFlow};

use arrayvec::ArrayVec;
use base_db::{CrateDisplayName, CrateId, CrateOrigin, Edition, FileId, ProcMacroKind};
Expand Down Expand Up @@ -593,6 +593,7 @@ impl Module {

let inherent_impls = db.inherent_impls_in_crate(self.id.krate());

let mut impl_assoc_items_scratch = vec![];
for impl_def in self.impl_defs(db) {
let loc = impl_def.id.lookup(db.upcast());
let tree = loc.id.item_tree(db.upcast());
Expand Down Expand Up @@ -661,8 +662,51 @@ impl Module {
_ => (),
};

for item in impl_def.items(db) {
let def: DefWithBody = match item {
if let Some(trait_) = trait_ {
let items = &db.trait_data(trait_.into()).items;
let required_items = items.iter().filter(|&(_, assoc)| match *assoc {
AssocItemId::FunctionId(it) => !db.function_data(it).has_body(),
AssocItemId::ConstId(_) => true,
AssocItemId::TypeAliasId(it) => db.type_alias_data(it).type_ref.is_none(),
});
impl_assoc_items_scratch.extend(db.impl_data(impl_def.id).items.iter().map(
|&item| {
(
item,
match item {
AssocItemId::FunctionId(it) => db.function_data(it).name.clone(),
AssocItemId::ConstId(it) => {
db.const_data(it).name.as_ref().unwrap().clone()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I forgot to replace the unwrap here. @Young-Flash since you are looking into #15909 already, do you mind changing this to ? and make this map a filter_map?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

}
AssocItemId::TypeAliasId(it) => db.type_alias_data(it).name.clone(),
},
)
},
));

let missing: Vec<_> = required_items
.filter(|(name, id)| {
!impl_assoc_items_scratch.iter().any(|(impl_item, impl_name)| {
discriminant(impl_item) == discriminant(id) && impl_name == name
})
})
.map(|(name, item)| (name.clone(), AssocItem::from(*item)))
.collect();
if !missing.is_empty() {
acc.push(
TraitImplMissingAssocItems {
impl_: ast_id_map.get(node.ast_id()),
file_id,
missing,
}
.into(),
)
}
impl_assoc_items_scratch.clear();
}

for &item in &db.impl_data(impl_def.id).items {
let def: DefWithBody = match AssocItem::from(item) {
AssocItem::Function(it) => it.into(),
AssocItem::Const(it) => it.into(),
AssocItem::TypeAlias(_) => continue,
Expand Down
40 changes: 27 additions & 13 deletions crates/ide-diagnostics/src/handlers/trait_impl_incorrect_safety.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use hir::InFile;
use syntax::ast;

use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext, Severity};
use crate::{adjusted_display_range, Diagnostic, DiagnosticCode, DiagnosticsContext, Severity};

// Diagnostic: trait-impl-incorrect-safety
//
Expand All @@ -9,15 +10,28 @@ pub(crate) fn trait_impl_incorrect_safety(
ctx: &DiagnosticsContext<'_>,
d: &hir::TraitImplIncorrectSafety,
) -> Diagnostic {
Diagnostic::new_with_syntax_node_ptr(
ctx,
Diagnostic::new(
DiagnosticCode::Ra("trait-impl-incorrect-safety", Severity::Error),
if d.should_be_safe {
"unsafe impl for safe trait"
} else {
"impl for unsafe trait needs to be unsafe"
},
InFile::new(d.file_id, d.impl_.clone().into()),
adjusted_display_range::<ast::Impl>(
ctx,
InFile { file_id: d.file_id, value: d.impl_.syntax_node_ptr() },
&|impl_| {
if d.should_be_safe {
Some(match (impl_.unsafe_token(), impl_.impl_token()) {
(None, None) => return None,
(None, Some(t)) | (Some(t), None) => t.text_range(),
(Some(t1), Some(t2)) => t1.text_range().cover(t2.text_range()),
})
} else {
impl_.impl_token().map(|t| t.text_range())
}
},
),
)
}

Expand All @@ -35,10 +49,10 @@ unsafe trait Unsafe {}
impl Safe for () {}

impl Unsafe for () {}
//^^^^^^^^^^^^^^^^^^^^^ error: impl for unsafe trait needs to be unsafe
//^^^^ error: impl for unsafe trait needs to be unsafe

unsafe impl Safe for () {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait
//^^^^^^^^^^^ error: unsafe impl for safe trait

unsafe impl Unsafe for () {}
"#,
Expand All @@ -57,20 +71,20 @@ struct L<'l>;
impl<T> Drop for S<T> {}

impl<#[may_dangle] T> Drop for S<T> {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: impl for unsafe trait needs to be unsafe
//^^^^ error: impl for unsafe trait needs to be unsafe

unsafe impl<T> Drop for S<T> {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait
//^^^^^^^^^^^ error: unsafe impl for safe trait

unsafe impl<#[may_dangle] T> Drop for S<T> {}

impl<'l> Drop for L<'l> {}

impl<#[may_dangle] 'l> Drop for L<'l> {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: impl for unsafe trait needs to be unsafe
//^^^^ error: impl for unsafe trait needs to be unsafe

unsafe impl<'l> Drop for L<'l> {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait
//^^^^^^^^^^^ error: unsafe impl for safe trait

unsafe impl<#[may_dangle] 'l> Drop for L<'l> {}
"#,
Expand All @@ -86,14 +100,14 @@ trait Trait {}
impl !Trait for () {}

unsafe impl !Trait for () {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait
//^^^^^^^^^^^ error: unsafe impl for safe trait

unsafe trait UnsafeTrait {}

impl !UnsafeTrait for () {}

unsafe impl !UnsafeTrait for () {}
//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait
//^^^^^^^^^^^ error: unsafe impl for safe trait

"#,
);
Expand All @@ -108,7 +122,7 @@ struct S;
impl S {}

unsafe impl S {}
//^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait
//^^^^^^^^^^^ error: unsafe impl for safe trait
"#,
);
}
Expand Down
102 changes: 102 additions & 0 deletions crates/ide-diagnostics/src/handlers/trait_impl_missing_assoc_item.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
use hir::InFile;
use itertools::Itertools;
use syntax::{ast, AstNode};

use crate::{adjusted_display_range, Diagnostic, DiagnosticCode, DiagnosticsContext};

// Diagnostic: trait-impl-missing-assoc_item
//
// Diagnoses missing trait items in a trait impl.
pub(crate) fn trait_impl_missing_assoc_item(
ctx: &DiagnosticsContext<'_>,
d: &hir::TraitImplMissingAssocItems,
) -> Diagnostic {
let missing = d.missing.iter().format_with(", ", |(name, item), f| {
f(&match *item {
hir::AssocItem::Function(_) => "`fn ",
hir::AssocItem::Const(_) => "`const ",
hir::AssocItem::TypeAlias(_) => "`type ",
})?;
f(&name.display(ctx.sema.db))?;
f(&"`")
});
Diagnostic::new(
DiagnosticCode::RustcHardError("E0046"),
format!("not all trait items implemented, missing: {missing}"),
adjusted_display_range::<ast::Impl>(
ctx,
InFile { file_id: d.file_id, value: d.impl_.syntax_node_ptr() },
&|impl_| impl_.trait_().map(|t| t.syntax().text_range()),
),
)
}

#[cfg(test)]
mod tests {
use crate::tests::check_diagnostics;

#[test]
fn simple() {
check_diagnostics(
r#"
trait Trait {
const C: ();
type T;
fn f();
}

impl Trait for () {
const C: () = ();
type T = ();
fn f() {}
}

impl Trait for () {
//^^^^^ error: not all trait items implemented, missing: `const C`
type T = ();
fn f() {}
}

impl Trait for () {
//^^^^^ error: not all trait items implemented, missing: `const C`, `type T`, `fn f`
}

"#,
);
}

#[test]
fn default() {
check_diagnostics(
r#"
trait Trait {
const C: ();
type T = ();
fn f() {}
}

impl Trait for () {
const C: () = ();
type T = ();
fn f() {}
}

impl Trait for () {
//^^^^^ error: not all trait items implemented, missing: `const C`
type T = ();
fn f() {}
}

impl Trait for () {
//^^^^^ error: not all trait items implemented, missing: `const C`
type T = ();
}

impl Trait for () {
//^^^^^ error: not all trait items implemented, missing: `const C`
}

"#,
);
}
}
2 changes: 2 additions & 0 deletions crates/ide-diagnostics/src/handlers/type_mismatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ struct Foo;
struct Bar;
impl core::ops::Deref for Foo {
type Target = Bar;
fn deref(&self) -> &Self::Target { loop {} }
}

fn main() {
Expand All @@ -290,6 +291,7 @@ struct Foo;
struct Bar;
impl core::ops::Deref for Foo {
type Target = Bar;
fn deref(&self) -> &Self::Target { loop {} }
}

fn main() {
Expand Down
4 changes: 3 additions & 1 deletion crates/ide-diagnostics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod handlers {
pub(crate) mod replace_filter_map_next_with_find_map;
pub(crate) mod trait_impl_orphan;
pub(crate) mod trait_impl_incorrect_safety;
pub(crate) mod trait_impl_missing_assoc_item;
pub(crate) mod typed_hole;
pub(crate) mod type_mismatch;
pub(crate) mod unimplemented_builtin_macro;
Expand Down Expand Up @@ -360,8 +361,9 @@ pub fn diagnostics(
AnyDiagnostic::PrivateAssocItem(d) => handlers::private_assoc_item::private_assoc_item(&ctx, &d),
AnyDiagnostic::PrivateField(d) => handlers::private_field::private_field(&ctx, &d),
AnyDiagnostic::ReplaceFilterMapNextWithFindMap(d) => handlers::replace_filter_map_next_with_find_map::replace_filter_map_next_with_find_map(&ctx, &d),
AnyDiagnostic::TraitImplOrphan(d) => handlers::trait_impl_orphan::trait_impl_orphan(&ctx, &d),
AnyDiagnostic::TraitImplIncorrectSafety(d) => handlers::trait_impl_incorrect_safety::trait_impl_incorrect_safety(&ctx, &d),
AnyDiagnostic::TraitImplMissingAssocItems(d) => handlers::trait_impl_missing_assoc_item::trait_impl_missing_assoc_item(&ctx, &d),
AnyDiagnostic::TraitImplOrphan(d) => handlers::trait_impl_orphan::trait_impl_orphan(&ctx, &d),
AnyDiagnostic::TypedHole(d) => handlers::typed_hole::typed_hole(&ctx, &d),
AnyDiagnostic::TypeMismatch(d) => handlers::type_mismatch::type_mismatch(&ctx, &d),
AnyDiagnostic::UndeclaredLabel(d) => handlers::undeclared_label::undeclared_label(&ctx, &d),
Expand Down
3 changes: 2 additions & 1 deletion crates/ide-diagnostics/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ fn check_nth_fix(nth: usize, ra_fixture_before: &str, ra_fixture_after: &str) {
super::diagnostics(&db, &conf, &AssistResolveStrategy::All, file_position.file_id)
.pop()
.expect("no diagnostics");
let fix = &diagnostic.fixes.expect("diagnostic misses fixes")[nth];
let fix =
&diagnostic.fixes.expect(&format!("{:?} diagnostic misses fixes", diagnostic.code))[nth];
let actual = {
let source_change = fix.source_change.as_ref().unwrap();
let file_id = *source_change.source_file_edits.keys().next().unwrap();
Expand Down