New lint: manual-range-contains

llogiq · llogiq · commit a77d6ee8fcd4 · 2020-10-15T22:05:51.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1793,6 +1793,7 @@ Released 2018-09-13
 [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
 [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
+[`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
 [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
 [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip
 [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -783,6 +783,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &ptr_eq::PTR_EQ,
         &ptr_offset_with_cast::PTR_OFFSET_WITH_CAST,
         &question_mark::QUESTION_MARK,
+        &ranges::MANUAL_RANGE_CONTAINS,
         &ranges::RANGE_MINUS_ONE,
         &ranges::RANGE_PLUS_ONE,
         &ranges::RANGE_ZIP_WITH_LEN,
@@ -1465,6 +1466,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&ptr_eq::PTR_EQ),
         LintId::of(&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST),
         LintId::of(&question_mark::QUESTION_MARK),
+        LintId::of(&ranges::MANUAL_RANGE_CONTAINS),
         LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
         LintId::of(&ranges::REVERSED_EMPTY_RANGES),
         LintId::of(&redundant_clone::REDUNDANT_CLONE),
@@ -1620,6 +1622,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&ptr::PTR_ARG),
         LintId::of(&ptr_eq::PTR_EQ),
         LintId::of(&question_mark::QUESTION_MARK),
+        LintId::of(&ranges::MANUAL_RANGE_CONTAINS),
         LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES),
         LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
         LintId::of(&regex::TRIVIAL_REGEX),
diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs
@@ -2,15 +2,19 @@ use crate::consts::{constant, Constant};
 use if_chain::if_chain;
 use rustc_ast::ast::RangeLimits;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
+use rustc_hir::{BinOpKind, Expr, ExprKind, PathSegment, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::source_map::Spanned;
+use rustc_span::source_map::{Span, Spanned};
+use rustc_span::symbol::Ident;
 use std::cmp::Ordering;
 
 use crate::utils::sugg::Sugg;
-use crate::utils::{get_parent_expr, is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
+use crate::utils::{
+    get_parent_expr, is_integer_const, single_segment_path, snippet, snippet_opt, snippet_with_applicability,
+    span_lint, span_lint_and_then,
+};
 use crate::utils::{higher, SpanlessEq};
 
 declare_clippy_lint! {
@@ -128,43 +132,51 @@ declare_clippy_lint! {
     "reversing the limits of range expressions, resulting in empty ranges"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for expressions like `x >= 3 && x < 8` that could
+    /// be more readably expressed as `(3..8).contains(x)`.
+    ///
+    /// **Why is this bad?** `contains` expresses the intent better and has less
+    /// failure modes (such as fencepost errors or using `||` instead of `&&`).
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// // given
+    /// let x = 6;
+    ///
+    /// assert!(x >= 3 && x < 8);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    ///# let x = 6;
+    /// assert!((3..8).contains(x));
+    /// ```
+    pub MANUAL_RANGE_CONTAINS,
+    style,
+    "manually reimplementing {`Range`, `RangeInclusive`}`::contains`"
+}
+
 declare_lint_pass!(Ranges => [
     RANGE_ZIP_WITH_LEN,
     RANGE_PLUS_ONE,
     RANGE_MINUS_ONE,
     REVERSED_EMPTY_RANGES,
+    MANUAL_RANGE_CONTAINS,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Ranges {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if let ExprKind::MethodCall(ref path, _, ref args, _) = expr.kind {
-            let name = path.ident.as_str();
-            if name == "zip" && args.len() == 2 {
-                let iter = &args[0].kind;
-                let zip_arg = &args[1];
-                if_chain! {
-                    // `.iter()` call
-                    if let ExprKind::MethodCall(ref iter_path, _, ref iter_args , _) = *iter;
-                    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(zip_arg);
-                    if is_integer_const(cx, start, 0);
-                    // `.len()` call
-                    if let ExprKind::MethodCall(ref len_path, _, ref len_args, _) = end.kind;
-                    if len_path.ident.name == sym!(len) && len_args.len() == 1;
-                    // `.iter()` and `.len()` called on same `Path`
-                    if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].kind;
-                    if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
-                    if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
-                     then {
-                         span_lint(cx,
-                                   RANGE_ZIP_WITH_LEN,
-                                   expr.span,
-                                   &format!("it is more idiomatic to use `{}.iter().enumerate()`",
-                                            snippet(cx, iter_args[0].span, "_")));
-                    }
-                }
-            }
+        match expr.kind {
+            ExprKind::MethodCall(ref path, _, ref args, _) => {
+                check_range_zip_with_len(cx, path, args, expr.span);
+            },
+            ExprKind::Binary(ref op, ref l, ref r) => {
+                check_possible_range_contains(cx, op.node, l, r, expr.span);
+            },
+            _ => {},
         }
 
         check_exclusive_range_plus_one(cx, expr);
@@ -173,6 +185,124 @@ impl<'tcx> LateLintPass<'tcx> for Ranges {
     }
 }
 
+fn check_possible_range_contains(cx: &LateContext<'_>, op: BinOpKind, l: &Expr<'_>, r: &Expr<'_>, span: Span) {
+    let combine_and = match op {
+        BinOpKind::And | BinOpKind::BitAnd => true,
+        BinOpKind::Or | BinOpKind::BitOr => false,
+        _ => return,
+    };
+    // value, name, order (higher/lower), inclusiveness
+    if let (Some((lval, lname, name_span, lval_span, lord, linc)), Some((rval, rname, _, rval_span, rord, rinc))) =
+        (check_range_bounds(cx, l), check_range_bounds(cx, r))
+    {
+        // we only lint comparisons on the same name and with different
+        // direction
+        if lname != rname || lord == rord {
+            return;
+        }
+        let ord = Constant::partial_cmp(cx.tcx, cx.typeck_results().expr_ty(l), &lval, &rval);
+        if combine_and && ord == Some(rord) {
+            // order lower bound and upper bound
+            let (l_span, u_span, l_inc, u_inc) = if rord == Ordering::Less {
+                (lval_span, rval_span, linc, rinc)
+            } else {
+                (rval_span, lval_span, rinc, linc)
+            };
+            // we only lint inclusive lower bounds
+            if !l_inc {
+                return;
+            }
+            let (range_type, range_op) = if u_inc {
+                ("RangeInclusive", "..=")
+            } else {
+                ("Range", "..")
+            };
+            span_lint_and_then(
+                cx,
+                MANUAL_RANGE_CONTAINS,
+                span,
+                &format!("manual `{}::contains` implementation", range_type),
+                |diag| {
+                    let mut applicability = Applicability::MachineApplicable;
+                    let name = snippet_with_applicability(cx, name_span, "_", &mut applicability);
+                    let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability);
+                    let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability);
+                    diag.span_suggestion(
+                        span,
+                        "use",
+                        format!("({}{}{}).contains({})", lo, range_op, hi, name),
+                        applicability,
+                    );
+                },
+            );
+        }
+    }
+    //TODO: lint !(..).contains
+}
+
+fn check_range_bounds(cx: &LateContext<'_>, ex: &Expr<'_>) -> Option<(Constant, Ident, Span, Span, Ordering, bool)> {
+    if let ExprKind::Binary(ref op, ref l, ref r) = ex.kind {
+        let (inclusive, ordering) = match op.node {
+            BinOpKind::Gt => (false, Ordering::Greater),
+            BinOpKind::Ge => (true, Ordering::Greater),
+            BinOpKind::Lt => (false, Ordering::Less),
+            BinOpKind::Le => (true, Ordering::Less),
+            _ => return None,
+        };
+        if let Some(id) = match_ident(l) {
+            if let Some((c, _)) = constant(cx, cx.typeck_results(), r) {
+                return Some((c, id, l.span, r.span, ordering, inclusive));
+            }
+        } else if let Some(id) = match_ident(r) {
+            if let Some((c, _)) = constant(cx, cx.typeck_results(), l) {
+                return Some((c, id, r.span, l.span, ordering.reverse(), inclusive));
+            }
+        }
+    }
+    None
+}
+
+fn match_ident(e: &Expr<'_>) -> Option<Ident> {
+    if let ExprKind::Path(ref qpath) = e.kind {
+        if let Some(seg) = single_segment_path(qpath) {
+            if seg.args.is_none() {
+                return Some(seg.ident);
+            }
+        }
+    }
+    None
+}
+
+fn check_range_zip_with_len(cx: &LateContext<'_>, path: &PathSegment<'_>, args: &[Expr<'_>], span: Span) {
+    let name = path.ident.as_str();
+    if name == "zip" && args.len() == 2 {
+        let iter = &args[0].kind;
+        let zip_arg = &args[1];
+        if_chain! {
+            // `.iter()` call
+            if let ExprKind::MethodCall(ref iter_path, _, ref iter_args, _) = *iter;
+            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(zip_arg);
+            if is_integer_const(cx, start, 0);
+            // `.len()` call
+            if let ExprKind::MethodCall(ref len_path, _, ref len_args, _) = end.kind;
+            if len_path.ident.name == sym!(len) && len_args.len() == 1;
+            // `.iter()` and `.len()` called on same `Path`
+            if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].kind;
+            if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
+            if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
+            then {
+                span_lint(cx,
+                RANGE_ZIP_WITH_LEN,
+                span,
+                &format!("it is more idiomatic to use `{}.iter().enumerate()`",
+                    snippet(cx, iter_args[0].span, "_")));
+            }
+        }
+    }
+}
+
 // exclusive range plus one: `x..(y+1)`
 fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
     if_chain! {
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
@@ -1159,6 +1159,13 @@ vec![
         deprecation: None,
         module: "manual_non_exhaustive",
     },
+    Lint {
+        name: "manual_range_contains",
+        group: "style",
+        desc: "manually reimplementing {`Range`, `RangeInclusive`}`::contains`",
+        deprecation: None,
+        module: "ranges",
+    },
     Lint {
         name: "manual_saturating_arithmetic",
         group: "style",
diff --git a/tests/ui/range.rs b/tests/ui/range.rs
@@ -14,3 +14,25 @@ fn no_panic_with_fake_range_types() {
 
     let _ = Range { foo: 0 };
 }
+
+#[warn(clippy::manual_range_contains)]
+#[allow(unused)]
+#[allow(clippy::no_effect)]
+#[allow(clippy::short_circuit_statement)]
+fn manual_range_contains(x: u32) {
+    // order shouldn't matter
+    x >= 8 && x < 12;
+    x < 42 && x >= 21;
+    100 > x && 0 <= x;
+
+    // also with inclusive ranges
+    x >= 9 && x <= 99;
+    x <= 33 && x >= 1;
+    999 >= x && 1 <= x;
+
+    // not a range.contains
+    x > 8 && x < 12; // lower bound not inclusive
+    x < 8 && x <= 12; // same direction
+    x >= 12 && 12 >= x; // same bounds
+    x < 8 && x > 12; // wrong direction
+}
diff --git a/tests/ui/range.stderr b/tests/ui/range.stderr
@@ -6,5 +6,60 @@ LL |     let _x = v1.iter().zip(0..v1.len());
    |
    = note: `-D clippy::range-zip-with-len` implied by `-D warnings`
 
-error: aborting due to previous error
+error: manual `Range::contains` implementation
+  --> $DIR/range.rs:24:5
+   |
+LL |     x >= 8 && x < 12;
+   |     ^^^^^^^^^^^^^^^^ help: use: `(8..12).contains(x)`
+   |
+   = note: `-D clippy::manual-range-contains` implied by `-D warnings`
+
+error: manual `Range::contains` implementation
+  --> $DIR/range.rs:25:5
+   |
+LL |     x < 42 && x >= 21;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(21..42).contains(x)`
+
+error: manual `Range::contains` implementation
+  --> $DIR/range.rs:26:5
+   |
+LL |     100 > x && 0 <= x;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(0..100).contains(x)`
+
+error: this comparison involving the minimum or maximum element for this type contains a case that is always true or always false
+  --> $DIR/range.rs:26:16
+   |
+LL |     100 > x && 0 <= x;
+   |                ^^^^^^
+   |
+   = note: `#[deny(clippy::absurd_extreme_comparisons)]` on by default
+   = help: because `0` is the minimum value for this type, this comparison is always true
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range.rs:29:5
+   |
+LL |     x >= 9 && x <= 99;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(9..=99).contains(x)`
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range.rs:30:5
+   |
+LL |     x <= 33 && x >= 1;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(1..=33).contains(x)`
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range.rs:31:5
+   |
+LL |     999 >= x && 1 <= x;
+   |     ^^^^^^^^^^^^^^^^^^ help: use: `(1..=999).contains(x)`
+
+error: comparison is useless due to type limits
+  --> $DIR/range.rs:26:16
+   |
+LL |     100 > x && 0 <= x;
+   |                ^^^^^^
+   |
+   = note: `-D unused-comparisons` implied by `-D warnings`
+
+error: aborting due to 9 previous errors
 
