Skip to content

Commit 9f1080c

Browse files
llogiqllogiq
committed
new lint: missing-spin-loop 
1 parent 28b1fe5 commit 9f1080c

12 files changed

+251
-0
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3297,6 +3297,7 @@ Released 2018-09-13
32973297
[`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items
32983298
[`missing_panics_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc
32993299
[`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
3300+
[`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop
33003301
[`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes
33013302
[`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals
33023303
[`mod_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#mod_module_files

‎clippy_lints/src/lib.register_all.rs

+1
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
109109
LintId::of(loops::ITER_NEXT_LOOP),
110110
LintId::of(loops::MANUAL_FLATTEN),
111111
LintId::of(loops::MANUAL_MEMCPY),
112+
LintId::of(loops::MISSING_SPIN_LOOP),
112113
LintId::of(loops::MUT_RANGE_BOUND),
113114
LintId::of(loops::NEEDLESS_COLLECT),
114115
LintId::of(loops::NEEDLESS_RANGE_LOOP),

‎clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ store.register_lints(&[
221221
loops::ITER_NEXT_LOOP,
222222
loops::MANUAL_FLATTEN,
223223
loops::MANUAL_MEMCPY,
224+
loops::MISSING_SPIN_LOOP,
224225
loops::MUT_RANGE_BOUND,
225226
loops::NEEDLESS_COLLECT,
226227
loops::NEEDLESS_RANGE_LOOP,

‎clippy_lints/src/lib.register_perf.rs

+1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
1010
LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
1111
LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
1212
LintId::of(loops::MANUAL_MEMCPY),
13+
LintId::of(loops::MISSING_SPIN_LOOP),
1314
LintId::of(loops::NEEDLESS_COLLECT),
1415
LintId::of(methods::EXPECT_FUN_CALL),
1516
LintId::of(methods::EXTEND_WITH_DRAIN),

‎clippy_lints/src/loops/missing_spin_loop.rs

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
use super::MISSING_SPIN_LOOP;
2+
use clippy_utils::diagnostics::span_lint_and_sugg;
3+
use clippy_utils::is_no_std_crate;
4+
use rustc_errors::Applicability;
5+
use rustc_hir::{Block, Expr, ExprKind};
6+
use rustc_lint::LateContext;
7+
use rustc_middle::ty;
8+
use rustc_span::sym;
9+
10+
fn unpack_cond<'tcx>(cond: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
11+
match &cond.kind {
12+
ExprKind::Block(
13+
Block {
14+
stmts: [],
15+
expr: Some(e),
16+
..
17+
},
18+
_,
19+
)
20+
| ExprKind::Unary(_, e) => unpack_cond(e),
21+
ExprKind::Binary(_, l, r) => {
22+
let l = unpack_cond(l);
23+
if let ExprKind::MethodCall(..) = l.kind {
24+
l
25+
} else {
26+
unpack_cond(r)
27+
}
28+
},
29+
_ => cond,
30+
}
31+
}
32+
33+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>) {
34+
if_chain! {
35+
if let ExprKind::Block(Block { stmts: [], expr: None, ..}, _) = body.kind;
36+
if let ExprKind::MethodCall(method, [callee, ..], _) = unpack_cond(cond).kind;
37+
if [sym::load, sym::compare_exchange, sym::compare_exchange_weak].contains(&method.ident.name);
38+
if let ty::Adt(def, _substs) = cx.typeck_results().expr_ty(callee).kind();
39+
if cx.tcx.is_diagnostic_item(sym::AtomicBool, def.did);
40+
then {
41+
span_lint_and_sugg(
42+
cx,
43+
MISSING_SPIN_LOOP,
44+
body.span,
45+
"busy-waiting loop should at least have a spin loop hint",
46+
"try this",
47+
(if is_no_std_crate(cx) {
48+
"{ core::hint::spin_loop() }"
49+
} else {
50+
"{ std::hint::spin_loop() }"
51+
}).into(),
52+
Applicability::MachineApplicable
53+
);
54+
}
55+
}
56+
}

‎clippy_lints/src/loops/mod.rs

+39
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ mod for_loops_over_fallibles;
77
mod iter_next_loop;
88
mod manual_flatten;
99
mod manual_memcpy;
10+
mod missing_spin_loop;
1011
mod mut_range_bound;
1112
mod needless_collect;
1213
mod needless_range_loop;
@@ -560,6 +561,42 @@ declare_clippy_lint! {
560561
"for loops over `Option`s or `Result`s with a single expression can be simplified"
561562
}
562563

564+
declare_clippy_lint! {
565+
/// ### What it does
566+
/// Check for empty spin loops
567+
///
568+
/// ### Why is this bad?
569+
/// The loop body should have something like `thread::park()` or at least
570+
/// `std::hint::spin_loop()` to avoid needlessly burning cycles and conserve
571+
/// energy. Perhaps even better use an actual lock, if possible.
572+
///
573+
/// ### Known problems
574+
/// This lint doesn't currently trigger on `while let` or
575+
/// `loop { match .. { .. } }` loops, which would be considered idiomatic in
576+
/// combination with e.g. `AtomicBool::compare_exchange_weak`.
577+
///
578+
/// ### Example
579+
///
580+
/// ```ignore
581+
/// use core::sync::atomic::{AtomicBool, Ordering};
582+
/// let b = AtomicBool::new(true);
583+
/// // give a ref to `b` to another thread,wait for it to become false
584+
/// while b.load(Ordering::Acquire) {};
585+
/// ```
586+
/// Use instead:
587+
/// ```rust,no_run
588+
///# use core::sync::atomic::{AtomicBool, Ordering};
589+
///# let b = AtomicBool::new(true);
590+
/// while b.load(Ordering::Acquire) {
591+
/// std::hint::spin_loop()
592+
/// }
593+
/// ```
594+
#[clippy::version = "1.59.0"]
595+
pub MISSING_SPIN_LOOP,
596+
perf,
597+
"An empty busy waiting loop"
598+
}
599+
563600
declare_lint_pass!(Loops => [
564601
MANUAL_MEMCPY,
565602
MANUAL_FLATTEN,
@@ -579,6 +616,7 @@ declare_lint_pass!(Loops => [
579616
WHILE_IMMUTABLE_CONDITION,
580617
SAME_ITEM_PUSH,
581618
SINGLE_ELEMENT_LOOP,
619+
MISSING_SPIN_LOOP,
582620
]);
583621

584622
impl<'tcx> LateLintPass<'tcx> for Loops {
@@ -628,6 +666,7 @@ impl<'tcx> LateLintPass<'tcx> for Loops {
628666

629667
if let Some(higher::While { condition, body }) = higher::While::hir(expr) {
630668
while_immutable_condition::check(cx, condition, body);
669+
missing_spin_loop::check(cx, condition, body);
631670
}
632671

633672
needless_collect::check(expr, cx);

‎tests/ui/missing_spin_loop.fixed

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// run-rustfix
2+
#![warn(clippy::missing_spin_loop)]
3+
#![allow(clippy::bool_comparison)]
4+
#![allow(unused_braces)]
5+
6+
use core::sync::atomic::{AtomicBool, Ordering};
7+
8+
fn main() {
9+
let b = AtomicBool::new(true);
10+
// Those should lint
11+
while b.load(Ordering::Acquire) { std::hint::spin_loop() }
12+
13+
while !b.load(Ordering::SeqCst) { std::hint::spin_loop() }
14+
15+
while b.load(Ordering::Acquire) == false { std::hint::spin_loop() }
16+
17+
while { true == b.load(Ordering::Acquire) } { std::hint::spin_loop() }
18+
19+
while b.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed) != Ok(true) { std::hint::spin_loop() }
20+
21+
while Ok(false) != b.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) { std::hint::spin_loop() }
22+
23+
// This is OK, as the body is not empty
24+
while b.load(Ordering::Acquire) {
25+
std::hint::spin_loop()
26+
}
27+
// TODO: also match on loop+match or while let
28+
}

‎tests/ui/missing_spin_loop.rs

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// run-rustfix
2+
#![warn(clippy::missing_spin_loop)]
3+
#![allow(clippy::bool_comparison)]
4+
#![allow(unused_braces)]
5+
6+
use core::sync::atomic::{AtomicBool, Ordering};
7+
8+
fn main() {
9+
let b = AtomicBool::new(true);
10+
// Those should lint
11+
while b.load(Ordering::Acquire) {}
12+
13+
while !b.load(Ordering::SeqCst) {}
14+
15+
while b.load(Ordering::Acquire) == false {}
16+
17+
while { true == b.load(Ordering::Acquire) } {}
18+
19+
while b.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed) != Ok(true) {}
20+
21+
while Ok(false) != b.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) {}
22+
23+
// This is OK, as the body is not empty
24+
while b.load(Ordering::Acquire) {
25+
std::hint::spin_loop()
26+
}
27+
// TODO: also match on loop+match or while let
28+
}

‎tests/ui/missing_spin_loop.stderr

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
error: busy-waiting loop should at least have a spin loop hint
2+
--> $DIR/missing_spin_loop.rs:11:37
3+
|
4+
LL | while b.load(Ordering::Acquire) {}
5+
| ^^ help: try this: `{ std::hint::spin_loop() }`
6+
|
7+
= note: `-D clippy::missing-spin-loop` implied by `-D warnings`
8+
9+
error: busy-waiting loop should at least have a spin loop hint
10+
--> $DIR/missing_spin_loop.rs:13:37
11+
|
12+
LL | while !b.load(Ordering::SeqCst) {}
13+
| ^^ help: try this: `{ std::hint::spin_loop() }`
14+
15+
error: busy-waiting loop should at least have a spin loop hint
16+
--> $DIR/missing_spin_loop.rs:15:46
17+
|
18+
LL | while b.load(Ordering::Acquire) == false {}
19+
| ^^ help: try this: `{ std::hint::spin_loop() }`
20+
21+
error: busy-waiting loop should at least have a spin loop hint
22+
--> $DIR/missing_spin_loop.rs:17:49
23+
|
24+
LL | while { true == b.load(Ordering::Acquire) } {}
25+
| ^^ help: try this: `{ std::hint::spin_loop() }`
26+
27+
error: busy-waiting loop should at least have a spin loop hint
28+
--> $DIR/missing_spin_loop.rs:19:93
29+
|
30+
LL | while b.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed) != Ok(true) {}
31+
| ^^ help: try this: `{ std::hint::spin_loop() }`
32+
33+
error: busy-waiting loop should at least have a spin loop hint
34+
--> $DIR/missing_spin_loop.rs:21:94
35+
|
36+
LL | while Ok(false) != b.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) {}
37+
| ^^ help: try this: `{ std::hint::spin_loop() }`
38+
39+
error: aborting due to 6 previous errors
40+

‎tests/ui/missing_spin_loop_no_std.fixed

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// run-rustfix
2+
#![warn(clippy::missing_spin_loop)]
3+
#![feature(lang_items, start, libc)]
4+
#![no_std]
5+
6+
use core::sync::atomic::{AtomicBool, Ordering};
7+
8+
#[start]
9+
fn main(_argc: isize, _argv: *const *const u8) -> isize {
10+
// This should trigger the lint
11+
let b = AtomicBool::new(true);
12+
// This should lint with `core::hint::spin_loop()`
13+
while b.load(Ordering::Acquire) { core::hint::spin_loop() }
14+
0
15+
}
16+
17+
#[panic_handler]
18+
fn panic(_info: &core::panic::PanicInfo) -> ! {
19+
loop {}
20+
}
21+
22+
#[lang = "eh_personality"]
23+
extern "C" fn eh_personality() {}

‎tests/ui/missing_spin_loop_no_std.rs

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// run-rustfix
2+
#![warn(clippy::missing_spin_loop)]
3+
#![feature(lang_items, start, libc)]
4+
#![no_std]
5+
6+
use core::sync::atomic::{AtomicBool, Ordering};
7+
8+
#[start]
9+
fn main(_argc: isize, _argv: *const *const u8) -> isize {
10+
// This should trigger the lint
11+
let b = AtomicBool::new(true);
12+
// This should lint with `core::hint::spin_loop()`
13+
while b.load(Ordering::Acquire) {}
14+
0
15+
}
16+
17+
#[panic_handler]
18+
fn panic(_info: &core::panic::PanicInfo) -> ! {
19+
loop {}
20+
}
21+
22+
#[lang = "eh_personality"]
23+
extern "C" fn eh_personality() {}

‎tests/ui/missing_spin_loop_no_std.stderr

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
error: busy-waiting loop should at least have a spin loop hint
2+
--> $DIR/missing_spin_loop_no_std.rs:13:37
3+
|
4+
LL | while b.load(Ordering::Acquire) {}
5+
| ^^ help: try this: `{ core::hint::spin_loop() }`
6+
|
7+
= note: `-D clippy::missing-spin-loop` implied by `-D warnings`
8+
9+
error: aborting due to previous error
10+

0 commit comments

Comments
 (0)