diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/format_impl.rs (renamed from clippy_lints/src/recursive_format_impl.rs) | 167 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_all.rs | 3 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_correctness.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_lints.rs | 3 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_suspicious.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 4 | ||||
| -rw-r--r-- | tests/ui/print_in_format_impl.rs | 58 | ||||
| -rw-r--r-- | tests/ui/print_in_format_impl.stderr | 46 |
9 files changed, 228 insertions, 57 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b52a6fcd05..35983b7fb50 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/recursive_format_impl.rs b/clippy_lints/src/format_impl.rs index 5bb9740749b..ef8be9e878f 100644 --- a/clippy_lints/src/recursive_format_impl.rs +++ b/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,13 +47,62 @@ 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, @@ -75,33 +110,29 @@ impl RecursiveFormatImpl { } } -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 } diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index c6f8470cd7d..f6d467941e3 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_correctness.rs b/clippy_lints/src/lib.register_correctness.rs index 18fe44282ed..35b1e644a8a 100644 --- a/clippy_lints/src/lib.register_correctness.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 75ef1b0a9d5..686482671b4 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 6a8859e19d7..465baa82581 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/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a21a87899aa..230e2b2ccdf 100644 --- a/clippy_lints/src/lib.rs +++ b/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)); diff --git a/tests/ui/print_in_format_impl.rs b/tests/ui/print_in_format_impl.rs new file mode 100644 index 00000000000..64e88686609 --- /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 00000000000..63b7179bca7 --- /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 + |