Auto merge of #115324 - francorbacho:master, r=davidtwco

bors · bors · commit 111adde7ed86 · 2023-10-23T00:51:35.000Z
Suggest removing redundant arguments in format!() Closes #105225. This is also a follow-up to #105635, which seems to have become stale. r? `@estebank`
diff --git a/compiler/rustc_builtin_macros/messages.ftl b/compiler/rustc_builtin_macros/messages.ftl
@@ -137,6 +137,20 @@ builtin_macros_format_positional_after_named = positional arguments cannot follo
     .label = positional arguments must be before named arguments
     .named_args = named argument
 
+builtin_macros_format_redundant_args = redundant {$n ->
+    [one] argument
+    *[more] arguments
+    }
+    .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_remove_raw_ident = remove the `r#`
 
 builtin_macros_format_requires_string = requires at least a format string argument
diff --git a/compiler/rustc_builtin_macros/src/errors.rs b/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) span: MultiSpan,
+    pub(crate) n: usize,
+
+    #[note]
+    pub(crate) note: MultiSpan,
+
+    #[subdiagnostic]
+    pub(crate) sugg: Option<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 {
diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs
@@ -1,3 +1,4 @@
+use parse::Position::ArgumentNamed;
 use rustc_ast::ptr::P;
 use rustc_ast::tokenstream::TokenStream;
 use rustc_ast::{token, StmtKind};
@@ -7,7 +8,9 @@ use rustc_ast::{
     FormatDebugHex, FormatOptions, FormatPlaceholder, FormatSign, FormatTrait,
 };
 use rustc_data_structures::fx::FxHashSet;
-use rustc_errors::{Applicability, MultiSpan, PResult, SingleLabelManySpans};
+use rustc_errors::{
+    Applicability, DiagnosticBuilder, ErrorGuaranteed, MultiSpan, PResult, SingleLabelManySpans,
+};
 use rustc_expand::base::{self, *};
 use rustc_parse_format as parse;
 use rustc_span::symbol::{Ident, Symbol};
@@ -364,8 +367,8 @@ fn make_format_args(
     let mut unfinished_literal = String::new();
     let mut placeholder_index = 0;
 
-    for piece in pieces {
-        match piece {
+    for piece in &pieces {
+        match *piece {
             parse::Piece::String(s) => {
                 unfinished_literal.push_str(s);
             }
@@ -513,7 +516,17 @@ fn make_format_args(
         // If there's a lot of unused arguments,
         // let's check if this format arguments looks like another syntax (printf / shell).
         let detect_foreign_fmt = unused.len() > args.explicit_args().len() / 2;
-        report_missing_placeholders(ecx, unused, detect_foreign_fmt, str_style, fmt_str, fmt_span);
+        report_missing_placeholders(
+            ecx,
+            unused,
+            &used,
+            &args,
+            &pieces,
+            detect_foreign_fmt,
+            str_style,
+            fmt_str,
+            fmt_span,
+        );
     }
 
     // Only check for unused named argument names if there are no other errors to avoid causing
@@ -580,6 +593,9 @@ fn invalid_placeholder_type_error(
 fn report_missing_placeholders(
     ecx: &mut ExtCtxt<'_>,
     unused: Vec<(Span, bool)>,
+    used: &[bool],
+    args: &FormatArguments,
+    pieces: &[parse::Piece<'_>],
     detect_foreign_fmt: bool,
     str_style: Option<usize>,
     fmt_str: &str,
@@ -598,6 +614,26 @@ fn report_missing_placeholders(
         })
     };
 
+    let placeholders = pieces
+        .iter()
+        .filter_map(|piece| {
+            if let parse::Piece::NextArgument(argument) = piece && let ArgumentNamed(binding) = argument.position {
+                let span = fmt_span.from_inner(InnerSpan::new(argument.position_span.start, argument.position_span.end));
+                Some((span, binding))
+            } else { None }
+        })
+        .collect::<Vec<_>>();
+
+    if !placeholders.is_empty() {
+        if let Some(mut new_diag) =
+            report_redundant_format_arguments(ecx, &args, used, placeholders)
+        {
+            diag.cancel();
+            new_diag.emit();
+            return;
+        }
+    }
+
     // Used to ensure we only report translations for *one* kind of foreign format.
     let mut found_foreign = false;
 
@@ -685,6 +721,76 @@ fn report_missing_placeholders(
     diag.emit();
 }
 
+/// This function detects and reports unused format!() arguments that are
+/// redundant due to implicit captures (e.g. `format!("{x}", x)`).
+fn report_redundant_format_arguments<'a>(
+    ecx: &mut ExtCtxt<'a>,
+    args: &FormatArguments,
+    used: &[bool],
+    placeholders: Vec<(Span, &str)>,
+) -> Option<DiagnosticBuilder<'a, ErrorGuaranteed>> {
+    let mut fmt_arg_indices = vec![];
+    let mut args_spans = vec![];
+    let mut fmt_spans = vec![];
+
+    for (i, unnamed_arg) in args.unnamed_args().iter().enumerate().rev() {
+        let Some(ty) = unnamed_arg.expr.to_ty() else { continue };
+        let Some(argument_binding) = ty.kind.is_simple_path() else { continue };
+        let argument_binding = argument_binding.as_str();
+
+        if used[i] {
+            continue;
+        }
+
+        let matching_placeholders = placeholders
+            .iter()
+            .filter(|(_, inline_binding)| argument_binding == *inline_binding)
+            .map(|(span, _)| span)
+            .collect::<Vec<_>>();
+
+        if !matching_placeholders.is_empty() {
+            fmt_arg_indices.push(i);
+            args_spans.push(unnamed_arg.expr.span);
+            for span in &matching_placeholders {
+                if fmt_spans.contains(*span) {
+                    continue;
+                }
+                fmt_spans.push(**span);
+            }
+        }
+    }
+
+    if !args_spans.is_empty() {
+        let multispan = MultiSpan::from(fmt_spans);
+        let mut suggestion_spans = vec![];
+
+        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)
+            };
+
+            suggestion_spans.push(span);
+        }
+
+        let sugg = if args.named_args().len() == 0 {
+            Some(errors::FormatRedundantArgsSugg { spans: suggestion_spans })
+        } else {
+            None
+        };
+
+        return Some(ecx.create_err(errors::FormatRedundantArgs {
+            n: args_spans.len(),
+            span: MultiSpan::from(args_spans),
+            note: multispan,
+            sugg,
+        }));
+    }
+
+    None
+}
+
 /// Handle invalid references to positional arguments. Output different
 /// errors for the case where all arguments are positional and for when
 /// there are named arguments or numbered positional arguments in the
diff --git a/tests/ui/did_you_mean/issue-105225-named-args.rs b/tests/ui/did_you_mean/issue-105225-named-args.rs
@@ -0,0 +1,10 @@
+fn main() {
+    let x = "x";
+    let y = "y";
+
+    println!("{x}", x, x = y);
+    //~^ ERROR: redundant argument
+
+    println!("{x}", x = y, x = y);
+    //~^ ERROR: duplicate argument named `x`
+}
diff --git a/tests/ui/did_you_mean/issue-105225-named-args.stderr b/tests/ui/did_you_mean/issue-105225-named-args.stderr
@@ -0,0 +1,22 @@
+error: redundant argument
+  --> $DIR/issue-105225-named-args.rs:5:21
+   |
+LL |     println!("{x}", x, x = y);
+   |                     ^
+   |
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225-named-args.rs:5:16
+   |
+LL |     println!("{x}", x, x = y);
+   |                ^
+
+error: duplicate argument named `x`
+  --> $DIR/issue-105225-named-args.rs:8:28
+   |
+LL |     println!("{x}", x = y, x = y);
+   |                     -      ^ duplicate argument
+   |                     |
+   |                     previously here
+
+error: aborting due to 2 previous errors
+
diff --git a/tests/ui/did_you_mean/issue-105225.fixed b/tests/ui/did_you_mean/issue-105225.fixed
@@ -0,0 +1,21 @@
+// run-rustfix
+
+fn main() {
+    let x = "x";
+    let y = "y";
+
+    println!("{x}", );
+    //~^ ERROR: redundant argument
+
+    println!("{x} {}", x, );
+    //~^ ERROR: redundant argument
+
+    println!("{} {x}", x, );
+    //~^ ERROR: redundant argument
+
+    println!("{x} {y}", );
+    //~^ ERROR: redundant arguments
+
+    println!("{} {} {x} {y} {}", x, x, x, );
+    //~^ ERROR: redundant arguments
+}
diff --git a/tests/ui/did_you_mean/issue-105225.rs b/tests/ui/did_you_mean/issue-105225.rs
@@ -0,0 +1,21 @@
+// run-rustfix
+
+fn main() {
+    let x = "x";
+    let y = "y";
+
+    println!("{x}", x);
+    //~^ ERROR: redundant argument
+
+    println!("{x} {}", x, x);
+    //~^ ERROR: redundant argument
+
+    println!("{} {x}", x, x);
+    //~^ ERROR: redundant argument
+
+    println!("{x} {y}", x, y);
+    //~^ ERROR: redundant arguments
+
+    println!("{} {} {x} {y} {}", x, x, x, y, y);
+    //~^ ERROR: redundant arguments
+}
diff --git a/tests/ui/did_you_mean/issue-105225.stderr b/tests/ui/did_you_mean/issue-105225.stderr
@@ -0,0 +1,72 @@
+error: redundant argument
+  --> $DIR/issue-105225.rs:7:21
+   |
+LL |     println!("{x}", x);
+   |                     ^ help: this can be removed
+   |
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225.rs:7:16
+   |
+LL |     println!("{x}", x);
+   |                ^
+
+error: redundant argument
+  --> $DIR/issue-105225.rs:10:27
+   |
+LL |     println!("{x} {}", x, x);
+   |                           ^ help: this can be removed
+   |
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225.rs:10:16
+   |
+LL |     println!("{x} {}", x, x);
+   |                ^
+
+error: redundant argument
+  --> $DIR/issue-105225.rs:13:27
+   |
+LL |     println!("{} {x}", x, x);
+   |                           ^ help: this can be removed
+   |
+note: the formatting specifier is referencing the binding already
+  --> $DIR/issue-105225.rs:13:19
+   |
+LL |     println!("{} {x}", x, x);
+   |                   ^
+
+error: redundant arguments
+  --> $DIR/issue-105225.rs:16:25
+   |
+LL |     println!("{x} {y}", x, y);
+   |                         ^  ^
+   |
+note: the formatting specifiers are referencing the bindings already
+  --> $DIR/issue-105225.rs:16:16
+   |
+LL |     println!("{x} {y}", x, y);
+   |                ^   ^
+help: this can be removed
+   |
+LL -     println!("{x} {y}", x, y);
+LL +     println!("{x} {y}", );
+   |
+
+error: redundant arguments
+  --> $DIR/issue-105225.rs:19:43
+   |
+LL |     println!("{} {} {x} {y} {}", x, x, x, y, y);
+   |                                           ^  ^
+   |
+note: the formatting specifiers are referencing the bindings already
+  --> $DIR/issue-105225.rs:19:26
+   |
+LL |     println!("{} {} {x} {y} {}", x, x, x, y, y);
+   |                          ^
+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
+
