fix must_use_unit suggestion when there're multiple attributes (#13830

)

fix #12320

When there're multiple attributes, clippy suggests leaving an extra
comma and it makes an error.

changelog: [`must_use_unit`]: No longer make incorrect suggestions when
multiple attributes present.

clippy_lints/src/functions/must_use.rs

@@ -29,7 +29,7 @@ pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>
         let is_public = cx.effective_visibilities.is_exported(item.owner_id.def_id);
         let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
         if let Some(attr) = attr {
-            check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, sig);
+            check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, attrs, sig);
         } else if is_public && !is_proc_macro(attrs) && !attrs.iter().any(|a| a.has_name(sym::no_mangle)) {
             check_must_use_candidate(
                 cx,
@@ -51,7 +51,7 @@ pub(super) fn check_impl_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Imp
         let attrs = cx.tcx.hir().attrs(item.hir_id());
         let attr = cx.tcx.get_attr(item.owner_id, sym::must_use);
         if let Some(attr) = attr {
-            check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, sig);
+            check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, attrs, sig);
         } else if is_public && !is_proc_macro(attrs) && trait_ref_of_method(cx, item.owner_id.def_id).is_none() {
             check_must_use_candidate(
                 cx,
@@ -74,7 +74,7 @@ pub(super) fn check_trait_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Tr
         let attrs = cx.tcx.hir().attrs(item.hir_id());
         let attr = cx.tcx.get_attr(item.owner_id, sym::must_use);
         if let Some(attr) = attr {
-            check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, sig);
+            check_needless_must_use(cx, sig.decl, item.owner_id, item.span, fn_header_span, attr, attrs, sig);
         } else if let hir::TraitFn::Provided(eid) = *eid {
             let body = cx.tcx.hir().body(eid);
             if attr.is_none() && is_public && !is_proc_macro(attrs) {
@@ -92,28 +92,62 @@ pub(super) fn check_trait_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Tr
     }
 }
 
+#[allow(clippy::too_many_arguments)]
 fn check_needless_must_use(
     cx: &LateContext<'_>,
     decl: &hir::FnDecl<'_>,
     item_id: hir::OwnerId,
     item_span: Span,
     fn_header_span: Span,
     attr: &Attribute,
+    attrs: &[Attribute],
     sig: &FnSig<'_>,
 ) {
     if in_external_macro(cx.sess(), item_span) {
         return;
     }
     if returns_unit(decl) {
-        span_lint_and_then(
-            cx,
-            MUST_USE_UNIT,
-            fn_header_span,
-            "this unit-returning function has a `#[must_use]` attribute",
-            |diag| {
-                diag.span_suggestion(attr.span, "remove the attribute", "", Applicability::MachineApplicable);
-            },
-        );
+        if attrs.len() == 1 {
+            span_lint_and_then(
+                cx,
+                MUST_USE_UNIT,
+                fn_header_span,
+                "this unit-returning function has a `#[must_use]` attribute",
+                |diag| {
+                    diag.span_suggestion(attr.span, "remove the attribute", "", Applicability::MachineApplicable);
+                },
+            );
+        } else {
+            // When there are multiple attributes, it is not sufficient to simply make `must_use` empty, see
+            // issue #12320.
+            span_lint_and_then(
+                cx,
+                MUST_USE_UNIT,
+                fn_header_span,
+                "this unit-returning function has a `#[must_use]` attribute",
+                |diag| {
+                    let mut attrs_without_must_use = attrs.to_vec();
+                    attrs_without_must_use.retain(|a| a.id != attr.id);
+                    let sugg_str = attrs_without_must_use
+                        .iter()
+                        .map(|a| {
+                            if a.value_str().is_none() {
+                                return a.name_or_empty().to_string();
+                            }
+                            format!("{} = \"{}\"", a.name_or_empty(), a.value_str().unwrap())
+                        })
+                        .collect::<Vec<_>>()
+                        .join(", ");
+
+                    diag.span_suggestion(
+                        attrs[0].span.with_hi(attrs[attrs.len() - 1].span.hi()),
+                        "change these attributes to",
+                        sugg_str,
+                        Applicability::MachineApplicable,
+                    );
+                },
+            );
+        }
     } else if attr.value_str().is_none() && is_must_use_ty(cx, return_ty(cx, item_id)) {
         // Ignore async functions unless Future::Output type is a must_use type
         if sig.header.is_async() {

tests/ui/must_use_unit.fixed

@@ -23,3 +23,9 @@ fn main() {
         fn foo() {}
     );
 }
+
+#[cfg_attr(all(), deprecated)]
+fn issue_12320() {}
+
+#[cfg_attr(all(), deprecated, doc = "foo")]
+fn issue_12320_2() {}

tests/ui/must_use_unit.rs

@@ -26,3 +26,9 @@ fn main() {
         fn foo() {}
     );
 }
+
+#[cfg_attr(all(), must_use, deprecated)]
+fn issue_12320() {}
+
+#[cfg_attr(all(), deprecated, doc = "foo", must_use)]
+fn issue_12320_2() {}

tests/ui/must_use_unit.stderr

@@ -25,5 +25,21 @@ LL | #[must_use = "With note"]
 LL | pub fn must_use_with_note() {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 3 previous errors
+error: this unit-returning function has a `#[must_use]` attribute
+  --> tests/ui/must_use_unit.rs:31:1
+   |
+LL | #[cfg_attr(all(), must_use, deprecated)]
+   |                   -------------------- help: change these attributes to: `deprecated`
+LL | fn issue_12320() {}
+   | ^^^^^^^^^^^^^^^^
+
+error: this unit-returning function has a `#[must_use]` attribute
+  --> tests/ui/must_use_unit.rs:34:1
+   |
+LL | #[cfg_attr(all(), deprecated, doc = "foo", must_use)]
+   |                   --------------------------------- help: change these attributes to: `deprecated, doc = "foo"`
+LL | fn issue_12320_2() {}
+   | ^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 5 previous errors