diff --git a/CHANGELOG.md b/CHANGELOG.md index 83fd8396bc72..9387e62d0dd0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 1bbd89e7822e..de72e8d124d4 100644 --- a/README.md +++ b/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. diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 944411087e95..84c96925b5b1 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index f9241d943b2c..cd8e6c3f1865 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 8594338ffa5a..e29aab1a0897 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/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), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c31acb5f4ef8..fefb222b66f4 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/clippy_lints/src/print_in_fmt.rs b/clippy_lints/src/print_in_fmt.rs new file mode 100644 index 000000000000..e44a3b7d7820 --- /dev/null +++ b/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, +} + +#[derive(Default)] +pub struct PrintInFmt { + fmt_impl: Option, +} + +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, + ); + } + } + } +} diff --git a/tests/ui/print_in_fmt.rs b/tests/ui/print_in_fmt.rs new file mode 100644 index 000000000000..07ed4bef85fb --- /dev/null +++ b/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"); +} diff --git a/tests/ui/print_in_fmt.stderr b/tests/ui/print_in_fmt.stderr new file mode 100644 index 000000000000..7323b91a3625 --- /dev/null +++ b/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 +