Rollup merge of #100081 - RalfJung:unused-unsafe-in-unsafe-fn, r=jackh726

never consider unsafe blocks unused if they would be required with deny(unsafe_op_in_unsafe_fn)

Judging from #71668 (comment) the consensus nowadays seems to be that we should never consider an unsafe block unused if it was required with `deny(unsafe_op_in_unsafe_fn)`, no matter whether that lint is actually enabled or not. So let's adjust rustc accordingly.

The first commit does the change, the 2nd does some cleanup.

Author: Dylan-DPC

compiler/rustc_errors/src/diagnostic_builder.rs

@@ -566,7 +566,7 @@ impl Drop for DiagnosticBuilderInner<'_> {
                         ),
                     ));
                     handler.emit_diagnostic(&mut self.diagnostic);
-                    panic!();
+                    panic!("error was constructed but not emitted");
                 }
             }
             // `.emit()` was previously called, or maybe we're during `.cancel()`.

compiler/rustc_middle/src/mir/query.rs

@@ -2,7 +2,7 @@
 
 use crate::mir::{Body, ConstantKind, Promoted};
 use crate::ty::{self, OpaqueHiddenType, Ty, TyCtxt};
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::vec_map::VecMap;
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir as hir;
@@ -115,21 +115,6 @@ pub enum UnusedUnsafe {
     /// `unsafe` block nested under another (used) `unsafe` block
     /// > ``… because it's nested under this `unsafe` block``
     InUnsafeBlock(hir::HirId),
-    /// `unsafe` block nested under `unsafe fn`
-    /// > ``… because it's nested under this `unsafe fn` ``
-    ///
-    /// the second HirId here indicates the first usage of the `unsafe` block,
-    /// which allows retrieval of the LintLevelSource for why that operation would
-    /// have been permitted without the block
-    InUnsafeFn(hir::HirId, hir::HirId),
-}
-
-#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
-pub enum UsedUnsafeBlockData {
-    SomeDisallowedInUnsafeFn,
-    // the HirId here indicates the first usage of the `unsafe` block
-    // (i.e. the one that's first encountered in the MIR traversal of the unsafety check)
-    AllAllowedInUnsafeFn(hir::HirId),
 }
 
 #[derive(TyEncodable, TyDecodable, HashStable, Debug)]
@@ -138,10 +123,7 @@ pub struct UnsafetyCheckResult {
     pub violations: Vec<UnsafetyViolation>,
 
     /// Used `unsafe` blocks in this function. This is used for the "unused_unsafe" lint.
-    ///
-    /// The keys are the used `unsafe` blocks, the UnusedUnsafeKind indicates whether
-    /// or not any of the usages happen at a place that doesn't allow `unsafe_op_in_unsafe_fn`.
-    pub used_unsafe_blocks: FxHashMap<hir::HirId, UsedUnsafeBlockData>,
+    pub used_unsafe_blocks: FxHashSet<hir::HirId>,
 
     /// This is `Some` iff the item is not a closure.
     pub unused_unsafes: Option<Vec<(hir::HirId, UnusedUnsafe)>>,

compiler/rustc_mir_build/src/check_unsafety.rs

@@ -75,10 +75,11 @@ impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
         match self.safety_context {
             SafetyContext::BuiltinUnsafeBlock => {}
             SafetyContext::UnsafeBlock { ref mut used, .. } => {
-                if !self.body_unsafety.is_unsafe() || !unsafe_op_in_unsafe_fn_allowed {
-                    // Mark this block as useful
-                    *used = true;
-                }
+                // Mark this block as useful (even inside `unsafe fn`, where it is technically
+                // redundant -- but we want to eventually enable `unsafe_op_in_unsafe_fn` by
+                // default which will require those blocks:
+                // https://github.com/rust-lang/rust/issues/71668#issuecomment-1203075594).
+                *used = true;
             }
             SafetyContext::UnsafeFn if unsafe_op_in_unsafe_fn_allowed => {}
             SafetyContext::UnsafeFn => {

compiler/rustc_mir_transform/src/check_unsafety.rs

@@ -1,17 +1,16 @@
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::struct_span_err;
 use rustc_hir as hir;
 use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::hir_id::HirId;
 use rustc_hir::intravisit;
 use rustc_middle::mir::visit::{MutatingUseContext, PlaceContext, Visitor};
+use rustc_middle::mir::*;
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::{self, TyCtxt};
-use rustc_middle::{lint, mir::*};
 use rustc_session::lint::builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
 use rustc_session::lint::Level;
 
-use std::collections::hash_map;
 use std::ops::Bound;
 
 pub struct UnsafetyChecker<'a, 'tcx> {
@@ -23,10 +22,7 @@ pub struct UnsafetyChecker<'a, 'tcx> {
     param_env: ty::ParamEnv<'tcx>,
 
     /// Used `unsafe` blocks in this function. This is used for the "unused_unsafe" lint.
-    ///
-    /// The keys are the used `unsafe` blocks, the UnusedUnsafeKind indicates whether
-    /// or not any of the usages happen at a place that doesn't allow `unsafe_op_in_unsafe_fn`.
-    used_unsafe_blocks: FxHashMap<HirId, UsedUnsafeBlockData>,
+    used_unsafe_blocks: FxHashSet<HirId>,
 }
 
 impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
@@ -130,10 +126,7 @@ impl<'tcx> Visitor<'tcx> for UnsafetyChecker<'_, 'tcx> {
                 &AggregateKind::Closure(def_id, _) | &AggregateKind::Generator(def_id, _, _) => {
                     let UnsafetyCheckResult { violations, used_unsafe_blocks, .. } =
                         self.tcx.unsafety_check_result(def_id);
-                    self.register_violations(
-                        violations,
-                        used_unsafe_blocks.iter().map(|(&h, &d)| (h, d)),
-                    );
+                    self.register_violations(violations, used_unsafe_blocks.iter().copied());
                 }
             },
             _ => {}
@@ -257,22 +250,8 @@ impl<'tcx> UnsafetyChecker<'_, 'tcx> {
     fn register_violations<'a>(
         &mut self,
         violations: impl IntoIterator<Item = &'a UnsafetyViolation>,
-        new_used_unsafe_blocks: impl IntoIterator<Item = (HirId, UsedUnsafeBlockData)>,
+        new_used_unsafe_blocks: impl IntoIterator<Item = HirId>,
     ) {
-        use UsedUnsafeBlockData::{AllAllowedInUnsafeFn, SomeDisallowedInUnsafeFn};
-
-        let update_entry = |this: &mut Self, hir_id, new_usage| {
-            match this.used_unsafe_blocks.entry(hir_id) {
-                hash_map::Entry::Occupied(mut entry) => {
-                    if new_usage == SomeDisallowedInUnsafeFn {
-                        *entry.get_mut() = SomeDisallowedInUnsafeFn;
-                    }
-                }
-                hash_map::Entry::Vacant(entry) => {
-                    entry.insert(new_usage);
-                }
-            };
-        };
         let safety = self.body.source_scopes[self.source_info.scope]
             .local_data
             .as_ref()
@@ -299,22 +278,14 @@ impl<'tcx> UnsafetyChecker<'_, 'tcx> {
                 }
             }),
             Safety::BuiltinUnsafe => {}
-            Safety::ExplicitUnsafe(hir_id) => violations.into_iter().for_each(|violation| {
-                update_entry(
-                    self,
-                    hir_id,
-                    match self.tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, violation.lint_root).0
-                    {
-                        Level::Allow => AllAllowedInUnsafeFn(violation.lint_root),
-                        _ => SomeDisallowedInUnsafeFn,
-                    },
-                )
+            Safety::ExplicitUnsafe(hir_id) => violations.into_iter().for_each(|_violation| {
+                self.used_unsafe_blocks.insert(hir_id);
             }),
         };
 
-        new_used_unsafe_blocks
-            .into_iter()
-            .for_each(|(hir_id, usage_data)| update_entry(self, hir_id, usage_data));
+        new_used_unsafe_blocks.into_iter().for_each(|hir_id| {
+            self.used_unsafe_blocks.insert(hir_id);
+        });
     }
     fn check_mut_borrowing_layout_constrained_field(
         &mut self,
@@ -411,34 +382,28 @@ enum Context {
 
 struct UnusedUnsafeVisitor<'a, 'tcx> {
     tcx: TyCtxt<'tcx>,
-    used_unsafe_blocks: &'a FxHashMap<HirId, UsedUnsafeBlockData>,
+    used_unsafe_blocks: &'a FxHashSet<HirId>,
     context: Context,
     unused_unsafes: &'a mut Vec<(HirId, UnusedUnsafe)>,
 }
 
 impl<'tcx> intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'_, 'tcx> {
     fn visit_block(&mut self, block: &'tcx hir::Block<'tcx>) {
-        use UsedUnsafeBlockData::{AllAllowedInUnsafeFn, SomeDisallowedInUnsafeFn};
-
         if let hir::BlockCheckMode::UnsafeBlock(hir::UnsafeSource::UserProvided) = block.rules {
             let used = match self.tcx.lint_level_at_node(UNUSED_UNSAFE, block.hir_id) {
-                (Level::Allow, _) => Some(SomeDisallowedInUnsafeFn),
-                _ => self.used_unsafe_blocks.get(&block.hir_id).copied(),
+                (Level::Allow, _) => true,
+                _ => self.used_unsafe_blocks.contains(&block.hir_id),
             };
             let unused_unsafe = match (self.context, used) {
-                (_, None) => UnusedUnsafe::Unused,
-                (Context::Safe, Some(_))
-                | (Context::UnsafeFn(_), Some(SomeDisallowedInUnsafeFn)) => {
+                (_, false) => UnusedUnsafe::Unused,
+                (Context::Safe, true) | (Context::UnsafeFn(_), true) => {
                     let previous_context = self.context;
                     self.context = Context::UnsafeBlock(block.hir_id);
                     intravisit::walk_block(self, block);
                     self.context = previous_context;
                     return;
                 }
-                (Context::UnsafeFn(hir_id), Some(AllAllowedInUnsafeFn(lint_root))) => {
-                    UnusedUnsafe::InUnsafeFn(hir_id, lint_root)
-                }
-                (Context::UnsafeBlock(hir_id), Some(_)) => UnusedUnsafe::InUnsafeBlock(hir_id),
+                (Context::UnsafeBlock(hir_id), true) => UnusedUnsafe::InUnsafeBlock(hir_id),
             };
             self.unused_unsafes.push((block.hir_id, unused_unsafe));
         }
@@ -462,7 +427,7 @@ impl<'tcx> intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'_, 'tcx> {
 fn check_unused_unsafe(
     tcx: TyCtxt<'_>,
     def_id: LocalDefId,
-    used_unsafe_blocks: &FxHashMap<HirId, UsedUnsafeBlockData>,
+    used_unsafe_blocks: &FxHashSet<HirId>,
 ) -> Vec<(HirId, UnusedUnsafe)> {
     let body_id = tcx.hir().maybe_body_owned_by(def_id);
 
@@ -535,25 +500,6 @@ fn report_unused_unsafe(tcx: TyCtxt<'_>, kind: UnusedUnsafe, id: HirId) {
                     "because it's nested under this `unsafe` block",
                 );
             }
-            UnusedUnsafe::InUnsafeFn(id, usage_lint_root) => {
-                db.span_label(
-                    tcx.sess.source_map().guess_head_span(tcx.hir().span(id)),
-                    "because it's nested under this `unsafe` fn",
-                )
-                .note(
-                    "this `unsafe` block does contain unsafe operations, \
-                    but those are already allowed in an `unsafe fn`",
-                );
-                let (level, source) =
-                    tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, usage_lint_root);
-                assert_eq!(level, Level::Allow);
-                lint::explain_lint_level_source(
-                    UNSAFE_OP_IN_UNSAFE_FN,
-                    Level::Allow,
-                    source,
-                    &mut db,
-                );
-            }
         }
 
         db.emit();

src/test/ui/span/lint-unused-unsafe-thir.rs

@@ -22,7 +22,7 @@ fn bad1() { unsafe {} }                  //~ ERROR: unnecessary `unsafe` block
 fn bad2() { unsafe { bad1() } }          //~ ERROR: unnecessary `unsafe` block
 unsafe fn bad3() { unsafe {} }           //~ ERROR: unnecessary `unsafe` block
 fn bad4() { unsafe { callback(||{}) } }  //~ ERROR: unnecessary `unsafe` block
-unsafe fn bad5() { unsafe { unsf() } }   //~ ERROR: unnecessary `unsafe` block
+unsafe fn bad5() { unsafe { unsf() } }
 fn bad6() {
     unsafe {                             // don't put the warning here
         unsafe {                         //~ ERROR: unnecessary `unsafe` block
@@ -31,7 +31,7 @@ fn bad6() {
     }
 }
 unsafe fn bad7() {
-    unsafe {                             //~ ERROR: unnecessary `unsafe` block
+    unsafe {
         unsafe {                         //~ ERROR: unnecessary `unsafe` block
             unsf()
         }

src/test/ui/span/lint-unused-unsafe-thir.stderr

@@ -30,14 +30,6 @@ error: unnecessary `unsafe` block
 LL | fn bad4() { unsafe { callback(||{}) } }
    |             ^^^^^^ unnecessary `unsafe` block
 
-error: unnecessary `unsafe` block
-  --> $DIR/lint-unused-unsafe-thir.rs:25:20
-   |
-LL | unsafe fn bad5() { unsafe { unsf() } }
-   | ----------------   ^^^^^^ unnecessary `unsafe` block
-   | |
-   | because it's nested under this `unsafe` fn
-
 error: unnecessary `unsafe` block
   --> $DIR/lint-unused-unsafe-thir.rs:28:9
    |
@@ -54,13 +46,5 @@ LL |     unsafe {
 LL |         unsafe {
    |         ^^^^^^ unnecessary `unsafe` block
 
-error: unnecessary `unsafe` block
-  --> $DIR/lint-unused-unsafe-thir.rs:34:5
-   |
-LL | unsafe fn bad7() {
-   | ---------------- because it's nested under this `unsafe` fn
-LL |     unsafe {
-   |     ^^^^^^ unnecessary `unsafe` block
-
-error: aborting due to 8 previous errors
+error: aborting due to 6 previous errors