From d655c0a938c16d30ae6824713165c749eff7210f Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Tue, 25 Aug 2020 15:13:40 +1200 Subject: [PATCH 1/3] Change the criteria of `interior_mutable_const` * 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 --- clippy_lints/src/non_copy_const.rs | 93 ++++++++++++------- tests/ui/borrow_interior_mutable_const.rs | 20 +++- tests/ui/borrow_interior_mutable_const.stderr | 44 +++++---- tests/ui/declare_interior_mutable_const.rs | 62 +++++++------ .../ui/declare_interior_mutable_const.stderr | 62 ++++--------- 5 files changed, 156 insertions(+), 125 deletions(-) diff --git a/clippy_lints/src/non_copy_const.rs b/clippy_lints/src/non_copy_const.rs index 73eabd4207e7..28c68a2b68c5 100644 --- a/clippy_lints/src/non_copy_const.rs +++ b/clippy_lints/src/non_copy_const.rs @@ -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 @@ -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 }, } @@ -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`. + // 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; } @@ -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"); }, @@ -152,14 +155,10 @@ 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 }); } } @@ -167,17 +166,47 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { 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 }); + }, + _ => (), } } } diff --git a/tests/ui/borrow_interior_mutable_const.rs b/tests/ui/borrow_interior_mutable_const.rs index 39f875105485..9fcc9ece49bb 100644 --- a/tests/ui/borrow_interior_mutable_const.rs +++ b/tests/ui/borrow_interior_mutable_const.rs @@ -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: Copy { - type NonCopyType; +trait Trait { + type AssocType; const ATOMIC: AtomicUsize; + const INPUT: T; + const ASSOC: Self::AssocType; + + fn function() { + let _ = &Self::INPUT; + let _ = &Self::ASSOC; + } } impl Trait 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, diff --git a/tests/ui/borrow_interior_mutable_const.stderr b/tests/ui/borrow_interior_mutable_const.stderr index 5800af7e960f..ed726a6b46e6 100644 --- a/tests/ui/borrow_interior_mutable_const.stderr +++ b/tests/ui/borrow_interior_mutable_const.stderr @@ -1,14 +1,22 @@ 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 | ^^^^^^ @@ -16,7 +24,7 @@ LL | assert_eq!(ATOMIC.load(Ordering::SeqCst), 5); //~ ERROR interior mutabi = 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 | ^^^^^^^^^ @@ -24,7 +32,7 @@ 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 | ^^^^^^^^^ @@ -32,7 +40,7 @@ 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 | ^^^^^^^^^ @@ -40,7 +48,7 @@ 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 | ^^^^^^^^^ @@ -48,7 +56,7 @@ 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 | ^^^^^^^^^^^^ @@ -56,7 +64,7 @@ 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 | ^^^^^^^^^^^^ @@ -64,7 +72,7 @@ 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 | ^^^^^^^^^^^^ @@ -72,7 +80,7 @@ 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 | ^^^^^^^^^^^^ @@ -80,7 +88,7 @@ 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 | ^^^^^^^^^^^^ @@ -88,7 +96,7 @@ LL | let _ = ATOMIC_TUPLE.0[0].load(Ordering::SeqCst); //~ ERROR interior mu = 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 | ^^^^^^^^^^^^ @@ -96,7 +104,7 @@ 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 | ^^^^ @@ -104,7 +112,7 @@ 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 | ^^^^ @@ -112,7 +120,7 @@ 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 | ^^^^^^^^^^^ @@ -120,12 +128,12 @@ 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 diff --git a/tests/ui/declare_interior_mutable_const.rs b/tests/ui/declare_interior_mutable_const.rs index b4003ed8932d..7471b3605406 100644 --- a/tests/ui/declare_interior_mutable_const.rs +++ b/tests/ui/declare_interior_mutable_const.rs @@ -34,60 +34,64 @@ static STATIC_TUPLE: (AtomicUsize, String) = (ATOMIC, STRING); #[allow(clippy::declare_interior_mutable_const)] const ONCE_INIT: Once = Once::new(); -trait Trait: Copy { - type NonCopyType; +struct Wrapper(T); + +trait Trait> { + type AssocType; + type AssocType2; + type AssocType3; const ATOMIC: AtomicUsize; //~ ERROR interior mutable const INTEGER: u64; const STRING: String; - const SELF: Self; // (no error) + const SELF: Self; const INPUT: T; - //~^ ERROR interior mutable - //~| HELP consider requiring `T` to be `Copy` - const ASSOC: Self::NonCopyType; - //~^ ERROR interior mutable - //~| HELP consider requiring `>::NonCopyType` to be `Copy` + const INPUT_ASSOC: T::AssocType4; + const INPUT_ASSOC_2: T::AssocType5; //~ ERROR interior mutable + const ASSOC: Self::AssocType; + const ASSOC_2: Self::AssocType2; + const WRAPPED_ASSOC_2: Wrapper; + const WRAPPED_ASSOC_3: Wrapper; const AN_INPUT: T = Self::INPUT; - //~^ ERROR interior mutable - //~| ERROR consider requiring `T` to be `Copy` - declare_const!(ANOTHER_INPUT: T = Self::INPUT); //~ ERROR interior mutable + declare_const!(ANOTHER_INPUT: T = Self::INPUT); + declare_const!(ANOTHER_ATOMIC: AtomicUsize = Self::ATOMIC); //~ ERROR interior mutable } trait Trait2 { - type CopyType: Copy; + type AssocType4; + type AssocType5; const SELF_2: Self; - //~^ ERROR interior mutable - //~| HELP consider requiring `Self` to be `Copy` - const ASSOC_2: Self::CopyType; // (no error) + const ASSOC_4: Self::AssocType4; } -// we don't lint impl of traits, because an impl has no power to change the interface. -impl Trait for u64 { - type NonCopyType = u16; +impl> Trait for u64 { + type AssocType = u16; + type AssocType2 = AtomicUsize; + type AssocType3 = T; const ATOMIC: AtomicUsize = AtomicUsize::new(9); const INTEGER: u64 = 10; const STRING: String = String::new(); const SELF: Self = 11; - const INPUT: u32 = 12; - const ASSOC: Self::NonCopyType = 13; + const INPUT: T = T::SELF_2; + const INPUT_ASSOC: T::AssocType4 = T::ASSOC_4; + const INPUT_ASSOC_2: T::AssocType5 = AtomicUsize::new(16); + const ASSOC: Self::AssocType = 13; + const ASSOC_2: Self::AssocType2 = AtomicUsize::new(15); //~ ERROR interior mutable + const WRAPPED_ASSOC_2: Wrapper = Wrapper(AtomicUsize::new(16)); //~ ERROR interior mutable + const WRAPPED_ASSOC_3: Wrapper = Wrapper(T::SELF_2); } struct Local(T, U); -impl, U: Trait2> Local { - const ASSOC_3: AtomicUsize = AtomicUsize::new(14); //~ ERROR interior mutable +impl, U: Trait2> Local { + const ASSOC_5: AtomicUsize = AtomicUsize::new(14); //~ ERROR interior mutable const COW: Cow<'static, str> = Cow::Borrowed("tuvwxy"); - const T_SELF: T = T::SELF_2; const U_SELF: U = U::SELF_2; - //~^ ERROR interior mutable - //~| HELP consider requiring `U` to be `Copy` - const T_ASSOC: T::NonCopyType = T::ASSOC; - //~^ ERROR interior mutable - //~| HELP consider requiring `>::NonCopyType` to be `Copy` - const U_ASSOC: U::CopyType = U::ASSOC_2; + const T_ASSOC: T::AssocType = T::ASSOC; + const U_ASSOC: U::AssocType5 = AtomicUsize::new(17); //~ ERROR interior mutable } fn main() {} diff --git a/tests/ui/declare_interior_mutable_const.stderr b/tests/ui/declare_interior_mutable_const.stderr index 6a9a57361f9f..0fcb726db46e 100644 --- a/tests/ui/declare_interior_mutable_const.stderr +++ b/tests/ui/declare_interior_mutable_const.stderr @@ -36,34 +36,16 @@ LL | declare_const!(_ONCE: Once = Once::new()); //~ ERROR interior mutable = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:40:5 + --> $DIR/declare_interior_mutable_const.rs:44:5 | LL | const ATOMIC: AtomicUsize; //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:44:5 - | -LL | const INPUT: T; - | ^^^^^^^^^^^^^-^ - | | - | consider requiring `T` to be `Copy` - -error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:47:5 + --> $DIR/declare_interior_mutable_const.rs:50:5 | -LL | const ASSOC: Self::NonCopyType; - | ^^^^^^^^^^^^^-----------------^ - | | - | consider requiring `>::NonCopyType` to be `Copy` - -error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:51:5 - | -LL | const AN_INPUT: T = Self::INPUT; - | ^^^^^^^^^^^^^^^^-^^^^^^^^^^^^^^^ - | | - | consider requiring `T` to be `Copy` +LL | const INPUT_ASSOC_2: T::AssocType5; //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable --> $DIR/declare_interior_mutable_const.rs:16:9 @@ -71,40 +53,34 @@ error: a `const` item should never be interior mutable LL | const $name: $ty = $e; | ^^^^^^^^^^^^^^^^^^^^^^ ... -LL | declare_const!(ANOTHER_INPUT: T = Self::INPUT); //~ ERROR interior mutable - | ----------------------------------------------- in this macro invocation +LL | declare_const!(ANOTHER_ATOMIC: AtomicUsize = Self::ATOMIC); //~ ERROR interior mutable + | ----------------------------------------------------------- in this macro invocation | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:60:5 + --> $DIR/declare_interior_mutable_const.rs:82:5 | -LL | const SELF_2: Self; - | ^^^^^^^^^^^^^^----^ - | | - | consider requiring `Self` to be `Copy` +LL | const ASSOC_2: Self::AssocType2 = AtomicUsize::new(15); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:81:5 + --> $DIR/declare_interior_mutable_const.rs:83:5 | -LL | const ASSOC_3: AtomicUsize = AtomicUsize::new(14); //~ ERROR interior mutable - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | const WRAPPED_ASSOC_2: Wrapper = Wrapper(AtomicUsize::new(16)); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:84:5 + --> $DIR/declare_interior_mutable_const.rs:90:5 | -LL | const U_SELF: U = U::SELF_2; - | ^^^^^^^^^^^^^^-^^^^^^^^^^^^^ - | | - | consider requiring `U` to be `Copy` +LL | const ASSOC_5: AtomicUsize = AtomicUsize::new(14); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:87:5 + --> $DIR/declare_interior_mutable_const.rs:94:5 | -LL | const T_ASSOC: T::NonCopyType = T::ASSOC; - | ^^^^^^^^^^^^^^^--------------^^^^^^^^^^^^ - | | - | consider requiring `>::NonCopyType` to be `Copy` +LL | const U_ASSOC: U::AssocType5 = AtomicUsize::new(17); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 13 previous errors +error: aborting due to 11 previous errors From 2fc9064921ce0afd2c07c5b576f95c7adf731541 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Thu, 17 Sep 2020 19:37:42 +1200 Subject: [PATCH 2/3] rewrite the test and fix a minor fp * rewrite the test for `declare_interior_mutable_const from scratch` * fix a minor false positive where `Cell<"const T>` gets linted twice --- clippy_lints/src/non_copy_const.rs | 24 +-- tests/ui/declare_interior_mutable_const.rs | 159 +++++++++++++----- .../ui/declare_interior_mutable_const.stderr | 58 ++++--- 3 files changed, 166 insertions(+), 75 deletions(-) diff --git a/clippy_lints/src/non_copy_const.rs b/clippy_lints/src/non_copy_const.rs index 28c68a2b68c5..bb44eeb6adc5 100644 --- a/clippy_lints/src/non_copy_const.rs +++ b/clippy_lints/src/non_copy_const.rs @@ -9,8 +9,7 @@ use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, Tr use rustc_infer::traits::specialization_graph; use rustc_lint::{LateContext, LateLintPass, Lint}; use rustc_middle::ty::adjustment::Adjust; -use rustc_middle::ty::fold::TypeFoldable as _; -use rustc_middle::ty::{AssocKind, Ty, TypeFlags}; +use rustc_middle::ty::{AssocKind, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{InnerSpan, Span, DUMMY_SP}; use rustc_typeck::hir_ty_to_ty; @@ -178,15 +177,18 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { 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); + if cx + .tcx + .layout_of(cx.tcx.param_env(of_trait_def_id).and( + // 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. + cx.tcx.normalize_erasing_regions( + cx.tcx.param_env(of_trait_def_id), + cx.tcx.type_of(of_assoc_item.def_id), + ), + )) + .is_err(); then { let ty = hir_ty_to_ty(cx.tcx, hir_ty); let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); diff --git a/tests/ui/declare_interior_mutable_const.rs b/tests/ui/declare_interior_mutable_const.rs index 7471b3605406..646d3ec8b472 100644 --- a/tests/ui/declare_interior_mutable_const.rs +++ b/tests/ui/declare_interior_mutable_const.rs @@ -34,64 +34,135 @@ static STATIC_TUPLE: (AtomicUsize, String) = (ATOMIC, STRING); #[allow(clippy::declare_interior_mutable_const)] const ONCE_INIT: Once = Once::new(); -struct Wrapper(T); - -trait Trait> { - type AssocType; - type AssocType2; - type AssocType3; - +// a constant whose type is a concrete type should be linted at the definition site. +trait ConcreteTypes { const ATOMIC: AtomicUsize; //~ ERROR interior mutable const INTEGER: u64; const STRING: String; - const SELF: Self; - const INPUT: T; - const INPUT_ASSOC: T::AssocType4; - const INPUT_ASSOC_2: T::AssocType5; //~ ERROR interior mutable - const ASSOC: Self::AssocType; - const ASSOC_2: Self::AssocType2; - const WRAPPED_ASSOC_2: Wrapper; - const WRAPPED_ASSOC_3: Wrapper; - - const AN_INPUT: T = Self::INPUT; - declare_const!(ANOTHER_INPUT: T = Self::INPUT); declare_const!(ANOTHER_ATOMIC: AtomicUsize = Self::ATOMIC); //~ ERROR interior mutable } -trait Trait2 { - type AssocType4; - type AssocType5; +impl ConcreteTypes for u64 { + const ATOMIC: AtomicUsize = AtomicUsize::new(9); + const INTEGER: u64 = 10; + const STRING: String = String::new(); +} - const SELF_2: Self; - const ASSOC_4: Self::AssocType4; +// a helper trait used below +trait ConstDefault { + const DEFAULT: Self; } -impl> Trait for u64 { - type AssocType = u16; - type AssocType2 = AtomicUsize; - type AssocType3 = T; +// a constant whose type is a generic type should be linted at the implementation site. +trait GenericTypes { + const TO_REMAIN_GENERIC: T; + const TO_BE_CONCRETE: U; - const ATOMIC: AtomicUsize = AtomicUsize::new(9); - const INTEGER: u64 = 10; - const STRING: String = String::new(); - const SELF: Self = 11; - const INPUT: T = T::SELF_2; - const INPUT_ASSOC: T::AssocType4 = T::ASSOC_4; - const INPUT_ASSOC_2: T::AssocType5 = AtomicUsize::new(16); - const ASSOC: Self::AssocType = 13; - const ASSOC_2: Self::AssocType2 = AtomicUsize::new(15); //~ ERROR interior mutable - const WRAPPED_ASSOC_2: Wrapper = Wrapper(AtomicUsize::new(16)); //~ ERROR interior mutable - const WRAPPED_ASSOC_3: Wrapper = Wrapper(T::SELF_2); + const HAVING_DEFAULT: T = Self::TO_REMAIN_GENERIC; + declare_const!(IN_MACRO: T = Self::TO_REMAIN_GENERIC); +} + +impl GenericTypes for u64 { + const TO_REMAIN_GENERIC: T = T::DEFAULT; + const TO_BE_CONCRETE: AtomicUsize = AtomicUsize::new(11); //~ ERROR interior mutable +} + +// a helper type used below +struct Wrapper(T); + +// a constant whose type is an associated type should be linted at the implementation site, too. +trait AssocTypes { + type ToBeFrozen; + type ToBeUnfrozen; + type ToBeGenericParam; + + const TO_BE_FROZEN: Self::ToBeFrozen; + const TO_BE_UNFROZEN: Self::ToBeUnfrozen; + const WRAPPED_TO_BE_UNFROZEN: Wrapper; + // to ensure it can handle things when a generic type remains after normalization. + const WRAPPED_TO_BE_GENERIC_PARAM: Wrapper; +} + +impl AssocTypes for Vec { + type ToBeFrozen = u16; + type ToBeUnfrozen = AtomicUsize; + type ToBeGenericParam = T; + + const TO_BE_FROZEN: Self::ToBeFrozen = 12; + const TO_BE_UNFROZEN: Self::ToBeUnfrozen = AtomicUsize::new(13); //~ ERROR interior mutable + const WRAPPED_TO_BE_UNFROZEN: Wrapper = Wrapper(AtomicUsize::new(14)); //~ ERROR interior mutable + const WRAPPED_TO_BE_GENERIC_PARAM: Wrapper = Wrapper(T::DEFAULT); } -struct Local(T, U); +// a helper trait used below +trait AssocTypesHelper { + type NotToBeBounded; + type ToBeBounded; -impl, U: Trait2> Local { - const ASSOC_5: AtomicUsize = AtomicUsize::new(14); //~ ERROR interior mutable + const NOT_TO_BE_BOUNDED: Self::NotToBeBounded; +} + +// a constant whose type is an assoc type originated from a generic param bounded at the definition +// site should be linted at there. +trait AssocTypesFromGenericParam +where + T: AssocTypesHelper, +{ + const NOT_BOUNDED: T::NotToBeBounded; + const BOUNDED: T::ToBeBounded; //~ ERROR interior mutable +} + +impl AssocTypesFromGenericParam for u64 +where + T: AssocTypesHelper, +{ + // an associated type could remain unknown in a trait impl. + const NOT_BOUNDED: T::NotToBeBounded = T::NOT_TO_BE_BOUNDED; + const BOUNDED: T::ToBeBounded = AtomicUsize::new(15); +} + +trait SelfType { + const SELF: Self; +} + +impl SelfType for u64 { + const SELF: Self = 16; +} + +impl SelfType for AtomicUsize { + // this (interior mutable `Self` const) exists in `parking_lot`. + // `const_trait_impl` will replace it in the future, hopefully. + const SELF: Self = AtomicUsize::new(17); //~ ERROR interior mutable +} + +// Even though a constant contains a generic type, if it also have a interior mutable type, +// it should be linted at the definition site. +trait BothOfCellAndGeneric { + // this is a false negative in the current implementation. + const DIRECT: Cell; + const INDIRECT: Cell<*const T>; //~ ERROR interior mutable +} + +impl BothOfCellAndGeneric for u64 { + const DIRECT: Cell = Cell::new(T::DEFAULT); + const INDIRECT: Cell<*const T> = Cell::new(std::ptr::null()); +} + +struct Local(T); + +// a constant in an inherent impl are essentially the same as a normal const item +// except there can be a generic or associated type. +impl Local +where + T: ConstDefault + AssocTypesHelper, +{ + const ATOMIC: AtomicUsize = AtomicUsize::new(18); //~ ERROR interior mutable const COW: Cow<'static, str> = Cow::Borrowed("tuvwxy"); - const U_SELF: U = U::SELF_2; - const T_ASSOC: T::AssocType = T::ASSOC; - const U_ASSOC: U::AssocType5 = AtomicUsize::new(17); //~ ERROR interior mutable + + const GENERIC_TYPE: T = T::DEFAULT; + + const ASSOC_TYPE: T::NotToBeBounded = T::NOT_TO_BE_BOUNDED; + const BOUNDED_ASSOC_TYPE: T::ToBeBounded = AtomicUsize::new(19); //~ ERROR interior mutable } fn main() {} diff --git a/tests/ui/declare_interior_mutable_const.stderr b/tests/ui/declare_interior_mutable_const.stderr index 0fcb726db46e..0a0b818b8b7f 100644 --- a/tests/ui/declare_interior_mutable_const.stderr +++ b/tests/ui/declare_interior_mutable_const.stderr @@ -36,17 +36,11 @@ LL | declare_const!(_ONCE: Once = Once::new()); //~ ERROR interior mutable = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:44:5 + --> $DIR/declare_interior_mutable_const.rs:39:5 | LL | const ATOMIC: AtomicUsize; //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:50:5 - | -LL | const INPUT_ASSOC_2: T::AssocType5; //~ ERROR interior mutable - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - error: a `const` item should never be interior mutable --> $DIR/declare_interior_mutable_const.rs:16:9 | @@ -59,28 +53,52 @@ LL | declare_const!(ANOTHER_ATOMIC: AtomicUsize = Self::ATOMIC); //~ ERROR i = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:82:5 + --> $DIR/declare_interior_mutable_const.rs:67:5 + | +LL | const TO_BE_CONCRETE: AtomicUsize = AtomicUsize::new(11); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: a `const` item should never be interior mutable + --> $DIR/declare_interior_mutable_const.rs:92:5 + | +LL | const TO_BE_UNFROZEN: Self::ToBeUnfrozen = AtomicUsize::new(13); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: a `const` item should never be interior mutable + --> $DIR/declare_interior_mutable_const.rs:93:5 + | +LL | const WRAPPED_TO_BE_UNFROZEN: Wrapper = Wrapper(AtomicUsize::new(14)); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: a `const` item should never be interior mutable + --> $DIR/declare_interior_mutable_const.rs:112:5 + | +LL | const BOUNDED: T::ToBeBounded; //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: a `const` item should never be interior mutable + --> $DIR/declare_interior_mutable_const.rs:135:5 | -LL | const ASSOC_2: Self::AssocType2 = AtomicUsize::new(15); //~ ERROR interior mutable - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | const SELF: Self = AtomicUsize::new(17); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:83:5 + --> $DIR/declare_interior_mutable_const.rs:143:5 | -LL | const WRAPPED_ASSOC_2: Wrapper = Wrapper(AtomicUsize::new(16)); //~ ERROR interior mutable - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | const INDIRECT: Cell<*const T>; //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:90:5 + --> $DIR/declare_interior_mutable_const.rs:159:5 | -LL | const ASSOC_5: AtomicUsize = AtomicUsize::new(14); //~ ERROR interior mutable - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | const ATOMIC: AtomicUsize = AtomicUsize::new(18); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:94:5 + --> $DIR/declare_interior_mutable_const.rs:165:5 | -LL | const U_ASSOC: U::AssocType5 = AtomicUsize::new(17); //~ ERROR interior mutable - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | const BOUNDED_ASSOC_TYPE: T::ToBeBounded = AtomicUsize::new(19); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 11 previous errors +error: aborting due to 14 previous errors From d5af360bb2de24235d2873e926d0b6f21135ae38 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Thu, 17 Sep 2020 21:14:14 +1200 Subject: [PATCH 3/3] add `WRAPPED_SELF: Option` in the test --- tests/ui/declare_interior_mutable_const.rs | 8 +++++++- tests/ui/declare_interior_mutable_const.stderr | 16 +++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/ui/declare_interior_mutable_const.rs b/tests/ui/declare_interior_mutable_const.rs index 646d3ec8b472..3afcdca2f04d 100644 --- a/tests/ui/declare_interior_mutable_const.rs +++ b/tests/ui/declare_interior_mutable_const.rs @@ -121,18 +121,24 @@ where const BOUNDED: T::ToBeBounded = AtomicUsize::new(15); } -trait SelfType { +// a constant whose type is `Self` should be linted at the implementation site as well. +// (`Option` requires `Sized` bound.) +trait SelfType: Sized { const SELF: Self; + // this was the one in the original issue (#5050). + const WRAPPED_SELF: Option; } impl SelfType for u64 { const SELF: Self = 16; + const WRAPPED_SELF: Option = Some(20); } impl SelfType for AtomicUsize { // this (interior mutable `Self` const) exists in `parking_lot`. // `const_trait_impl` will replace it in the future, hopefully. const SELF: Self = AtomicUsize::new(17); //~ ERROR interior mutable + const WRAPPED_SELF: Option = Some(AtomicUsize::new(21)); //~ ERROR interior mutable } // Even though a constant contains a generic type, if it also have a interior mutable type, diff --git a/tests/ui/declare_interior_mutable_const.stderr b/tests/ui/declare_interior_mutable_const.stderr index 0a0b818b8b7f..5cb10be88d89 100644 --- a/tests/ui/declare_interior_mutable_const.stderr +++ b/tests/ui/declare_interior_mutable_const.stderr @@ -77,28 +77,34 @@ LL | const BOUNDED: T::ToBeBounded; //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:135:5 + --> $DIR/declare_interior_mutable_const.rs:140:5 | LL | const SELF: Self = AtomicUsize::new(17); //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:143:5 + --> $DIR/declare_interior_mutable_const.rs:141:5 + | +LL | const WRAPPED_SELF: Option = Some(AtomicUsize::new(21)); //~ ERROR interior mutable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: a `const` item should never be interior mutable + --> $DIR/declare_interior_mutable_const.rs:149:5 | LL | const INDIRECT: Cell<*const T>; //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:159:5 + --> $DIR/declare_interior_mutable_const.rs:165:5 | LL | const ATOMIC: AtomicUsize = AtomicUsize::new(18); //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: a `const` item should never be interior mutable - --> $DIR/declare_interior_mutable_const.rs:165:5 + --> $DIR/declare_interior_mutable_const.rs:171:5 | LL | const BOUNDED_ASSOC_TYPE: T::ToBeBounded = AtomicUsize::new(19); //~ ERROR interior mutable | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 14 previous errors +error: aborting due to 15 previous errors