Detect fetch_update misuse in invalid_atomic_ordering too

Author: thomcc

clippy_lints/src/atomic_ordering.rs

@@ -8,7 +8,8 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for usage of invalid atomic
-    /// ordering in atomic loads/stores/exchanges and memory fences
+    /// ordering in atomic loads/stores/exchanges/updates and
+    /// memory fences.
     ///
     /// **Why is this bad?** Using an invalid atomic ordering
     /// will cause a panic at run-time.
@@ -32,10 +33,11 @@ declare_clippy_lint! {
     ///
     /// let _ = x.compare_exchange(false, false, Ordering::Relaxed, Ordering::SeqCst);
     /// let _ = x.compare_exchange_weak(false, true, Ordering::SeqCst, Ordering::Release);
+    /// let _ = x.fetch_update(Ordering::AcqRel, Ordering::AcqRel, |val| Some(val ^ val));
     /// ```
     pub INVALID_ATOMIC_ORDERING,
     correctness,
-    "usage of invalid atomic ordering in atomic loads/stores/exchanges ane memory fences"
+    "usage of invalid atomic ordering in atomic operations and memory fences"
 }
 
 declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]);
@@ -142,8 +144,12 @@ fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) {
         if let ExprKind::MethodCall(ref method_path, _, args, _) = &expr.kind;
         let method = method_path.ident.name.as_str();
         if type_is_atomic(cx, &args[0]);
-        if method == "compare_exchange" || method == "compare_exchange_weak";
-        let failure_order_arg = &args[4];
+        if method == "compare_exchange" || method == "compare_exchange_weak" || method == "fetch_update";
+        let (success_order_arg, failure_order_arg) = if method == "fetch_update" {
+            (&args[1], &args[2])
+        } else {
+            (&args[3], &args[4])
+        };
         if let Some(fail_ordering_def_id) = opt_ordering_defid(cx, failure_order_arg);
         then {
             // Helper type holding on to some checking and error reporting data. Has
@@ -158,7 +164,7 @@ fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) {
             let acqrel = ("AcqRel", acquire.1, acquire.2);
             let search = [relaxed, acquire, seq_cst, release, acqrel];
 
-            let success_lint_info = opt_ordering_defid(cx, &args[3])
+            let success_lint_info = opt_ordering_defid(cx, success_order_arg)
                 .and_then(|success_ord_def_id| -> Option<OrdLintInfo> {
                     search
                         .iter()

src/lintlist/mod.rs

@@ -923,7 +923,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
     Lint {
         name: "invalid_atomic_ordering",
         group: "correctness",
-        desc: "usage of invalid atomic ordering in atomic loads/stores and memory fences",
+        desc: "usage of invalid atomic ordering in atomic operations and memory fences",
         deprecation: None,
         module: "atomic_ordering",
     },

tests/ui/atomic_ordering_fetch_update.rs

@@ -0,0 +1,45 @@
+#![warn(clippy::invalid_atomic_ordering)]
+
+use std::sync::atomic::{AtomicIsize, Ordering};
+
+fn main() {
+    // `fetch_update` testing
+    let x = AtomicIsize::new(0);
+
+    // Allowed ordering combos
+    let _ = x.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Acquire, Ordering::Acquire, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Acquire, Ordering::Relaxed, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Release, Ordering::Relaxed, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::AcqRel, Ordering::Acquire, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::AcqRel, Ordering::Relaxed, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::SeqCst, Ordering::Relaxed, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::SeqCst, Ordering::Acquire, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::SeqCst, Ordering::SeqCst, |old| Some(old + 1));
+
+    // AcqRel is always forbidden as a failure ordering
+    let _ = x.fetch_update(Ordering::Relaxed, Ordering::AcqRel, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Acquire, Ordering::AcqRel, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Release, Ordering::AcqRel, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::AcqRel, Ordering::AcqRel, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::SeqCst, Ordering::AcqRel, |old| Some(old + 1));
+
+    // Release is always forbidden as a failure ordering
+    let _ = x.fetch_update(Ordering::Relaxed, Ordering::Release, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Acquire, Ordering::Release, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Release, Ordering::Release, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::AcqRel, Ordering::Release, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::SeqCst, Ordering::Release, |old| Some(old + 1));
+
+    // Release success order forbids failure order of Acquire or SeqCst
+    let _ = x.fetch_update(Ordering::Release, Ordering::Acquire, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Release, Ordering::SeqCst, |old| Some(old + 1));
+
+    // Relaxed success order also forbids failure order of Acquire or SeqCst
+    let _ = x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::Relaxed, Ordering::Acquire, |old| Some(old + 1));
+
+    // Acquire/AcqRel forbids failure order of SeqCst
+    let _ = x.fetch_update(Ordering::Acquire, Ordering::SeqCst, |old| Some(old + 1));
+    let _ = x.fetch_update(Ordering::AcqRel, Ordering::SeqCst, |old| Some(old + 1));
+}

tests/ui/atomic_ordering_fetch_update.stderr

@@ -0,0 +1,131 @@
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:21:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Relaxed, Ordering::AcqRel, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::invalid-atomic-ordering` implied by `-D warnings`
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:22:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Acquire, Ordering::AcqRel, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:23:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Release, Ordering::AcqRel, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:24:46
+   |
+LL |     let _ = x.fetch_update(Ordering::AcqRel, Ordering::AcqRel, |old| Some(old + 1));
+   |                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:25:46
+   |
+LL |     let _ = x.fetch_update(Ordering::SeqCst, Ordering::AcqRel, |old| Some(old + 1));
+   |                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:28:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Relaxed, Ordering::Release, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:29:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Acquire, Ordering::Release, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:30:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Release, Ordering::Release, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:31:46
+   |
+LL |     let _ = x.fetch_update(Ordering::AcqRel, Ordering::Release, |old| Some(old + 1));
+   |                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:32:46
+   |
+LL |     let _ = x.fetch_update(Ordering::SeqCst, Ordering::Release, |old| Some(old + 1));
+   |                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_fetch_update.rs:35:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Release, Ordering::Acquire, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_fetch_update.rs:36:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Release, Ordering::SeqCst, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_fetch_update.rs:39:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_fetch_update.rs:40:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Relaxed, Ordering::Acquire, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: fetch_update's failure ordering may not stronger than the success ordering of `Acquire`
+  --> $DIR/atomic_ordering_fetch_update.rs:43:47
+   |
+LL |     let _ = x.fetch_update(Ordering::Acquire, Ordering::SeqCst, |old| Some(old + 1));
+   |                                               ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: fetch_update's failure ordering may not stronger than the success ordering of `AcqRel`
+  --> $DIR/atomic_ordering_fetch_update.rs:44:46
+   |
+LL |     let _ = x.fetch_update(Ordering::AcqRel, Ordering::SeqCst, |old| Some(old + 1));
+   |                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: aborting due to 16 previous errors
+