From 679bc32f46c912bb91fa32c8d3977f6423a40f80 Mon Sep 17 00:00:00 2001 From: daubaris Date: Thu, 30 Aug 2018 20:06:13 +0300 Subject: [PATCH 1/4] range_plus_one suggestion should not remove braces fix --- clippy_lints/src/ranges.rs | 17 ++++++++++++++--- tests/ui/range_plus_minus_one.stderr | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index bef4532bbe53..d86f264addab 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -4,7 +4,7 @@ use if_chain::if_chain; use rustc::hir::*; use syntax::ast::RangeLimits; use syntax::source_map::Spanned; -use crate::utils::{is_integer_literal, paths, snippet, span_lint, span_lint_and_then}; +use crate::utils::{is_integer_literal, paths, snippet, span_lint, span_lint_and_then, snippet_opt}; use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq}; use crate::utils::sugg::Sugg; @@ -49,7 +49,10 @@ declare_clippy_lint! { /// **Why is this bad?** The code is more readable with an inclusive range /// like `x..=y`. /// -/// **Known problems:** None. +/// **Known problems:** Will add unnecessary pair of parentheses when the +/// expression is not wrapped in a pair but starts with a opening parenthesis +/// and ends with a closing one. +/// I.e: let _ = (f()+1)..(f()+1) results in let _ = ((f()+1)..(f()+1)). /// /// **Example:** /// ```rust @@ -145,9 +148,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { |db| { let start = start.map_or("".to_owned(), |x| Sugg::hir(cx, x, "x").to_string()); let end = Sugg::hir(cx, y, "y"); - db.span_suggestion(expr.span, + if let Some(is_wrapped) = &snippet_opt(cx, expr.span) { + if is_wrapped.starts_with("(") && is_wrapped.ends_with(")") { + db.span_suggestion(expr.span, + "use", + format!("({}..={})", start, end)); + } else { + db.span_suggestion(expr.span, "use", format!("{}..={}", start, end)); + } + } }, ); } diff --git a/tests/ui/range_plus_minus_one.stderr b/tests/ui/range_plus_minus_one.stderr index 1990300ef904..9b51176b7caa 100644 --- a/tests/ui/range_plus_minus_one.stderr +++ b/tests/ui/range_plus_minus_one.stderr @@ -42,7 +42,7 @@ error: an inclusive range would be more readable --> $DIR/range_plus_minus_one.rs:28:13 | 28 | let _ = (f()+1)..(f()+1); - | ^^^^^^^^^^^^^^^^ help: use: `(f()+1)..=f()` + | ^^^^^^^^^^^^^^^^ help: use: `((f()+1)..=f())` error: aborting due to 7 previous errors From b825578a4a1049f0a29205e35c61f0f47e3ae6dc Mon Sep 17 00:00:00 2001 From: daubaris Date: Mon, 3 Sep 2018 18:24:38 +0300 Subject: [PATCH 2/4] backticks and testcase --- clippy_lints/src/ranges.rs | 2 +- tests/ui/range_plus_minus_one.rs | 1 + tests/ui/range_plus_minus_one.stderr | 10 ++++++++-- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index c69a0943b17e..db2fbbad8763 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -52,7 +52,7 @@ declare_clippy_lint! { /// **Known problems:** Will add unnecessary pair of parentheses when the /// expression is not wrapped in a pair but starts with a opening parenthesis /// and ends with a closing one. -/// I.e: let _ = (f()+1)..(f()+1) results in let _ = ((f()+1)..(f()+1)). +/// I.e: `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..(f()+1))`. /// /// **Example:** /// ```rust diff --git a/tests/ui/range_plus_minus_one.rs b/tests/ui/range_plus_minus_one.rs index 12a1312de366..1ee3637f266d 100644 --- a/tests/ui/range_plus_minus_one.rs +++ b/tests/ui/range_plus_minus_one.rs @@ -27,6 +27,7 @@ fn main() { let _ = ..11-1; let _ = ..=11-1; let _ = ..=(11-1); + let _ = (1..11+1); let _ = (f()+1)..(f()+1); let mut vec: Vec<()> = std::vec::Vec::new(); diff --git a/tests/ui/range_plus_minus_one.stderr b/tests/ui/range_plus_minus_one.stderr index 1c0d8906caa9..3fe4e7ca073f 100644 --- a/tests/ui/range_plus_minus_one.stderr +++ b/tests/ui/range_plus_minus_one.stderr @@ -41,8 +41,14 @@ error: an exclusive range would be more readable error: an inclusive range would be more readable --> $DIR/range_plus_minus_one.rs:30:13 | -28 | let _ = (f()+1)..(f()+1); +30 | let _ = (1..11+1); + | ^^^^^^^^^ help: use: `(1..=11)` + +error: an inclusive range would be more readable + --> $DIR/range_plus_minus_one.rs:31:13 + | +31 | let _ = (f()+1)..(f()+1); | ^^^^^^^^^^^^^^^^ help: use: `((f()+1)..=f())` -error: aborting due to 7 previous errors +error: aborting due to 8 previous errors From 2f0a99a3a47988a8ef34fa3c76564179445bae8f Mon Sep 17 00:00:00 2001 From: daubaris Date: Mon, 3 Sep 2018 23:01:28 +0300 Subject: [PATCH 3/4] fixed known problems expression --- clippy_lints/src/ranges.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index db2fbbad8763..22fcdf0fc0f5 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -52,7 +52,7 @@ declare_clippy_lint! { /// **Known problems:** Will add unnecessary pair of parentheses when the /// expression is not wrapped in a pair but starts with a opening parenthesis /// and ends with a closing one. -/// I.e: `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..(f()+1))`. +/// I.e: `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..=f())`. /// /// **Example:** /// ```rust From 009c29069c1cf188e6d1935292c288c22d57cc04 Mon Sep 17 00:00:00 2001 From: daubaris Date: Tue, 4 Sep 2018 18:56:48 +0300 Subject: [PATCH 4/4] switched to ticks for chars --- clippy_lints/src/ranges.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index 22fcdf0fc0f5..f9cdffea79c5 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -149,7 +149,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { let start = start.map_or("".to_owned(), |x| Sugg::hir(cx, x, "x").to_string()); let end = Sugg::hir(cx, y, "y"); if let Some(is_wrapped) = &snippet_opt(cx, expr.span) { - if is_wrapped.starts_with("(") && is_wrapped.ends_with(")") { + if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') { db.span_suggestion(expr.span, "use", format!("({}..={})", start, end));