Add lint to detect usage of invalid atomic ordering

CHANGELOG.md

@@ -1134,6 +1134,7 @@ Released 2018-09-13
 [`integer_division`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_division
 [`into_iter_on_array`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array
 [`into_iter_on_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref
+[`invalid_atomic_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering
 [`invalid_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_ref
 [`invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_regex
 [`invalid_upcast_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 344 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 345 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/atomic_ordering.rs

@@ -0,0 +1,102 @@
+use crate::utils::{match_def_path, span_help_and_lint};
+use if_chain::if_chain;
+use rustc::declare_lint_pass;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::ty;
+use rustc_hir::def_id::DefId;
+use rustc_hir::*;
+use rustc_session::declare_tool_lint;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of invalid atomic
+    /// ordering in Atomic*::{load, store} calls.
+    ///
+    /// **Why is this bad?** Using an invalid atomic ordering
+    /// will cause a panic at run-time.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust,no_run
+    /// # use std::sync::atomic::{AtomicBool, Ordering};
+    ///
+    /// let x = AtomicBool::new(true);
+    ///
+    /// let _ = x.load(Ordering::Release);
+    /// let _ = x.load(Ordering::AcqRel);
+    ///
+    /// x.store(false, Ordering::Acquire);
+    /// x.store(false, Ordering::AcqRel);
+    /// ```
+    pub INVALID_ATOMIC_ORDERING,
+    correctness,
+    "usage of invalid atomic ordering in atomic load/store calls"
+}
+
+declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]);
+
+const ATOMIC_TYPES: [&str; 12] = [
+    "AtomicBool",
+    "AtomicI8",
+    "AtomicI16",
+    "AtomicI32",
+    "AtomicI64",
+    "AtomicIsize",
+    "AtomicPtr",
+    "AtomicU8",
+    "AtomicU16",
+    "AtomicU32",
+    "AtomicU64",
+    "AtomicUsize",
+];
+
+fn type_is_atomic(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
+    if let ty::Adt(&ty::AdtDef { did, .. }, _) = cx.tables.expr_ty(expr).kind {
+        ATOMIC_TYPES
+            .iter()
+            .any(|ty| match_def_path(cx, did, &["core", "sync", "atomic", ty]))
+    } else {
+        false
+    }
+}
+
+fn match_ordering_def_path(cx: &LateContext<'_, '_>, did: DefId, orderings: &[&str]) -> bool {
+    orderings
+        .iter()
+        .any(|ordering| match_def_path(cx, did, &["core", "sync", "atomic", "Ordering", ordering]))
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AtomicOrdering {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx 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 == "load" || method == "store";
+            let ordering_arg = if method == "load" { &args[1] } else { &args[2] };
+            if let ExprKind::Path(ref ordering_qpath) = ordering_arg.kind;
+            if let Some(ordering_def_id) = cx.tables.qpath_res(ordering_qpath, ordering_arg.hir_id).opt_def_id();
+            then {
+                if method == "load" &&
+                    match_ordering_def_path(cx, ordering_def_id, &["Release", "AcqRel"]) {
+                    span_help_and_lint(
+                        cx,
+                        INVALID_ATOMIC_ORDERING,
+                        ordering_arg.span,
+                        "atomic loads cannot have `Release` and `AcqRel` ordering",
+                        "consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`"
+                    );
+                } else if method == "store" &&
+                    match_ordering_def_path(cx, ordering_def_id, &["Acquire", "AcqRel"]) {
+                    span_help_and_lint(
+                        cx,
+                        INVALID_ATOMIC_ORDERING,
+                        ordering_arg.span,
+                        "atomic stores cannot have `Acquire` and `AcqRel` ordering",
+                        "consider using ordering modes `Release`, `SeqCst` or `Relaxed`"
+                    );
+                }
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -165,6 +165,7 @@ pub mod arithmetic;
 pub mod as_conversions;
 pub mod assertions_on_constants;
 pub mod assign_ops;
+pub mod atomic_ordering;
 pub mod attrs;
 pub mod bit_mask;
 pub mod blacklisted_name;
@@ -466,6 +467,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         &assertions_on_constants::ASSERTIONS_ON_CONSTANTS,
         &assign_ops::ASSIGN_OP_PATTERN,
         &assign_ops::MISREFACTORED_ASSIGN_OP,
+        &atomic_ordering::INVALID_ATOMIC_ORDERING,
         &attrs::DEPRECATED_CFG_ATTR,
         &attrs::DEPRECATED_SEMVER,
         &attrs::EMPTY_LINE_AFTER_OUTER_ATTR,
@@ -984,6 +986,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
     store.register_early_pass(|| box as_conversions::AsConversions);
     store.register_early_pass(|| box utils::internal_lints::ProduceIce);
     store.register_late_pass(|| box let_underscore::LetUnderscore);
+    store.register_late_pass(|| box atomic_ordering::AtomicOrdering);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1089,6 +1092,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         LintId::of(&assertions_on_constants::ASSERTIONS_ON_CONSTANTS),
         LintId::of(&assign_ops::ASSIGN_OP_PATTERN),
         LintId::of(&assign_ops::MISREFACTORED_ASSIGN_OP),
+        LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING),
         LintId::of(&attrs::DEPRECATED_CFG_ATTR),
         LintId::of(&attrs::DEPRECATED_SEMVER),
         LintId::of(&attrs::UNKNOWN_CLIPPY_LINTS),
@@ -1509,6 +1513,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
 
     store.register_group(true, "clippy::correctness", Some("clippy_correctness"), vec![
         LintId::of(&approx_const::APPROX_CONSTANT),
+        LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING),
         LintId::of(&attrs::DEPRECATED_SEMVER),
         LintId::of(&attrs::USELESS_ATTRIBUTE),
         LintId::of(&bit_mask::BAD_BIT_MASK),

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 344] = [
+pub const ALL_LINTS: [Lint; 345] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -833,6 +833,13 @@ pub const ALL_LINTS: [Lint; 344] = [
         deprecation: None,
         module: "methods",
     },
+    Lint {
+        name: "invalid_atomic_ordering",
+        group: "correctness",
+        desc: "usage of invalid atomic ordering in atomic load/store calls",
+        deprecation: None,
+        module: "atomic_ordering",
+    },
     Lint {
         name: "invalid_regex",
         group: "correctness",

tests/ui/atomic_ordering_bool.rs

@@ -0,0 +1,25 @@
+#![warn(clippy::invalid_atomic_ordering)]
+
+use std::sync::atomic::{AtomicBool, Ordering};
+
+fn main() {
+    let x = AtomicBool::new(true);
+
+    // Allowed load ordering modes
+    let _ = x.load(Ordering::Acquire);
+    let _ = x.load(Ordering::SeqCst);
+    let _ = x.load(Ordering::Relaxed);
+
+    // Disallowed load ordering modes
+    let _ = x.load(Ordering::Release);
+    let _ = x.load(Ordering::AcqRel);
+
+    // Allowed store ordering modes
+    x.store(false, Ordering::Release);
+    x.store(false, Ordering::SeqCst);
+    x.store(false, Ordering::Relaxed);
+
+    // Disallowed store ordering modes
+    x.store(false, Ordering::Acquire);
+    x.store(false, Ordering::AcqRel);
+}

tests/ui/atomic_ordering_bool.stderr

@@ -0,0 +1,35 @@
+error: atomic loads cannot have `Release` and `AcqRel` ordering
+  --> $DIR/atomic_ordering_bool.rs:14:20
+   |
+LL |     let _ = x.load(Ordering::Release);
+   |                    ^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::invalid-atomic-ordering` implied by `-D warnings`
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`
+
+error: atomic loads cannot have `Release` and `AcqRel` ordering
+  --> $DIR/atomic_ordering_bool.rs:15:20
+   |
+LL |     let _ = x.load(Ordering::AcqRel);
+   |                    ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`
+
+error: atomic stores cannot have `Acquire` and `AcqRel` ordering
+  --> $DIR/atomic_ordering_bool.rs:23:20
+   |
+LL |     x.store(false, Ordering::Acquire);
+   |                    ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`
+
+error: atomic stores cannot have `Acquire` and `AcqRel` ordering
+  --> $DIR/atomic_ordering_bool.rs:24:20
+   |
+LL |     x.store(false, Ordering::AcqRel);
+   |                    ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`
+
+error: aborting due to 4 previous errors
+

tests/ui/atomic_ordering_int.rs

@@ -0,0 +1,86 @@
+#![warn(clippy::invalid_atomic_ordering)]
+
+use std::sync::atomic::{AtomicI16, AtomicI32, AtomicI64, AtomicI8, AtomicIsize, Ordering};
+
+fn main() {
+    // `AtomicI8` test cases
+    let x = AtomicI8::new(0);
+
+    // Allowed load ordering modes
+    let _ = x.load(Ordering::Acquire);
+    let _ = x.load(Ordering::SeqCst);
+    let _ = x.load(Ordering::Relaxed);
+
+    // Disallowed load ordering modes
+    let _ = x.load(Ordering::Release);
+    let _ = x.load(Ordering::AcqRel);
+
+    // Allowed store ordering modes
+    x.store(1, Ordering::Release);
+    x.store(1, Ordering::SeqCst);
+    x.store(1, Ordering::Relaxed);
+
+    // Disallowed store ordering modes
+    x.store(1, Ordering::Acquire);
+    x.store(1, Ordering::AcqRel);
+
+    // `AtomicI16` test cases
+    let x = AtomicI16::new(0);
+
+    let _ = x.load(Ordering::Acquire);
+    let _ = x.load(Ordering::SeqCst);
+    let _ = x.load(Ordering::Relaxed);
+    let _ = x.load(Ordering::Release);
+    let _ = x.load(Ordering::AcqRel);
+
+    x.store(1, Ordering::Release);
+    x.store(1, Ordering::SeqCst);
+    x.store(1, Ordering::Relaxed);
+    x.store(1, Ordering::Acquire);
+    x.store(1, Ordering::AcqRel);
+
+    // `AtomicI32` test cases
+    let x = AtomicI32::new(0);
+
+    let _ = x.load(Ordering::Acquire);
+    let _ = x.load(Ordering::SeqCst);
+    let _ = x.load(Ordering::Relaxed);
+    let _ = x.load(Ordering::Release);
+    let _ = x.load(Ordering::AcqRel);
+
+    x.store(1, Ordering::Release);
+    x.store(1, Ordering::SeqCst);
+    x.store(1, Ordering::Relaxed);
+    x.store(1, Ordering::Acquire);
+    x.store(1, Ordering::AcqRel);
+
+    // `AtomicI64` test cases
+    let x = AtomicI64::new(0);
+
+    let _ = x.load(Ordering::Acquire);
+    let _ = x.load(Ordering::SeqCst);
+    let _ = x.load(Ordering::Relaxed);
+    let _ = x.load(Ordering::Release);
+    let _ = x.load(Ordering::AcqRel);
+
+    x.store(1, Ordering::Release);
+    x.store(1, Ordering::SeqCst);
+    x.store(1, Ordering::Relaxed);
+    x.store(1, Ordering::Acquire);
+    x.store(1, Ordering::AcqRel);
+
+    // `AtomicIsize` test cases
+    let x = AtomicIsize::new(0);
+
+    let _ = x.load(Ordering::Acquire);
+    let _ = x.load(Ordering::SeqCst);
+    let _ = x.load(Ordering::Relaxed);
+    let _ = x.load(Ordering::Release);
+    let _ = x.load(Ordering::AcqRel);
+
+    x.store(1, Ordering::Release);
+    x.store(1, Ordering::SeqCst);
+    x.store(1, Ordering::Relaxed);
+    x.store(1, Ordering::Acquire);
+    x.store(1, Ordering::AcqRel);
+}