From 6325fe1f5481f6dde2cabda173575a334a3cb8a2 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Thu, 1 Apr 2021 10:39:44 +0900 Subject: [PATCH] clippy_utils: fix needless parenthesis output from sugg::Sugg::maybe_par --- clippy_utils/src/sugg.rs | 42 ++++++++++++++++++- tests/ui/floating_point_log.fixed | 2 +- tests/ui/floating_point_log.stderr | 2 +- tests/ui/from_str_radix_10.stderr | 2 +- .../manual_memcpy/with_loop_counters.stderr | 2 +- 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index b2fe4317154..0633a19391f 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -267,17 +267,44 @@ impl<'a> Sugg<'a> { Sugg::NonParen(..) => self, // `(x)` and `(x).y()` both don't need additional parens. Sugg::MaybeParen(sugg) => { - if sugg.starts_with('(') && sugg.ends_with(')') { + if has_enclosing_paren(&sugg) { Sugg::MaybeParen(sugg) } else { Sugg::NonParen(format!("({})", sugg).into()) } }, - Sugg::BinOp(_, sugg) => Sugg::NonParen(format!("({})", sugg).into()), + Sugg::BinOp(_, sugg) => { + if has_enclosing_paren(&sugg) { + Sugg::NonParen(sugg) + } else { + Sugg::NonParen(format!("({})", sugg).into()) + } + }, } } } +/// Return `true` if `sugg` is enclosed in parenthesis. +fn has_enclosing_paren(sugg: impl AsRef) -> bool { + let mut chars = sugg.as_ref().chars(); + if let Some('(') = chars.next() { + let mut depth = 1; + while let Some(c) = chars.next() { + if c == '(' { + depth += 1; + } else if c == ')' { + depth -= 1; + } + if depth == 0 { + break; + } + } + chars.next().is_none() + } else { + false + } +} + // Copied from the rust standart library, and then edited macro_rules! forward_binop_impls_to_ref { (impl $imp:ident, $method:ident for $t:ty, type Output = $o:ty) => { @@ -668,6 +695,8 @@ impl DiagnosticBuilderExt for rustc_errors::DiagnosticBuilder #[cfg(test)] mod test { use super::Sugg; + + use rustc_ast::util::parser::AssocOp; use std::borrow::Cow; const SUGGESTION: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("function_call()")); @@ -681,4 +710,13 @@ mod test { fn blockify_transforms_sugg_into_a_block() { assert_eq!("{ function_call() }", SUGGESTION.blockify().to_string()); } + + #[test] + fn binop_maybe_par() { + let sugg = Sugg::BinOp(AssocOp::Add, "(1 + 1)".into()); + assert_eq!("(1 + 1)", sugg.maybe_par().to_string()); + + let sugg = Sugg::BinOp(AssocOp::Add, "(1 + 1) + (1 + 1)".into()); + assert_eq!("((1 + 1) + (1 + 1))", sugg.maybe_par().to_string()); + } } diff --git a/tests/ui/floating_point_log.fixed b/tests/ui/floating_point_log.fixed index 7dc7ee94aff..5b487bb8fcf 100644 --- a/tests/ui/floating_point_log.fixed +++ b/tests/ui/floating_point_log.fixed @@ -27,7 +27,7 @@ fn check_ln1p() { let _ = (x / 2.0).ln_1p(); let _ = x.powi(3).ln_1p(); let _ = (x.powi(3) / 2.0).ln_1p(); - let _ = ((std::f32::consts::E - 1.0)).ln_1p(); + let _ = (std::f32::consts::E - 1.0).ln_1p(); let _ = x.ln_1p(); let _ = x.powi(3).ln_1p(); let _ = (x + 2.0).ln_1p(); diff --git a/tests/ui/floating_point_log.stderr b/tests/ui/floating_point_log.stderr index 900dc2b7933..96e5a154441 100644 --- a/tests/ui/floating_point_log.stderr +++ b/tests/ui/floating_point_log.stderr @@ -90,7 +90,7 @@ error: ln(1 + x) can be computed more accurately --> $DIR/floating_point_log.rs:30:13 | LL | let _ = (1.0 + (std::f32::consts::E - 1.0)).ln(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `((std::f32::consts::E - 1.0)).ln_1p()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(std::f32::consts::E - 1.0).ln_1p()` error: ln(1 + x) can be computed more accurately --> $DIR/floating_point_log.rs:31:13 diff --git a/tests/ui/from_str_radix_10.stderr b/tests/ui/from_str_radix_10.stderr index 471bf52a9a7..da5c16f8d01 100644 --- a/tests/ui/from_str_radix_10.stderr +++ b/tests/ui/from_str_radix_10.stderr @@ -28,7 +28,7 @@ error: this call to `from_str_radix` can be replaced with a call to `str::parse` --> $DIR/from_str_radix_10.rs:32:5 | LL | u16::from_str_radix(&("10".to_owned() + "5"), 10)?; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(("10".to_owned() + "5")).parse::()` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `("10".to_owned() + "5").parse::()` error: this call to `from_str_radix` can be replaced with a call to `str::parse` --> $DIR/from_str_radix_10.rs:33:5 diff --git a/tests/ui/manual_memcpy/with_loop_counters.stderr b/tests/ui/manual_memcpy/with_loop_counters.stderr index 2547b19f5d1..a2f2dfce168 100644 --- a/tests/ui/manual_memcpy/with_loop_counters.stderr +++ b/tests/ui/manual_memcpy/with_loop_counters.stderr @@ -43,7 +43,7 @@ LL | / for i in 3..(3 + src.len()) { LL | | dst[i] = src[count]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[3..((3 + src.len()))].clone_from_slice(&src[..((3 + src.len()) - 3)]);` + | |_____^ help: try replacing the loop by: `dst[3..(3 + src.len())].clone_from_slice(&src[..((3 + src.len()) - 3)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:35:5