double_parens: add structured suggestions, fix bug (#15420)

The issue that I saw made me rethink the lint design by quite a lot, so
I decided to include that change in this PR

fixes #9000

changelog: [`double_parens`]: add structured suggestions
changelog: [`double_parens`]: fix FP when macros are involved

Author: Jarcho

clippy_lints/src/double_parens.rs

@@ -1,5 +1,7 @@
-use clippy_utils::diagnostics::span_lint;
-use rustc_ast::ast::{Expr, ExprKind};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::{HasSession, snippet_with_applicability, snippet_with_context};
+use rustc_ast::ast::{Expr, ExprKind, MethodCall};
+use rustc_errors::Applicability;
 use rustc_lint::{EarlyContext, EarlyLintPass};
 use rustc_session::declare_lint_pass;
 
@@ -24,7 +26,7 @@ declare_clippy_lint! {
     /// Use instead:
     /// ```no_run
     /// fn simple_no_parens() -> i32 {
-    ///     0
+    ///     (0)
     /// }
     ///
     /// # fn foo(bar: usize) {}
@@ -40,29 +42,54 @@ declare_lint_pass!(DoubleParens => [DOUBLE_PARENS]);
 
 impl EarlyLintPass for DoubleParens {
     fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
-        let span = match &expr.kind {
-            ExprKind::Paren(in_paren) if matches!(in_paren.kind, ExprKind::Paren(_) | ExprKind::Tup(_)) => expr.span,
-            ExprKind::Call(_, params)
-                if let [param] = &**params
-                    && let ExprKind::Paren(_) = param.kind =>
-            {
-                param.span
+        match &expr.kind {
+            // ((..))
+            // ^^^^^^ expr
+            //  ^^^^  inner
+            ExprKind::Paren(inner) if matches!(inner.kind, ExprKind::Paren(_) | ExprKind::Tup(_)) => {
+                // suggest removing the outer parens
+                if expr.span.eq_ctxt(inner.span) {
+                    let mut applicability = Applicability::MachineApplicable;
+                    // We don't need to use `snippet_with_context` here, because:
+                    // - if `inner`'s `ctxt` is from macro, we don't lint in the first place (see the check above)
+                    // - otherwise, calling `snippet_with_applicability` on a not-from-macro span is fine
+                    let sugg = snippet_with_applicability(cx.sess(), inner.span, "_", &mut applicability);
+                    span_lint_and_sugg(
+                        cx,
+                        DOUBLE_PARENS,
+                        expr.span,
+                        "unnecessary parentheses",
+                        "remove them",
+                        sugg.to_string(),
+                        applicability,
+                    );
+                }
             },
-            ExprKind::MethodCall(call)
-                if let [arg] = &*call.args
-                    && let ExprKind::Paren(_) = arg.kind =>
+
+            // func((n))
+            // ^^^^^^^^^ expr
+            //      ^^^  arg
+            //       ^   inner
+            ExprKind::Call(_, args) | ExprKind::MethodCall(box MethodCall { args, .. })
+                if let [arg] = &**args
+                    && let ExprKind::Paren(inner) = &arg.kind =>
             {
-                arg.span
+                // suggest removing the inner parens
+                if expr.span.eq_ctxt(arg.span) {
+                    let mut applicability = Applicability::MachineApplicable;
+                    let sugg = snippet_with_context(cx.sess(), inner.span, arg.span.ctxt(), "_", &mut applicability).0;
+                    span_lint_and_sugg(
+                        cx,
+                        DOUBLE_PARENS,
+                        arg.span,
+                        "unnecessary parentheses",
+                        "remove them",
+                        sugg.to_string(),
+                        applicability,
+                    );
+                }
             },
-            _ => return,
-        };
-        if !expr.span.from_expansion() {
-            span_lint(
-                cx,
-                DOUBLE_PARENS,
-                span,
-                "consider removing unnecessary double parentheses",
-            );
+            _ => {},
         }
     }
 }

tests/ui/double_parens.fixed

@@ -0,0 +1,99 @@
+#![warn(clippy::double_parens)]
+#![expect(clippy::eq_op, clippy::no_effect)]
+#![feature(custom_inner_attributes)]
+#![rustfmt::skip]
+
+fn dummy_fn<T>(_: T) {}
+
+struct DummyStruct;
+
+impl DummyStruct {
+    fn dummy_method<T>(&self, _: T) {}
+}
+
+fn simple_double_parens() -> i32 {
+    (0)
+    //~^ double_parens
+}
+
+fn fn_double_parens() {
+    dummy_fn(0);
+    //~^ double_parens
+}
+
+fn method_double_parens(x: DummyStruct) {
+    x.dummy_method(0);
+    //~^ double_parens
+}
+
+fn tuple_double_parens() -> (i32, i32) {
+    (1, 2)
+    //~^ double_parens
+}
+
+#[allow(clippy::unused_unit)]
+fn unit_double_parens() {
+    ()
+    //~^ double_parens
+}
+
+fn fn_tuple_ok() {
+    dummy_fn((1, 2));
+}
+
+fn method_tuple_ok(x: DummyStruct) {
+    x.dummy_method((1, 2));
+}
+
+fn fn_unit_ok() {
+    dummy_fn(());
+}
+
+fn method_unit_ok(x: DummyStruct) {
+    x.dummy_method(());
+}
+
+// Issue #3206
+fn inside_macro() {
+    assert_eq!((1, 2), (1, 2), "Error");
+    assert_eq!((1, 2), (1, 2), "Error");
+    //~^ double_parens
+}
+
+fn issue9000(x: DummyStruct) {
+    macro_rules! foo {
+        () => {(100)}
+    }
+    // don't lint: the inner paren comes from the macro expansion
+    (foo!());
+    dummy_fn(foo!());
+    x.dummy_method(foo!());
+
+    macro_rules! baz {
+        ($n:literal) => {($n)}
+    }
+    // don't lint: don't get confused by the expression inside the inner paren
+    // having the same `ctxt` as the overall expression
+    // (this is a bug that happened during the development of the fix)
+    (baz!(100));
+    dummy_fn(baz!(100));
+    x.dummy_method(baz!(100));
+
+    // should lint: both parens are from inside the macro
+    macro_rules! bar {
+        () => {(100)}
+        //~^ double_parens
+    }
+    bar!();
+
+    // should lint: both parens are from outside the macro;
+    // make sure to suggest the macro unexpanded
+    (vec![1, 2]);
+    //~^ double_parens
+    dummy_fn(vec![1, 2]);
+    //~^ double_parens
+    x.dummy_method(vec![1, 2]);
+    //~^ double_parens
+}
+
+fn main() {}

tests/ui/double_parens.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::double_parens)]
-#![allow(dead_code, clippy::eq_op)]
+#![expect(clippy::eq_op, clippy::no_effect)]
 #![feature(custom_inner_attributes)]
 #![rustfmt::skip]
 
@@ -8,38 +8,33 @@ fn dummy_fn<T>(_: T) {}
 struct DummyStruct;
 
 impl DummyStruct {
-    fn dummy_method<T>(self, _: T) {}
+    fn dummy_method<T>(&self, _: T) {}
 }
 
 fn simple_double_parens() -> i32 {
     ((0))
     //~^ double_parens
-
-
 }
 
 fn fn_double_parens() {
     dummy_fn((0));
     //~^ double_parens
-
 }
 
 fn method_double_parens(x: DummyStruct) {
     x.dummy_method((0));
     //~^ double_parens
-
 }
 
 fn tuple_double_parens() -> (i32, i32) {
     ((1, 2))
     //~^ double_parens
-
 }
 
+#[allow(clippy::unused_unit)]
 fn unit_double_parens() {
     (())
     //~^ double_parens
-
 }
 
 fn fn_tuple_ok() {
@@ -63,7 +58,42 @@ fn inside_macro() {
     assert_eq!((1, 2), (1, 2), "Error");
     assert_eq!(((1, 2)), (1, 2), "Error");
     //~^ double_parens
+}
 
+fn issue9000(x: DummyStruct) {
+    macro_rules! foo {
+        () => {(100)}
+    }
+    // don't lint: the inner paren comes from the macro expansion
+    (foo!());
+    dummy_fn(foo!());
+    x.dummy_method(foo!());
+
+    macro_rules! baz {
+        ($n:literal) => {($n)}
+    }
+    // don't lint: don't get confused by the expression inside the inner paren
+    // having the same `ctxt` as the overall expression
+    // (this is a bug that happened during the development of the fix)
+    (baz!(100));
+    dummy_fn(baz!(100));
+    x.dummy_method(baz!(100));
+
+    // should lint: both parens are from inside the macro
+    macro_rules! bar {
+        () => {((100))}
+        //~^ double_parens
+    }
+    bar!();
+
+    // should lint: both parens are from outside the macro;
+    // make sure to suggest the macro unexpanded
+    ((vec![1, 2]));
+    //~^ double_parens
+    dummy_fn((vec![1, 2]));
+    //~^ double_parens
+    x.dummy_method((vec![1, 2]));
+    //~^ double_parens
 }
 
 fn main() {}

tests/ui/double_parens.stderr

@@ -1,41 +1,70 @@
-error: consider removing unnecessary double parentheses
+error: unnecessary parentheses
   --> tests/ui/double_parens.rs:15:5
    |
 LL |     ((0))
-   |     ^^^^^
+   |     ^^^^^ help: remove them: `(0)`
    |
    = note: `-D clippy::double-parens` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::double_parens)]`
 
-error: consider removing unnecessary double parentheses
-  --> tests/ui/double_parens.rs:22:14
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:20:14
    |
 LL |     dummy_fn((0));
-   |              ^^^
+   |              ^^^ help: remove them: `0`
 
-error: consider removing unnecessary double parentheses
-  --> tests/ui/double_parens.rs:28:20
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:25:20
    |
 LL |     x.dummy_method((0));
-   |                    ^^^
+   |                    ^^^ help: remove them: `0`
 
-error: consider removing unnecessary double parentheses
-  --> tests/ui/double_parens.rs:34:5
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:30:5
    |
 LL |     ((1, 2))
-   |     ^^^^^^^^
+   |     ^^^^^^^^ help: remove them: `(1, 2)`
 
-error: consider removing unnecessary double parentheses
-  --> tests/ui/double_parens.rs:40:5
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:36:5
    |
 LL |     (())
-   |     ^^^^
+   |     ^^^^ help: remove them: `()`
 
-error: consider removing unnecessary double parentheses
-  --> tests/ui/double_parens.rs:64:16
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:59:16
    |
 LL |     assert_eq!(((1, 2)), (1, 2), "Error");
-   |                ^^^^^^^^
+   |                ^^^^^^^^ help: remove them: `(1, 2)`
 
-error: aborting due to 6 previous errors
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:84:16
+   |
+LL |         () => {((100))}
+   |                ^^^^^^^ help: remove them: `(100)`
+...
+LL |     bar!();
+   |     ------ in this macro invocation
+   |
+   = note: this error originates in the macro `bar` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:91:5
+   |
+LL |     ((vec![1, 2]));
+   |     ^^^^^^^^^^^^^^ help: remove them: `(vec![1, 2])`
+
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:93:14
+   |
+LL |     dummy_fn((vec![1, 2]));
+   |              ^^^^^^^^^^^^ help: remove them: `vec![1, 2]`
+
+error: unnecessary parentheses
+  --> tests/ui/double_parens.rs:95:20
+   |
+LL |     x.dummy_method((vec![1, 2]));
+   |                    ^^^^^^^^^^^^ help: remove them: `vec![1, 2]`
+
+error: aborting due to 10 previous errors