Skip to content

Commit

Permalink
Restrict same_item_push to suppress false positives
Browse files Browse the repository at this point in the history 
It emits a lint when the pushed item is a literal, a constant and an immutable binding that are initialized with those.
  • Loading branch information
@giraffate @flip1995
giraffate authored and flip1995 committed Sep 10, 2020
1 parent 09bd400 commit 1778a1e
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 36 deletions.
118 changes: 95 additions & 23 deletions clippy_lints/src/loops.rs
View file
Expand Up @@ -826,7 +826,7 @@ struct FixedOffsetVar<'hir> {
}

fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'_>) -> bool {
let is_slice = match ty.kind {
let is_slice = match ty.kind() {
ty::Ref(_, subty, _) => is_slice_like(cx, subty),
ty::Slice(..) | ty::Array(..) => true,
_ => false,
Expand Down Expand Up @@ -1003,7 +1003,7 @@ fn detect_manual_memcpy<'tcx>(
start: Some(start),
end: Some(end),
limits,
}) = higher::range(cx, arg)
}) = higher::range(arg)
{
// the var must be a single name
if let PatKind::Binding(_, canonical_id, _, _) = pat.kind {
Expand Down Expand Up @@ -1140,23 +1140,95 @@ fn detect_same_item_push<'tcx>(
walk_expr(&mut same_item_push_visitor, body);
if same_item_push_visitor.should_lint {
if let Some((vec, pushed_item)) = same_item_push_visitor.vec_push {
// Make sure that the push does not involve possibly mutating values
if mutated_variables(pushed_item, cx).map_or(false, |mutvars| mutvars.is_empty()) {
let vec_ty = cx.typeck_results().expr_ty(vec);
let ty = vec_ty.walk().nth(1).unwrap().expect_ty();
if cx
.tcx
.lang_items()
.clone_trait()
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
{
// Make sure that the push does not involve possibly mutating values
if let PatKind::Wild = pat.kind {
let vec_str = snippet_with_macro_callsite(cx, vec.span, "");
let item_str = snippet_with_macro_callsite(cx, pushed_item.span, "");

span_lint_and_help(
cx,
SAME_ITEM_PUSH,
vec.span,
"it looks like the same item is being pushed into this Vec",
None,
&format!(
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
item_str, vec_str, item_str
),
)
if let ExprKind::Path(ref qpath) = pushed_item.kind {
match qpath_res(cx, qpath, pushed_item.hir_id) {
// immutable bindings that are initialized with literal or constant
Res::Local(hir_id) => {
if_chain! {
let node = cx.tcx.hir().get(hir_id);
if let Node::Binding(pat) = node;
if let PatKind::Binding(bind_ann, ..) = pat.kind;
if !matches!(bind_ann, BindingAnnotation::RefMut | BindingAnnotation::Mutable);
let parent_node = cx.tcx.hir().get_parent_node(hir_id);
if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node);
if let rustc_hir::Local { init: Some(init), .. } = parent_let_expr;
then {
match init.kind {
// immutable bindings that are initialized with literal
ExprKind::Lit(..) => {
span_lint_and_help(
cx,
SAME_ITEM_PUSH,
vec.span,
"it looks like the same item is being pushed into this Vec",
None,
&format!(
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
item_str, vec_str, item_str
),
)
},
// immutable bindings that are initialized with constant
ExprKind::Path(ref path) => {
if let Res::Def(DefKind::Const, ..) = qpath_res(cx, path, init.hir_id) {
span_lint_and_help(
cx,
SAME_ITEM_PUSH,
vec.span,
"it looks like the same item is being pushed into this Vec",
None,
&format!(
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
item_str, vec_str, item_str
),
)
}
}
_ => {},
}
}
}
},
// constant
Res::Def(DefKind::Const, ..) => span_lint_and_help(
cx,
SAME_ITEM_PUSH,
vec.span,
"it looks like the same item is being pushed into this Vec",
None,
&format!(
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
item_str, vec_str, item_str
),
),
_ => {},
}
} else if let ExprKind::Lit(..) = pushed_item.kind {
// literal
span_lint_and_help(
cx,
SAME_ITEM_PUSH,
vec.span,
"it looks like the same item is being pushed into this Vec",
None,
&format!(
"try using vec![{};SIZE] or {}.resize(NEW_SIZE, {})",
item_str, vec_str, item_str
),
)
}
}
}
}
Expand All @@ -1177,7 +1249,7 @@ fn check_for_loop_range<'tcx>(
start: Some(start),
ref end,
limits,
}) = higher::range(cx, arg)
}) = higher::range(arg)
{
// the var must be a single name
if let PatKind::Binding(_, canonical_id, ident, _) = pat.kind {
Expand Down Expand Up @@ -1355,7 +1427,7 @@ fn is_end_eq_array_len<'tcx>(
if_chain! {
if let ExprKind::Lit(ref lit) = end.kind;
if let ast::LitKind::Int(end_int, _) = lit.node;
if let ty::Array(_, arr_len_const) = indexed_ty.kind;
if let ty::Array(_, arr_len_const) = indexed_ty.kind();
if let Some(arr_len) = arr_len_const.try_eval_usize(cx.tcx, cx.param_env);
then {
return match limits {
Expand Down Expand Up @@ -1592,7 +1664,7 @@ fn check_for_loop_over_map_kv<'tcx>(
if let PatKind::Tuple(ref pat, _) = pat.kind {
if pat.len() == 2 {
let arg_span = arg.span;
let (new_pat_span, kind, ty, mutbl) = match cx.typeck_results().expr_ty(arg).kind {
let (new_pat_span, kind, ty, mutbl) = match *cx.typeck_results().expr_ty(arg).kind() {
ty::Ref(_, ty, mutbl) => match (&pat[0].kind, &pat[1].kind) {
(key, _) if pat_is_wild(key, body) => (pat[1].span, "value", ty, mutbl),
(_, value) if pat_is_wild(value, body) => (pat[0].span, "key", ty, Mutability::Not),
Expand Down Expand Up @@ -1679,7 +1751,7 @@ fn check_for_mut_range_bound(cx: &LateContext<'_>, arg: &Expr<'_>, body: &Expr<'
start: Some(start),
end: Some(end),
..
}) = higher::range(cx, arg)
}) = higher::range(arg)
{
let mut_ids = vec![check_for_mutability(cx, start), check_for_mutability(cx, end)];
if mut_ids[0].is_some() || mut_ids[1].is_some() {
Expand Down Expand Up @@ -1920,7 +1992,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
for expr in args {
let ty = self.cx.typeck_results().expr_ty_adjusted(expr);
self.prefer_mutable = false;
if let ty::Ref(_, _, mutbl) = ty.kind {
if let ty::Ref(_, _, mutbl) = *ty.kind() {
if mutbl == Mutability::Mut {
self.prefer_mutable = true;
}
Expand All @@ -1932,7 +2004,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> {
let def_id = self.cx.typeck_results().type_dependent_def_id(expr.hir_id).unwrap();
for (ty, expr) in self.cx.tcx.fn_sig(def_id).inputs().skip_binder().iter().zip(args) {
self.prefer_mutable = false;
if let ty::Ref(_, _, mutbl) = ty.kind {
if let ty::Ref(_, _, mutbl) = *ty.kind() {
if mutbl == Mutability::Mut {
self.prefer_mutable = true;
}
Expand Down Expand Up @@ -2030,7 +2102,7 @@ fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {

fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
// IntoIterator is currently only implemented for array sizes <= 32 in rustc
match ty.kind {
match ty.kind() {
ty::Array(_, n) => n
.try_eval_usize(cx.tcx, cx.param_env)
.map_or(false, |val| (0..=32).contains(&val)),
Expand Down
38 changes: 38 additions & 0 deletions tests/ui/same_item_push.rs
View file
@@ -1,5 +1,7 @@
#![warn(clippy::same_item_push)]

const VALUE: u8 = 7;

fn mutate_increment(x: &mut u8) -> u8 {
*x += 1;
*x
Expand Down Expand Up @@ -86,4 +88,40 @@ fn main() {
for a in vec_a {
vec12.push(2u8.pow(a.kind));
}

// Fix #5902
let mut vec13: Vec<u8> = Vec::new();
let mut item = 0;
for _ in 0..10 {
vec13.push(item);
item += 10;
}

// Fix #5979
let mut vec14: Vec<std::fs::File> = Vec::new();
for _ in 0..10 {
vec14.push(std::fs::File::open("foobar").unwrap());
}
// Fix #5979
#[derive(Clone)]
struct S {}

trait T {}
impl T for S {}

let mut vec15: Vec<Box<dyn T>> = Vec::new();
for _ in 0..10 {
vec15.push(Box::new(S {}));
}

let mut vec16 = Vec::new();
for _ in 0..20 {
vec16.push(VALUE);
}

let mut vec17 = Vec::new();
let item = VALUE;
for _ in 0..20 {
vec17.push(item);
}
}
34 changes: 21 additions & 13 deletions tests/ui/same_item_push.stderr
View file
@@ -1,35 +1,43 @@
error: it looks like the same item is being pushed into this Vec
--> $DIR/same_item_push.rs:16:9
|
LL | spaces.push(vec![b' ']);
| ^^^^^^
|
= note: `-D clippy::same-item-push` implied by `-D warnings`
= help: try using vec![vec![b' '];SIZE] or spaces.resize(NEW_SIZE, vec![b' '])

error: it looks like the same item is being pushed into this Vec
--> $DIR/same_item_push.rs:22:9
--> $DIR/same_item_push.rs:24:9
|
LL | vec2.push(item);
| ^^^^
|
= note: `-D clippy::same-item-push` implied by `-D warnings`
= help: try using vec![item;SIZE] or vec2.resize(NEW_SIZE, item)

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

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

error: aborting due to 4 previous errors
error: it looks like the same item is being pushed into this Vec
--> $DIR/same_item_push.rs:119:9
|
LL | vec16.push(VALUE);
| ^^^^^
|
= help: try using vec![VALUE;SIZE] or vec16.resize(NEW_SIZE, VALUE)

error: it looks like the same item is being pushed into this Vec
--> $DIR/same_item_push.rs:125:9
|
LL | vec17.push(item);
| ^^^^^
|
= help: try using vec![item;SIZE] or vec17.resize(NEW_SIZE, item)

error: aborting due to 5 previous errors

0 comments on commit 1778a1e

Please sign in to comment.