diff options
| -rw-r--r-- | clippy_lints/src/loops.rs | 10 | ||||
| -rw-r--r-- | clippy_lints/src/misc.rs | 4 | ||||
| -rw-r--r-- | clippy_lints/src/ranges.rs | 18 | ||||
| -rw-r--r-- | clippy_lints/src/utils/mod.rs | 17 | ||||
| -rw-r--r-- | tests/ui/for_loop.rs | 8 | ||||
| -rw-r--r-- | tests/ui/for_loop.stderr | 14 | ||||
| -rw-r--r-- | tests/ui/modulo_one.rs | 7 | ||||
| -rw-r--r-- | tests/ui/modulo_one.stderr | 24 | ||||
| -rw-r--r-- | tests/ui/range_plus_minus_one.fixed | 4 | ||||
| -rw-r--r-- | tests/ui/range_plus_minus_one.rs | 4 | ||||
| -rw-r--r-- | tests/ui/range_plus_minus_one.stderr | 8 |
11 files changed, 98 insertions, 20 deletions
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 5816224a275..26a4076bac4 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -27,7 +27,7 @@ use syntax_pos::{BytePos, Symbol}; use crate::utils::paths; use crate::utils::{ - get_enclosing_block, get_parent_expr, has_iter_method, higher, is_integer_literal, is_refutable, last_path_segment, + get_enclosing_block, get_parent_expr, has_iter_method, higher, is_integer_const, is_refutable, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, SpanlessEq, }; @@ -1096,7 +1096,7 @@ fn check_for_loop_range<'a, 'tcx>( return; } - let starts_at_zero = is_integer_literal(start, 0); + let starts_at_zero = is_integer_const(cx, start, 0); let skip = if starts_at_zero { String::new() @@ -2042,7 +2042,7 @@ impl<'a, 'tcx> Visitor<'tcx> for IncrementVisitor<'a, 'tcx> { match parent.node { ExprKind::AssignOp(op, ref lhs, ref rhs) => { if lhs.hir_id == expr.hir_id { - if op.node == BinOpKind::Add && is_integer_literal(rhs, 1) { + if op.node == BinOpKind::Add && is_integer_const(self.cx, rhs, 1) { *state = match *state { VarState::Initial if self.depth == 0 => VarState::IncrOnce, _ => VarState::DontWarn, @@ -2094,7 +2094,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { self.name = Some(ident.name); self.state = if let Some(ref init) = local.init { - if is_integer_literal(init, 0) { + if is_integer_const(&self.cx, init, 0) { VarState::Warn } else { VarState::Declared @@ -2130,7 +2130,7 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { self.state = VarState::DontWarn; }, ExprKind::Assign(ref lhs, ref rhs) if lhs.hir_id == expr.hir_id => { - self.state = if is_integer_literal(rhs, 0) && self.depth == 0 { + self.state = if is_integer_const(&self.cx, rhs, 0) && self.depth == 0 { VarState::Warn } else { VarState::DontWarn diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index a761f80b793..38fdabaaf7a 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -12,7 +12,7 @@ use syntax::source_map::{ExpnKind, Span}; use crate::consts::{constant, Constant}; use crate::utils::sugg::Sugg; use crate::utils::{ - get_item_name, get_parent_expr, implements_trait, in_constant, is_integer_literal, iter_input_pats, + get_item_name, get_parent_expr, implements_trait, in_constant, is_integer_const, iter_input_pats, last_path_segment, match_qpath, match_trait_method, paths, snippet, span_lint, span_lint_and_then, span_lint_hir_and_then, walk_ptrs_ty, SpanlessEq, }; @@ -388,7 +388,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints { ); db.span_note(expr.span, "std::f32::EPSILON and std::f64::EPSILON are available."); }); - } else if op == BinOpKind::Rem && is_integer_literal(right, 1) { + } else if op == BinOpKind::Rem && is_integer_const(cx, right, 1) { span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0"); } }, diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index 7b6b6cbc8f8..0fb40ee9f56 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -8,7 +8,7 @@ use syntax::source_map::Spanned; use crate::utils::sugg::Sugg; use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq}; -use crate::utils::{is_integer_literal, paths, snippet, snippet_opt, span_lint, span_lint_and_then}; +use crate::utils::{is_integer_const, paths, snippet, snippet_opt, span_lint, span_lint_and_then}; declare_clippy_lint! { /// **What it does:** Checks for calling `.step_by(0)` on iterators, @@ -132,7 +132,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges { if iter_path.ident.name == sym!(iter); // range expression in `.zip()` call: `0..x.len()` if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(cx, zip_arg); - if is_integer_literal(start, 0); + if is_integer_const(cx, start, 0); // `.len()` call if let ExprKind::MethodCall(ref len_path, _, ref len_args) = end.node; if len_path.ident.name == sym!(len) && len_args.len() == 1; @@ -164,7 +164,7 @@ fn check_exclusive_range_plus_one(cx: &LateContext<'_, '_>, expr: &Expr) { end: Some(end), limits: RangeLimits::HalfOpen }) = higher::range(cx, expr); - if let Some(y) = y_plus_one(end); + if let Some(y) = y_plus_one(cx, end); then { let span = if expr.span.from_expansion() { expr.span @@ -209,7 +209,7 @@ fn check_exclusive_range_plus_one(cx: &LateContext<'_, '_>, expr: &Expr) { fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr) { if_chain! { if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr); - if let Some(y) = y_minus_one(end); + if let Some(y) = y_minus_one(cx, end); then { span_lint_and_then( cx, @@ -239,7 +239,7 @@ fn has_step_by(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { get_trait_def_id(cx, &paths::ITERATOR).map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[])) } -fn y_plus_one(expr: &Expr) -> Option<&Expr> { +fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> { match expr.node { ExprKind::Binary( Spanned { @@ -248,9 +248,9 @@ fn y_plus_one(expr: &Expr) -> Option<&Expr> { ref lhs, ref rhs, ) => { - if is_integer_literal(lhs, 1) { + if is_integer_const(cx, lhs, 1) { Some(rhs) - } else if is_integer_literal(rhs, 1) { + } else if is_integer_const(cx, rhs, 1) { Some(lhs) } else { None @@ -260,7 +260,7 @@ fn y_plus_one(expr: &Expr) -> Option<&Expr> { } } -fn y_minus_one(expr: &Expr) -> Option<&Expr> { +fn y_minus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr) -> Option<&'t Expr> { match expr.node { ExprKind::Binary( Spanned { @@ -268,7 +268,7 @@ fn y_minus_one(expr: &Expr) -> Option<&Expr> { }, ref lhs, ref rhs, - ) if is_integer_literal(rhs, 1) => Some(lhs), + ) if is_integer_const(cx, rhs, 1) => Some(lhs), _ => None, } } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index bcb44315839..438274fad28 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -48,6 +48,7 @@ use syntax::source_map::{Span, DUMMY_SP}; use syntax::symbol::{kw, Symbol}; use crate::reexport::*; +use crate::consts::{constant, Constant}; /// Returns `true` if the two spans come from differing expansions (i.e., one is /// from a macro and one isn't). @@ -669,6 +670,22 @@ pub fn walk_ptrs_ty_depth(ty: Ty<'_>) -> (Ty<'_>, usize) { inner(ty, 0) } +/// Checks whether the given expression is a constant integer of the given value. +/// unlike `is_integer_literal`, this version does const folding +pub fn is_integer_const(cx: &LateContext<'_, '_>, e: &Expr, value: u128) -> bool { + if is_integer_literal(e, value) { + return true; + } + let map = cx.tcx.hir(); + let parent_item = map.get_parent_item(e.hir_id); + if let Some((Constant::Int(v), _)) = map.maybe_body_owned_by(parent_item) + .and_then(|body_id| constant(cx, cx.tcx.body_tables(body_id), e)) { + value == v + } else { + false + } +} + /// Checks whether the given expression is a constant literal of the given value. pub fn is_integer_literal(expr: &Expr, value: u128) -> bool { // FIXME: use constant folding diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index 14f21f1b703..5d367a62fc9 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -275,6 +275,14 @@ fn main() { for mid in 1..vec.len() { let (_, _) = vec.split_at(mid); } + + const ZERO: usize = 0; + + for i in ZERO..vec.len() { + if f(&vec[i], &vec[i]) { + panic!("at the disco"); + } + } } #[allow(dead_code)] diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 257a05c02e0..0f84abf45ed 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -152,5 +152,17 @@ LL | for _v in vec.iter().next() {} | = note: `-D clippy::iter-next-loop` implied by `-D warnings` -error: aborting due to 21 previous errors +error: the loop variable `i` is only used to index `vec`. + --> $DIR/for_loop.rs:281:14 + | +LL | for i in ZERO..vec.len() { + | ^^^^^^^^^^^^^^^ + | + = note: `-D clippy::needless-range-loop` implied by `-D warnings` +help: consider using an iterator + | +LL | for <item> in &vec { + | ^^^^^^ ^^^^ + +error: aborting due to 22 previous errors diff --git a/tests/ui/modulo_one.rs b/tests/ui/modulo_one.rs index 81603175ab4..cc8c8e7cdae 100644 --- a/tests/ui/modulo_one.rs +++ b/tests/ui/modulo_one.rs @@ -1,7 +1,14 @@ #![warn(clippy::modulo_one)] #![allow(clippy::no_effect, clippy::unnecessary_operation)] +static STATIC_ONE: usize = 2 - 1; + fn main() { 10 % 1; 10 % 2; + + const ONE: u32 = 1 * 1; + + 2 % ONE; + 5 % STATIC_ONE; } diff --git a/tests/ui/modulo_one.stderr b/tests/ui/modulo_one.stderr index a7feeb56ebc..6bee68360b6 100644 --- a/tests/ui/modulo_one.stderr +++ b/tests/ui/modulo_one.stderr @@ -1,10 +1,30 @@ error: any number modulo 1 will be 0 - --> $DIR/modulo_one.rs:5:5 + --> $DIR/modulo_one.rs:7:5 | LL | 10 % 1; | ^^^^^^ | = note: `-D clippy::modulo-one` implied by `-D warnings` -error: aborting due to previous error +error: the operation is ineffective. Consider reducing it to `1` + --> $DIR/modulo_one.rs:10:22 + | +LL | const ONE: u32 = 1 * 1; + | ^^^^^ + | + = note: `-D clippy::identity-op` implied by `-D warnings` + +error: the operation is ineffective. Consider reducing it to `1` + --> $DIR/modulo_one.rs:10:22 + | +LL | const ONE: u32 = 1 * 1; + | ^^^^^ + +error: any number modulo 1 will be 0 + --> $DIR/modulo_one.rs:12:5 + | +LL | 2 % ONE; + | ^^^^^^^ + +error: aborting due to 4 previous errors diff --git a/tests/ui/range_plus_minus_one.fixed b/tests/ui/range_plus_minus_one.fixed index badfb5baf3c..6b402114099 100644 --- a/tests/ui/range_plus_minus_one.fixed +++ b/tests/ui/range_plus_minus_one.fixed @@ -32,6 +32,10 @@ fn main() { let _ = (1..=11); let _ = ((f() + 1)..=f()); + const ONE: usize = 1; + // integer consts are linted, too + for _ in 1..=ONE {} + let mut vec: Vec<()> = std::vec::Vec::new(); vec.drain(..); } diff --git a/tests/ui/range_plus_minus_one.rs b/tests/ui/range_plus_minus_one.rs index c4facd2c23d..3cfed4125b3 100644 --- a/tests/ui/range_plus_minus_one.rs +++ b/tests/ui/range_plus_minus_one.rs @@ -32,6 +32,10 @@ fn main() { let _ = (1..11 + 1); let _ = (f() + 1)..(f() + 1); + const ONE: usize = 1; + // integer consts are linted, too + for _ in 1..ONE + ONE {} + let mut vec: Vec<()> = std::vec::Vec::new(); vec.drain(..); } diff --git a/tests/ui/range_plus_minus_one.stderr b/tests/ui/range_plus_minus_one.stderr index 8318f6b2596..f72943a04f2 100644 --- a/tests/ui/range_plus_minus_one.stderr +++ b/tests/ui/range_plus_minus_one.stderr @@ -50,5 +50,11 @@ error: an inclusive range would be more readable LL | let _ = (f() + 1)..(f() + 1); | ^^^^^^^^^^^^^^^^^^^^ help: use: `((f() + 1)..=f())` -error: aborting due to 8 previous errors +error: an inclusive range would be more readable + --> $DIR/range_plus_minus_one.rs:37:14 + | +LL | for _ in 1..ONE + ONE {} + | ^^^^^^^^^^^^ help: use: `1..=ONE` + +error: aborting due to 9 previous errors |