Threadlocal_initializer_can_be_made_const will not trigger for unreachable initializers

PartiallyUntyped · PartiallyUntyped · commit 9580952c5c85 · 2024-04-17T12:36:21.000+03:00
This commit introduces a check to ensure that the lint won't trigger when the initializer is unreachable, such as: ``` thread_local! { static STATE: Cell<usize> = panic!(); } ``` This is achieved by looking at the unpeeled initializer expression and ensuring that the parent macro is not `panic!()`, `todo!()`, `unreachable!()`, `unimplemented!()`. fixes #12637 changelog: [`threadlocal_initializer_can_be_made_const`] will no longer trigger on `unreachable` macros.
diff --git a/clippy_lints/src/thread_local_initializer_can_be_made_const.rs b/clippy_lints/src/thread_local_initializer_can_be_made_const.rs
@@ -3,8 +3,9 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::qualify_min_const_fn::is_min_const_fn;
 use clippy_utils::source::snippet;
 use clippy_utils::{fn_has_unsatisfiable_preds, peel_blocks};
+use clippy_utils::macros::{is_panic, macro_backtrace};
 use rustc_errors::Applicability;
-use rustc_hir::{intravisit, ExprKind};
+use rustc_hir::{intravisit, ExprKind, Expr};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
 use rustc_span::sym::thread_local_macro;
@@ -69,6 +70,16 @@ fn is_thread_local_initializer(
     )
 }
 
+fn is_unreachable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let Some(macro_call) = macro_backtrace(expr.span).next() else {
+        return false;
+    };
+    if is_panic(cx, macro_call.def_id) {
+        return !cx.tcx.hir().is_inside_const_context(expr.hir_id);
+    }
+    matches!(cx.tcx.item_name(macro_call.def_id).as_str(), "todo" | "unimplemented" | "unreachable")
+}
+
 #[inline]
 fn initializer_can_be_made_const(cx: &LateContext<'_>, defid: rustc_span::def_id::DefId, msrv: &Msrv) -> bool {
     // Building MIR for `fn`s with unsatisfiable preds results in ICE.
@@ -102,12 +113,17 @@ impl<'tcx> LateLintPass<'tcx> for ThreadLocalInitializerCanBeMadeConst {
             // for details on this issue, see:
             // https://github.com/rust-lang/rust-clippy/pull/12276
             && !cx.tcx.is_const_fn(defid)
-            && initializer_can_be_made_const(cx, defid, &self.msrv)
-            // we know that the function is const-qualifiable, so now
-            // we need only to get the initializer expression to span-lint it.
             && let ExprKind::Block(block, _) = body.value.kind
             && let Some(unpeeled) = block.expr
             && let ret_expr = peel_blocks(unpeeled)
+            // A common pattern around threadlocal! is to make the value unreachable
+            // to force an initialization before usage
+            // https://github.com/rust-lang/rust-clippy/issues/12637
+            // we ensure that this is reachable before we check in mir
+            && !is_unreachable(cx, ret_expr)
+            && initializer_can_be_made_const(cx, defid, &self.msrv)
+            // we know that the function is const-qualifiable, so now
+            // we need only to get the initializer expression to span-lint it.
             && let initializer_snippet = snippet(cx, ret_expr.span, "thread_local! { ... }")
             && initializer_snippet != "thread_local! { ... }"
         {
diff --git a/tests/ui/thread_local_initializer_can_be_made_const.fixed b/tests/ui/thread_local_initializer_can_be_made_const.fixed
@@ -1,6 +1,7 @@
 #![warn(clippy::thread_local_initializer_can_be_made_const)]
 
 use std::cell::RefCell;
+use std::cell::Cell;
 
 fn main() {
     // lint and suggest const
@@ -36,6 +37,37 @@ fn main() {
     }
 }
 
+
+fn issue_12637() {
+    /// The set methods on LocalKey<Cell<T>> and LocalKey<RefCell<T>> are
+    /// guaranteed to bypass the thread_local's initialization expression. 
+    /// See rust-lang/rust#92122. Thus, = panic!() is a useful idiom for
+    /// forcing the use of set on each thread before it accesses the thread local in any other manner.
+    thread_local! {
+        static STATE_12637_PANIC: Cell<usize> = panic!();
+    }
+    STATE_12637_PANIC.set(9);
+    println!("{}", STATE_12637_PANIC.get());
+
+    thread_local! {
+        static STATE_12637_TODO: Cell<usize> = todo!();
+    }
+    STATE_12637_TODO.set(9);
+    println!("{}", STATE_12637_TODO.get());
+
+    thread_local! {
+        static STATE_12637_UNIMPLEMENTED: Cell<usize> = unimplemented!();
+    }
+    STATE_12637_UNIMPLEMENTED.set(9);
+    println!("{}", STATE_12637_UNIMPLEMENTED.get());
+
+    thread_local! {
+        static STATE_12637_UNREACHABLE: Cell<usize> = unreachable!();
+    }
+    STATE_12637_UNREACHABLE.set(9);
+    println!("{}", STATE_12637_UNREACHABLE.get());
+}
+
 #[clippy::msrv = "1.58"]
 fn f() {
     thread_local! {
diff --git a/tests/ui/thread_local_initializer_can_be_made_const.rs b/tests/ui/thread_local_initializer_can_be_made_const.rs
@@ -1,6 +1,7 @@
 #![warn(clippy::thread_local_initializer_can_be_made_const)]
 
 use std::cell::RefCell;
+use std::cell::Cell;
 
 fn main() {
     // lint and suggest const
@@ -36,6 +37,37 @@ fn main() {
     }
 }
 
+
+fn issue_12637() {
+    /// The set methods on LocalKey<Cell<T>> and LocalKey<RefCell<T>> are
+    /// guaranteed to bypass the thread_local's initialization expression. 
+    /// See rust-lang/rust#92122. Thus, = panic!() is a useful idiom for
+    /// forcing the use of set on each thread before it accesses the thread local in any other manner.
+    thread_local! {
+        static STATE_12637_PANIC: Cell<usize> = panic!();
+    }
+    STATE_12637_PANIC.set(9);
+    println!("{}", STATE_12637_PANIC.get());
+
+    thread_local! {
+        static STATE_12637_TODO: Cell<usize> = todo!();
+    }
+    STATE_12637_TODO.set(9);
+    println!("{}", STATE_12637_TODO.get());
+
+    thread_local! {
+        static STATE_12637_UNIMPLEMENTED: Cell<usize> = unimplemented!();
+    }
+    STATE_12637_UNIMPLEMENTED.set(9);
+    println!("{}", STATE_12637_UNIMPLEMENTED.get());
+
+    thread_local! {
+        static STATE_12637_UNREACHABLE: Cell<usize> = unreachable!();
+    }
+    STATE_12637_UNREACHABLE.set(9);
+    println!("{}", STATE_12637_UNREACHABLE.get());
+}
+
 #[clippy::msrv = "1.58"]
 fn f() {
     thread_local! {
diff --git a/tests/ui/thread_local_initializer_can_be_made_const.stderr b/tests/ui/thread_local_initializer_can_be_made_const.stderr
@@ -1,5 +1,5 @@
 error: initializer for `thread_local` value can be made `const`
-  --> tests/ui/thread_local_initializer_can_be_made_const.rs:8:41
+  --> tests/ui/thread_local_initializer_can_be_made_const.rs:9:41
    |
 LL |         static BUF_1: RefCell<String> = RefCell::new(String::new());
    |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `const { RefCell::new(String::new()) }`
@@ -8,31 +8,31 @@ LL |         static BUF_1: RefCell<String> = RefCell::new(String::new());
    = help: to override `-D warnings` add `#[allow(clippy::thread_local_initializer_can_be_made_const)]`
 
 error: initializer for `thread_local` value can be made `const`
-  --> tests/ui/thread_local_initializer_can_be_made_const.rs:18:29
+  --> tests/ui/thread_local_initializer_can_be_made_const.rs:19:29
    |
 LL |         static SIMPLE:i32 = 1;
    |                             ^ help: replace with: `const { 1 }`
 
 error: initializer for `thread_local` value can be made `const`
-  --> tests/ui/thread_local_initializer_can_be_made_const.rs:24:59
+  --> tests/ui/thread_local_initializer_can_be_made_const.rs:25:59
    |
 LL |         static BUF_3_CAN_BE_MADE_CONST: RefCell<String> = RefCell::new(String::new());
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `const { RefCell::new(String::new()) }`
 
 error: initializer for `thread_local` value can be made `const`
-  --> tests/ui/thread_local_initializer_can_be_made_const.rs:26:59
+  --> tests/ui/thread_local_initializer_can_be_made_const.rs:27:59
    |
 LL |         static BUF_4_CAN_BE_MADE_CONST: RefCell<String> = RefCell::new(String::new());
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `const { RefCell::new(String::new()) }`
 
 error: initializer for `thread_local` value can be made `const`
-  --> tests/ui/thread_local_initializer_can_be_made_const.rs:32:31
+  --> tests/ui/thread_local_initializer_can_be_made_const.rs:33:31
    |
 LL |         static PEEL_ME: i32 = { 1 };
    |                               ^^^^^ help: replace with: `const { 1 }`
 
 error: initializer for `thread_local` value can be made `const`
-  --> tests/ui/thread_local_initializer_can_be_made_const.rs:34:36
+  --> tests/ui/thread_local_initializer_can_be_made_const.rs:35:36
    |
 LL |         static PEEL_ME_MANY: i32 = { let x = 1; x * x };
    |                                    ^^^^^^^^^^^^^^^^^^^^ help: replace with: `const { { let x = 1; x * x } }`
