Extend invalid_atomic_ordering to detect misuse of compare_exchange{,_weak}

clippy_lints/src/atomic_ordering.rs

@@ -8,7 +8,7 @@ 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 and memory fences.
+    /// ordering in atomic loads/stores/exchanges and memory fences
     ///
     /// **Why is this bad?** Using an invalid atomic ordering
     /// will cause a panic at run-time.
@@ -29,10 +29,13 @@ declare_clippy_lint! {
     ///
     /// atomic::fence(Ordering::Relaxed);
     /// atomic::compiler_fence(Ordering::Relaxed);
+    ///
+    /// let _ = x.compare_exchange(false, false, Ordering::Relaxed, Ordering::SeqCst);
+    /// let _ = x.compare_exchange_weak(false, true, Ordering::SeqCst, Ordering::Release);
     /// ```
     pub INVALID_ATOMIC_ORDERING,
     correctness,
-    "usage of invalid atomic ordering in atomic loads/stores and memory fences"
+    "usage of invalid atomic ordering in atomic loads/stores/exchanges ane memory fences"
 }
 
 declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]);
@@ -127,9 +130,84 @@ fn check_memory_fence(cx: &LateContext<'_>, expr: &Expr<'_>) {
     }
 }
 
+fn opt_ordering_defid(cx: &LateContext<'_>, ord_arg: &Expr<'_>) -> Option<DefId> {
+    if let ExprKind::Path(ref ord_qpath) = ord_arg.kind {
+        cx.qpath_res(ord_qpath, ord_arg.hir_id).opt_def_id()
+    } else {
+        None
+    }
+}
+fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) {
+    if_chain! {
+        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 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
+            // - (success ordering name,
+            // - list of failure orderings forbidden by the success order,
+            // - suggestion message)
+            type OrdLintInfo = (&'static str, &'static [&'static str], &'static str);
+            let relaxed: OrdLintInfo = ("Relaxed", &["SeqCst", "Acquire"], "ordering mode `Relaxed`");
+            let acquire: OrdLintInfo = ("Acquire", &["SeqCst"], "ordering modes `Acquire` or `Relaxed`");
+            let seq_cst: OrdLintInfo = ("SeqCst", &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`");
+            let release = ("Release", relaxed.1, relaxed.2);
+            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])
+                .and_then(|success_ord_def_id| -> Option<OrdLintInfo> {
+                    search
+                        .iter()
+                        .find(|(ordering, ..)| {
+                            match_def_path(cx, success_ord_def_id,
+                                &["core", "sync", "atomic", "Ordering", ordering])
+                        })
+                        .copied()
+                });
+
+            if match_ordering_def_path(cx, fail_ordering_def_id, &["Release", "AcqRel"]) {
+                // If we don't know the success order is, use what we'd suggest
+                // if it were maximally permissive.
+                let suggested = success_lint_info.unwrap_or(seq_cst).2;
+                span_lint_and_help(
+                    cx,
+                    INVALID_ATOMIC_ORDERING,
+                    failure_order_arg.span,
+                    &format!(
+                        "{}'s failure ordering may not be `Release` or `AcqRel`",
+                        method,
+                    ),
+                    None,
+                    &format!("consider using {} instead", suggested),
+                );
+            } else if let Some((success_ord_name, bad_ords_given_success, suggested)) = success_lint_info {
+                if match_ordering_def_path(cx, fail_ordering_def_id, bad_ords_given_success) {
+                    span_lint_and_help(
+                        cx,
+                        INVALID_ATOMIC_ORDERING,
+                        failure_order_arg.span,
+                        &format!(
+                            "{}'s failure ordering may not stronger than the success ordering of `{}`",
+                            method,
+                            success_ord_name,
+                        ),
+                        None,
+                        &format!("consider using {} instead", suggested),
+                    );
+                }
+            }
+        }
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for AtomicOrdering {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         check_atomic_load_store(cx, expr);
         check_memory_fence(cx, expr);
+        check_atomic_compare_exchange(cx, expr);
     }
 }

tests/ui/atomic_ordering_exchange.rs

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

tests/ui/atomic_ordering_exchange.stderr

@@ -0,0 +1,259 @@
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:21:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::AcqRel);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::invalid-atomic-ordering` implied by `-D warnings`
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:22:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::AcqRel);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:23:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::AcqRel);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:24:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::AcqRel);
+   |                                                        ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:25:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::AcqRel);
+   |                                                        ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:28:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Release);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:29:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Release);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:30:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Release);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:31:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Release);
+   |                                                        ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:32:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Release);
+   |                                                        ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_exchange.rs:35:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Acquire);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_exchange.rs:36:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::SeqCst);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_exchange.rs:39:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::SeqCst);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_exchange.rs:40:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Acquire);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not stronger than the success ordering of `Acquire`
+  --> $DIR/atomic_ordering_exchange.rs:43:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::SeqCst);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not stronger than the success ordering of `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:44:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::SeqCst);
+   |                                                        ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:60:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::AcqRel);
+   |                                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:61:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::AcqRel);
+   |                                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:62:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::AcqRel);
+   |                                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:63:61
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::AcqRel);
+   |                                                             ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:64:61
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::AcqRel);
+   |                                                             ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:67:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::Release);
+   |                                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:68:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::Release);
+   |                                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:69:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::Release);
+   |                                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:70:61
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::Release);
+   |                                                             ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:71:61
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::Release);
+   |                                                             ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_exchange.rs:74:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::Acquire);
+   |                                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_exchange.rs:75:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::SeqCst);
+   |                                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_exchange.rs:78:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::SeqCst);
+   |                                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_exchange.rs:79:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::Acquire);
+   |                                                              ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not stronger than the success ordering of `Acquire`
+  --> $DIR/atomic_ordering_exchange.rs:82:62
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::SeqCst);
+   |                                                              ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange_weak's failure ordering may not stronger than the success ordering of `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:83:61
+   |
+LL |     let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::SeqCst);
+   |                                                             ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: aborting due to 32 previous errors
+