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::{FormatArgsArg, FormatExpn};
+use clippy_utils::higher::FormatExpn;
 use clippy_utils::source::{snippet_opt, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use if_chain::if_chain;

clippy_lints/src/format_args.rs

@@ -1,18 +1,16 @@
-use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::higher::{FormatArgsArg, FormatArgsExpn, FormatExpn};
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{get_trait_def_id, match_def_path, paths};
+use clippy_utils::{get_trait_def_id, is_diag_trait_item, 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::adjustment::{Adjust, Adjustment};
 use rustc_middle::ty::Ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{sym, BytePos, ExpnKind, Span, Symbol};
+use rustc_span::{sym, BytePos, ExpnData, ExpnKind, Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -65,112 +63,129 @@ declare_clippy_lint! {
 
 declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
 
+const FORMAT_MACROS: &[&[&str]] = &[
+    &["alloc", "macros", "format"],
+    &["core", "macros", "assert_eq"],
+    &["core", "macros", "assert_ne"],
+    &["core", "macros", "write"],
+    &["core", "macros", "writeln"],
+    &["core", "macros", "builtin", "assert"],
+    &["core", "macros", "builtin", "format_args"],
+    &["std", "macros", "eprint"],
+    &["std", "macros", "eprintln"],
+    &["std", "macros", "panic"],
+    &["std", "macros", "print"],
+    &["std", "macros", "println"],
+];
+
 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 let ExpnKind::Macro(_, name) = expn_data.kind;
+            let expr_expn_data = expr.span.ctxt().outer_expn_data();
+            let outermost_expn_data = outermost_expn_data(expr_expn_data);
+            if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
+            if FORMAT_MACROS.iter().any(|path| match_def_path(cx, macro_def_id, path));
+            if let ExpnKind::Macro(_, name) = outermost_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 arg.has_string_formatting() {
+                        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);
+                    check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg);
+                    check_to_string_in_format_args(cx, name, arg);
                 }
             }
         }
     }
 }
 
-fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &FormatArgsArg<'_, '_>) {
+fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
+    if expn_data.call_site.from_expansion() {
+        outermost_expn_data(expn_data.call_site.ctxt().outer_expn_data())
+    } else {
+        expn_data
+    }
+}
+
+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(
+        span_lint_and_then(
             cx,
             FORMAT_IN_FORMAT_ARGS,
             trim_semicolon(cx, call_site),
             &format!("`format!` in `{}!` args", name),
-            None,
-            "inline the `format!` call",
+            |diag| {
+                diag.help(&format!(
+                    "combine the `format!(..)` arguments with the outer `{}!(..)` call",
+                    name
+                ));
+                diag.help("or consider changing `format!` to `format_args!`");
+            },
         );
     }
 }
 
-fn check_to_string_in_format_args<'tcx>(
-    cx: &LateContext<'tcx>,
-    name: Symbol,
-    arg: &FormatArgsArg<'_, 'tcx>,
-) {
+fn check_to_string_in_format_args<'tcx>(cx: &LateContext<'tcx>, name: Symbol, arg: &FormatArgsArg<'tcx>) {
     let value = arg.value;
     if_chain! {
-        if let ExprKind::MethodCall(_, _, [receiver], span) = value.kind;
+        if let ExprKind::MethodCall(_, _, [receiver], _) = value.kind;
         if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
         if is_diag_trait_item(cx, method_def_id, sym::ToString);
-        if let Some(receiver) = args.get(0);
-        let ty = cx.typeck_results().expr_ty(receiver);
+        let receiver_ty = cx.typeck_results().expr_ty(receiver);
         if let Some(display_trait_id) = get_trait_def_id(cx, &paths::DISPLAY_TRAIT);
-        if let Some(snippet) = snippet_opt(cx, value.span);
-        if let Some(dot) = snippet.rfind('.');
+        if let Some(value_snippet) = snippet_opt(cx, value.span);
+        if let Some(dot) = value_snippet.rfind('.');
+        if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
         then {
-            let span = value.span.with_lo(
-                value.span.lo() + BytePos(u32::try_from(dot).unwrap())
+            let (n_derefs, target) = count_derefs(
+                0,
+                receiver_ty,
+                &mut cx.typeck_results().expr_adjustments(receiver).iter(),
             );
-            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,
-                );
+            if implements_trait(cx, target, display_trait_id, &[]) {
+                if n_derefs == 0 {
+                    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,
+                    );
+                } else {
+                    span_lint_and_sugg(
+                        cx,
+                        TO_STRING_IN_FORMAT_ARGS,
+                        value.span,
+                        &format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
+                        "use this",
+                        format!("{:*>width$}{}", "", receiver_snippet, width = n_derefs),
+                        Applicability::MachineApplicable,
+                    );
+                }
             }
         }
     }
 }
 
 // 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
-            }
-        }
-    })
+fn is_aliased(args: &[FormatArgsArg<'_>], i: usize) -> bool {
+    let value = args[i].value;
+    args.iter()
+        .enumerate()
+        .any(|(j, arg)| i != j && std::ptr::eq(value, arg.value))
 }
 
 fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
@@ -180,23 +195,17 @@ fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
     })
 }
 
-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
-        }
+fn count_derefs<'tcx, I>(n: usize, ty: Ty<'tcx>, iter: &mut I) -> (usize, Ty<'tcx>)
+where
+    I: Iterator<Item = &'tcx Adjustment<'tcx>>,
+{
+    if let Some(Adjustment {
+        kind: Adjust::Deref(Some(_)),
+        target,
+    }) = iter.next()
+    {
+        count_derefs(n + 1, target, iter)
+    } else {
+        (n, ty)
     }
 }

clippy_utils/src/higher.rs

@@ -574,13 +574,13 @@ impl FormatArgsExpn<'tcx> {
     }
 
     /// Returns a vector of `FormatArgsArg`.
-    pub fn args<'fmt>(&'fmt self) -> Option<Vec<FormatArgsArg<'fmt, 'tcx>>> {
+    pub fn args(&self) -> Option<Vec<FormatArgsArg<'tcx>>> {
         if_chain! {
             if let Some(expr) = self.fmt_expr;
             if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
             if let ExprKind::Array(exprs) = expr.kind;
             then {
-                exprs.iter().map(|fmt| {
+                return exprs.iter().map(|fmt| {
                     if_chain! {
                         // struct `core::fmt::rt::v1::Argument`
                         if let ExprKind::Struct(_, fields, _) = fmt.kind;
@@ -595,17 +595,20 @@ impl FormatArgsExpn<'tcx> {
                         }
                     }
                 }).collect()
-            } else {
-                Some(self.value_args.iter().zip(self.args.iter()).map(|(value, arg)| {
-                    FormatArgsArg { value, arg, fmt: None }
-                }).collect())
             }
         }
+        Some(
+            self.value_args
+                .iter()
+                .zip(self.args.iter())
+                .map(|(value, arg)| FormatArgsArg { value, arg, fmt: None })
+                .collect(),
+        )
     }
 }
 
 /// Type representing a `FormatArgsExpn`'s format arguments
-pub struct FormatArgsArg<'fmt, 'tcx> {
+pub struct FormatArgsArg<'tcx> {
     /// An element of `value_args` according to `position`
     pub value: &'tcx Expr<'tcx>,
     /// An element of `args` according to `position`
@@ -614,7 +617,7 @@ pub struct FormatArgsArg<'fmt, 'tcx> {
     pub fmt: Option<&'tcx Expr<'tcx>>,
 }
 
-impl<'fmt, 'tcx> FormatArgsArg<'fmt, 'tcx> {
+impl<'tcx> FormatArgsArg<'tcx> {
     /// Returns true if any formatting parameters are used that would have an effect on strings,
     /// like `{:+2}` instead of just `{}`.
     pub fn has_string_formatting(&self) -> bool {

tests/ui/format_args.fixed

@@ -1,11 +1,13 @@
 // run-rustfix
 
 #![allow(unreachable_code)]
+#![allow(unused_macros)]
 #![allow(unused_variables)]
 #![allow(clippy::assertions_on_constants)]
 #![allow(clippy::eq_op)]
 #![warn(clippy::to_string_in_format_args)]
 
+use std::io::{stdout, Write};
 use std::ops::Deref;
 use std::panic::Location;
 
@@ -27,34 +29,66 @@ impl Deref for X {
     }
 }
 
-struct Y(u32);
+struct Y(X);
 
 impl Deref for Y {
+    type Target = X;
+
+    fn deref(&self) -> &X {
+        &self.0
+    }
+}
+
+struct Z(u32);
+
+impl Deref for Z {
     type Target = u32;
 
     fn deref(&self) -> &u32 {
         &self.0
     }
 }
 
-impl std::fmt::Display for Y {
+impl std::fmt::Display for Z {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "Y")
+        write!(f, "Z")
     }
 }
 
 fn main() {
     let x = 'x';
 
-    println!("error: something failed at {}", Location::caller());
     let _ = format!("error: something failed at {}", Location::caller());
+    let _ = write!(
+        stdout(),
+        "error: something failed at {}",
+        Location::caller()
+    );
+    let _ = writeln!(
+        stdout(),
+        "error: something failed at {}",
+        Location::caller()
+    );
+    print!("error: something failed at {}", Location::caller());
+    println!("error: something failed at {}", Location::caller());
+    eprint!("error: something failed at {}", Location::caller());
+    eprintln!("error: something failed at {}", Location::caller());
     let _ = format_args!("error: something failed at {}", Location::caller());
     assert!(true, "error: something failed at {}", Location::caller());
     assert_eq!(0, 0, "error: something failed at {}", Location::caller());
+    assert_ne!(0, 0, "error: something failed at {}", Location::caller());
     panic!("error: something failed at {}", Location::caller());
-    println!("{}", X(1).deref());
-    println!("{}", Y(1));
+    println!("{}", *X(1));
+    println!("{}", **Y(X(1)));
+    println!("{}", Z(1));
 
     println!("error: something failed at {}", Somewhere.to_string());
     println!("{} and again {0}", x.to_string());
 }
+
+macro_rules! my_macro {
+    () => {
+        // here be dragons, do not enter (or lint)
+        println!("error: something failed at {}", Location::caller().to_string());
+    };
+}