FIX: NegMultiply should preserve parenthesis when method is called (rust-lang#15179)

Hi,

I noticed that the lint
[neg_multiply](https://rust-lang.github.io/rust-clippy/master/index.html#neg_multiply)
generates bad code when we call a method on the expression.
Consider `((a.delta - 0.5).abs() * -1.0).total_cmp(&1.0)`. Currently
this would be changed by clippy to `-(a.delta - 0.5).abs()
.total_cmp(&1.0)` - which does not compile because we are trying to
negate an ordering enum - but what we really want is `(-(a.delta -
0.5).abs()).total_cmp(&1.0)`.

This PR fixes this.

changelog: [`neg_multiply`] does not remove parenthesis anymore if a
method is being called on the affected expression

NOTE: This is the first time I am contributing to clippy or the rust
repo in general. So I am not sure whether my approach to fixing this
issue is goo, if there are better solutions or if I missed something.

Thanks & hope you have a good day,
Dario

Author: llogiq

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,22 +33,35 @@ 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
             && BinOpKind::Mul == op.node
         {
             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,16 +76,27 @@ 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}")
         };
         span_lint_and_sugg(
             cx,
             NEG_MULTIPLY,
-            span,
+            mul_expr.span,
             "this multiplication by -1 can be written more succinctly",
             "consider using",
             suggestion,

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);
+}

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);
+}

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