Merge pull request #2832 from kennytm/non-copy-const

Lint against const items which are interior mutable.

Author: oli-obk

clippy_lints/src/lib.rs

@@ -148,6 +148,7 @@ pub mod neg_cmp_op_on_partial_ord;
 pub mod neg_multiply;
 pub mod new_without_default;
 pub mod no_effect;
+pub mod non_copy_const;
 pub mod non_expressive_names;
 pub mod ok_if_let;
 pub mod open_options;
@@ -405,6 +406,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box duration_subsec::DurationSubsec);
     reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess);
     reg.register_late_lint_pass(box indexing_slicing::IndexingSlicing);
+    reg.register_late_lint_pass(box non_copy_const::NonCopyConst);
 
     reg.register_lint_group("clippy_restriction", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -613,6 +615,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         new_without_default::NEW_WITHOUT_DEFAULT_DERIVE,
         no_effect::NO_EFFECT,
         no_effect::UNNECESSARY_OPERATION,
+        non_copy_const::BORROW_INTERIOR_MUTABLE_CONST,
+        non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST,
         non_expressive_names::JUST_UNDERSCORES_AND_DIGITS,
         non_expressive_names::MANY_SINGLE_CHAR_NAMES,
         ok_if_let::IF_LET_SOME_RESULT,
@@ -868,6 +872,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         misc::CMP_NAN,
         misc::FLOAT_CMP,
         misc::MODULO_ONE,
+        non_copy_const::BORROW_INTERIOR_MUTABLE_CONST,
+        non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST,
         open_options::NONSENSICAL_OPEN_OPTIONS,
         ptr::MUT_FROM_REF,
         ranges::ITERATOR_STEP_BY_ZERO,

clippy_lints/src/non_copy_const.rs

@@ -0,0 +1,268 @@
+//! Checks for uses of const which the type is not Freeze (Cell-free).
+//!
+//! This lint is **deny** by default.
+
+use rustc::lint::{LateContext, LateLintPass, Lint, LintArray, LintPass};
+use rustc::hir::*;
+use rustc::hir::def::Def;
+use rustc::ty::{self, TyRef, TypeFlags};
+use rustc::ty::adjustment::Adjust;
+use rustc_errors::Applicability;
+use rustc_typeck::hir_ty_to_ty;
+use syntax_pos::{DUMMY_SP, Span};
+use std::ptr;
+use crate::utils::{in_constant, in_macro, is_copy, span_lint_and_then};
+
+/// **What it does:** Checks for declaration of `const` items which is interior
+/// mutable (e.g. contains a `Cell`, `Mutex`, `AtomicXxxx` etc).
+///
+/// **Why is this bad?** Consts are copied everywhere they are referenced, i.e.
+/// every time you refer to the const a fresh instance of the `Cell` or `Mutex`
+/// or `AtomicXxxx` will be created, which defeats the whole purpose of using
+/// these types in the first place.
+///
+/// The `const` should better be replaced by a `static` item if a global
+/// variable is wanted, or replaced by a `const fn` if a constructor is wanted.
+///
+/// **Known problems:** A "non-constant" const item is a legacy way to supply an
+/// initialized value to downstream `static` items (e.g. the
+/// `std::sync::ONCE_INIT` constant). In this case the use of `const` is legit,
+/// and this lint should be suppressed.
+///
+/// **Example:**
+/// ```rust
+/// use std::sync::atomic::{Ordering::SeqCst, AtomicUsize};
+///
+/// // Bad.
+/// const CONST_ATOM: AtomicUsize = AtomicUsize::new(12);
+/// CONST_ATOM.store(6, SeqCst);             // the content of the atomic is unchanged
+/// assert_eq!(CONST_ATOM.load(SeqCst), 12); // because the CONST_ATOM in these lines are distinct
+///
+/// // Good.
+/// static STATIC_ATOM: AtomicUsize = AtomicUsize::new(15);
+/// STATIC_ATOM.store(9, SeqCst);
+/// assert_eq!(STATIC_ATOM.load(SeqCst), 9); // use a `static` item to refer to the same instance
+/// ```
+declare_clippy_lint! {
+    pub DECLARE_INTERIOR_MUTABLE_CONST,
+    correctness,
+    "declaring const with interior mutability"
+}
+
+/// **What it does:** Checks if `const` items which is interior mutable (e.g.
+/// contains a `Cell`, `Mutex`, `AtomicXxxx` etc) has been borrowed directly.
+///
+/// **Why is this bad?** Consts are copied everywhere they are referenced, i.e.
+/// every time you refer to the const a fresh instance of the `Cell` or `Mutex`
+/// or `AtomicXxxx` will be created, which defeats the whole purpose of using
+/// these types in the first place.
+///
+/// The `const` value should be stored inside a `static` item.
+///
+/// **Known problems:** None
+///
+/// **Example:**
+/// ```rust
+/// use std::sync::atomic::{Ordering::SeqCst, AtomicUsize};
+/// const CONST_ATOM: AtomicUsize = AtomicUsize::new(12);
+///
+/// // Bad.
+/// CONST_ATOM.store(6, SeqCst);             // the content of the atomic is unchanged
+/// assert_eq!(CONST_ATOM.load(SeqCst), 12); // because the CONST_ATOM in these lines are distinct
+///
+/// // Good.
+/// static STATIC_ATOM: AtomicUsize = CONST_ATOM;
+/// STATIC_ATOM.store(9, SeqCst);
+/// assert_eq!(STATIC_ATOM.load(SeqCst), 9); // use a `static` item to refer to the same instance
+/// ```
+declare_clippy_lint! {
+    pub BORROW_INTERIOR_MUTABLE_CONST,
+    correctness,
+    "referencing const with interior mutability"
+}
+
+#[derive(Copy, Clone)]
+enum Source {
+    Item {
+        item: Span,
+    },
+    Assoc {
+        item: Span,
+        ty: Span,
+    },
+    Expr {
+        expr: Span,
+    },
+}
+
+impl Source {
+    fn lint(&self) -> (&'static Lint, &'static str, Span) {
+        match self {
+            Source::Item { item } | Source::Assoc { item, .. } => (
+                DECLARE_INTERIOR_MUTABLE_CONST,
+                "a const item should never be interior mutable",
+                *item,
+            ),
+            Source::Expr { expr } => (
+                BORROW_INTERIOR_MUTABLE_CONST,
+                "a const item with interior mutability should not be borrowed",
+                *expr,
+            ),
+        }
+    }
+}
+
+fn verify_ty_bound<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    ty: ty::Ty<'tcx>,
+    source: Source,
+) {
+    if ty.is_freeze(cx.tcx, cx.param_env, DUMMY_SP) || is_copy(cx, ty) {
+        // an UnsafeCell is !Copy, and an UnsafeCell is also the only type which
+        // is !Freeze, thus if our type is Copy we can be sure it must be Freeze
+        // as well.
+        return;
+    }
+
+    let (lint, msg, span) = source.lint();
+    span_lint_and_then(cx, lint, span, msg, |db| {
+        if in_macro(span) {
+            return; // Don't give suggestions into macros.
+        }
+        match source {
+            Source::Item { .. } => {
+                let const_kw_span = span.from_inner_byte_pos(0, 5);
+                db.span_suggestion_with_applicability(
+                    const_kw_span,
+                    "make this a static item",
+                    "static".to_string(),
+                    Applicability::MachineApplicable,
+                );
+            }
+            Source::Assoc { ty: ty_span, .. } => {
+                if ty.flags.contains(TypeFlags::HAS_FREE_LOCAL_NAMES) {
+                    db.span_help(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
+                }
+            }
+            Source::Expr { .. } => {
+                db.help(
+                    "assign this const to a local or static variable, and use the variable here",
+                );
+            }
+        }
+    });
+}
+
+
+pub struct NonCopyConst;
+
+impl LintPass for NonCopyConst {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST)
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NonCopyConst {
+    fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, it: &'tcx Item) {
+        if let ItemConst(hir_ty, ..) = &it.node {
+            let ty = hir_ty_to_ty(cx.tcx, hir_ty);
+            verify_ty_bound(cx, ty, Source::Item { item: it.span });
+        }
+    }
+
+    fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, trait_item: &'tcx TraitItem) {
+        if let TraitItemKind::Const(hir_ty, ..) = &trait_item.node {
+            let ty = hir_ty_to_ty(cx.tcx, hir_ty);
+            verify_ty_bound(cx, ty, Source::Assoc { ty: hir_ty.span, item: trait_item.span });
+        }
+    }
+
+    fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, impl_item: &'tcx ImplItem) {
+        if let ImplItemKind::Const(hir_ty, ..) = &impl_item.node {
+            let item_node_id = cx.tcx.hir.get_parent_node(impl_item.id);
+            let item = cx.tcx.hir.expect_item(item_node_id);
+            // ensure the impl is an inherent impl.
+            if let ItemImpl(_, _, _, _, None, _, _) = item.node {
+                let ty = hir_ty_to_ty(cx.tcx, hir_ty);
+                verify_ty_bound(cx, ty, Source::Assoc { ty: hir_ty.span, item: impl_item.span });
+            }
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if let ExprPath(qpath) = &expr.node {
+            // Only lint if we use the const item inside a function.
+            if in_constant(cx, expr.id) {
+                return;
+            }
+
+            // make sure it is a const item.
+            match cx.tables.qpath_def(qpath, expr.hir_id) {
+                Def::Const(_) | Def::AssociatedConst(_) => {},
+                _ => return,
+            };
+
+            // climb up to resolve any field access and explicit referencing.
+            let mut cur_expr = expr;
+            let mut dereferenced_expr = expr;
+            let mut needs_check_adjustment = true;
+            loop {
+                let parent_id = cx.tcx.hir.get_parent_node(cur_expr.id);
+                if parent_id == cur_expr.id {
+                    break;
+                }
+                if let Some(map::NodeExpr(parent_expr)) = cx.tcx.hir.find(parent_id) {
+                    match &parent_expr.node {
+                        ExprAddrOf(..) => {
+                            // `&e` => `e` must be referenced
+                            needs_check_adjustment = false;
+                        }
+                        ExprField(..) => {
+                            dereferenced_expr = parent_expr;
+                            needs_check_adjustment = true;
+                        }
+                        ExprIndex(e, _) if ptr::eq(&**e, cur_expr) => {
+                            // `e[i]` => desugared to `*Index::index(&e, i)`,
+                            // meaning `e` must be referenced.
+                            // no need to go further up since a method call is involved now.
+                            needs_check_adjustment = false;
+                            break;
+                        }
+                        ExprUnary(UnDeref, _) => {
+                            // `*e` => desugared to `*Deref::deref(&e)`,
+                            // meaning `e` must be referenced.
+                            // no need to go further up since a method call is involved now.
+                            needs_check_adjustment = false;
+                            break;
+                        }
+                        _ => break,
+                    }
+                    cur_expr = parent_expr;
+                } else {
+                    break;
+                }
+            }
+
+            let ty = if !needs_check_adjustment {
+                cx.tables.expr_ty(dereferenced_expr)
+            } else {
+                let adjustments = cx.tables.expr_adjustments(dereferenced_expr);
+                if let Some(i) = adjustments.iter().position(|adj| match adj.kind {
+                    Adjust::Borrow(_) | Adjust::Deref(_) => true,
+                    _ => false,
+                }) {
+                    if i == 0 {
+                        cx.tables.expr_ty(dereferenced_expr)
+                    } else {
+                        adjustments[i - 1].target
+                    }
+                } else {
+                    // No borrow adjustments = the entire const is moved.
+                    return;
+                }
+            };
+
+            verify_ty_bound(cx, ty, Source::Expr { expr: expr.span });
+        }
+    }
+}