diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b52a6fcd..35983b7fb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/format_impl.rs b/clippy_lints/src/format_impl.rs index 02b2d7219..ef8be9e87 100644 --- a/clippy_lints/src/format_impl.rs +++ b/clippy_lints/src/format_impl.rs @@ -1,26 +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 @@ -60,6 +47,55 @@ 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, +} + #[derive(Default)] pub struct FormatImpl { // Whether we are inside Display or Debug trait impl - None for neither @@ -74,33 +110,29 @@ impl FormatImpl { } } -impl_lint_pass!(FormatImpl => [RECURSIVE_FORMAT_IMPL]); +impl_lint_pass!(FormatImpl => [RECURSIVE_FORMAT_IMPL, PRINT_IN_FORMAT_IMPL]); impl<'tcx> LateLintPass<'tcx> for FormatImpl { - 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); - } + 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); } } @@ -139,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); @@ -155,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 { +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 { + 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 } diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index b2d9889ba..f6d467941 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -68,6 +68,7 @@ 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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 1243fc978..686482671 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -152,6 +152,7 @@ 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, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 6a8859e19..465baa825 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/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), diff --git a/tests/ui/print_in_format_impl.rs b/tests/ui/print_in_format_impl.rs new file mode 100644 index 000000000..64e886866 --- /dev/null +++ b/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"); +} diff --git a/tests/ui/print_in_format_impl.stderr b/tests/ui/print_in_format_impl.stderr new file mode 100644 index 000000000..63b7179bc --- /dev/null +++ b/tests/ui/print_in_format_impl.stderr @@ -0,0 +1,46 @@ +error: use of `print!` in `Debug` impl + --> $DIR/print_in_format_impl.rs:20:9 + | +LL | print!("{}", 1); + | ^^^^^^^^^^^^^^^ help: replace with: `write!(f, ..)` + | + = note: `-D clippy::print-in-format-impl` implied by `-D warnings` + +error: use of `println!` in `Debug` impl + --> $DIR/print_in_format_impl.rs:21:9 + | +LL | println!("{}", 2); + | ^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(f, ..)` + +error: use of `eprint!` in `Debug` impl + --> $DIR/print_in_format_impl.rs:22:9 + | +LL | eprint!("{}", 3); + | ^^^^^^^^^^^^^^^^ help: replace with: `write!(f, ..)` + +error: use of `eprintln!` in `Debug` impl + --> $DIR/print_in_format_impl.rs:23:9 + | +LL | eprintln!("{}", 4); + | ^^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(f, ..)` + +error: use of `println!` in `Debug` impl + --> $DIR/print_in_format_impl.rs:25:13 + | +LL | println!("nested"); + | ^^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(f, ..)` + +error: use of `print!` in `Display` impl + --> $DIR/print_in_format_impl.rs:38:9 + | +LL | print!("Display"); + | ^^^^^^^^^^^^^^^^^ help: replace with: `write!(f, ..)` + +error: use of `println!` in `Debug` impl + --> $DIR/print_in_format_impl.rs:48:9 + | +LL | println!("UnnamedFormatter"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `writeln!(..)` + +error: aborting due to 7 previous errors +