Auto merge of #93678 - steffahn:better_unsafe_diagnostics, r=nagisa

Improve `unused_unsafe` lint

I’m going to add some motivation and explanation below, particularly pointing the changes in behavior from this PR.

_Edit:_ Looking for existing issues, looks like this PR fixes #88260.

_Edit2:_ Now also contains code that closes #90776.

Author: bors

compiler/rustc_codegen_llvm/src/abi.rs

@@ -599,13 +599,11 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
         if self.conv == Conv::CCmseNonSecureCall {
             // This will probably get ignored on all targets but those supporting the TrustZone-M
             // extension (thumbv8m targets).
-            unsafe {
-                llvm::AddCallSiteAttrString(
-                    callsite,
-                    llvm::AttributePlace::Function,
-                    cstr::cstr!("cmse_nonsecure_call"),
-                );
-            }
+            llvm::AddCallSiteAttrString(
+                callsite,
+                llvm::AttributePlace::Function,
+                cstr::cstr!("cmse_nonsecure_call"),
+            );
         }
     }
 }

compiler/rustc_middle/src/hir/nested_filter.rs

@@ -3,6 +3,10 @@ use rustc_hir::intravisit::nested_filter::NestedFilter;
 /// Do not visit nested item-like things, but visit nested things
 /// that are inside of an item-like.
 ///
+/// Notably, possible occurrences of bodies in non-item-like things
+/// include: closures/generators, inline `const {}` blocks, and
+/// constant arguments of types, e.g. in `let _: [(); /* HERE */];`.
+///
 /// **This is the most common choice.** A very common pattern is
 /// to use `visit_all_item_likes()` as an outer loop,
 /// and to have the visitor that visits the contents of each item

compiler/rustc_middle/src/lint.rs

@@ -202,6 +202,77 @@ impl<'a> LintDiagnosticBuilder<'a> {
     }
 }
 
+pub fn explain_lint_level_source<'s>(
+    sess: &'s Session,
+    lint: &'static Lint,
+    level: Level,
+    src: LintLevelSource,
+    err: &mut DiagnosticBuilder<'s>,
+) {
+    let name = lint.name_lower();
+    match src {
+        LintLevelSource::Default => {
+            sess.diag_note_once(
+                err,
+                DiagnosticMessageId::from(lint),
+                &format!("`#[{}({})]` on by default", level.as_str(), name),
+            );
+        }
+        LintLevelSource::CommandLine(lint_flag_val, orig_level) => {
+            let flag = match orig_level {
+                Level::Warn => "-W",
+                Level::Deny => "-D",
+                Level::Forbid => "-F",
+                Level::Allow => "-A",
+                Level::ForceWarn => "--force-warn",
+            };
+            let hyphen_case_lint_name = name.replace('_', "-");
+            if lint_flag_val.as_str() == name {
+                sess.diag_note_once(
+                    err,
+                    DiagnosticMessageId::from(lint),
+                    &format!(
+                        "requested on the command line with `{} {}`",
+                        flag, hyphen_case_lint_name
+                    ),
+                );
+            } else {
+                let hyphen_case_flag_val = lint_flag_val.as_str().replace('_', "-");
+                sess.diag_note_once(
+                    err,
+                    DiagnosticMessageId::from(lint),
+                    &format!(
+                        "`{} {}` implied by `{} {}`",
+                        flag, hyphen_case_lint_name, flag, hyphen_case_flag_val
+                    ),
+                );
+            }
+        }
+        LintLevelSource::Node(lint_attr_name, src, reason) => {
+            if let Some(rationale) = reason {
+                err.note(rationale.as_str());
+            }
+            sess.diag_span_note_once(
+                err,
+                DiagnosticMessageId::from(lint),
+                src,
+                "the lint level is defined here",
+            );
+            if lint_attr_name.as_str() != name {
+                let level_str = level.as_str();
+                sess.diag_note_once(
+                    err,
+                    DiagnosticMessageId::from(lint),
+                    &format!(
+                        "`#[{}({})]` implied by `#[{}({})]`",
+                        level_str, name, level_str, lint_attr_name
+                    ),
+                );
+            }
+        }
+    }
+}
+
 pub fn struct_lint_level<'s, 'd>(
     sess: &'s Session,
     lint: &'static Lint,
@@ -277,69 +348,9 @@ pub fn struct_lint_level<'s, 'd>(
             }
         }
 
-        let name = lint.name_lower();
-        match src {
-            LintLevelSource::Default => {
-                sess.diag_note_once(
-                    &mut err,
-                    DiagnosticMessageId::from(lint),
-                    &format!("`#[{}({})]` on by default", level.as_str(), name),
-                );
-            }
-            LintLevelSource::CommandLine(lint_flag_val, orig_level) => {
-                let flag = match orig_level {
-                    Level::Warn => "-W",
-                    Level::Deny => "-D",
-                    Level::Forbid => "-F",
-                    Level::Allow => "-A",
-                    Level::ForceWarn => "--force-warn",
-                };
-                let hyphen_case_lint_name = name.replace('_', "-");
-                if lint_flag_val.as_str() == name {
-                    sess.diag_note_once(
-                        &mut err,
-                        DiagnosticMessageId::from(lint),
-                        &format!(
-                            "requested on the command line with `{} {}`",
-                            flag, hyphen_case_lint_name
-                        ),
-                    );
-                } else {
-                    let hyphen_case_flag_val = lint_flag_val.as_str().replace('_', "-");
-                    sess.diag_note_once(
-                        &mut err,
-                        DiagnosticMessageId::from(lint),
-                        &format!(
-                            "`{} {}` implied by `{} {}`",
-                            flag, hyphen_case_lint_name, flag, hyphen_case_flag_val
-                        ),
-                    );
-                }
-            }
-            LintLevelSource::Node(lint_attr_name, src, reason) => {
-                if let Some(rationale) = reason {
-                    err.note(rationale.as_str());
-                }
-                sess.diag_span_note_once(
-                    &mut err,
-                    DiagnosticMessageId::from(lint),
-                    src,
-                    "the lint level is defined here",
-                );
-                if lint_attr_name.as_str() != name {
-                    let level_str = level.as_str();
-                    sess.diag_note_once(
-                        &mut err,
-                        DiagnosticMessageId::from(lint),
-                        &format!(
-                            "`#[{}({})]` implied by `#[{}({})]`",
-                            level_str, name, level_str, lint_attr_name
-                        ),
-                    );
-                }
-            }
-        }
+        explain_lint_level_source(sess, lint, level, src, &mut err);
 
+        let name = lint.name_lower();
         let is_force_warn = matches!(level, Level::ForceWarn);
         err.code(DiagnosticId::Lint { name, has_future_breakage, is_force_warn });
 

compiler/rustc_middle/src/mir/query.rs

@@ -2,7 +2,7 @@
 
 use crate::mir::{Body, Promoted};
 use crate::ty::{self, Ty, TyCtxt};
-use rustc_data_structures::sync::Lrc;
+use rustc_data_structures::stable_map::FxHashMap;
 use rustc_data_structures::vec_map::VecMap;
 use rustc_errors::ErrorReported;
 use rustc_hir as hir;
@@ -114,13 +114,44 @@ pub struct UnsafetyViolation {
     pub details: UnsafetyViolationDetails,
 }
 
-#[derive(Clone, TyEncodable, TyDecodable, HashStable, Debug)]
+#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
+pub enum UnusedUnsafe {
+    /// `unsafe` block contains no unsafe operations
+    /// > ``unnecessary `unsafe` block``
+    Unused,
+    /// `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 retrival 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)]
 pub struct UnsafetyCheckResult {
     /// Violations that are propagated *upwards* from this function.
-    pub violations: Lrc<[UnsafetyViolation]>,
-    /// `unsafe` blocks in this function, along with whether they are used. This is
-    /// used for the "unused_unsafe" lint.
-    pub unsafe_blocks: Lrc<[(hir::HirId, bool)]>,
+    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>,
+
+    /// This is `Some` iff the item is not a closure.
+    pub unused_unsafes: Option<Vec<(hir::HirId, UnusedUnsafe)>>,
 }
 
 rustc_index::newtype_index! {

compiler/rustc_mir_build/src/build/block.rs

@@ -3,8 +3,6 @@ use crate::build::ForGuard::OutsideGuard;
 use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
 use rustc_middle::mir::*;
 use rustc_middle::thir::*;
-use rustc_session::lint::builtin::UNSAFE_OP_IN_UNSAFE_FN;
-use rustc_session::lint::Level;
 use rustc_span::Span;
 
 impl<'a, 'tcx> Builder<'a, 'tcx> {
@@ -209,28 +207,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         block.unit()
     }
 
-    /// If we are changing the safety mode, create a new source scope
+    /// If we are entering an unsafe block, create a new source scope
     fn update_source_scope_for_safety_mode(&mut self, span: Span, safety_mode: BlockSafety) {
         debug!("update_source_scope_for({:?}, {:?})", span, safety_mode);
         let new_unsafety = match safety_mode {
-            BlockSafety::Safe => None,
-            BlockSafety::BuiltinUnsafe => Some(Safety::BuiltinUnsafe),
+            BlockSafety::Safe => return,
+            BlockSafety::BuiltinUnsafe => Safety::BuiltinUnsafe,
             BlockSafety::ExplicitUnsafe(hir_id) => {
-                match self.in_scope_unsafe {
-                    Safety::Safe => {}
-                    // no longer treat `unsafe fn`s as `unsafe` contexts (see RFC #2585)
-                    Safety::FnUnsafe
-                        if self.tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, hir_id).0
-                            != Level::Allow => {}
-                    _ => return,
-                }
                 self.in_scope_unsafe = Safety::ExplicitUnsafe(hir_id);
-                Some(Safety::ExplicitUnsafe(hir_id))
+                Safety::ExplicitUnsafe(hir_id)
             }
         };
 
-        if let Some(unsafety) = new_unsafety {
-            self.source_scope = self.new_source_scope(span, LintLevel::Inherited, Some(unsafety));
-        }
+        self.source_scope = self.new_source_scope(span, LintLevel::Inherited, Some(new_unsafety));
     }
 }