Fix range_minus_one and range_plus_one clippy lints

Author: estebank

src/tools/clippy/clippy_lints/src/ranges.rs

@@ -3,7 +3,6 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_the
 use clippy_utils::higher;
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::{snippet, snippet_opt, snippet_with_applicability};
-use clippy_utils::sugg::Sugg;
 use clippy_utils::{get_parent_expr, in_constant, is_integer_const, path_to_local};
 use if_chain::if_chain;
 use rustc_ast::ast::RangeLimits;
@@ -356,6 +355,8 @@ fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
             end: Some(end),
             limits: RangeLimits::HalfOpen
         }) = higher::Range::hir(expr);
+        if start.map_or(true, |start| start.span.can_be_used_for_suggestions());
+        if end.span.can_be_used_for_suggestions();
         if let Some(y) = y_plus_one(cx, end);
         then {
             let span = expr.span;
@@ -365,25 +366,19 @@ fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
                 span,
                 "an inclusive range would be more readable",
                 |diag| {
-                    let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").maybe_par().to_string());
-                    let end = Sugg::hir(cx, y, "y").maybe_par();
-                    if let Some(is_wrapped) = &snippet_opt(cx, span) {
-                        if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') {
-                            diag.span_suggestion(
-                                span,
-                                "use",
-                                format!("({start}..={end})"),
-                                Applicability::MaybeIncorrect,
-                            );
-                        } else {
-                            diag.span_suggestion(
-                                span,
-                                "use",
-                                format!("{start}..={end}"),
-                                Applicability::MachineApplicable, // snippet
-                            );
-                        }
-                    }
+                    diag.multipart_suggestion(
+                        "use an inclusive range instead",
+                        vec![
+                            (
+                                start.map(|s| s.span.shrink_to_hi())
+                                    .unwrap_or(expr.span)
+                                    .until(end.span),
+                                "..=".to_string(),
+                            ),
+                            (y, String::new()),
+                        ],
+                        Applicability::MachineApplicable,
+                    );
                 },
             );
         }
@@ -392,9 +387,12 @@ fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
 
 // inclusive range minus one: `x..=(y-1)`
 fn check_inclusive_range_minus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
+
     if_chain! {
         if expr.span.can_be_used_for_suggestions();
         if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::Range::hir(expr);
+        if start.map_or(true, |start| start.span.can_be_used_for_suggestions());
+        if end.span.can_be_used_for_suggestions();
         if let Some(y) = y_minus_one(cx, end);
         then {
             span_lint_and_then(
@@ -403,13 +401,18 @@ fn check_inclusive_range_minus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
                 expr.span,
                 "an exclusive range would be more readable",
                 |diag| {
-                    let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").maybe_par().to_string());
-                    let end = Sugg::hir(cx, y, "y").maybe_par();
-                    diag.span_suggestion(
-                        expr.span,
-                        "use",
-                        format!("{start}..{end}"),
-                        Applicability::MachineApplicable, // snippet
+                    diag.multipart_suggestion(
+                        "use an exclusive range instead",
+                        vec![
+                            (
+                                start.map(|s| s.span.shrink_to_hi())
+                                    .unwrap_or(expr.span)
+                                    .until(end.span),
+                                "..".to_string(),
+                            ),
+                            (y, String::new()),
+                        ],
+                        Applicability::MachineApplicable,
                     );
                 },
             );
@@ -497,19 +500,24 @@ 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<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<Span> {
     match expr.kind {
         ExprKind::Binary(
             Spanned {
-                node: BinOpKind::Add, ..
+                node: BinOpKind::Add,
+                span,
             },
             lhs,
             rhs,
         ) => {
-            if is_integer_const(cx, lhs, 1) {
-                Some(rhs)
-            } else if is_integer_const(cx, rhs, 1) {
-                Some(lhs)
+            if is_integer_const(cx, lhs, 1)
+                && lhs.span.peel_ctxt().ctxt() == span.peel_ctxt().ctxt()
+            {
+                Some(lhs.span.to(span))
+            } else if is_integer_const(cx, rhs, 1)
+                && rhs.span.peel_ctxt().ctxt() == span.peel_ctxt().ctxt()
+            {
+                Some(span.to(rhs.span))
             } else {
                 None
             }
@@ -518,15 +526,18 @@ 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<'t>(cx: &LateContext<'_>, expr: &'t Expr<'_>) -> Option<Span> {
     match expr.kind {
         ExprKind::Binary(
             Spanned {
-                node: BinOpKind::Sub, ..
+                node: BinOpKind::Sub,
+                span,
             },
-            lhs,
+            _,
             rhs,
-        ) if is_integer_const(cx, rhs, 1) => Some(lhs),
+        ) if is_integer_const(cx, rhs, 1)
+            && rhs.span.peel_ctxt().ctxt() == span.peel_ctxt().ctxt()
+            => Some(span.to(rhs.span)),
         _ => None,
     }
 }

src/tools/clippy/tests/ui/range_plus_minus_one.fixed

@@ -1,22 +1,22 @@
 // run-rustfix
 
 #![allow(unused_parens)]
-#![allow(clippy::iter_with_drain)]
+#![allow(clippy::iter_with_drain, clippy::needless_parens_on_range_literals)]
 fn f() -> usize {
     42
 }
 
 macro_rules! macro_plus_one {
     ($m: literal) => {
-        for i in 0..$m + 1 {
+        for i in 0..=$m  {
             println!("{}", i);
         }
     };
 }
 
 macro_rules! macro_minus_one {
     ($m: literal) => {
-        for i in 0..=$m - 1 {
+        for i in 0..$m  {
             println!("{}", i);
         }
     };
@@ -28,30 +28,30 @@ fn main() {
     for _ in 0..2 {}
     for _ in 0..=2 {}
 
-    for _ in 0..=3 {}
+    for _ in 0..=3  {}
     for _ in 0..=3 + 1 {}
 
-    for _ in 0..=5 {}
+    for _ in 0..= 5 {}
     for _ in 0..=1 + 5 {}
 
-    for _ in 1..=1 {}
+    for _ in 1..= 1 {}
     for _ in 1..=1 + 1 {}
 
     for _ in 0..13 + 13 {}
     for _ in 0..=13 - 7 {}
 
-    for _ in 0..=f() {}
+    for _ in 0..=( f()) {}
     for _ in 0..=(1 + f()) {}
 
     let _ = ..11 - 1;
-    let _ = ..11;
-    let _ = ..11;
-    let _ = (1..=11);
-    let _ = ((f() + 1)..=f());
+    let _ = ..11 ;
+    let _ = ..(11 );
+    let _ = (1..=11 );
+    let _ = (f() + 1)..=(f() );
 
     const ONE: usize = 1;
     // integer consts are linted, too
-    for _ in 1..=ONE {}
+    for _ in 1..= ONE {}
 
     let mut vec: Vec<()> = std::vec::Vec::new();
     vec.drain(..);

src/tools/clippy/tests/ui/range_plus_minus_one.rs

@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![allow(unused_parens)]
-#![allow(clippy::iter_with_drain)]
+#![allow(clippy::iter_with_drain, clippy::needless_parens_on_range_literals)]
 fn f() -> usize {
     42
 }

src/tools/clippy/tests/ui/range_plus_minus_one.stderr

@@ -2,59 +2,143 @@ error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:31:14
    |
 LL |     for _ in 0..3 + 1 {}
-   |              ^^^^^^^^ help: use: `0..=3`
+   |              ^^^^^^^^
    |
    = note: `-D clippy::range-plus-one` implied by `-D warnings`
+help: use an inclusive range instead
+   |
+LL -     for _ in 0..3 + 1 {}
+LL +     for _ in 0..=3  {}
+   |
 
 error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:34:14
    |
 LL |     for _ in 0..1 + 5 {}
-   |              ^^^^^^^^ help: use: `0..=5`
+   |              ^^^^^^^^
+   |
+help: use an inclusive range instead
+   |
+LL -     for _ in 0..1 + 5 {}
+LL +     for _ in 0..= 5 {}
+   |
 
 error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:37:14
    |
 LL |     for _ in 1..1 + 1 {}
-   |              ^^^^^^^^ help: use: `1..=1`
+   |              ^^^^^^^^
+   |
+help: use an inclusive range instead
+   |
+LL -     for _ in 1..1 + 1 {}
+LL +     for _ in 1..= 1 {}
+   |
 
 error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:43:14
    |
 LL |     for _ in 0..(1 + f()) {}
-   |              ^^^^^^^^^^^^ help: use: `0..=f()`
+   |              ^^^^^^^^^^^^
+   |
+help: use an inclusive range instead
+   |
+LL -     for _ in 0..(1 + f()) {}
+LL +     for _ in 0..=( f()) {}
+   |
 
 error: an exclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:47:13
    |
 LL |     let _ = ..=11 - 1;
-   |             ^^^^^^^^^ help: use: `..11`
+   |             ^^^^^^^^^
    |
    = note: `-D clippy::range-minus-one` implied by `-D warnings`
+help: use an exclusive range instead
+   |
+LL -     let _ = ..=11 - 1;
+LL +     let _ = ..11 ;
+   |
 
 error: an exclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:48:13
    |
 LL |     let _ = ..=(11 - 1);
-   |             ^^^^^^^^^^^ help: use: `..11`
+   |             ^^^^^^^^^^^
+   |
+help: use an exclusive range instead
+   |
+LL -     let _ = ..=(11 - 1);
+LL +     let _ = ..(11 );
+   |
 
 error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:49:13
    |
 LL |     let _ = (1..11 + 1);
-   |             ^^^^^^^^^^^ help: use: `(1..=11)`
+   |             ^^^^^^^^^^^
+   |
+help: use an inclusive range instead
+   |
+LL -     let _ = (1..11 + 1);
+LL +     let _ = (1..=11 );
+   |
 
 error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:50:13
    |
 LL |     let _ = (f() + 1)..(f() + 1);
-   |             ^^^^^^^^^^^^^^^^^^^^ help: use: `((f() + 1)..=f())`
+   |             ^^^^^^^^^^^^^^^^^^^^
+   |
+help: use an inclusive range instead
+   |
+LL -     let _ = (f() + 1)..(f() + 1);
+LL +     let _ = (f() + 1)..=(f() );
+   |
 
 error: an inclusive range would be more readable
   --> $DIR/range_plus_minus_one.rs:54:14
    |
 LL |     for _ in 1..ONE + ONE {}
-   |              ^^^^^^^^^^^^ help: use: `1..=ONE`
+   |              ^^^^^^^^^^^^
+   |
+help: use an inclusive range instead
+   |
+LL -     for _ in 1..ONE + ONE {}
+LL +     for _ in 1..= ONE {}
+   |
+
+error: an inclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:11:18
+   |
+LL |         for i in 0..$m + 1 {
+   |                  ^^^^^^^^^
+...
+LL |     macro_plus_one!(5);
+   |     ------------------ in this macro invocation
+   |
+   = note: this error originates in the macro `macro_plus_one` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: use an inclusive range instead
+   |
+LL -         for i in 0..$m + 1 {
+LL +         for i in 0..=$m  {
+   |
+
+error: an exclusive range would be more readable
+  --> $DIR/range_plus_minus_one.rs:19:18
+   |
+LL |         for i in 0..=$m - 1 {
+   |                  ^^^^^^^^^^
+...
+LL |     macro_minus_one!(5);
+   |     ------------------- in this macro invocation
+   |
+   = note: this error originates in the macro `macro_minus_one` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: use an exclusive range instead
+   |
+LL -         for i in 0..=$m - 1 {
+LL +         for i in 0..$m  {
+   |
 
-error: aborting due to 9 previous errors
+error: aborting due to 11 previous errors