From 61b1f0e37fad6842c24c41b0de2cb7dbf90f658f Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 23 Aug 2025 06:51:50 -0400 Subject: [PATCH] `assertions_on_constants`: Don't suggest removing assertions with non-local constants. --- clippy_lints/src/assertions_on_constants.rs | 102 +++++++++++--------- clippy_lints/src/lib.rs | 2 +- clippy_utils/src/msrvs.rs | 3 +- tests/ui/assertions_on_constants.rs | 29 +++++- tests/ui/assertions_on_constants.stderr | 88 +++++++++++------ 5 files changed, 145 insertions(+), 79 deletions(-) diff --git a/clippy_lints/src/assertions_on_constants.rs b/clippy_lints/src/assertions_on_constants.rs index b6684825835a..a6518216aa82 100644 --- a/clippy_lints/src/assertions_on_constants.rs +++ b/clippy_lints/src/assertions_on_constants.rs @@ -1,10 +1,13 @@ +use clippy_config::Conf; use clippy_utils::consts::{ConstEvalCtxt, Constant}; use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::is_inside_always_const_context; -use clippy_utils::macros::{PanicExpn, find_assert_args, root_macro_call_first_node}; +use clippy_utils::macros::{find_assert_args, root_macro_call_first_node}; +use clippy_utils::msrvs::Msrv; +use clippy_utils::{is_inside_always_const_context, msrvs}; +use rustc_ast::LitKind; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::declare_lint_pass; +use rustc_session::impl_lint_pass; use rustc_span::sym; declare_clippy_lint! { @@ -28,56 +31,59 @@ declare_clippy_lint! { "`assert!(true)` / `assert!(false)` will be optimized out by the compiler, and should probably be replaced by a `panic!()` or `unreachable!()`" } -declare_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]); +impl_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]); +pub struct AssertionsOnConstants { + msrv: Msrv, +} +impl AssertionsOnConstants { + pub fn new(conf: &Conf) -> Self { + Self { msrv: conf.msrv } + } +} impl<'tcx> LateLintPass<'tcx> for AssertionsOnConstants { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { - let Some(macro_call) = root_macro_call_first_node(cx, e) else { - return; - }; - let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) { - Some(sym::debug_assert_macro) => true, - Some(sym::assert_macro) => false, - _ => return, - }; - let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) else { - return; - }; - let Some(Constant::Bool(val)) = ConstEvalCtxt::new(cx).eval(condition) else { - return; - }; + if let Some(macro_call) = root_macro_call_first_node(cx, e) + && let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) { + Some(sym::debug_assert_macro) => true, + Some(sym::assert_macro) => false, + _ => return, + } + && let Some((condition, _)) = find_assert_args(cx, e, macro_call.expn) + && let Some((Constant::Bool(assert_val), const_src)) = ConstEvalCtxt::new(cx).eval_with_source(condition) + && let in_const_context = is_inside_always_const_context(cx.tcx, e.hir_id) + && (const_src.is_local() || !in_const_context) + && !(is_debug && as_bool_lit(condition) == Some(false)) + { + let (msg, help) = if !const_src.is_local() { + let help = if self.msrv.meets(cx, msrvs::CONST_BLOCKS) { + "consider moving this into a const block: `const { assert!(..) }`" + } else if self.msrv.meets(cx, msrvs::CONST_PANIC) { + "consider moving this to an anonymous constant: `const _: () = { assert!(..); }`" + } else { + return; + }; + ("this assertion has a constant value", help) + } else if assert_val { + ("this assertion is always `true`", "remove the assertion") + } else { + ( + "this assertion is always `false`", + "replace this with `panic!()` or `unreachable!()`", + ) + }; - match condition.kind { - ExprKind::Path(..) | ExprKind::Lit(_) => {}, - _ if is_inside_always_const_context(cx.tcx, e.hir_id) => return, - _ => {}, + span_lint_and_help(cx, ASSERTIONS_ON_CONSTANTS, macro_call.span, msg, None, help); } + } +} - if val { - span_lint_and_help( - cx, - ASSERTIONS_ON_CONSTANTS, - macro_call.span, - format!( - "`{}!(true)` will be optimized out by the compiler", - cx.tcx.item_name(macro_call.def_id) - ), - None, - "remove it", - ); - } else if !is_debug { - let (assert_arg, panic_arg) = match panic_expn { - PanicExpn::Empty => ("", ""), - _ => (", ..", ".."), - }; - span_lint_and_help( - cx, - ASSERTIONS_ON_CONSTANTS, - macro_call.span, - format!("`assert!(false{assert_arg})` should probably be replaced"), - None, - format!("use `panic!({panic_arg})` or `unreachable!({panic_arg})`"), - ); - } +fn as_bool_lit(e: &Expr<'_>) -> Option { + if let ExprKind::Lit(l) = e.kind + && let LitKind::Bool(b) = l.node + { + Some(b) + } else { + None } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 51dabee78e9f..0a955d238319 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -595,7 +595,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(redundant_clone::RedundantClone)); store.register_late_pass(|_| Box::new(slow_vector_initialization::SlowVectorInit)); store.register_late_pass(move |_| Box::new(unnecessary_wraps::UnnecessaryWraps::new(conf))); - store.register_late_pass(|_| Box::new(assertions_on_constants::AssertionsOnConstants)); + store.register_late_pass(|_| Box::new(assertions_on_constants::AssertionsOnConstants::new(conf))); store.register_late_pass(|_| Box::new(assertions_on_result_states::AssertionsOnResultStates)); store.register_late_pass(|_| Box::new(inherent_to_string::InherentToString)); store.register_late_pass(move |_| Box::new(trait_bounds::TraitBounds::new(conf))); diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 5ce28a582f4a..995deba07778 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -31,6 +31,7 @@ msrv_aliases! { 1,82,0 { IS_NONE_OR, REPEAT_N, RAW_REF_OP } 1,81,0 { LINT_REASONS_STABILIZATION, ERROR_IN_CORE, EXPLICIT_SELF_TYPE_ELISION, DURATION_ABS_DIFF } 1,80,0 { BOX_INTO_ITER, LAZY_CELL } + 1,79,0 { CONST_BLOCKS } 1,77,0 { C_STR_LITERALS } 1,76,0 { PTR_FROM_REF, OPTION_RESULT_INSPECT } 1,75,0 { OPTION_AS_SLICE } @@ -46,7 +47,7 @@ msrv_aliases! { 1,60,0 { ABS_DIFF } 1,59,0 { THREAD_LOCAL_CONST_INIT } 1,58,0 { FORMAT_ARGS_CAPTURE, PATTERN_TRAIT_CHAR_ARRAY, CONST_RAW_PTR_DEREF } - 1,57,0 { MAP_WHILE } + 1,57,0 { MAP_WHILE, CONST_PANIC } 1,56,0 { CONST_FN_UNION } 1,55,0 { SEEK_REWIND } 1,54,0 { INTO_KEYS } diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index c2516c541475..b613892f206a 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -42,7 +42,6 @@ fn main() { assert_const!(3); assert_const!(-1); - // Don't lint if based on `cfg!(..)`: assert!(cfg!(feature = "hey") || cfg!(not(feature = "asdf"))); let flag: bool = cfg!(not(feature = "asdf")); @@ -62,9 +61,37 @@ fn main() { const _: () = assert!(N.is_power_of_two()); } +const C: bool = true; + const _: () = { assert!(true); //~^ assertions_on_constants assert!(8 == (7 + 1)); + //~^ assertions_on_constants + + assert!(C); }; + +#[clippy::msrv = "1.57"] +fn _f1() { + assert!(C); + //~^ assertions_on_constants +} + +#[clippy::msrv = "1.56"] +fn _f2() { + assert!(C); +} + +#[clippy::msrv = "1.79"] +fn _f3() { + assert!(C); + //~^ assertions_on_constants +} + +#[clippy::msrv = "1.78"] +fn _f4() { + assert!(C); + //~^ assertions_on_constants +} diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr index 8b7440ec4832..9aed1405fa04 100644 --- a/tests/ui/assertions_on_constants.stderr +++ b/tests/ui/assertions_on_constants.stderr @@ -1,100 +1,132 @@ -error: `assert!(true)` will be optimized out by the compiler +error: this assertion is always `true` --> tests/ui/assertions_on_constants.rs:10:5 | LL | assert!(true); | ^^^^^^^^^^^^^ | - = help: remove it + = help: remove the assertion = note: `-D clippy::assertions-on-constants` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::assertions_on_constants)]` -error: `assert!(false)` should probably be replaced +error: this assertion is always `false` --> tests/ui/assertions_on_constants.rs:13:5 | LL | assert!(false); | ^^^^^^^^^^^^^^ | - = help: use `panic!()` or `unreachable!()` + = help: replace this with `panic!()` or `unreachable!()` -error: `assert!(true)` will be optimized out by the compiler +error: this assertion is always `true` --> tests/ui/assertions_on_constants.rs:16:5 | LL | assert!(true, "true message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: remove it + = help: remove the assertion -error: `assert!(false, ..)` should probably be replaced +error: this assertion is always `false` --> tests/ui/assertions_on_constants.rs:19:5 | LL | assert!(false, "false message"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: use `panic!(..)` or `unreachable!(..)` + = help: replace this with `panic!()` or `unreachable!()` -error: `assert!(false, ..)` should probably be replaced +error: this assertion is always `false` --> tests/ui/assertions_on_constants.rs:23:5 | LL | assert!(false, "{}", msg.to_uppercase()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: use `panic!(..)` or `unreachable!(..)` + = help: replace this with `panic!()` or `unreachable!()` -error: `assert!(true)` will be optimized out by the compiler +error: this assertion has a constant value --> tests/ui/assertions_on_constants.rs:27:5 | LL | assert!(B); | ^^^^^^^^^^ | - = help: remove it + = help: consider moving this into a const block: `const { assert!(..) }` -error: `assert!(false)` should probably be replaced +error: this assertion has a constant value --> tests/ui/assertions_on_constants.rs:31:5 | LL | assert!(C); | ^^^^^^^^^^ | - = help: use `panic!()` or `unreachable!()` + = help: consider moving this into a const block: `const { assert!(..) }` -error: `assert!(false, ..)` should probably be replaced +error: this assertion has a constant value --> tests/ui/assertions_on_constants.rs:34:5 | LL | assert!(C, "C message"); | ^^^^^^^^^^^^^^^^^^^^^^^ | - = help: use `panic!(..)` or `unreachable!(..)` + = help: consider moving this into a const block: `const { assert!(..) }` -error: `debug_assert!(true)` will be optimized out by the compiler +error: this assertion is always `true` --> tests/ui/assertions_on_constants.rs:37:5 | LL | debug_assert!(true); | ^^^^^^^^^^^^^^^^^^^ | - = help: remove it + = help: remove the assertion -error: `assert!(true)` will be optimized out by the compiler - --> tests/ui/assertions_on_constants.rs:54:19 +error: this assertion is always `true` + --> tests/ui/assertions_on_constants.rs:53:19 | LL | const _: () = assert!(true); | ^^^^^^^^^^^^^ | - = help: remove it + = help: remove the assertion -error: `assert!(true)` will be optimized out by the compiler - --> tests/ui/assertions_on_constants.rs:57:5 +error: this assertion is always `true` + --> tests/ui/assertions_on_constants.rs:56:5 | LL | assert!(8 == (7 + 1)); | ^^^^^^^^^^^^^^^^^^^^^ | - = help: remove it + = help: remove the assertion -error: `assert!(true)` will be optimized out by the compiler - --> tests/ui/assertions_on_constants.rs:66:5 +error: this assertion is always `true` + --> tests/ui/assertions_on_constants.rs:67:5 | LL | assert!(true); | ^^^^^^^^^^^^^ | - = help: remove it + = help: remove the assertion -error: aborting due to 12 previous errors +error: this assertion is always `true` + --> tests/ui/assertions_on_constants.rs:70:5 + | +LL | assert!(8 == (7 + 1)); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove the assertion + +error: this assertion has a constant value + --> tests/ui/assertions_on_constants.rs:78:5 + | +LL | assert!(C); + | ^^^^^^^^^^ + | + = help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }` + +error: this assertion has a constant value + --> tests/ui/assertions_on_constants.rs:89:5 + | +LL | assert!(C); + | ^^^^^^^^^^ + | + = help: consider moving this into a const block: `const { assert!(..) }` + +error: this assertion has a constant value + --> tests/ui/assertions_on_constants.rs:95:5 + | +LL | assert!(C); + | ^^^^^^^^^^ + | + = help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }` + +error: aborting due to 16 previous errors