From 5d8b4c40eb46c44c3859cc00dc19f1b05de1176f Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 7 Apr 2022 18:33:03 +0200 Subject: [PATCH] Determine function unsafety semantically --- crates/hir/src/display.rs | 10 +-- crates/hir/src/lib.rs | 12 ++-- crates/hir_def/src/data.rs | 16 ++--- crates/hir_def/src/item_tree.rs | 8 +-- crates/hir_def/src/item_tree/lower.rs | 69 ++----------------- crates/hir_def/src/item_tree/tests.rs | 1 - crates/hir_ty/src/diagnostics/unsafe_check.rs | 10 +-- crates/hir_ty/src/infer.rs | 2 +- crates/hir_ty/src/lib.rs | 2 +- crates/hir_ty/src/utils.rs | 69 ++++++++++++++++++- .../ide/src/syntax_highlighting/highlight.rs | 4 +- crates/ide_completion/src/render/function.rs | 2 +- 12 files changed, 107 insertions(+), 98 deletions(-) diff --git a/crates/hir/src/display.rs b/crates/hir/src/display.rs index c1efab0918..6d5ddad356 100644 --- a/crates/hir/src/display.rs +++ b/crates/hir/src/display.rs @@ -26,16 +26,16 @@ impl HirDisplay for Function { fn hir_fmt(&self, f: &mut HirFormatter) -> Result<(), HirDisplayError> { let data = f.db.function_data(self.id); write_visibility(self.module(f.db).id, self.visibility(f.db), f)?; - if data.is_default() { + if data.has_default_kw() { f.write_str("default ")?; } - if data.is_const() { + if data.has_const_kw() { f.write_str("const ")?; } - if data.is_async() { + if data.has_async_kw() { f.write_str("async ")?; } - if data.is_unsafe() { + if self.is_unsafe_to_call(f.db) { f.write_str("unsafe ")?; } if let Some(abi) = &data.abi { @@ -96,7 +96,7 @@ impl HirDisplay for Function { // `FunctionData::ret_type` will be `::core::future::Future` for async fns. // Use ugly pattern match to strip the Future trait. // Better way? - let ret_type = if !data.is_async() { + let ret_type = if !data.has_async_kw() { &data.ret_type } else { match &*data.ret_type { diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 8b99662685..8bab7c1f3e 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -1421,16 +1421,16 @@ impl Function { .collect() } - pub fn is_unsafe(self, db: &dyn HirDatabase) -> bool { - db.function_data(self.id).is_unsafe() - } - pub fn is_const(self, db: &dyn HirDatabase) -> bool { - db.function_data(self.id).is_const() + db.function_data(self.id).has_const_kw() } pub fn is_async(self, db: &dyn HirDatabase) -> bool { - db.function_data(self.id).is_async() + db.function_data(self.id).has_async_kw() + } + + pub fn is_unsafe_to_call(self, db: &dyn HirDatabase) -> bool { + hir_ty::is_fn_unsafe_to_call(db, self.id) } /// Whether this function declaration has a definition. diff --git a/crates/hir_def/src/data.rs b/crates/hir_def/src/data.rs index 4cce419a7f..3ef1029724 100644 --- a/crates/hir_def/src/data.rs +++ b/crates/hir_def/src/data.rs @@ -110,20 +110,20 @@ impl FunctionData { self.flags.contains(FnFlags::HAS_SELF_PARAM) } - pub fn is_default(&self) -> bool { - self.flags.contains(FnFlags::IS_DEFAULT) + pub fn has_default_kw(&self) -> bool { + self.flags.contains(FnFlags::HAS_DEFAULT_KW) } - pub fn is_const(&self) -> bool { - self.flags.contains(FnFlags::IS_CONST) + pub fn has_const_kw(&self) -> bool { + self.flags.contains(FnFlags::HAS_CONST_KW) } - pub fn is_async(&self) -> bool { - self.flags.contains(FnFlags::IS_ASYNC) + pub fn has_async_kw(&self) -> bool { + self.flags.contains(FnFlags::HAS_ASYNC_KW) } - pub fn is_unsafe(&self) -> bool { - self.flags.contains(FnFlags::IS_UNSAFE) + pub fn has_unsafe_kw(&self) -> bool { + self.flags.contains(FnFlags::HAS_UNSAFE_KW) } pub fn is_varargs(&self) -> bool { diff --git a/crates/hir_def/src/item_tree.rs b/crates/hir_def/src/item_tree.rs index 23a0101039..375587ee93 100644 --- a/crates/hir_def/src/item_tree.rs +++ b/crates/hir_def/src/item_tree.rs @@ -606,10 +606,10 @@ bitflags::bitflags! { pub(crate) struct FnFlags: u8 { const HAS_SELF_PARAM = 1 << 0; const HAS_BODY = 1 << 1; - const IS_DEFAULT = 1 << 2; - const IS_CONST = 1 << 3; - const IS_ASYNC = 1 << 4; - const IS_UNSAFE = 1 << 5; + const HAS_DEFAULT_KW = 1 << 2; + const HAS_CONST_KW = 1 << 3; + const HAS_ASYNC_KW = 1 << 4; + const HAS_UNSAFE_KW = 1 << 5; const IS_VARARGS = 1 << 6; } } diff --git a/crates/hir_def/src/item_tree/lower.rs b/crates/hir_def/src/item_tree/lower.rs index dd7a8a5f24..cdae0d0803 100644 --- a/crates/hir_def/src/item_tree/lower.rs +++ b/crates/hir_def/src/item_tree/lower.rs @@ -2,7 +2,7 @@ use std::{collections::hash_map::Entry, mem, sync::Arc}; -use hir_expand::{ast_id_map::AstIdMap, hygiene::Hygiene, name::known, HirFileId}; +use hir_expand::{ast_id_map::AstIdMap, hygiene::Hygiene, HirFileId}; use syntax::ast::{self, HasModuleItem}; use crate::{ @@ -343,16 +343,16 @@ impl<'a> Ctx<'a> { flags |= FnFlags::HAS_SELF_PARAM; } if func.default_token().is_some() { - flags |= FnFlags::IS_DEFAULT; + flags |= FnFlags::HAS_DEFAULT_KW; } if func.const_token().is_some() { - flags |= FnFlags::IS_CONST; + flags |= FnFlags::HAS_CONST_KW; } if func.async_token().is_some() { - flags |= FnFlags::IS_ASYNC; + flags |= FnFlags::HAS_ASYNC_KW; } if func.unsafe_token().is_some() { - flags |= FnFlags::IS_UNSAFE; + flags |= FnFlags::HAS_UNSAFE_KW; } let mut res = Function { @@ -554,22 +554,10 @@ impl<'a> Ctx<'a> { // should be considered to be in an extern block too. let attrs = RawAttrs::new(self.db, &item, self.hygiene()); let id: ModItem = match item { - ast::ExternItem::Fn(ast) => { - let func_id = self.lower_function(&ast)?; - let func = &mut self.data().functions[func_id.index]; - if is_intrinsic_fn_unsafe(&func.name) { - // FIXME: this breaks in macros - func.flags |= FnFlags::IS_UNSAFE; - } - func_id.into() - } + ast::ExternItem::Fn(ast) => self.lower_function(&ast)?.into(), ast::ExternItem::Static(ast) => self.lower_static(&ast)?.into(), ast::ExternItem::TypeAlias(ty) => self.lower_type_alias(&ty)?.into(), - ast::ExternItem::MacroCall(call) => { - // FIXME: we need some way of tracking that the macro call is in an - // extern block - self.lower_macro_call(&call)?.into() - } + ast::ExternItem::MacroCall(call) => self.lower_macro_call(&call)?.into(), }; self.add_attrs(id.into(), attrs); Some(id) @@ -716,49 +704,6 @@ enum GenericsOwner<'a> { Impl, } -/// Returns `true` if the given intrinsic is unsafe to call, or false otherwise. -fn is_intrinsic_fn_unsafe(name: &Name) -> bool { - // Should be kept in sync with https://github.com/rust-lang/rust/blob/532d2b14c05f9bc20b2d27cbb5f4550d28343a36/compiler/rustc_typeck/src/check/intrinsic.rs#L72-L106 - ![ - known::abort, - known::add_with_overflow, - known::bitreverse, - known::black_box, - known::bswap, - known::caller_location, - known::ctlz, - known::ctpop, - known::cttz, - known::discriminant_value, - known::forget, - known::likely, - known::maxnumf32, - known::maxnumf64, - known::min_align_of, - known::minnumf32, - known::minnumf64, - known::mul_with_overflow, - known::needs_drop, - known::ptr_guaranteed_eq, - known::ptr_guaranteed_ne, - known::rotate_left, - known::rotate_right, - known::rustc_peek, - known::saturating_add, - known::saturating_sub, - known::size_of, - known::sub_with_overflow, - known::type_id, - known::type_name, - known::unlikely, - known::variant_count, - known::wrapping_add, - known::wrapping_mul, - known::wrapping_sub, - ] - .contains(name) -} - fn lower_abi(abi: ast::Abi) -> Interned { // FIXME: Abi::abi() -> Option? match abi.syntax().last_token() { diff --git a/crates/hir_def/src/item_tree/tests.rs b/crates/hir_def/src/item_tree/tests.rs index cb3fd9b94a..65352e7444 100644 --- a/crates/hir_def/src/item_tree/tests.rs +++ b/crates/hir_def/src/item_tree/tests.rs @@ -76,7 +76,6 @@ extern "C" { pub(self) static EX_STATIC: u8 = _; #[on_extern_fn] // AttrId { ast_index: 0 } - // flags = 0x20 pub(self) fn ex_fn() -> (); } "##]], diff --git a/crates/hir_ty/src/diagnostics/unsafe_check.rs b/crates/hir_ty/src/diagnostics/unsafe_check.rs index b0fc49fc61..161b19a739 100644 --- a/crates/hir_ty/src/diagnostics/unsafe_check.rs +++ b/crates/hir_ty/src/diagnostics/unsafe_check.rs @@ -8,14 +8,16 @@ use hir_def::{ DefWithBodyId, }; -use crate::{db::HirDatabase, InferenceResult, Interner, TyExt, TyKind}; +use crate::{ + db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TyExt, TyKind, +}; pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> Vec { let infer = db.infer(def); let mut res = Vec::new(); let is_unsafe = match def { - DefWithBodyId::FunctionId(it) => db.function_data(it).is_unsafe(), + DefWithBodyId::FunctionId(it) => db.function_data(it).has_unsafe_kw(), DefWithBodyId::StaticId(_) | DefWithBodyId::ConstId(_) => false, }; if is_unsafe { @@ -62,7 +64,7 @@ fn walk_unsafe( match expr { &Expr::Call { callee, .. } => { if let Some(func) = infer[callee].as_fn_def(db) { - if db.function_data(func).is_unsafe() { + if is_fn_unsafe_to_call(db, func) { unsafe_expr_cb(UnsafeExpr { expr: current, inside_unsafe_block }); } } @@ -79,7 +81,7 @@ fn walk_unsafe( Expr::MethodCall { .. } => { if infer .method_resolution(current) - .map(|(func, _)| db.function_data(func).is_unsafe()) + .map(|(func, _)| is_fn_unsafe_to_call(db, func)) .unwrap_or(false) { unsafe_expr_cb(UnsafeExpr { expr: current, inside_unsafe_block }); diff --git a/crates/hir_ty/src/infer.rs b/crates/hir_ty/src/infer.rs index 70bb56e02f..eca6b3a076 100644 --- a/crates/hir_ty/src/infer.rs +++ b/crates/hir_ty/src/infer.rs @@ -748,7 +748,7 @@ impl<'a> InferenceContext<'a> { self.infer_pat(*pat, &ty, BindingMode::default()); } let error_ty = &TypeRef::Error; - let return_ty = if data.is_async() { + let return_ty = if data.has_async_kw() { data.async_ret_type.as_deref().unwrap_or(error_ty) } else { &*data.ret_type diff --git a/crates/hir_ty/src/lib.rs b/crates/hir_ty/src/lib.rs index 8729b52ae8..225bcdd25e 100644 --- a/crates/hir_ty/src/lib.rs +++ b/crates/hir_ty/src/lib.rs @@ -64,7 +64,7 @@ pub use mapping::{ to_placeholder_idx, }; pub use traits::TraitEnvironment; -pub use utils::all_super_traits; +pub use utils::{all_super_traits, is_fn_unsafe_to_call}; pub use walk::TypeWalk; pub use chalk_ir::{ diff --git a/crates/hir_ty/src/utils.rs b/crates/hir_ty/src/utils.rs index c4a11c86d4..0ffd428cff 100644 --- a/crates/hir_ty/src/utils.rs +++ b/crates/hir_ty/src/utils.rs @@ -15,10 +15,10 @@ use hir_def::{ path::Path, resolver::{HasResolver, TypeNs}, type_ref::{TraitBoundModifier, TypeRef}, - ConstParamId, GenericDefId, ItemContainerId, Lookup, TraitId, TypeAliasId, TypeOrConstParamId, - TypeParamId, + ConstParamId, FunctionId, GenericDefId, ItemContainerId, Lookup, TraitId, TypeAliasId, + TypeOrConstParamId, TypeParamId, }; -use hir_expand::name::{name, Name}; +use hir_expand::name::{known, name, Name}; use itertools::Either; use rustc_hash::FxHashSet; use smallvec::{smallvec, SmallVec}; @@ -375,3 +375,66 @@ fn parent_generic_def(db: &dyn DefDatabase, def: GenericDefId) -> Option None, } } + +pub fn is_fn_unsafe_to_call(db: &dyn HirDatabase, func: FunctionId) -> bool { + let data = db.function_data(func); + if data.has_unsafe_kw() { + return true; + } + + match func.lookup(db.upcast()).container { + hir_def::ItemContainerId::ExternBlockId(block) => { + // Function in an `extern` block are always unsafe to call, except when it has + // `"rust-intrinsic"` ABI there are a few exceptions. + let id = block.lookup(db.upcast()).id; + match id.item_tree(db.upcast())[id.value].abi.as_deref() { + Some("rust-intrinsic") => is_intrinsic_fn_unsafe(&data.name), + _ => true, + } + } + _ => false, + } +} + +/// Returns `true` if the given intrinsic is unsafe to call, or false otherwise. +fn is_intrinsic_fn_unsafe(name: &Name) -> bool { + // Should be kept in sync with https://github.com/rust-lang/rust/blob/532d2b14c05f9bc20b2d27cbb5f4550d28343a36/compiler/rustc_typeck/src/check/intrinsic.rs#L72-L106 + ![ + known::abort, + known::add_with_overflow, + known::bitreverse, + known::black_box, + known::bswap, + known::caller_location, + known::ctlz, + known::ctpop, + known::cttz, + known::discriminant_value, + known::forget, + known::likely, + known::maxnumf32, + known::maxnumf64, + known::min_align_of, + known::minnumf32, + known::minnumf64, + known::mul_with_overflow, + known::needs_drop, + known::ptr_guaranteed_eq, + known::ptr_guaranteed_ne, + known::rotate_left, + known::rotate_right, + known::rustc_peek, + known::saturating_add, + known::saturating_sub, + known::size_of, + known::sub_with_overflow, + known::type_id, + known::type_name, + known::unlikely, + known::variant_count, + known::wrapping_add, + known::wrapping_mul, + known::wrapping_sub, + ] + .contains(name) +} diff --git a/crates/ide/src/syntax_highlighting/highlight.rs b/crates/ide/src/syntax_highlighting/highlight.rs index 5fb87598f4..866bba7d1b 100644 --- a/crates/ide/src/syntax_highlighting/highlight.rs +++ b/crates/ide/src/syntax_highlighting/highlight.rs @@ -362,7 +362,7 @@ fn highlight_def(sema: &Semantics, krate: hir::Crate, def: Definit } } - if func.is_unsafe(db) { + if func.is_unsafe_to_call(db) { h |= HlMod::Unsafe; } if func.is_async(db) { @@ -508,7 +508,7 @@ fn highlight_method_call( let mut h = SymbolKind::Function.into(); h |= HlMod::Associated; - if func.is_unsafe(sema.db) || sema.is_unsafe_method_call(method_call) { + if func.is_unsafe_to_call(sema.db) || sema.is_unsafe_method_call(method_call) { h |= HlMod::Unsafe; } if func.is_async(sema.db) { diff --git a/crates/ide_completion/src/render/function.rs b/crates/ide_completion/src/render/function.rs index 2031eef049..211aa432c7 100644 --- a/crates/ide_completion/src/render/function.rs +++ b/crates/ide_completion/src/render/function.rs @@ -237,7 +237,7 @@ fn detail(db: &dyn HirDatabase, func: hir::Function) -> String { if func.is_async(db) { format_to!(detail, "async "); } - if func.is_unsafe(db) { + if func.is_unsafe_to_call(db) { format_to!(detail, "unsafe "); }