diff options
| -rw-r--r-- | clippy_lints/src/needless_pass_by_value.rs | 16 | ||||
| -rw-r--r-- | clippy_lints/src/suspicious_trait_impl.rs | 37 | ||||
| -rw-r--r--[-rwxr-xr-x] | clippy_lints/src/utils/ast_utils.rs | 0 | ||||
| -rw-r--r-- | tests/compile-test.rs | 3 | ||||
| -rw-r--r-- | tests/ui/suspicious_arithmetic_impl.rs | 30 | ||||
| -rwxr-xr-x | util/dev | 7 |
6 files changed, 51 insertions, 42 deletions
diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index 09577877744..8263f5eda33 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -40,9 +40,8 @@ declare_clippy_lint! { /// assert_eq!(v.len(), 42); /// } /// ``` - /// + /// should be /// ```rust - /// // should be /// fn foo(v: &[i32]) { /// assert_eq!(v.len(), 42); /// } @@ -159,26 +158,19 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { } } + // // * Exclude a type that is specifically bounded by `Borrow`. // * Exclude a type whose reference also fulfills its bound. (e.g., `std::convert::AsRef`, // `serde::Serialize`) let (implements_borrow_trait, all_borrowable_trait) = { - let preds = preds - .iter() - .filter(|t| t.self_ty() == ty) - .collect::<Vec<_>>(); + let preds = preds.iter().filter(|t| t.self_ty() == ty).collect::<Vec<_>>(); ( preds.iter().any(|t| t.def_id() == borrow_trait), !preds.is_empty() && { let ty_empty_region = cx.tcx.mk_imm_ref(cx.tcx.lifetimes.re_root_empty, ty); preds.iter().all(|t| { - let ty_params = t - .trait_ref - .substs - .iter() - .skip(1) - .collect::<Vec<_>>(); + let ty_params = t.trait_ref.substs.iter().skip(1).collect::<Vec<_>>(); implements_trait(cx, ty_empty_region, t.def_id(), &ty_params) }) }, diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs index 6d1d083fa8d..502fffc5e6c 100644 --- a/clippy_lints/src/suspicious_trait_impl.rs +++ b/clippy_lints/src/suspicious_trait_impl.rs @@ -64,26 +64,22 @@ impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl { | hir::BinOpKind::Gt => return, _ => {}, } - // Check if the binary expression is part of another bi/unary expression - // or operator assignment as a child node - let mut parent_expr = cx.tcx.hir().get_parent_node(expr.hir_id); - while parent_expr != hir::CRATE_HIR_ID { - if let hir::Node::Expr(e) = cx.tcx.hir().get(parent_expr) { - match e.kind { - hir::ExprKind::Binary(..) - | hir::ExprKind::Unary(hir::UnOp::UnNot | hir::UnOp::UnNeg, _) - | hir::ExprKind::AssignOp(..) => return, - _ => {}, + + // Check for more than one binary operation in the implemented function + // Linting when multiple operations are involved can result in false positives + if_chain! { + let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id); + if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn); + if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind; + let body = cx.tcx.hir().body(body_id); + let mut visitor = BinaryExprVisitor { nb_binops: 0 }; + + then { + walk_expr(&mut visitor, &body.value); + if visitor.nb_binops > 1 { + return; } } - parent_expr = cx.tcx.hir().get_parent_node(parent_expr); - } - // as a parent node - let mut visitor = BinaryExprVisitor { in_binary_expr: false }; - walk_expr(&mut visitor, expr); - - if visitor.in_binary_expr { - return; } if let Some(impl_trait) = check_binop( @@ -181,7 +177,7 @@ fn check_binop( } struct BinaryExprVisitor { - in_binary_expr: bool, + nb_binops: u32, } impl<'tcx> Visitor<'tcx> for BinaryExprVisitor { @@ -191,12 +187,13 @@ impl<'tcx> Visitor<'tcx> for BinaryExprVisitor { match expr.kind { hir::ExprKind::Binary(..) | hir::ExprKind::Unary(hir::UnOp::UnNot | hir::UnOp::UnNeg, _) - | hir::ExprKind::AssignOp(..) => self.in_binary_expr = true, + | hir::ExprKind::AssignOp(..) => self.nb_binops += 1, _ => {}, } walk_expr(self, expr); } + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { NestedVisitorMap::None } diff --git a/clippy_lints/src/utils/ast_utils.rs b/clippy_lints/src/utils/ast_utils.rs index 58c1103da9f..58c1103da9f 100755..100644 --- a/clippy_lints/src/utils/ast_utils.rs +++ b/clippy_lints/src/utils/ast_utils.rs diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 26a47d23706..eb6d495acbe 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -147,9 +147,6 @@ fn run_ui_toml(config: &mut compiletest::Config) { } fn run_ui_cargo(config: &mut compiletest::Config) { - if cargo::is_rustc_test_suite() { - return; - } fn run_tests( config: &compiletest::Config, filter: &Option<String>, diff --git a/tests/ui/suspicious_arithmetic_impl.rs b/tests/ui/suspicious_arithmetic_impl.rs index 1f5b9811887..60c2f3ec9b6 100644 --- a/tests/ui/suspicious_arithmetic_impl.rs +++ b/tests/ui/suspicious_arithmetic_impl.rs @@ -88,3 +88,33 @@ fn main() {} fn do_nothing(x: u32) -> u32 { x } + +struct MultipleBinops(u32); + +impl Add for MultipleBinops { + type Output = MultipleBinops; + + // OK: multiple Binops in `add` impl + fn add(self, other: Self) -> Self::Output { + let mut result = self.0 + other.0; + if result >= u32::max_value() { + result -= u32::max_value(); + } + MultipleBinops(result) + } +} + +impl Mul for MultipleBinops { + type Output = MultipleBinops; + + // OK: multiple Binops in `mul` impl + fn mul(self, other: Self) -> Self::Output { + let mut result: u32 = 0; + let size = std::cmp::max(self.0, other.0) as usize; + let mut v = vec![0; size + 1]; + for i in 0..size + 1 { + result *= i as u32; + } + MultipleBinops(result) + } +} diff --git a/util/dev b/util/dev deleted file mode 100755 index 319de217e0d..00000000000 --- a/util/dev +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/sh -CARGO_TARGET_DIR=$(pwd)/target/ -export CARGO_TARGET_DIR - -echo 'Deprecated! `util/dev` usage is deprecated, please use `cargo dev` instead.' - -cd clippy_dev && cargo run -- "$@" |