Skip to content

Commit 1778a1e

Browse files
giraffateflip1995
giraffate
authored and
flip1995
committed
Restrict same_item_push to suppress false positives
It emits a lint when the pushed item is a literal, a constant and an immutable binding that are initialized with those.
1 parent 09bd400 commit 1778a1e

File tree

3 files changed

+154
-36
lines changed

3 files changed

+154
-36
lines changed

‎clippy_lints/src/loops.rs

Lines changed: 95 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -826,7 +826,7 @@ struct FixedOffsetVar<'hir> {
826826
}
827827

828828
fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'_>) -> bool {
829-
let is_slice = match ty.kind {
829+
let is_slice = match ty.kind() {
830830
ty::Ref(_, subty, _) => is_slice_like(cx, subty),
831831
ty::Slice(..) | ty::Array(..) => true,
832832
_ => false,
@@ -1003,7 +1003,7 @@ fn detect_manual_memcpy<'tcx>(
10031003
start: Some(start),
10041004
end: Some(end),
10051005
limits,
1006-
}) = higher::range(cx, arg)
1006+
}) = higher::range(arg)
10071007
{
10081008
// the var must be a single name
10091009
if let PatKind::Binding(_, canonical_id, _, _) = pat.kind {
@@ -1140,23 +1140,95 @@ fn detect_same_item_push<'tcx>(
11401140
walk_expr(&mut same_item_push_visitor, body);
11411141
if same_item_push_visitor.should_lint {
11421142
if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push {
1143-
// Make sure that the push does not involve possibly mutating values
1144-
if mutated_variables(pushed_item, cx).map_or(false, |mutvars| mutvars.is_empty()) {
1143+
let vec_ty = cx.typeck_results().expr_ty(vec);
1144+
let ty = vec_ty.walk().nth(1).unwrap().expect_ty();
1145+
if cx
1146+
.tcx
1147+
.lang_items()
1148+
.clone_trait()
1149+
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
1150+
{
1151+
// Make sure that the push does not involve possibly mutating values
11451152
if let PatKind::Wild = pat.kind {
11461153
let vec_str = snippet_with_macro_callsite(cx, vec.span, "");
11471154
let item_str = snippet_with_macro_callsite(cx, pushed_item.span, "");
1148-
1149-
span_lint_and_help(
1150-
cx,
1151-
SAME_ITEM_PUSH,
1152-
vec.span,
1153-
"it looks like the same item is being pushed into this Vec",
1154-
None,
1155-
&format!(
1156-
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
1157-
item_str, vec_str, item_str
1158-
),
1159-
)
1155+
if let ExprKind::Path(ref qpath) = pushed_item.kind {
1156+
match qpath_res(cx, qpath, pushed_item.hir_id) {
1157+
// immutable bindings that are initialized with literal or constant
1158+
Res::Local(hir_id) => {
1159+
if_chain! {
1160+
let node = cx.tcx.hir().get(hir_id);
1161+
if let Node::Binding(pat) = node;
1162+
if let PatKind::Binding(bind_ann, ..) = pat.kind;
1163+
if !matches!(bind_ann, BindingAnnotation::RefMut | BindingAnnotation::Mutable);
1164+
let parent_node = cx.tcx.hir().get_parent_node(hir_id);
1165+
if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node);
1166+
if let rustc_hir::Local { init: Some(init), .. } = parent_let_expr;
1167+
then {
1168+
match init.kind {
1169+
// immutable bindings that are initialized with literal
1170+
ExprKind::Lit(..) => {
1171+
span_lint_and_help(
1172+
cx,
1173+
SAME_ITEM_PUSH,
1174+
vec.span,
1175+
"it looks like the same item is being pushed into this Vec",
1176+
None,
1177+
&format!(
1178+
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
1179+
item_str, vec_str, item_str
1180+
),
1181+
)
1182+
},
1183+
// immutable bindings that are initialized with constant
1184+
ExprKind::Path(ref path) => {
1185+
if let Res::Def(DefKind::Const, ..) = qpath_res(cx, path, init.hir_id) {
1186+
span_lint_and_help(
1187+
cx,
1188+
SAME_ITEM_PUSH,
1189+
vec.span,
1190+
"it looks like the same item is being pushed into this Vec",
1191+
None,
1192+
&format!(
1193+
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
1194+
item_str, vec_str, item_str
1195+
),
1196+
)
1197+
}
1198+
}
1199+
_ => {},
1200+
}
1201+
}
1202+
}
1203+
},
1204+
// constant
1205+
Res::Def(DefKind::Const, ..) => span_lint_and_help(
1206+
cx,
1207+
SAME_ITEM_PUSH,
1208+
vec.span,
1209+
"it looks like the same item is being pushed into this Vec",
1210+
None,
1211+
&format!(
1212+
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
1213+
item_str, vec_str, item_str
1214+
),
1215+
),
1216+
_ => {},
1217+
}
1218+
} else if let ExprKind::Lit(..) = pushed_item.kind {
1219+
// literal
1220+
span_lint_and_help(
1221+
cx,
1222+
SAME_ITEM_PUSH,
1223+
vec.span,
1224+
"it looks like the same item is being pushed into this Vec",
1225+
None,
1226+
&format!(
1227+
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
1228+
item_str, vec_str, item_str
1229+
),
1230+
)
1231+
}
11601232
}
11611233
}
11621234
}
@@ -1177,7 +1249,7 @@ fn check_for_loop_range<'tcx>(
11771249
start: Some(start),
11781250
ref end,
11791251
limits,
1180-
}) = higher::range(cx, arg)
1252+
}) = higher::range(arg)
11811253
{
11821254
// the var must be a single name
11831255
if let PatKind::Binding(_, canonical_id, ident, _) = pat.kind {
@@ -1355,7 +1427,7 @@ fn is_end_eq_array_len<'tcx>(
13551427
if_chain! {
13561428
if let ExprKind::Lit(ref lit) = end.kind;
13571429
if let ast::LitKind::Int(end_int, _) = lit.node;
1358-
if let ty::Array(_, arr_len_const) = indexed_ty.kind;
1430+
if let ty::Array(_, arr_len_const) = indexed_ty.kind();
13591431
if let Some(arr_len) = arr_len_const.try_eval_usize(cx.tcx, cx.param_env);
13601432
then {
13611433
return match limits {
@@ -1592,7 +1664,7 @@ fn check_for_loop_over_map_kv<'tcx>(
15921664
if let PatKind::Tuple(ref pat, _) = pat.kind {
15931665
if pat.len() == 2 {
15941666
let arg_span = arg.span;
1595-
let (new_pat_span, kind, ty, mutbl) = match cx.typeck_results().expr_ty(arg).kind {
1667+
let (new_pat_span, kind, ty, mutbl) = match *cx.typeck_results().expr_ty(arg).kind() {
15961668
ty::Ref(_, ty, mutbl) => match (&pat[0].kind, &pat[1].kind) {
15971669
(key, _) if pat_is_wild(key, body) => (pat[1].span, "value", ty, mutbl),
15981670
(_, value) if pat_is_wild(value, body) => (pat[0].span, "key", ty, Mutability::Not),
@@ -1679,7 +1751,7 @@ fn check_for_mut_range_bound(cx: &LateContext<'_>, arg: &Expr<'_>, body: &Expr<'
16791751
start: Some(start),
16801752
end: Some(end),
16811753
..
1682-
}) = higher::range(cx, arg)
1754+
}) = higher::range(arg)
16831755
{
16841756
let mut_ids = vec![check_for_mutability(cx, start), check_for_mutability(cx, end)];
16851757
if mut_ids[0].is_some() || mut_ids[1].is_some() {
@@ -1920,7 +1992,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
19201992
for expr in args {
19211993
let ty = self.cx.typeck_results().expr_ty_adjusted(expr);
19221994
self.prefer_mutable = false;
1923-
if let ty::Ref(_, _, mutbl) = ty.kind {
1995+
if let ty::Ref(_, _, mutbl) = *ty.kind() {
19241996
if mutbl == Mutability::Mut {
19251997
self.prefer_mutable = true;
19261998
}
@@ -1932,7 +2004,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
19322004
let def_id = self.cx.typeck_results().type_dependent_def_id(expr.hir_id).unwrap();
19332005
for (ty, expr) in self.cx.tcx.fn_sig(def_id).inputs().skip_binder().iter().zip(args) {
19342006
self.prefer_mutable = false;
1935-
if let ty::Ref(_, _, mutbl) = ty.kind {
2007+
if let ty::Ref(_, _, mutbl) = *ty.kind() {
19362008
if mutbl == Mutability::Mut {
19372009
self.prefer_mutable = true;
19382010
}
@@ -2030,7 +2102,7 @@ fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
20302102

20312103
fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
20322104
// IntoIterator is currently only implemented for array sizes <= 32 in rustc
2033-
match ty.kind {
2105+
match ty.kind() {
20342106
ty::Array(_, n) => n
20352107
.try_eval_usize(cx.tcx, cx.param_env)
20362108
.map_or(false, |val| (0..=32).contains(&val)),

‎tests/ui/same_item_push.rs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#![warn(clippy::same_item_push)]
22

3+
const VALUE: u8 = 7;
4+
35
fn mutate_increment(x: &mut u8) -> u8 {
46
*x += 1;
57
*x
@@ -86,4 +88,40 @@ fn main() {
8688
for a in vec_a {
8789
vec12.push(2u8.pow(a.kind));
8890
}
91+
92+
// Fix #5902
93+
let mut vec13: Vec<u8> = Vec::new();
94+
let mut item = 0;
95+
for _ in 0..10 {
96+
vec13.push(item);
97+
item += 10;
98+
}
99+
100+
// Fix #5979
101+
let mut vec14: Vec<std::fs::File> = Vec::new();
102+
for _ in 0..10 {
103+
vec14.push(std::fs::File::open("foobar").unwrap());
104+
}
105+
// Fix #5979
106+
#[derive(Clone)]
107+
struct S {}
108+
109+
trait T {}
110+
impl T for S {}
111+
112+
let mut vec15: Vec<Box<dyn T>> = Vec::new();
113+
for _ in 0..10 {
114+
vec15.push(Box::new(S {}));
115+
}
116+
117+
let mut vec16 = Vec::new();
118+
for _ in 0..20 {
119+
vec16.push(VALUE);
120+
}
121+
122+
let mut vec17 = Vec::new();
123+
let item = VALUE;
124+
for _ in 0..20 {
125+
vec17.push(item);
126+
}
89127
}

‎tests/ui/same_item_push.stderr

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,43 @@
11
error: it looks like the same item is being pushed into this Vec
2-
--> $DIR/same_item_push.rs:16:9
3-
|
4-
LL | spaces.push(vec![b' ']);
5-
| ^^^^^^
6-
|
7-
= note: `-D clippy::same-item-push` implied by `-D warnings`
8-
= help: try using vec![vec![b' '];SIZE] or spaces.resize(NEW_SIZE, vec![b' '])
9-
10-
error: it looks like the same item is being pushed into this Vec
11-
--> $DIR/same_item_push.rs:22:9
2+
--> $DIR/same_item_push.rs:24:9
123
|
134
LL | vec2.push(item);
145
| ^^^^
156
|
7+
= note: `-D clippy::same-item-push` implied by `-D warnings`
168
= help: try using vec![item;SIZE] or vec2.resize(NEW_SIZE, item)
179

1810
error: it looks like the same item is being pushed into this Vec
19-
--> $DIR/same_item_push.rs:28:9
11+
--> $DIR/same_item_push.rs:30:9
2012
|
2113
LL | vec3.push(item);
2214
| ^^^^
2315
|
2416
= help: try using vec![item;SIZE] or vec3.resize(NEW_SIZE, item)
2517

2618
error: it looks like the same item is being pushed into this Vec
27-
--> $DIR/same_item_push.rs:33:9
19+
--> $DIR/same_item_push.rs:35:9
2820
|
2921
LL | vec4.push(13);
3022
| ^^^^
3123
|
3224
= help: try using vec![13;SIZE] or vec4.resize(NEW_SIZE, 13)
3325

34-
error: aborting due to 4 previous errors
26+
error: it looks like the same item is being pushed into this Vec
27+
--> $DIR/same_item_push.rs:119:9
28+
|
29+
LL | vec16.push(VALUE);
30+
| ^^^^^
31+
|
32+
= help: try using vec![VALUE;SIZE] or vec16.resize(NEW_SIZE, VALUE)
33+
34+
error: it looks like the same item is being pushed into this Vec
35+
--> $DIR/same_item_push.rs:125:9
36+
|
37+
LL | vec17.push(item);
38+
| ^^^^^
39+
|
40+
= help: try using vec![item;SIZE] or vec17.resize(NEW_SIZE, item)
41+
42+
error: aborting due to 5 previous errors
3543

0 commit comments

Comments
 (0)