Refactor range_plus_one and range_minus_one lints

samueltardieu · samueltardieu · commit 204487797000 · 2025-05-23T13:08:12.000+02:00
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.
diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs
@@ -12,7 +12,7 @@ use rustc_ast::Mutability;
 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, GenericArgKind, PredicatePolarity, Ty};
 use rustc_session::impl_lint_pass;
 use rustc_span::source_map::Spanned;
@@ -460,70 +460,70 @@ fn can_switch_ranges<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, original: Ra
 
 // 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, RangeLimits::HalfOpen, cx.typeck_results().expr_ty(y))
-    {
-        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 => {},
-                }
-            },
-        );
-    }
+    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<'_>) {
+    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(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    kind: RangeLimits,
+    predicate: impl for<'tcx> FnOnce(&LateContext<'_>, &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>>,
+    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)
-        && can_switch_ranges(cx, expr, RangeLimits::Closed, cx.typeck_results().expr_ty(y))
+        && limits == kind
+        && let Some(y) = predicate(cx, end)
+        && can_switch_ranges(cx, expr, kind, cx.typeck_results().expr_ty(y))
     {
-        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 => {},
+            }
+        });
     }
 }
 
@@ -610,7 +610,7 @@ fn check_reversed_empty_range(cx: &LateContext<'_>, expr: &Expr<'_>) {
     }
 }
 
-fn y_plus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
+fn y_plus_one<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
     match expr.kind {
         ExprKind::Binary(
             Spanned {
@@ -631,7 +631,7 @@ fn y_plus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'
     }
 }
 
-fn y_minus_one<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> {
+fn y_minus_one<'tcx>(cx: &LateContext<'_>, expr: &Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
     match expr.kind {
         ExprKind::Binary(
             Spanned {
diff --git a/tests/ui/range_plus_minus_one.fixed b/tests/ui/range_plus_minus_one.fixed
@@ -1,5 +1,4 @@
-#![warn(clippy::range_minus_one)]
-#![warn(clippy::range_plus_one)]
+#![warn(clippy::range_minus_one, clippy::range_plus_one)]
 #![allow(unused_parens)]
 #![allow(clippy::iter_with_drain)]
 
@@ -153,3 +152,24 @@ fn no_index_mut_with_switched_range(a: usize) {
 
     S(2)[0..a + 1] = 3;
 }
+
+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 --git a/tests/ui/range_plus_minus_one.rs b/tests/ui/range_plus_minus_one.rs
@@ -1,5 +1,4 @@
-#![warn(clippy::range_minus_one)]
-#![warn(clippy::range_plus_one)]
+#![warn(clippy::range_minus_one, clippy::range_plus_one)]
 #![allow(unused_parens)]
 #![allow(clippy::iter_with_drain)]
 
@@ -153,3 +152,24 @@ fn no_index_mut_with_switched_range(a: usize) {
 
     S(2)[0..a + 1] = 3;
 }
+
+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 --git a/tests/ui/range_plus_minus_one.stderr b/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:32:14
+  --> tests/ui/range_plus_minus_one.rs:31:14
    |
 LL |     for _ in 0..3 + 1 {}
    |              ^^^^^^^^ help: use: `0..=3`
@@ -8,70 +8,79 @@ 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:36:14
+  --> tests/ui/range_plus_minus_one.rs:35: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:40:14
+  --> tests/ui/range_plus_minus_one.rs:39: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:47:14
+  --> tests/ui/range_plus_minus_one.rs:46: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:61:14
+  --> tests/ui/range_plus_minus_one.rs:60: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:71:5
+  --> tests/ui/range_plus_minus_one.rs:70: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:76:5
+  --> tests/ui/range_plus_minus_one.rs:75: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:81:17
+  --> tests/ui/range_plus_minus_one.rs:80: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:87:16
+  --> tests/ui/range_plus_minus_one.rs:86: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:91:15
+  --> tests/ui/range_plus_minus_one.rs:90: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:95:14
+  --> tests/ui/range_plus_minus_one.rs:94:14
    |
 LL |     take_arg(10..20 + 1);
    |              ^^^^^^^^^^ help: use: `10..=20`
 
 error: an inclusive range would be more readable
-  --> tests/ui/range_plus_minus_one.rs:106:7
+  --> tests/ui/range_plus_minus_one.rs:105:7
    |
 LL |     a[0..2 + 1][0] = 1;
    |       ^^^^^^^^ help: use: `0..=2`
 
-error: aborting due to 12 previous errors
+error: an exclusive range would be more readable
+  --> tests/ui/range_plus_minus_one.rs:173: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 13 previous errors
 
