Fix #[rustc_deprecated_safe_2024]

It should be considered by the edition of the caller, not the callee.

Technically we still don't do it correctly - we need the span of the method name (if it comes from a macro), but we don't keep it and this is good enough for now.

Author: ChayimFriedman2

src/tools/rust-analyzer/crates/hir-def/src/data.rs

@@ -95,10 +95,10 @@ impl FunctionData {
             .map(Box::new);
         let rustc_allow_incoherent_impl = attrs.by_key(&sym::rustc_allow_incoherent_impl).exists();
         if flags.contains(FnFlags::HAS_UNSAFE_KW)
-            && !crate_graph[krate].edition.at_least_2024()
             && attrs.by_key(&sym::rustc_deprecated_safe_2024).exists()
         {
             flags.remove(FnFlags::HAS_UNSAFE_KW);
+            flags.insert(FnFlags::DEPRECATED_SAFE_2024);
         }
 
         if attrs.by_key(&sym::target_feature).exists() {
@@ -152,6 +152,10 @@ impl FunctionData {
         self.flags.contains(FnFlags::HAS_UNSAFE_KW)
     }
 
+    pub fn is_deprecated_safe_2024(&self) -> bool {
+        self.flags.contains(FnFlags::DEPRECATED_SAFE_2024)
+    }
+
     pub fn is_safe(&self) -> bool {
         self.flags.contains(FnFlags::HAS_SAFE_KW)
     }

src/tools/rust-analyzer/crates/hir-def/src/expr_store/lower.rs

@@ -2238,17 +2238,27 @@ impl ExprCollector<'_> {
         let unsafe_arg_new = self.alloc_expr_desugared(Expr::Path(unsafe_arg_new));
         let unsafe_arg_new =
             self.alloc_expr_desugared(Expr::Call { callee: unsafe_arg_new, args: Box::default() });
-        let unsafe_arg_new = self.alloc_expr_desugared(Expr::Unsafe {
+        let mut unsafe_arg_new = self.alloc_expr_desugared(Expr::Unsafe {
             id: None,
-            // We collect the unused expressions here so that we still infer them instead of
-            // dropping them out of the expression tree
-            statements: fmt
-                .orphans
-                .into_iter()
-                .map(|expr| Statement::Expr { expr, has_semi: true })
-                .collect(),
+            statements: Box::new([]),
             tail: Some(unsafe_arg_new),
         });
+        if !fmt.orphans.is_empty() {
+            unsafe_arg_new = self.alloc_expr_desugared(Expr::Block {
+                id: None,
+                // We collect the unused expressions here so that we still infer them instead of
+                // dropping them out of the expression tree. We cannot store them in the `Unsafe`
+                // block because then unsafe blocks within them will get a false "unused unsafe"
+                // diagnostic (rustc has a notion of builtin unsafe blocks, but we don't).
+                statements: fmt
+                    .orphans
+                    .into_iter()
+                    .map(|expr| Statement::Expr { expr, has_semi: true })
+                    .collect(),
+                tail: Some(unsafe_arg_new),
+                label: None,
+            });
+        }
 
         let idx = self.alloc_expr(
             Expr::Call {

src/tools/rust-analyzer/crates/hir-def/src/item_tree.rs

@@ -951,6 +951,7 @@ bitflags::bitflags! {
         /// only very few functions with it. So we only encode its existence here, and lookup
         /// it if needed.
         const HAS_TARGET_FEATURE = 1 << 8;
+        const DEPRECATED_SAFE_2024 = 1 << 9;
     }
 }
 

src/tools/rust-analyzer/crates/hir-ty/src/diagnostics/unsafe_check.rs

@@ -10,24 +10,26 @@ use hir_def::{
     path::Path,
     resolver::{HasResolver, ResolveValueResult, Resolver, ValueNs},
     type_ref::Rawness,
-    AdtId, DefWithBodyId, FieldId, VariantId,
+    AdtId, DefWithBodyId, FieldId, FunctionId, VariantId,
 };
+use span::Edition;
 
 use crate::{
     db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TargetFeatures, TyExt,
     TyKind,
 };
 
-/// Returns `(unsafe_exprs, fn_is_unsafe)`.
-///
-/// If `fn_is_unsafe` is false, `unsafe_exprs` are hard errors. If true, they're `unsafe_op_in_unsafe_fn`.
-pub fn missing_unsafe(
-    db: &dyn HirDatabase,
-    def: DefWithBodyId,
-) -> (Vec<(ExprOrPatId, UnsafetyReason)>, bool) {
+#[derive(Debug, Default)]
+pub struct MissingUnsafeResult {
+    pub unsafe_exprs: Vec<(ExprOrPatId, UnsafetyReason)>,
+    /// If `fn_is_unsafe` is false, `unsafe_exprs` are hard errors. If true, they're `unsafe_op_in_unsafe_fn`.
+    pub fn_is_unsafe: bool,
+    pub deprecated_safe_calls: Vec<ExprId>,
+}
+
+pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> MissingUnsafeResult {
     let _p = tracing::info_span!("missing_unsafe").entered();
 
-    let mut res = Vec::new();
     let is_unsafe = match def {
         DefWithBodyId::FunctionId(it) => db.function_data(it).is_unsafe(),
         DefWithBodyId::StaticId(_)
@@ -37,11 +39,19 @@ pub fn missing_unsafe(
         | DefWithBodyId::FieldId(_) => false,
     };
 
+    let mut res = MissingUnsafeResult { fn_is_unsafe: is_unsafe, ..MissingUnsafeResult::default() };
     let body = db.body(def);
     let infer = db.infer(def);
-    let mut callback = |node, inside_unsafe_block, reason| {
-        if inside_unsafe_block == InsideUnsafeBlock::No {
-            res.push((node, reason));
+    let mut callback = |diag| match diag {
+        UnsafeDiagnostic::UnsafeOperation { node, inside_unsafe_block, reason } => {
+            if inside_unsafe_block == InsideUnsafeBlock::No {
+                res.unsafe_exprs.push((node, reason));
+            }
+        }
+        UnsafeDiagnostic::DeprecatedSafe2024 { node, inside_unsafe_block } => {
+            if inside_unsafe_block == InsideUnsafeBlock::No {
+                res.deprecated_safe_calls.push(node)
+            }
         }
     };
     let mut visitor = UnsafeVisitor::new(db, &infer, &body, def, &mut callback);
@@ -56,7 +66,7 @@ pub fn missing_unsafe(
         }
     }
 
-    (res, is_unsafe)
+    res
 }
 
 #[derive(Debug, Clone, Copy)]
@@ -75,15 +85,31 @@ pub enum InsideUnsafeBlock {
     Yes,
 }
 
+#[derive(Debug)]
+enum UnsafeDiagnostic {
+    UnsafeOperation {
+        node: ExprOrPatId,
+        inside_unsafe_block: InsideUnsafeBlock,
+        reason: UnsafetyReason,
+    },
+    /// A lint.
+    DeprecatedSafe2024 { node: ExprId, inside_unsafe_block: InsideUnsafeBlock },
+}
+
 pub fn unsafe_expressions(
     db: &dyn HirDatabase,
     infer: &InferenceResult,
     def: DefWithBodyId,
     body: &Body,
     current: ExprId,
-    unsafe_expr_cb: &mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
+    callback: &mut dyn FnMut(InsideUnsafeBlock),
 ) {
-    let mut visitor = UnsafeVisitor::new(db, infer, body, def, unsafe_expr_cb);
+    let mut visitor_callback = |diag| {
+        if let UnsafeDiagnostic::UnsafeOperation { inside_unsafe_block, .. } = diag {
+            callback(inside_unsafe_block);
+        }
+    };
+    let mut visitor = UnsafeVisitor::new(db, infer, body, def, &mut visitor_callback);
     _ = visitor.resolver.update_to_inner_scope(db.upcast(), def, current);
     visitor.walk_expr(current);
 }
@@ -97,8 +123,10 @@ struct UnsafeVisitor<'a> {
     inside_unsafe_block: InsideUnsafeBlock,
     inside_assignment: bool,
     inside_union_destructure: bool,
-    unsafe_expr_cb: &'a mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
+    callback: &'a mut dyn FnMut(UnsafeDiagnostic),
     def_target_features: TargetFeatures,
+    // FIXME: This needs to be the edition of the span of each call.
+    edition: Edition,
 }
 
 impl<'a> UnsafeVisitor<'a> {
@@ -107,13 +135,14 @@ impl<'a> UnsafeVisitor<'a> {
         infer: &'a InferenceResult,
         body: &'a Body,
         def: DefWithBodyId,
-        unsafe_expr_cb: &'a mut dyn FnMut(ExprOrPatId, InsideUnsafeBlock, UnsafetyReason),
+        unsafe_expr_cb: &'a mut dyn FnMut(UnsafeDiagnostic),
     ) -> 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(),
         };
+        let edition = db.crate_graph()[resolver.module().krate()].edition;
         Self {
             db,
             infer,
@@ -123,13 +152,34 @@ impl<'a> UnsafeVisitor<'a> {
             inside_unsafe_block: InsideUnsafeBlock::No,
             inside_assignment: false,
             inside_union_destructure: false,
-            unsafe_expr_cb,
+            callback: unsafe_expr_cb,
             def_target_features,
+            edition,
         }
     }
 
-    fn call_cb(&mut self, node: ExprOrPatId, reason: UnsafetyReason) {
-        (self.unsafe_expr_cb)(node, self.inside_unsafe_block, reason);
+    fn on_unsafe_op(&mut self, node: ExprOrPatId, reason: UnsafetyReason) {
+        (self.callback)(UnsafeDiagnostic::UnsafeOperation {
+            node,
+            inside_unsafe_block: self.inside_unsafe_block,
+            reason,
+        });
+    }
+
+    fn check_call(&mut self, node: ExprId, func: FunctionId) {
+        let unsafety = is_fn_unsafe_to_call(self.db, func, &self.def_target_features, self.edition);
+        match unsafety {
+            crate::utils::Unsafety::Safe => {}
+            crate::utils::Unsafety::Unsafe => {
+                self.on_unsafe_op(node.into(), UnsafetyReason::UnsafeFnCall)
+            }
+            crate::utils::Unsafety::DeprecatedSafe2024 => {
+                (self.callback)(UnsafeDiagnostic::DeprecatedSafe2024 {
+                    node,
+                    inside_unsafe_block: self.inside_unsafe_block,
+                })
+            }
+        }
     }
 
     fn walk_pats_top(&mut self, pats: impl Iterator<Item = PatId>, parent_expr: ExprId) {
@@ -154,7 +204,9 @@ impl<'a> UnsafeVisitor<'a> {
                 | Pat::Ref { .. }
                 | Pat::Box { .. }
                 | Pat::Expr(..)
-                | Pat::ConstBlock(..) => self.call_cb(current.into(), UnsafetyReason::UnionField),
+                | Pat::ConstBlock(..) => {
+                    self.on_unsafe_op(current.into(), UnsafetyReason::UnionField)
+                }
                 // `Or` only wraps other patterns, and `Missing`/`Wild` do not constitute a read.
                 Pat::Missing | Pat::Wild | Pat::Or(_) => {}
             }
@@ -189,9 +241,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, &self.def_target_features) {
-                        self.call_cb(current.into(), UnsafetyReason::UnsafeFnCall);
-                    }
+                    self.check_call(current, func);
                 }
             }
             Expr::Path(path) => {
@@ -217,18 +267,13 @@ impl<'a> UnsafeVisitor<'a> {
                 }
             }
             Expr::MethodCall { .. } => {
-                if self
-                    .infer
-                    .method_resolution(current)
-                    .map(|(func, _)| is_fn_unsafe_to_call(self.db, func, &self.def_target_features))
-                    .unwrap_or(false)
-                {
-                    self.call_cb(current.into(), UnsafetyReason::UnsafeFnCall);
+                if let Some((func, _)) = self.infer.method_resolution(current) {
+                    self.check_call(current, func);
                 }
             }
             Expr::UnaryOp { expr, op: UnaryOp::Deref } => {
                 if let TyKind::Raw(..) = &self.infer[*expr].kind(Interner) {
-                    self.call_cb(current.into(), UnsafetyReason::RawPtrDeref);
+                    self.on_unsafe_op(current.into(), UnsafetyReason::RawPtrDeref);
                 }
             }
             Expr::Unsafe { .. } => {
@@ -243,7 +288,7 @@ impl<'a> UnsafeVisitor<'a> {
                 self.walk_pats_top(std::iter::once(target), current);
                 self.inside_assignment = old_inside_assignment;
             }
-            Expr::InlineAsm(_) => self.call_cb(current.into(), UnsafetyReason::InlineAsm),
+            Expr::InlineAsm(_) => self.on_unsafe_op(current.into(), UnsafetyReason::InlineAsm),
             // rustc allows union assignment to propagate through field accesses and casts.
             Expr::Cast { .. } => self.inside_assignment = inside_assignment,
             Expr::Field { .. } => {
@@ -252,7 +297,7 @@ impl<'a> UnsafeVisitor<'a> {
                     if let Some(Either::Left(FieldId { parent: VariantId::UnionId(_), .. })) =
                         self.infer.field_resolution(current)
                     {
-                        self.call_cb(current.into(), UnsafetyReason::UnionField);
+                        self.on_unsafe_op(current.into(), UnsafetyReason::UnionField);
                     }
                 }
             }
@@ -287,9 +332,9 @@ impl<'a> UnsafeVisitor<'a> {
         if let Some(ResolveValueResult::ValueNs(ValueNs::StaticId(id), _)) = value_or_partial {
             let static_data = self.db.static_data(id);
             if static_data.mutable {
-                self.call_cb(node, UnsafetyReason::MutableStatic);
+                self.on_unsafe_op(node, UnsafetyReason::MutableStatic);
             } else if static_data.is_extern && !static_data.has_safe_kw {
-                self.call_cb(node, UnsafetyReason::ExternStatic);
+                self.on_unsafe_op(node, UnsafetyReason::ExternStatic);
             }
         }
     }

src/tools/rust-analyzer/crates/hir-ty/src/lib.rs

@@ -100,7 +100,9 @@ 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, TargetFeatures};
+pub use utils::{
+    all_super_traits, direct_super_traits, is_fn_unsafe_to_call, TargetFeatures, Unsafety,
+};
 pub use variance::Variance;
 
 pub use chalk_ir::{

src/tools/rust-analyzer/crates/hir-ty/src/utils.rs

@@ -24,6 +24,7 @@ use intern::{sym, Symbol};
 use rustc_abi::TargetDataLayout;
 use rustc_hash::FxHashSet;
 use smallvec::{smallvec, SmallVec};
+use span::Edition;
 use stdx::never;
 
 use crate::{
@@ -292,21 +293,38 @@ impl TargetFeatures {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Unsafety {
+    Safe,
+    Unsafe,
+    /// A lint.
+    DeprecatedSafe2024,
+}
+
 pub fn is_fn_unsafe_to_call(
     db: &dyn HirDatabase,
     func: FunctionId,
     caller_target_features: &TargetFeatures,
-) -> bool {
+    call_edition: Edition,
+) -> Unsafety {
     let data = db.function_data(func);
     if data.is_unsafe() {
-        return true;
+        return Unsafety::Unsafe;
     }
 
     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;
+            return Unsafety::Unsafe;
+        }
+    }
+
+    if data.is_deprecated_safe_2024() {
+        if call_edition.at_least_2024() {
+            return Unsafety::Unsafe;
+        } else {
+            return Unsafety::DeprecatedSafe2024;
         }
     }
 
@@ -319,14 +337,22 @@ pub fn is_fn_unsafe_to_call(
             if is_intrinsic_block {
                 // legacy intrinsics
                 // extern "rust-intrinsic" intrinsics are unsafe unless they have the rustc_safe_intrinsic attribute
-                !db.attrs(func.into()).by_key(&sym::rustc_safe_intrinsic).exists()
+                if db.attrs(func.into()).by_key(&sym::rustc_safe_intrinsic).exists() {
+                    Unsafety::Safe
+                } else {
+                    Unsafety::Unsafe
+                }
             } else {
                 // Function in an `extern` block are always unsafe to call, except when
                 // it is marked as `safe`.
-                !data.is_safe()
+                if data.is_safe() {
+                    Unsafety::Safe
+                } else {
+                    Unsafety::Unsafe
+                }
             }
         }
-        _ => false,
+        _ => Unsafety::Safe,
     }
 }