Refactor range_plus_one and range_minus_one lints

Those lints share the detection logic structure, but differed, probably
because of an oversight, in lint emission: only one of them would take
care of emitting parentheses around the suggestion if required.

Factor the code into one function which checks for the right condition
and emits the lint in an identical way.

Author: samueltardieu

Diff for: clippy_lints/src/ranges.rs

@@ -10,7 +10,7 @@ use clippy_utils::{
 use rustc_ast::ast::RangeLimits;
 use rustc_errors::Applicability;
 use rustc_hir::{BinOpKind, Expr, ExprKind, HirId, LangItem};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_lint::{LateContext, LateLintPass, Lint};
 use rustc_middle::ty::{self, ClauseKind, PredicatePolarity};
 use rustc_session::impl_lint_pass;
 use rustc_span::source_map::Spanned;
@@ -430,71 +430,71 @@ fn can_switch_ranges(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
 }
 
 // exclusive range plus one: `x..(y+1)`
-fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
-    if expr.span.can_be_used_for_suggestions()
-        && let Some(higher::Range {
-            start,
-            end: Some(end),
-            limits: RangeLimits::HalfOpen,
-        }) = higher::Range::hir(expr)
-        && let Some(y) = y_plus_one(cx, end)
-        && can_switch_ranges(cx, expr)
-    {
-        let span = expr.span;
-        span_lint_and_then(
-            cx,
-            RANGE_PLUS_ONE,
-            span,
-            "an inclusive range would be more readable",
-            |diag| {
-                let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").maybe_paren().to_string());
-                let end = Sugg::hir(cx, y, "y").maybe_paren();
-                match span.with_source_text(cx, |src| src.starts_with('(') && src.ends_with(')')) {
-                    Some(true) => {
-                        diag.span_suggestion(span, "use", format!("({start}..={end})"), Applicability::MaybeIncorrect);
-                    },
-                    Some(false) => {
-                        diag.span_suggestion(
-                            span,
-                            "use",
-                            format!("{start}..={end}"),
-                            Applicability::MachineApplicable, // snippet
-                        );
-                    },
-                    None => {},
-                }
-            },
-        );
-    }
+fn check_exclusive_range_plus_one<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+    check_range_switch(
+        cx,
+        expr,
+        RangeLimits::HalfOpen,
+        y_plus_one,
+        RANGE_PLUS_ONE,
+        "an inclusive range would be more readable",
+        "..=",
+    );
 }
 
 // inclusive range minus one: `x..=(y-1)`
-fn check_inclusive_range_minus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
+fn check_inclusive_range_minus_one<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+    check_range_switch(
+        cx,
+        expr,
+        RangeLimits::Closed,
+        y_minus_one,
+        RANGE_MINUS_ONE,
+        "an exclusive range would be more readable",
+        "..",
+    );
+}
+
+/// Check for a `kind` of range in `expr`, check for `predicate` on the end,
+/// and emit the `lint` with `msg` and the `operator`.
+fn check_range_switch<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    kind: RangeLimits,
+    predicate: impl for<'lt> FnOnce(&LateContext<'lt>, &'lt Expr<'lt>) -> Option<&'lt Expr<'lt>>,
+    lint: &'static Lint,
+    msg: &'static str,
+    operator: &str,
+) {
     if expr.span.can_be_used_for_suggestions()
         && let Some(higher::Range {
             start,
             end: Some(end),
-            limits: RangeLimits::Closed,
+            limits,
         }) = higher::Range::hir(expr)
-        && let Some(y) = y_minus_one(cx, end)
+        && limits == kind
+        && let Some(y) = predicate(cx, end)
         && can_switch_ranges(cx, expr)
     {
-        span_lint_and_then(
-            cx,
-            RANGE_MINUS_ONE,
-            expr.span,
-            "an exclusive range would be more readable",
-            |diag| {
-                let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").maybe_paren().to_string());
-                let end = Sugg::hir(cx, y, "y").maybe_paren();
-                diag.span_suggestion(
-                    expr.span,
-                    "use",
-                    format!("{start}..{end}"),
-                    Applicability::MachineApplicable, // snippet
-                );
-            },
-        );
+        let span = expr.span;
+        span_lint_and_then(cx, lint, span, msg, |diag| {
+            let mut app = Applicability::MachineApplicable;
+            let start = start.map_or(String::new(), |x| {
+                Sugg::hir_with_applicability(cx, x, "<x>", &mut app)
+                    .maybe_paren()
+                    .to_string()
+            });
+            let end = Sugg::hir_with_applicability(cx, y, "<y>", &mut app).maybe_paren();
+            match span.with_source_text(cx, |src| src.starts_with('(') && src.ends_with(')')) {
+                Some(true) => {
+                    diag.span_suggestion(span, "use", format!("({start}{operator}{end})"), app);
+                },
+                Some(false) => {
+                    diag.span_suggestion(span, "use", format!("{start}{operator}{end}"), app);
+                },
+                None => {},
+            }
+        });
     }
 }
 

Diff for: tests/ui/range_plus_minus_one.fixed

@@ -1,3 +1,4 @@
+#![warn(clippy::range_minus_one, clippy::range_plus_one)]
 #![allow(unused_parens)]
 #![allow(clippy::iter_with_drain)]
 fn f() -> usize {
@@ -20,8 +21,6 @@ macro_rules! macro_minus_one {
     };
 }
 
-#[warn(clippy::range_plus_one)]
-#[warn(clippy::range_minus_one)]
 fn main() {
     for _ in 0..2 {}
     for _ in 0..=2 {}
@@ -99,3 +98,24 @@ fn main() {
 
 fn take_arg<T: Iterator<Item = u32>>(_: T) {}
 fn take_args<T: Iterator<Item = u32>>(_: T, _: T) {}
+
+fn issue9908() {
+    // Simplified test case
+    let _ = || 0..=1;
+
+    // Original test case
+    let full_length = 1024;
+    let range = {
+        // do some stuff, omit here
+        None
+    };
+
+    let range = range.map(|(s, t)| s..=t).unwrap_or(0..=(full_length - 1));
+
+    assert_eq!(range, 0..=1023);
+}
+
+fn issue9908_2(n: usize) -> usize {
+    (1..n).sum()
+    //~^ range_minus_one
+}

Diff for: tests/ui/range_plus_minus_one.rs

@@ -1,3 +1,4 @@
+#![warn(clippy::range_minus_one, clippy::range_plus_one)]
 #![allow(unused_parens)]
 #![allow(clippy::iter_with_drain)]
 fn f() -> usize {
@@ -20,8 +21,6 @@ macro_rules! macro_minus_one {
     };
 }
 
-#[warn(clippy::range_plus_one)]
-#[warn(clippy::range_minus_one)]
 fn main() {
     for _ in 0..2 {}
     for _ in 0..=2 {}
@@ -99,3 +98,24 @@ fn main() {
 
 fn take_arg<T: Iterator<Item = u32>>(_: T) {}
 fn take_args<T: Iterator<Item = u32>>(_: T, _: T) {}
+
+fn issue9908() {
+    // Simplified test case
+    let _ = || 0..=1;
+
+    // Original test case
+    let full_length = 1024;
+    let range = {
+        // do some stuff, omit here
+        None
+    };
+
+    let range = range.map(|(s, t)| s..=t).unwrap_or(0..=(full_length - 1));
+
+    assert_eq!(range, 0..=1023);
+}
+
+fn issue9908_2(n: usize) -> usize {
+    (1..=n - 1).sum()
+    //~^ range_minus_one
+}

Diff for: tests/ui/range_plus_minus_one.stderr

@@ -1,5 +1,5 @@
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:29:14
+  --> tests/ui/range_plus_minus_one.rs:28:14
    |
 LL |     for _ in 0..3 + 1 {}
    |              ^^^^^^^^ help: use: `0..=3`
@@ -8,64 +8,73 @@ LL |     for _ in 0..3 + 1 {}
    = help: to override `-D warnings` add `#[allow(clippy::range_plus_one)]`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:33:14
+  --> tests/ui/range_plus_minus_one.rs:32:14
    |
 LL |     for _ in 0..1 + 5 {}
    |              ^^^^^^^^ help: use: `0..=5`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:37:14
+  --> tests/ui/range_plus_minus_one.rs:36:14
    |
 LL |     for _ in 1..1 + 1 {}
    |              ^^^^^^^^ help: use: `1..=1`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:44:14
+  --> tests/ui/range_plus_minus_one.rs:43:14
    |
 LL |     for _ in 0..(1 + f()) {}
    |              ^^^^^^^^^^^^ help: use: `0..=f()`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:58:14
+  --> tests/ui/range_plus_minus_one.rs:57:14
    |
 LL |     for _ in 1..ONE + ONE {}
    |              ^^^^^^^^^^^^ help: use: `1..=ONE`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:68:5
+  --> tests/ui/range_plus_minus_one.rs:67:5
    |
 LL |     (1..10 + 1).for_each(|_| {});
    |     ^^^^^^^^^^^ help: use: `(1..=10)`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:73:5
+  --> tests/ui/range_plus_minus_one.rs:72:5
    |
 LL |     (1..10 + 1).into_iter().for_each(|_| {});
    |     ^^^^^^^^^^^ help: use: `(1..=10)`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:79:17
+  --> tests/ui/range_plus_minus_one.rs:78:17
    |
 LL |         let _ = (1..10 + 1).start_bound();
    |                 ^^^^^^^^^^^ help: use: `(1..=10)`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:85:16
+  --> tests/ui/range_plus_minus_one.rs:84:16
    |
 LL |     let _ = &a[1..1 + 1];
    |                ^^^^^^^^ help: use: `1..=1`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:89:15
+  --> tests/ui/range_plus_minus_one.rs:88:15
    |
 LL |     vec.drain(2..3 + 1);
    |               ^^^^^^^^ help: use: `2..=3`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:93:14
+  --> tests/ui/range_plus_minus_one.rs:92:14
    |
 LL |     take_arg(10..20 + 1);
    |              ^^^^^^^^^^ help: use: `10..=20`
 
-error: aborting due to 11 previous errors
+error: an exclusive range would be more readable
+  --> tests/ui/range_plus_minus_one.rs:119:5
+   |
+LL |     (1..=n - 1).sum()
+   |     ^^^^^^^^^^^ help: use: `(1..n)`
+   |
+   = note: `-D clippy::range-minus-one` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::range_minus_one)]`
+
+error: aborting due to 12 previous errors