diff --git a/CHANGELOG.md b/CHANGELOG.md index 27bac4718b6c..000aa424e98e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3120,6 +3120,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 diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 944411087e95..1e1cc4cc02e6 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -102,6 +102,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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 002122793f3b..ae297aff0896 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -212,6 +212,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, diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs index 2ea0b696f1fe..76b0ca541b64 100644 --- a/clippy_lints/src/lib.register_perf.rs +++ b/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), diff --git a/clippy_lints/src/loops/missing_spin_loop.rs b/clippy_lints/src/loops/missing_spin_loop.rs new file mode 100644 index 000000000000..2164ff526975 --- /dev/null +++ b/clippy_lints/src/loops/missing_spin_loop.rs @@ -0,0 +1,33 @@ +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; + +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, _ordering], _) = cond.kind; + if method.ident.name == sym::load; + //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 + ); + } + } +} diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index e2f9aee063dd..4b4ad686948e 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/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,37 @@ 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. + /// + /// ### 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 +611,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 +661,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); diff --git a/tests/ui/missing_spin_loop.fixed b/tests/ui/missing_spin_loop.fixed new file mode 100644 index 000000000000..097f99062ee9 --- /dev/null +++ b/tests/ui/missing_spin_loop.fixed @@ -0,0 +1,17 @@ +// run-rustfix +#![warn(clippy::missing_spin_loop)] + +use core::sync::atomic::{AtomicBool, Ordering}; + +fn main() { + let b = AtomicBool::new(true); + // This should lint + while b.load(Ordering::Acquire) { 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 compare_exchange(_weak), but that could be + // loop+match or while let +} diff --git a/tests/ui/missing_spin_loop.rs b/tests/ui/missing_spin_loop.rs new file mode 100644 index 000000000000..b47fadb46226 --- /dev/null +++ b/tests/ui/missing_spin_loop.rs @@ -0,0 +1,17 @@ +// run-rustfix +#![warn(clippy::missing_spin_loop)] + +use core::sync::atomic::{AtomicBool, Ordering}; + +fn main() { + let b = AtomicBool::new(true); + // This should lint + while b.load(Ordering::Acquire) {} + + // This is OK, as the body is not empty + while b.load(Ordering::Acquire) { + std::hint::spin_loop() + } + // TODO: also match on compare_exchange(_weak), but that could be + // loop+match or while let +} diff --git a/tests/ui/missing_spin_loop.stderr b/tests/ui/missing_spin_loop.stderr new file mode 100644 index 000000000000..ccd05919f119 --- /dev/null +++ b/tests/ui/missing_spin_loop.stderr @@ -0,0 +1,10 @@ +error: busy-waiting loop should at least have a spin loop hint + --> $DIR/missing_spin_loop.rs:9: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: aborting due to previous error + diff --git a/tests/ui/missing_spin_loop_no_std.fixed b/tests/ui/missing_spin_loop_no_std.fixed new file mode 100644 index 000000000000..bb4b4795516e --- /dev/null +++ b/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() {} diff --git a/tests/ui/missing_spin_loop_no_std.rs b/tests/ui/missing_spin_loop_no_std.rs new file mode 100644 index 000000000000..a19bc72baf8d --- /dev/null +++ b/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() {} diff --git a/tests/ui/missing_spin_loop_no_std.stderr b/tests/ui/missing_spin_loop_no_std.stderr new file mode 100644 index 000000000000..2b3b6873c3c4 --- /dev/null +++ b/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 +