Merge #11930

11930: internal: move function unsafety determination out of the ItemTree r=jonas-schievink a=jonas-schievink

Resolves some FIXMEs.

I've also renamed some FnFlags to be more explicit about what they represent (presence of keywords, not necessarily presence of semantics)

bors r+

Co-authored-by: Jonas Schievink <jonas.schievink@ferrous-systems.com>

12 files changed, 107 insertions, 98 deletions

diff --git a/crates/hir/src/display.rs b/crates/hir/src/display.rs
index c1efab09183..6d5ddad3569 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<Output = ...>` 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 8b99662685c..8bab7c1f3ea 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 4cce419a7fe..3ef10297241 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 23a01010391..375587ee935 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 dd7a8a5f24b..cdae0d0803b 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<str> {
     // FIXME: Abi::abi() -> Option<SyntaxToken>?
     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 cb3fd9b94a0..65352e74440 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 b0fc49fc619..161b19a739c 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<ExprId> {
     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 70bb56e02f2..eca6b3a0762 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 8729b52ae8b..225bcdd25e1 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 c4a11c86d4a..0ffd428cff1 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<Generic
         ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => 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 5fb87598f47..866bba7d1b6 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<RootDatabase>, 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 2031eef049a..211aa432c76 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 ");
     }