Skip to content

Commit d655c0a

Browse files
rail-rainrail-rain
committed
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
1 parent 5af88e3 commit d655c0a

File tree

5 files changed

+156
-125
lines changed

5 files changed

+156
-125
lines changed

‎clippy_lints/src/non_copy_const.rs‎

Lines changed: 61 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,17 @@ use std::ptr;
66

77
use rustc_hir::def::{DefKind, Res};
88
use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, TraitItem, TraitItemKind, UnOp};
9+
use rustc_infer::traits::specialization_graph;
910
use rustc_lint::{LateContext, LateLintPass, Lint};
1011
use rustc_middle::ty::adjustment::Adjust;
11-
use rustc_middle::ty::{Ty, TypeFlags};
12+
use rustc_middle::ty::fold::TypeFoldable as _;
13+
use rustc_middle::ty::{AssocKind, Ty, TypeFlags};
1214
use rustc_session::{declare_lint_pass, declare_tool_lint};
1315
use rustc_span::{InnerSpan, Span, DUMMY_SP};
1416
use rustc_typeck::hir_ty_to_ty;
1517

16-
use crate::utils::{in_constant, is_copy, qpath_res, span_lint_and_then};
18+
use crate::utils::{in_constant, qpath_res, span_lint_and_then};
19+
use if_chain::if_chain;
1720

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

86-
#[allow(dead_code)]
8789
#[derive(Copy, Clone)]
8890
enum Source {
8991
Item { item: Span },
90-
Assoc { item: Span, ty: Span },
92+
Assoc { item: Span },
9193
Expr { expr: Span },
9294
}
9395

@@ -110,10 +112,15 @@ impl Source {
110112
}
111113

112114
fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) {
113-
if ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) || is_copy(cx, ty) {
114-
// An `UnsafeCell` is `!Copy`, and an `UnsafeCell` is also the only type which
115-
// is `!Freeze`, thus if our type is `Copy` we can be sure it must be `Freeze`
116-
// as well.
115+
// Ignore types whose layout is unknown since `is_freeze` reports every generic types as `!Freeze`,
116+
// making it indistinguishable from `UnsafeCell`. i.e. it isn't a tool to prove a type is
117+
// 'unfrozen'. However, this code causes a false negative in which
118+
// a type contains a layout-unknown type, but also a unsafe cell like `const CELL: Cell<T>`.
119+
// Yet, it's better than `ty.has_type_flags(TypeFlags::HAS_TY_PARAM | TypeFlags::HAS_PROJECTION)`
120+
// since it works when a pointer indirection involves (`Cell<*const T>`).
121+
// Making up a `ParamEnv` where every generic params and assoc types are `Freeze`is another option;
122+
// but I'm not sure whether it's a decent way, if possible.
123+
if cx.tcx.layout_of(cx.param_env.and(ty)).is_err() || ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) {
117124
return;
118125
}
119126

@@ -127,11 +134,7 @@ fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) {
127134
let const_kw_span = span.from_inner(InnerSpan::new(0, 5));
128135
diag.span_label(const_kw_span, "make this a static item (maybe with lazy_static)");
129136
},
130-
Source::Assoc { ty: ty_span, .. } => {
131-
if ty.flags().intersects(TypeFlags::HAS_FREE_LOCAL_NAMES) {
132-
diag.span_label(ty_span, &format!("consider requiring `{}` to be `Copy`", ty));
133-
}
134-
},
137+
Source::Assoc { .. } => (),
135138
Source::Expr { .. } => {
136139
diag.help("assign this const to a local or static variable, and use the variable here");
137140
},
@@ -152,32 +155,58 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst {
152155
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitItem<'_>) {
153156
if let TraitItemKind::Const(hir_ty, ..) = &trait_item.kind {
154157
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
155-
verify_ty_bound(
156-
cx,
157-
ty,
158-
Source::Assoc {
159-
ty: hir_ty.span,
160-
item: trait_item.span,
161-
},
162-
);
158+
// Normalize assoc types because ones originated from generic params
159+
// bounded other traits could have their bound.
160+
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
161+
verify_ty_bound(cx, normalized, Source::Assoc { item: trait_item.span });
163162
}
164163
}
165164

166165
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx ImplItem<'_>) {
167166
if let ImplItemKind::Const(hir_ty, ..) = &impl_item.kind {
168167
let item_hir_id = cx.tcx.hir().get_parent_node(impl_item.hir_id);
169168
let item = cx.tcx.hir().expect_item(item_hir_id);
170-
// Ensure the impl is an inherent impl.
171-
if let ItemKind::Impl { of_trait: None, .. } = item.kind {
172-
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
173-
verify_ty_bound(
174-
cx,
175-
ty,
176-
Source::Assoc {
177-
ty: hir_ty.span,
178-
item: impl_item.span,
179-
},
180-
);
169+
170+
match &item.kind {
171+
ItemKind::Impl {
172+
of_trait: Some(of_trait_ref),
173+
..
174+
} => {
175+
if_chain! {
176+
// Lint a trait impl item only when the definition is a generic type,
177+
// assuming a assoc const is not meant to be a interior mutable type.
178+
if let Some(of_trait_def_id) = of_trait_ref.trait_def_id();
179+
if let Some(of_assoc_item) = specialization_graph::Node::Trait(of_trait_def_id)
180+
.item(cx.tcx, impl_item.ident, AssocKind::Const, of_trait_def_id);
181+
if cx.tcx
182+
// Normalize assoc types because ones originated from generic params
183+
// bounded other traits could have their bound at the trait defs;
184+
// and, in that case, the definition is *not* generic.
185+
.normalize_erasing_regions(
186+
cx.tcx.param_env(of_trait_def_id),
187+
cx.tcx.type_of(of_assoc_item.def_id),
188+
)
189+
.has_type_flags(TypeFlags::HAS_PROJECTION | TypeFlags::HAS_TY_PARAM);
190+
then {
191+
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
192+
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
193+
verify_ty_bound(
194+
cx,
195+
normalized,
196+
Source::Assoc {
197+
item: impl_item.span,
198+
},
199+
);
200+
}
201+
}
202+
},
203+
ItemKind::Impl { of_trait: None, .. } => {
204+
let ty = hir_ty_to_ty(cx.tcx, hir_ty);
205+
// Normalize assoc types originated from generic params.
206+
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
207+
verify_ty_bound(cx, normalized, Source::Assoc { item: impl_item.span });
208+
},
209+
_ => (),
181210
}
182211
}
183212
}

‎tests/ui/borrow_interior_mutable_const.rs‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,30 @@ const NO_ANN: &dyn Display = &70;
1919
static STATIC_TUPLE: (AtomicUsize, String) = (ATOMIC, STRING);
2020
const ONCE_INIT: Once = Once::new();
2121

22-
trait Trait<T>: Copy {
23-
type NonCopyType;
22+
trait Trait<T> {
23+
type AssocType;
2424

2525
const ATOMIC: AtomicUsize;
26+
const INPUT: T;
27+
const ASSOC: Self::AssocType;
28+
29+
fn function() {
30+
let _ = &Self::INPUT;
31+
let _ = &Self::ASSOC;
32+
}
2633
}
2734

2835
impl Trait<u32> for u64 {
29-
type NonCopyType = u16;
36+
type AssocType = AtomicUsize;
3037

3138
const ATOMIC: AtomicUsize = AtomicUsize::new(9);
39+
const INPUT: u32 = 10;
40+
const ASSOC: Self::AssocType = AtomicUsize::new(11);
41+
42+
fn function() {
43+
let _ = &Self::INPUT;
44+
let _ = &Self::ASSOC; //~ ERROR interior mutability
45+
}
3246
}
3347

3448
// This is just a pointer that can be safely dereferended,

‎tests/ui/borrow_interior_mutable_const.stderr‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,131 +1,139 @@
11
error: a `const` item with interior mutability should not be borrowed
2-
--> $DIR/borrow_interior_mutable_const.rs:66:5
2+
--> $DIR/borrow_interior_mutable_const.rs:44:18
3+
|
4+
LL | let _ = &Self::ASSOC; //~ ERROR interior mutability
5+
| ^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::borrow-interior-mutable-const` implied by `-D warnings`
8+
= help: assign this const to a local or static variable, and use the variable here
9+
10+
error: a `const` item with interior mutability should not be borrowed
11+
--> $DIR/borrow_interior_mutable_const.rs:80:5
312
|
413
LL | ATOMIC.store(1, Ordering::SeqCst); //~ ERROR interior mutability
514
| ^^^^^^
615
|
7-
= note: `-D clippy::borrow-interior-mutable-const` implied by `-D warnings`
816
= help: assign this const to a local or static variable, and use the variable here
917

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

130-
error: aborting due to 16 previous errors
138+
error: aborting due to 17 previous errors
131139

0 commit comments

Comments
 (0)