Add format_in_format_args and to_string_in_format_args lints

CHANGELOG.md

@@ -2736,6 +2736,7 @@ Released 2018-09-13
 [`for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles
 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
+[`format_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args
 [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
 [`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
 [`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10
@@ -3015,6 +3016,7 @@ Released 2018-09-13
 [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
 [`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
 [`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
+[`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
 [`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
 [`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
 [`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines

clippy_dev/src/update_lints.rs

@@ -619,8 +619,8 @@ mod tests {
             Lint::new("should_assert_eq2", "group2", "abc", None, "module_name"),
         ];
         let expected = vec![
-            format!("[`should_assert_eq`]: {}#should_assert_eq", DOCS_LINK.to_string()),
-            format!("[`should_assert_eq2`]: {}#should_assert_eq2", DOCS_LINK.to_string()),
+            format!("[`should_assert_eq`]: {}#should_assert_eq", DOCS_LINK),
+            format!("[`should_assert_eq2`]: {}#should_assert_eq2", DOCS_LINK),
         ];
         assert_eq!(expected, gen_changelog_lint_list(lints.iter()));
     }

clippy_lints/src/format.rs

@@ -1,11 +1,10 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::higher::FormatExpn;
-use clippy_utils::last_path_segment;
 use clippy_utils::source::{snippet_opt, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{BorrowKind, Expr, ExprKind, QPath};
+use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -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 let Some(args) = format_args.args();
+                if args.iter().all(|arg| arg.is_display() && !arg.has_string_formatting());
                 then {
                     let is_new_string = match value.kind {
                         ExprKind::Binary(..) => true,
@@ -106,47 +105,3 @@ fn span_useless_format(cx: &LateContext<'_>, span: Span, mut sugg: String, mut a
         applicability,
     );
 }
-
-fn is_display_arg(expr: &Expr<'_>) -> bool {
-    if_chain! {
-        if let ExprKind::Call(_, [_, fmt]) = expr.kind;
-        if let ExprKind::Path(QPath::Resolved(_, path)) = fmt.kind;
-        if let [.., t, _] = path.segments;
-        if t.ident.name == sym::Display;
-        then { true } else { false }
-    }
-}
-
-/// Checks if the expression matches
-/// ```rust,ignore
-/// &[_ {
-///    format: _ {
-///         width: _::Implied,
-///         precision: _::Implied,
-///         ...
-///    },
-///    ...,
-/// }]
-/// ```
-fn check_unformatted(expr: &Expr<'_>) -> bool {
-    if_chain! {
-        if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
-        if let ExprKind::Array([expr]) = expr.kind;
-        // struct `core::fmt::rt::v1::Argument`
-        if let ExprKind::Struct(_, fields, _) = expr.kind;
-        if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym::format);
-        // struct `core::fmt::rt::v1::FormatSpec`
-        if let ExprKind::Struct(_, fields, _) = format_field.expr.kind;
-        if let Some(precision_field) = fields.iter().find(|f| f.ident.name == sym::precision);
-        if let ExprKind::Path(ref precision_path) = precision_field.expr.kind;
-        if last_path_segment(precision_path).ident.name == sym::Implied;
-        if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym::width);
-        if let ExprKind::Path(ref width_qpath) = width_field.expr.kind;
-        if last_path_segment(width_qpath).ident.name == sym::Implied;
-        then {
-            return true;
-        }
-    }
-
-    false
-}

clippy_lints/src/format_args.rs

@@ -0,0 +1,223 @@
+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::{is_diag_trait_item, match_def_path, paths};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+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, ExpnData, ExpnKind, Span, Symbol};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detects `format!` within the arguments of another macro that does
+    /// formatting such as `format!` itself, `write!` or `println!`. Suggests
+    /// inlining the `format!` call.
+    ///
+    /// ### Why is this bad?
+    /// The recommended code is both shorter and avoids a temporary allocation.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # use std::panic::Location;
+    /// println!("error: {}", format!("something failed at {}", Location::caller()));
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # use std::panic::Location;
+    /// println!("error: something failed at {}", Location::caller());
+    /// ```
+    pub FORMAT_IN_FORMAT_ARGS,
+    perf,
+    "`format!` used in a macro that does formatting"
+}
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for [`ToString::to_string`](https://doc.rust-lang.org/std/string/trait.ToString.html#tymethod.to_string)
+    /// applied to a type that implements [`Display`](https://doc.rust-lang.org/std/fmt/trait.Display.html)
+    /// in a macro that does formatting.
+    ///
+    /// ### Why is this bad?
+    /// Since the type implements `Display`, the use of `to_string` is
+    /// unnecessary.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # use std::panic::Location;
+    /// println!("error: something failed at {}", Location::caller().to_string());
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # use std::panic::Location;
+    /// println!("error: something failed at {}", Location::caller());
+    /// ```
+    pub TO_STRING_IN_FORMAT_ARGS,
+    perf,
+    "`to_string` applied to a type that implements `Display` in format args"
+}
+
+declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
+
+const FORMAT_MACRO_PATHS: &[&[&str]] = &[
+    &paths::FORMAT_ARGS_MACRO,
+    &paths::ASSERT_EQ_MACRO,
+    &paths::ASSERT_MACRO,
+    &paths::ASSERT_NE_MACRO,
+    &paths::EPRINT_MACRO,
+    &paths::EPRINTLN_MACRO,
+    &paths::PRINT_MACRO,
+    &paths::PRINTLN_MACRO,
+    &paths::WRITE_MACRO,
+    &paths::WRITELN_MACRO,
+];
+
+const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[sym::format_macro, sym::std_panic_macro];
+
+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 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_MACRO_PATHS
+                .iter()
+                .any(|path| match_def_path(cx, macro_def_id, path))
+                || FORMAT_MACRO_DIAG_ITEMS
+                    .iter()
+                    .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, macro_def_id));
+            if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
+            if let Some(args) = format_args.args();
+            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, outermost_expn_data.call_site, name, arg);
+                    check_to_string_in_format_args(cx, name, arg);
+                }
+            }
+        }
+    }
+}
+
+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_chain! {
+        if FormatExpn::parse(arg.value).is_some();
+        if !arg.value.span.ctxt().outer_expn_data().call_site.from_expansion();
+        then {
+            span_lint_and_then(
+                cx,
+                FORMAT_IN_FORMAT_ARGS,
+                trim_semicolon(cx, call_site),
+                &format!("`format!` in `{}!` args", name),
+                |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>) {
+    let value = arg.value;
+    if_chain! {
+        if !value.span.from_expansion();
+        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);
+        let receiver_ty = cx.typeck_results().expr_ty(receiver);
+        if let Some(display_trait_id) = cx.tcx.get_diagnostic_item(sym::Display);
+        if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
+        then {
+            let (n_needed_derefs, target) = count_needed_derefs(
+                receiver_ty,
+                cx.typeck_results().expr_adjustments(receiver).iter(),
+            );
+            if implements_trait(cx, target, display_trait_id, &[]) {
+                if n_needed_derefs == 0 {
+                    span_lint_and_sugg(
+                        cx,
+                        TO_STRING_IN_FORMAT_ARGS,
+                        value.span.with_lo(receiver.span.hi()),
+                        &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_needed_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 {
+    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 {
+    snippet_opt(cx, span).map_or(span, |snippet| {
+        let snippet = snippet.trim_end_matches(';');
+        span.with_hi(span.lo() + BytePos(u32::try_from(snippet.len()).unwrap()))
+    })
+}
+
+fn count_needed_derefs<'tcx, I>(mut ty: Ty<'tcx>, mut iter: I) -> (usize, Ty<'tcx>)
+where
+    I: Iterator<Item = &'tcx Adjustment<'tcx>>,
+{
+    let mut n_total = 0;
+    let mut n_needed = 0;
+    loop {
+        if let Some(Adjustment {
+            kind: Adjust::Deref(overloaded_deref),
+            target,
+        }) = iter.next()
+        {
+            n_total += 1;
+            if overloaded_deref.is_some() {
+                n_needed = n_total;
+            }
+            ty = target;
+        } else {
+            return (n_needed, ty);
+        }
+    }
+}

clippy_lints/src/if_then_panic.rs

@@ -78,10 +78,10 @@ impl LateLintPass<'_> for IfThenPanic {
                     if let Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..} = e {
                          sugg::Sugg::hir_with_applicability(cx, not_expr, "..", &mut applicability).maybe_par().to_string()
                     } else {
-                       format!("!{}", sugg::Sugg::hir_with_applicability(cx, e, "..", &mut applicability).maybe_par().to_string())
+                       format!("!{}", sugg::Sugg::hir_with_applicability(cx, e, "..", &mut applicability).maybe_par())
                     }
                 } else {
-                   format!("!{}", sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par().to_string())
+                   format!("!{}", sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par())
                 };
 
                 span_lint_and_sugg(

clippy_lints/src/inherent_to_string.rs

@@ -138,10 +138,10 @@ fn show_lint(cx: &LateContext<'_>, item: &ImplItem<'_>) {
             item.span,
             &format!(
                 "type `{}` implements inherent method `to_string(&self) -> String` which shadows the implementation of `Display`",
-                self_type.to_string()
+                self_type
             ),
             None,
-            &format!("remove the inherent method from type `{}`", self_type.to_string()),
+            &format!("remove the inherent method from type `{}`", self_type),
         );
     } else {
         span_lint_and_help(
@@ -150,10 +150,10 @@ fn show_lint(cx: &LateContext<'_>, item: &ImplItem<'_>) {
             item.span,
             &format!(
                 "implementation of inherent method `to_string(&self) -> String` for type `{}`",
-                self_type.to_string()
+                self_type
             ),
             None,
-            &format!("implement trait `Display` for type `{}` instead", self_type.to_string()),
+            &format!("implement trait `Display` for type `{}` instead", self_type),
         );
     }
 }

clippy_lints/src/lib.register_all.rs

@@ -60,6 +60,8 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
     LintId::of(float_literal::EXCESSIVE_PRECISION),
     LintId::of(format::USELESS_FORMAT),
+    LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
+    LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
     LintId::of(formatting::POSSIBLE_MISSING_COMMA),
     LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
     LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING),

clippy_lints/src/lib.register_lints.rs

@@ -139,6 +139,8 @@ store.register_lints(&[
     floating_point_arithmetic::IMPRECISE_FLOPS,
     floating_point_arithmetic::SUBOPTIMAL_FLOPS,
     format::USELESS_FORMAT,
+    format_args::FORMAT_IN_FORMAT_ARGS,
+    format_args::TO_STRING_IN_FORMAT_ARGS,
     formatting::POSSIBLE_MISSING_COMMA,
     formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
     formatting::SUSPICIOUS_ELSE_FORMATTING,

clippy_lints/src/lib.register_perf.rs

@@ -5,6 +5,8 @@
 store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(entry::MAP_ENTRY),
     LintId::of(escape::BOXED_LOCAL),
+    LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
+    LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
     LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
     LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
     LintId::of(loops::MANUAL_MEMCPY),