diff --git a/clippy_lints/src/floating_point_arithmetic.rs b/clippy_lints/src/floating_point_arithmetic.rs index f801d95563d..4fefd9fd18a 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, ".."))); } } } diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs index aa382f8a974..c62fa5e998b 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 d391fbecf82..778d49cb4b6 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_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index 09c64485bbb..a1e67c33abf 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", diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index 586934df460..92662c59226 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,10 +461,25 @@ impl Neg for Sugg<'_> { } } -impl Not for Sugg<'_> { - type Output = Sugg<'static>; - fn not(self) -> Sugg<'static> { - make_unop("!", self) +impl Not for Sugg<'a> { + type Output = Sugg<'a>; + fn not(self) -> Sugg<'a> { + use AssocOp::{Equal, Greater, GreaterEqual, Less, LessEqual, NotEqual}; + + 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) + } } } @@ -463,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`. @@ -1007,10 +1060,32 @@ 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] + fn not_op() { + use AssocOp::{Add, Equal, Greater, GreaterEqual, LAnd, LOr, Less, LessEqual, NotEqual}; + + fn test_not(op: AssocOp, correct: &str) { + let sugg = Sugg::BinOp(op, "x".into(), "y".into()); + assert_eq!((!sugg).to_string(), correct); + } + + // 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 !(..). + test_not(Add, "!(x + y)"); + test_not(LAnd, "!(x && y)"); + test_not(LOr, "!(x || y)"); + } } diff --git a/tests/ui/manual_memcpy/with_loop_counters.stderr b/tests/ui/manual_memcpy/with_loop_counters.stderr index 0243158dec5..2e3ebadd7b5 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 diff --git a/tests/ui/needless_bool/fixable.fixed b/tests/ui/needless_bool/fixable.fixed index 85da1f4e104..89dc13fd5b1 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 add60630251..c11d9472e8d 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 22c0a7bb491..d2c48376f76 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 diff --git a/tests/ui/short_circuit_statement.fixed b/tests/ui/short_circuit_statement.fixed index af0a397bd1a..dd22ecab0b5 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 0a3f60c3d13..aa84ac3a792 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