diff options
| -rw-r--r-- | crates/hir-def/src/data.rs | 11 | ||||
| -rw-r--r-- | crates/hir-def/src/item_tree.rs | 1 | ||||
| -rw-r--r-- | crates/hir-def/src/item_tree/lower.rs | 4 | ||||
| -rw-r--r-- | crates/hir-def/src/item_tree/pretty.rs | 14 | ||||
| -rw-r--r-- | crates/hir/src/diagnostics.rs | 11 | ||||
| -rw-r--r-- | crates/hir/src/lib.rs | 70 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/handlers/trait_impl_incorrect_safety.rs | 115 | ||||
| -rw-r--r-- | crates/ide-diagnostics/src/lib.rs | 2 |
8 files changed, 204 insertions, 24 deletions
diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs index 93fd741aff6..72ccc17486f 100644 --- a/crates/hir-def/src/data.rs +++ b/crates/hir-def/src/data.rs @@ -327,6 +327,7 @@ pub struct ImplData { pub self_ty: Interned<TypeRef>, pub items: Vec<AssocItemId>, pub is_negative: bool, + pub is_unsafe: bool, // box it as the vec is usually empty anyways pub attribute_calls: Option<Box<Vec<(AstId<ast::Item>, MacroCallId)>>>, } @@ -348,6 +349,7 @@ impl ImplData { let target_trait = impl_def.target_trait.clone(); let self_ty = impl_def.self_ty.clone(); let is_negative = impl_def.is_negative; + let is_unsafe = impl_def.is_unsafe; let mut collector = AssocItemCollector::new(db, module_id, tree_id.file_id(), ItemContainerId::ImplId(id)); @@ -357,7 +359,14 @@ impl ImplData { let items = items.into_iter().map(|(_, item)| item).collect(); ( - Arc::new(ImplData { target_trait, self_ty, items, is_negative, attribute_calls }), + Arc::new(ImplData { + target_trait, + self_ty, + items, + is_negative, + is_unsafe, + attribute_calls, + }), diagnostics.into(), ) } diff --git a/crates/hir-def/src/item_tree.rs b/crates/hir-def/src/item_tree.rs index 3c4f21d5c69..70b96b25739 100644 --- a/crates/hir-def/src/item_tree.rs +++ b/crates/hir-def/src/item_tree.rs @@ -709,6 +709,7 @@ pub struct Impl { pub target_trait: Option<Interned<TraitRef>>, pub self_ty: Interned<TypeRef>, pub is_negative: bool, + pub is_unsafe: bool, pub items: Box<[AssocItem]>, pub ast_id: FileAstId<ast::Impl>, } diff --git a/crates/hir-def/src/item_tree/lower.rs b/crates/hir-def/src/item_tree/lower.rs index 30b649d2f3a..c898eb5f921 100644 --- a/crates/hir-def/src/item_tree/lower.rs +++ b/crates/hir-def/src/item_tree/lower.rs @@ -492,6 +492,7 @@ impl<'a> Ctx<'a> { let target_trait = impl_def.trait_().and_then(|tr| self.lower_trait_ref(&tr)); let self_ty = self.lower_type_ref(&impl_def.self_ty()?); let is_negative = impl_def.excl_token().is_some(); + let is_unsafe = impl_def.unsafe_token().is_some(); // We cannot use `assoc_items()` here as that does not include macro calls. let items = impl_def @@ -506,7 +507,8 @@ impl<'a> Ctx<'a> { }) .collect(); let ast_id = self.source_ast_id_map.ast_id(impl_def); - let res = Impl { generic_params, target_trait, self_ty, is_negative, items, ast_id }; + let res = + Impl { generic_params, target_trait, self_ty, is_negative, is_unsafe, items, ast_id }; Some(id(self.data().impls.alloc(res))) } diff --git a/crates/hir-def/src/item_tree/pretty.rs b/crates/hir-def/src/item_tree/pretty.rs index 5036c2b8829..ca3785bf28d 100644 --- a/crates/hir-def/src/item_tree/pretty.rs +++ b/crates/hir-def/src/item_tree/pretty.rs @@ -388,8 +388,18 @@ impl Printer<'_> { wln!(self); } ModItem::Impl(it) => { - let Impl { target_trait, self_ty, is_negative, items, generic_params, ast_id: _ } = - &self.tree[it]; + let Impl { + target_trait, + self_ty, + is_negative, + is_unsafe, + items, + generic_params, + ast_id: _, + } = &self.tree[it]; + if *is_unsafe { + w!(self, "unsafe"); + } w!(self, "impl"); self.print_generic_params(generic_params); w!(self, " "); diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 3a456226755..dd5c9b3535a 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -38,7 +38,6 @@ diagnostics![ IncorrectCase, InvalidDeriveTarget, IncoherentImpl, - TraitImplOrphan, MacroDefError, MacroError, MacroExpansionParseError, @@ -54,6 +53,8 @@ diagnostics![ PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, + TraitImplIncorrectSafety, + TraitImplOrphan, TypedHole, TypeMismatch, UndeclaredLabel, @@ -293,3 +294,11 @@ pub struct TraitImplOrphan { pub file_id: HirFileId, pub impl_: AstPtr<ast::Impl>, } + +// FIXME: Split this off into the corresponding 4 rustc errors +#[derive(Debug, PartialEq, Eq)] +pub struct TraitImplIncorrectSafety { + pub file_id: HirFileId, + pub impl_: AstPtr<ast::Impl>, + pub should_be_safe: bool, +} diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 8d82e88da9d..066d7e635a2 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -53,10 +53,10 @@ use hir_def::{ resolver::{HasResolver, Resolver}, src::HasSource as _, AssocItemId, AssocItemLoc, AttrDefId, ConstId, ConstParamId, CrateRootModuleId, DefWithBodyId, - EnumId, EnumVariantId, ExternCrateId, FunctionId, GenericDefId, HasModule, ImplId, - InTypeConstId, ItemContainerId, LifetimeParamId, LocalEnumVariantId, LocalFieldId, Lookup, - MacroExpander, MacroId, ModuleId, StaticId, StructId, TraitAliasId, TraitId, TypeAliasId, - TypeOrConstParamId, TypeParamId, UnionId, + EnumId, EnumVariantId, ExternCrateId, FunctionId, GenericDefId, GenericParamId, HasModule, + ImplId, InTypeConstId, ItemContainerId, LifetimeParamId, LocalEnumVariantId, LocalFieldId, + Lookup, MacroExpander, MacroId, ModuleId, StaticId, StructId, TraitAliasId, TraitId, + TypeAliasId, TypeOrConstParamId, TypeParamId, UnionId, }; use hir_expand::{name::name, MacroCallKind}; use hir_ty::{ @@ -89,17 +89,7 @@ use crate::db::{DefDatabase, HirDatabase}; pub use crate::{ attrs::{resolve_doc_path_on, HasAttrs}, - diagnostics::{ - AnyDiagnostic, BreakOutsideOfLoop, CaseType, ExpectedFunction, InactiveCode, - IncoherentImpl, IncorrectCase, InvalidDeriveTarget, MacroDefError, MacroError, - MacroExpansionParseError, MalformedDerive, MismatchedArgCount, - MismatchedTupleStructPatArgCount, MissingFields, MissingMatchArms, MissingUnsafe, - MovedOutOfRef, NeedMut, NoSuchField, PrivateAssocItem, PrivateField, - ReplaceFilterMapNextWithFindMap, TraitImplOrphan, TypeMismatch, TypedHole, UndeclaredLabel, - UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, UnresolvedField, - UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, UnresolvedModule, - UnresolvedProcMacro, UnusedMut, UnusedVariable, - }, + diagnostics::*, has_source::HasSource, semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo, VisibleTraits}, }; @@ -613,22 +603,64 @@ impl Module { // FIXME: Once we diagnose the inputs to builtin derives, we should at least extract those diagnostics somehow continue; } + let ast_id_map = db.ast_id_map(file_id); for diag in db.impl_data_with_diagnostics(impl_def.id).1.iter() { emit_def_diagnostic(db, acc, diag); } if inherent_impls.invalid_impls().contains(&impl_def.id) { - let ast_id_map = db.ast_id_map(file_id); - acc.push(IncoherentImpl { impl_: ast_id_map.get(node.ast_id()), file_id }.into()) } if !impl_def.check_orphan_rules(db) { - let ast_id_map = db.ast_id_map(file_id); acc.push(TraitImplOrphan { impl_: ast_id_map.get(node.ast_id()), file_id }.into()) } + let trait_ = impl_def.trait_(db); + let trait_is_unsafe = trait_.map_or(false, |t| t.is_unsafe(db)); + let impl_is_negative = impl_def.is_negative(db); + let impl_is_unsafe = impl_def.is_unsafe(db); + + let drop_maybe_dangle = (|| { + // FIXME: This can be simplified a lot by exposing hir-ty's utils.rs::Generics helper + let trait_ = trait_?; + let drop_trait = db.lang_item(self.krate().into(), LangItem::Drop)?.as_trait()?; + if drop_trait != trait_.into() { + return None; + } + let parent = impl_def.id.into(); + let generic_params = db.generic_params(parent); + let lifetime_params = generic_params.lifetimes.iter().map(|(local_id, _)| { + GenericParamId::LifetimeParamId(LifetimeParamId { parent, local_id }) + }); + let type_params = generic_params + .iter() + .filter(|(_, it)| it.type_param().is_some()) + .map(|(local_id, _)| { + GenericParamId::TypeParamId(TypeParamId::from_unchecked( + TypeOrConstParamId { parent, local_id }, + )) + }); + let res = type_params + .chain(lifetime_params) + .any(|p| db.attrs(AttrDefId::GenericParamId(p)).by_key("may_dangle").exists()); + Some(res) + })() + .unwrap_or(false); + + match (impl_is_unsafe, trait_is_unsafe, impl_is_negative, drop_maybe_dangle) { + // unsafe negative impl + (true, _, true, _) | + // unsafe impl for safe trait + (true, false, _, false) => acc.push(TraitImplIncorrectSafety { impl_: ast_id_map.get(node.ast_id()), file_id, should_be_safe: true }.into()), + // safe impl for unsafe trait + (false, true, false, _) | + // safe impl of dangling drop + (false, false, _, true) => acc.push(TraitImplIncorrectSafety { impl_: ast_id_map.get(node.ast_id()), file_id, should_be_safe: false }.into()), + _ => (), + }; + for item in impl_def.items(db) { let def: DefWithBody = match item { AssocItem::Function(it) => it.into(), @@ -3404,7 +3436,7 @@ impl Impl { } pub fn is_unsafe(self, db: &dyn HirDatabase) -> bool { - db.impl_data(self.id).is_unique() + db.impl_data(self.id).is_unsafe } pub fn module(self, db: &dyn HirDatabase) -> Module { diff --git a/crates/ide-diagnostics/src/handlers/trait_impl_incorrect_safety.rs b/crates/ide-diagnostics/src/handlers/trait_impl_incorrect_safety.rs new file mode 100644 index 00000000000..f28298de7d8 --- /dev/null +++ b/crates/ide-diagnostics/src/handlers/trait_impl_incorrect_safety.rs @@ -0,0 +1,115 @@ +use hir::InFile; + +use crate::{Diagnostic, DiagnosticCode, DiagnosticsContext, Severity}; + +// Diagnostic: trait-impl-incorrect-safety +// +// Diagnoses incorrect safety annotations of trait impls. +pub(crate) fn trait_impl_incorrect_safety( + ctx: &DiagnosticsContext<'_>, + d: &hir::TraitImplIncorrectSafety, +) -> Diagnostic { + Diagnostic::new_with_syntax_node_ptr( + ctx, + 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()), + ) +} + +#[cfg(test)] +mod tests { + use crate::tests::check_diagnostics; + + #[test] + fn simple() { + check_diagnostics( + r#" +trait Safe {} +unsafe trait Unsafe {} + + impl Safe for () {} + + impl Unsafe for () {} +//^^^^^^^^^^^^^^^^^^^^^ error: impl for unsafe trait needs to be unsafe + + unsafe impl Safe for () {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait + + unsafe impl Unsafe for () {} +"#, + ); + } + + #[test] + fn drop_may_dangle() { + check_diagnostics( + r#" +#[lang = "drop"] +trait Drop {} +struct S<T>; +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 + + unsafe impl<T> Drop for S<T> {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 + + unsafe impl<'l> Drop for L<'l> {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait + + unsafe impl<#[may_dangle] 'l> Drop for L<'l> {} +"#, + ); + } + + #[test] + fn negative() { + check_diagnostics( + r#" +trait Trait {} + + impl !Trait for () {} + + unsafe impl !Trait for () {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait + +unsafe trait UnsafeTrait {} + + impl !UnsafeTrait for () {} + + unsafe impl !UnsafeTrait for () {} +//^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait + +"#, + ); + } + + #[test] + fn inherent() { + check_diagnostics( + r#" +struct S; + + impl S {} + + unsafe impl S {} +//^^^^^^^^^^^^^^^^ error: unsafe impl for safe trait +"#, + ); + } +} diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index 3226dd29d21..99921c1107d 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -45,6 +45,7 @@ mod handlers { pub(crate) mod private_field; 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 typed_hole; pub(crate) mod type_mismatch; pub(crate) mod unimplemented_builtin_macro; @@ -360,6 +361,7 @@ pub fn diagnostics( 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::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), |