Skip to content

Commit 2d24b01

Browse files
JarchoJarcho
committed
assertions_on_constants: Don't suggest removing assertions with non-local constants.
1 parent 3c93ba0 commit 2d24b01

File tree

5 files changed

+116
-79
lines changed

5 files changed

+116
-79
lines changed

‎clippy_lints/src/assertions_on_constants.rs‎

Lines changed: 54 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,59 @@ 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)) = ConstEvalCtxt::new(cx).eval_with_source(condition, )
54+
&& let in_const_context = is_inside_always_const_context(cx.tcx, e.hir_id)
55+
&& (const_src.is_local() || !in_const_context)
56+
&& !(is_debug && as_bool_lit(condition) == Some(false))
57+
{
58+
let (msg, help) = if !const_src.is_local() {
59+
if self.msrv.meets(cx, msrvs::CONST_PANIC) {
60+
(
61+
"this assertion has a constant value",
62+
"consider moving this to an anonymous constant: `const _: () = { assert!(..); }`",
63+
)
64+
} else {
65+
return;
66+
}
67+
} else if assert_val {
68+
("this assertion is always `true`", "remove the assertion")
69+
} else {
70+
(
71+
"this assertion is always `false`",
72+
"replace this with `panic!()` or `unreachable!()`",
73+
)
74+
};
4975

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

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-
}
81+
fn as_bool_lit(e: &Expr<'_>) -> Option<bool> {
82+
if let ExprKind::Lit(l) = e.kind
83+
&& let LitKind::Bool(b) = l.node
84+
{
85+
Some(b)
86+
} else {
87+
None
8288
}
8389
}

‎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: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ 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")));
4746

4847
let flag: bool = cfg!(not(feature = "asdf"));
@@ -62,9 +61,25 @@ fn main() {
6261
const _: () = assert!(N.is_power_of_two());
6362
}
6463

64+
const C: bool = true;
65+
6566
const _: () = {
6667
assert!(true);
6768
//~^ assertions_on_constants
6869

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

‎tests/ui/assertions_on_constants.stderr‎

Lines changed: 44 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,100 +1,116 @@
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
76-
--> tests/ui/assertions_on_constants.rs:54:19
75+
error: this assertion is always `true`
76+
--> tests/ui/assertions_on_constants.rs:53:19
7777
|
7878
LL | const _: () = assert!(true);
7979
| ^^^^^^^^^^^^^
8080
|
81-
= help: remove it
81+
= help: remove the assertion
8282

83-
error: `assert!(true)` will be optimized out by the compiler
84-
--> tests/ui/assertions_on_constants.rs:57:5
83+
error: this assertion is always `true`
84+
--> tests/ui/assertions_on_constants.rs:56:5
8585
|
8686
LL | assert!(8 == (7 + 1));
8787
| ^^^^^^^^^^^^^^^^^^^^^
8888
|
89-
= help: remove it
89+
= help: remove the assertion
9090

91-
error: `assert!(true)` will be optimized out by the compiler
92-
--> tests/ui/assertions_on_constants.rs:66:5
91+
error: this assertion is always `true`
92+
--> tests/ui/assertions_on_constants.rs:67:5
9393
|
9494
LL | assert!(true);
9595
| ^^^^^^^^^^^^^
9696
|
97-
= help: remove it
97+
= help: remove the assertion
9898

99-
error: aborting due to 12 previous errors
99+
error: this assertion is always `true`
100+
--> tests/ui/assertions_on_constants.rs:70:5
101+
|
102+
LL | assert!(8 == (7 + 1));
103+
| ^^^^^^^^^^^^^^^^^^^^^
104+
|
105+
= help: remove the assertion
106+
107+
error: this assertion has a constant value
108+
--> tests/ui/assertions_on_constants.rs:78:5
109+
|
110+
LL | assert!(C);
111+
| ^^^^^^^^^^
112+
|
113+
= help: consider moving this to an anonymous constant: `const _: () = { assert!(..); }`
114+
115+
error: aborting due to 14 previous errors
100116

0 commit comments

Comments
 (0)