Properly handle positional arguments

Author: smoelius

clippy_lints/src/format.rs

@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::format::{check_unformatted, is_display_arg};
-use clippy_utils::higher::FormatExpn;
+use clippy_utils::higher::{FormatExpn, Formatting};
 use clippy_utils::source::{snippet_opt, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use if_chain::if_chain;
@@ -69,8 +68,8 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
                     ty::Str => true,
                     _ => false,
                 };
-                if format_args.args.iter().all(is_display_arg);
-                if format_args.fmt_expr.map_or(true, check_unformatted);
+                if format_args.args().all(|arg| arg.is_display());
+                if !format_args.has_formatting(Formatting::PRECISION | Formatting::WIDTH);
                 then {
                     let is_new_string = match value.kind {
                         ExprKind::Binary(..) => true,

clippy_lints/src/format_args.rs

@@ -1,6 +1,5 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::format::{check_unformatted, is_display_arg};
-use clippy_utils::higher::{FormatArgsExpn, FormatExpn};
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::higher::{FormatArgsArg, FormatArgsExpn, FormatExpn, Formatting};
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::implements_trait;
 use clippy_utils::{get_trait_def_id, match_def_path, paths};
@@ -78,22 +77,21 @@ impl<'tcx> LateLintPass<'tcx> for ToStringInFormatArgs {
 
 fn check_expr<'tcx, F>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, check_value: F)
 where
-    F: Fn(&LateContext<'_>, &FormatArgsExpn<'_>, Span, Symbol, usize, &Expr<'_>) -> bool,
+    F: Fn(&LateContext<'_>, &FormatArgsExpn<'_>, Span, Symbol, usize, &FormatArgsArg<'_>) -> bool,
 {
     if_chain! {
         if let Some(format_args) = FormatArgsExpn::parse(expr);
         let call_site = expr.span.ctxt().outer_expn_data().call_site;
         if call_site.from_expansion();
         let expn_data = call_site.ctxt().outer_expn_data();
         if let ExpnKind::Macro(_, name) = expn_data.kind;
-        if format_args.fmt_expr.map_or(true, check_unformatted);
+        if !format_args.has_formatting(Formatting::all());
         then {
-            assert_eq!(format_args.args.len(), format_args.value_args.len());
-            for (i, (arg, value)) in format_args.args.iter().zip(format_args.value_args.iter()).enumerate() {
-                if !is_display_arg(arg) {
+            for (i, arg) in format_args.args().enumerate() {
+                if !arg.is_display() {
                     continue;
                 }
-                if check_value(cx, &format_args, expn_data.call_site, name, i, value) {
+                if check_value(cx, &format_args, expn_data.call_site, name, i, &arg) {
                     break;
                 }
             }
@@ -107,22 +105,14 @@ fn format_in_format_args(
     call_site: Span,
     name: Symbol,
     i: usize,
-    value: &Expr<'_>,
+    arg: &FormatArgsArg<'_>,
 ) -> bool {
-    if_chain! {
-        if let Some(FormatExpn{ format_args: inner_format_args, .. }) = FormatExpn::parse(value);
-        if let Some(format_string) = snippet_opt(cx, format_args.format_string_span);
-        if let Some(inner_format_string) = snippet_opt(cx, inner_format_args.format_string_span);
-        if let Some((sugg, applicability)) = format_in_format_args_sugg(
-            cx,
-            name,
-            &format_string,
-            &format_args.value_args,
-            i,
-            &inner_format_string,
-            &inner_format_args.value_args
-        );
-        then {
+    if let Some(FormatExpn {
+        format_args: inner_format_args,
+        ..
+    }) = FormatExpn::parse(arg.value())
+    {
+        if let Some((sugg, applicability)) = format_in_format_args_sugg(cx, name, format_args, i, &inner_format_args) {
             span_lint_and_sugg(
                 cx,
                 FORMAT_IN_FORMAT_ARGS,
@@ -132,9 +122,18 @@ fn format_in_format_args(
                 sugg,
                 applicability,
             );
-            // Report only the first instance.
-            return true;
+        } else {
+            span_lint_and_help(
+                cx,
+                FORMAT_IN_FORMAT_ARGS,
+                trim_semicolon(cx, call_site),
+                &format!("`format!` in `{}!` args", name),
+                None,
+                "inline the `format!` call",
+            );
         }
+        // Report only the first instance.
+        return true;
     }
     false
 }
@@ -145,8 +144,9 @@ fn to_string_in_format_args(
     _call_site: Span,
     name: Symbol,
     _i: usize,
-    value: &Expr<'_>,
+    arg: &FormatArgsArg<'_>,
 ) -> bool {
+    let value = arg.value();
     if_chain! {
         if let ExprKind::MethodCall(_, _, args, _) = value.kind;
         if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
@@ -178,27 +178,35 @@ fn to_string_in_format_args(
 fn format_in_format_args_sugg(
     cx: &LateContext<'_>,
     name: Symbol,
-    format_string: &str,
-    values: &[&Expr<'_>],
+    format_args: &FormatArgsExpn<'_>,
     i: usize,
-    inner_format_string: &str,
-    inner_values: &[&Expr<'_>],
+    inner_format_args: &FormatArgsExpn<'_>,
 ) -> Option<(String, Applicability)> {
-    let (left, right) = split_format_string(format_string, i);
+    let format_string = snippet_opt(cx, format_args.format_string_span)?;
+    if is_positional(&format_string) {
+        return None;
+    }
+    let (left, right) = split_format_string(&format_string, i);
+    let inner_format_string = snippet_opt(cx, inner_format_args.format_string_span)?;
+    if is_positional(&inner_format_string) {
+        return None;
+    }
     // If the inner format string is raw, the user is on their own.
     let (new_format_string, applicability) = if inner_format_string.starts_with('r') {
         (left + ".." + &right, Applicability::HasPlaceholders)
     } else {
         (
-            left + &trim_quotes(inner_format_string) + &right,
+            left + &trim_quotes(&inner_format_string) + &right,
             Applicability::MachineApplicable,
         )
     };
-    let values = values
+    let values = format_args
+        .value_args
         .iter()
         .map(|value| snippet_opt(cx, value.span))
         .collect::<Option<Vec<_>>>()?;
-    let inner_values = inner_values
+    let inner_values = inner_format_args
+        .value_args
         .iter()
         .map(|value| snippet_opt(cx, value.span))
         .collect::<Option<Vec<_>>>()?;
@@ -216,6 +224,23 @@ fn format_in_format_args_sugg(
     ))
 }
 
+// Checking the position fields of the `core::fmt::rt::v1::Argument` would not be sufficient
+// because, for example, "{}{}" and "{}{1:}" could not be distinguished. Note that the `{}` could be
+// replaced in the former, but not the latter.
+fn is_positional(format_string: &str) -> bool {
+    let mut iter = format_string.chars();
+    while let Some(first_char) = iter.next() {
+        if first_char != '{' {
+            continue;
+        }
+        let second_char = iter.next().unwrap();
+        if second_char.is_digit(10) {
+            return true;
+        }
+    }
+    false
+}
+
 fn split_format_string(format_string: &str, i: usize) -> (String, String) {
     let mut iter = format_string.chars();
     for j in 0..=i {

clippy_utils/Cargo.toml

@@ -5,6 +5,7 @@ edition = "2021"
 publish = false
 
 [dependencies]
+bitflags = "1.2"
 if_chain = "1.0"
 rustc-semver = "1.1"