From b6122b4451a522ddd79480867fa459c58d33cb26 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 11 Feb 2024 03:11:19 -0500 Subject: [PATCH] Refactor into a struct --- clippy_lints/src/format_impl.rs | 210 +++++++++--------- .../format_args_unfixable.stderr | 102 --------- .../uninlined_format_args.stderr | 1 + .../unused_format_specs.fixed | 53 ----- .../unused_format_specs.stderr | 156 ------------- .../toml_unknown_key/conf_unknown_key.stderr | 2 + 6 files changed, 111 insertions(+), 413 deletions(-) delete mode 100644 tests/ui-toml/include_custom_format_macros/format_args_unfixable.stderr delete mode 100644 tests/ui-toml/include_custom_format_macros/unused_format_specs.fixed delete mode 100644 tests/ui-toml/include_custom_format_macros/unused_format_specs.stderr diff --git a/clippy_lints/src/format_impl.rs b/clippy_lints/src/format_impl.rs index aa9d4145f2f7..8c3fd111bfaf 100644 --- a/clippy_lints/src/format_impl.rs +++ b/clippy_lints/src/format_impl.rs @@ -7,7 +7,7 @@ use rustc_hir::{Expr, ExprKind, Impl, ImplItem, ImplItemKind, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; use rustc_span::symbol::kw; -use rustc_span::{sym, Span, Symbol}; +use rustc_span::{sym, Symbol}; declare_clippy_lint! { /// ### What it does @@ -121,128 +121,134 @@ impl<'tcx> LateLintPass<'tcx> for FormatImpl { } fn check_impl_item_post(&mut self, cx: &LateContext<'_>, impl_item: &ImplItem<'_>) { - // Assume no nested Impl of Debug and Display within eachother + // Assume no nested Impl of Debug and Display within each other 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<'_>) { - 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); + if let Some(format_trait_impl) = self.format_trait_impl { + let linter = FormatImplExpr { + cx, + expr, + format_trait_impl, + include_custom: self.include_custom, + }; + linter.check_to_string_in_display(); + linter.check_self_in_format_args(); + linter.check_print_in_format_impl(); } - - check_self_in_format_args(cx, expr, format_trait_impl, self.include_custom); - check_print_in_format_impl(cx, expr, format_trait_impl); } } -fn check_to_string_in_display(cx: &LateContext<'_>, expr: &Expr<'_>) { - if let ExprKind::MethodCall(path, self_arg, ..) = expr.kind - // Get the hir_id of the object we are calling the method on - // Is the method to_string() ? - && path.ident.name == sym::to_string - // Is the method a part of the ToString trait? (i.e. not to_string() implemented - // separately) - && let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) - && is_diag_trait_item(cx, expr_def_id, sym::ToString) - // Is the method is called on self - && let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind - && let [segment] = path.segments - && segment.ident.name == kw::SelfLower - { - span_lint( - cx, - RECURSIVE_FORMAT_IMPL, - expr.span, - "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion", - ); - } +struct FormatImplExpr<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + format_trait_impl: FormatTraitNames, + include_custom: bool, } -fn check_self_in_format_args<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'_>, - impl_trait: FormatTraitNames, - include_custom: bool, -) { - // Check each arg in format calls - do we ever use Display on self (directly or via deref)? - if let Some(outer_macro) = root_macro_call_first_node(cx, expr) - && let macro_def_id = outer_macro.def_id - && (include_custom || is_format_macro(cx, macro_def_id)) - && let Some(format_args) = find_format_args(cx, expr, outer_macro.expn) - { - for piece in &format_args.template { - if let FormatArgsPiece::Placeholder(placeholder) = piece - && let trait_name = match placeholder.format_trait { - FormatTrait::Display => sym::Display, - FormatTrait::Debug => sym::Debug, - FormatTrait::LowerExp => sym!(LowerExp), - FormatTrait::UpperExp => sym!(UpperExp), - FormatTrait::Octal => sym!(Octal), - FormatTrait::Pointer => sym::Pointer, - FormatTrait::Binary => sym!(Binary), - FormatTrait::LowerHex => sym!(LowerHex), - FormatTrait::UpperHex => sym!(UpperHex), +impl<'a, 'tcx> FormatImplExpr<'a, 'tcx> { + fn check_to_string_in_display(&self) { + if self.format_trait_impl.name == sym::Display + && let ExprKind::MethodCall(path, self_arg, ..) = self.expr.kind + // Get the hir_id of the object we are calling the method on + // Is the method to_string() ? + && path.ident.name == sym::to_string + // Is the method a part of the ToString trait? (i.e. not to_string() implemented + // separately) + && let Some(expr_def_id) = self.cx.typeck_results().type_dependent_def_id(self.expr.hir_id) + && is_diag_trait_item(self.cx, expr_def_id, sym::ToString) + // Is the method is called on self + && let ExprKind::Path(QPath::Resolved(_, path)) = self_arg.kind + && let [segment] = path.segments + && segment.ident.name == kw::SelfLower + { + span_lint( + self.cx, + RECURSIVE_FORMAT_IMPL, + self.expr.span, + "using `self.to_string` in `fmt::Display` implementation will cause infinite recursion", + ); + } + } + + fn check_self_in_format_args(&self) { + // Check each arg in format calls - do we ever use Display on self (directly or via deref)? + if let Some(outer_macro) = root_macro_call_first_node(self.cx, self.expr) + && let macro_def_id = outer_macro.def_id + && (self.include_custom || is_format_macro(self.cx, macro_def_id)) + && let Some(format_args) = find_format_args(self.cx, self.expr, outer_macro.expn) + { + for piece in &format_args.template { + if let FormatArgsPiece::Placeholder(placeholder) = piece + && let trait_name = match placeholder.format_trait { + FormatTrait::Display => sym::Display, + FormatTrait::Debug => sym::Debug, + FormatTrait::LowerExp => sym!(LowerExp), + FormatTrait::UpperExp => sym!(UpperExp), + FormatTrait::Octal => sym!(Octal), + FormatTrait::Pointer => sym::Pointer, + FormatTrait::Binary => sym!(Binary), + FormatTrait::LowerHex => sym!(LowerHex), + FormatTrait::UpperHex => sym!(UpperHex), + } + && trait_name == self.format_trait_impl.name + && let Ok(index) = placeholder.argument.index + && let Some(arg) = format_args.arguments.all_args().get(index) + && let Ok(arg_expr) = find_format_arg_expr(self.expr, arg) + { + self.check_format_arg_self(arg_expr); } - && trait_name == impl_trait.name - && let Ok(index) = placeholder.argument.index - && let Some(arg) = format_args.arguments.all_args().get(index) - && let Ok(arg_expr) = find_format_arg_expr(expr, arg) - { - check_format_arg_self(cx, expr.span, arg_expr, impl_trait); } } } -} -fn check_format_arg_self(cx: &LateContext<'_>, span: Span, arg: &Expr<'_>, impl_trait: FormatTraitNames) { - // Handle multiple dereferencing of references e.g. &&self - // Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl) - // Since the argument to fmt is itself a reference: &self - let reference = peel_ref_operators(cx, arg); - let map = cx.tcx.hir(); - // Is the reference self? - if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) { - let FormatTraitNames { name, .. } = impl_trait; - span_lint( - cx, - RECURSIVE_FORMAT_IMPL, - span, - &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"), - ); + fn check_format_arg_self(&self, arg: &Expr<'_>) { + // Handle multiple dereferencing of references e.g. &&self + // Handle dereference of &self -> self that is equivalent (i.e. via *self in fmt() impl) + // Since the argument to fmt is itself a reference: &self + let reference = peel_ref_operators(self.cx, arg); + let map = self.cx.tcx.hir(); + // Is the reference self? + if path_to_local(reference).map(|x| map.name(x)) == Some(kw::SelfLower) { + let FormatTraitNames { name, .. } = self.format_trait_impl; + span_lint( + self.cx, + RECURSIVE_FORMAT_IMPL, + self.expr.span, + &format!("using `self` as `{name}` in `impl {name}` will cause infinite recursion"), + ); + } } -} -fn check_print_in_format_impl(cx: &LateContext<'_>, expr: &Expr<'_>, impl_trait: FormatTraitNames) { - if let Some(macro_call) = root_macro_call_first_node(cx, expr) - && let Some(name) = cx.tcx.get_diagnostic_name(macro_call.def_id) - { - let replacement = match name { - sym::print_macro | sym::eprint_macro => "write", - sym::println_macro | sym::eprintln_macro => "writeln", - _ => return, - }; + fn check_print_in_format_impl(&self) { + if let Some(macro_call) = root_macro_call_first_node(self.cx, self.expr) + && let Some(name) = self.cx.tcx.get_diagnostic_name(macro_call.def_id) + { + 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(); + let name = name.as_str().strip_suffix("_macro").unwrap(); - span_lint_and_sugg( - cx, - PRINT_IN_FORMAT_IMPL, - macro_call.span, - &format!("use of `{name}!` in `{}` impl", impl_trait.name), - "replace with", - if let Some(formatter_name) = impl_trait.formatter_name { - format!("{replacement}!({formatter_name}, ..)") - } else { - format!("{replacement}!(..)") - }, - Applicability::HasPlaceholders, - ); + span_lint_and_sugg( + self.cx, + PRINT_IN_FORMAT_IMPL, + macro_call.span, + &format!("use of `{name}!` in `{}` impl", self.format_trait_impl.name), + "replace with", + if let Some(formatter_name) = self.format_trait_impl.formatter_name { + format!("{replacement}!({formatter_name}, ..)") + } else { + format!("{replacement}!(..)") + }, + Applicability::HasPlaceholders, + ); + } } } diff --git a/tests/ui-toml/include_custom_format_macros/format_args_unfixable.stderr b/tests/ui-toml/include_custom_format_macros/format_args_unfixable.stderr deleted file mode 100644 index 3f1ff2315c12..000000000000 --- a/tests/ui-toml/include_custom_format_macros/format_args_unfixable.stderr +++ /dev/null @@ -1,102 +0,0 @@ -error: `format!` in `my_println2!` args - --> $DIR/format_args_unfixable.rs:33:5 - | -LL | my_println2!(true, "error: {}", format!("something failed at {}", Location::caller())); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: combine the `format!(..)` arguments with the outer `my_println2!(..)` call - = help: or consider changing `format!` to `format_args!` - = note: `-D clippy::format-in-format-args` implied by `-D warnings` - -error: `format!` in `my_println2!` args - --> $DIR/format_args_unfixable.rs:34:5 - | -LL | / my_println2!( -LL | | true, -LL | | "{}: {}", -LL | | error, -LL | | format!("something failed at {}", Location::caller()) -LL | | ); - | |_____^ - | - = help: combine the `format!(..)` arguments with the outer `my_println2!(..)` call - = help: or consider changing `format!` to `format_args!` - -error: `format!` in `my_println2!` args - --> $DIR/format_args_unfixable.rs:41:5 - | -LL | / my_println2!( -LL | | true, -LL | | "error: {}", -LL | | format!("something failed at {}", Location::caller().to_string()) -LL | | ); - | |_____^ - | - = help: combine the `format!(..)` arguments with the outer `my_println2!(..)` call - = help: or consider changing `format!` to `format_args!` - -error: `to_string` applied to a type that implements `Display` in `format!` args - --> $DIR/format_args_unfixable.rs:44:61 - | -LL | format!("something failed at {}", Location::caller().to_string()) - | ^^^^^^^^^^^^ help: remove this - | - = note: `-D clippy::to-string-in-format-args` implied by `-D warnings` - -error: `format!` in `my_println2!` args - --> $DIR/format_args_unfixable.rs:46:5 - | -LL | / my_println2!( -LL | | true, -LL | | "{}: {}", -LL | | error, -LL | | format!("something failed at {}", Location::caller().to_string()) -LL | | ); - | |_____^ - | - = help: combine the `format!(..)` arguments with the outer `my_println2!(..)` call - = help: or consider changing `format!` to `format_args!` - -error: `to_string` applied to a type that implements `Display` in `format!` args - --> $DIR/format_args_unfixable.rs:50:61 - | -LL | format!("something failed at {}", Location::caller().to_string()) - | ^^^^^^^^^^^^ help: remove this - -error: `to_string` applied to a type that implements `Display` in `my_println2!` args - --> $DIR/format_args_unfixable.rs:53:55 - | -LL | my_println2!(true, "error: {}", Location::caller().to_string()); - | ^^^^^^^^^^^^ help: remove this - -error: `to_string` applied to a type that implements `Display` in `my_println2!` args - --> $DIR/format_args_unfixable.rs:54:59 - | -LL | my_println2!(true, "{}: {}", error, Location::caller().to_string()); - | ^^^^^^^^^^^^ help: remove this - -error: `format!` in `my_println2_args!` args - --> $DIR/format_args_unfixable.rs:56:5 - | -LL | my_println2_args!(true, "error: {}", format!("something failed at {}", Location::caller())); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: combine the `format!(..)` arguments with the outer `my_println2_args!(..)` call - = help: or consider changing `format!` to `format_args!` - -error: `format!` in `my_println2_args!` args - --> $DIR/format_args_unfixable.rs:57:5 - | -LL | / my_println2_args!( -LL | | true, -LL | | "{}: {}", -LL | | error, -LL | | format!("something failed at {}", Location::caller()) -LL | | ); - | |_____^ - | - = help: combine the `format!(..)` arguments with the outer `my_println2_args!(..)` call - = help: or consider changing `format!` to `format_args!` - -error: aborting due to 10 previous errors - diff --git a/tests/ui-toml/include_custom_format_macros/uninlined_format_args.stderr b/tests/ui-toml/include_custom_format_macros/uninlined_format_args.stderr index d2e3dd030e2a..7ce7d8e33fab 100644 --- a/tests/ui-toml/include_custom_format_macros/uninlined_format_args.stderr +++ b/tests/ui-toml/include_custom_format_macros/uninlined_format_args.stderr @@ -5,6 +5,7 @@ LL | println!("val='{}'", local_i32); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::uninlined-format-args` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::uninlined_format_args)]` help: change this to | LL - println!("val='{}'", local_i32); diff --git a/tests/ui-toml/include_custom_format_macros/unused_format_specs.fixed b/tests/ui-toml/include_custom_format_macros/unused_format_specs.fixed deleted file mode 100644 index fc5ebb22806c..000000000000 --- a/tests/ui-toml/include_custom_format_macros/unused_format_specs.fixed +++ /dev/null @@ -1,53 +0,0 @@ -// run-rustfix - -#![warn(clippy::unused_format_specs)] -#![allow(unused)] - -macro_rules! _internal { - ($($args:tt)*) => { - println!("{}", format_args!($($args)*)) - }; -} - -macro_rules! my_println2 { - ($target:expr, $($args:tt)+) => {{ - if $target { - _internal!($($args)+) - } - }}; -} - -macro_rules! my_println2_args { - ($target:expr, $($args:tt)+) => {{ - if $target { - _internal!("foo: {}", format_args!($($args)+)) - } - }}; -} - -fn main() { - let f = 1.0f64; - println!("{:.}", 1.0); - println!("{f:.} {f:.?}"); - println!("{:.}", 1); - - my_println2!(true, "{:.}", 1.0); - my_println2!(true, "{f:.} {f:.?}"); - my_println2!(true, "{:.}", 1); - - my_println2_args!(true, "{:.}", 1.0); - my_println2_args!(true, "{f:.} {f:.?}"); - my_println2_args!(true, "{:.}", 1); -} - -fn should_not_lint() { - let f = 1.0f64; - println!("{:.1}", 1.0); - println!("{f:.w$} {f:.*?}", 3, w = 2); - - my_println2!(true, "{:.1}", 1.0); - my_println2!(true, "{f:.w$} {f:.*?}", 3, w = 2); - - my_println2_args!(true, "{:.1}", 1.0); - my_println2_args!(true, "{f:.w$} {f:.*?}", 3, w = 2); -} diff --git a/tests/ui-toml/include_custom_format_macros/unused_format_specs.stderr b/tests/ui-toml/include_custom_format_macros/unused_format_specs.stderr deleted file mode 100644 index 076116b75068..000000000000 --- a/tests/ui-toml/include_custom_format_macros/unused_format_specs.stderr +++ /dev/null @@ -1,156 +0,0 @@ -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:30:17 - | -LL | println!("{:.}", 1.0); - | ^ - | - = note: a precision specifier is not required to format floats - = note: `-D clippy::unused-format-specs` implied by `-D warnings` -help: remove the `.` - | -LL - println!("{:.}", 1.0); -LL + println!("{}", 1.0); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:31:18 - | -LL | println!("{f:.} {f:.?}"); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - println!("{f:.} {f:.?}"); -LL + println!("{f} {f:.?}"); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:31:24 - | -LL | println!("{f:.} {f:.?}"); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - println!("{f:.} {f:.?}"); -LL + println!("{f:.} {f:?}"); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:32:17 - | -LL | println!("{:.}", 1); - | ^ - | -help: remove the `.` - | -LL - println!("{:.}", 1); -LL + println!("{}", 1); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:34:27 - | -LL | my_println2!(true, "{:.}", 1.0); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - my_println2!(true, "{:.}", 1.0); -LL + my_println2!(true, "{}", 1.0); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:35:28 - | -LL | my_println2!(true, "{f:.} {f:.?}"); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - my_println2!(true, "{f:.} {f:.?}"); -LL + my_println2!(true, "{f} {f:.?}"); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:35:34 - | -LL | my_println2!(true, "{f:.} {f:.?}"); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - my_println2!(true, "{f:.} {f:.?}"); -LL + my_println2!(true, "{f:.} {f:?}"); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:36:27 - | -LL | my_println2!(true, "{:.}", 1); - | ^ - | -help: remove the `.` - | -LL - my_println2!(true, "{:.}", 1); -LL + my_println2!(true, "{}", 1); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:38:32 - | -LL | my_println2_args!(true, "{:.}", 1.0); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - my_println2_args!(true, "{:.}", 1.0); -LL + my_println2_args!(true, "{}", 1.0); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:39:33 - | -LL | my_println2_args!(true, "{f:.} {f:.?}"); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - my_println2_args!(true, "{f:.} {f:.?}"); -LL + my_println2_args!(true, "{f} {f:.?}"); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:39:39 - | -LL | my_println2_args!(true, "{f:.} {f:.?}"); - | ^ - | - = note: a precision specifier is not required to format floats -help: remove the `.` - | -LL - my_println2_args!(true, "{f:.} {f:.?}"); -LL + my_println2_args!(true, "{f:.} {f:?}"); - | - -error: empty precision specifier has no effect - --> $DIR/unused_format_specs.rs:40:32 - | -LL | my_println2_args!(true, "{:.}", 1); - | ^ - | -help: remove the `.` - | -LL - my_println2_args!(true, "{:.}", 1); -LL + my_println2_args!(true, "{}", 1); - | - -error: aborting due to 12 previous errors - diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index ef1d70693535..dc26bb82fdb5 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -120,6 +120,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect excessive-nesting-threshold future-size-threshold ignore-interior-mutability + include-custom-format-macros large-error-threshold literal-representation-threshold matches-for-let-else @@ -199,6 +200,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni excessive-nesting-threshold future-size-threshold ignore-interior-mutability + include-custom-format-macros large-error-threshold literal-representation-threshold matches-for-let-else