Auto merge of #6177 - rust-lang:manual-range-contains, r=flip1995

New lint: manual-range-contains

This fixes #1110, at least for the contains-suggesting part.

- \[x] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`
---

changelog: new lint: manual-range-contains

CHANGELOG.md

@@ -1795,6 +1795,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

clippy_lints/src/lib.rs

@@ -792,6 +792,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,
@@ -1483,6 +1484,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),
@@ -1640,6 +1642,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),

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_sugg, span_lint_and_then,
+};
 use crate::utils::{higher, SpanlessEq};
 
 declare_clippy_lint! {
@@ -128,16 +132,171 @@ 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 {
+        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);
+        check_inclusive_range_minus_one(cx, expr);
+        check_reversed_empty_range(cx, expr);
+    }
+}
+
+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", "..")
+            };
+            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);
+            span_lint_and_sugg(
+                cx,
+                MANUAL_RANGE_CONTAINS,
+                span,
+                &format!("manual `{}::contains` implementation", range_type),
+                "use",
+                format!("({}{}{}).contains(&{})", lo, range_op, hi, name),
+                applicability,
+            );
+        } else if !combine_and && ord == Some(lord) {
+            // `!_.contains(_)`
+            // order lower bound and upper bound
+            let (l_span, u_span, l_inc, u_inc) = if lord == Ordering::Less {
+                (lval_span, rval_span, linc, rinc)
+            } else {
+                (rval_span, lval_span, rinc, linc)
+            };
+            if l_inc {
+                return;
+            }
+            let (range_type, range_op) = if u_inc {
+                ("Range", "..")
+            } else {
+                ("RangeInclusive", "..=")
+            };
+            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);
+            span_lint_and_sugg(
+                cx,
+                MANUAL_RANGE_CONTAINS,
+                span,
+                &format!("manual `!{}::contains` implementation", range_type),
+                "use",
+                format!("!({}{}{}).contains(&{})", lo, range_op, hi, name),
+                applicability,
+            );
+        }
+    }
+}
+
+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;
@@ -159,20 +318,15 @@ impl<'tcx> LateLintPass<'tcx> for Ranges {
             then {
                 span_lint(cx,
                     RANGE_ZIP_WITH_LEN,
-                                   expr.span,
+                    span,
                     &format!("it is more idiomatic to use `{}.iter().enumerate()`",
-                                            snippet(cx, iter_args[0].span, "_")));
+                        snippet(cx, iter_args[0].span, "_"))
+                );
             }
         }
     }
 }
 
-        check_exclusive_range_plus_one(cx, expr);
-        check_inclusive_range_minus_one(cx, expr);
-        check_reversed_empty_range(cx, expr);
-    }
-}
-
 // exclusive range plus one: `x..(y+1)`
 fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
     if_chain! {

src/lintlist/mod.rs

@@ -1173,6 +1173,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",

tests/ui/range_contains.fixed

@@ -0,0 +1,41 @@
+// run-rustfix
+
+#[warn(clippy::manual_range_contains)]
+#[allow(unused)]
+#[allow(clippy::no_effect)]
+#[allow(clippy::short_circuit_statement)]
+#[allow(clippy::unnecessary_operation)]
+fn main() {
+    let x = 9_u32;
+
+    // order shouldn't matter
+    (8..12).contains(&x);
+    (21..42).contains(&x);
+    (1..100).contains(&x);
+
+    // also with inclusive ranges
+    (9..=99).contains(&x);
+    (1..=33).contains(&x);
+    (1..=999).contains(&x);
+
+    // and the outside
+    !(8..12).contains(&x);
+    !(21..42).contains(&x);
+    !(1..100).contains(&x);
+
+    // also with the outside of inclusive ranges
+    !(9..=99).contains(&x);
+    !(1..=33).contains(&x);
+    !(1..=999).contains(&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
+
+    x <= 8 || x >= 12;
+    x >= 8 || x >= 12;
+    x < 12 || 12 < x;
+    x >= 8 || x <= 12;
+}

tests/ui/range_contains.rs

@@ -0,0 +1,41 @@
+// run-rustfix
+
+#[warn(clippy::manual_range_contains)]
+#[allow(unused)]
+#[allow(clippy::no_effect)]
+#[allow(clippy::short_circuit_statement)]
+#[allow(clippy::unnecessary_operation)]
+fn main() {
+    let x = 9_u32;
+
+    // order shouldn't matter
+    x >= 8 && x < 12;
+    x < 42 && x >= 21;
+    100 > x && 1 <= x;
+
+    // also with inclusive ranges
+    x >= 9 && x <= 99;
+    x <= 33 && x >= 1;
+    999 >= x && 1 <= x;
+
+    // and the outside
+    x < 8 || x >= 12;
+    x >= 42 || x < 21;
+    100 <= x || 1 > x;
+
+    // also with the outside of 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
+
+    x <= 8 || x >= 12;
+    x >= 8 || x >= 12;
+    x < 12 || 12 < x;
+    x >= 8 || x <= 12;
+}

tests/ui/range_contains.stderr

@@ -0,0 +1,76 @@
+error: manual `Range::contains` implementation
+  --> $DIR/range_contains.rs:12: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_contains.rs:13:5
+   |
+LL |     x < 42 && x >= 21;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(21..42).contains(&x)`
+
+error: manual `Range::contains` implementation
+  --> $DIR/range_contains.rs:14:5
+   |
+LL |     100 > x && 1 <= x;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(1..100).contains(&x)`
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:17:5
+   |
+LL |     x >= 9 && x <= 99;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(9..=99).contains(&x)`
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:18:5
+   |
+LL |     x <= 33 && x >= 1;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `(1..=33).contains(&x)`
+
+error: manual `RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:19:5
+   |
+LL |     999 >= x && 1 <= x;
+   |     ^^^^^^^^^^^^^^^^^^ help: use: `(1..=999).contains(&x)`
+
+error: manual `!Range::contains` implementation
+  --> $DIR/range_contains.rs:22:5
+   |
+LL |     x < 8 || x >= 12;
+   |     ^^^^^^^^^^^^^^^^ help: use: `!(8..12).contains(&x)`
+
+error: manual `!Range::contains` implementation
+  --> $DIR/range_contains.rs:23:5
+   |
+LL |     x >= 42 || x < 21;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `!(21..42).contains(&x)`
+
+error: manual `!Range::contains` implementation
+  --> $DIR/range_contains.rs:24:5
+   |
+LL |     100 <= x || 1 > x;
+   |     ^^^^^^^^^^^^^^^^^ help: use: `!(1..100).contains(&x)`
+
+error: manual `!RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:27:5
+   |
+LL |     x < 9 || x > 99;
+   |     ^^^^^^^^^^^^^^^ help: use: `!(9..=99).contains(&x)`
+
+error: manual `!RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:28:5
+   |
+LL |     x > 33 || x < 1;
+   |     ^^^^^^^^^^^^^^^ help: use: `!(1..=33).contains(&x)`
+
+error: manual `!RangeInclusive::contains` implementation
+  --> $DIR/range_contains.rs:29:5
+   |
+LL |     999 < x || 1 > x;
+   |     ^^^^^^^^^^^^^^^^ help: use: `!(1..=999).contains(&x)`
+
+error: aborting due to 12 previous errors
+