Auto merge of rust-lang#124895 - obeis:static-mut-hidden-ref, r=michaelwoerister

Disallow hidden references to mutable static

Closes rust-lang#123060

Tracking:
- rust-lang#123758

Author: bors

compiler/rustc_driver_impl/src/signal_handler.rs

@@ -34,6 +34,8 @@ macro raw_errln($tokens:tt) {
 }
 
 /// Signal handler installed for SIGSEGV
+// FIXME(obeis): Use `SyncUnsafeCell` instead of allowing `static_mut_refs` lint
+#[allow(static_mut_refs)]
 extern "C" fn print_stack_trace(_: libc::c_int) {
     const MAX_FRAMES: usize = 256;
     // Reserve data segment so we don't have to malloc in a signal handler, which might fail

compiler/rustc_hir_analysis/messages.ftl

@@ -463,24 +463,20 @@ hir_analysis_start_not_target_feature = `#[start]` function is not allowed to ha
 hir_analysis_start_not_track_caller = `#[start]` function is not allowed to be `#[track_caller]`
     .label = `#[start]` function is not allowed to be `#[track_caller]`
 
-hir_analysis_static_mut_ref = creating a {$shared} reference to a mutable static
-    .label = {$shared} reference to mutable static
-    .note = {$shared ->
-        [shared] this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
-        *[mutable] this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
-    }
+hir_analysis_static_mut_ref = creating a {$shared_label}reference to a mutable static
+    .label = {$shared_label}reference to mutable static
+    .shared_note = shared references to mutable statics are dangerous since if there's any kind of mutation of, or mutable reference created for, that static while the reference lives, that's undefined behavior
+    .mut_note = mutable references to mutable statics are dangerous since if there's any other pointer used or reference created for that static while the reference lives, that's undefined behavior
     .suggestion = use `addr_of!` instead to create a raw pointer
     .suggestion_mut = use `addr_of_mut!` instead to create a raw pointer
 
-hir_analysis_static_mut_refs_lint = creating a {$shared} reference to mutable static is discouraged
-    .label = {$shared} reference to mutable static
+hir_analysis_static_mut_refs_lint = creating a {$shared_label}reference to mutable static is discouraged
+    .label = {$shared_label}reference to mutable static
     .suggestion = use `addr_of!` instead to create a raw pointer
     .suggestion_mut = use `addr_of_mut!` instead to create a raw pointer
     .note = this will be a hard error in the 2024 edition
-    .why_note = {$shared ->
-        [shared] this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
-        *[mutable] this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
-    }
+    .shared_note = shared references to mutable statics are dangerous since if there's any kind of mutation of, or mutable reference created for, that static while the reference lives, that's undefined behavior
+    .mut_note = mutable references to mutable statics are dangerous since if there's any other pointer used or reference created for that static while the reference lives, that's undefined behavior
 
 hir_analysis_static_specialize = cannot specialize on `'static` lifetime
 

compiler/rustc_hir_analysis/src/check/errs.rs

@@ -1,60 +1,113 @@
 use rustc_hir as hir;
 use rustc_lint_defs::builtin::STATIC_MUT_REFS;
-use rustc_middle::ty::{Mutability, TyCtxt};
+use rustc_middle::ty::{Mutability, TyCtxt, TyKind};
 use rustc_span::Span;
 
 use crate::errors;
 
 /// Check for shared or mutable references of `static mut` inside expression
-pub fn maybe_expr_static_mut(tcx: TyCtxt<'_>, expr: hir::Expr<'_>) {
-    let span = expr.span;
-    let hir_id = expr.hir_id;
-    if let hir::ExprKind::AddrOf(borrow_kind, m, expr) = expr.kind
-        && matches!(borrow_kind, hir::BorrowKind::Ref)
-        && path_if_static_mut(expr)
-    {
-        handle_static_mut_ref(
-            tcx,
-            span,
-            span.with_hi(expr.span.lo()),
-            span.shrink_to_hi(),
-            span.edition().at_least_rust_2024(),
-            m,
-            hir_id,
-        );
+pub fn maybe_expr_static_mut(tcx: TyCtxt<'_>, expr: &hir::Expr<'_>) {
+    let err_span = expr.span;
+    let lint_level_hir_id = expr.hir_id;
+    match expr.kind {
+        hir::ExprKind::AddrOf(borrow_kind, m, ex)
+            if matches!(borrow_kind, hir::BorrowKind::Ref)
+                && let Some(err_span) = path_is_static_mut(ex, err_span) =>
+        {
+            handle_static_mut_ref(
+                tcx,
+                err_span,
+                err_span.with_hi(ex.span.lo()),
+                err_span.shrink_to_hi(),
+                err_span.edition().at_least_rust_2024(),
+                Some(m),
+                lint_level_hir_id,
+                !expr.span.from_expansion(),
+            );
+        }
+        hir::ExprKind::Index(expr, _, _)
+            if let Some(err_span) = path_is_static_mut(expr, err_span) =>
+        {
+            handle_static_mut_ref(
+                tcx,
+                err_span,
+                err_span.with_hi(expr.span.lo()),
+                err_span.shrink_to_hi(),
+                err_span.edition().at_least_rust_2024(),
+                None,
+                lint_level_hir_id,
+                false,
+            );
+        }
+        _ => {}
     }
 }
 
 /// Check for shared or mutable references of `static mut` inside statement
-pub fn maybe_stmt_static_mut(tcx: TyCtxt<'_>, stmt: hir::Stmt<'_>) {
+pub fn maybe_stmt_static_mut(tcx: TyCtxt<'_>, stmt: &hir::Stmt<'_>) {
     if let hir::StmtKind::Let(loc) = stmt.kind
         && let hir::PatKind::Binding(ba, _, _, _) = loc.pat.kind
         && let hir::ByRef::Yes(rmutbl) = ba.0
         && let Some(init) = loc.init
-        && path_if_static_mut(init)
+        && let Some(err_span) = path_is_static_mut(init, init.span)
     {
         handle_static_mut_ref(
             tcx,
-            init.span,
-            init.span.shrink_to_lo(),
-            init.span.shrink_to_hi(),
+            err_span,
+            err_span.shrink_to_lo(),
+            err_span.shrink_to_hi(),
             loc.span.edition().at_least_rust_2024(),
-            rmutbl,
+            Some(rmutbl),
             stmt.hir_id,
+            false,
+        );
+    }
+}
+
+/// Check if method call takes shared or mutable references of `static mut`
+#[allow(rustc::usage_of_ty_tykind)]
+pub fn maybe_method_static_mut(tcx: TyCtxt<'_>, expr: &hir::Expr<'_>) {
+    if let hir::ExprKind::MethodCall(_, e, _, _) = expr.kind
+        && let Some(err_span) = path_is_static_mut(e, expr.span)
+        && let typeck = tcx.typeck(expr.hir_id.owner)
+        && let Some(method_def_id) = typeck.type_dependent_def_id(expr.hir_id)
+        && let inputs = tcx.fn_sig(method_def_id).skip_binder().inputs().skip_binder()
+        && !inputs.is_empty()
+        && inputs[0].is_ref()
+    {
+        let m = if let TyKind::Ref(_, _, m) = inputs[0].kind() { *m } else { Mutability::Not };
+
+        handle_static_mut_ref(
+            tcx,
+            err_span,
+            err_span.shrink_to_lo(),
+            err_span.shrink_to_hi(),
+            err_span.edition().at_least_rust_2024(),
+            Some(m),
+            expr.hir_id,
+            false,
         );
     }
 }
 
-fn path_if_static_mut(expr: &hir::Expr<'_>) -> bool {
+fn path_is_static_mut(mut expr: &hir::Expr<'_>, mut err_span: Span) -> Option<Span> {
+    if err_span.from_expansion() {
+        err_span = expr.span;
+    }
+
+    while let hir::ExprKind::Field(e, _) = expr.kind {
+        expr = e;
+    }
+
     if let hir::ExprKind::Path(qpath) = expr.kind
         && let hir::QPath::Resolved(_, path) = qpath
         && let hir::def::Res::Def(def_kind, _) = path.res
         && let hir::def::DefKind::Static { safety: _, mutability: Mutability::Mut, nested: false } =
             def_kind
     {
-        return true;
+        return Some(err_span);
     }
-    false
+    None
 }
 
 fn handle_static_mut_ref(
@@ -63,27 +116,37 @@ fn handle_static_mut_ref(
     lo: Span,
     hi: Span,
     e2024: bool,
-    mutable: Mutability,
-    hir_id: hir::HirId,
+    mutable: Option<Mutability>,
+    lint_level_hir_id: hir::HirId,
+    suggest_addr_of: bool,
 ) {
+    let (shared_label, shared_note, mut_note, sugg) = match mutable {
+        Some(Mutability::Mut) => {
+            let sugg =
+                if suggest_addr_of { Some(errors::MutRefSugg::Mut { lo, hi }) } else { None };
+            ("mutable ", false, true, sugg)
+        }
+        Some(Mutability::Not) => {
+            let sugg =
+                if suggest_addr_of { Some(errors::MutRefSugg::Shared { lo, hi }) } else { None };
+            ("shared ", true, false, sugg)
+        }
+        None => ("", true, true, None),
+    };
     if e2024 {
-        let (sugg, shared) = if mutable == Mutability::Mut {
-            (errors::MutRefSugg::Mut { lo, hi }, "mutable")
-        } else {
-            (errors::MutRefSugg::Shared { lo, hi }, "shared")
-        };
-        tcx.dcx().emit_err(errors::StaticMutRef { span, sugg, shared });
+        tcx.dcx().emit_err(errors::StaticMutRef {
+            span,
+            sugg,
+            shared_label,
+            shared_note,
+            mut_note,
+        });
     } else {
-        let (sugg, shared) = if mutable == Mutability::Mut {
-            (errors::MutRefSugg::Mut { lo, hi }, "mutable")
-        } else {
-            (errors::MutRefSugg::Shared { lo, hi }, "shared")
-        };
         tcx.emit_node_span_lint(
             STATIC_MUT_REFS,
-            hir_id,
+            lint_level_hir_id,
             span,
-            errors::RefOfMutStatic { span, sugg, shared },
+            errors::RefOfMutStatic { span, sugg, shared_label, shared_note, mut_note },
         );
     }
 }

compiler/rustc_hir_analysis/src/check/mod.rs

@@ -66,7 +66,7 @@ mod check;
 mod compare_impl_item;
 pub mod dropck;
 mod entry;
-mod errs;
+pub mod errs;
 pub mod intrinsic;
 pub mod intrinsicck;
 mod region;

compiler/rustc_hir_analysis/src/check/region.rs

@@ -228,7 +228,7 @@ fn resolve_stmt<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, stmt: &'tcx h
     let stmt_id = stmt.hir_id.local_id;
     debug!("resolve_stmt(stmt.id={:?})", stmt_id);
 
-    maybe_stmt_static_mut(visitor.tcx, *stmt);
+    maybe_stmt_static_mut(visitor.tcx, stmt);
 
     // Every statement will clean up the temporaries created during
     // execution of that statement. Therefore each statement has an
@@ -248,7 +248,7 @@ fn resolve_stmt<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, stmt: &'tcx h
 fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
     debug!("resolve_expr - pre-increment {} expr = {:?}", visitor.expr_and_pat_count, expr);
 
-    maybe_expr_static_mut(visitor.tcx, *expr);
+    maybe_expr_static_mut(visitor.tcx, expr);
 
     let prev_cx = visitor.cx;
     visitor.enter_node_scope_with_dtor(expr.hir_id.local_id);

compiler/rustc_hir_analysis/src/collect.rs

@@ -43,6 +43,7 @@ use std::cell::Cell;
 use std::iter;
 use std::ops::Bound;
 
+use crate::check::errs::maybe_method_static_mut;
 use crate::check::intrinsic::intrinsic_operation_unsafety;
 use crate::errors;
 use crate::hir_ty_lowering::{HirTyLowerer, RegionInferReason};
@@ -324,6 +325,7 @@ impl<'tcx> Visitor<'tcx> for CollectItemTypesVisitor<'tcx> {
             // depends on typecheck and would therefore hide
             // any further errors in case one typeck fails.
         }
+        maybe_method_static_mut(self.tcx, expr);
         intravisit::walk_expr(self, expr);
     }
 

compiler/rustc_hir_analysis/src/errors.rs

@@ -1502,14 +1502,17 @@ pub struct OnlyCurrentTraitsPointerSugg<'a> {
 
 #[derive(Diagnostic)]
 #[diag(hir_analysis_static_mut_ref, code = E0796)]
-#[note]
 pub struct StaticMutRef<'a> {
     #[primary_span]
     #[label]
     pub span: Span,
     #[subdiagnostic]
-    pub sugg: MutRefSugg,
-    pub shared: &'a str,
+    pub sugg: Option<MutRefSugg>,
+    pub shared_label: &'a str,
+    #[note(hir_analysis_shared_note)]
+    pub shared_note: bool,
+    #[note(hir_analysis_mut_note)]
+    pub mut_note: bool,
 }
 
 #[derive(Subdiagnostic)]
@@ -1538,17 +1541,20 @@ pub enum MutRefSugg {
     },
 }
 
-// STATIC_MUT_REF lint
+// `STATIC_MUT_REF` lint
 #[derive(LintDiagnostic)]
 #[diag(hir_analysis_static_mut_refs_lint)]
 #[note]
-#[note(hir_analysis_why_note)]
 pub struct RefOfMutStatic<'a> {
     #[label]
     pub span: Span,
     #[subdiagnostic]
-    pub sugg: MutRefSugg,
-    pub shared: &'a str,
+    pub sugg: Option<MutRefSugg>,
+    pub shared_label: &'a str,
+    #[note(hir_analysis_shared_note)]
+    pub shared_note: bool,
+    #[note(hir_analysis_mut_note)]
+    pub mut_note: bool,
 }
 
 #[derive(Diagnostic)]

library/alloc/tests/vec.rs

@@ -1285,6 +1285,8 @@ fn test_from_iter_specialization_panic_during_iteration_drops() {
 
 #[test]
 #[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
+// FIXME(obeis): Use `SyncUnsafeCell` instead of allowing `static_mut_ref` lint
+#[cfg_attr(not(bootstrap), allow(static_mut_refs))]
 fn test_from_iter_specialization_panic_during_drop_doesnt_leak() {
     static mut DROP_COUNTER_OLD: [usize; 5] = [0; 5];
     static mut DROP_COUNTER_NEW: [usize; 2] = [0; 2];

library/core/tests/atomic.rs

@@ -228,6 +228,8 @@ fn static_init() {
 }
 
 #[test]
+// FIXME(obeis): Use `SyncUnsafeCell` instead of allowing `static_mut_ref` lint
+#[cfg_attr(not(bootstrap), allow(static_mut_refs))]
 fn atomic_access_bool() {
     static mut ATOMIC: AtomicBool = AtomicBool::new(false);
 

library/panic_unwind/src/seh.rs

@@ -297,6 +297,8 @@ cfg_if::cfg_if! {
    }
 }
 
+// FIXME(obeis): Do not allow `static_mut_refs` lint
+#[allow(static_mut_refs)]
 pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     use core::intrinsics::atomic_store_seqcst;
 

library/std/src/sync/mod.rs

@@ -9,6 +9,9 @@
 //! Consider the following code, operating on some global static variables:
 //!
 //! ```rust
+//! // FIXME(obeis): Do not allow `static_mut_refs` lint
+//! #![allow(static_mut_refs)]
+//!
 //! static mut A: u32 = 0;
 //! static mut B: u32 = 0;
 //! static mut C: u32 = 0;

library/std/src/sys/pal/wasm/alloc.rs

@@ -16,6 +16,9 @@
 //! The crate itself provides a global allocator which on wasm has no
 //! synchronization as there are no threads!
 
+// FIXME(obeis): Do not allow `static_mut_refs` lint
+#![allow(static_mut_refs)]
+
 use crate::alloc::{GlobalAlloc, Layout, System};
 
 static mut DLMALLOC: dlmalloc::Dlmalloc = dlmalloc::Dlmalloc::new();

library/std/src/thread/local/tests.rs

@@ -103,6 +103,9 @@ fn smoke_dtor() {
 
 #[test]
 fn circular() {
+    // FIXME(obeis): Do not allow `static_mut_refs` lint
+    #![allow(static_mut_refs)]
+
     struct S1(&'static LocalKey<UnsafeCell<Option<S1>>>, &'static LocalKey<UnsafeCell<Option<S2>>>);
     struct S2(&'static LocalKey<UnsafeCell<Option<S1>>>, &'static LocalKey<UnsafeCell<Option<S2>>>);
     thread_local!(static K1: UnsafeCell<Option<S1>> = UnsafeCell::new(None));

src/tools/clippy/tests/ui/checked_unwrap/simple_conditionals.rs

@@ -172,6 +172,7 @@ fn issue11371() {
     static mut X: Option<i32> = Some(123);
     unsafe {
         if X.is_some() {
+            //~^ ERROR: creating a shared reference to mutable static is discouraged
             X = None;
             X.unwrap();
         }