From 6d989c729d03f5f9900d153417488b5eaf1e2f2f Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 23 Aug 2017 17:54:35 +0200 Subject: [PATCH] add closure arg check, also catch non-consts --- clippy_lints/src/bytecount.rs | 47 ++++++++++++++++++++++++++++++----- clippy_lints/src/shadow.rs | 30 +++------------------- clippy_lints/src/utils/mod.rs | 28 +++++++++++++++++++++ tests/ui/bytecount.rs | 13 ++++++++++ tests/ui/bytecount.stderr | 26 +++++++++++++------ 5 files changed, 103 insertions(+), 41 deletions(-) diff --git a/clippy_lints/src/bytecount.rs b/clippy_lints/src/bytecount.rs index 99987f84b..cbd0c6d20 100644 --- a/clippy_lints/src/bytecount.rs +++ b/clippy_lints/src/bytecount.rs @@ -1,8 +1,8 @@ -use consts::{constant, Constant}; -use rustc_const_math::ConstInt; use rustc::hir::*; use rustc::lint::*; -use utils::{match_type, paths, snippet, span_lint_and_sugg, walk_ptrs_ty}; +use rustc::ty; +use syntax::ast::{Name, UintTy}; +use utils::{contains_name, match_type, paths, single_segment_path, snippet, span_lint_and_sugg, walk_ptrs_ty}; /// **What it does:** Checks for naive byte counts /// @@ -47,14 +47,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ByteCount { ], { let body = cx.tcx.hir.body(body_id); if_let_chain!([ + body.arguments.len() == 1, + let Some(argname) = get_pat_name(&body.arguments[0].pat), let ExprBinary(ref op, ref l, ref r) = body.value.node, op.node == BiEq, match_type(cx, walk_ptrs_ty(cx.tables.expr_ty(&filter_args[0])), &paths::SLICE_ITER), - let Some((Constant::Int(ConstInt::U8(needle)), _)) = - constant(cx, l).or_else(|| constant(cx, r)) ], { + let needle = match get_path_name(l) { + Some(name) if check_arg(name, argname, r) => r, + _ => match get_path_name(r) { + Some(name) if check_arg(name, argname, l) => l, + _ => { return; } + } + }; + if ty::TyUint(UintTy::U8) != walk_ptrs_ty(cx.tables.expr_ty(needle)).sty { + return; + } let haystack = if let ExprMethodCall(ref path, _, ref args) = filter_args[0].node { let p = path.name; @@ -73,8 +83,33 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ByteCount { "Consider using the bytecount crate", format!("bytecount::count({}, {})", snippet(cx, haystack.span, ".."), - needle)); + snippet(cx, needle.span, ".."))); }); }); } } + +fn check_arg(name: Name, arg: Name, needle: &Expr) -> bool { + name == arg && !contains_name(name, needle) +} + +fn get_pat_name(pat: &Pat) -> Option { + match pat.node { + PatKind::Binding(_, _, ref spname, _) => Some(spname.node), + PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.name), + PatKind::Box(ref p) | PatKind::Ref(ref p, _) => get_pat_name(&*p), + _ => None + } +} + +fn get_path_name(expr: &Expr) -> Option { + match expr.node { + ExprBox(ref e) | ExprAddrOf(_, ref e) | ExprUnary(UnOp::UnDeref, ref e) => get_path_name(e), + ExprBlock(ref b) => if b.stmts.is_empty() { + b.expr.as_ref().and_then(|p| get_path_name(p)) + } else { None }, + ExprPath(ref qpath) => single_segment_path(qpath).map(|ps| ps.name), + _ => None + } +} + diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index b4857f1b6..ccb339390 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -1,10 +1,10 @@ use reexport::*; use rustc::lint::*; use rustc::hir::*; -use rustc::hir::intravisit::{Visitor, FnKind, NestedVisitorMap}; +use rustc::hir::intravisit::FnKind; use rustc::ty; use syntax::codemap::Span; -use utils::{higher, in_external_macro, snippet, span_lint_and_then, iter_input_pats}; +use utils::{contains_name, higher, in_external_macro, snippet, span_lint_and_then, iter_input_pats}; /// **What it does:** Checks for bindings that shadow other bindings already in /// scope, while just changing reference level or mutability. @@ -261,7 +261,7 @@ fn lint_shadow<'a, 'tcx: 'a>( ), |db| { db.span_note(prev_span, "previous binding is here"); }, ); - } else if contains_self(name, expr) { + } else if contains_name(name, expr) { span_lint_and_then( cx, SHADOW_REUSE, @@ -391,27 +391,3 @@ fn path_eq_name(name: Name, path: &Path) -> bool { !path.is_global() && path.segments.len() == 1 && path.segments[0].name.as_str() == name.as_str() } -struct ContainsSelf { - name: Name, - result: bool, -} - -impl<'tcx> Visitor<'tcx> for ContainsSelf { - fn visit_name(&mut self, _: Span, name: Name) { - if self.name == name { - self.result = true; - } - } - fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { - NestedVisitorMap::None - } -} - -fn contains_self(name: Name, expr: &Expr) -> bool { - let mut cs = ContainsSelf { - name: name, - result: false, - }; - cs.visit_expr(expr); - cs.result -} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 7f8603f0f..c24ccd5d5 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -3,6 +3,7 @@ use rustc::hir; use rustc::hir::*; use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX}; use rustc::hir::def::Def; +use rustc::hir::intravisit::{NestedVisitorMap, Visitor}; use rustc::hir::map::Node; use rustc::lint::{LintContext, Level, LateContext, Lint}; use rustc::session::Session; @@ -393,6 +394,33 @@ pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option { } } +struct ContainsName { + name: Name, + result: bool, +} + +impl<'tcx> Visitor<'tcx> for ContainsName { + fn visit_name(&mut self, _: Span, name: Name) { + if self.name == name { + self.result = true; + } + } + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} + +/// check if an `Expr` contains a certain name +pub fn contains_name(name: Name, expr: &Expr) -> bool { + let mut cn = ContainsName { + name: name, + result: false, + }; + cn.visit_expr(expr); + cn.result +} + + /// Convert a span to a code snippet if available, otherwise use default. /// /// # Example diff --git a/tests/ui/bytecount.rs b/tests/ui/bytecount.rs index 880a86eba..8fc27c49f 100644 --- a/tests/ui/bytecount.rs +++ b/tests/ui/bytecount.rs @@ -1,6 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] +#[deny(naive_bytecount)] fn main() { let x = vec![0_u8; 16]; @@ -11,4 +12,16 @@ fn main() { let _ = x.iter().filter(|a| **a > 0).count(); // not an equality count, OK. let _ = x.iter().map(|a| a + 1).filter(|&a| a < 15).count(); // not a slice + + let b = 0; + + let _ = x.iter().filter(|_| b > 0).count(); // woah there + + let _ = x.iter().filter(|_a| b == b + 1).count(); // nothing to see here, move along + + let _ = x.iter().filter(|a| b + 1 == **a).count(); // naive byte count + + let y = vec![0_u16; 3]; + + let _ = y.iter().filter(|&&a| a == 0).count(); // naive count, but not bytes } diff --git a/tests/ui/bytecount.stderr b/tests/ui/bytecount.stderr index 818fa7eab..307edecfd 100644 --- a/tests/ui/bytecount.stderr +++ b/tests/ui/bytecount.stderr @@ -1,16 +1,26 @@ error: You appear to be counting bytes the naive way - --> $DIR/bytecount.rs:7:13 + --> $DIR/bytecount.rs:8:13 | -7 | let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count +8 | let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count(x, 0)` | - = note: `-D naive-bytecount` implied by `-D warnings` +note: lint level defined here + --> $DIR/bytecount.rs:4:8 + | +4 | #[deny(naive_bytecount)] + | ^^^^^^^^^^^^^^^ error: You appear to be counting bytes the naive way - --> $DIR/bytecount.rs:9:13 - | -9 | let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count((&x[..]), 0)` + --> $DIR/bytecount.rs:10:13 + | +10 | let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count((&x[..]), 0)` -error: aborting due to 2 previous errors +error: You appear to be counting bytes the naive way + --> $DIR/bytecount.rs:22:13 + | +22 | let _ = x.iter().filter(|a| b + 1 == **a).count(); // naive byte count + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count(x, b + 1)` + +error: aborting due to 3 previous errors