Skip to content

Commit 57406c9

Browse files
borsbors
committed
Auto merge of #7018 - Y-Nak:same_item_push, r=Manishearth
Don't trigger `same_item_push` if the vec is used in the loop body fixes #6987 changelog: `same_item_push`: Don't trigger if the `vec` is used in the loop body
2 parents 25c1ed3 + 9f6f001 commit 57406c9

File tree

2 files changed

+91
-53
lines changed

2 files changed

+91
-53
lines changed

‎clippy_lints/src/loops/same_item_push.rs‎

Lines changed: 84 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
use super::SAME_ITEM_PUSH;
22
use clippy_utils::diagnostics::span_lint_and_help;
3+
use clippy_utils::path_to_local;
34
use clippy_utils::source::snippet_with_macro_callsite;
45
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item};
56
use if_chain::if_chain;
7+
use rustc_data_structures::fx::FxHashSet;
68
use rustc_hir::def::{DefKind, Res};
79
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
8-
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, Node, Pat, PatKind, Stmt, StmtKind};
10+
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Node, Pat, PatKind, Stmt, StmtKind};
911
use rustc_lint::LateContext;
1012
use rustc_middle::hir::map::Map;
1113
use rustc_span::symbol::sym;
@@ -41,70 +43,94 @@ pub(super) fn check<'tcx>(
4143
}
4244

4345
// Determine whether it is safe to lint the body
44-
let mut same_item_push_visitor = SameItemPushVisitor {
45-
should_lint: true,
46-
vec_push: None,
47-
cx,
48-
};
46+
let mut same_item_push_visitor = SameItemPushVisitor::new(cx);
4947
walk_expr(&mut same_item_push_visitor, body);
50-
if same_item_push_visitor.should_lint {
51-
if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push {
52-
let vec_ty = cx.typeck_results().expr_ty(vec);
53-
let ty = vec_ty.walk().nth(1).unwrap().expect_ty();
54-
if cx
55-
.tcx
56-
.lang_items()
57-
.clone_trait()
58-
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
59-
{
60-
// Make sure that the push does not involve possibly mutating values
61-
match pushed_item.kind {
62-
ExprKind::Path(ref qpath) => {
63-
match cx.qpath_res(qpath, pushed_item.hir_id) {
64-
// immutable bindings that are initialized with literal or constant
65-
Res::Local(hir_id) => {
66-
let node = cx.tcx.hir().get(hir_id);
67-
if_chain! {
68-
if let Node::Binding(pat) = node;
69-
if let PatKind::Binding(bind_ann, ..) = pat.kind;
70-
if !matches!(bind_ann, BindingAnnotation::RefMut | BindingAnnotation::Mutable);
71-
let parent_node = cx.tcx.hir().get_parent_node(hir_id);
72-
if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node);
73-
if let Some(init) = parent_let_expr.init;
74-
then {
75-
match init.kind {
76-
// immutable bindings that are initialized with literal
77-
ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item),
78-
// immutable bindings that are initialized with constant
79-
ExprKind::Path(ref path) => {
80-
if let Res::Def(DefKind::Const, ..) = cx.qpath_res(path, init.hir_id) {
81-
emit_lint(cx, vec, pushed_item);
82-
}
48+
if_chain! {
49+
if same_item_push_visitor.should_lint();
50+
if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push;
51+
let vec_ty = cx.typeck_results().expr_ty(vec);
52+
let ty = vec_ty.walk().nth(1).unwrap().expect_ty();
53+
if cx
54+
.tcx
55+
.lang_items()
56+
.clone_trait()
57+
.map_or(false, |id| implements_trait(cx, ty, id, &[]));
58+
then {
59+
// Make sure that the push does not involve possibly mutating values
60+
match pushed_item.kind {
61+
ExprKind::Path(ref qpath) => {
62+
match cx.qpath_res(qpath, pushed_item.hir_id) {
63+
// immutable bindings that are initialized with literal or constant
64+
Res::Local(hir_id) => {
65+
let node = cx.tcx.hir().get(hir_id);
66+
if_chain! {
67+
if let Node::Binding(pat) = node;
68+
if let PatKind::Binding(bind_ann, ..) = pat.kind;
69+
if !matches!(bind_ann, BindingAnnotation::RefMut | BindingAnnotation::Mutable);
70+
let parent_node = cx.tcx.hir().get_parent_node(hir_id);
71+
if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node);
72+
if let Some(init) = parent_let_expr.init;
73+
then {
74+
match init.kind {
75+
// immutable bindings that are initialized with literal
76+
ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item),
77+
// immutable bindings that are initialized with constant
78+
ExprKind::Path(ref path) => {
79+
if let Res::Def(DefKind::Const, ..) = cx.qpath_res(path, init.hir_id) {
80+
emit_lint(cx, vec, pushed_item);
8381
}
84-
_ => {},
8582
}
83+
_ => {},
8684
}
8785
}
88-
},
89-
// constant
90-
Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item),
91-
_ => {},
92-
}
93-
},
94-
ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item),
95-
_ => {},
96-
}
86+
}
87+
},
88+
// constant
89+
Res::Def(DefKind::Const, ..) => emit_lint(cx, vec, pushed_item),
90+
_ => {},
91+
}
92+
},
93+
ExprKind::Lit(..) => emit_lint(cx, vec, pushed_item),
94+
_ => {},
9795
}
9896
}
9997
}
10098
}
10199

102100
// Scans the body of the for loop and determines whether lint should be given
103101
struct SameItemPushVisitor<'a, 'tcx> {
104-
should_lint: bool,
102+
non_deterministic_expr: bool,
103+
multiple_pushes: bool,
105104
// this field holds the last vec push operation visited, which should be the only push seen
106105
vec_push: Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)>,
107106
cx: &'a LateContext<'tcx>,
107+
used_locals: FxHashSet<HirId>,
108+
}
109+
110+
impl<'a, 'tcx> SameItemPushVisitor<'a, 'tcx> {
111+
fn new(cx: &'a LateContext<'tcx>) -> Self {
112+
Self {
113+
non_deterministic_expr: false,
114+
multiple_pushes: false,
115+
vec_push: None,
116+
cx,
117+
used_locals: FxHashSet::default(),
118+
}
119+
}
120+
121+
fn should_lint(&self) -> bool {
122+
if_chain! {
123+
if !self.non_deterministic_expr;
124+
if !self.multiple_pushes;
125+
if let Some((vec, _)) = self.vec_push;
126+
if let Some(hir_id) = path_to_local(vec);
127+
then {
128+
!self.used_locals.contains(&hir_id)
129+
} else {
130+
false
131+
}
132+
}
133+
}
108134
}
109135

110136
impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> {
@@ -113,9 +139,14 @@ impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> {
113139
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
114140
match &expr.kind {
115141
// Non-determinism may occur ... don't give a lint
116-
ExprKind::Loop(..) | ExprKind::Match(..) => self.should_lint = false,
142+
ExprKind::Loop(..) | ExprKind::Match(..) | ExprKind::If(..) => self.non_deterministic_expr = true,
117143
ExprKind::Block(block, _) => self.visit_block(block),
118-
_ => {},
144+
_ => {
145+
if let Some(hir_id) = path_to_local(expr) {
146+
self.used_locals.insert(hir_id);
147+
}
148+
walk_expr(self, expr);
149+
},
119150
}
120151
}
121152

@@ -140,7 +171,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SameItemPushVisitor<'a, 'tcx> {
140171
self.vec_push = vec_push_option;
141172
} else {
142173
// There are multiple pushes ... don't lint
143-
self.should_lint = false;
174+
self.multiple_pushes = true;
144175
}
145176
}
146177
}

‎tests/ui/same_item_push.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,4 +148,11 @@ fn main() {
148148
};
149149
vec.push(item);
150150
}
151+
152+
// Fix #6987
153+
let mut vec = Vec::new();
154+
for _ in 0..10 {
155+
vec.push(1);
156+
vec.extend(&[2]);
157+
}
151158
}

0 commit comments

Comments
 (0)