diff --git a/clippy_lints/src/neg_multiply.rs b/clippy_lints/src/neg_multiply.rs index 442280f99982..946114e10419 100644 --- a/clippy_lints/src/neg_multiply.rs +++ b/clippy_lints/src/neg_multiply.rs @@ -1,13 +1,13 @@ use clippy_utils::consts::{self, Constant}; use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_context; +use clippy_utils::get_parent_expr; +use clippy_utils::source::{snippet, snippet_with_context}; use clippy_utils::sugg::has_enclosing_paren; use rustc_ast::util::parser::ExprPrecedence; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; -use rustc_span::Span; declare_clippy_lint! { /// ### What it does @@ -33,6 +33,19 @@ declare_clippy_lint! { declare_lint_pass!(NegMultiply => [NEG_MULTIPLY]); +fn is_in_parens_with_postfix(cx: &LateContext<'_>, mul_expr: &Expr<'_>) -> bool { + if let Some(parent) = get_parent_expr(cx, mul_expr) { + let mult_snippet = snippet(cx, mul_expr.span, ""); + if has_enclosing_paren(&mult_snippet) + && let ExprKind::MethodCall(_, _, _, _) = parent.kind + { + return true; + } + } + + false +} + impl<'tcx> LateLintPass<'tcx> for NegMultiply { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { if let ExprKind::Binary(ref op, left, right) = e.kind @@ -40,15 +53,15 @@ impl<'tcx> LateLintPass<'tcx> for NegMultiply { { match (&left.kind, &right.kind) { (&ExprKind::Unary(..), &ExprKind::Unary(..)) => {}, - (&ExprKind::Unary(UnOp::Neg, lit), _) => check_mul(cx, e.span, lit, right), - (_, &ExprKind::Unary(UnOp::Neg, lit)) => check_mul(cx, e.span, lit, left), + (&ExprKind::Unary(UnOp::Neg, lit), _) => check_mul(cx, e, lit, right), + (_, &ExprKind::Unary(UnOp::Neg, lit)) => check_mul(cx, e, lit, left), _ => {}, } } } } -fn check_mul(cx: &LateContext<'_>, span: Span, lit: &Expr<'_>, exp: &Expr<'_>) { +fn check_mul(cx: &LateContext<'_>, mul_expr: &Expr<'_>, lit: &Expr<'_>, exp: &Expr<'_>) { const F16_ONE: u16 = 1.0_f16.to_bits(); const F128_ONE: u128 = 1.0_f128.to_bits(); if let ExprKind::Lit(l) = lit.kind @@ -63,8 +76,19 @@ fn check_mul(cx: &LateContext<'_>, span: Span, lit: &Expr<'_>, exp: &Expr<'_>) { && cx.typeck_results().expr_ty(exp).is_numeric() { let mut applicability = Applicability::MachineApplicable; - let (snip, from_macro) = snippet_with_context(cx, exp.span, span.ctxt(), "..", &mut applicability); - let suggestion = if !from_macro && cx.precedence(exp) < ExprPrecedence::Prefix && !has_enclosing_paren(&snip) { + let (snip, from_macro) = snippet_with_context(cx, exp.span, mul_expr.span.ctxt(), "..", &mut applicability); + + let needs_parens_for_postfix = is_in_parens_with_postfix(cx, mul_expr); + + let suggestion = if needs_parens_for_postfix { + // Special case: when the multiplication is in parentheses followed by a method call + // we need to preserve the grouping but negate the inner expression. + // Consider this expression: `((a.delta - 0.5).abs() * -1.0).total_cmp(&1.0)` + // We need to end up with: `(-(a.delta - 0.5).abs()).total_cmp(&1.0)` + // Otherwise, without the parentheses we would try to negate an Ordering: + // `-(a.delta - 0.5).abs().total_cmp(&1.0)` + format!("(-{snip})") + } else if !from_macro && cx.precedence(exp) < ExprPrecedence::Prefix && !has_enclosing_paren(&snip) { format!("-({snip})") } else { format!("-{snip}") @@ -72,7 +96,7 @@ fn check_mul(cx: &LateContext<'_>, span: Span, lit: &Expr<'_>, exp: &Expr<'_>) { span_lint_and_sugg( cx, NEG_MULTIPLY, - span, + mul_expr.span, "this multiplication by -1 can be written more succinctly", "consider using", suggestion, diff --git a/tests/ui/neg_multiply.fixed b/tests/ui/neg_multiply.fixed index ff6e08300e29..32d466e88fc8 100644 --- a/tests/ui/neg_multiply.fixed +++ b/tests/ui/neg_multiply.fixed @@ -82,3 +82,15 @@ fn float() { -1.0 * -1.0; // should be ok } + +struct Y { + delta: f64, +} + +fn nested() { + let a = Y { delta: 1.0 }; + let b = Y { delta: 1.0 }; + let _ = (-(a.delta - 0.5).abs()).total_cmp(&1.0); + //~^ neg_multiply + let _ = (-(a.delta - 0.5).abs()).total_cmp(&1.0); +} diff --git a/tests/ui/neg_multiply.rs b/tests/ui/neg_multiply.rs index b0f4e85c78e5..241a72c6d990 100644 --- a/tests/ui/neg_multiply.rs +++ b/tests/ui/neg_multiply.rs @@ -82,3 +82,15 @@ fn float() { -1.0 * -1.0; // should be ok } + +struct Y { + delta: f64, +} + +fn nested() { + let a = Y { delta: 1.0 }; + let b = Y { delta: 1.0 }; + let _ = ((a.delta - 0.5).abs() * -1.0).total_cmp(&1.0); + //~^ neg_multiply + let _ = (-(a.delta - 0.5).abs()).total_cmp(&1.0); +} diff --git a/tests/ui/neg_multiply.stderr b/tests/ui/neg_multiply.stderr index 2ef7e32ce05e..f4fb6d3ce541 100644 --- a/tests/ui/neg_multiply.stderr +++ b/tests/ui/neg_multiply.stderr @@ -97,5 +97,11 @@ error: this multiplication by -1 can be written more succinctly LL | (3.0_f32 as f64) * -1.0; | ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `-(3.0_f32 as f64)` -error: aborting due to 16 previous errors +error: this multiplication by -1 can be written more succinctly + --> tests/ui/neg_multiply.rs:93:13 + | +LL | let _ = ((a.delta - 0.5).abs() * -1.0).total_cmp(&1.0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `(-(a.delta - 0.5).abs())` + +error: aborting due to 17 previous errors