Skip to content

Commit 43c2ef0

Browse files
JarchoJarcho
committed
Reword declare_interior_mutable_const and
`borrow_interior_mutable_const` messages to be less assertive. Update documentation for both lints.
1 parent 0bda286 commit 43c2ef0

File tree

8 files changed

+188
-158
lines changed

8 files changed

+188
-158
lines changed

‎clippy_lints/src/non_copy_const.rs‎

Lines changed: 71 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -42,35 +42,36 @@ use std::collections::hash_map::Entry;
4242

4343
declare_clippy_lint! {
4444
/// ### What it does
45-
/// Checks for declaration of `const` items which is interior
46-
/// mutable (e.g., contains a `Cell`, `Mutex`, `AtomicXxxx`, etc.).
45+
/// Checks for the declaration of named constant which contain interior mutability.
4746
///
4847
/// ### Why is this bad?
49-
/// Consts are copied everywhere they are referenced, i.e.,
50-
/// every time you refer to the const a fresh instance of the `Cell` or `Mutex`
51-
/// or `AtomicXxxx` will be created, which defeats the whole purpose of using
52-
/// these types in the first place.
48+
/// Named constants are copied at every use site which means any change to their value
49+
/// will be lost after the newly created value is dropped. e.g.
5350
///
54-
/// The `const` should better be replaced by a `static` item if a global
55-
/// variable is wanted, or replaced by a `const fn` if a constructor is wanted.
51+
/// ```rust
52+
/// use core::sync::atomic::{AtomicUsize, Ordering};
53+
/// const ATOMIC: AtomicUsize = AtomicUsize::new(0);
54+
/// fn add_one() -> usize {
55+
/// // This will always return `0` since `ATOMIC` is copied before it's used.
56+
/// ATOMIC.fetch_add(1, Ordering::AcqRel)
57+
/// }
58+
/// ```
5659
///
57-
/// ### Known problems
58-
/// A "non-constant" const item is a legacy way to supply an
59-
/// initialized value to downstream `static` items (e.g., the
60-
/// `std::sync::ONCE_INIT` constant). In this case the use of `const` is legit,
61-
/// and this lint should be suppressed.
60+
/// If shared modification of the value is desired, a `static` item is needed instead.
61+
/// If that is not desired, a `const fn` constructor should be used to make it obvious
62+
/// at the use site that a new value is created.
6263
///
63-
/// Even though the lint avoids triggering on a constant whose type has enums that have variants
64-
/// with interior mutability, and its value uses non interior mutable variants (see
65-
/// [#3962](https://github.com/rust-lang/rust-clippy/issues/3962) and
66-
/// [#3825](https://github.com/rust-lang/rust-clippy/issues/3825) for examples);
67-
/// it complains about associated constants without default values only based on its types;
68-
/// which might not be preferable.
69-
/// There're other enums plus associated constants cases that the lint cannot handle.
64+
/// ### Known problems
65+
/// Prior to `const fn` stabilization this was the only way to provide a value which
66+
/// could initialize a `static` item (e.g. the `std::sync::ONCE_INIT` constant). In
67+
/// this case the use of `const` is required and this lint should be suppressed.
7068
///
71-
/// Types that have underlying or potential interior mutability trigger the lint whether
72-
/// the interior mutable field is used or not. See issue
73-
/// [#5812](https://github.com/rust-lang/rust-clippy/issues/5812)
69+
/// There also exists types which contain private fields with interior mutability, but
70+
/// no way to both create a value as a constant and modify any mutable field using the
71+
/// type's public interface (e.g. `bytes::Bytes`). As there is no reasonable way to
72+
/// scan a crate's interface to see if this is the case, all such types will be linted.
73+
/// If this happens use the `ignore-interior-mutability` configuration option to allow
74+
/// the type.
7475
///
7576
/// ### Example
7677
/// ```no_run
@@ -96,16 +97,42 @@ declare_clippy_lint! {
9697

9798
declare_clippy_lint! {
9899
/// ### What it does
99-
/// Checks if `const` items which is interior mutable (e.g.,
100-
/// contains a `Cell`, `Mutex`, `AtomicXxxx`, etc.) has been borrowed directly.
100+
/// Checks for a borrow of a named constant with interior mutability.
101101
///
102102
/// ### Why is this bad?
103-
/// Consts are copied everywhere they are referenced, i.e.,
104-
/// every time you refer to the const a fresh instance of the `Cell` or `Mutex`
105-
/// or `AtomicXxxx` will be created, which defeats the whole purpose of using
106-
/// these types in the first place.
103+
/// Named constants are copied at every use site which means any change to their value
104+
/// will be lost after the newly created value is dropped. e.g.
105+
///
106+
/// ```rust
107+
/// use core::sync::atomic::{AtomicUsize, Ordering};
108+
/// const ATOMIC: AtomicUsize = AtomicUsize::new(0);
109+
/// fn add_one() -> usize {
110+
/// // This will always return `0` since `ATOMIC` is copied before it's borrowed
111+
/// // for use by `fetch_add`.
112+
/// ATOMIC.fetch_add(1, Ordering::AcqRel)
113+
/// }
114+
/// ```
107115
///
108-
/// The `const` value should be stored inside a `static` item.
116+
/// ### Known problems
117+
/// This lint does not, and cannot in general, determine if the borrow of the constant
118+
/// is used in a way which causes a mutation. e.g.
119+
///
120+
/// ```rust
121+
/// use core::cell::Cell;
122+
/// const CELL: Cell<usize> = Cell::new(0);
123+
/// fn get_cell() -> Cell<usize> {
124+
/// // This is fine. It borrows a copy of `CELL`, but never mutates it through the
125+
/// // borrow.
126+
/// CELL.clone()
127+
/// }
128+
/// ```
129+
///
130+
/// There also exists types which contain private fields with interior mutability, but
131+
/// no way to both create a value as a constant and modify any mutable field using the
132+
/// type's public interface (e.g. `bytes::Bytes`). As there is no reasonable way to
133+
/// scan a crate's interface to see if this is the case, all such types will be linted.
134+
/// If this happens use the `ignore-interior-mutability` configuration option to allow
135+
/// the type.
109136
///
110137
/// ### Example
111138
/// ```no_run
@@ -179,6 +206,7 @@ enum BorrowCause {
179206
Index,
180207
AutoDeref,
181208
AutoBorrow,
209+
AutoDerefField,
182210
}
183211
impl BorrowCause {
184212
fn note(self) -> Option<&'static str> {
@@ -188,6 +216,9 @@ impl BorrowCause {
188216
Self::Index => Some("this index expression is a call to `Index::index`"),
189217
Self::AutoDeref => Some("there is a compiler inserted call to `Deref::deref` here"),
190218
Self::AutoBorrow => Some("there is a compiler inserted borrow here"),
219+
Self::AutoDerefField => {
220+
Some("there is a compiler inserted call to `Deref::deref` when accessing this field")
221+
},
191222
}
192223
}
193224
}
@@ -207,6 +238,7 @@ impl<'tcx> BorrowSource<'tcx> {
207238
match parent.kind {
208239
ExprKind::Unary(UnOp::Deref, _) => (parent, BorrowCause::Deref),
209240
ExprKind::Index(..) => (parent, BorrowCause::Index),
241+
ExprKind::Field(..) => (parent, BorrowCause::AutoDerefField),
210242
_ => (expr, cause),
211243
}
212244
} else {
@@ -690,17 +722,15 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
690722
cx,
691723
DECLARE_INTERIOR_MUTABLE_CONST,
692724
ident.span,
693-
"a `const` item should not be interior mutable",
725+
"named constant with interior mutability",
694726
|diag| {
695727
let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else {
696728
return;
697729
};
698730
if implements_trait(cx, ty, sync_trait, &[]) {
699-
diag.help("consider making this a static item");
731+
diag.help("did you mean to make this a `static` item");
700732
} else {
701-
diag.help(
702-
"consider making this `Sync` so that it can go in a static item or using a `thread_local`",
703-
);
733+
diag.help("did you mean to make this a `thread_local!` item");
704734
}
705735
},
706736
);
@@ -734,7 +764,7 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
734764
cx,
735765
DECLARE_INTERIOR_MUTABLE_CONST,
736766
item.ident.span,
737-
"a `const` item should not be interior mutable",
767+
"named constant with interior mutability",
738768
);
739769
}
740770
}
@@ -786,7 +816,7 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
786816
cx,
787817
DECLARE_INTERIOR_MUTABLE_CONST,
788818
item.ident.span,
789-
"a `const` item should not be interior mutable",
819+
"named constant with interior mutability",
790820
);
791821
}
792822
}
@@ -821,12 +851,12 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
821851
cx,
822852
BORROW_INTERIOR_MUTABLE_CONST,
823853
borrow_src.expr.span,
824-
"a `const` item with interior mutability should not be borrowed",
854+
"borrow of a named constant with interior mutability",
825855
|diag| {
826-
if let Some(msg) = borrow_src.cause.note() {
827-
diag.note(msg);
856+
if let Some(note) = borrow_src.cause.note() {
857+
diag.note(note);
828858
}
829-
diag.help("assign this const to a local or static variable, and use the variable here");
859+
diag.help("this lint can be silenced by assigning the value to a local variable before borrowing");
830860
},
831861
);
832862
}

‎tests/ui/borrow_interior_mutable_const/enums.stderr‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,55 @@
1-
error: a `const` item with interior mutability should not be borrowed
1+
error: borrow of a named constant with interior mutability
22
--> tests/ui/borrow_interior_mutable_const/enums.rs:22:13
33
|
44
LL | let _ = &UNFROZEN_VARIANT;
55
| ^^^^^^^^^^^^^^^^^
66
|
7-
= help: assign this const to a local or static variable, and use the variable here
7+
= help: this lint can be silenced by assigning the value to a local variable before borrowing
88
note: the lint level is defined here
99
--> tests/ui/borrow_interior_mutable_const/enums.rs:3:9
1010
|
1111
LL | #![deny(clippy::borrow_interior_mutable_const)]
1212
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1313

14-
error: a `const` item with interior mutability should not be borrowed
14+
error: borrow of a named constant with interior mutability
1515
--> tests/ui/borrow_interior_mutable_const/enums.rs:50:17
1616
|
1717
LL | let _ = &<Self as AssocConsts>::TO_BE_UNFROZEN_VARIANT;
1818
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1919
|
20-
= help: assign this const to a local or static variable, and use the variable here
20+
= help: this lint can be silenced by assigning the value to a local variable before borrowing
2121

22-
error: a `const` item with interior mutability should not be borrowed
22+
error: borrow of a named constant with interior mutability
2323
--> tests/ui/borrow_interior_mutable_const/enums.rs:52:17
2424
|
2525
LL | let _ = &Self::DEFAULTED_ON_UNFROZEN_VARIANT;
2626
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2727
|
28-
= help: assign this const to a local or static variable, and use the variable here
28+
= help: this lint can be silenced by assigning the value to a local variable before borrowing
2929

30-
error: a `const` item with interior mutability should not be borrowed
30+
error: borrow of a named constant with interior mutability
3131
--> tests/ui/borrow_interior_mutable_const/enums.rs:74:17
3232
|
3333
LL | let _ = &<Self as AssocTypes>::TO_BE_UNFROZEN_VARIANT;
3434
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
3535
|
36-
= help: assign this const to a local or static variable, and use the variable here
36+
= help: this lint can be silenced by assigning the value to a local variable before borrowing
3737

38-
error: a `const` item with interior mutability should not be borrowed
38+
error: borrow of a named constant with interior mutability
3939
--> tests/ui/borrow_interior_mutable_const/enums.rs:91:17
4040
|
4141
LL | let _ = &Self::UNFROZEN_VARIANT;
4242
| ^^^^^^^^^^^^^^^^^^^^^^^
4343
|
44-
= help: assign this const to a local or static variable, and use the variable here
44+
= help: this lint can be silenced by assigning the value to a local variable before borrowing
4545

46-
error: a `const` item with interior mutability should not be borrowed
46+
error: borrow of a named constant with interior mutability
4747
--> tests/ui/borrow_interior_mutable_const/enums.rs:99:13
4848
|
4949
LL | let _ = &helper::WRAPPED_PRIVATE_UNFROZEN_VARIANT;
5050
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5151
|
52-
= help: assign this const to a local or static variable, and use the variable here
52+
= help: this lint can be silenced by assigning the value to a local variable before borrowing
5353

5454
error: aborting due to 6 previous errors
5555

0 commit comments

Comments
 (0)