Auto merge of rust-lang#8253 - Alexendoo:print-in-fmt, r=camsteffen

Add `print_in_format_impl` lint

changelog: new lint: [`print_in_format_impl`]

Lints the use of `print`-like macros in manual `Display`/`Debug` impls. I feel like I make this mistake every time I write one 😄

r? `@camsteffen`

CHANGELOG.md

@@ -3370,6 +3370,7 @@ Released 2018-09-13
 [`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch
 [`possible_missing_comma`]: https://rust-lang.github.io/rust-clippy/master/index.html#possible_missing_comma
 [`precedence`]: https://rust-lang.github.io/rust-clippy/master/index.html#precedence
+[`print_in_format_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_in_format_impl
 [`print_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_literal
 [`print_stderr`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_stderr
 [`print_stdout`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_stdout

clippy_lints/src/recursive_format_impl.rs → clippy_lints/src/format_impl.rs

@@ -1,27 +1,13 @@
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg};
 use clippy_utils::macros::{is_format_macro, root_macro_call_first_node, FormatArgsArg, FormatArgsExpn};
-use clippy_utils::{is_diag_trait_item, path_to_local, peel_ref_operators};
+use clippy_utils::{get_parent_as_impl, is_diag_trait_item, path_to_local, peel_ref_operators};
 use if_chain::if_chain;
-use rustc_hir::{Expr, ExprKind, Impl, Item, ItemKind, QPath};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, Impl, ImplItem, ImplItemKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{sym, symbol::kw, Symbol};
 
-#[derive(Clone, Copy)]
-enum FormatTrait {
-    Debug,
-    Display,
-}
-
-impl FormatTrait {
-    fn name(self) -> Symbol {
-        match self {
-            FormatTrait::Debug => sym::Debug,
-            FormatTrait::Display => sym::Display,
-        }
-    }
-}
-
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for format trait implementations (e.g. `Display`) with a recursive call to itself
@@ -61,47 +47,92 @@ declare_clippy_lint! {
     "Format trait method called while implementing the same Format trait"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for use of `println`, `print`, `eprintln` or `eprint` in an
+    /// implementation of a formatting trait.
+    ///
+    /// ### Why is this bad?
+    /// Using a print macro is likely unintentional since formatting traits
+    /// should write to the `Formatter`, not stdout/stderr.
+    ///
+    /// ### Example
+    /// ```rust
+    /// use std::fmt::{Display, Error, Formatter};
+    ///
+    /// struct S;
+    /// impl Display for S {
+    ///     fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
+    ///         println!("S");
+    ///
+    ///         Ok(())
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::fmt::{Display, Error, Formatter};
+    ///
+    /// struct S;
+    /// impl Display for S {
+    ///     fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
+    ///         writeln!(f, "S");
+    ///
+    ///         Ok(())
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.61.0"]
+    pub PRINT_IN_FORMAT_IMPL,
+    suspicious,
+    "use of a print macro in a formatting trait impl"
+}
+
+#[derive(Clone, Copy)]
+struct FormatTrait {
+    /// e.g. `sym::Display`
+    name: Symbol,
+    /// `f` in `fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {}`
+    formatter_name: Option<Symbol>,
+}
+
 #[derive(Default)]
-pub struct RecursiveFormatImpl {
+pub struct FormatImpl {
     // Whether we are inside Display or Debug trait impl - None for neither
     format_trait_impl: Option<FormatTrait>,
 }
 
-impl RecursiveFormatImpl {
+impl FormatImpl {
     pub fn new() -> Self {
         Self {
             format_trait_impl: None,
         }
     }
 }
 
-impl_lint_pass!(RecursiveFormatImpl => [RECURSIVE_FORMAT_IMPL]);
+impl_lint_pass!(FormatImpl => [RECURSIVE_FORMAT_IMPL, PRINT_IN_FORMAT_IMPL]);
 
-impl<'tcx> LateLintPass<'tcx> for RecursiveFormatImpl {
-    fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
-        if let Some(format_trait_impl) = is_format_trait_impl(cx, item) {
-            self.format_trait_impl = Some(format_trait_impl);
-        }
+impl<'tcx> LateLintPass<'tcx> for FormatImpl {
+    fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
+        self.format_trait_impl = is_format_trait_impl(cx, impl_item);
     }
 
-    fn check_item_post(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
+    fn check_impl_item_post(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
         // Assume no nested Impl of Debug and Display within eachother
-        if is_format_trait_impl(cx, item).is_some() {
+        if is_format_trait_impl(cx, impl_item).is_some() {
             self.format_trait_impl = None;
         }
     }
 
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        match self.format_trait_impl {
-            Some(FormatTrait::Display) => {
-                check_to_string_in_display(cx, expr);
-                check_self_in_format_args(cx, expr, FormatTrait::Display);
-            },
-            Some(FormatTrait::Debug) => {
-                check_self_in_format_args(cx, expr, FormatTrait::Debug);
-            },
-            None => {},
+        let Some(format_trait_impl) = self.format_trait_impl else { return };
+
+        if format_trait_impl.name == sym::Display {
+            check_to_string_in_display(cx, expr);
         }
+
+        check_self_in_format_args(cx, expr, format_trait_impl);
+        check_print_in_format_impl(cx, expr, format_trait_impl);
     }
 }
 
@@ -140,7 +171,7 @@ fn check_self_in_format_args<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>,
         if let Some(args) = format_args.args();
         then {
             for arg in args {
-                if arg.format_trait != impl_trait.name() {
+                if arg.format_trait != impl_trait.name {
                     continue;
                 }
                 check_format_arg_self(cx, expr, &arg, impl_trait);
@@ -156,33 +187,65 @@ fn check_format_arg_self(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &FormatArgs
     let reference = peel_ref_operators(cx, arg.value);
     let map = cx.tcx.hir();
     // Is the reference self?
-    let symbol_ident = impl_trait.name().to_ident_string();
     if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) {
+        let FormatTrait { name, .. } = impl_trait;
         span_lint(
             cx,
             RECURSIVE_FORMAT_IMPL,
             expr.span,
-            &format!(
-                "using `self` as `{}` in `impl {}` will cause infinite recursion",
-                &symbol_ident, &symbol_ident
-            ),
+            &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"),
         );
     }
 }
 
-fn is_format_trait_impl(cx: &LateContext<'_>, item: &Item<'_>) -> Option<FormatTrait> {
+fn check_print_in_format_impl(cx: &LateContext<'_>, expr: &Expr<'_>, impl_trait: FormatTrait) {
     if_chain! {
-        // Are we at an Impl?
-        if let ItemKind::Impl(Impl { of_trait: Some(trait_ref), .. }) = &item.kind;
+        if let Some(macro_call) = root_macro_call_first_node(cx, expr);
+        if let Some(name) = cx.tcx.get_diagnostic_name(macro_call.def_id);
+        then {
+            let replacement = match name {
+                sym::print_macro | sym::eprint_macro => "write",
+                sym::println_macro | sym::eprintln_macro => "writeln",
+                _ => return,
+            };
+
+            let name = name.as_str().strip_suffix("_macro").unwrap();
+
+            span_lint_and_sugg(
+                cx,
+                PRINT_IN_FORMAT_IMPL,
+                macro_call.span,
+                &format!("use of `{}!` in `{}` impl", name, impl_trait.name),
+                "replace with",
+                if let Some(formatter_name) = impl_trait.formatter_name {
+                    format!("{}!({}, ..)", replacement, formatter_name)
+                } else {
+                    format!("{}!(..)", replacement)
+                },
+                Applicability::HasPlaceholders,
+            );
+        }
+    }
+}
+
+fn is_format_trait_impl(cx: &LateContext<'_>, impl_item: &ImplItem<'_>) -> Option<FormatTrait> {
+    if_chain! {
+        if impl_item.ident.name == sym::fmt;
+        if let ImplItemKind::Fn(_, body_id) = impl_item.kind;
+        if let Some(Impl { of_trait: Some(trait_ref),..}) = get_parent_as_impl(cx.tcx, impl_item.hir_id());
         if let Some(did) = trait_ref.trait_def_id();
         if let Some(name) = cx.tcx.get_diagnostic_name(did);
+        if matches!(name, sym::Debug | sym::Display);
         then {
-            // Is Impl for Debug or Display?
-            match name {
-                sym::Debug => Some(FormatTrait::Debug),
-                sym::Display => Some(FormatTrait::Display),
-                _ => None,
-            }
+            let body = cx.tcx.hir().body(body_id);
+            let formatter_name = body.params.get(1)
+                .and_then(|param| param.pat.simple_ident())
+                .map(|ident| ident.name);
+
+            Some(FormatTrait {
+                name,
+                formatter_name,
+            })
         } else {
             None
         }

clippy_lints/src/lib.register_all.rs

@@ -68,6 +68,8 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(format::USELESS_FORMAT),
     LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
     LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
+    LintId::of(format_impl::PRINT_IN_FORMAT_IMPL),
+    LintId::of(format_impl::RECURSIVE_FORMAT_IMPL),
     LintId::of(formatting::POSSIBLE_MISSING_COMMA),
     LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
     LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING),
@@ -244,7 +246,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(ranges::MANUAL_RANGE_CONTAINS),
     LintId::of(ranges::RANGE_ZIP_WITH_LEN),
     LintId::of(ranges::REVERSED_EMPTY_RANGES),
-    LintId::of(recursive_format_impl::RECURSIVE_FORMAT_IMPL),
     LintId::of(redundant_clone::REDUNDANT_CLONE),
     LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL),
     LintId::of(redundant_field_names::REDUNDANT_FIELD_NAMES),

clippy_lints/src/lib.register_correctness.rs

@@ -24,6 +24,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
     LintId::of(enum_clike::ENUM_CLIKE_UNPORTABLE_VARIANT),
     LintId::of(eq_op::EQ_OP),
     LintId::of(erasing_op::ERASING_OP),
+    LintId::of(format_impl::RECURSIVE_FORMAT_IMPL),
     LintId::of(formatting::POSSIBLE_MISSING_COMMA),
     LintId::of(functions::NOT_UNSAFE_PTR_ARG_DEREF),
     LintId::of(if_let_mutex::IF_LET_MUTEX),
@@ -52,7 +53,6 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
     LintId::of(ptr::INVALID_NULL_PTR_USAGE),
     LintId::of(ptr::MUT_FROM_REF),
     LintId::of(ranges::REVERSED_EMPTY_RANGES),
-    LintId::of(recursive_format_impl::RECURSIVE_FORMAT_IMPL),
     LintId::of(regex::INVALID_REGEX),
     LintId::of(self_assignment::SELF_ASSIGNMENT),
     LintId::of(serde_api::SERDE_API_MISUSE),

clippy_lints/src/lib.register_lints.rs

@@ -152,6 +152,8 @@ store.register_lints(&[
     format::USELESS_FORMAT,
     format_args::FORMAT_IN_FORMAT_ARGS,
     format_args::TO_STRING_IN_FORMAT_ARGS,
+    format_impl::PRINT_IN_FORMAT_IMPL,
+    format_impl::RECURSIVE_FORMAT_IMPL,
     formatting::POSSIBLE_MISSING_COMMA,
     formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
     formatting::SUSPICIOUS_ELSE_FORMATTING,
@@ -417,7 +419,6 @@ store.register_lints(&[
     ranges::RANGE_PLUS_ONE,
     ranges::RANGE_ZIP_WITH_LEN,
     ranges::REVERSED_EMPTY_RANGES,
-    recursive_format_impl::RECURSIVE_FORMAT_IMPL,
     redundant_clone::REDUNDANT_CLONE,
     redundant_closure_call::REDUNDANT_CLOSURE_CALL,
     redundant_else::REDUNDANT_ELSE,

clippy_lints/src/lib.register_suspicious.rs

@@ -10,6 +10,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(casts::CAST_ENUM_TRUNCATION),
     LintId::of(eval_order_dependence::EVAL_ORDER_DEPENDENCE),
     LintId::of(float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
+    LintId::of(format_impl::PRINT_IN_FORMAT_IMPL),
     LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
     LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING),
     LintId::of(formatting::SUSPICIOUS_UNARY_OP_FORMATTING),

clippy_lints/src/lib.rs

@@ -226,6 +226,7 @@ mod float_literal;
 mod floating_point_arithmetic;
 mod format;
 mod format_args;
+mod format_impl;
 mod formatting;
 mod from_over_into;
 mod from_str_radix_10;
@@ -333,7 +334,6 @@ mod ptr_eq;
 mod ptr_offset_with_cast;
 mod question_mark;
 mod ranges;
-mod recursive_format_impl;
 mod redundant_clone;
 mod redundant_closure_call;
 mod redundant_else;
@@ -705,7 +705,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(modulo_arithmetic::ModuloArithmetic));
     store.register_early_pass(|| Box::new(reference::DerefAddrOf));
     store.register_early_pass(|| Box::new(double_parens::DoubleParens));
-    store.register_late_pass(|| Box::new(recursive_format_impl::RecursiveFormatImpl::new()));
+    store.register_late_pass(|| Box::new(format_impl::FormatImpl::new()));
     store.register_early_pass(|| Box::new(unsafe_removed_from_name::UnsafeNameRemoval));
     store.register_early_pass(|| Box::new(else_if_without_else::ElseIfWithoutElse));
     store.register_early_pass(|| Box::new(int_plus_one::IntPlusOne));

tests/ui/print_in_format_impl.rs

@@ -0,0 +1,58 @@
+#![allow(unused, clippy::print_literal, clippy::write_literal)]
+#![warn(clippy::print_in_format_impl)]
+use std::fmt::{Debug, Display, Error, Formatter};
+
+macro_rules! indirect {
+    () => {{ println!() }};
+}
+
+macro_rules! nested {
+    ($($tt:tt)*) => {
+        $($tt)*
+    };
+}
+
+struct Foo;
+impl Debug for Foo {
+    fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
+        static WORKS_WITH_NESTED_ITEMS: bool = true;
+
+        print!("{}", 1);
+        println!("{}", 2);
+        eprint!("{}", 3);
+        eprintln!("{}", 4);
+        nested! {
+            println!("nested");
+        };
+
+        write!(f, "{}", 5);
+        writeln!(f, "{}", 6);
+        indirect!();
+
+        Ok(())
+    }
+}
+
+impl Display for Foo {
+    fn fmt(&self, f: &mut Formatter) -> Result<(), Error> {
+        print!("Display");
+        write!(f, "Display");
+
+        Ok(())
+    }
+}
+
+struct UnnamedFormatter;
+impl Debug for UnnamedFormatter {
+    fn fmt(&self, _: &mut Formatter) -> Result<(), Error> {
+        println!("UnnamedFormatter");
+        Ok(())
+    }
+}
+
+fn main() {
+    print!("outside fmt");
+    println!("outside fmt");
+    eprint!("outside fmt");
+    eprintln!("outside fmt");
+}