Merge pull request #19038 from ChayimFriedman2/unused-unsafe

feat: Support RFC 2396

crates/hir-def/src/data.rs

@@ -101,6 +101,10 @@ impl FunctionData {
             flags.remove(FnFlags::HAS_UNSAFE_KW);
         }
 
+        if attrs.by_key(&sym::target_feature).exists() {
+            flags.insert(FnFlags::HAS_TARGET_FEATURE);
+        }
+
         Arc::new(FunctionData {
             name: func.name.clone(),
             params: func
@@ -155,6 +159,10 @@ impl FunctionData {
     pub fn is_varargs(&self) -> bool {
         self.flags.contains(FnFlags::IS_VARARGS)
     }
+
+    pub fn has_target_feature(&self) -> bool {
+        self.flags.contains(FnFlags::HAS_TARGET_FEATURE)
+    }
 }
 
 fn parse_rustc_legacy_const_generics(tt: &crate::tt::TopSubtree) -> Box<[u32]> {

crates/hir-def/src/item_tree.rs

@@ -937,7 +937,7 @@ pub struct Param {
 
 bitflags::bitflags! {
     #[derive(Debug, Clone, Copy, Eq, PartialEq, Default)]
-    pub(crate) struct FnFlags: u8 {
+    pub(crate) struct FnFlags: u16 {
         const HAS_SELF_PARAM = 1 << 0;
         const HAS_BODY = 1 << 1;
         const HAS_DEFAULT_KW = 1 << 2;
@@ -946,6 +946,11 @@ bitflags::bitflags! {
         const HAS_UNSAFE_KW = 1 << 5;
         const IS_VARARGS = 1 << 6;
         const HAS_SAFE_KW = 1 << 7;
+        /// The `#[target_feature]` attribute is necessary to check safety (with RFC 2396),
+        /// but keeping it for all functions will consume a lot of memory when there are
+        /// only very few functions with it. So we only encode its existence here, and lookup
+        /// it if needed.
+        const HAS_TARGET_FEATURE = 1 << 8;
     }
 }
 

crates/hir-ty/src/diagnostics/unsafe_check.rs

@@ -14,7 +14,8 @@ use hir_def::{
 };
 
 use crate::{
-    db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TyExt, TyKind,
+    db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TargetFeatures, TyExt,
+    TyKind,
 };
 
 /// Returns `(unsafe_exprs, fn_is_unsafe)`.
@@ -96,6 +97,7 @@ struct UnsafeVisitor<'a> {
     inside_assignment: bool,
     inside_union_destructure: bool,
     unsafe_expr_cb: &'a mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
+    def_target_features: TargetFeatures,
 }
 
 impl<'a> UnsafeVisitor<'a> {
@@ -107,6 +109,10 @@ impl<'a> UnsafeVisitor<'a> {
         unsafe_expr_cb: &'a mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
     ) -> Self {
         let resolver = def.resolver(db.upcast());
+        let def_target_features = match def {
+            DefWithBodyId::FunctionId(func) => TargetFeatures::from_attrs(&db.attrs(func.into())),
+            _ => TargetFeatures::default(),
+        };
         Self {
             db,
             infer,
@@ -117,6 +123,7 @@ impl<'a> UnsafeVisitor<'a> {
             inside_assignment: false,
             inside_union_destructure: false,
             unsafe_expr_cb,
+            def_target_features,
         }
     }
 
@@ -181,7 +188,7 @@ impl<'a> UnsafeVisitor<'a> {
         match expr {
             &Expr::Call { callee, .. } => {
                 if let Some(func) = self.infer[callee].as_fn_def(self.db) {
-                    if is_fn_unsafe_to_call(self.db, func) {
+                    if is_fn_unsafe_to_call(self.db, func, &self.def_target_features) {
                         self.call_cb(current.into(), UnsafetyReason::UnsafeFnCall);
                     }
                 }
@@ -212,7 +219,7 @@ impl<'a> UnsafeVisitor<'a> {
                 if self
                     .infer
                     .method_resolution(current)
-                    .map(|(func, _)| is_fn_unsafe_to_call(self.db, func))
+                    .map(|(func, _)| is_fn_unsafe_to_call(self.db, func, &self.def_target_features))
                     .unwrap_or(false)
                 {
                     self.call_cb(current.into(), UnsafetyReason::UnsafeFnCall);

crates/hir-ty/src/lib.rs

@@ -100,7 +100,7 @@ pub use mapping::{
 };
 pub use method_resolution::check_orphan_rules;
 pub use traits::TraitEnvironment;
-pub use utils::{all_super_traits, direct_super_traits, is_fn_unsafe_to_call};
+pub use utils::{all_super_traits, direct_super_traits, is_fn_unsafe_to_call, TargetFeatures};
 pub use variance::Variance;
 
 pub use chalk_ir::{

crates/hir-ty/src/utils.rs

@@ -9,16 +9,18 @@ use chalk_ir::{
     DebruijnIndex,
 };
 use hir_def::{
+    attr::Attrs,
     db::DefDatabase,
     generics::{WherePredicate, WherePredicateTypeTarget},
     lang_item::LangItem,
     resolver::{HasResolver, TypeNs},
+    tt,
     type_ref::{TraitBoundModifier, TypeRef},
     EnumId, EnumVariantId, FunctionId, Lookup, OpaqueInternableThing, TraitId, TypeAliasId,
     TypeOrConstParamId,
 };
 use hir_expand::name::Name;
-use intern::sym;
+use intern::{sym, Symbol};
 use rustc_abi::TargetDataLayout;
 use rustc_hash::FxHashSet;
 use smallvec::{smallvec, SmallVec};
@@ -264,12 +266,50 @@ impl<'a> ClosureSubst<'a> {
     }
 }
 
-pub fn is_fn_unsafe_to_call(db: &dyn HirDatabase, func: FunctionId) -> bool {
+#[derive(Debug, Default)]
+pub struct TargetFeatures {
+    enabled: FxHashSet<Symbol>,
+}
+
+impl TargetFeatures {
+    pub fn from_attrs(attrs: &Attrs) -> Self {
+        let enabled = attrs
+            .by_key(&sym::target_feature)
+            .tt_values()
+            .filter_map(|tt| {
+                match tt.token_trees().flat_tokens() {
+                    [
+                        tt::TokenTree::Leaf(tt::Leaf::Ident(enable_ident)),
+                        tt::TokenTree::Leaf(tt::Leaf::Punct(tt::Punct { char: '=', .. })),
+                        tt::TokenTree::Leaf(tt::Leaf::Literal(tt::Literal { kind: tt::LitKind::Str, symbol: features, .. })),
+                    ] if enable_ident.sym == sym::enable => Some(features),
+                    _ => None,
+                }
+            })
+            .flat_map(|features| features.as_str().split(',').map(Symbol::intern))
+            .collect();
+        Self { enabled }
+    }
+}
+
+pub fn is_fn_unsafe_to_call(
+    db: &dyn HirDatabase,
+    func: FunctionId,
+    caller_target_features: &TargetFeatures,
+) -> bool {
     let data = db.function_data(func);
     if data.is_unsafe() {
         return true;
     }
 
+    if data.has_target_feature() {
+        // RFC 2396 <https://rust-lang.github.io/rfcs/2396-target-feature-1.1.html>.
+        let callee_target_features = TargetFeatures::from_attrs(&db.attrs(func.into()));
+        if !caller_target_features.enabled.is_superset(&callee_target_features.enabled) {
+            return true;
+        }
+    }
+
     let loc = func.lookup(db.upcast());
     match loc.container {
         hir_def::ItemContainerId::ExternBlockId(block) => {

crates/hir/src/display.rs

@@ -80,7 +80,9 @@ impl HirDisplay for Function {
         if data.is_async() {
             f.write_str("async ")?;
         }
-        if self.is_unsafe_to_call(db) {
+        // FIXME: This will show `unsafe` for functions that are `#[target_feature]` but not unsafe
+        // (they are conditionally unsafe to call). We probably should show something else.
+        if self.is_unsafe_to_call(db, None) {
             f.write_str("unsafe ")?;
         }
         if let Some(abi) = &data.abi {

crates/hir/src/lib.rs

@@ -2361,8 +2361,11 @@ impl Function {
         db.attrs(self.id.into()).is_unstable()
     }
 
-    pub fn is_unsafe_to_call(self, db: &dyn HirDatabase) -> bool {
-        hir_ty::is_fn_unsafe_to_call(db, self.id)
+    pub fn is_unsafe_to_call(self, db: &dyn HirDatabase, caller: Option<Function>) -> bool {
+        let target_features = caller
+            .map(|caller| hir_ty::TargetFeatures::from_attrs(&db.attrs(caller.id.into())))
+            .unwrap_or_default();
+        hir_ty::is_fn_unsafe_to_call(db, self.id, &target_features)
     }
 
     /// Whether this function declaration has a definition.

crates/hir/src/semantics.rs

@@ -2040,6 +2040,13 @@ impl SemanticsScope<'_> {
         Crate { id: self.resolver.krate() }
     }
 
+    pub fn containing_function(&self) -> Option<Function> {
+        self.resolver.body_owner().and_then(|owner| match owner {
+            DefWithBodyId::FunctionId(id) => Some(id.into()),
+            _ => None,
+        })
+    }
+
     pub(crate) fn resolver(&self) -> &Resolver {
         &self.resolver
     }

crates/hir/src/term_search/tactics.rs

@@ -365,7 +365,7 @@ pub(super) fn free_function<'a, DB: HirDatabase>(
                         let ret_ty = it.ret_type_with_args(db, generics.iter().cloned());
                         // Filter out private and unsafe functions
                         if !it.is_visible_from(db, module)
-                            || it.is_unsafe_to_call(db)
+                            || it.is_unsafe_to_call(db, None)
                             || it.is_unstable(db)
                             || ctx.config.enable_borrowcheck && ret_ty.contains_reference(db)
                             || ret_ty.is_raw_ptr()
@@ -470,7 +470,10 @@ pub(super) fn impl_method<'a, DB: HirDatabase>(
             }
 
             // Filter out private and unsafe functions
-            if !it.is_visible_from(db, module) || it.is_unsafe_to_call(db) || it.is_unstable(db) {
+            if !it.is_visible_from(db, module)
+                || it.is_unsafe_to_call(db, None)
+                || it.is_unstable(db)
+            {
                 return None;
             }
 
@@ -658,7 +661,10 @@ pub(super) fn impl_static_method<'a, DB: HirDatabase>(
             }
 
             // Filter out private and unsafe functions
-            if !it.is_visible_from(db, module) || it.is_unsafe_to_call(db) || it.is_unstable(db) {
+            if !it.is_visible_from(db, module)
+                || it.is_unsafe_to_call(db, None)
+                || it.is_unstable(db)
+            {
                 return None;
             }
 

crates/ide-completion/src/context.rs

@@ -442,6 +442,8 @@ pub(crate) struct CompletionContext<'a> {
     pub(crate) krate: hir::Crate,
     /// The module of the `scope`.
     pub(crate) module: hir::Module,
+    /// The function where we're completing, if inside a function.
+    pub(crate) containing_function: Option<hir::Function>,
     /// Whether nightly toolchain is used. Cached since this is looked up a lot.
     pub(crate) is_nightly: bool,
     /// The edition of the current crate
@@ -760,6 +762,7 @@ impl<'a> CompletionContext<'a> {
 
         let krate = scope.krate();
         let module = scope.module();
+        let containing_function = scope.containing_function();
         let edition = krate.edition(db);
 
         let toolchain = db.toolchain_channel(krate.into());
@@ -874,6 +877,7 @@ impl<'a> CompletionContext<'a> {
             token,
             krate,
             module,
+            containing_function,
             is_nightly,
             edition,
             expected_name,

crates/ide-completion/src/render/function.rs

@@ -144,7 +144,7 @@ fn render(
     let detail = if ctx.completion.config.full_function_signatures {
         detail_full(db, func, ctx.completion.edition)
     } else {
-        detail(db, func, ctx.completion.edition)
+        detail(ctx.completion, func, ctx.completion.edition)
     };
     item.set_documentation(ctx.docs(func))
         .set_deprecated(ctx.is_deprecated(func) || ctx.is_deprecated_assoc_item(func))
@@ -307,26 +307,26 @@ fn ref_of_param(ctx: &CompletionContext<'_>, arg: &str, ty: &hir::Type) -> &'sta
     ""
 }
 
-fn detail(db: &dyn HirDatabase, func: hir::Function, edition: Edition) -> String {
-    let mut ret_ty = func.ret_type(db);
+fn detail(ctx: &CompletionContext<'_>, func: hir::Function, edition: Edition) -> String {
+    let mut ret_ty = func.ret_type(ctx.db);
     let mut detail = String::new();
 
-    if func.is_const(db) {
+    if func.is_const(ctx.db) {
         format_to!(detail, "const ");
     }
-    if func.is_async(db) {
+    if func.is_async(ctx.db) {
         format_to!(detail, "async ");
-        if let Some(async_ret) = func.async_ret_type(db) {
+        if let Some(async_ret) = func.async_ret_type(ctx.db) {
             ret_ty = async_ret;
         }
     }
-    if func.is_unsafe_to_call(db) {
+    if func.is_unsafe_to_call(ctx.db, ctx.containing_function) {
         format_to!(detail, "unsafe ");
     }
 
-    format_to!(detail, "fn({})", params_display(db, func, edition));
+    format_to!(detail, "fn({})", params_display(ctx.db, func, edition));
     if !ret_ty.is_unit() {
-        format_to!(detail, " -> {}", ret_ty.display(db, edition));
+        format_to!(detail, " -> {}", ret_ty.display(ctx.db, edition));
     }
     detail
 }

crates/ide-diagnostics/src/handlers/missing_unsafe.rs

@@ -812,4 +812,24 @@ fn main() {
 "#,
         )
     }
+
+    #[test]
+    fn target_feature() {
+        check_diagnostics(
+            r#"
+#[target_feature(enable = "avx")]
+fn foo() {}
+
+#[target_feature(enable = "avx,avx2")]
+fn bar() {
+    foo();
+}
+
+fn baz() {
+    foo();
+ // ^^^^^ 💡 error: call to unsafe function is unsafe and requires an unsafe function or block
+}
+        "#,
+        );
+    }
 }

crates/ide/src/syntax_highlighting/highlight.rs

@@ -427,7 +427,11 @@ pub(super) fn highlight_def(
                 }
             }
 
-            if func.is_unsafe_to_call(db) {
+            // FIXME: Passing `None` here means not-unsafe functions with `#[target_feature]` will be
+            // highlighted as unsafe, even when the current target features set is a superset (RFC 2396).
+            // We probably should consider checking the current function, but I found no easy way to do
+            // that (also I'm worried about perf). There's also an instance below.
+            if func.is_unsafe_to_call(db, None) {
                 h |= HlMod::Unsafe;
             }
             if func.is_async(db) {
@@ -589,7 +593,7 @@ fn highlight_method_call(
 
     let mut h = SymbolKind::Method.into();
 
-    if func.is_unsafe_to_call(sema.db) || sema.is_unsafe_method_call(method_call) {
+    if func.is_unsafe_to_call(sema.db, None) || sema.is_unsafe_method_call(method_call) {
         h |= HlMod::Unsafe;
     }
     if func.is_async(sema.db) {

crates/intern/src/symbol/symbols.rs

@@ -457,6 +457,8 @@ define_symbols! {
     system,
     sysv64,
     Target,
+    target_feature,
+    enable,
     termination,
     test_case,
     test,