Address review comments

Author: smoelius

clippy_lints/src/format.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::higher::FormatExpn;
+use clippy_utils::higher::{FormatArgsArg, FormatExpn};
 use clippy_utils::source::{snippet_opt, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use if_chain::if_chain;
@@ -68,8 +68,9 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
                     ty::Str => true,
                     _ => false,
                 };
-                if format_args.args().all(|arg| arg.is_display());
-                if !format_args.has_string_formatting();
+                if let Some(args) = format_args.args();
+                if args.iter().all(FormatArgsArg::is_display);
+                if !args.iter().any(FormatArgsArg::has_string_formatting);
                 then {
                     let is_new_string = match value.kind {
                         ExprKind::Binary(..) => true,

clippy_lints/src/format_args.rs

@@ -4,11 +4,15 @@ use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::implements_trait;
 use clippy_utils::{get_trait_def_id, match_def_path, paths};
 use if_chain::if_chain;
+use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
+use rustc_hir::def_id::DefId;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::subst::GenericArg;
+use rustc_middle::ty::Ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{BytePos, ExpnKind, Span, Symbol};
+use rustc_span::{sym, BytePos, ExpnKind, Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -34,14 +38,6 @@ declare_clippy_lint! {
     "`format!` used in a macro that does formatting"
 }
 
-declare_lint_pass!(FormatInFormatArgs => [FORMAT_IN_FORMAT_ARGS]);
-
-impl<'tcx> LateLintPass<'tcx> for FormatInFormatArgs {
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        check_expr(cx, expr, format_in_format_args);
-    }
-}
-
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for [`ToString::to_string`](https://doc.rust-lang.org/std/string/trait.ToString.html#tymethod.to_string)
@@ -67,216 +63,115 @@ declare_clippy_lint! {
     "`to_string` applied to a type that implements `Display` in format args"
 }
 
-declare_lint_pass!(ToStringInFormatArgs => [TO_STRING_IN_FORMAT_ARGS]);
-
-impl<'tcx> LateLintPass<'tcx> for ToStringInFormatArgs {
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        check_expr(cx, expr, to_string_in_format_args);
-    }
-}
-
-fn check_expr<'tcx, F>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, check_value: F)
-where
-    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.has_string_formatting();
-        then {
-            for (i, arg) in format_args.args().enumerate() {
-                if !arg.is_display() {
-                    continue;
+declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
+
+impl<'tcx> LateLintPass<'tcx> for FormatArgs {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if_chain! {
+            if let Some(format_args) = FormatArgsExpn::parse(expr);
+            let expn_data = {
+                let expn_data = expr.span.ctxt().outer_expn_data();
+                if expn_data.call_site.from_expansion() {
+                    expn_data.call_site.ctxt().outer_expn_data()
+                } else {
+                    expn_data
                 }
-                if check_value(cx, &format_args, expn_data.call_site, name, i, &arg) {
-                    break;
+            };
+            if let ExpnKind::Macro(_, name) = expn_data.kind;
+            if let Some(args) = format_args.args();
+            if !args.iter().any(FormatArgsArg::has_string_formatting);
+            then {
+                for (i, arg) in args.iter().enumerate() {
+                    if !arg.is_display() {
+                        continue;
+                    }
+                    if is_aliased(&args, i) {
+                        continue;
+                    }
+                    check_format_in_format_args(cx, expn_data.call_site, name, arg);
+                    check_to_string_in_format_args(cx, expn_data.call_site, name, arg);
                 }
             }
         }
     }
 }
 
-fn format_in_format_args(
-    cx: &LateContext<'_>,
-    format_args: &FormatArgsExpn<'_>,
-    call_site: Span,
-    name: Symbol,
-    i: usize,
-    arg: &FormatArgsArg<'_>,
-) -> bool {
-    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,
-                trim_semicolon(cx, call_site),
-                &format!("`format!` in `{}!` args", name),
-                "inline the `format!` call",
-                sugg,
-                applicability,
-            );
-        } 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;
+fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &FormatArgsArg<'_, '_>) {
+    if FormatExpn::parse(arg.value).is_some() {
+        span_lint_and_help(
+            cx,
+            FORMAT_IN_FORMAT_ARGS,
+            trim_semicolon(cx, call_site),
+            &format!("`format!` in `{}!` args", name),
+            None,
+            "inline the `format!` call",
+        );
     }
-    false
 }
 
-fn to_string_in_format_args(
-    cx: &LateContext<'_>,
-    _format_args: &FormatArgsExpn<'_>,
+fn check_to_string_in_format_args<'tcx>(
+    cx: &LateContext<'tcx>,
     _call_site: Span,
     name: Symbol,
-    _i: usize,
-    arg: &FormatArgsArg<'_>,
-) -> bool {
-    let value = arg.value();
+    arg: &FormatArgsArg<'_, 'tcx>,
+) {
+    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);
         if match_def_path(cx, method_def_id, &paths::TO_STRING_METHOD);
         if let Some(receiver) = args.get(0);
         let ty = cx.typeck_results().expr_ty(receiver);
         if let Some(display_trait_id) = get_trait_def_id(cx, &paths::DISPLAY_TRAIT);
-        if implements_trait(cx, ty, display_trait_id, &[]);
         if let Some(snippet) = snippet_opt(cx, value.span);
         if let Some(dot) = snippet.rfind('.');
         then {
             let span = value.span.with_lo(
                 value.span.lo() + BytePos(u32::try_from(dot).unwrap())
             );
-            span_lint_and_sugg(
-                cx,
-                TO_STRING_IN_FORMAT_ARGS,
-                span,
-                &format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
-                "remove this",
-                String::new(),
-                Applicability::MachineApplicable,
-            );
-        }
-    }
-    false
-}
-
-fn format_in_format_args_sugg(
-    cx: &LateContext<'_>,
-    name: Symbol,
-    format_args: &FormatArgsExpn<'_>,
-    i: usize,
-    inner_format_args: &FormatArgsExpn<'_>,
-) -> Option<(String, Applicability)> {
-    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,
-            Applicability::MachineApplicable,
-        )
-    };
-    let values = format_args
-        .value_args
-        .iter()
-        .map(|value| snippet_opt(cx, value.span))
-        .collect::<Option<Vec<_>>>()?;
-    let inner_values = inner_format_args
-        .value_args
-        .iter()
-        .map(|value| snippet_opt(cx, value.span))
-        .collect::<Option<Vec<_>>>()?;
-    let new_values = itertools::join(
-        values
-            .iter()
-            .take(i)
-            .chain(inner_values.iter())
-            .chain(values.iter().skip(i + 1)),
-        ", ",
-    );
-    Some((
-        format!(r#"{}!({}, {})"#, name, new_format_string, new_values),
-        applicability,
-    ))
-}
-
-// 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;
+            if implements_trait(cx, ty, display_trait_id, &[]) {
+                span_lint_and_sugg(
+                    cx,
+                    TO_STRING_IN_FORMAT_ARGS,
+                    span,
+                    &format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
+                    "remove this",
+                    String::new(),
+                    Applicability::MachineApplicable,
+                );
+            } else if implements_trait_via_deref(cx, ty, display_trait_id, &[]) {
+                span_lint_and_sugg(
+                    cx,
+                    TO_STRING_IN_FORMAT_ARGS,
+                    span,
+                    &format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
+                    "use this",
+                    String::from(".deref()"),
+                    Applicability::MachineApplicable,
+                );
+            }
         }
     }
-    false
-}
-
-fn split_format_string(format_string: &str, i: usize) -> (String, String) {
-    let mut iter = format_string.chars();
-    for j in 0..=i {
-        assert!(advance(&mut iter) == '}' || j < i);
-    }
-
-    let right = iter.collect::<String>();
-
-    let size = format_string.len();
-    let right_size = right.len();
-    let left_size = size - right_size;
-    assert!(left_size >= 2);
-    let left = std::str::from_utf8(&format_string.as_bytes()[..left_size - 2])
-        .unwrap()
-        .to_owned();
-    (left, right)
 }
 
-fn advance(iter: &mut std::str::Chars<'_>) -> char {
-    loop {
-        let first_char = iter.next().unwrap();
-        if first_char != '{' {
-            continue;
-        }
-        let second_char = iter.next().unwrap();
-        if second_char == '{' {
-            continue;
+// Returns true if `args[i]` "refers to" or "is referred to by" another argument.
+fn is_aliased(args: &[FormatArgsArg<'_, '_>], i: usize) -> bool {
+    args.iter().enumerate().any(|(j, arg)| {
+        if_chain! {
+            if let Some(fmt) = arg.fmt;
+            // struct `core::fmt::rt::v1::Argument`
+            if let ExprKind::Struct(_, fields, _) = fmt.kind;
+            if let Some(position_field) = fields.iter().find(|f| f.ident.name == sym::position);
+            if let ExprKind::Lit(lit) = &position_field.expr.kind;
+            if let LitKind::Int(position, _) = lit.node;
+            then {
+                let position = usize::try_from(position).unwrap();
+                (j == i && position != i) || (j != i && position == i)
+            } else {
+                false
+            }
         }
-        return second_char;
-    }
-}
-
-fn trim_quotes(string_literal: &str) -> String {
-    let len = string_literal.chars().count();
-    assert!(len >= 2);
-    string_literal.chars().skip(1).take(len - 2).collect()
+    })
 }
 
 fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
@@ -285,3 +180,24 @@ fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
         span.with_hi(span.lo() + BytePos(u32::try_from(snippet.len()).unwrap()))
     })
 }
+
+fn implements_trait_via_deref<'tcx>(
+    cx: &LateContext<'tcx>,
+    ty: Ty<'tcx>,
+    trait_id: DefId,
+    ty_params: &[GenericArg<'tcx>],
+) -> bool {
+    if_chain! {
+        if let Some(deref_trait_id) = cx.tcx.lang_items().deref_trait();
+        if implements_trait(cx, ty, deref_trait_id, &[]);
+        if let Some(deref_target_id) = cx.tcx.lang_items().deref_target();
+        then {
+            let substs = cx.tcx.mk_substs_trait(ty, &[]);
+            let projection_ty = cx.tcx.mk_projection(deref_target_id, substs);
+            let target_ty = cx.tcx.normalize_erasing_regions(cx.param_env, projection_ty);
+            implements_trait(cx, target_ty, trait_id, ty_params)
+        } else {
+            false
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -772,8 +772,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let enable_raw_pointer_heuristic_for_send = conf.enable_raw_pointer_heuristic_for_send;
     store.register_late_pass(move || Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(enable_raw_pointer_heuristic_for_send)));
     store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
-    store.register_late_pass(move || Box::new(format_args::FormatInFormatArgs));
-    store.register_late_pass(move || Box::new(format_args::ToStringInFormatArgs));
+    store.register_late_pass(move || Box::new(format_args::FormatArgs));
 }
 
 #[rustfmt::skip]