Auto merge of #8174 - rust-lang:missing-spin-loop, r=flip1995

new lint: `missing-spin-loop`

This fixes #7809. I went with the shorter name because the function is called `std::hint::spin_loop`. It doesn't yet detect `while let` loops. I left that for a follow-up PR.

---

changelog: new lint: [`missing_spin_loop`]

Author: bors

CHANGELOG.md

@@ -3297,6 +3297,7 @@ Released 2018-09-13
 [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items
 [`missing_panics_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc
 [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
+[`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop
 [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes
 [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals
 [`mod_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#mod_module_files

clippy_lints/src/lib.register_all.rs

@@ -109,6 +109,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(loops::ITER_NEXT_LOOP),
     LintId::of(loops::MANUAL_FLATTEN),
     LintId::of(loops::MANUAL_MEMCPY),
+    LintId::of(loops::MISSING_SPIN_LOOP),
     LintId::of(loops::MUT_RANGE_BOUND),
     LintId::of(loops::NEEDLESS_COLLECT),
     LintId::of(loops::NEEDLESS_RANGE_LOOP),

clippy_lints/src/lib.register_lints.rs

@@ -221,6 +221,7 @@ store.register_lints(&[
     loops::ITER_NEXT_LOOP,
     loops::MANUAL_FLATTEN,
     loops::MANUAL_MEMCPY,
+    loops::MISSING_SPIN_LOOP,
     loops::MUT_RANGE_BOUND,
     loops::NEEDLESS_COLLECT,
     loops::NEEDLESS_RANGE_LOOP,

clippy_lints/src/lib.register_perf.rs

@@ -10,6 +10,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
     LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
     LintId::of(loops::MANUAL_MEMCPY),
+    LintId::of(loops::MISSING_SPIN_LOOP),
     LintId::of(loops::NEEDLESS_COLLECT),
     LintId::of(methods::EXPECT_FUN_CALL),
     LintId::of(methods::EXTEND_WITH_DRAIN),

clippy_lints/src/loops/missing_spin_loop.rs

@@ -0,0 +1,56 @@
+use super::MISSING_SPIN_LOOP;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_no_std_crate;
+use rustc_errors::Applicability;
+use rustc_hir::{Block, Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use rustc_span::sym;
+
+fn unpack_cond<'tcx>(cond: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
+    match &cond.kind {
+        ExprKind::Block(
+            Block {
+                stmts: [],
+                expr: Some(e),
+                ..
+            },
+            _,
+        )
+        | ExprKind::Unary(_, e) => unpack_cond(e),
+        ExprKind::Binary(_, l, r) => {
+            let l = unpack_cond(l);
+            if let ExprKind::MethodCall(..) = l.kind {
+                l
+            } else {
+                unpack_cond(r)
+            }
+        },
+        _ => cond,
+    }
+}
+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>) {
+    if_chain! {
+        if let ExprKind::Block(Block { stmts: [], expr: None, ..}, _) = body.kind;
+        if let ExprKind::MethodCall(method, [callee, ..], _) = unpack_cond(cond).kind;
+        if [sym::load, sym::compare_exchange, sym::compare_exchange_weak].contains(&method.ident.name);
+        if let ty::Adt(def, _substs) = cx.typeck_results().expr_ty(callee).kind();
+        if cx.tcx.is_diagnostic_item(sym::AtomicBool, def.did);
+        then {
+            span_lint_and_sugg(
+                cx,
+                MISSING_SPIN_LOOP,
+                body.span,
+                "busy-waiting loop should at least have a spin loop hint",
+                "try this",
+                (if is_no_std_crate(cx) {
+                    "{ core::hint::spin_loop() }"
+                } else {
+                    "{ std::hint::spin_loop() }"
+                }).into(),
+                Applicability::MachineApplicable
+            );
+        }
+    }
+}

clippy_lints/src/loops/mod.rs

@@ -7,6 +7,7 @@ mod for_loops_over_fallibles;
 mod iter_next_loop;
 mod manual_flatten;
 mod manual_memcpy;
+mod missing_spin_loop;
 mod mut_range_bound;
 mod needless_collect;
 mod needless_range_loop;
@@ -560,6 +561,42 @@ declare_clippy_lint! {
     "for loops over `Option`s or `Result`s with a single expression can be simplified"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Check for empty spin loops
+    ///
+    /// ### Why is this bad?
+    /// The loop body should have something like `thread::park()` or at least
+    /// `std::hint::spin_loop()` to avoid needlessly burning cycles and conserve
+    /// energy. Perhaps even better use an actual lock, if possible.
+    ///
+    /// ### Known problems
+    /// This lint doesn't currently trigger on `while let` or
+    /// `loop { match .. { .. } }` loops, which would be considered idiomatic in
+    /// combination with e.g. `AtomicBool::compare_exchange_weak`.
+    ///
+    /// ### Example
+    ///
+    /// ```ignore
+    /// use core::sync::atomic::{AtomicBool, Ordering};
+    /// let b = AtomicBool::new(true);
+    /// // give a ref to `b` to another thread,wait for it to become false
+    /// while b.load(Ordering::Acquire) {};
+    /// ```
+    /// Use instead:
+    /// ```rust,no_run
+    ///# use core::sync::atomic::{AtomicBool, Ordering};
+    ///# let b = AtomicBool::new(true);
+    /// while b.load(Ordering::Acquire) {
+    ///     std::hint::spin_loop()
+    /// }
+    /// ```
+    #[clippy::version = "1.59.0"]
+    pub MISSING_SPIN_LOOP,
+    perf,
+    "An empty busy waiting loop"
+}
+
 declare_lint_pass!(Loops => [
     MANUAL_MEMCPY,
     MANUAL_FLATTEN,
@@ -579,6 +616,7 @@ declare_lint_pass!(Loops => [
     WHILE_IMMUTABLE_CONDITION,
     SAME_ITEM_PUSH,
     SINGLE_ELEMENT_LOOP,
+    MISSING_SPIN_LOOP,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Loops {
@@ -628,6 +666,7 @@ impl<'tcx> LateLintPass<'tcx> for Loops {
 
         if let Some(higher::While { condition, body }) = higher::While::hir(expr) {
             while_immutable_condition::check(cx, condition, body);
+            missing_spin_loop::check(cx, condition, body);
         }
 
         needless_collect::check(expr, cx);

tests/ui/missing_spin_loop.fixed

@@ -0,0 +1,28 @@
+// run-rustfix
+#![warn(clippy::missing_spin_loop)]
+#![allow(clippy::bool_comparison)]
+#![allow(unused_braces)]
+
+use core::sync::atomic::{AtomicBool, Ordering};
+
+fn main() {
+    let b = AtomicBool::new(true);
+    // Those should lint
+    while b.load(Ordering::Acquire) { std::hint::spin_loop() }
+
+    while !b.load(Ordering::SeqCst) { std::hint::spin_loop() }
+
+    while b.load(Ordering::Acquire) == false { std::hint::spin_loop() }
+
+    while { true == b.load(Ordering::Acquire) } { std::hint::spin_loop() }
+
+    while b.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed) != Ok(true) { std::hint::spin_loop() }
+
+    while Ok(false) != b.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) { std::hint::spin_loop() }
+
+    // This is OK, as the body is not empty
+    while b.load(Ordering::Acquire) {
+        std::hint::spin_loop()
+    }
+    // TODO: also match on loop+match or while let
+}

tests/ui/missing_spin_loop.rs

@@ -0,0 +1,28 @@
+// run-rustfix
+#![warn(clippy::missing_spin_loop)]
+#![allow(clippy::bool_comparison)]
+#![allow(unused_braces)]
+
+use core::sync::atomic::{AtomicBool, Ordering};
+
+fn main() {
+    let b = AtomicBool::new(true);
+    // Those should lint
+    while b.load(Ordering::Acquire) {}
+
+    while !b.load(Ordering::SeqCst) {}
+
+    while b.load(Ordering::Acquire) == false {}
+
+    while { true == b.load(Ordering::Acquire) } {}
+
+    while b.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed) != Ok(true) {}
+
+    while Ok(false) != b.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) {}
+
+    // This is OK, as the body is not empty
+    while b.load(Ordering::Acquire) {
+        std::hint::spin_loop()
+    }
+    // TODO: also match on loop+match or while let
+}

tests/ui/missing_spin_loop.stderr

@@ -0,0 +1,40 @@
+error: busy-waiting loop should at least have a spin loop hint
+  --> $DIR/missing_spin_loop.rs:11:37
+   |
+LL |     while b.load(Ordering::Acquire) {}
+   |                                     ^^ help: try this: `{ std::hint::spin_loop() }`
+   |
+   = note: `-D clippy::missing-spin-loop` implied by `-D warnings`
+
+error: busy-waiting loop should at least have a spin loop hint
+  --> $DIR/missing_spin_loop.rs:13:37
+   |
+LL |     while !b.load(Ordering::SeqCst) {}
+   |                                     ^^ help: try this: `{ std::hint::spin_loop() }`
+
+error: busy-waiting loop should at least have a spin loop hint
+  --> $DIR/missing_spin_loop.rs:15:46
+   |
+LL |     while b.load(Ordering::Acquire) == false {}
+   |                                              ^^ help: try this: `{ std::hint::spin_loop() }`
+
+error: busy-waiting loop should at least have a spin loop hint
+  --> $DIR/missing_spin_loop.rs:17:49
+   |
+LL |     while { true == b.load(Ordering::Acquire) } {}
+   |                                                 ^^ help: try this: `{ std::hint::spin_loop() }`
+
+error: busy-waiting loop should at least have a spin loop hint
+  --> $DIR/missing_spin_loop.rs:19:93
+   |
+LL |     while b.compare_exchange(true, false, Ordering::Acquire, Ordering::Relaxed) != Ok(true) {}
+   |                                                                                             ^^ help: try this: `{ std::hint::spin_loop() }`
+
+error: busy-waiting loop should at least have a spin loop hint
+  --> $DIR/missing_spin_loop.rs:21:94
+   |
+LL |     while Ok(false) != b.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) {}
+   |                                                                                              ^^ help: try this: `{ std::hint::spin_loop() }`
+
+error: aborting due to 6 previous errors
+

tests/ui/missing_spin_loop_no_std.fixed

@@ -0,0 +1,23 @@
+// run-rustfix
+#![warn(clippy::missing_spin_loop)]
+#![feature(lang_items, start, libc)]
+#![no_std]
+
+use core::sync::atomic::{AtomicBool, Ordering};
+
+#[start]
+fn main(_argc: isize, _argv: *const *const u8) -> isize {
+    // This should trigger the lint
+    let b = AtomicBool::new(true);
+    // This should lint with `core::hint::spin_loop()`
+    while b.load(Ordering::Acquire) { core::hint::spin_loop() }
+    0
+}
+
+#[panic_handler]
+fn panic(_info: &core::panic::PanicInfo) -> ! {
+    loop {}
+}
+
+#[lang = "eh_personality"]
+extern "C" fn eh_personality() {}

tests/ui/missing_spin_loop_no_std.rs

@@ -0,0 +1,23 @@
+// run-rustfix
+#![warn(clippy::missing_spin_loop)]
+#![feature(lang_items, start, libc)]
+#![no_std]
+
+use core::sync::atomic::{AtomicBool, Ordering};
+
+#[start]
+fn main(_argc: isize, _argv: *const *const u8) -> isize {
+    // This should trigger the lint
+    let b = AtomicBool::new(true);
+    // This should lint with `core::hint::spin_loop()`
+    while b.load(Ordering::Acquire) {}
+    0
+}
+
+#[panic_handler]
+fn panic(_info: &core::panic::PanicInfo) -> ! {
+    loop {}
+}
+
+#[lang = "eh_personality"]
+extern "C" fn eh_personality() {}

tests/ui/missing_spin_loop_no_std.stderr

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