Skip to content

Commit 7c41ec7

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 57782e0 commit 7c41ec7

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
@@ -43,35 +43,36 @@ use std::collections::hash_map::Entry;
4343

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

9899
declare_clippy_lint! {
99100
/// ### What it does
100-
/// Checks if `const` items which is interior mutable (e.g.,
101-
/// contains a `Cell`, `Mutex`, `AtomicXxxx`, etc.) has been borrowed directly.
101+
/// Checks for a borrow of a named constant with interior mutability.
102102
///
103103
/// ### Why is this bad?
104-
/// Consts are copied everywhere they are referenced, i.e.,
105-
/// every time you refer to the const a fresh instance of the `Cell` or `Mutex`
106-
/// or `AtomicXxxx` will be created, which defeats the whole purpose of using
107-
/// these types in the first place.
104+
/// Named constants are copied at every use site which means any change to their value
105+
/// will be lost after the newly created value is dropped. e.g.
106+
///
107+
/// ```rust
108+
/// use core::sync::atomic::{AtomicUsize, Ordering};
109+
/// const ATOMIC: AtomicUsize = AtomicUsize::new(0);
110+
/// fn add_one() -> usize {
111+
/// // This will always return `0` since `ATOMIC` is copied before it's borrowed
112+
/// // for use by `fetch_add`.
113+
/// ATOMIC.fetch_add(1, Ordering::AcqRel)
114+
/// }
115+
/// ```
108116
///
109-
/// The `const` value should be stored inside a `static` item.
117+
/// ### Known problems
118+
/// This lint does not, and cannot in general, determine if the borrow of the constant
119+
/// is used in a way which causes a mutation. e.g.
120+
///
121+
/// ```rust
122+
/// use core::cell::Cell;
123+
/// const CELL: Cell<usize> = Cell::new(0);
124+
/// fn get_cell() -> Cell<usize> {
125+
/// // This is fine. It borrows a copy of `CELL`, but never mutates it through the
126+
/// // borrow.
127+
/// CELL.clone()
128+
/// }
129+
/// ```
130+
///
131+
/// There also exists types which contain private fields with interior mutability, but
132+
/// no way to both create a value as a constant and modify any mutable field using the
133+
/// type's public interface (e.g. `bytes::Bytes`). As there is no reasonable way to
134+
/// scan a crate's interface to see if this is the case, all such types will be linted.
135+
/// If this happens use the `ignore-interior-mutability` configuration option to allow
136+
/// the type.
110137
///
111138
/// ### Example
112139
/// ```no_run
@@ -180,6 +207,7 @@ enum BorrowCause {
180207
Index,
181208
AutoDeref,
182209
AutoBorrow,
210+
AutoDerefField,
183211
}
184212
impl BorrowCause {
185213
fn note(self) -> Option<&'static str> {
@@ -189,6 +217,9 @@ impl BorrowCause {
189217
Self::Index => Some("this index expression is a call to `Index::index`"),
190218
Self::AutoDeref => Some("there is a compiler inserted call to `Deref::deref` here"),
191219
Self::AutoBorrow => Some("there is a compiler inserted borrow here"),
220+
Self::AutoDerefField => {
221+
Some("there is a compiler inserted call to `Deref::deref` when accessing this field")
222+
},
192223
}
193224
}
194225
}
@@ -208,6 +239,7 @@ impl<'tcx> BorrowSource<'tcx> {
208239
match parent.kind {
209240
ExprKind::Unary(UnOp::Deref, _) => (parent, BorrowCause::Deref),
210241
ExprKind::Index(..) => (parent, BorrowCause::Index),
242+
ExprKind::Field(..) => (parent, BorrowCause::AutoDerefField),
211243
_ => (expr, cause),
212244
}
213245
} else {
@@ -688,17 +720,15 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
688720
cx,
689721
DECLARE_INTERIOR_MUTABLE_CONST,
690722
ident.span,
691-
"a `const` item should not be interior mutable",
723+
"named constant with interior mutability",
692724
|diag| {
693725
let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else {
694726
return;
695727
};
696728
if implements_trait(cx, ty, sync_trait, &[]) {
697-
diag.help("consider making this a static item");
729+
diag.help("did you mean to make this a `static` item");
698730
} else {
699-
diag.help(
700-
"consider making this `Sync` so that it can go in a static item or using a `thread_local`",
701-
);
731+
diag.help("did you mean to make this a `thread_local!` item");
702732
}
703733
},
704734
);
@@ -732,7 +762,7 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
732762
cx,
733763
DECLARE_INTERIOR_MUTABLE_CONST,
734764
item.ident.span,
735-
"a `const` item should not be interior mutable",
765+
"named constant with interior mutability",
736766
);
737767
}
738768
}
@@ -784,7 +814,7 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
784814
cx,
785815
DECLARE_INTERIOR_MUTABLE_CONST,
786816
item.ident.span,
787-
"a `const` item should not be interior mutable",
817+
"named constant with interior mutability",
788818
);
789819
}
790820
}
@@ -819,12 +849,12 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
819849
cx,
820850
BORROW_INTERIOR_MUTABLE_CONST,
821851
borrow_src.expr.span,
822-
"a `const` item with interior mutability should not be borrowed",
852+
"borrow of a named constant with interior mutability",
823853
|diag| {
824-
if let Some(msg) = borrow_src.cause.note() {
825-
diag.note(msg);
854+
if let Some(note) = borrow_src.cause.note() {
855+
diag.note(note);
826856
}
827-
diag.help("assign this const to a local or static variable, and use the variable here");
857+
diag.help("this lint can be silenced by assigning the value to a local variable before borrowing");
828858
},
829859
);
830860
}

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