Skip to content

Commit b51b811

Browse files
JarchoJarcho
committed
assertions_on_constants: Don't suggest removing assertions with non-local constants.
1 parent 4db4b32 commit b51b811

File tree

5 files changed

+124
-77
lines changed

5 files changed

+124
-77
lines changed

‎clippy_lints/src/assertions_on_constants.rs‎

Lines changed: 55 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1+
use clippy_config::Conf;
12
use clippy_utils::consts::{ConstEvalCtxt, Constant};
23
use clippy_utils::diagnostics::span_lint_and_help;
3-
use clippy_utils::is_inside_always_const_context;
4-
use clippy_utils::macros::{PanicExpn, find_assert_args, root_macro_call_first_node};
4+
use clippy_utils::macros::{find_assert_args, root_macro_call_first_node};
5+
use clippy_utils::msrvs::Msrv;
6+
use clippy_utils::{is_inside_always_const_context, msrvs};
7+
use rustc_ast::LitKind;
58
use rustc_hir::{Expr, ExprKind};
69
use rustc_lint::{LateContext, LateLintPass};
7-
use rustc_session::declare_lint_pass;
10+
use rustc_session::impl_lint_pass;
811
use rustc_span::sym;
912

1013
declare_clippy_lint! {
@@ -28,56 +31,60 @@ declare_clippy_lint! {
2831
"`assert!(true)` / `assert!(false)` will be optimized out by the compiler, and should probably be replaced by a `panic!()` or `unreachable!()`"
2932
}
3033

31-
declare_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]);
34+
impl_lint_pass!(AssertionsOnConstants => [ASSERTIONS_ON_CONSTANTS]);
35+
pub struct AssertionsOnConstants {
36+
msrv: Msrv,
37+
}
38+
impl AssertionsOnConstants {
39+
pub fn new(conf: &Conf) -> Self {
40+
Self { msrv: conf.msrv }
41+
}
42+
}
3243

3344
impl<'tcx> LateLintPass<'tcx> for AssertionsOnConstants {
3445
fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
35-
let Some(macro_call) = root_macro_call_first_node(cx, e) else {
36-
return;
37-
};
38-
let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) {
39-
Some(sym::debug_assert_macro) => true,
40-
Some(sym::assert_macro) => false,
41-
_ => return,
42-
};
43-
let Some((condition, panic_expn)) = find_assert_args(cx, e, macro_call.expn) else {
44-
return;
45-
};
46-
let Some(Constant::Bool(val)) = ConstEvalCtxt::new(cx).eval(condition) else {
47-
return;
48-
};
46+
if let Some(macro_call) = root_macro_call_first_node(cx, e)
47+
&& let is_debug = match cx.tcx.get_diagnostic_name(macro_call.def_id) {
48+
Some(sym::debug_assert_macro) => true,
49+
Some(sym::assert_macro) => false,
50+
_ => return,
51+
}
52+
&& let Some((condition, _)) = find_assert_args(cx, e, macro_call.expn)
53+
&& let Some((Constant::Bool(assert_val), const_src)) =
54+
ConstEvalCtxt::new(cx).eval_with_source(condition, macro_call.span.ctxt())
55+
&& let in_const_context = is_inside_always_const_context(cx.tcx, e.hir_id)
56+
&& (const_src.is_local() || !in_const_context)
57+
&& !(is_debug && as_bool_lit(condition) == Some(false))
58+
{
59+
let (msg, help) = if !const_src.is_local() {
60+
if self.msrv.meets(cx, msrvs::CONST_PANIC) {
61+
(
62+
"this assertion has a constant value",
63+
"consider moving this to an anonymous constant: `const _: () = { assert!(..); }`",
64+
)
65+
} else {
66+
return;
67+
}
68+
} else if assert_val {
69+
("this assertion is always `true`", "remove the assertion")
70+
} else {
71+
(
72+
"this assertion is always `false`",
73+
"replace this with `panic!()` or `unreachable!()`",
74+
)
75+
};
4976

50-
match condition.kind {
51-
ExprKind::Path(..) | ExprKind::Lit(_) => {},
52-
_ if is_inside_always_const_context(cx.tcx, e.hir_id) => return,
53-
_ => {},
77+
span_lint_and_help(cx, ASSERTIONS_ON_CONSTANTS, macro_call.span, msg, None, help);
5478
}
79+
}
80+
}
5581

56-
if val {
57-
span_lint_and_help(
58-
cx,
59-
ASSERTIONS_ON_CONSTANTS,
60-
macro_call.span,
61-
format!(
62-
"`{}!(true)` will be optimized out by the compiler",
63-
cx.tcx.item_name(macro_call.def_id)
64-
),
65-
None,
66-
"remove it",
67-
);
68-
} else if !is_debug {
69-
let (assert_arg, panic_arg) = match panic_expn {
70-
PanicExpn::Empty => ("", ""),
71-
_ => (", ..", ".."),
72-
};
73-
span_lint_and_help(
74-
cx,
75-
ASSERTIONS_ON_CONSTANTS,
76-
macro_call.span,
77-
format!("`assert!(false{assert_arg})` should probably be replaced"),
78-
None,
79-
format!("use `panic!({panic_arg})` or `unreachable!({panic_arg})`"),
80-
);
81-
}
82+
fn as_bool_lit(e: &Expr<'_>) -> Option<bool> {
83+
if let ExprKind::Lit(l) = e.kind
84+
&& let LitKind::Bool(b) = l.node
85+
{
86+
Some(b)
87+
} else {
88+
None
8289
}
8390
}

‎clippy_lints/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -595,7 +595,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
595595
store.register_late_pass(|_| Box::new(redundant_clone::RedundantClone));
596596
store.register_late_pass(|_| Box::new(slow_vector_initialization::SlowVectorInit));
597597
store.register_late_pass(move |_| Box::new(unnecessary_wraps::UnnecessaryWraps::new(conf)));
598-
store.register_late_pass(|_| Box::new(assertions_on_constants::AssertionsOnConstants));
598+
store.register_late_pass(|_| Box::new(assertions_on_constants::AssertionsOnConstants::new(conf)));
599599
store.register_late_pass(|_| Box::new(assertions_on_result_states::AssertionsOnResultStates));
600600
store.register_late_pass(|_| Box::new(inherent_to_string::InherentToString));
601601
store.register_late_pass(move |_| Box::new(trait_bounds::TraitBounds::new(conf)));

‎clippy_utils/src/msrvs.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ msrv_aliases! {
4646
1,60,0 { ABS_DIFF }
4747
1,59,0 { THREAD_LOCAL_CONST_INIT }
4848
1,58,0 { FORMAT_ARGS_CAPTURE, PATTERN_TRAIT_CHAR_ARRAY, CONST_RAW_PTR_DEREF }
49-
1,57,0 { MAP_WHILE }
49+
1,57,0 { MAP_WHILE, CONST_PANIC }
5050
1,56,0 { CONST_FN_UNION }
5151
1,55,0 { SEEK_REWIND }
5252
1,54,0 { INTO_KEYS }

‎tests/ui/assertions_on_constants.rs‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ fn main() {
4242
assert_const!(3);
4343
assert_const!(-1);
4444

45-
// Don't lint if based on `cfg!(..)`:
4645
assert!(cfg!(feature = "hey") || cfg!(not(feature = "asdf")));
46+
//~^ assertions_on_constants
4747

4848
let flag: bool = cfg!(not(feature = "asdf"));
4949
assert!(flag);
@@ -62,9 +62,25 @@ fn main() {
6262
const _: () = assert!(N.is_power_of_two());
6363
}
6464

65+
const C: bool = true;
66+
6567
const _: () = {
6668
assert!(true);
6769
//~^ assertions_on_constants
6870

6971
assert!(8 == (7 + 1));
72+
//~^ assertions_on_constants
73+
74+
assert!(C);
7075
};
76+
77+
#[clippy::msrv = "1.57"]
78+
fn _f1() {
79+
assert!(C);
80+
//~^ assertions_on_constants
81+
}
82+
83+
#[clippy::msrv = "1.56"]
84+
fn _f2() {
85+
assert!(C);
86+
}

‎tests/ui/assertions_on_constants.stderr‎

Lines changed: 50 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,100 +1,124 @@
1-
error: `assert!(true)` will be optimized out by the compiler
1+
error: this assertion is always `true`
22
--> tests/ui/assertions_on_constants.rs:10:5
33
|
44
LL | assert!(true);
55
| ^^^^^^^^^^^^^
66
|
7-
= help: remove it
7+
= help: remove the assertion
88
= note: `-D clippy::assertions-on-constants` implied by `-D warnings`
99
= help: to override `-D warnings` add `#[allow(clippy::assertions_on_constants)]`
1010

11-
error: `assert!(false)` should probably be replaced
11+
error: this assertion is always `false`
1212
--> tests/ui/assertions_on_constants.rs:13:5
1313
|
1414
LL | assert!(false);
1515
| ^^^^^^^^^^^^^^
1616
|
17-
= help: use `panic!()` or `unreachable!()`
17+
= help: replace this with `panic!()` or `unreachable!()`
1818

19-
error: `assert!(true)` will be optimized out by the compiler
19+
error: this assertion is always `true`
2020
--> tests/ui/assertions_on_constants.rs:16:5
2121
|
2222
LL | assert!(true, "true message");
2323
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2424
|
25-
= help: remove it
25+
= help: remove the assertion
2626

27-
error: `assert!(false, ..)` should probably be replaced
27+
error: this assertion is always `false`
2828
--> tests/ui/assertions_on_constants.rs:19:5
2929
|
3030
LL | assert!(false, "false message");
3131
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3232
|
33-
= help: use `panic!(..)` or `unreachable!(..)`
33+
= help: replace this with `panic!()` or `unreachable!()`
3434

35-
error: `assert!(false, ..)` should probably be replaced
35+
error: this assertion is always `false`
3636
--> tests/ui/assertions_on_constants.rs:23:5
3737
|
3838
LL | assert!(false, "{}", msg.to_uppercase());
3939
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
4040
|
41-
= help: use `panic!(..)` or `unreachable!(..)`
41+
= help: replace this with `panic!()` or `unreachable!()`
4242

43-
error: `assert!(true)` will be optimized out by the compiler
43+
error: this assertion has a constant value
4444
--> tests/ui/assertions_on_constants.rs:27:5
4545
|
4646
LL | assert!(B);
4747
| ^^^^^^^^^^
4848
|
49-
= help: remove it
49+
= help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }`
5050

51-
error: `assert!(false)` should probably be replaced
51+
error: this assertion has a constant value
5252
--> tests/ui/assertions_on_constants.rs:31:5
5353
|
5454
LL | assert!(C);
5555
| ^^^^^^^^^^
5656
|
57-
= help: use `panic!()` or `unreachable!()`
57+
= help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }`
5858

59-
error: `assert!(false, ..)` should probably be replaced
59+
error: this assertion has a constant value
6060
--> tests/ui/assertions_on_constants.rs:34:5
6161
|
6262
LL | assert!(C, "C message");
6363
| ^^^^^^^^^^^^^^^^^^^^^^^
6464
|
65-
= help: use `panic!(..)` or `unreachable!(..)`
65+
= help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }`
6666

67-
error: `debug_assert!(true)` will be optimized out by the compiler
67+
error: this assertion is always `true`
6868
--> tests/ui/assertions_on_constants.rs:37:5
6969
|
7070
LL | debug_assert!(true);
7171
| ^^^^^^^^^^^^^^^^^^^
7272
|
73-
= help: remove it
73+
= help: remove the assertion
7474

75-
error: `assert!(true)` will be optimized out by the compiler
75+
error: this assertion has a constant value
76+
--> tests/ui/assertions_on_constants.rs:45:5
77+
|
78+
LL | assert!(cfg!(feature = "hey") || cfg!(not(feature = "asdf")));
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
80+
|
81+
= help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }`
82+
83+
error: this assertion is always `true`
7684
--> tests/ui/assertions_on_constants.rs:54:19
7785
|
7886
LL | const _: () = assert!(true);
7987
| ^^^^^^^^^^^^^
8088
|
81-
= help: remove it
89+
= help: remove the assertion
8290

83-
error: `assert!(true)` will be optimized out by the compiler
91+
error: this assertion is always `true`
8492
--> tests/ui/assertions_on_constants.rs:57:5
8593
|
8694
LL | assert!(8 == (7 + 1));
8795
| ^^^^^^^^^^^^^^^^^^^^^
8896
|
89-
= help: remove it
97+
= help: remove the assertion
9098

91-
error: `assert!(true)` will be optimized out by the compiler
92-
--> tests/ui/assertions_on_constants.rs:66:5
99+
error: this assertion is always `true`
100+
--> tests/ui/assertions_on_constants.rs:68:5
93101
|
94102
LL | assert!(true);
95103
| ^^^^^^^^^^^^^
96104
|
97-
= help: remove it
105+
= help: remove the assertion
106+
107+
error: this assertion is always `true`
108+
--> tests/ui/assertions_on_constants.rs:71:5
109+
|
110+
LL | assert!(8 == (7 + 1));
111+
| ^^^^^^^^^^^^^^^^^^^^^
112+
|
113+
= help: remove the assertion
114+
115+
error: this assertion has a constant value
116+
--> tests/ui/assertions_on_constants.rs:79:5
117+
|
118+
LL | assert!(C);
119+
| ^^^^^^^^^^
120+
|
121+
= help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }`
98122

99-
error: aborting due to 12 previous errors
123+
error: aborting due to 15 previous errors
100124

0 commit comments

Comments
 (0)