From 13ad14b22d77b2b9041c77cb70262db700fe1b02 Mon Sep 17 00:00:00 2001 From: hotate29 Date: Sat, 11 Dec 2021 02:32:23 +0900 Subject: [PATCH 1/6] update: ```Sugg::not()``` replacing the comparison operator. #7320 When inverting an expression, the output is now like ```foo != 0``` instead of ```!(foo == 0)```, the comparison operator is now replaced. --- clippy_utils/src/sugg.rs | 29 ++++++++++++++++++++++++- tests/ui/short_circuit_statement.fixed | 2 +- tests/ui/short_circuit_statement.stderr | 2 +- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index 586934df4603..cf46a75791c9 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -394,7 +394,34 @@ impl Neg for Sugg<'_> { impl Not for Sugg<'_> { type Output = Sugg<'static>; fn not(self) -> Sugg<'static> { - make_unop("!", self) + use AssocOp::{Equal, Greater, GreaterEqual, Less, LessEqual, NotEqual}; + + /// Convert ```AssocOp``` to a string of operators. + fn op_as_str(op: AssocOp) -> &'static str { + op.to_ast_binop().unwrap().to_string() + } + + /// Replace the operator in the Snippet. + fn replace_op(from_op: AssocOp, to_op: AssocOp, snip: Cow<'_, str>) -> Sugg<'static> { + let from = op_as_str(from_op); + let to = op_as_str(to_op); + let snip = snip.into_owned().replace(from, to); + Sugg::BinOp(to_op, Cow::Owned(snip)) + } + + if let Sugg::BinOp(op, snip) = self { + match op { + Equal => replace_op(op, NotEqual, snip), + NotEqual => replace_op(op, Equal, snip), + Less => replace_op(op, GreaterEqual, snip), + GreaterEqual => replace_op(op, Less, snip), + Greater => replace_op(op, LessEqual, snip), + LessEqual => replace_op(op, Greater, snip), + _ => make_unop("!", Sugg::BinOp(op, snip)), + } + } else { + make_unop("!", self) + } } } diff --git a/tests/ui/short_circuit_statement.fixed b/tests/ui/short_circuit_statement.fixed index af0a397bd1af..dd22ecab0b55 100644 --- a/tests/ui/short_circuit_statement.fixed +++ b/tests/ui/short_circuit_statement.fixed @@ -6,7 +6,7 @@ fn main() { if f() { g(); } if !f() { g(); } - if !(1 == 2) { g(); } + if 1 != 2 { g(); } } fn f() -> bool { diff --git a/tests/ui/short_circuit_statement.stderr b/tests/ui/short_circuit_statement.stderr index 0a3f60c3d132..aa84ac3a7925 100644 --- a/tests/ui/short_circuit_statement.stderr +++ b/tests/ui/short_circuit_statement.stderr @@ -16,7 +16,7 @@ error: boolean short circuit operator in statement may be clearer using an expli --> $DIR/short_circuit_statement.rs:9:5 | LL | 1 == 2 || g(); - | ^^^^^^^^^^^^^^ help: replace it with: `if !(1 == 2) { g(); }` + | ^^^^^^^^^^^^^^ help: replace it with: `if 1 != 2 { g(); }` error: aborting due to 3 previous errors From b3b65a1bf6790af0bbaf6dba2e6a688ecd6e9387 Mon Sep 17 00:00:00 2001 From: hotate29 Date: Tue, 14 Dec 2021 21:27:55 +0900 Subject: [PATCH 2/6] Add test --- clippy_utils/src/sugg.rs | 33 ++++++++++ tests/ui/needless_bool/fixable.fixed | 9 +++ tests/ui/needless_bool/fixable.rs | 33 ++++++++++ tests/ui/needless_bool/fixable.stderr | 86 +++++++++++++++++++++++---- 4 files changed, 148 insertions(+), 13 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index cf46a75791c9..8989872cbcee 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -1040,4 +1040,37 @@ mod test { let sugg = Sugg::BinOp(AssocOp::Add, "(1 + 1) + (1 + 1)".into()); assert_eq!("((1 + 1) + (1 + 1))", sugg.maybe_par().to_string()); } + #[test] + fn not_op() { + use AssocOp::{Add, Equal, Greater, GreaterEqual, LAnd, LOr, Less, LessEqual, NotEqual}; + + // Invert the comparison operator. + let sugg = Sugg::BinOp(Equal, "1 == 1".into()); + assert_eq!("1 != 1", (!sugg).to_string()); + + let sugg = Sugg::BinOp(NotEqual, "1 != 1".into()); + assert_eq!("1 == 1", (!sugg).to_string()); + + let sugg = Sugg::BinOp(Less, "1 < 1".into()); + assert_eq!("1 >= 1", (!sugg).to_string()); + + let sugg = Sugg::BinOp(LessEqual, "1 <= 1".into()); + assert_eq!("1 > 1", (!sugg).to_string()); + + let sugg = Sugg::BinOp(Greater, "1 > 1".into()); + assert_eq!("1 <= 1", (!sugg).to_string()); + + let sugg = Sugg::BinOp(GreaterEqual, "1 >= 1".into()); + assert_eq!("1 < 1", (!sugg).to_string()); + + // Other operators are inverted like !(..). + let sugg = Sugg::BinOp(Add, "1 + 1".into()); + assert_eq!("!(1 + 1)", (!sugg).to_string()); + + let sugg = Sugg::BinOp(LAnd, "1 && 1".into()); + assert_eq!("!(1 && 1)", (!sugg).to_string()); + + let sugg = Sugg::BinOp(LOr, "1 || 1".into()); + assert_eq!("!(1 || 1)", (!sugg).to_string()); + } } diff --git a/tests/ui/needless_bool/fixable.fixed b/tests/ui/needless_bool/fixable.fixed index 85da1f4e1043..89dc13fd5b13 100644 --- a/tests/ui/needless_bool/fixable.fixed +++ b/tests/ui/needless_bool/fixable.fixed @@ -41,6 +41,15 @@ fn main() { x; !x; !(x && y); + let a = 0; + let b = 1; + + a != b; + a == b; + a >= b; + a > b; + a <= b; + a < b; if x { x } else { diff --git a/tests/ui/needless_bool/fixable.rs b/tests/ui/needless_bool/fixable.rs index add606302511..c11d9472e8d0 100644 --- a/tests/ui/needless_bool/fixable.rs +++ b/tests/ui/needless_bool/fixable.rs @@ -53,6 +53,39 @@ fn main() { } else { true }; + let a = 0; + let b = 1; + + if a == b { + false + } else { + true + }; + if a != b { + false + } else { + true + }; + if a < b { + false + } else { + true + }; + if a <= b { + false + } else { + true + }; + if a > b { + false + } else { + true + }; + if a >= b { + false + } else { + true + }; if x { x } else { diff --git a/tests/ui/needless_bool/fixable.stderr b/tests/ui/needless_bool/fixable.stderr index 22c0a7bb491c..d2c48376f766 100644 --- a/tests/ui/needless_bool/fixable.stderr +++ b/tests/ui/needless_bool/fixable.stderr @@ -31,7 +31,67 @@ LL | | }; | |_____^ help: you can reduce it to: `!(x && y)` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:72:5 + --> $DIR/fixable.rs:59:5 + | +LL | / if a == b { +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ help: you can reduce it to: `a != b` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:64:5 + | +LL | / if a != b { +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ help: you can reduce it to: `a == b` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:69:5 + | +LL | / if a < b { +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ help: you can reduce it to: `a >= b` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:74:5 + | +LL | / if a <= b { +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ help: you can reduce it to: `a > b` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:79:5 + | +LL | / if a > b { +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ help: you can reduce it to: `a <= b` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:84:5 + | +LL | / if a >= b { +LL | | false +LL | | } else { +LL | | true +LL | | }; + | |_____^ help: you can reduce it to: `a < b` + +error: this if-then-else expression returns a bool literal + --> $DIR/fixable.rs:105:5 | LL | / if x { LL | | return true; @@ -41,7 +101,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return x` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:80:5 + --> $DIR/fixable.rs:113:5 | LL | / if x { LL | | return false; @@ -51,7 +111,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return !x` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:88:5 + --> $DIR/fixable.rs:121:5 | LL | / if x && y { LL | | return true; @@ -61,7 +121,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return x && y` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:96:5 + --> $DIR/fixable.rs:129:5 | LL | / if x && y { LL | | return false; @@ -71,7 +131,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return !(x && y)` error: equality checks against true are unnecessary - --> $DIR/fixable.rs:104:8 + --> $DIR/fixable.rs:137:8 | LL | if x == true {}; | ^^^^^^^^^ help: try simplifying it as shown: `x` @@ -79,25 +139,25 @@ LL | if x == true {}; = note: `-D clippy::bool-comparison` implied by `-D warnings` error: equality checks against false can be replaced by a negation - --> $DIR/fixable.rs:108:8 + --> $DIR/fixable.rs:141:8 | LL | if x == false {}; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: equality checks against true are unnecessary - --> $DIR/fixable.rs:118:8 + --> $DIR/fixable.rs:151:8 | LL | if x == true {}; | ^^^^^^^^^ help: try simplifying it as shown: `x` error: equality checks against false can be replaced by a negation - --> $DIR/fixable.rs:119:8 + --> $DIR/fixable.rs:152:8 | LL | if x == false {}; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:128:12 + --> $DIR/fixable.rs:161:12 | LL | } else if returns_bool() { | ____________^ @@ -108,7 +168,7 @@ LL | | }; | |_____^ help: you can reduce it to: `{ !returns_bool() }` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:141:5 + --> $DIR/fixable.rs:174:5 | LL | / if unsafe { no(4) } & 1 != 0 { LL | | true @@ -118,16 +178,16 @@ LL | | }; | |_____^ help: you can reduce it to: `(unsafe { no(4) } & 1 != 0)` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:146:30 + --> $DIR/fixable.rs:179:30 | LL | let _brackets_unneeded = if unsafe { no(4) } & 1 != 0 { true } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `unsafe { no(4) } & 1 != 0` error: this if-then-else expression returns a bool literal - --> $DIR/fixable.rs:149:9 + --> $DIR/fixable.rs:182:9 | LL | if unsafe { no(4) } & 1 != 0 { true } else { false } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `(unsafe { no(4) } & 1 != 0)` -error: aborting due to 15 previous errors +error: aborting due to 21 previous errors From 0b6d1fdea210877c54e3334c610cf7ff209e11ae Mon Sep 17 00:00:00 2001 From: hotate29 Date: Wed, 15 Dec 2021 23:23:36 +0900 Subject: [PATCH 3/6] refactor ```Sugg::BinOp``` --- clippy_lints/src/loops/manual_memcpy.rs | 26 +- clippy_lints/src/needless_bool.rs | 12 +- clippy_utils/src/sugg.rs | 264 ++++++++++-------- .../manual_memcpy/with_loop_counters.stderr | 2 +- 4 files changed, 166 insertions(+), 138 deletions(-) diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs index aa382f8a974b..c62fa5e998bd 100644 --- a/clippy_lints/src/loops/manual_memcpy.rs +++ b/clippy_lints/src/loops/manual_memcpy.rs @@ -12,6 +12,7 @@ use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Pat, PatKind, StmtKind} use rustc_lint::LateContext; use rustc_middle::ty::{self, Ty}; use rustc_span::symbol::sym; +use std::fmt::Display; use std::iter::Iterator; /// Checks for for loops that sequentially copy items from one slice-like @@ -108,7 +109,7 @@ fn build_manual_memcpy_suggestion<'tcx>( src: &IndexExpr<'_>, ) -> String { fn print_offset(offset: MinifyingSugg<'static>) -> MinifyingSugg<'static> { - if offset.as_str() == "0" { + if offset.to_string() == "0" { sugg::EMPTY.into() } else { offset @@ -123,7 +124,7 @@ fn build_manual_memcpy_suggestion<'tcx>( if let Some(arg) = len_args.get(0); if path_to_local(arg) == path_to_local(base); then { - if sugg.as_str() == end_str { + if sugg.to_string() == end_str { sugg::EMPTY.into() } else { sugg @@ -147,7 +148,7 @@ fn build_manual_memcpy_suggestion<'tcx>( print_offset(apply_offset(&start_str, &idx_expr.idx_offset)).into_sugg(), print_limit( end, - end_str.as_str(), + end_str.to_string().as_str(), idx_expr.base, apply_offset(&end_str, &idx_expr.idx_offset), ) @@ -159,7 +160,7 @@ fn build_manual_memcpy_suggestion<'tcx>( print_offset(apply_offset(&counter_start, &idx_expr.idx_offset)).into_sugg(), print_limit( end, - end_str.as_str(), + end_str.to_string().as_str(), idx_expr.base, apply_offset(&end_str, &idx_expr.idx_offset) + &counter_start - &start_str, ) @@ -202,12 +203,13 @@ fn build_manual_memcpy_suggestion<'tcx>( #[derive(Clone)] struct MinifyingSugg<'a>(Sugg<'a>); -impl<'a> MinifyingSugg<'a> { - fn as_str(&self) -> &str { - let (Sugg::NonParen(s) | Sugg::MaybeParen(s) | Sugg::BinOp(_, s)) = &self.0; - s.as_ref() +impl Display for MinifyingSugg<'a> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) } +} +impl<'a> MinifyingSugg<'a> { fn into_sugg(self) -> Sugg<'a> { self.0 } @@ -222,7 +224,7 @@ impl<'a> From> for MinifyingSugg<'a> { impl std::ops::Add for &MinifyingSugg<'static> { type Output = MinifyingSugg<'static>; fn add(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> { - match (self.as_str(), rhs.as_str()) { + match (self.to_string().as_str(), rhs.to_string().as_str()) { ("0", _) => rhs.clone(), (_, "0") => self.clone(), (_, _) => (&self.0 + &rhs.0).into(), @@ -233,7 +235,7 @@ impl std::ops::Add for &MinifyingSugg<'static> { impl std::ops::Sub for &MinifyingSugg<'static> { type Output = MinifyingSugg<'static>; fn sub(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> { - match (self.as_str(), rhs.as_str()) { + match (self.to_string().as_str(), rhs.to_string().as_str()) { (_, "0") => self.clone(), ("0", _) => (-rhs.0.clone()).into(), (x, y) if x == y => sugg::ZERO.into(), @@ -245,7 +247,7 @@ impl std::ops::Sub for &MinifyingSugg<'static> { impl std::ops::Add<&MinifyingSugg<'static>> for MinifyingSugg<'static> { type Output = MinifyingSugg<'static>; fn add(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> { - match (self.as_str(), rhs.as_str()) { + match (self.to_string().as_str(), rhs.to_string().as_str()) { ("0", _) => rhs.clone(), (_, "0") => self, (_, _) => (self.0 + &rhs.0).into(), @@ -256,7 +258,7 @@ impl std::ops::Add<&MinifyingSugg<'static>> for MinifyingSugg<'static> { impl std::ops::Sub<&MinifyingSugg<'static>> for MinifyingSugg<'static> { type Output = MinifyingSugg<'static>; fn sub(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> { - match (self.as_str(), rhs.as_str()) { + match (self.to_string().as_str(), rhs.to_string().as_str()) { (_, "0") => self, ("0", _) => (-rhs.0.clone()).into(), (x, y) if x == y => sugg::ZERO.into(), diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index d391fbecf82e..778d49cb4b6e 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -187,14 +187,14 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison { BinOpKind::Eq => { let true_case = Some((|h| h, "equality checks against true are unnecessary")); let false_case = Some(( - |h: Sugg<'_>| !h, + |h: Sugg<'tcx>| !h, "equality checks against false can be replaced by a negation", )); check_comparison(cx, e, true_case, false_case, true_case, false_case, ignore_no_literal); }, BinOpKind::Ne => { let true_case = Some(( - |h: Sugg<'_>| !h, + |h: Sugg<'tcx>| !h, "inequality checks against true can be replaced by a negation", )); let false_case = Some((|h| h, "inequality checks against false are unnecessary")); @@ -206,12 +206,12 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison { ignore_case, Some((|h| h, "greater than checks against false are unnecessary")), Some(( - |h: Sugg<'_>| !h, + |h: Sugg<'tcx>| !h, "less than comparison against true can be replaced by a negation", )), ignore_case, Some(( - |l: Sugg<'_>, r: Sugg<'_>| (!l).bit_and(&r), + |l: Sugg<'tcx>, r: Sugg<'tcx>| (!l).bit_and(&r), "order comparisons between booleans can be simplified", )), ), @@ -219,14 +219,14 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison { cx, e, Some(( - |h: Sugg<'_>| !h, + |h: Sugg<'tcx>| !h, "less than comparison against true can be replaced by a negation", )), ignore_case, ignore_case, Some((|h| h, "greater than checks against false are unnecessary")), Some(( - |l: Sugg<'_>, r: Sugg<'_>| l.bit_and(&(!r)), + |l: Sugg<'tcx>, r: Sugg<'tcx>| l.bit_and(&(!r)), "order comparisons between booleans can be simplified", )), ), diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index 8989872cbcee..0a240a8b7637 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -1,9 +1,7 @@ //! Contains utility functions to generate suggestions. #![deny(clippy::missing_docs_in_private_items)] -use crate::source::{ - snippet, snippet_opt, snippet_with_applicability, snippet_with_context, snippet_with_macro_callsite, -}; +use crate::source::{snippet, snippet_opt, snippet_with_applicability, snippet_with_macro_callsite}; use crate::{get_parent_expr_for_hir, higher}; use rustc_ast::util::parser::AssocOp; use rustc_ast::{ast, token}; @@ -33,7 +31,7 @@ pub enum Sugg<'a> { MaybeParen(Cow<'a, str>), /// A binary operator expression, including `as`-casts and explicit type /// coercion. - BinOp(AssocOp, Cow<'a, str>), + BinOp(AssocOp, Cow<'a, str>, Cow<'a, str>), } /// Literal constant `0`, for convenience. @@ -46,7 +44,8 @@ pub const EMPTY: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("")); impl Display for Sugg<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { match *self { - Sugg::NonParen(ref s) | Sugg::MaybeParen(ref s) | Sugg::BinOp(_, ref s) => s.fmt(f), + Sugg::NonParen(ref s) | Sugg::MaybeParen(ref s) => s.fmt(f), + Sugg::BinOp(op, ref lhs, ref rhs) => binop_to_string(op, lhs, rhs).fmt(f), } } } @@ -55,10 +54,8 @@ impl Display for Sugg<'_> { impl<'a> Sugg<'a> { /// Prepare a suggestion from an expression. pub fn hir_opt(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option { - snippet_opt(cx, expr.span).map(|snippet| { - let snippet = Cow::Owned(snippet); - Self::hir_from_snippet(expr, snippet) - }) + let get_snippet = |span| snippet(cx, span, ""); + snippet_opt(cx, expr.span).map(|_| Self::hir_from_snippet(expr, get_snippet)) } /// Convenience function around `hir_opt` for suggestions with a default @@ -93,9 +90,8 @@ impl<'a> Sugg<'a> { /// Same as `hir`, but will use the pre expansion span if the `expr` was in a macro. pub fn hir_with_macro_callsite(cx: &LateContext<'_>, expr: &hir::Expr<'_>, default: &'a str) -> Self { - let snippet = snippet_with_macro_callsite(cx, expr.span, default); - - Self::hir_from_snippet(expr, snippet) + let get_snippet = |span| snippet_with_macro_callsite(cx, span, default); + Self::hir_from_snippet(expr, get_snippet) } /// Same as `hir`, but first walks the span up to the given context. This will result in the @@ -112,24 +108,26 @@ impl<'a> Sugg<'a> { default: &'a str, applicability: &mut Applicability, ) -> Self { - let (snippet, in_macro) = snippet_with_context(cx, expr.span, ctxt, default, applicability); - - if in_macro { - Sugg::NonParen(snippet) + if expr.span.ctxt() == ctxt { + Self::hir_from_snippet(expr, |span| snippet(cx, span, default)) } else { - Self::hir_from_snippet(expr, snippet) + let snip = snippet_with_applicability(cx, expr.span, default, applicability); + Sugg::NonParen(snip) } } /// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*` /// function variants of `Sugg`, since these use different snippet functions. - fn hir_from_snippet(expr: &hir::Expr<'_>, snippet: Cow<'a, str>) -> Self { + fn hir_from_snippet(expr: &hir::Expr<'_>, get_snippet: impl Fn(Span) -> Cow<'a, str>) -> Self { if let Some(range) = higher::Range::hir(expr) { let op = match range.limits { ast::RangeLimits::HalfOpen => AssocOp::DotDot, ast::RangeLimits::Closed => AssocOp::DotDotEq, }; - return Sugg::BinOp(op, snippet); + let start = range.start.map_or("".into(), |expr| get_snippet(expr.span)); + let end = range.end.map_or("".into(), |expr| get_snippet(expr.span)); + + return Sugg::BinOp(op, start, end); } match expr.kind { @@ -139,7 +137,7 @@ impl<'a> Sugg<'a> { | hir::ExprKind::Let(..) | hir::ExprKind::Closure(..) | hir::ExprKind::Unary(..) - | hir::ExprKind::Match(..) => Sugg::MaybeParen(snippet), + | hir::ExprKind::Match(..) => Sugg::MaybeParen(get_snippet(expr.span)), hir::ExprKind::Continue(..) | hir::ExprKind::Yield(..) | hir::ExprKind::Array(..) @@ -160,12 +158,20 @@ impl<'a> Sugg<'a> { | hir::ExprKind::Struct(..) | hir::ExprKind::Tup(..) | hir::ExprKind::DropTemps(_) - | hir::ExprKind::Err => Sugg::NonParen(snippet), - hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet), - hir::ExprKind::AssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet), - hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(op.node.into()), snippet), - hir::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet), - hir::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet), + | hir::ExprKind::Err => Sugg::NonParen(get_snippet(expr.span)), + hir::ExprKind::Assign(lhs, rhs, _) => { + Sugg::BinOp(AssocOp::Assign, get_snippet(lhs.span), get_snippet(rhs.span)) + }, + hir::ExprKind::AssignOp(op, lhs, rhs) => { + Sugg::BinOp(hirbinop2assignop(op), get_snippet(lhs.span), get_snippet(rhs.span)) + }, + hir::ExprKind::Binary(op, lhs, rhs) => Sugg::BinOp( + AssocOp::from_ast_binop(op.node.into()), + get_snippet(lhs.span), + get_snippet(rhs.span), + ), + hir::ExprKind::Cast(lhs, ty) => Sugg::BinOp(AssocOp::As, get_snippet(lhs.span), get_snippet(ty.span)), + hir::ExprKind::Type(lhs, ty) => Sugg::BinOp(AssocOp::Colon, get_snippet(lhs.span), get_snippet(ty.span)), } } @@ -173,10 +179,12 @@ impl<'a> Sugg<'a> { pub fn ast(cx: &EarlyContext<'_>, expr: &ast::Expr, default: &'a str) -> Self { use rustc_ast::ast::RangeLimits; - let snippet = if expr.span.from_expansion() { - snippet_with_macro_callsite(cx, expr.span, default) - } else { - snippet(cx, expr.span, default) + let get_whole_snippet = || { + if expr.span.from_expansion() { + snippet_with_macro_callsite(cx, expr.span, default) + } else { + snippet(cx, expr.span, default) + } }; match expr.kind { @@ -186,7 +194,7 @@ impl<'a> Sugg<'a> { | ast::ExprKind::If(..) | ast::ExprKind::Let(..) | ast::ExprKind::Unary(..) - | ast::ExprKind::Match(..) => Sugg::MaybeParen(snippet), + | ast::ExprKind::Match(..) => Sugg::MaybeParen(get_whole_snippet()), ast::ExprKind::Async(..) | ast::ExprKind::Block(..) | ast::ExprKind::Break(..) @@ -215,14 +223,42 @@ impl<'a> Sugg<'a> { | ast::ExprKind::Array(..) | ast::ExprKind::While(..) | ast::ExprKind::Await(..) - | ast::ExprKind::Err => Sugg::NonParen(snippet), - ast::ExprKind::Range(.., RangeLimits::HalfOpen) => Sugg::BinOp(AssocOp::DotDot, snippet), - ast::ExprKind::Range(.., RangeLimits::Closed) => Sugg::BinOp(AssocOp::DotDotEq, snippet), - ast::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet), - ast::ExprKind::AssignOp(op, ..) => Sugg::BinOp(astbinop2assignop(op), snippet), - ast::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(op.node), snippet), - ast::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet), - ast::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet), + | ast::ExprKind::Err => Sugg::NonParen(get_whole_snippet()), + ast::ExprKind::Range(ref lhs, ref rhs, RangeLimits::HalfOpen) => Sugg::BinOp( + AssocOp::DotDot, + lhs.as_ref().map_or("".into(), |lhs| snippet(cx, lhs.span, default)), + rhs.as_ref().map_or("".into(), |rhs| snippet(cx, rhs.span, default)), + ), + ast::ExprKind::Range(ref lhs, ref rhs, RangeLimits::Closed) => Sugg::BinOp( + AssocOp::DotDotEq, + lhs.as_ref().map_or("".into(), |lhs| snippet(cx, lhs.span, default)), + rhs.as_ref().map_or("".into(), |rhs| snippet(cx, rhs.span, default)), + ), + ast::ExprKind::Assign(ref lhs, ref rhs, _) => Sugg::BinOp( + AssocOp::Assign, + snippet(cx, lhs.span, default), + snippet(cx, rhs.span, default), + ), + ast::ExprKind::AssignOp(op, ref lhs, ref rhs) => Sugg::BinOp( + astbinop2assignop(op), + snippet(cx, lhs.span, default), + snippet(cx, rhs.span, default), + ), + ast::ExprKind::Binary(op, ref lhs, ref rhs) => Sugg::BinOp( + AssocOp::from_ast_binop(op.node), + snippet(cx, lhs.span, default), + snippet(cx, rhs.span, default), + ), + ast::ExprKind::Cast(ref lhs, ref ty) => Sugg::BinOp( + AssocOp::As, + snippet(cx, lhs.span, default), + snippet(cx, ty.span, default), + ), + ast::ExprKind::Type(ref lhs, ref ty) => Sugg::BinOp( + AssocOp::Colon, + snippet(cx, lhs.span, default), + snippet(cx, ty.span, default), + ), } } @@ -306,17 +342,51 @@ impl<'a> Sugg<'a> { Sugg::NonParen(format!("({})", sugg).into()) } }, - Sugg::BinOp(_, sugg) => { - if has_enclosing_paren(&sugg) { - Sugg::NonParen(sugg) - } else { - Sugg::NonParen(format!("({})", sugg).into()) - } + Sugg::BinOp(op, lhs, rhs) => { + let sugg = binop_to_string(op, &lhs, &rhs); + Sugg::NonParen(format!("({})", sugg).into()) }, } } } +/// Generates a string from the operator and both sides. +fn binop_to_string(op: AssocOp, lhs: &str, rhs: &str) -> String { + match op { + AssocOp::Add + | AssocOp::Subtract + | AssocOp::Multiply + | AssocOp::Divide + | AssocOp::Modulus + | AssocOp::LAnd + | AssocOp::LOr + | AssocOp::BitXor + | AssocOp::BitAnd + | AssocOp::BitOr + | AssocOp::ShiftLeft + | AssocOp::ShiftRight + | AssocOp::Equal + | AssocOp::Less + | AssocOp::LessEqual + | AssocOp::NotEqual + | AssocOp::Greater + | AssocOp::GreaterEqual => format!( + "{} {} {}", + lhs, + op.to_ast_binop().expect("Those are AST ops").to_string(), + rhs + ), + AssocOp::Assign => format!("{} = {}", lhs, rhs), + AssocOp::AssignOp(op) => { + format!("{} {}= {}", lhs, token_kind_to_string(&token::BinOp(op)), rhs) + }, + AssocOp::As => format!("{} as {}", lhs, rhs), + AssocOp::DotDot => format!("{}..{}", lhs, rhs), + AssocOp::DotDotEq => format!("{}..={}", lhs, rhs), + AssocOp::Colon => format!("{}: {}", lhs, rhs), + } +} + /// Return `true` if `sugg` is enclosed in parenthesis. fn has_enclosing_paren(sugg: impl AsRef) -> bool { let mut chars = sugg.as_ref().chars(); @@ -391,34 +461,22 @@ impl Neg for Sugg<'_> { } } -impl Not for Sugg<'_> { - type Output = Sugg<'static>; - fn not(self) -> Sugg<'static> { +impl Not for Sugg<'a> { + type Output = Sugg<'a>; + fn not(self) -> Sugg<'a> { use AssocOp::{Equal, Greater, GreaterEqual, Less, LessEqual, NotEqual}; - /// Convert ```AssocOp``` to a string of operators. - fn op_as_str(op: AssocOp) -> &'static str { - op.to_ast_binop().unwrap().to_string() - } - - /// Replace the operator in the Snippet. - fn replace_op(from_op: AssocOp, to_op: AssocOp, snip: Cow<'_, str>) -> Sugg<'static> { - let from = op_as_str(from_op); - let to = op_as_str(to_op); - let snip = snip.into_owned().replace(from, to); - Sugg::BinOp(to_op, Cow::Owned(snip)) - } - - if let Sugg::BinOp(op, snip) = self { - match op { - Equal => replace_op(op, NotEqual, snip), - NotEqual => replace_op(op, Equal, snip), - Less => replace_op(op, GreaterEqual, snip), - GreaterEqual => replace_op(op, Less, snip), - Greater => replace_op(op, LessEqual, snip), - LessEqual => replace_op(op, Greater, snip), - _ => make_unop("!", Sugg::BinOp(op, snip)), - } + if let Sugg::BinOp(op, lhs, rhs) = self { + let to_op = match op { + Equal => NotEqual, + NotEqual => Equal, + Less => GreaterEqual, + GreaterEqual => Less, + Greater => LessEqual, + LessEqual => Greater, + _ => return make_unop("!", Sugg::BinOp(op, lhs, rhs)), + }; + Sugg::BinOp(to_op, lhs, rhs) } else { make_unop("!", self) } @@ -490,53 +548,21 @@ pub fn make_assoc(op: AssocOp, lhs: &Sugg<'_>, rhs: &Sugg<'_>) -> Sugg<'static> || is_shift(other) && is_arith(op) } - let lhs_paren = if let Sugg::BinOp(lop, _) = *lhs { + let lhs_paren = if let Sugg::BinOp(lop, _, _) = *lhs { needs_paren(op, lop, Associativity::Left) } else { false }; - let rhs_paren = if let Sugg::BinOp(rop, _) = *rhs { + let rhs_paren = if let Sugg::BinOp(rop, _, _) = *rhs { needs_paren(op, rop, Associativity::Right) } else { false }; - let lhs = ParenHelper::new(lhs_paren, lhs); - let rhs = ParenHelper::new(rhs_paren, rhs); - let sugg = match op { - AssocOp::Add - | AssocOp::BitAnd - | AssocOp::BitOr - | AssocOp::BitXor - | AssocOp::Divide - | AssocOp::Equal - | AssocOp::Greater - | AssocOp::GreaterEqual - | AssocOp::LAnd - | AssocOp::LOr - | AssocOp::Less - | AssocOp::LessEqual - | AssocOp::Modulus - | AssocOp::Multiply - | AssocOp::NotEqual - | AssocOp::ShiftLeft - | AssocOp::ShiftRight - | AssocOp::Subtract => format!( - "{} {} {}", - lhs, - op.to_ast_binop().expect("Those are AST ops").to_string(), - rhs - ), - AssocOp::Assign => format!("{} = {}", lhs, rhs), - AssocOp::AssignOp(op) => format!("{} {}= {}", lhs, token_kind_to_string(&token::BinOp(op)), rhs), - AssocOp::As => format!("{} as {}", lhs, rhs), - AssocOp::DotDot => format!("{}..{}", lhs, rhs), - AssocOp::DotDotEq => format!("{}..={}", lhs, rhs), - AssocOp::Colon => format!("{}: {}", lhs, rhs), - }; - - Sugg::BinOp(op, sugg.into()) + let lhs = ParenHelper::new(lhs_paren, lhs).to_string(); + let rhs = ParenHelper::new(rhs_paren, rhs).to_string(); + Sugg::BinOp(op, lhs.into(), rhs.into()) } /// Convenience wrapper around `make_assoc` and `AssocOp::from_ast_binop`. @@ -1034,10 +1060,10 @@ mod test { #[test] fn binop_maybe_par() { - let sugg = Sugg::BinOp(AssocOp::Add, "(1 + 1)".into()); + let sugg = Sugg::BinOp(AssocOp::Add, "1".into(), "1".into()); assert_eq!("(1 + 1)", sugg.maybe_par().to_string()); - let sugg = Sugg::BinOp(AssocOp::Add, "(1 + 1) + (1 + 1)".into()); + let sugg = Sugg::BinOp(AssocOp::Add, "(1 + 1)".into(), "(1 + 1)".into()); assert_eq!("((1 + 1) + (1 + 1))", sugg.maybe_par().to_string()); } #[test] @@ -1045,32 +1071,32 @@ mod test { use AssocOp::{Add, Equal, Greater, GreaterEqual, LAnd, LOr, Less, LessEqual, NotEqual}; // Invert the comparison operator. - let sugg = Sugg::BinOp(Equal, "1 == 1".into()); + let sugg = Sugg::BinOp(Equal, "1".into(), "1".into()); assert_eq!("1 != 1", (!sugg).to_string()); - let sugg = Sugg::BinOp(NotEqual, "1 != 1".into()); + let sugg = Sugg::BinOp(NotEqual, "1".into(), "1".into()); assert_eq!("1 == 1", (!sugg).to_string()); - let sugg = Sugg::BinOp(Less, "1 < 1".into()); + let sugg = Sugg::BinOp(Less, "1".into(), "1".into()); assert_eq!("1 >= 1", (!sugg).to_string()); - let sugg = Sugg::BinOp(LessEqual, "1 <= 1".into()); + let sugg = Sugg::BinOp(LessEqual, "1".into(), "1".into()); assert_eq!("1 > 1", (!sugg).to_string()); - let sugg = Sugg::BinOp(Greater, "1 > 1".into()); + let sugg = Sugg::BinOp(Greater, "1".into(), "1".into()); assert_eq!("1 <= 1", (!sugg).to_string()); - let sugg = Sugg::BinOp(GreaterEqual, "1 >= 1".into()); + let sugg = Sugg::BinOp(GreaterEqual, "1".into(), "1".into()); assert_eq!("1 < 1", (!sugg).to_string()); // Other operators are inverted like !(..). - let sugg = Sugg::BinOp(Add, "1 + 1".into()); + let sugg = Sugg::BinOp(Add, "1".into(), "1".into()); assert_eq!("!(1 + 1)", (!sugg).to_string()); - let sugg = Sugg::BinOp(LAnd, "1 && 1".into()); + let sugg = Sugg::BinOp(LAnd, "1".into(), "1".into()); assert_eq!("!(1 && 1)", (!sugg).to_string()); - let sugg = Sugg::BinOp(LOr, "1 || 1".into()); + let sugg = Sugg::BinOp(LOr, "1".into(), "1".into()); assert_eq!("!(1 || 1)", (!sugg).to_string()); } } diff --git a/tests/ui/manual_memcpy/with_loop_counters.stderr b/tests/ui/manual_memcpy/with_loop_counters.stderr index 0243158dec50..2e3ebadd7b5d 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 From 45a3b51c6d52ba6bb0c2e0b1e45c687b54419cb3 Mon Sep 17 00:00:00 2001 From: hotate29 Date: Tue, 21 Dec 2021 22:20:58 +0900 Subject: [PATCH 4/6] Use helper functions in ```Sugg``` tests. --- clippy_utils/src/sugg.rs | 39 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index 0a240a8b7637..92662c59226a 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -1070,33 +1070,22 @@ mod test { fn not_op() { use AssocOp::{Add, Equal, Greater, GreaterEqual, LAnd, LOr, Less, LessEqual, NotEqual}; - // Invert the comparison operator. - let sugg = Sugg::BinOp(Equal, "1".into(), "1".into()); - assert_eq!("1 != 1", (!sugg).to_string()); - - let sugg = Sugg::BinOp(NotEqual, "1".into(), "1".into()); - assert_eq!("1 == 1", (!sugg).to_string()); - - let sugg = Sugg::BinOp(Less, "1".into(), "1".into()); - assert_eq!("1 >= 1", (!sugg).to_string()); - - let sugg = Sugg::BinOp(LessEqual, "1".into(), "1".into()); - assert_eq!("1 > 1", (!sugg).to_string()); - - let sugg = Sugg::BinOp(Greater, "1".into(), "1".into()); - assert_eq!("1 <= 1", (!sugg).to_string()); + fn test_not(op: AssocOp, correct: &str) { + let sugg = Sugg::BinOp(op, "x".into(), "y".into()); + assert_eq!((!sugg).to_string(), correct); + } - let sugg = Sugg::BinOp(GreaterEqual, "1".into(), "1".into()); - assert_eq!("1 < 1", (!sugg).to_string()); + // Invert the comparison operator. + test_not(Equal, "x != y"); + test_not(NotEqual, "x == y"); + test_not(Less, "x >= y"); + test_not(LessEqual, "x > y"); + test_not(Greater, "x <= y"); + test_not(GreaterEqual, "x < y"); // Other operators are inverted like !(..). - let sugg = Sugg::BinOp(Add, "1".into(), "1".into()); - assert_eq!("!(1 + 1)", (!sugg).to_string()); - - let sugg = Sugg::BinOp(LAnd, "1".into(), "1".into()); - assert_eq!("!(1 && 1)", (!sugg).to_string()); - - let sugg = Sugg::BinOp(LOr, "1".into(), "1".into()); - assert_eq!("!(1 || 1)", (!sugg).to_string()); + test_not(Add, "!(x + y)"); + test_not(LAnd, "!(x && y)"); + test_not(LOr, "!(x || y)"); } } From 07b6927180915ed6a8379c8cc9bec069d13ff0dc Mon Sep 17 00:00:00 2001 From: hotate29 Date: Fri, 24 Dec 2021 23:54:15 +0900 Subject: [PATCH 5/6] Change ```floating_point_arthmetic::detect_hypot()``` to enclose the expression in parentheses. --- clippy_lints/src/floating_point_arithmetic.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/floating_point_arithmetic.rs b/clippy_lints/src/floating_point_arithmetic.rs index 2de2bfc040b5..06cd78582bbf 100644 --- a/clippy_lints/src/floating_point_arithmetic.rs +++ b/clippy_lints/src/floating_point_arithmetic.rs @@ -356,7 +356,7 @@ fn detect_hypot(cx: &LateContext<'_>, args: &[Expr<'_>]) -> Option { if eq_expr_value(cx, lmul_lhs, lmul_rhs); if eq_expr_value(cx, rmul_lhs, rmul_rhs); then { - return Some(format!("{}.hypot({})", Sugg::hir(cx, lmul_lhs, ".."), Sugg::hir(cx, rmul_lhs, ".."))); + return Some(format!("{}.hypot({})", Sugg::hir(cx, lmul_lhs, "..").maybe_par(), Sugg::hir(cx, rmul_lhs, ".."))); } } @@ -379,7 +379,7 @@ fn detect_hypot(cx: &LateContext<'_>, args: &[Expr<'_>]) -> Option { if let Some((rvalue, _)) = constant(cx, cx.typeck_results(), rargs_1); if Int(2) == lvalue && Int(2) == rvalue; then { - return Some(format!("{}.hypot({})", Sugg::hir(cx, largs_0, ".."), Sugg::hir(cx, rargs_0, ".."))); + return Some(format!("{}.hypot({})", Sugg::hir(cx, largs_0, "..").maybe_par(), Sugg::hir(cx, rargs_0, ".."))); } } } From a172439f29301d7c34645406f439f064edc58d69 Mon Sep 17 00:00:00 2001 From: hotate29 Date: Tue, 21 Dec 2021 21:45:55 +0900 Subject: [PATCH 6/6] Change to enclose both sides of Range in parentheses. --- clippy_lints/src/ranges.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index 09c64485bbb2..a1e67c33abf1 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -378,8 +378,8 @@ 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").to_string()); - let end = Sugg::hir(cx, y, "y"); + 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( @@ -415,8 +415,8 @@ 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").to_string()); - let end = Sugg::hir(cx, y, "y"); + 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",