Skip to content

Commit

Permalink
Change the criteria of interior_mutable_const
Browse files Browse the repository at this point in the history 
* stop linting associated types and generic type parameters
* start linting ones in trait impls
  whose corresponding definitions in the traits are generic
* remove the `is_copy` check
  as presumably the only purpose of it is to allow
  generics with `Copy` bounds as `Freeze` is internal
  and generics are no longer linted
* remove the term 'copy' from the tests
  as being `Copy` no longer have meaning
  • Loading branch information
@rail-rain
rail-rain committed Sep 17, 2020
1 parent 5af88e3 commit d655c0a
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 125 deletions.
93 changes: 61 additions & 32 deletions clippy_lints/src/non_copy_const.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,17 @@ use std::ptr;

use rustc_hir::def::{DefKind, Res};
use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, TraitItem, TraitItemKind, UnOp};
use rustc_infer::traits::specialization_graph;
use rustc_lint::{LateContext, LateLintPass, Lint};
use rustc_middle::ty::adjustment::Adjust;
use rustc_middle::ty::{Ty, TypeFlags};
use rustc_middle::ty::fold::TypeFoldable as _;
use rustc_middle::ty::{AssocKind, Ty, TypeFlags};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::{InnerSpan, Span, DUMMY_SP};
use rustc_typeck::hir_ty_to_ty;

use crate::utils::{in_constant, is_copy, qpath_res, span_lint_and_then};
use crate::utils::{in_constant, qpath_res, span_lint_and_then};
use if_chain::if_chain;

declare_clippy_lint! {
/// **What it does:** Checks for declaration of `const` items which is interior
Expand Down Expand Up @@ -83,11 +86,10 @@ declare_clippy_lint! {
"referencing `const` with interior mutability"
}

#[allow(dead_code)]
#[derive(Copy, Clone)]
enum Source {
Item { item: Span },
Assoc { item: Span, ty: Span },
Assoc { item: Span },
Expr { expr: Span },
}

Expand All @@ -110,10 +112,15 @@ impl Source {
}

fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) {
if ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) || 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.
// Ignore types whose layout is unknown since `is_freeze` reports every generic types as `!Freeze`,
// making it indistinguishable from `UnsafeCell`. i.e. it isn't a tool to prove a type is
// 'unfrozen'. However, this code causes a false negative in which
// a type contains a layout-unknown type, but also a unsafe cell like `const CELL: Cell<T>`.
// Yet, it's better than `ty.has_type_flags(TypeFlags::HAS_TY_PARAM | TypeFlags::HAS_PROJECTION)`
// since it works when a pointer indirection involves (`Cell<*const T>`).
// Making up a `ParamEnv` where every generic params and assoc types are `Freeze`is another option;
// but I'm not sure whether it's a decent way, if possible.
if cx.tcx.layout_of(cx.param_env.and(ty)).is_err() || ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) {
return;
}

Expand All @@ -127,11 +134,7 @@ fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) {
let const_kw_span = span.from_inner(InnerSpan::new(0, 5));
diag.span_label(const_kw_span, "make this a static item (maybe with lazy_static)");
},
Source::Assoc { ty: ty_span, .. } => {
if ty.flags().intersects(TypeFlags::HAS_FREE_LOCAL_NAMES) {
diag.span_label(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
}
},
Source::Assoc { .. } => (),
Source::Expr { .. } => {
diag.help("assign this const to a local or static variable, and use the variable here");
},
Expand All @@ -152,32 +155,58 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst {
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitItem<'_>) {
if let TraitItemKind::Const(hir_ty, ..) = &trait_item.kind {
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,
},
);
// Normalize assoc types because ones originated from generic params
// bounded other traits could have their bound.
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
verify_ty_bound(cx, normalized, Source::Assoc { item: trait_item.span });
}
}

fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) {
if let ImplItemKind::Const(hir_ty, ..) = &impl_item.kind {
let item_hir_id = cx.tcx.hir().get_parent_node(impl_item.hir_id);
let item = cx.tcx.hir().expect_item(item_hir_id);
// Ensure the impl is an inherent impl.
if let ItemKind::Impl { of_trait: None, .. } = item.kind {
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,
},
);

match &item.kind {
ItemKind::Impl {
of_trait: Some(of_trait_ref),
..
} => {
if_chain! {
// Lint a trait impl item only when the definition is a generic type,
// assuming a assoc const is not meant to be a interior mutable type.
if let Some(of_trait_def_id) = of_trait_ref.trait_def_id();
if let Some(of_assoc_item) = specialization_graph::Node::Trait(of_trait_def_id)
.item(cx.tcx, impl_item.ident, AssocKind::Const, of_trait_def_id);
if cx.tcx
// Normalize assoc types because ones originated from generic params
// bounded other traits could have their bound at the trait defs;
// and, in that case, the definition is *not* generic.
.normalize_erasing_regions(
cx.tcx.param_env(of_trait_def_id),
cx.tcx.type_of(of_assoc_item.def_id),
)
.has_type_flags(TypeFlags::HAS_PROJECTION | TypeFlags::HAS_TY_PARAM);
then {
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
verify_ty_bound(
cx,
normalized,
Source::Assoc {
item: impl_item.span,
},
);
}
}
},
ItemKind::Impl { of_trait: None, .. } => {
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
// Normalize assoc types originated from generic params.
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
verify_ty_bound(cx, normalized, Source::Assoc { item: impl_item.span });
},
_ => (),
}
}
}
Expand Down
20 changes: 17 additions & 3 deletions tests/ui/borrow_interior_mutable_const.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,30 @@ const NO_ANN: &dyn Display = &70;
static STATIC_TUPLE: (AtomicUsize, String) = (ATOMIC, STRING);
const ONCE_INIT: Once = Once::new();

trait Trait<T>: Copy {
type NonCopyType;
trait Trait<T> {
type AssocType;

const ATOMIC: AtomicUsize;
const INPUT: T;
const ASSOC: Self::AssocType;

fn function() {
let _ = &Self::INPUT;
let _ = &Self::ASSOC;
}
}

impl Trait<u32> for u64 {
type NonCopyType = u16;
type AssocType = AtomicUsize;

const ATOMIC: AtomicUsize = AtomicUsize::new(9);
const INPUT: u32 = 10;
const ASSOC: Self::AssocType = AtomicUsize::new(11);

fn function() {
let _ = &Self::INPUT;
let _ = &Self::ASSOC; //~ ERROR interior mutability
}
}

// This is just a pointer that can be safely dereferended,
Expand Down
44 changes: 26 additions & 18 deletions tests/ui/borrow_interior_mutable_const.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,131 +1,139 @@
error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:66:5
--> $DIR/borrow_interior_mutable_const.rs:44:18
|
LL | let _ = &Self::ASSOC; //~ ERROR interior mutability
| ^^^^^^^^^^^
|
= note: `-D clippy::borrow-interior-mutable-const` implied by `-D warnings`
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:80:5
|
LL | ATOMIC.store(1, Ordering::SeqCst); //~ ERROR interior mutability
| ^^^^^^
|
= note: `-D clippy::borrow-interior-mutable-const` implied by `-D warnings`
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:67:16
--> $DIR/borrow_interior_mutable_const.rs:81:16
|
LL | assert_eq!(ATOMIC.load(Ordering::SeqCst), 5); //~ ERROR interior mutability
| ^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:70:22
--> $DIR/borrow_interior_mutable_const.rs:84:22
|
LL | let _once_ref = &ONCE_INIT; //~ ERROR interior mutability
| ^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:71:25
--> $DIR/borrow_interior_mutable_const.rs:85:25
|
LL | let _once_ref_2 = &&ONCE_INIT; //~ ERROR interior mutability
| ^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:72:27
--> $DIR/borrow_interior_mutable_const.rs:86:27
|
LL | let _once_ref_4 = &&&&ONCE_INIT; //~ ERROR interior mutability
| ^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:73:26
--> $DIR/borrow_interior_mutable_const.rs:87:26
|
LL | let _once_mut = &mut ONCE_INIT; //~ ERROR interior mutability
| ^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:84:14
--> $DIR/borrow_interior_mutable_const.rs:98:14
|
LL | let _ = &ATOMIC_TUPLE; //~ ERROR interior mutability
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:85:14
--> $DIR/borrow_interior_mutable_const.rs:99:14
|
LL | let _ = &ATOMIC_TUPLE.0; //~ ERROR interior mutability
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:86:19
--> $DIR/borrow_interior_mutable_const.rs:100:19
|
LL | let _ = &(&&&&ATOMIC_TUPLE).0; //~ ERROR interior mutability
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:87:14
--> $DIR/borrow_interior_mutable_const.rs:101:14
|
LL | let _ = &ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:88:13
--> $DIR/borrow_interior_mutable_const.rs:102:13
|
LL | let _ = ATOMIC_TUPLE.0[0].load(Ordering::SeqCst); //~ ERROR interior mutability
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:94:13
--> $DIR/borrow_interior_mutable_const.rs:108:13
|
LL | let _ = ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability
| ^^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:99:5
--> $DIR/borrow_interior_mutable_const.rs:113:5
|
LL | CELL.set(2); //~ ERROR interior mutability
| ^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:100:16
--> $DIR/borrow_interior_mutable_const.rs:114:16
|
LL | assert_eq!(CELL.get(), 6); //~ ERROR interior mutability
| ^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:113:5
--> $DIR/borrow_interior_mutable_const.rs:127:5
|
LL | u64::ATOMIC.store(5, Ordering::SeqCst); //~ ERROR interior mutability
| ^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: a `const` item with interior mutability should not be borrowed
--> $DIR/borrow_interior_mutable_const.rs:114:16
--> $DIR/borrow_interior_mutable_const.rs:128:16
|
LL | assert_eq!(u64::ATOMIC.load(Ordering::SeqCst), 9); //~ ERROR interior mutability
| ^^^^^^^^^^^
|
= help: assign this const to a local or static variable, and use the variable here

error: aborting due to 16 previous errors
error: aborting due to 17 previous errors

Loading

0 comments on commit d655c0a

Please sign in to comment.