Use diagnostic impls and add suggestions in redundant format!() args

Author: francorbacho

compiler/rustc_builtin_macros/messages.ftl

@@ -139,6 +139,17 @@ builtin_macros_format_positional_after_named = positional arguments cannot follo
 
 builtin_macros_format_remove_raw_ident = remove the `r#`
 
+builtin_macros_format_redundant_args = redundant argument
+    .help = {$n ->
+        [one] the formatting string already captures the binding directly, it doesn't need to be included in the argument list
+        *[more] the formatting strings already captures the bindings directly, they don't need to be included in the argument list
+    }
+    .note = {$n ->
+        [one] the formatting specifier is referencing the binding already
+        *[more] the formatting specifiers are referencing the bindings already
+    }
+    .suggestion = this can be removed
+
 builtin_macros_format_requires_string = requires at least a format string argument
 
 builtin_macros_format_string_invalid = invalid format string: {$desc}

compiler/rustc_builtin_macros/src/errors.rs

@@ -646,6 +646,27 @@ pub(crate) struct FormatPositionalMismatch {
     pub(crate) highlight: SingleLabelManySpans,
 }
 
+#[derive(Diagnostic)]
+#[diag(builtin_macros_format_redundant_args)]
+pub(crate) struct FormatRedundantArgs {
+    #[primary_span]
+    pub(crate) fmt_span: Span,
+    pub(crate) n: usize,
+
+    #[note]
+    pub(crate) note: MultiSpan,
+
+    #[subdiagnostic]
+    pub(crate) sugg: FormatRedundantArgsSugg,
+}
+
+#[derive(Subdiagnostic)]
+#[multipart_suggestion(builtin_macros_suggestion, applicability = "machine-applicable")]
+pub(crate) struct FormatRedundantArgsSugg {
+    #[suggestion_part(code = "")]
+    pub(crate) spans: Vec<Span>,
+}
+
 #[derive(Diagnostic)]
 #[diag(builtin_macros_test_case_non_item)]
 pub(crate) struct TestCaseNonItem {

compiler/rustc_builtin_macros/src/format.rs

@@ -7,12 +7,12 @@ use rustc_ast::{
     FormatArgsPiece, FormatArgument, FormatArgumentKind, FormatArguments, FormatCount,
     FormatDebugHex, FormatOptions, FormatPlaceholder, FormatSign, FormatTrait,
 };
-use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
-use rustc_errors::{Applicability, DiagnosticBuilder, MultiSpan, PResult, SingleLabelManySpans};
+use rustc_data_structures::fx::FxHashSet;
+use rustc_errors::{Applicability, MultiSpan, PResult, SingleLabelManySpans};
 use rustc_expand::base::{self, *};
 use rustc_parse_format as parse;
 use rustc_span::symbol::{Ident, Symbol};
-use rustc_span::{BytePos, ErrorGuaranteed, InnerSpan, Span};
+use rustc_span::{BytePos, InnerSpan, Span};
 
 use rustc_lint_defs::builtin::NAMED_ARGUMENTS_USED_POSITIONALLY;
 use rustc_lint_defs::{BufferedEarlyLint, BuiltinLintDiagnostics, LintId};
@@ -616,7 +616,9 @@ fn report_missing_placeholders(
         .collect::<Vec<_>>();
 
     if !placeholders.is_empty() {
-        report_redundant_placeholders(&mut diag, &args, used, placeholders);
+        report_redundant_placeholders(ecx, fmt_span, &args, used, placeholders);
+        diag.cancel();
+        return;
     }
 
     // Used to ensure we only report translations for *one* kind of foreign format.
@@ -707,13 +709,16 @@ fn report_missing_placeholders(
 }
 
 fn report_redundant_placeholders(
-    diag: &mut DiagnosticBuilder<'_, ErrorGuaranteed>,
+    ecx: &mut ExtCtxt<'_>,
+    fmt_span: Span,
     args: &FormatArguments,
     used: &[bool],
     placeholders: Vec<(Span, &str)>,
 ) {
+    let mut fmt_arg_indices = vec![];
     let mut args_spans = vec![];
-    let mut fmt_spans = FxIndexSet::default();
+    let mut fmt_spans = vec![];
+    let mut bindings = vec![];
 
     for (i, unnamed_arg) in args.unnamed_args().iter().enumerate().rev() {
         let Some(ty) = unnamed_arg.expr.to_ty() else { continue };
@@ -730,36 +735,39 @@ fn report_redundant_placeholders(
             .collect::<Vec<_>>();
 
         if !matching_placeholders.is_empty() {
+            fmt_arg_indices.push(i);
             args_spans.push(unnamed_arg.expr.span);
-            for placeholder in &matching_placeholders {
-                fmt_spans.insert(*placeholder);
+            for (span, binding) in &matching_placeholders {
+                if fmt_spans.contains(span) {
+                    continue;
+                }
+                fmt_spans.push(*span);
+                bindings.push(binding);
             }
         }
     }
 
     if !args_spans.is_empty() {
-        let mut multispan = MultiSpan::from(args_spans.clone());
+        let multispan = MultiSpan::from(fmt_spans);
+        let mut suggestion_spans = vec![];
 
-        let msg = if fmt_spans.len() > 1 {
-            "the formatting strings already captures the bindings \
-            directly, they don't need to be included in the argument list"
-        } else {
-            "the formatting string already captures the binding \
-            directly, it doesn't need to be included in the argument list"
-        };
+        for (arg_span, fmt_arg_idx) in args_spans.iter().zip(fmt_arg_indices.iter()) {
+            let span = if fmt_arg_idx + 1 == args.explicit_args().len() {
+                *arg_span
+            } else {
+                arg_span.until(args.explicit_args()[*fmt_arg_idx + 1].expr.span)
+            };
 
-        for (span, binding) in fmt_spans {
-            multispan.push_span_label(
-                *span,
-                format!("this formatting specifier is referencing the `{binding}` binding"),
-            );
+            suggestion_spans.push(span);
         }
 
-        for span in &args_spans {
-            multispan.push_span_label(*span, "this can be removed");
-        }
+        let mut diag = ecx.create_err(errors::FormatRedundantArgs {
+            fmt_span,
+            note: multispan,
+            n: args_spans.len(),
+            sugg: errors::FormatRedundantArgsSugg { spans: suggestion_spans },
+        });
 
-        diag.span_help(multispan, msg);
         diag.emit();
     }
 }

tests/ui/did_you_mean/issue-105225.rs

@@ -1,19 +1,19 @@
 fn main() {
     let x = 0;
     println!("{x}", x);
-    //~^ ERROR: argument never used
+    //~^ ERROR: redundant argument
 
     println!("{x} {}", x, x);
-    //~^ ERROR: argument never used
+    //~^ ERROR: redundant argument
 
     println!("{} {x}", x, x);
-    //~^ ERROR: argument never used
+    //~^ ERROR: redundant argument
 
     let y = 0;
     println!("{x} {y}", x, y);
-    //~^ ERROR: multiple unused formatting arguments
+    //~^ ERROR: redundant argument
 
     let y = 0;
     println!("{} {} {x} {y} {}", x, x, x, y, y);
-    //~^ ERROR: multiple unused formatting arguments
+    //~^ ERROR: redundant argument
 }

tests/ui/did_you_mean/issue-105225.stderr

@@ -1,81 +1,72 @@
-error: argument never used
-  --> $DIR/issue-105225.rs:3:21
+error: redundant argument
+  --> $DIR/issue-105225.rs:3:14
    |
 LL |     println!("{x}", x);
-   |                     ^ argument never used
+   |              ^^^^^  - help: this can be removed
    |
-help: the formatting string already captures the binding directly, it doesn't need to be included in the argument list
-  --> $DIR/issue-105225.rs:3:21
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225.rs:3:16
    |
 LL |     println!("{x}", x);
-   |                -    ^ this can be removed
-   |                |
-   |                this formatting specifier is referencing the `x` binding
+   |                ^
 
-error: argument never used
-  --> $DIR/issue-105225.rs:6:27
+error: redundant argument
+  --> $DIR/issue-105225.rs:6:14
    |
 LL |     println!("{x} {}", x, x);
-   |                           ^ argument never used
+   |              ^^^^^^^^     - help: this can be removed
    |
-help: the formatting string already captures the binding directly, it doesn't need to be included in the argument list
-  --> $DIR/issue-105225.rs:6:27
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225.rs:6:16
    |
 LL |     println!("{x} {}", x, x);
-   |                -          ^ this can be removed
-   |                |
-   |                this formatting specifier is referencing the `x` binding
+   |                ^
 
-error: argument never used
-  --> $DIR/issue-105225.rs:9:27
+error: redundant argument
+  --> $DIR/issue-105225.rs:9:14
    |
 LL |     println!("{} {x}", x, x);
-   |                           ^ argument never used
+   |              ^^^^^^^^     - help: this can be removed
    |
-help: the formatting string already captures the binding directly, it doesn't need to be included in the argument list
-  --> $DIR/issue-105225.rs:9:27
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225.rs:9:19
    |
 LL |     println!("{} {x}", x, x);
-   |                   -       ^ this can be removed
-   |                   |
-   |                   this formatting specifier is referencing the `x` binding
+   |                   ^
 
-error: multiple unused formatting arguments
-  --> $DIR/issue-105225.rs:13:25
+error: redundant argument
+  --> $DIR/issue-105225.rs:13:14
    |
 LL |     println!("{x} {y}", x, y);
-   |              ---------  ^  ^ argument never used
-   |              |          |
-   |              |          argument never used
-   |              multiple missing formatting specifiers
+   |              ^^^^^^^^^
    |
-help: the formatting strings already captures the bindings directly, they don't need to be included in the argument list
-  --> $DIR/issue-105225.rs:13:25
+note: the formatting specifiers are referencing the bindings already
+  --> $DIR/issue-105225.rs:13:16
    |
 LL |     println!("{x} {y}", x, y);
-   |                -   -    ^  ^ this can be removed
-   |                |   |    |
-   |                |   |    this can be removed
-   |                |   this formatting specifier is referencing the `y` binding
-   |                this formatting specifier is referencing the `x` binding
+   |                ^   ^
+help: this can be removed
+   |
+LL -     println!("{x} {y}", x, y);
+LL +     println!("{x} {y}", );
+   |
 
-error: multiple unused formatting arguments
-  --> $DIR/issue-105225.rs:17:43
+error: redundant argument
+  --> $DIR/issue-105225.rs:17:14
    |
 LL |     println!("{} {} {x} {y} {}", x, x, x, y, y);
-   |              ------------------           ^  ^ argument never used
-   |              |                            |
-   |              |                            argument never used
-   |              multiple missing formatting specifiers
+   |              ^^^^^^^^^^^^^^^^^^
    |
-help: the formatting string already captures the binding directly, it doesn't need to be included in the argument list
-  --> $DIR/issue-105225.rs:17:43
+note: the formatting specifiers are referencing the bindings already
+  --> $DIR/issue-105225.rs:17:26
    |
 LL |     println!("{} {} {x} {y} {}", x, x, x, y, y);
-   |                          -                ^  ^ this can be removed
-   |                          |                |
-   |                          |                this can be removed
-   |                          this formatting specifier is referencing the `y` binding
+   |                          ^
+help: this can be removed
+   |
+LL -     println!("{} {} {x} {y} {}", x, x, x, y, y);
+LL +     println!("{} {} {x} {y} {}", x, x, x, );
+   |
 
 error: aborting due to 5 previous errors