Auto merge of #4532 - rust-lang:integer-const, r=oli-obk

New `is_integer_const` to check more const ints This mostly affects loop checks and the modulo_one lint. Tests were also updated where applicable. changelog: none
rust-lang · Sep 10, 2019 · 12bb7d6 · 12bb7d6
2 parents c53d2b8 + 5823e94
commit 12bb7d6
Show file tree

Hide file tree

Showing 11 changed files with 98 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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,15 +260,15 @@ 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 {
                 node: BinOpKind::Sub, ..
             },
             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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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