Auto merge of rust-lang#15893 - Veykril:unsafe-impls-diagnostic, r=Veykril

bors · bors · commit 989000b8c5d3 · 2023-11-14T20:24:21.000Z
Diagnose incorrect unsafety for trait impls
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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),

Original file line number	Diff line number	Diff line change
`@@ -709,6 +709,7 @@ pub struct Impl {`
`709`	`709`	`pub target_trait: Option<Interned<TraitRef>>,`
`710`	`710`	`pub self_ty: Interned<TypeRef>,`
`711`	`711`	`pub is_negative: bool,`
	`712`	`+ pub is_unsafe: bool,`
`712`	`713`	`pub items: Box<[AssocItem]>,`
`713`	`714`	`pub ast_id: FileAstId<ast::Impl>,`
`714`	`715`	`}`