From 0f3345e8b2f839f1d3e0c8472537d7d954828ccd Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 13 Oct 2018 13:51:53 -0700 Subject: [PATCH 01/38] OUT_OF_BOUNDS_INDEXING fix #3102 false negative --- clippy_lints/src/indexing_slicing.rs | 70 +++++++++++++++++++--------- tests/ui/indexing_slicing.rs | 5 ++ tests/ui/indexing_slicing.stderr | 14 +++++- 3 files changed, 66 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 984d725898d..8b7a1f7882b 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -111,17 +111,43 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { // Ranged indexes, i.e. &x[n..m], &x[n..], &x[..n] and &x[..] if let ty::Array(_, s) = ty.sty { let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); - // Index is a constant range. - if let Some((start, end)) = to_const_range(cx, range, size) { - if start > size || end > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - } - return; + + match to_const_range(cx, range, size) { + (None, None) => {}, + (Some(start), None) => { + if start > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + return; + } + }, + (None, Some(end)) => { + if end > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + return; + } + }, + (Some(start), Some(end)) => { + if start > size || end > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + } + // early return because both start and end are constant + return; + }, } } @@ -161,20 +187,20 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { } } -/// Returns an option containing a tuple with the start and end (exclusive) of -/// the range. +/// Returns a tuple of options with the start and end (exclusive) values of +/// the range. If the start or end is not constant, None is returned. fn to_const_range<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, range: Range<'_>, array_size: u128, -) -> Option<(u128, u128)> { +) -> (Option, Option) { let s = range .start .map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c)); let start = match s { - Some(Some(Constant::Int(x))) => x, - Some(_) => return None, - None => 0, + Some(Some(Constant::Int(x))) => Some(x), + Some(_) => None, + None => Some(0), }; let e = range @@ -182,13 +208,13 @@ fn to_const_range<'a, 'tcx>( .map(|expr| constant(cx, cx.tables, expr).map(|(c, _)| c)); let end = match e { Some(Some(Constant::Int(x))) => if range.limits == RangeLimits::Closed { - x + 1 + Some(x + 1) } else { - x + Some(x) }, - Some(_) => return None, - None => array_size, + Some(_) => None, + None => Some(array_size), }; - Some((start, end)) + (start, end) } diff --git a/tests/ui/indexing_slicing.rs b/tests/ui/indexing_slicing.rs index 6a32eb87491..ff154091bb8 100644 --- a/tests/ui/indexing_slicing.rs +++ b/tests/ui/indexing_slicing.rs @@ -91,4 +91,9 @@ fn main() { x[M]; // Ok, should not produce stderr. v[N]; v[M]; + + // issue 3102 + let num = 1; + &x[num..10]; // should trigger out of bounds error + &x[10..num]; // should trigger out of bounds error } diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index 7d847c7a673..c587269e3e5 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -267,5 +267,17 @@ error: indexing may panic. | = help: Consider using `.get(n)` or `.get_mut(n)` instead -error: aborting due to 37 previous errors +error: range is out of bounds + --> $DIR/indexing_slicing.rs:97:6 + | +97 | &x[num..10]; // should trigger out of bounds error + | ^^^^^^^^^^ + +error: range is out of bounds + --> $DIR/indexing_slicing.rs:98:6 + | +98 | &x[10..num]; // should trigger out of bounds error + | ^^^^^^^^^^ + +error: aborting due to 39 previous errors From 5c3928282699244f5e85a66f0cded09ea3dfbeda Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sun, 14 Oct 2018 07:49:28 -0700 Subject: [PATCH 02/38] out_of_bounds_indexing refactoring --- clippy_lints/src/indexing_slicing.rs | 65 +++++++++++++--------------- 1 file changed, 30 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 8b7a1f7882b..9f9c25f7728 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -108,46 +108,41 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { if let ExprKind::Index(ref array, ref index) = &expr.node { let ty = cx.tables.expr_ty(array); if let Some(range) = higher::range(cx, index) { + // Ranged indexes, i.e. &x[n..m], &x[n..], &x[..n] and &x[..] if let ty::Array(_, s) = ty.sty { let size: u128 = s.assert_usize(cx.tcx).unwrap().into(); - match to_const_range(cx, range, size) { - (None, None) => {}, - (Some(start), None) => { - if start > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - return; - } - }, - (None, Some(end)) => { - if end > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - return; - } - }, - (Some(start), Some(end)) => { - if start > size || end > size { - utils::span_lint( - cx, - OUT_OF_BOUNDS_INDEXING, - expr.span, - "range is out of bounds", - ); - } - // early return because both start and end are constant + let const_range = to_const_range(cx, range, size); + + if let (Some(start), _) = const_range { + if start > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); return; - }, + } + } + + if let (_, Some(end)) = const_range { + if end > size { + utils::span_lint( + cx, + OUT_OF_BOUNDS_INDEXING, + expr.span, + "range is out of bounds", + ); + return; + } + } + + if let (Some(_), Some(_)) = const_range { + // early return because both start and end are constants + // and we have proven above that they are in bounds + return; } } From 66d3672b2696918a074d193813af1f74d1c48be9 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Mon, 15 Oct 2018 04:44:39 -0700 Subject: [PATCH 03/38] out_of_bounds_indexing improved reporting of out of bounds value --- clippy_lints/src/indexing_slicing.rs | 4 +- tests/ui/indexing_slicing.stderr | 68 ++++++++++++++-------------- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index 9f9c25f7728..f960ab5958c 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -120,7 +120,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { utils::span_lint( cx, OUT_OF_BOUNDS_INDEXING, - expr.span, + range.start.map_or(expr.span, |start| start.span), "range is out of bounds", ); return; @@ -132,7 +132,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for IndexingSlicing { utils::span_lint( cx, OUT_OF_BOUNDS_INDEXING, - expr.span, + range.end.map_or(expr.span, |end| end.span), "range is out of bounds", ); return; diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index c587269e3e5..fafcb1bc485 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -48,18 +48,18 @@ error: slicing may panic. = help: Consider using `.get(n..)` or .get_mut(n..)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:30:6 + --> $DIR/indexing_slicing.rs:30:11 | 30 | &x[..=4]; - | ^^^^^^^ + | ^ | = note: `-D clippy::out-of-bounds-indexing` implied by `-D warnings` error: range is out of bounds - --> $DIR/indexing_slicing.rs:31:6 + --> $DIR/indexing_slicing.rs:31:11 | 31 | &x[1..5]; - | ^^^^^^^ + | ^ error: slicing may panic. --> $DIR/indexing_slicing.rs:32:6 @@ -70,34 +70,34 @@ error: slicing may panic. = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:32:6 + --> $DIR/indexing_slicing.rs:32:8 | 32 | &x[5..][..10]; // Two lint reports, one for [5..] and another for [..10]. - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:33:6 + --> $DIR/indexing_slicing.rs:33:8 | 33 | &x[5..]; - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:34:6 + --> $DIR/indexing_slicing.rs:34:10 | 34 | &x[..5]; - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:35:6 + --> $DIR/indexing_slicing.rs:35:8 | 35 | &x[5..].iter().map(|x| 2 * x).collect::>(); - | ^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:36:6 + --> $DIR/indexing_slicing.rs:36:12 | 36 | &x[0..=4]; - | ^^^^^^^^ + | ^ error: slicing may panic. --> $DIR/indexing_slicing.rs:37:6 @@ -148,46 +148,46 @@ error: slicing may panic. = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:60:6 + --> $DIR/indexing_slicing.rs:60:12 | 60 | &empty[1..5]; - | ^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:61:6 + --> $DIR/indexing_slicing.rs:61:16 | 61 | &empty[0..=4]; - | ^^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:62:6 + --> $DIR/indexing_slicing.rs:62:15 | 62 | &empty[..=4]; - | ^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:63:6 + --> $DIR/indexing_slicing.rs:63:12 | 63 | &empty[1..]; - | ^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:64:6 + --> $DIR/indexing_slicing.rs:64:14 | 64 | &empty[..4]; - | ^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:65:6 + --> $DIR/indexing_slicing.rs:65:16 | 65 | &empty[0..=0]; - | ^^^^^^^^^^^^ + | ^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:66:6 + --> $DIR/indexing_slicing.rs:66:15 | 66 | &empty[..=0]; - | ^^^^^^^^^^^ + | ^ error: indexing may panic. --> $DIR/indexing_slicing.rs:74:5 @@ -230,10 +230,10 @@ error: slicing may panic. = help: Consider using `.get(..n)`or `.get_mut(..n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:78:6 + --> $DIR/indexing_slicing.rs:78:8 | 78 | &x[10..][..100]; // Two lint reports, one for [10..] and another for [..100]. - | ^^^^^^^ + | ^^ error: slicing may panic. --> $DIR/indexing_slicing.rs:79:6 @@ -268,16 +268,16 @@ error: indexing may panic. = help: Consider using `.get(n)` or `.get_mut(n)` instead error: range is out of bounds - --> $DIR/indexing_slicing.rs:97:6 + --> $DIR/indexing_slicing.rs:97:13 | 97 | &x[num..10]; // should trigger out of bounds error - | ^^^^^^^^^^ + | ^^ error: range is out of bounds - --> $DIR/indexing_slicing.rs:98:6 + --> $DIR/indexing_slicing.rs:98:8 | 98 | &x[10..num]; // should trigger out of bounds error - | ^^^^^^^^^^ + | ^^ error: aborting due to 39 previous errors From 6e75050be07d5f457c8854e9392fd756ef211a06 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 19 Oct 2018 04:55:06 -0700 Subject: [PATCH 04/38] new_ret_no_self correct linting of tuple return types --- clippy_lints/src/methods/mod.rs | 11 +++++++++++ tests/ui/new_ret_no_self.rs | 28 ++++++++++++++++++++++++++++ tests/ui/new_ret_no_self.stderr | 8 +++++++- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6d22abf2d33..6e015487561 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -936,6 +936,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); +// println!("ret_ty: {:?}", ret_ty); +// println!("ret_ty.sty {:?}", ret_ty.sty); + // if return type is impl trait if let TyKind::Opaque(def_id, _) = ret_ty.sty { @@ -955,6 +958,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } + // if return type is tuple + if let TyKind::Tuple(list) = ret_ty.sty { + // then at least one of the types in the tuple must be Self + for ret_type in list { + if same_tys(cx, ty, ret_type) { return; } + } + } + if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 1a4b91cc9da..77731149678 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -91,3 +91,31 @@ impl V { unimplemented!(); } } + +struct TupleReturnerOk; + +impl TupleReturnerOk { + // should not trigger lint + pub fn new() -> (Self, u32) { unimplemented!(); } +} + +struct TupleReturnerOk2; + +impl TupleReturnerOk2 { + // should not trigger lint (it doesn't matter which element in the tuple is Self) + pub fn new() -> (u32, Self) { unimplemented!(); } +} + +struct TupleReturnerOk3; + +impl TupleReturnerOk3 { + // should not trigger lint (tuple can contain multiple Self) + pub fn new() -> (Self, Self) { unimplemented!(); } +} + +struct TupleReturnerBad; + +impl TupleReturnerBad { + // should trigger lint + pub fn new() -> (u32, u32) { unimplemented!(); } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index ad26438d4ef..6f8e2d136a7 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -24,5 +24,11 @@ error: methods called `new` usually return `Self` 92 | | } | |_____^ -error: aborting due to 3 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:120:5 + | +120 | pub fn new() -> (u32, u32) { unimplemented!(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 4 previous errors From 097df8f223e8a3d10ea8bd66ecd94ead376c4416 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 19 Oct 2018 05:20:33 -0700 Subject: [PATCH 05/38] new_ret_no_self correct false positive on raw pointer return types --- clippy_lints/src/methods/mod.rs | 6 ++++++ tests/ui/new_ret_no_self.rs | 21 +++++++++++++++++++++ tests/ui/new_ret_no_self.stderr | 8 +++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6e015487561..0992067636e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -966,6 +966,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } + // if return type is mutable pointer + if let TyKind::RawPtr(ty::TypeAndMut{ty: ret_type, ..}) = ret_ty.sty { + // then the pointer must point to Self + if same_tys(cx, ty, ret_type) { return; } + } + if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 77731149678..b267a3aecdf 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -119,3 +119,24 @@ impl TupleReturnerBad { // should trigger lint pub fn new() -> (u32, u32) { unimplemented!(); } } + +struct MutPointerReturnerOk; + +impl MutPointerReturnerOk { + // should not trigger lint + pub fn new() -> *mut Self { unimplemented!(); } +} + +struct MutPointerReturnerOk2; + +impl MutPointerReturnerOk2 { + // should not trigger lint + pub fn new() -> *const Self { unimplemented!(); } +} + +struct MutPointerReturnerBad; + +impl MutPointerReturnerBad { + // should trigger lint + pub fn new() -> *mut V { unimplemented!(); } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index 6f8e2d136a7..20f0dbbe8a3 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -30,5 +30,11 @@ error: methods called `new` usually return `Self` 120 | pub fn new() -> (u32, u32) { unimplemented!(); } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 4 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:141:5 + | +141 | pub fn new() -> *mut V { unimplemented!(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 5 previous errors From 2e9172aea2b169e6f4ee888c4bea9d56eaf6d78e Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Fri, 19 Oct 2018 16:34:16 -0400 Subject: [PATCH 06/38] Check for known array length in `needless_range_loop` --- clippy_lints/src/loops.rs | 38 +++++++++++++++++++++++++---- tests/ui/needless_range_loop.rs | 16 +++++++++++- tests/ui/needless_range_loop.stderr | 32 +++++++++++++++++++++++- 3 files changed, 79 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 9e45757f3f0..0d1b960cc1f 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1068,7 +1068,7 @@ fn check_for_loop_range<'a, 'tcx>( // linting condition: we only indexed one variable, and indexed it directly if visitor.indexed_indirectly.is_empty() && visitor.indexed_directly.len() == 1 { - let (indexed, indexed_extent) = visitor + let (indexed, (indexed_extent, indexed_ty)) = visitor .indexed_directly .into_iter() .next() @@ -1119,7 +1119,7 @@ fn check_for_loop_range<'a, 'tcx>( } } - if is_len_call(end, indexed) { + if is_len_call(end, indexed) || is_end_eq_array_len(cx, end, limits, indexed_ty) { String::new() } else { match limits { @@ -1207,6 +1207,28 @@ fn is_len_call(expr: &Expr, var: Name) -> bool { false } +fn is_end_eq_array_len( + cx: &LateContext<'_, '_>, + end: &Expr, + limits: ast::RangeLimits, + indexed_ty: Ty<'_>, +) -> bool { + if_chain! { + if let ExprKind::Lit(ref lit) = end.node; + if let ast::LitKind::Int(end_int, _) = lit.node; + if let ty::TyKind::Array(_, arr_len_const) = indexed_ty.sty; + if let Some(arr_len) = arr_len_const.assert_usize(cx.tcx); + then { + return match limits { + ast::RangeLimits::Closed => end_int + 1 >= arr_len.into(), + ast::RangeLimits::HalfOpen => end_int >= arr_len.into(), + }; + } + } + + false +} + fn check_for_loop_reverse_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arg: &'tcx Expr, expr: &'tcx Expr) { // if this for loop is iterating over a two-sided range... if let Some(higher::Range { @@ -1678,7 +1700,7 @@ struct VarVisitor<'a, 'tcx: 'a> { indexed_indirectly: FxHashMap>, /// subset of `indexed` of vars that are indexed directly: `v[i]` /// this will not contain cases like `v[calc_index(i)]` or `v[(i + 4) % N]` - indexed_directly: FxHashMap>, + indexed_directly: FxHashMap, Ty<'tcx>)>, /// Any names that are used outside an index operation. /// Used to detect things like `&mut vec` used together with `vec[i]` referenced: FxHashSet, @@ -1725,7 +1747,10 @@ impl<'a, 'tcx> VarVisitor<'a, 'tcx> { self.indexed_indirectly.insert(seqvar.segments[0].ident.name, Some(extent)); } if index_used_directly { - self.indexed_directly.insert(seqvar.segments[0].ident.name, Some(extent)); + self.indexed_directly.insert( + seqvar.segments[0].ident.name, + (Some(extent), self.cx.tables.node_id_to_type(seqexpr.hir_id)), + ); } return false; // no need to walk further *on the variable* } @@ -1734,7 +1759,10 @@ impl<'a, 'tcx> VarVisitor<'a, 'tcx> { self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None); } if index_used_directly { - self.indexed_directly.insert(seqvar.segments[0].ident.name, None); + self.indexed_directly.insert( + seqvar.segments[0].ident.name, + (None, self.cx.tables.node_id_to_type(seqexpr.hir_id)), + ); } return false; // no need to walk further *on the variable* } diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs index 3da9267d38b..c1992bba548 100644 --- a/tests/ui/needless_range_loop.rs +++ b/tests/ui/needless_range_loop.rs @@ -13,7 +13,7 @@ fn calc_idx(i: usize) -> usize { } fn main() { - let ns = [2, 3, 5, 7]; + let ns = vec![2, 3, 5, 7]; for i in 3..10 { println!("{}", ns[i]); @@ -76,4 +76,18 @@ fn main() { for i in x..=x + 4 { vec[i] += 1; } + + let arr = [1,2,3]; + + for i in 0..3 { + println!("{}", arr[i]); + } + + for i in 0..2 { + println!("{}", arr[i]); + } + + for i in 1..3 { + println!("{}", arr[i]); + } } diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index d62a0434d0b..688e9fc3a2c 100644 --- a/tests/ui/needless_range_loop.stderr +++ b/tests/ui/needless_range_loop.stderr @@ -50,5 +50,35 @@ help: consider using an iterator 76 | for in vec.iter_mut().skip(x).take(4 + 1) { | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: the loop variable `i` is only used to index `arr`. + --> $DIR/needless_range_loop.rs:82:14 + | +82 | for i in 0..3 { + | ^^^^ +help: consider using an iterator + | +82 | for in &arr { + | ^^^^^^ ^^^^ + +error: the loop variable `i` is only used to index `arr`. + --> $DIR/needless_range_loop.rs:86:14 + | +86 | for i in 0..2 { + | ^^^^ +help: consider using an iterator + | +86 | for in arr.iter().take(2) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^ + +error: the loop variable `i` is only used to index `arr`. + --> $DIR/needless_range_loop.rs:90:14 + | +90 | for i in 1..3 { + | ^^^^ +help: consider using an iterator + | +90 | for in arr.iter().skip(1) { + | ^^^^^^ ^^^^^^^^^^^^^^^^^^ + +error: aborting due to 8 previous errors From 553d01d9c74477a8172581aba00d179fcd63a78f Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Fri, 19 Oct 2018 17:17:13 -0400 Subject: [PATCH 07/38] Update `ui/for_loop` test output --- tests/ui/for_loop.stderr | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 33176335783..695209de53f 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -97,8 +97,8 @@ error: the loop variable `j` is only used to index `STATIC`. | ^^^^ help: consider using an iterator | -110 | for in STATIC.iter().take(4) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^^ +110 | for in &STATIC { + | ^^^^^^ ^^^^^^^ error: the loop variable `j` is only used to index `CONST`. --> $DIR/for_loop.rs:114:14 @@ -107,8 +107,8 @@ error: the loop variable `j` is only used to index `CONST`. | ^^^^ help: consider using an iterator | -114 | for in CONST.iter().take(4) { - | ^^^^^^ ^^^^^^^^^^^^^^^^^^^^ +114 | for in &CONST { + | ^^^^^^ ^^^^^^ error: the loop variable `i` is used to index `vec` --> $DIR/for_loop.rs:118:14 From 079f9f45b5a44a0feaf60d56576758dd7b0c9fdd Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Fri, 19 Oct 2018 17:54:25 -0700 Subject: [PATCH 08/38] new_ret_no_self walk return type to check for self --- clippy_lints/src/methods/mod.rs | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 0992067636e..f0810c906ef 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -936,13 +936,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { if let hir::ImplItemKind::Method(_, _) = implitem.node { let ret_ty = return_ty(cx, implitem.id); -// println!("ret_ty: {:?}", ret_ty); -// println!("ret_ty.sty {:?}", ret_ty.sty); + // walk the return type and check for Self (this does not check associated types) + for inner_type in ret_ty.walk() { + if same_tys(cx, ty, inner_type) { return; } + } - // if return type is impl trait + // if return type is impl trait, check the associated types if let TyKind::Opaque(def_id, _) = ret_ty.sty { - // then one of the associated types must be Self + // one of the associated types must be Self for predicate in cx.tcx.predicates_of(def_id).predicates.iter() { match predicate { (Predicate::Projection(poly_projection_predicate), _) => { @@ -958,20 +960,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { } } - // if return type is tuple - if let TyKind::Tuple(list) = ret_ty.sty { - // then at least one of the types in the tuple must be Self - for ret_type in list { - if same_tys(cx, ty, ret_type) { return; } - } - } - - // if return type is mutable pointer - if let TyKind::RawPtr(ty::TypeAndMut{ty: ret_type, ..}) = ret_ty.sty { - // then the pointer must point to Self - if same_tys(cx, ty, ret_type) { return; } - } - if name == "new" && !same_tys(cx, ret_ty, ty) { span_lint(cx, NEW_RET_NO_SELF, From a6245835573760d7e2dfd6a608af40fab7ba5223 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Sat, 20 Oct 2018 06:29:17 -0700 Subject: [PATCH 09/38] new_ret_no_self added test cases --- clippy_lints/src/types.rs | 1 - tests/ui/new_ret_no_self.rs | 21 +++++++++++++++++++++ tests/ui/new_ret_no_self.stderr | 8 +++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 59c55168232..035ca2b0496 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1920,7 +1920,6 @@ enum ImplicitHasherType<'tcx> { impl<'tcx> ImplicitHasherType<'tcx> { /// Checks that `ty` is a target type without a BuildHasher. - #[allow(clippy::new_ret_no_self)] fn new<'a>(cx: &LateContext<'a, 'tcx>, hir_ty: &hir::Ty) -> Option { if let TyKind::Path(QPath::Resolved(None, ref path)) = hir_ty.node { let params: Vec<_> = path.segments.last().as_ref()?.args.as_ref()? diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index b267a3aecdf..b7daf3d49bc 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -140,3 +140,24 @@ impl MutPointerReturnerBad { // should trigger lint pub fn new() -> *mut V { unimplemented!(); } } + +struct GenericReturnerOk; + +impl GenericReturnerOk { + // should not trigger lint + pub fn new() -> Option { unimplemented!(); } +} + +struct GenericReturnerBad; + +impl GenericReturnerBad { + // should trigger lint + pub fn new() -> Option { unimplemented!(); } +} + +struct NestedReturnerOk; + +impl NestedReturnerOk { + // should trigger lint + pub fn new() -> (Option, u32) { unimplemented!(); } +} diff --git a/tests/ui/new_ret_no_self.stderr b/tests/ui/new_ret_no_self.stderr index 20f0dbbe8a3..bab9627ca22 100644 --- a/tests/ui/new_ret_no_self.stderr +++ b/tests/ui/new_ret_no_self.stderr @@ -36,5 +36,11 @@ error: methods called `new` usually return `Self` 141 | pub fn new() -> *mut V { unimplemented!(); } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: methods called `new` usually return `Self` + --> $DIR/new_ret_no_self.rs:155:5 + | +155 | pub fn new() -> Option { unimplemented!(); } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 6 previous errors From 8fc84b1f5504ce8812b6b99e04aa30263ffd73bc Mon Sep 17 00:00:00 2001 From: flip1995 Date: Mon, 22 Oct 2018 13:09:48 +0200 Subject: [PATCH 10/38] Setup bors --- .travis.yml | 9 +++++++++ bors.toml | 4 ++++ 2 files changed, 13 insertions(+) create mode 100644 bors.toml diff --git a/.travis.yml b/.travis.yml index 818353e0c16..97cec5ee86b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,6 +9,15 @@ os: sudo: false +branches: + only: + # This is where pull requests from "bors r+" are built. + - staging + # This is where pull requests from "bors try" are built. + - trying + # Also build pull requests. + - master + env: global: - RUST_BACKTRACE=1 diff --git a/bors.toml b/bors.toml new file mode 100644 index 00000000000..4e6e85f45fe --- /dev/null +++ b/bors.toml @@ -0,0 +1,4 @@ +status = [ + "continuous-integration/travis-ci/push", + "continuous-integration/appveyor/branch" +] From 9086730dc43329d2bb11f92078e2eb1f6b040421 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Mon, 22 Oct 2018 17:30:01 +0200 Subject: [PATCH 11/38] Add branch configuration to appveyor.yml --- appveyor.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index f50d1e88a24..8ab4a994476 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -6,7 +6,16 @@ environment: #- TARGET: i686-pc-windows-msvc #- TARGET: x86_64-pc-windows-gnu - TARGET: x86_64-pc-windows-msvc - + +branches: + only: + # This is where pull requests from "bors r+" are built. + - staging + # This is where pull requests from "bors try" are built. + - trying + # Also build pull requests. + - master + install: - curl -sSf -o rustup-init.exe https://win.rustup.rs/ - rustup-init.exe -y --default-host %TARGET% --default-toolchain nightly From fd3651a55151086390a877d5c8773647742f51c3 Mon Sep 17 00:00:00 2001 From: Guillem Nieto Date: Tue, 23 Oct 2018 23:03:23 +0200 Subject: [PATCH 12/38] Fix inspector pass documentation When using `#[clippy_dump]`, the compiler complains about an unknown attribute. The correct one seems to be `#[clippy::dump]`. --- clippy_lints/src/utils/inspector.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/utils/inspector.rs b/clippy_lints/src/utils/inspector.rs index ea48aa9ab5e..54ca0736c52 100644 --- a/clippy_lints/src/utils/inspector.rs +++ b/clippy_lints/src/utils/inspector.rs @@ -19,12 +19,12 @@ use crate::rustc::hir::print; use crate::syntax::ast::Attribute; use crate::utils::get_attr; -/// **What it does:** Dumps every ast/hir node which has the `#[clippy_dump]` +/// **What it does:** Dumps every ast/hir node which has the `#[clippy::dump]` /// attribute /// /// **Example:** /// ```rust -/// #[clippy_dump] +/// #[clippy::dump] /// extern crate foo; /// ``` /// From 50b9e7aebc2e7b00f098736ff67f94300106f7fc Mon Sep 17 00:00:00 2001 From: Owen Sanchez Date: Sun, 21 Oct 2018 21:59:45 -0700 Subject: [PATCH 13/38] Don't emit `new_without_default_derive` if an impl of Default exists --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/new_without_default.rs | 37 ++++++++++++++++++++++--- tests/ui/new_without_default.rs | 9 ++++++ 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 23bd71a08ab..bf37b239064 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -391,7 +391,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_early_lint_pass(box int_plus_one::IntPlusOne); reg.register_late_lint_pass(box overflow_check_conditional::OverflowCheckConditional); reg.register_late_lint_pass(box unused_label::UnusedLabel); - reg.register_late_lint_pass(box new_without_default::NewWithoutDefault); + reg.register_late_lint_pass(box new_without_default::NewWithoutDefault::default()); reg.register_late_lint_pass(box blacklisted_name::BlackListedName::new(conf.blacklisted_names.clone())); reg.register_late_lint_pass(box functions::Functions::new(conf.too_many_arguments_threshold)); reg.register_early_lint_pass(box doc::Doc::new(conf.doc_valid_idents.clone())); diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index 865b1c987c8..21b966a6bd9 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_without_default.rs @@ -11,6 +11,7 @@ use crate::rustc::hir::def_id::DefId; use crate::rustc::hir; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass, in_external_macro, LintContext}; +use crate::rustc::util::nodemap::NodeSet; use crate::rustc::{declare_tool_lint, lint_array}; use if_chain::if_chain; use crate::rustc::ty::{self, Ty}; @@ -91,8 +92,10 @@ declare_clippy_lint! { "`fn new() -> Self` without `#[derive]`able `Default` implementation" } -#[derive(Copy, Clone)] -pub struct NewWithoutDefault; +#[derive(Clone, Default)] +pub struct NewWithoutDefault { + impling_types: Option, +} impl LintPass for NewWithoutDefault { fn get_lints(&self) -> LintArray { @@ -130,13 +133,39 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NewWithoutDefault { return; } if sig.decl.inputs.is_empty() && name == "new" && cx.access_levels.is_reachable(id) { + let self_did = cx.tcx.hir.local_def_id(cx.tcx.hir.get_parent(id)); let self_ty = cx.tcx - .type_of(cx.tcx.hir.local_def_id(cx.tcx.hir.get_parent(id))); + .type_of(self_did); if_chain! { if same_tys(cx, self_ty, return_ty(cx, id)); if let Some(default_trait_id) = get_trait_def_id(cx, &paths::DEFAULT_TRAIT); - if !implements_trait(cx, self_ty, default_trait_id, &[]); then { + if self.impling_types.is_none() { + let mut impls = NodeSet(); + cx.tcx.for_each_impl(default_trait_id, |d| { + if let Some(ty_def) = cx.tcx.type_of(d).ty_adt_def() { + if let Some(node_id) = cx.tcx.hir.as_local_node_id(ty_def.did) { + impls.insert(node_id); + } + } + }); + self.impling_types = Some(impls); + } + + // Check if a Default implementation exists for the Self type, regardless of + // generics + if_chain! { + if let Some(ref impling_types) = self.impling_types; + if let Some(self_def) = cx.tcx.type_of(self_did).ty_adt_def(); + if self_def.did.is_local(); + then { + let self_id = cx.tcx.hir.local_def_id_to_node_id(self_def.did.to_local()); + if impling_types.contains(&self_id) { + return; + } + } + } + if let Some(sp) = can_derive_default(self_ty, cx, default_trait_id) { span_lint_and_then( cx, diff --git a/tests/ui/new_without_default.rs b/tests/ui/new_without_default.rs index 783308d264a..16b9bd5c71b 100644 --- a/tests/ui/new_without_default.rs +++ b/tests/ui/new_without_default.rs @@ -107,4 +107,13 @@ impl IgnoreUnsafeNew { pub unsafe fn new() -> Self { IgnoreUnsafeNew } } +#[derive(Default)] +pub struct OptionRefWrapper<'a, T: 'a>(Option<&'a T>); + +impl<'a, T: 'a> OptionRefWrapper<'a, T> { + pub fn new() -> Self { + OptionRefWrapper(None) + } +} + fn main() {} From 0263ddde92d865a7e42dfe0d567286389d032faf Mon Sep 17 00:00:00 2001 From: Hidehito Yabuuchi Date: Mon, 22 Oct 2018 22:39:51 +0900 Subject: [PATCH 14/38] Lint for wildcard dependencies in Cargo.toml --- clippy_lints/src/lib.rs | 3 + clippy_lints/src/wildcard_dependencies.rs | 70 +++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100644 clippy_lints/src/wildcard_dependencies.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 23bd71a08ab..b9f437a953f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -200,6 +200,7 @@ pub mod unused_label; pub mod unwrap; pub mod use_self; pub mod vec; +pub mod wildcard_dependencies; pub mod write; pub mod zero_div_zero; // end lints modules, do not remove this comment, it’s used in `update_lints` @@ -438,6 +439,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box question_mark::Pass); reg.register_late_lint_pass(box suspicious_trait_impl::SuspiciousImpl); reg.register_early_lint_pass(box multiple_crate_versions::Pass); + reg.register_early_lint_pass(box wildcard_dependencies::Pass); reg.register_late_lint_pass(box map_unit_fn::Pass); reg.register_late_lint_pass(box infallible_destructuring_match::Pass); reg.register_late_lint_pass(box inherent_impl::Pass::default()); @@ -967,6 +969,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::cargo", Some("clippy_cargo"), vec![ multiple_crate_versions::MULTIPLE_CRATE_VERSIONS, + wildcard_dependencies::WILDCARD_DEPENDENCIES, ]); reg.register_lint_group("clippy::nursery", Some("clippy_nursery"), vec![ diff --git a/clippy_lints/src/wildcard_dependencies.rs b/clippy_lints/src/wildcard_dependencies.rs new file mode 100644 index 00000000000..3045130231c --- /dev/null +++ b/clippy_lints/src/wildcard_dependencies.rs @@ -0,0 +1,70 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use crate::rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::syntax::ast::*; +use crate::utils::span_lint; + +use cargo_metadata; +use lazy_static::lazy_static; +use semver; + +/// **What it does:** Checks to see if wildcard dependencies are being used. +/// +/// **Why is this bad?** [As the edition guide sais](https://rust-lang-nursery.github.io/edition-guide/rust-2018/cargo-and-crates-io/crates-io-disallows-wildcard-dependencies.html), +/// it is highly unlikely that you work with any possible version of your dependency, +/// and wildcard dependencies would cause unnecessary breakage in the ecosystem. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```toml +/// [dependencies] +/// regex = "*" +/// ``` +declare_clippy_lint! { + pub WILDCARD_DEPENDENCIES, + cargo, + "wildcard dependencies being used" +} + +pub struct Pass; + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!(WILDCARD_DEPENDENCIES) + } +} + +impl EarlyLintPass for Pass { + fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &Crate) { + let metadata = if let Ok(metadata) = cargo_metadata::metadata(None) { + metadata + } else { + span_lint(cx, WILDCARD_DEPENDENCIES, krate.span, "could not read cargo metadata"); + return; + }; + + lazy_static! { + static ref WILDCARD_VERSION_REQ: semver::VersionReq = semver::VersionReq::parse("*").unwrap(); + } + + for dep in &metadata.packages[0].dependencies { + if dep.req == *WILDCARD_VERSION_REQ { + span_lint( + cx, + WILDCARD_DEPENDENCIES, + krate.span, + &format!("wildcard dependency for `{}`", dep.name), + ); + } + } + } +} From fa6c9f838cbe5a50b8e7871c146cd1d8fe8e4e00 Mon Sep 17 00:00:00 2001 From: Hidehito Yabuuchi Date: Wed, 24 Oct 2018 11:34:36 +0900 Subject: [PATCH 15/38] Minor changes on clippy_lints/src/wildcard_dependencies.rs --- clippy_lints/src/wildcard_dependencies.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/wildcard_dependencies.rs b/clippy_lints/src/wildcard_dependencies.rs index 3045130231c..e02501005e2 100644 --- a/clippy_lints/src/wildcard_dependencies.rs +++ b/clippy_lints/src/wildcard_dependencies.rs @@ -25,6 +25,7 @@ use semver; /// **Known problems:** None. /// /// **Example:** +/// /// ```toml /// [dependencies] /// regex = "*" @@ -53,6 +54,7 @@ impl EarlyLintPass for Pass { }; lazy_static! { + // VersionReq::any() does not work static ref WILDCARD_VERSION_REQ: semver::VersionReq = semver::VersionReq::parse("*").unwrap(); } From d334fab4d0c28bd59538864e02847876a032439c Mon Sep 17 00:00:00 2001 From: Hidehito Yabuuchi Date: Wed, 24 Oct 2018 14:59:19 +0900 Subject: [PATCH 16/38] Run util/update_lints.py --- CHANGELOG.md | 1 + README.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 626c39457e2..768751b2f08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -893,6 +893,7 @@ All notable changes to this project will be documented in this file. [`while_immutable_condition`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#while_immutable_condition [`while_let_loop`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#while_let_loop [`while_let_on_iterator`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#while_let_on_iterator +[`wildcard_dependencies`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#wildcard_dependencies [`write_literal`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#write_literal [`write_with_newline`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#write_with_newline [`writeln_empty_string`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#writeln_empty_string diff --git a/README.md b/README.md index d32f66b5957..94fb8c6c02f 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 280 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 281 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: From 663f2cff7e08301841dd212fcf945b706ca2f224 Mon Sep 17 00:00:00 2001 From: Hidehito Yabuuchi Date: Wed, 24 Oct 2018 20:18:19 +0900 Subject: [PATCH 17/38] Some fixes for wildcard_dependencies --- clippy_lints/src/wildcard_dependencies.rs | 28 +++++++++++------------ 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/wildcard_dependencies.rs b/clippy_lints/src/wildcard_dependencies.rs index e02501005e2..00533efb8e5 100644 --- a/clippy_lints/src/wildcard_dependencies.rs +++ b/clippy_lints/src/wildcard_dependencies.rs @@ -10,15 +10,15 @@ use crate::rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::syntax::ast::*; +use crate::syntax::source_map::DUMMY_SP; use crate::utils::span_lint; use cargo_metadata; -use lazy_static::lazy_static; use semver; -/// **What it does:** Checks to see if wildcard dependencies are being used. +/// **What it does:** Checks for wildcard dependencies in the `Cargo.toml`. /// -/// **Why is this bad?** [As the edition guide sais](https://rust-lang-nursery.github.io/edition-guide/rust-2018/cargo-and-crates-io/crates-io-disallows-wildcard-dependencies.html), +/// **Why is this bad?** [As the edition guide says](https://rust-lang-nursery.github.io/edition-guide/rust-2018/cargo-and-crates-io/crates-io-disallows-wildcard-dependencies.html), /// it is highly unlikely that you work with any possible version of your dependency, /// and wildcard dependencies would cause unnecessary breakage in the ecosystem. /// @@ -53,19 +53,17 @@ impl EarlyLintPass for Pass { return; }; - lazy_static! { - // VersionReq::any() does not work - static ref WILDCARD_VERSION_REQ: semver::VersionReq = semver::VersionReq::parse("*").unwrap(); - } - for dep in &metadata.packages[0].dependencies { - if dep.req == *WILDCARD_VERSION_REQ { - span_lint( - cx, - WILDCARD_DEPENDENCIES, - krate.span, - &format!("wildcard dependency for `{}`", dep.name), - ); + // VersionReq::any() does not work + if let Ok(wildcard_ver) = semver::VersionReq::parse("*") { + if dep.req == wildcard_ver { + span_lint( + cx, + WILDCARD_DEPENDENCIES, + DUMMY_SP, + &format!("wildcard dependency for `{}`", dep.name), + ); + } } } } From 0d577c36a9e48fce3b9a0a49b7d5d59f8710af35 Mon Sep 17 00:00:00 2001 From: Hidehito Yabuuchi Date: Wed, 24 Oct 2018 20:22:38 +0900 Subject: [PATCH 18/38] Use DUMMY_SP in multiple_crate_versions --- clippy_lints/src/multiple_crate_versions.rs | 12 +++--------- clippy_lints/src/wildcard_dependencies.rs | 3 +-- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/multiple_crate_versions.rs b/clippy_lints/src/multiple_crate_versions.rs index dbf8cbe16c2..9507522ed10 100644 --- a/clippy_lints/src/multiple_crate_versions.rs +++ b/clippy_lints/src/multiple_crate_versions.rs @@ -7,12 +7,11 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. - //! lint on multiple versions of a crate being used use crate::rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; -use crate::syntax::ast::*; +use crate::syntax::{ast::*, source_map::DUMMY_SP}; use crate::utils::span_lint; use cargo_metadata; @@ -54,12 +53,7 @@ impl EarlyLintPass for Pass { let metadata = if let Ok(metadata) = cargo_metadata::metadata_deps(None, true) { metadata } else { - span_lint( - cx, - MULTIPLE_CRATE_VERSIONS, - krate.span, - "could not read cargo metadata" - ); + span_lint(cx, MULTIPLE_CRATE_VERSIONS, krate.span, "could not read cargo metadata"); return; }; @@ -76,7 +70,7 @@ impl EarlyLintPass for Pass { span_lint( cx, MULTIPLE_CRATE_VERSIONS, - krate.span, + DUMMY_SP, &format!("multiple versions for dependency `{}`: {}", name, versions), ); } diff --git a/clippy_lints/src/wildcard_dependencies.rs b/clippy_lints/src/wildcard_dependencies.rs index 00533efb8e5..8f2f1aeb27a 100644 --- a/clippy_lints/src/wildcard_dependencies.rs +++ b/clippy_lints/src/wildcard_dependencies.rs @@ -9,8 +9,7 @@ use crate::rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; -use crate::syntax::ast::*; -use crate::syntax::source_map::DUMMY_SP; +use crate::syntax::{ast::*, source_map::DUMMY_SP}; use crate::utils::span_lint; use cargo_metadata; From 99b78f06506c4f04399d2186b434a5dd4e16e792 Mon Sep 17 00:00:00 2001 From: Hidehito Yabuuchi Date: Wed, 24 Oct 2018 21:15:27 +0900 Subject: [PATCH 19/38] Replace remaining `krate.span` with `DUMMY_SP` --- clippy_lints/src/multiple_crate_versions.rs | 2 +- clippy_lints/src/wildcard_dependencies.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/multiple_crate_versions.rs b/clippy_lints/src/multiple_crate_versions.rs index 9507522ed10..da1ccd8fdf0 100644 --- a/clippy_lints/src/multiple_crate_versions.rs +++ b/clippy_lints/src/multiple_crate_versions.rs @@ -53,7 +53,7 @@ impl EarlyLintPass for Pass { let metadata = if let Ok(metadata) = cargo_metadata::metadata_deps(None, true) { metadata } else { - span_lint(cx, MULTIPLE_CRATE_VERSIONS, krate.span, "could not read cargo metadata"); + span_lint(cx, MULTIPLE_CRATE_VERSIONS, DUMMY_SP, "could not read cargo metadata"); return; }; diff --git a/clippy_lints/src/wildcard_dependencies.rs b/clippy_lints/src/wildcard_dependencies.rs index 8f2f1aeb27a..c172b38a6cb 100644 --- a/clippy_lints/src/wildcard_dependencies.rs +++ b/clippy_lints/src/wildcard_dependencies.rs @@ -48,7 +48,7 @@ impl EarlyLintPass for Pass { let metadata = if let Ok(metadata) = cargo_metadata::metadata(None) { metadata } else { - span_lint(cx, WILDCARD_DEPENDENCIES, krate.span, "could not read cargo metadata"); + span_lint(cx, WILDCARD_DEPENDENCIES, DUMMY_SP, "could not read cargo metadata"); return; }; From 30ffc17ef754f6b6c7bd47809afd51b1c5632f22 Mon Sep 17 00:00:00 2001 From: Josh Mcguigan Date: Wed, 24 Oct 2018 06:43:21 -0700 Subject: [PATCH 20/38] new_ret_no_self added test cases --- tests/ui/new_ret_no_self.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index b7daf3d49bc..bed43f550f2 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/tests/ui/new_ret_no_self.rs @@ -158,6 +158,20 @@ impl GenericReturnerBad { struct NestedReturnerOk; impl NestedReturnerOk { - // should trigger lint + // should not trigger lint pub fn new() -> (Option, u32) { unimplemented!(); } } + +struct NestedReturnerOk2; + +impl NestedReturnerOk2 { + // should not trigger lint + pub fn new() -> ((Self, u32), u32) { unimplemented!(); } +} + +struct NestedReturnerOk3; + +impl NestedReturnerOk3 { + // should not trigger lint + pub fn new() -> Option<(Self, u32)> { unimplemented!(); } +} From 57a18b65206371eafadaf16561f27025c7e33d9e Mon Sep 17 00:00:00 2001 From: flip1995 Date: Wed, 24 Oct 2018 16:18:01 +0200 Subject: [PATCH 21/38] Fix warnings introduced by #3349 --- clippy_lints/src/multiple_crate_versions.rs | 2 +- clippy_lints/src/wildcard_dependencies.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/multiple_crate_versions.rs b/clippy_lints/src/multiple_crate_versions.rs index da1ccd8fdf0..c554c8729ce 100644 --- a/clippy_lints/src/multiple_crate_versions.rs +++ b/clippy_lints/src/multiple_crate_versions.rs @@ -49,7 +49,7 @@ impl LintPass for Pass { } impl EarlyLintPass for Pass { - fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &Crate) { + fn check_crate(&mut self, cx: &EarlyContext<'_>, _: &Crate) { let metadata = if let Ok(metadata) = cargo_metadata::metadata_deps(None, true) { metadata } else { diff --git a/clippy_lints/src/wildcard_dependencies.rs b/clippy_lints/src/wildcard_dependencies.rs index c172b38a6cb..59f3cc78afe 100644 --- a/clippy_lints/src/wildcard_dependencies.rs +++ b/clippy_lints/src/wildcard_dependencies.rs @@ -44,7 +44,7 @@ impl LintPass for Pass { } impl EarlyLintPass for Pass { - fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &Crate) { + fn check_crate(&mut self, cx: &EarlyContext<'_>, _: &Crate) { let metadata = if let Ok(metadata) = cargo_metadata::metadata(None) { metadata } else { From 0b9e9c9e3d3a163941414d4b7632ff37fd4da224 Mon Sep 17 00:00:00 2001 From: Owen Sanchez Date: Wed, 17 Oct 2018 21:20:36 -0700 Subject: [PATCH 22/38] Disable arithmetic lints in constant items --- clippy_lints/src/arithmetic.rs | 55 +++++++++++++++++++++++++++++----- tests/ui/arithmetic.rs | 32 ++++++++++++++++++++ 2 files changed, 79 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/arithmetic.rs b/clippy_lints/src/arithmetic.rs index 4d7e921567d..a481d46cce0 100644 --- a/clippy_lints/src/arithmetic.rs +++ b/clippy_lints/src/arithmetic.rs @@ -51,7 +51,9 @@ declare_clippy_lint! { #[derive(Copy, Clone, Default)] pub struct Arithmetic { - span: Option, + expr_span: Option, + /// This field is used to check whether expressions are constants, such as in enum discriminants and consts + const_span: Option, } impl LintPass for Arithmetic { @@ -62,9 +64,15 @@ impl LintPass for Arithmetic { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Arithmetic { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { - if self.span.is_some() { + if self.expr_span.is_some() { return; } + + if let Some(span) = self.const_span { + if span.contains(expr.span) { + return; + } + } match expr.node { hir::ExprKind::Binary(ref op, ref l, ref r) => { match op.node { @@ -86,20 +94,20 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Arithmetic { let (l_ty, r_ty) = (cx.tables.expr_ty(l), cx.tables.expr_ty(r)); if l_ty.is_integral() && r_ty.is_integral() { span_lint(cx, INTEGER_ARITHMETIC, expr.span, "integer arithmetic detected"); - self.span = Some(expr.span); + self.expr_span = Some(expr.span); } else if l_ty.is_floating_point() && r_ty.is_floating_point() { span_lint(cx, FLOAT_ARITHMETIC, expr.span, "floating-point arithmetic detected"); - self.span = Some(expr.span); + self.expr_span = Some(expr.span); } }, hir::ExprKind::Unary(hir::UnOp::UnNeg, ref arg) => { let ty = cx.tables.expr_ty(arg); if ty.is_integral() { span_lint(cx, INTEGER_ARITHMETIC, expr.span, "integer arithmetic detected"); - self.span = Some(expr.span); + self.expr_span = Some(expr.span); } else if ty.is_floating_point() { span_lint(cx, FLOAT_ARITHMETIC, expr.span, "floating-point arithmetic detected"); - self.span = Some(expr.span); + self.expr_span = Some(expr.span); } }, _ => (), @@ -107,8 +115,39 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Arithmetic { } fn check_expr_post(&mut self, _: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { - if Some(expr.span) == self.span { - self.span = None; + if Some(expr.span) == self.expr_span { + self.expr_span = None; } } + + fn check_body(&mut self, cx: &LateContext<'_, '_>, body: &hir::Body) { + let body_owner = cx.tcx.hir.body_owner(body.id()); + + match cx.tcx.hir.body_owner_kind(body_owner) { + hir::BodyOwnerKind::Static(_) + | hir::BodyOwnerKind::Const => { + let body_span = cx.tcx.hir.span(body_owner); + + if let Some(span) = self.const_span { + if span.contains(body_span) { + return; + } + } + self.const_span = Some(body_span); + } + hir::BodyOwnerKind::Fn => (), + } + } + + fn check_body_post(&mut self, cx: &LateContext<'_, '_>, body: &hir::Body) { + let body_owner = cx.tcx.hir.body_owner(body.id()); + let body_span = cx.tcx.hir.span(body_owner); + + if let Some(span) = self.const_span { + if span.contains(body_span) { + return; + } + } + self.const_span = None; + } } diff --git a/tests/ui/arithmetic.rs b/tests/ui/arithmetic.rs index ff550c9593c..61a601468fb 100644 --- a/tests/ui/arithmetic.rs +++ b/tests/ui/arithmetic.rs @@ -37,4 +37,36 @@ fn main() { f / 2.0; f - 2.0 * 4.2; -f; + + // No errors for the following items because they are constant expressions + enum Foo { + Bar = -2, + } + struct Baz([i32; 1 + 1]); + union Qux { + field: [i32; 1 + 1], + } + type Alias = [i32; 1 + 1]; + + const FOO: i32 = -2; + static BAR: i32 = -2; + + let _: [i32; 1 + 1] = [0, 0]; + + let _: [i32; 1 + 1] = { + let a: [i32; 1 + 1] = [0, 0]; + a + }; + + trait Trait { + const ASSOC: i32 = 1 + 1; + } + + impl Trait for Foo { + const ASSOC: i32 = { + let _: [i32; 1 + 1]; + fn foo() {} + 1 + 1 + }; + } } From 3db14f182c4dce4d6ace923c291456e7d1249bc9 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Wed, 24 Oct 2018 23:28:54 -0400 Subject: [PATCH 23/38] Check existential types in `use_self` --- clippy_lints/src/use_self.rs | 2 +- tests/ui/use_self.rs | 14 ++++++++++++++ tests/ui/use_self.stderr | 8 +++++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index d770ea120eb..a8b7e820681 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -226,6 +226,6 @@ impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { } fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { - NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir) + NestedVisitorMap::All(&self.cx.tcx.hir) } } diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 6ebe8f16a90..073d64d5a4b 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -205,3 +205,17 @@ mod issue2894 { } } } + +mod existential { + struct Foo; + + impl Foo { + fn bad(foos: &[Self]) -> impl Iterator { + foos.iter() + } + + fn good(foos: &[Self]) -> impl Iterator { + foos.iter() + } + } +} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 627fc3a97cb..b71c7a9a4c5 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -120,5 +120,11 @@ error: unnecessary structure name repetition 119 | fn mul(self, rhs: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` -error: aborting due to 20 previous errors +error: unnecessary structure name repetition + --> $DIR/use_self.rs:213:54 + | +213 | fn bad(foos: &[Self]) -> impl Iterator { + | ^^^ help: use the applicable keyword: `Self` + +error: aborting due to 21 previous errors From aabf8083bd7e3e45ca4701623ede99fb35dd0578 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Sat, 20 Oct 2018 23:46:13 -0400 Subject: [PATCH 24/38] Add lint for calling `mem::discriminant` on a non-enum type --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 4 ++ clippy_lints/src/mem_discriminant.rs | 93 ++++++++++++++++++++++++++++ clippy_lints/src/utils/paths.rs | 1 + tests/ui/mem_discriminant.rs | 44 +++++++++++++ tests/ui/mem_discriminant.stderr | 76 +++++++++++++++++++++++ 7 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/mem_discriminant.rs create mode 100644 tests/ui/mem_discriminant.rs create mode 100644 tests/ui/mem_discriminant.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 768751b2f08..9ad6514ee10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -744,6 +744,7 @@ All notable changes to this project will be documented in this file. [`match_same_arms`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#match_same_arms [`match_wild_err_arm`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#match_wild_err_arm [`maybe_infinite_iter`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#maybe_infinite_iter +[`mem_discriminant_non_enum`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum [`mem_forget`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mem_forget [`mem_replace_option_with_none`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mem_replace_option_with_none [`min_max`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#min_max diff --git a/README.md b/README.md index 94fb8c6c02f..a13d8ecef66 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 281 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 282 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8b2070561d2..7a91890633a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -144,6 +144,7 @@ pub mod loops; pub mod map_clone; pub mod map_unit_fn; pub mod matches; +pub mod mem_discriminant; pub mod mem_forget; pub mod mem_replace; pub mod methods; @@ -398,6 +399,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_early_lint_pass(box doc::Doc::new(conf.doc_valid_idents.clone())); reg.register_late_lint_pass(box neg_multiply::NegMultiply); reg.register_early_lint_pass(box unsafe_removed_from_name::UnsafeNameRemoval); + reg.register_late_lint_pass(box mem_discriminant::MemDiscriminant); reg.register_late_lint_pass(box mem_forget::MemForget); reg.register_late_lint_pass(box mem_replace::MemReplace); reg.register_late_lint_pass(box arithmetic::Arithmetic::default()); @@ -612,6 +614,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { matches::MATCH_REF_PATS, matches::MATCH_WILD_ERR_ARM, matches::SINGLE_MATCH, + mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, mem_replace::MEM_REPLACE_OPTION_WITH_NONE, methods::CHARS_LAST_CMP, methods::CHARS_NEXT_CMP, @@ -924,6 +927,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { loops::NEVER_LOOP, loops::REVERSE_RANGE_LOOP, loops::WHILE_IMMUTABLE_CONDITION, + mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, methods::CLONE_DOUBLE_REF, methods::TEMPORARY_CSTRING_AS_PTR, minmax::MIN_MAX, diff --git a/clippy_lints/src/mem_discriminant.rs b/clippy_lints/src/mem_discriminant.rs new file mode 100644 index 00000000000..356162fd6f4 --- /dev/null +++ b/clippy_lints/src/mem_discriminant.rs @@ -0,0 +1,93 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + + +use crate::rustc::hir::{Expr, ExprKind}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::utils::{match_def_path, opt_def_id, paths, snippet, span_lint_and_then, walk_ptrs_ty_depth}; +use if_chain::if_chain; + +use std::iter; + +/// **What it does:** Checks for calls of `mem::discriminant()` on a non-enum type. +/// +/// **Why is this bad?** The value of `mem::discriminant()` on non-enum types +/// is unspecified. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// mem::discriminant(&"hello"); +/// mem::discriminant(&&Some(2)); +/// ``` +declare_clippy_lint! { + pub MEM_DISCRIMINANT_NON_ENUM, + correctness, + "calling mem::descriminant on non-enum type" +} + +pub struct MemDiscriminant; + +impl LintPass for MemDiscriminant { + fn get_lints(&self) -> LintArray { + lint_array![MEM_DISCRIMINANT_NON_ENUM] + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemDiscriminant { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + if_chain! { + if let ExprKind::Call(ref func, ref func_args) = expr.node; + // is `mem::discriminant` + if let ExprKind::Path(ref func_qpath) = func.node; + if let Some(def_id) = opt_def_id(cx.tables.qpath_def(func_qpath, func.hir_id)); + if match_def_path(cx.tcx, def_id, &paths::MEM_DISCRIMINANT); + // type is non-enum + let ty_param = cx.tables.node_substs(func.hir_id).type_at(0); + if !ty_param.is_enum(); + + then { + span_lint_and_then( + cx, + MEM_DISCRIMINANT_NON_ENUM, + expr.span, + &format!("calling `mem::discriminant` on non-enum type `{}`", ty_param), + |db| { + // if this is a reference to an enum, suggest dereferencing + let (base_ty, ptr_depth) = walk_ptrs_ty_depth(ty_param); + if ptr_depth >= 1 && base_ty.is_enum() { + let param = &func_args[0]; + + // cancel out '&'s first + let mut derefs_needed = ptr_depth; + let mut cur_expr = param; + while derefs_needed > 0 { + if let ExprKind::AddrOf(_, ref inner_expr) = cur_expr.node { + derefs_needed -= 1; + cur_expr = inner_expr; + } else { + break; + } + } + + let derefs: String = iter::repeat('*').take(derefs_needed).collect(); + db.span_suggestion( + param.span, + "try dereferencing", + format!("{}{}", derefs, snippet(cx, cur_expr.span, "")), + ); + } + }, + ) + } + } + } +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 12ef2f51d8c..474f16679a7 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -55,6 +55,7 @@ pub const LATE_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "LateContext"]; pub const LINKED_LIST: [&str; 4] = ["alloc", "collections", "linked_list", "LinkedList"]; pub const LINT: [&str; 3] = ["rustc", "lint", "Lint"]; pub const LINT_ARRAY: [&str; 3] = ["rustc", "lint", "LintArray"]; +pub const MEM_DISCRIMINANT: [&str; 3] = ["core", "mem", "discriminant"]; pub const MEM_FORGET: [&str; 3] = ["core", "mem", "forget"]; pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"]; pub const MEM_UNINIT: [&str; 3] = ["core", "mem", "uninitialized"]; diff --git a/tests/ui/mem_discriminant.rs b/tests/ui/mem_discriminant.rs new file mode 100644 index 00000000000..a7176fb7985 --- /dev/null +++ b/tests/ui/mem_discriminant.rs @@ -0,0 +1,44 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + + +#![deny(clippy::mem_discriminant_non_enum)] + +use std::mem; + +enum Foo { + One(usize), + Two(u8), +} + +struct A(Foo); + +fn main() { + // bad + mem::discriminant(&"hello"); + mem::discriminant(&&Some(2)); + mem::discriminant(&&None::); + mem::discriminant(&&Foo::One(5)); + mem::discriminant(&&Foo::Two(5)); + mem::discriminant(&A(Foo::One(0))); + + let ro = &Some(3); + let rro = &ro; + mem::discriminant(&ro); + mem::discriminant(rro); + mem::discriminant(&rro); + + + // ok + mem::discriminant(&Some(2)); + mem::discriminant(&None::); + mem::discriminant(&Foo::One(5)); + mem::discriminant(&Foo::Two(5)); + mem::discriminant(ro); +} diff --git a/tests/ui/mem_discriminant.stderr b/tests/ui/mem_discriminant.stderr new file mode 100644 index 00000000000..5255458d8f8 --- /dev/null +++ b/tests/ui/mem_discriminant.stderr @@ -0,0 +1,76 @@ +error: calling `mem::discriminant` on non-enum type `&str` + --> $DIR/mem_discriminant.rs:24:5 + | +24 | mem::discriminant(&"hello"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: lint level defined here + --> $DIR/mem_discriminant.rs:11:9 + | +11 | #![deny(clippy::mem_discriminant_non_enum)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: calling `mem::discriminant` on non-enum type `&std::option::Option` + --> $DIR/mem_discriminant.rs:25:5 + | +25 | mem::discriminant(&&Some(2)); + | ^^^^^^^^^^^^^^^^^^---------^ + | | + | help: try dereferencing: `&Some(2)` + +error: calling `mem::discriminant` on non-enum type `&std::option::Option` + --> $DIR/mem_discriminant.rs:26:5 + | +26 | mem::discriminant(&&None::); + | ^^^^^^^^^^^^^^^^^^------------^ + | | + | help: try dereferencing: `&None::` + +error: calling `mem::discriminant` on non-enum type `&Foo` + --> $DIR/mem_discriminant.rs:27:5 + | +27 | mem::discriminant(&&Foo::One(5)); + | ^^^^^^^^^^^^^^^^^^-------------^ + | | + | help: try dereferencing: `&Foo::One(5)` + +error: calling `mem::discriminant` on non-enum type `&Foo` + --> $DIR/mem_discriminant.rs:28:5 + | +28 | mem::discriminant(&&Foo::Two(5)); + | ^^^^^^^^^^^^^^^^^^-------------^ + | | + | help: try dereferencing: `&Foo::Two(5)` + +error: calling `mem::discriminant` on non-enum type `A` + --> $DIR/mem_discriminant.rs:29:5 + | +29 | mem::discriminant(&A(Foo::One(0))); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: calling `mem::discriminant` on non-enum type `&std::option::Option` + --> $DIR/mem_discriminant.rs:33:5 + | +33 | mem::discriminant(&ro); + | ^^^^^^^^^^^^^^^^^^---^ + | | + | help: try dereferencing: `ro` + +error: calling `mem::discriminant` on non-enum type `&std::option::Option` + --> $DIR/mem_discriminant.rs:34:5 + | +34 | mem::discriminant(rro); + | ^^^^^^^^^^^^^^^^^^---^ + | | + | help: try dereferencing: `*rro` + +error: calling `mem::discriminant` on non-enum type `&&std::option::Option` + --> $DIR/mem_discriminant.rs:35:5 + | +35 | mem::discriminant(&rro); + | ^^^^^^^^^^^^^^^^^^----^ + | | + | help: try dereferencing: `*rro` + +error: aborting due to 9 previous errors + From 5dbca1f6b11fd7ece50675f0c8f1ff7c254a30bd Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Sun, 21 Oct 2018 15:27:01 -0400 Subject: [PATCH 25/38] Add `Applicability` --- clippy_lints/src/mem_discriminant.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/mem_discriminant.rs b/clippy_lints/src/mem_discriminant.rs index 356162fd6f4..c53c276991d 100644 --- a/clippy_lints/src/mem_discriminant.rs +++ b/clippy_lints/src/mem_discriminant.rs @@ -11,6 +11,7 @@ use crate::rustc::hir::{Expr, ExprKind}; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; +use crate::rustc_errors::Applicability; use crate::utils::{match_def_path, opt_def_id, paths, snippet, span_lint_and_then, walk_ptrs_ty_depth}; use if_chain::if_chain; @@ -79,10 +80,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemDiscriminant { } let derefs: String = iter::repeat('*').take(derefs_needed).collect(); - db.span_suggestion( + db.span_suggestion_with_applicability( param.span, "try dereferencing", format!("{}{}", derefs, snippet(cx, cur_expr.span, "")), + Applicability::MachineApplicable, ); } }, From 1a6bfecf383ca1e48fe9a5838261a60ade5af403 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Sun, 21 Oct 2018 15:23:51 -0400 Subject: [PATCH 26/38] Add test case for `mem::discriminant` inside a macro --- tests/ui/mem_discriminant.rs | 5 +++++ tests/ui/mem_discriminant.stderr | 14 +++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/ui/mem_discriminant.rs b/tests/ui/mem_discriminant.rs index a7176fb7985..64d056fb2fe 100644 --- a/tests/ui/mem_discriminant.rs +++ b/tests/ui/mem_discriminant.rs @@ -34,6 +34,11 @@ fn main() { mem::discriminant(rro); mem::discriminant(&rro); + macro_rules! mem_discriminant_but_in_a_macro { + ($param:expr) => (mem::discriminant($param)) + } + + mem_discriminant_but_in_a_macro!(&rro); // ok mem::discriminant(&Some(2)); diff --git a/tests/ui/mem_discriminant.stderr b/tests/ui/mem_discriminant.stderr index 5255458d8f8..57e03013392 100644 --- a/tests/ui/mem_discriminant.stderr +++ b/tests/ui/mem_discriminant.stderr @@ -72,5 +72,17 @@ error: calling `mem::discriminant` on non-enum type `&&std::option::Option` | | | help: try dereferencing: `*rro` -error: aborting due to 9 previous errors +error: calling `mem::discriminant` on non-enum type `&&std::option::Option` + --> $DIR/mem_discriminant.rs:38:27 + | +38 | ($param:expr) => (mem::discriminant($param)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +... +41 | mem_discriminant_but_in_a_macro!(&rro); + | --------------------------------------- + | | | + | | help: try dereferencing: `*rro` + | in this macro invocation + +error: aborting due to 10 previous errors From d53e6f87e94c83651ad1e22f294f6f59d8d1b5d1 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Wed, 24 Oct 2018 22:27:47 -0400 Subject: [PATCH 27/38] Add tests for more than one level of reference --- tests/ui/mem_discriminant.rs | 6 ++++++ tests/ui/mem_discriminant.stderr | 18 +++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/ui/mem_discriminant.rs b/tests/ui/mem_discriminant.rs index 64d056fb2fe..5ddd90ac8b5 100644 --- a/tests/ui/mem_discriminant.rs +++ b/tests/ui/mem_discriminant.rs @@ -40,10 +40,16 @@ fn main() { mem_discriminant_but_in_a_macro!(&rro); + let rrrrro = &&&rro; + mem::discriminant(&rrrrro); + mem::discriminant(*rrrrro); + // ok mem::discriminant(&Some(2)); mem::discriminant(&None::); mem::discriminant(&Foo::One(5)); mem::discriminant(&Foo::Two(5)); mem::discriminant(ro); + mem::discriminant(*rro); + mem::discriminant(****rrrrro); } diff --git a/tests/ui/mem_discriminant.stderr b/tests/ui/mem_discriminant.stderr index 57e03013392..6414e4c96d6 100644 --- a/tests/ui/mem_discriminant.stderr +++ b/tests/ui/mem_discriminant.stderr @@ -84,5 +84,21 @@ error: calling `mem::discriminant` on non-enum type `&&std::option::Option` | | help: try dereferencing: `*rro` | in this macro invocation -error: aborting due to 10 previous errors +error: calling `mem::discriminant` on non-enum type `&&&&&std::option::Option` + --> $DIR/mem_discriminant.rs:44:5 + | +44 | mem::discriminant(&rrrrro); + | ^^^^^^^^^^^^^^^^^^-------^ + | | + | help: try dereferencing: `****rrrrro` + +error: calling `mem::discriminant` on non-enum type `&&&std::option::Option` + --> $DIR/mem_discriminant.rs:45:5 + | +45 | mem::discriminant(*rrrrro); + | ^^^^^^^^^^^^^^^^^^-------^ + | | + | help: try dereferencing: `****rrrrro` + +error: aborting due to 12 previous errors From b8a909901123fb326edf46e99d3e2d83ed22b15c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Thu, 25 Oct 2018 13:37:50 +0200 Subject: [PATCH 28/38] Revert "new_ret_no_self: add sample from #3313 to Known Problems section." This reverts commit fd2f6dd3824b32af031d19830b6ccdc732dd3dfc. Issue #3313 has been fixed. --- clippy_lints/src/methods/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index f0810c906ef..01f97264d0b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -452,8 +452,7 @@ declare_clippy_lint! { /// **Why is this bad?** As a convention, `new` methods are used to make a new /// instance of a type. /// -/// **Known problems:** The lint fires when the return type is wrapping `Self`. -/// Example: `fn new() -> Result {}` +/// **Known problems:** None. /// /// **Example:** /// ```rust From 3ca08959200b69f1736c1af3e5bc944ab43e739b Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Tue, 23 Oct 2018 16:01:45 +0900 Subject: [PATCH 29/38] Add redundant_clone lint --- clippy_lints/src/lib.rs | 3 + clippy_lints/src/redundant_clone.rs | 281 ++++++++++++++++++++++++++++ clippy_lints/src/utils/paths.rs | 8 + tests/ui/redundant_clone.rs | 44 +++++ tests/ui/redundant_clone.stderr | 111 +++++++++++ 5 files changed, 447 insertions(+) create mode 100644 clippy_lints/src/redundant_clone.rs create mode 100644 tests/ui/redundant_clone.rs create mode 100644 tests/ui/redundant_clone.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7a91890633a..eaff87e78f8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -179,6 +179,7 @@ pub mod ptr; pub mod ptr_offset_with_cast; pub mod question_mark; pub mod ranges; +pub mod redundant_clone; pub mod redundant_field_names; pub mod redundant_pattern_matching; pub mod reference; @@ -452,6 +453,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box indexing_slicing::IndexingSlicing); reg.register_late_lint_pass(box non_copy_const::NonCopyConst); reg.register_late_lint_pass(box ptr_offset_with_cast::Pass); + reg.register_late_lint_pass(box redundant_clone::RedundantClone); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -981,6 +983,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { fallible_impl_from::FALLIBLE_IMPL_FROM, mutex_atomic::MUTEX_INTEGER, needless_borrow::NEEDLESS_BORROW, + redundant_clone::REDUNDANT_CLONE, unwrap::PANICKING_UNWRAP, unwrap::UNNECESSARY_UNWRAP, ]); diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs new file mode 100644 index 00000000000..fc760a3ee29 --- /dev/null +++ b/clippy_lints/src/redundant_clone.rs @@ -0,0 +1,281 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use crate::rustc::hir::intravisit::FnKind; +use crate::rustc::hir::{def_id, Body, FnDecl}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::mir::{ + self, traversal, + visit::{PlaceContext, Visitor}, + TerminatorKind, +}; +use crate::rustc::ty; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::rustc_errors::Applicability; +use crate::syntax::{ + ast::NodeId, + source_map::{BytePos, Span}, +}; +use crate::utils::{ + in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint, span_lint_and_then, + walk_ptrs_ty_depth, +}; +use if_chain::if_chain; +use std::convert::TryFrom; + +/// **What it does:** Checks for a redudant `clone()` (and its relatives) which clones an owned +/// value that is going to be dropped without further use. +/// +/// **Why is this bad?** It is not always possible for the compiler to eliminate useless +/// allocations and deallocations generated by redundant `clone()`s. +/// +/// **Known problems:** +/// +/// * Suggestions made by this lint could require NLL to be enabled. +/// * False-positive if there is a borrow preventing the value from moving out. +/// +/// ```rust +/// let x = String::new(); +/// +/// let y = &x; +/// +/// foo(x.clone()); // This lint suggests to remove this `clone()` +/// ``` +/// +/// **Example:** +/// ```rust +/// { +/// let x = Foo::new(); +/// call(x.clone()); +/// call(x.clone()); // this can just pass `x` +/// } +/// +/// ["lorem", "ipsum"].join(" ").to_string() +/// +/// Path::new("/a/b").join("c").to_path_buf() +/// ``` +declare_clippy_lint! { + pub REDUNDANT_CLONE, + nursery, + "`clone()` of an owned value that is going to be dropped immediately" +} + +pub struct RedundantClone; + +impl LintPass for RedundantClone { + fn get_lints(&self) -> LintArray { + lint_array!(REDUNDANT_CLONE) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { + fn check_fn( + &mut self, + cx: &LateContext<'a, 'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl, + body: &'tcx Body, + _: Span, + _: NodeId, + ) { + let def_id = cx.tcx.hir.body_owner_def_id(body.id()); + let mir = cx.tcx.optimized_mir(def_id); + + // Looks for `call(&T)` where `T: !Copy` + let call = |kind: &mir::TerminatorKind<'tcx>| -> Option<(def_id::DefId, mir::Local, ty::Ty<'tcx>)> { + if_chain! { + if let TerminatorKind::Call { func, args, .. } = kind; + if args.len() == 1; + if let mir::Operand::Move(mir::Place::Local(local)) = &args[0]; + if let ty::FnDef(def_id, _) = func.ty(&*mir, cx.tcx).sty; + if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx)); + if !is_copy(cx, inner_ty); + then { + Some((def_id, *local, inner_ty)) + } else { + None + } + } + }; + + for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { + let terminator = if let Some(terminator) = &bbdata.terminator { + terminator + } else { + continue; + }; + + // Give up on loops + if terminator.successors().any(|s| *s == bb) { + continue; + } + + let (fn_def_id, arg, arg_ty) = if let Some(t) = call(&terminator.kind) { + t + } else { + continue; + }; + + let from_borrow = match_def_path(cx.tcx, fn_def_id, &paths::CLONE_TRAIT_METHOD) + || match_def_path(cx.tcx, fn_def_id, &paths::TO_OWNED_METHOD) + || (match_def_path(cx.tcx, fn_def_id, &paths::TO_STRING_METHOD) + && match_type(cx, arg_ty, &paths::STRING)); + + let from_deref = !from_borrow + && (match_def_path(cx.tcx, fn_def_id, &paths::PATH_TO_PATH_BUF) + || match_def_path(cx.tcx, fn_def_id, &paths::OS_STR_TO_OS_STRING)); + + if !from_borrow && !from_deref { + continue; + } + + // _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } + let cloned = if let Some(referent) = bbdata + .statements + .iter() + .rev() + .filter_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { + if *local == arg { + if from_deref { + // `r` is already a reference. + if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v { + return Some(r); + } + } else if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v { + return Some(r); + } + } + } + + None + }) + .next() + { + referent + } else { + continue; + }; + + // _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }` + let referent = if from_deref { + let ps = mir.predecessors_for(bb); + let pred_arg = if_chain! { + if ps.len() == 1; + if let Some(pred_terminator) = &mir[ps[0]].terminator; + if let mir::TerminatorKind::Call { destination: Some((res, _)), .. } = &pred_terminator.kind; + if *res == mir::Place::Local(cloned); + if let Some((pred_fn_def_id, pred_arg, pred_arg_ty)) = call(&pred_terminator.kind); + if match_def_path(cx.tcx, pred_fn_def_id, &paths::DEREF_TRAIT_METHOD); + if match_type(cx, pred_arg_ty, &paths::PATH_BUF) + || match_type(cx, pred_arg_ty, &paths::OS_STRING); + then { + pred_arg + } else { + continue; + } + }; + + if let Some(referent) = mir[ps[0]] + .statements + .iter() + .rev() + .filter_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(l), v) = &stmt.kind { + if *l == pred_arg { + if let mir::Rvalue::Ref(_, _, mir::Place::Local(referent)) = **v { + return Some(referent); + } + } + } + + None + }) + .next() + { + referent + } else { + continue; + } + } else { + cloned + }; + + let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| { + if let Some(term) = &tdata.terminator { + // Give up on loops + if term.successors().any(|s| *s == bb) { + return true; + } + } + + let mut vis = LocalUseVisitor { + local: referent, + used_other_than_drop: false, + }; + vis.visit_basic_block_data(tbb, tdata); + vis.used_other_than_drop + }); + + if !used_later { + let span = terminator.source_info.span; + if_chain! { + if !in_macro(span); + if let Some(snip) = snippet_opt(cx, span); + if let Some(dot) = snip.rfind('.'); + then { + let sugg_span = span.with_lo( + span.lo() + BytePos(u32::try_from(dot).unwrap()) + ); + + span_lint_and_then(cx, REDUNDANT_CLONE, sugg_span, "redundant clone", |db| { + db.span_suggestion_with_applicability( + sugg_span, + "remove this", + String::new(), + Applicability::MaybeIncorrect, + ); + db.span_note( + span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())), + "this value is dropped without further use", + ); + }); + } else { + span_lint(cx, REDUNDANT_CLONE, span, "redundant clone"); + } + } + } + } + } +} + +struct LocalUseVisitor { + local: mir::Local, + used_other_than_drop: bool, +} + +impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { + fn visit_statement(&mut self, block: mir::BasicBlock, statement: &mir::Statement<'tcx>, location: mir::Location) { + // Once flagged, skip remaining statements + if !self.used_other_than_drop { + self.super_statement(block, statement, location); + } + } + + fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) { + match ctx { + PlaceContext::Drop | PlaceContext::StorageDead => return, + _ => {}, + } + + if *local == self.local { + self.used_other_than_drop = true; + } + } +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 474f16679a7..8941d303156 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -23,6 +23,7 @@ pub const BTREEMAP: [&str; 5] = ["alloc", "collections", "btree", "map", "BTreeM pub const BTREEMAP_ENTRY: [&str; 5] = ["alloc", "collections", "btree", "map", "Entry"]; pub const BTREESET: [&str; 5] = ["alloc", "collections", "btree", "set", "BTreeSet"]; pub const CLONE_TRAIT: [&str; 3] = ["core", "clone", "Clone"]; +pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"]; pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"]; pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"]; pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"]; @@ -31,6 +32,7 @@ pub const C_VOID: [&str; 3] = ["core", "ffi", "c_void"]; pub const C_VOID_LIBC: [&str; 2] = ["libc", "c_void"]; pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"]; pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"]; +pub const DEREF_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "Deref", "deref"]; pub const DISPLAY_FMT_METHOD: [&str; 4] = ["core", "fmt", "Display", "fmt"]; pub const DOUBLE_ENDED_ITERATOR: [&str; 4] = ["core", "iter", "traits", "DoubleEndedIterator"]; pub const DROP: [&str; 3] = ["core", "mem", "drop"]; @@ -67,7 +69,11 @@ pub const OPTION: [&str; 3] = ["core", "option", "Option"]; pub const OPTION_NONE: [&str; 4] = ["core", "option", "Option", "None"]; pub const OPTION_SOME: [&str; 4] = ["core", "option", "Option", "Some"]; pub const ORD: [&str; 3] = ["core", "cmp", "Ord"]; +pub const OS_STRING: [&str; 4] = ["std", "ffi", "os_str", "OsString"]; +pub const OS_STR_TO_OS_STRING: [&str; 5] = ["std", "ffi", "os_str", "OsStr", "to_os_string"]; pub const PARTIAL_ORD: [&str; 3] = ["core", "cmp", "PartialOrd"]; +pub const PATH_BUF: [&str; 3] = ["std", "path", "PathBuf"]; +pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"]; pub const PTR_NULL: [&str; 2] = ["ptr", "null"]; pub const PTR_NULL_MUT: [&str; 2] = ["ptr", "null_mut"]; pub const RANGE: [&str; 3] = ["core", "ops", "Range"]; @@ -100,7 +106,9 @@ pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "", "into_vec pub const SLICE_ITER: [&str; 3] = ["core", "slice", "Iter"]; pub const STRING: [&str; 3] = ["alloc", "string", "String"]; pub const TO_OWNED: [&str; 3] = ["alloc", "borrow", "ToOwned"]; +pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"]; pub const TO_STRING: [&str; 3] = ["alloc", "string", "ToString"]; +pub const TO_STRING_METHOD: [&str; 4] = ["alloc", "string", "ToString", "to_string"]; pub const TRANSMUTE: [&str; 4] = ["core", "intrinsics", "", "transmute"]; pub const TRY_INTO_RESULT: [&str; 4] = ["std", "ops", "Try", "into_result"]; pub const UNINIT: [&str; 4] = ["core", "intrinsics", "", "uninit"]; diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs new file mode 100644 index 00000000000..5fd7ffae71b --- /dev/null +++ b/tests/ui/redundant_clone.rs @@ -0,0 +1,44 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![warn(clippy::redundant_clone)] + +use std::path::Path; +use std::ffi::OsString; + +fn main() { + let _ = ["lorem", "ipsum"].join(" ").to_string(); + + let s = String::from("foo"); + let _ = s.clone(); + + let s = String::from("foo"); + let _ = s.to_string(); + + let s = String::from("foo"); + let _ = s.to_owned(); + + let _ = Path::new("/a/b/").join("c").to_owned(); + + let _ = Path::new("/a/b/").join("c").to_path_buf(); + + let _ = OsString::new().to_owned(); + + let _ = OsString::new().to_os_string(); +} + +#[derive(Clone)] +struct Alpha; +fn double(a: Alpha) -> (Alpha, Alpha) { + if true { + (a.clone(), a.clone()) + } else { + (Alpha, a) + } +} diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr new file mode 100644 index 00000000000..ad84a5754b5 --- /dev/null +++ b/tests/ui/redundant_clone.stderr @@ -0,0 +1,111 @@ +error: redundant clone + --> $DIR/redundant_clone.rs:16:41 + | +16 | let _ = ["lorem", "ipsum"].join(" ").to_string(); + | ^^^^^^^^^^^^ help: remove this + | + = note: `-D clippy::redundant-clone` implied by `-D warnings` +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:16:13 + | +16 | let _ = ["lorem", "ipsum"].join(" ").to_string(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:19:14 + | +19 | let _ = s.clone(); + | ^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:19:13 + | +19 | let _ = s.clone(); + | ^ + +error: redundant clone + --> $DIR/redundant_clone.rs:22:14 + | +22 | let _ = s.to_string(); + | ^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:22:13 + | +22 | let _ = s.to_string(); + | ^ + +error: redundant clone + --> $DIR/redundant_clone.rs:25:14 + | +25 | let _ = s.to_owned(); + | ^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:25:13 + | +25 | let _ = s.to_owned(); + | ^ + +error: redundant clone + --> $DIR/redundant_clone.rs:27:41 + | +27 | let _ = Path::new("/a/b/").join("c").to_owned(); + | ^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:27:13 + | +27 | let _ = Path::new("/a/b/").join("c").to_owned(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:29:41 + | +29 | let _ = Path::new("/a/b/").join("c").to_path_buf(); + | ^^^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:29:13 + | +29 | let _ = Path::new("/a/b/").join("c").to_path_buf(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:31:28 + | +31 | let _ = OsString::new().to_owned(); + | ^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:31:13 + | +31 | let _ = OsString::new().to_owned(); + | ^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:33:28 + | +33 | let _ = OsString::new().to_os_string(); + | ^^^^^^^^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:33:13 + | +33 | let _ = OsString::new().to_os_string(); + | ^^^^^^^^^^^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:40:22 + | +40 | (a.clone(), a.clone()) + | ^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:40:21 + | +40 | (a.clone(), a.clone()) + | ^ + +error: aborting due to 9 previous errors + From 5285372f686ed5ff7a84cd0cefb287af6b9c62b7 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Wed, 24 Oct 2018 22:57:31 +0900 Subject: [PATCH 30/38] Run update_lints --- CHANGELOG.md | 1 + README.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ad6514ee10..5d9d470925f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -813,6 +813,7 @@ All notable changes to this project will be documented in this file. [`range_plus_one`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#range_plus_one [`range_step_by_zero`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#range_step_by_zero [`range_zip_with_len`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#range_zip_with_len +[`redundant_clone`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_clone [`redundant_closure`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_closure [`redundant_closure_call`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_closure_call [`redundant_field_names`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#redundant_field_names diff --git a/README.md b/README.md index a13d8ecef66..b4091cdab6c 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 282 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) +[There are 283 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: From 105ae712f4f136adb7c94f1cfa35da30bd0e4952 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 14:59:14 +0900 Subject: [PATCH 31/38] update_references indexing_slicing --- tests/ui/indexing_slicing.stderr | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/ui/indexing_slicing.stderr b/tests/ui/indexing_slicing.stderr index fafcb1bc485..14e9627e573 100644 --- a/tests/ui/indexing_slicing.stderr +++ b/tests/ui/indexing_slicing.stderr @@ -1,3 +1,29 @@ +error: index out of bounds: the len is 4 but the index is 4 + --> $DIR/indexing_slicing.rs:28:5 + | +28 | x[4]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^ + | + = note: #[deny(const_err)] on by default + +error: index out of bounds: the len is 4 but the index is 8 + --> $DIR/indexing_slicing.rs:29:5 + | +29 | x[1 << 3]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^^^^^^ + +error: index out of bounds: the len is 0 but the index is 0 + --> $DIR/indexing_slicing.rs:59:5 + | +59 | empty[0]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^^^^^ + +error: index out of bounds: the len is 4 but the index is 15 + --> $DIR/indexing_slicing.rs:90:5 + | +90 | x[N]; // Ok, let rustc's `const_err` lint handle `usize` indexing on arrays. + | ^^^^ + error: indexing may panic. --> $DIR/indexing_slicing.rs:23:5 | @@ -279,5 +305,5 @@ error: range is out of bounds 98 | &x[10..num]; // should trigger out of bounds error | ^^ -error: aborting due to 39 previous errors +error: aborting due to 43 previous errors From 24d3f5b48f177379ba7b8727e5ba9b52b52da2f5 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 20:33:40 +0900 Subject: [PATCH 32/38] Implement visit_basic_block_data --- clippy_lints/src/redundant_clone.rs | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index fc760a3ee29..fa377dcca67 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -261,10 +261,31 @@ struct LocalUseVisitor { } impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { - fn visit_statement(&mut self, block: mir::BasicBlock, statement: &mir::Statement<'tcx>, location: mir::Location) { - // Once flagged, skip remaining statements - if !self.used_other_than_drop { - self.super_statement(block, statement, location); + fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) { + let mir::BasicBlockData { + statements, + terminator, + is_cleanup: _, + } = data; + + for (statement_index, statement) in statements.iter().enumerate() { + self.visit_statement(block, statement, mir::Location { block, statement_index }); + + // Once flagged, skip remaining statements + if self.used_other_than_drop { + return; + } + } + + if let Some(terminator) = terminator { + self.visit_terminator( + block, + terminator, + mir::Location { + block, + statement_index: statements.len(), + }, + ); } } From 9a150b4aa123a6d67fbf8819fe67f2ef1015b726 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 21:08:32 +0900 Subject: [PATCH 33/38] Use lint_root --- clippy_lints/src/redundant_clone.rs | 14 ++++++++++---- clippy_lints/src/utils/mod.rs | 17 +++++++++++++++++ tests/ui/redundant_clone.rs | 3 +++ tests/ui/redundant_clone.stderr | 8 ++++---- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index fa377dcca67..1a8a6273358 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -23,7 +23,7 @@ use crate::syntax::{ source_map::{BytePos, Span}, }; use crate::utils::{ - in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint, span_lint_and_then, + in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node, span_lint_node_and_then, walk_ptrs_ty_depth, }; use if_chain::if_chain; @@ -87,7 +87,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let def_id = cx.tcx.hir.body_owner_def_id(body.id()); let mir = cx.tcx.optimized_mir(def_id); - // Looks for `call(&T)` where `T: !Copy` + // Looks for `call(x: &T)` where `T: !Copy` let call = |kind: &mir::TerminatorKind<'tcx>| -> Option<(def_id::DefId, mir::Local, ty::Ty<'tcx>)> { if_chain! { if let TerminatorKind::Call { func, args, .. } = kind; @@ -225,6 +225,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { if !used_later { let span = terminator.source_info.span; + let node = if let mir::ClearCrossCrate::Set(scope_local_data) = &mir.source_scope_local_data { + scope_local_data[terminator.source_info.scope].lint_root + } else { + unreachable!() + }; + if_chain! { if !in_macro(span); if let Some(snip) = snippet_opt(cx, span); @@ -234,7 +240,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { span.lo() + BytePos(u32::try_from(dot).unwrap()) ); - span_lint_and_then(cx, REDUNDANT_CLONE, sugg_span, "redundant clone", |db| { + span_lint_node_and_then(cx, REDUNDANT_CLONE, node, sugg_span, "redundant clone", |db| { db.span_suggestion_with_applicability( sugg_span, "remove this", @@ -247,7 +253,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { ); }); } else { - span_lint(cx, REDUNDANT_CLONE, span, "redundant clone"); + span_lint_node(cx, REDUNDANT_CLONE, node, span, "redundant clone"); } } } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 05356f8d385..1a8db837f32 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -562,6 +562,23 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>( db.docs_link(lint); } +pub fn span_lint_node(cx: &LateContext<'_, '_>, lint: &'static Lint, node: NodeId, sp: Span, msg: &str) { + DiagnosticWrapper(cx.tcx.struct_span_lint_node(lint, node, sp, msg)).docs_link(lint); +} + +pub fn span_lint_node_and_then( + cx: &LateContext<'_, '_>, + lint: &'static Lint, + node: NodeId, + sp: Span, + msg: &str, + f: impl FnOnce(&mut DiagnosticBuilder<'_>), +) { + let mut db = DiagnosticWrapper(cx.tcx.struct_span_lint_node(lint, node, sp, msg)); + f(&mut db.0); + db.docs_link(lint); +} + /// Add a span lint with a suggestion on how to fix it. /// /// These suggestions can be parsed by rustfix to allow it to automatically fix your code. diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index 5fd7ffae71b..deedde38231 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -31,6 +31,9 @@ fn main() { let _ = OsString::new().to_owned(); let _ = OsString::new().to_os_string(); + + // Check that lint level works + #[allow(clippy::redundant_clone)] let _ = String::new().to_string(); } #[derive(Clone)] diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index ad84a5754b5..db452822f89 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -96,15 +96,15 @@ note: this value is dropped without further use | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:40:22 + --> $DIR/redundant_clone.rs:43:22 | -40 | (a.clone(), a.clone()) +43 | (a.clone(), a.clone()) | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:40:21 + --> $DIR/redundant_clone.rs:43:21 | -40 | (a.clone(), a.clone()) +43 | (a.clone(), a.clone()) | ^ error: aborting due to 9 previous errors From 6d6ff885852e669a59013629193b74c2458005af Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Thu, 25 Oct 2018 22:02:46 +0900 Subject: [PATCH 34/38] Refactor --- clippy_lints/src/redundant_clone.rs | 146 +++++++++++++--------------- 1 file changed, 67 insertions(+), 79 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 1a8a6273358..85f8b525677 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -29,6 +29,15 @@ use crate::utils::{ use if_chain::if_chain; use std::convert::TryFrom; +macro_rules! unwrap_or_continue { + ($x:expr) => { + match $x { + Some(x) => x, + None => continue, + } + }; +} + /// **What it does:** Checks for a redudant `clone()` (and its relatives) which clones an owned /// value that is going to be dropped without further use. /// @@ -87,40 +96,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let def_id = cx.tcx.hir.body_owner_def_id(body.id()); let mir = cx.tcx.optimized_mir(def_id); - // Looks for `call(x: &T)` where `T: !Copy` - let call = |kind: &mir::TerminatorKind<'tcx>| -> Option<(def_id::DefId, mir::Local, ty::Ty<'tcx>)> { - if_chain! { - if let TerminatorKind::Call { func, args, .. } = kind; - if args.len() == 1; - if let mir::Operand::Move(mir::Place::Local(local)) = &args[0]; - if let ty::FnDef(def_id, _) = func.ty(&*mir, cx.tcx).sty; - if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx)); - if !is_copy(cx, inner_ty); - then { - Some((def_id, *local, inner_ty)) - } else { - None - } - } - }; - for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { - let terminator = if let Some(terminator) = &bbdata.terminator { - terminator - } else { - continue; - }; + let terminator = unwrap_or_continue!(&bbdata.terminator); // Give up on loops if terminator.successors().any(|s| *s == bb) { continue; } - let (fn_def_id, arg, arg_ty) = if let Some(t) = call(&terminator.kind) { - t - } else { - continue; - }; + let (fn_def_id, arg, arg_ty, _) = unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind)); let from_borrow = match_def_path(cx.tcx, fn_def_id, &paths::CLONE_TRAIT_METHOD) || match_def_path(cx.tcx, fn_def_id, &paths::TO_OWNED_METHOD) @@ -135,43 +119,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { continue; } - // _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } - let cloned = if let Some(referent) = bbdata - .statements - .iter() - .rev() - .filter_map(|stmt| { - if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { - if *local == arg { - if from_deref { - // `r` is already a reference. - if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v { - return Some(r); - } - } else if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v { - return Some(r); - } - } - } - - None - }) - .next() - { - referent - } else { - continue; - }; + // _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } (from_deref) + // In case of `from_deref`, `arg` is already a reference since it is `deref`ed in the previous + // block. + let cloned = unwrap_or_continue!(find_stmt_assigns_to(arg, from_borrow, bbdata.statements.iter().rev())); // _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }` let referent = if from_deref { let ps = mir.predecessors_for(bb); + if ps.len() != 1 { + continue; + } + let pred_terminator = unwrap_or_continue!(&mir[ps[0]].terminator); + let pred_arg = if_chain! { - if ps.len() == 1; - if let Some(pred_terminator) = &mir[ps[0]].terminator; - if let mir::TerminatorKind::Call { destination: Some((res, _)), .. } = &pred_terminator.kind; + if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) = + is_call_with_ref_arg(cx, mir, &pred_terminator.kind); if *res == mir::Place::Local(cloned); - if let Some((pred_fn_def_id, pred_arg, pred_arg_ty)) = call(&pred_terminator.kind); if match_def_path(cx.tcx, pred_fn_def_id, &paths::DEREF_TRAIT_METHOD); if match_type(cx, pred_arg_ty, &paths::PATH_BUF) || match_type(cx, pred_arg_ty, &paths::OS_STRING); @@ -182,27 +146,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { } }; - if let Some(referent) = mir[ps[0]] - .statements - .iter() - .rev() - .filter_map(|stmt| { - if let mir::StatementKind::Assign(mir::Place::Local(l), v) = &stmt.kind { - if *l == pred_arg { - if let mir::Rvalue::Ref(_, _, mir::Place::Local(referent)) = **v { - return Some(referent); - } - } - } - - None - }) - .next() - { - referent - } else { - continue; - } + unwrap_or_continue!(find_stmt_assigns_to(pred_arg, true, mir[ps[0]].statements.iter().rev())) } else { cloned }; @@ -261,6 +205,50 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { } } +/// If `kind` is `y = func(x: &T)` where `T: !Copy`, returns `(DefId of func, x, T, y)`. +fn is_call_with_ref_arg<'tcx>( + cx: &LateContext<'_, 'tcx>, + mir: &'tcx mir::Mir<'tcx>, + kind: &'tcx mir::TerminatorKind<'tcx>, +) -> Option<(def_id::DefId, mir::Local, ty::Ty<'tcx>, Option<&'tcx mir::Place<'tcx>>)> { + if_chain! { + if let TerminatorKind::Call { func, args, destination, .. } = kind; + if args.len() == 1; + if let mir::Operand::Move(mir::Place::Local(local)) = &args[0]; + if let ty::FnDef(def_id, _) = func.ty(&*mir, cx.tcx).sty; + if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx)); + if !is_copy(cx, inner_ty); + then { + Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest))) + } else { + None + } + } +} + +/// Finds the first `to = (&)from`, and returns `Some(from)`. +fn find_stmt_assigns_to<'a, 'tcx: 'a>( + to: mir::Local, + by_ref: bool, + mut stmts: impl Iterator>, +) -> Option { + stmts.find_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { + if *local == to { + if by_ref { + if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v { + return Some(r); + } + } else if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v { + return Some(r); + } + } + } + + None + }) +} + struct LocalUseVisitor { local: mir::Local, used_other_than_drop: bool, From a8286927800f2e9f050f0d8c3d6797d5f0885656 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 26 Oct 2018 01:27:28 +0900 Subject: [PATCH 35/38] Use BasicBlockData::terminator --- clippy_lints/src/redundant_clone.rs | 35 +++++++++++------------------ 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 85f8b525677..85f7bbb637c 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -97,7 +97,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let mir = cx.tcx.optimized_mir(def_id); for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { - let terminator = unwrap_or_continue!(&bbdata.terminator); + let terminator = bbdata.terminator(); // Give up on loops if terminator.successors().any(|s| *s == bb) { @@ -130,7 +130,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { if ps.len() != 1 { continue; } - let pred_terminator = unwrap_or_continue!(&mir[ps[0]].terminator); + let pred_terminator = mir[ps[0]].terminator(); let pred_arg = if_chain! { if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) = @@ -152,11 +152,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { }; let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| { - if let Some(term) = &tdata.terminator { - // Give up on loops - if term.successors().any(|s| *s == bb) { - return true; - } + // Give up on loops + if tdata.terminator().successors().any(|s| *s == bb) { + return true; } let mut vis = LocalUseVisitor { @@ -256,12 +254,7 @@ struct LocalUseVisitor { impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) { - let mir::BasicBlockData { - statements, - terminator, - is_cleanup: _, - } = data; - + let statements = &data.statements; for (statement_index, statement) in statements.iter().enumerate() { self.visit_statement(block, statement, mir::Location { block, statement_index }); @@ -271,16 +264,14 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { } } - if let Some(terminator) = terminator { - self.visit_terminator( + self.visit_terminator( + block, + data.terminator(), + mir::Location { block, - terminator, - mir::Location { - block, - statement_index: statements.len(), - }, - ); - } + statement_index: statements.len(), + }, + ); } fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) { From 9034b87a539904c96c01ece3c43878ce25887214 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 26 Oct 2018 03:07:29 +0900 Subject: [PATCH 36/38] Move in_macro check --- clippy_lints/src/redundant_clone.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 85f7bbb637c..8c895915921 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -99,6 +99,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { for (bb, bbdata) in mir.basic_blocks().iter_enumerated() { let terminator = bbdata.terminator(); + if in_macro(terminator.source_info.span) { + continue; + } + // Give up on loops if terminator.successors().any(|s| *s == bb) { continue; @@ -174,7 +178,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { }; if_chain! { - if !in_macro(span); if let Some(snip) = snippet_opt(cx, span); if let Some(dot) = snip.rfind('.'); then { From 9e15791f0ae14d66c093bb4bcd25986a5661a64b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Thu, 25 Oct 2018 20:14:39 +0200 Subject: [PATCH 37/38] ci: allow all branches except trying.tmp and staging.tmp to be built --- .travis.yml | 12 +++++------- appveyor.yml | 16 +++++++--------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/.travis.yml b/.travis.yml index 97cec5ee86b..75b45c9db40 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,13 +10,11 @@ os: sudo: false branches: - only: - # This is where pull requests from "bors r+" are built. - - staging - # This is where pull requests from "bors try" are built. - - trying - # Also build pull requests. - - master + # Don't build these branches + except: + # Used by bors + - trying.tmp + - staging.tmp env: global: diff --git a/appveyor.yml b/appveyor.yml index 8ab4a994476..fb0b326c713 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -6,16 +6,14 @@ environment: #- TARGET: i686-pc-windows-msvc #- TARGET: x86_64-pc-windows-gnu - TARGET: x86_64-pc-windows-msvc - + branches: - only: - # This is where pull requests from "bors r+" are built. - - staging - # This is where pull requests from "bors try" are built. - - trying - # Also build pull requests. - - master - + # Don't build these branches + except: + # Used by bors + - trying.tmp + - staging.tmp + install: - curl -sSf -o rustup-init.exe https://win.rustup.rs/ - rustup-init.exe -y --default-host %TARGET% --default-toolchain nightly From 326270ad1221b54028f9d029881fa0b1fb742db9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Fri, 26 Oct 2018 09:57:20 +0200 Subject: [PATCH 38/38] travis: work around temporary test failure due to rustc crashing on hyper. Upstream ticket: https://github.com/rust-lang/rust/issues/55376 --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 97cec5ee86b..04027833e7a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -65,7 +65,8 @@ matrix: - env: INTEGRATION=chronotope/chrono - env: INTEGRATION=serde-rs/serde - env: INTEGRATION=Geal/nom - - env: INTEGRATION=hyperium/hyper +# uncomment once https://github.com/rust-lang/rust/issues/55376 is fixed +# - env: INTEGRATION=hyperium/hyper allow_failures: - os: windows env: BASE_TEST=true