Add print_in_fmt lint

CHANGELOG.md

@@ -3193,6 +3193,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_fmt`]: https://rust-lang.github.io/rust-clippy/master/index.html#print_in_fmt
 [`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

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 450 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 500 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
 You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.

clippy_lints/src/lib.register_all.rs

@@ -228,6 +228,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL),
     LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL),
     LintId::of(precedence::PRECEDENCE),
+    LintId::of(print_in_fmt::PRINT_IN_FMT),
     LintId::of(ptr::CMP_NULL),
     LintId::of(ptr::INVALID_NULL_PTR_USAGE),
     LintId::of(ptr::MUT_FROM_REF),

clippy_lints/src/lib.register_lints.rs

@@ -399,6 +399,7 @@ store.register_lints(&[
     path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE,
     pattern_type_mismatch::PATTERN_TYPE_MISMATCH,
     precedence::PRECEDENCE,
+    print_in_fmt::PRINT_IN_FMT,
     ptr::CMP_NULL,
     ptr::INVALID_NULL_PTR_USAGE,
     ptr::MUT_FROM_REF,

clippy_lints/src/lib.register_suspicious.rs

@@ -16,6 +16,7 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(methods::SUSPICIOUS_MAP),
     LintId::of(mut_key::MUTABLE_KEY_TYPE),
     LintId::of(octal_escapes::OCTAL_ESCAPES),
+    LintId::of(print_in_fmt::PRINT_IN_FMT),
     LintId::of(return_self_not_must_use::RETURN_SELF_NOT_MUST_USE),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),

clippy_lints/src/lib.rs

@@ -327,6 +327,7 @@ mod pass_by_ref_or_value;
 mod path_buf_push_overwrite;
 mod pattern_type_mismatch;
 mod precedence;
+mod print_in_fmt;
 mod ptr;
 mod ptr_eq;
 mod ptr_offset_with_cast;
@@ -860,6 +861,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(return_self_not_must_use::ReturnSelfNotMustUse));
     store.register_late_pass(|| Box::new(init_numbered_fields::NumberedFields));
     store.register_early_pass(|| Box::new(single_char_lifetime_names::SingleCharLifetimeNames));
+    store.register_late_pass(|| Box::new(print_in_fmt::PrintInFmt::default()));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/print_in_fmt.rs

@@ -0,0 +1,118 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::macros::root_macro_call_first_node;
+use clippy_utils::{get_parent_as_impl, match_any_diagnostic_items};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, Impl, ImplItem, ImplItemKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{sym, Symbol};
+
+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?
+    /// Printing during a formatting trait impl is likely unintentional. It can
+    /// cause output to be split between the wrong streams. If `format!` is used
+    /// text would go to stdout/stderr even if not desired.
+    ///
+    /// ### 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.59.0"]
+    pub PRINT_IN_FMT,
+    suspicious,
+    "use of a print macro in a formatting trait impl"
+}
+
+struct FmtImpl {
+    trait_name: Symbol,
+    formatter_name: Option<Symbol>,
+}
+
+#[derive(Default)]
+pub struct PrintInFmt {
+    fmt_impl: Option<FmtImpl>,
+}
+
+impl_lint_pass!(PrintInFmt => [PRINT_IN_FMT]);
+
+impl LateLintPass<'_> for PrintInFmt {
+    fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) {
+        let formatting_traits = [sym::Display, sym::Debug];
+
+        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(trait_def_id) = trait_ref.trait_def_id();
+            if let Some(index) = match_any_diagnostic_items(cx, trait_def_id, &formatting_traits);
+            then {
+                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);
+
+                self.fmt_impl = Some(FmtImpl {
+                    formatter_name,
+                    trait_name: formatting_traits[index],
+                });
+            }
+        };
+    }
+
+    fn check_impl_item_post(&mut self, _: &LateContext<'_>, _: &ImplItem<'_>) {
+        self.fmt_impl = None;
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if let Some(fmt_impl) = &self.fmt_impl {
+            if let Some(macro_call) = root_macro_call_first_node(cx, expr) {
+                let name = cx.tcx.item_name(macro_call.def_id);
+                let replacement = match name.as_str() {
+                    "print" | "eprint" => "write",
+                    "println" | "eprintln" => "writeln",
+                    _ => return,
+                };
+
+                span_lint_and_sugg(
+                    cx,
+                    PRINT_IN_FMT,
+                    macro_call.span,
+                    &format!("use of `{}!` in `{}` impl", name.as_str(), fmt_impl.trait_name),
+                    "replace with",
+                    if let Some(formatter_name) = fmt_impl.formatter_name {
+                        format!("{}!({}, ..)", replacement, formatter_name)
+                    } else {
+                        format!("{}!(..)", replacement)
+                    },
+                    Applicability::HasPlaceholders,
+                );
+            }
+        }
+    }
+}

tests/ui/print_in_fmt.rs

@@ -0,0 +1,56 @@
+#![allow(unused, clippy::print_literal, clippy::write_literal)]
+#![warn(clippy::print_in_fmt)]
+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> {
+        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");
+}

tests/ui/print_in_fmt.stderr

@@ -0,0 +1,46 @@
+error: use of `print!` in `Debug` impl
+  --> $DIR/print_in_fmt.rs:18:9
+   |
+LL |         print!("{}", 1);
+   |         ^^^^^^^^^^^^^^^ help: replace with: `write!(f, ..)`
+   |
+   = note: `-D clippy::print-in-fmt` implied by `-D warnings`
+
+error: use of `println!` in `Debug` impl
+  --> $DIR/print_in_fmt.rs:19:9
+   |
+LL |         println!("{}", 2);
+   |         ^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(f, ..)`
+
+error: use of `eprint!` in `Debug` impl
+  --> $DIR/print_in_fmt.rs:20:9
+   |
+LL |         eprint!("{}", 3);
+   |         ^^^^^^^^^^^^^^^^ help: replace with: `write!(f, ..)`
+
+error: use of `eprintln!` in `Debug` impl
+  --> $DIR/print_in_fmt.rs:21:9
+   |
+LL |         eprintln!("{}", 4);
+   |         ^^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(f, ..)`
+
+error: use of `println!` in `Debug` impl
+  --> $DIR/print_in_fmt.rs:23:13
+   |
+LL |             println!("nested");
+   |             ^^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(f, ..)`
+
+error: use of `print!` in `Display` impl
+  --> $DIR/print_in_fmt.rs:36:9
+   |
+LL |         print!("Display");
+   |         ^^^^^^^^^^^^^^^^^ help: replace with: `write!(f, ..)`
+
+error: use of `println!` in `Debug` impl
+  --> $DIR/print_in_fmt.rs:46:9
+   |
+LL |         println!("UnnamedFormatter");
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(..)`
+
+error: aborting due to 7 previous errors
+