Repository: rust-analyzer/rust-analyzer
Pull request #11930 (2022-04-07 16:38:27)

internal: move function unsafety determination out of the ItemTree
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+


Patch:
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<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 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<str> {
     // FIXME: Abi::abi() -> Option<SyntaxToken>?
     match abi.syntax().last_token() {
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<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 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<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 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<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 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 ");
     }
 


Test patch:
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() -> ();
             }
         "##]],