Skip to content

Commit d88b9b7

Browse files
borsbors
committed
Auto merge of #6046 - rail-rain:change_criteria_non_copy_const, r=flip1995
Change the criteria of `interior_mutable_const` This implements my suggestion [here](#5050 (comment)), and so hopefully fixes #5050. * 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 --- changelog: Change the criteria of `declare_interior_mutable_const` and `borrow_interior_mutable_const` to narrow the lints to only lint things that defenitly is a interior mutable type, not potentially.
2 parents 5af88e3 + d5af360 commit d88b9b7

File tree

5 files changed

+269
-135
lines changed

5 files changed

+269
-135
lines changed

‎clippy_lints/src/non_copy_const.rs‎

Lines changed: 63 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,16 @@ 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::{AssocKind, Ty};
1213
use rustc_session::{declare_lint_pass, declare_tool_lint};
1314
use rustc_span::{InnerSpan, Span, DUMMY_SP};
1415
use rustc_typeck::hir_ty_to_ty;
1516

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

1820
declare_clippy_lint! {
1921
/// **What it does:** Checks for declaration of `const` items which is interior
@@ -83,11 +85,10 @@ declare_clippy_lint! {
8385
"referencing `const` with interior mutability"
8486
}
8587

86-
#[allow(dead_code)]
8788
#[derive(Copy, Clone)]
8889
enum Source {
8990
Item { item: Span },
90-
Assoc { item: Span, ty: Span },
91+
Assoc { item: Span },
9192
Expr { expr: Span },
9293
}
9394

@@ -110,10 +111,15 @@ impl Source {
110111
}
111112

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

@@ -127,11 +133,7 @@ fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) {
127133
let const_kw_span = span.from_inner(InnerSpan::new(0, 5));
128134
diag.span_label(const_kw_span, "make this a static item (maybe with lazy_static)");
129135
},
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-
},
136+
Source::Assoc { .. } => (),
135137
Source::Expr { .. } => {
136138
diag.help("assign this const to a local or static variable, and use the variable here");
137139
},
@@ -152,32 +154,61 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst {
152154
fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitItem<'_>) {
153155
if let TraitItemKind::Const(hir_ty, ..) = &trait_item.kind {
154156
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-
);
157+
// Normalize assoc types because ones originated from generic params
158+
// bounded other traits could have their bound.
159+
let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty);
160+
verify_ty_bound(cx, normalized, Source::Assoc { item: trait_item.span });
163161
}
164162
}
165163

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

‎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)