Mirrors/rust-analyzer
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-analyzer synced 2024-12-26 21:13:37 +00:00
Code Issues Projects Releases Packages Wiki Activity

Diagnose incorrect unsafety for trait impls

Browse source
This commit is contained in:
Lukas Wirth 2023-11-14 20:02:23 +01:00
parent d5faad1dae
commit e21d21a8fb
8 changed files with 204 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
crates/hir-def/src/data.rs
View file

@ -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(),
)
}

1
crates/hir-def/src/item_tree.rs
View file

@ -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>,
}

4
crates/hir-def/src/item_tree/lower.rs
View file

@ -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)))
}

14
crates/hir-def/src/item_tree/pretty.rs
View file

@ -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, " ");

11
crates/hir/src/diagnostics.rs
View file

@ -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,
}

70
crates/hir/src/lib.rs
View file

@ -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 {

115
crates/ide-diagnostics/src/handlers/trait_impl_incorrect_safety.rs Normal file
View file

@ -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
"#,
);
}
}

2
crates/ide-diagnostics/src/lib.rs
View file

@ -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),