diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e0ff5db0ee..8e7ac644b9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4316,6 +4316,7 @@ Released 2018-09-13 [`unstable_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_slice [`unused_async`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_async [`unused_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_collect +[`unused_format_specs`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_format_specs [`unused_io_amount`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_io_amount [`unused_label`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_label [`unused_peekable`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_peekable diff --git a/clippy_lints/src/format_args.rs b/clippy_lints/src/format_args.rs index 4f06fc8fa5a..4c4a1e06cd4 100644 --- a/clippy_lints/src/format_args.rs +++ b/clippy_lints/src/format_args.rs @@ -1,8 +1,10 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred}; -use clippy_utils::macros::{is_format_macro, is_panic, FormatArgsExpn, FormatParam, FormatParamUsage}; +use clippy_utils::macros::{ + is_format_macro, is_panic, root_macro_call, Count, FormatArg, FormatArgsExpn, FormatParam, FormatParamUsage, +}; use clippy_utils::source::snippet_opt; -use clippy_utils::ty::implements_trait; +use clippy_utils::ty::{implements_trait, is_type_diagnostic_item}; use clippy_utils::{is_diag_trait_item, meets_msrv, msrvs}; use if_chain::if_chain; use itertools::Itertools; @@ -117,7 +119,43 @@ declare_clippy_lint! { "using non-inlined variables in `format!` calls" } -impl_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, UNINLINED_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]); +declare_clippy_lint! { + /// ### What it does + /// Detects [formatting parameters] that have no effect on the output of + /// `format!()`, `println!()` or similar macros. + /// + /// ### Why is this bad? + /// Shorter format specifiers are easier to read, it may also indicate that + /// an expected formatting operation such as adding padding isn't happening. + /// + /// ### Example + /// ```rust + /// println!("{:.}", 1.0); + /// + /// println!("not padded: {:5}", format_args!("...")); + /// ``` + /// Use instead: + /// ```rust + /// println!("{}", 1.0); + /// + /// println!("not padded: {}", format_args!("...")); + /// // OR + /// println!("padded: {:5}", format!("...")); + /// ``` + /// + /// [formatting parameters]: https://doc.rust-lang.org/std/fmt/index.html#formatting-parameters + #[clippy::version = "1.66.0"] + pub UNUSED_FORMAT_SPECS, + complexity, + "use of a format specifier that has no effect" +} + +impl_lint_pass!(FormatArgs => [ + FORMAT_IN_FORMAT_ARGS, + TO_STRING_IN_FORMAT_ARGS, + UNINLINED_FORMAT_ARGS, + UNUSED_FORMAT_SPECS, +]); pub struct FormatArgs { msrv: Option, @@ -132,27 +170,26 @@ impl FormatArgs { impl<'tcx> LateLintPass<'tcx> for FormatArgs { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if_chain! { - if let Some(format_args) = FormatArgsExpn::parse(cx, expr); - let expr_expn_data = expr.span.ctxt().outer_expn_data(); - let outermost_expn_data = outermost_expn_data(expr_expn_data); - if let Some(macro_def_id) = outermost_expn_data.macro_def_id; - if is_format_macro(cx, macro_def_id); - if let ExpnKind::Macro(_, name) = outermost_expn_data.kind; - then { - for arg in &format_args.args { - if !arg.format.is_default() { - continue; - } - if is_aliased(&format_args, arg.param.value.hir_id) { - continue; - } - check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg.param.value); - check_to_string_in_format_args(cx, name, arg.param.value); + if let Some(format_args) = FormatArgsExpn::parse(cx, expr) + && let expr_expn_data = expr.span.ctxt().outer_expn_data() + && let outermost_expn_data = outermost_expn_data(expr_expn_data) + && let Some(macro_def_id) = outermost_expn_data.macro_def_id + && is_format_macro(cx, macro_def_id) + && let ExpnKind::Macro(_, name) = outermost_expn_data.kind + { + for arg in &format_args.args { + check_unused_format_specifier(cx, arg); + if !arg.format.is_default() { + continue; } - if meets_msrv(self.msrv, msrvs::FORMAT_ARGS_CAPTURE) { - check_uninlined_args(cx, &format_args, outermost_expn_data.call_site, macro_def_id); + if is_aliased(&format_args, arg.param.value.hir_id) { + continue; } + check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg.param.value); + check_to_string_in_format_args(cx, name, arg.param.value); + } + if meets_msrv(self.msrv, msrvs::FORMAT_ARGS_CAPTURE) { + check_uninlined_args(cx, &format_args, outermost_expn_data.call_site, macro_def_id); } } } @@ -160,6 +197,76 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs { extract_msrv_attr!(LateContext); } +fn check_unused_format_specifier(cx: &LateContext<'_>, arg: &FormatArg<'_>) { + let param_ty = cx.typeck_results().expr_ty(arg.param.value).peel_refs(); + + if let Count::Implied(Some(mut span)) = arg.format.precision + && !span.is_empty() + { + span_lint_and_then( + cx, + UNUSED_FORMAT_SPECS, + span, + "empty precision specifier has no effect", + |diag| { + if param_ty.is_floating_point() { + diag.note("a precision specifier is not required to format floats"); + } + + if arg.format.is_default() { + // If there's no other specifiers remove the `:` too + span = arg.format_span(); + } + + diag.span_suggestion_verbose(span, "remove the `.`", "", Applicability::MachineApplicable); + }, + ); + } + + if is_type_diagnostic_item(cx, param_ty, sym::Arguments) && !arg.format.is_default_for_trait() { + span_lint_and_then( + cx, + UNUSED_FORMAT_SPECS, + arg.span, + "format specifiers have no effect on `format_args!()`", + |diag| { + let mut suggest_format = |spec, span| { + let message = format!("for the {spec} to apply consider using `format!()`"); + + if let Some(mac_call) = root_macro_call(arg.param.value.span) + && cx.tcx.is_diagnostic_item(sym::format_args_macro, mac_call.def_id) + && arg.span.eq_ctxt(mac_call.span) + { + diag.span_suggestion( + cx.sess().source_map().span_until_char(mac_call.span, '!'), + message, + "format", + Applicability::MaybeIncorrect, + ); + } else if let Some(span) = span { + diag.span_help(span, message); + } + }; + + if !arg.format.width.is_implied() { + suggest_format("width", arg.format.width.span()); + } + + if !arg.format.precision.is_implied() { + suggest_format("precision", arg.format.precision.span()); + } + + diag.span_suggestion_verbose( + arg.format_span(), + "if the current behavior is intentional, remove the format specifiers", + "", + Applicability::MaybeIncorrect, + ); + }, + ); + } +} + fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_site: Span, def_id: DefId) { if args.format_string.span.from_expansion() { return; diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index b5f0e5fd7d6..3f575d1f96a 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -73,6 +73,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(format_args::FORMAT_IN_FORMAT_ARGS), LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS), LintId::of(format_args::UNINLINED_FORMAT_ARGS), + LintId::of(format_args::UNUSED_FORMAT_SPECS), LintId::of(format_impl::PRINT_IN_FORMAT_IMPL), LintId::of(format_impl::RECURSIVE_FORMAT_IMPL), LintId::of(formatting::POSSIBLE_MISSING_COMMA), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index e3849e5a626..8be9dc4baf1 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -13,6 +13,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(double_parens::DOUBLE_PARENS), LintId::of(explicit_write::EXPLICIT_WRITE), LintId::of(format::USELESS_FORMAT), + LintId::of(format_args::UNUSED_FORMAT_SPECS), LintId::of(functions::TOO_MANY_ARGUMENTS), LintId::of(int_plus_one::INT_PLUS_ONE), LintId::of(lifetimes::EXTRA_UNUSED_LIFETIMES), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c188e9057f1..39866adeaa8 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -164,6 +164,7 @@ store.register_lints(&[ format_args::FORMAT_IN_FORMAT_ARGS, format_args::TO_STRING_IN_FORMAT_ARGS, format_args::UNINLINED_FORMAT_ARGS, + format_args::UNUSED_FORMAT_SPECS, format_impl::PRINT_IN_FORMAT_IMPL, format_impl::RECURSIVE_FORMAT_IMPL, format_push_string::FORMAT_PUSH_STRING, diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs index 5a63c290a31..9a682fbe604 100644 --- a/clippy_utils/src/macros.rs +++ b/clippy_utils/src/macros.rs @@ -627,7 +627,7 @@ pub enum Count<'tcx> { /// `FormatParamKind::Numbered`. Param(FormatParam<'tcx>), /// Not specified. - Implied, + Implied(Option), } impl<'tcx> Count<'tcx> { @@ -638,8 +638,10 @@ impl<'tcx> Count<'tcx> { inner: Option, values: &FormatArgsValues<'tcx>, ) -> Option { + let span = inner.map(|inner| span_from_inner(values.format_string_span, inner)); + Some(match count { - rpf::Count::CountIs(val) => Self::Is(val, span_from_inner(values.format_string_span, inner?)), + rpf::Count::CountIs(val) => Self::Is(val, span?), rpf::Count::CountIsName(name, _) => Self::Param(FormatParam::new( FormatParamKind::Named(Symbol::intern(name)), usage, @@ -661,12 +663,12 @@ impl<'tcx> Count<'tcx> { inner?, values, )?), - rpf::Count::CountImplied => Self::Implied, + rpf::Count::CountImplied => Self::Implied(span), }) } pub fn is_implied(self) -> bool { - matches!(self, Count::Implied) + matches!(self, Count::Implied(_)) } pub fn param(self) -> Option> { @@ -675,6 +677,14 @@ impl<'tcx> Count<'tcx> { _ => None, } } + + pub fn span(self) -> Option { + match self { + Count::Is(_, span) => Some(span), + Count::Param(param) => Some(param.span), + Count::Implied(span) => span, + } + } } /// Specification for the formatting of an argument in the format string. See @@ -738,8 +748,13 @@ impl<'tcx> FormatSpec<'tcx> { /// Returns true if this format spec is unchanged from the default. e.g. returns true for `{}`, /// `{foo}` and `{2}`, but false for `{:?}`, `{foo:5}` and `{3:.5}` pub fn is_default(&self) -> bool { - self.r#trait == sym::Display - && self.width.is_implied() + self.r#trait == sym::Display && self.is_default_for_trait() + } + + /// Has no other formatting specifiers than setting the format trait. returns true for `{}`, + /// `{foo}`, `{:?}`, but false for `{foo:5}`, `{3:.5?}` + pub fn is_default_for_trait(&self) -> bool { + self.width.is_implied() && self.precision.is_implied() && self.align == Alignment::AlignUnknown && self.flags == 0 @@ -757,6 +772,22 @@ pub struct FormatArg<'tcx> { pub span: Span, } +impl<'tcx> FormatArg<'tcx> { + /// Span of the `:` and format specifiers + /// + /// ```ignore + /// format!("{:.}"), format!("{foo:.}") + /// ^^ ^^ + /// ``` + pub fn format_span(&self) -> Span { + let base = self.span.data(); + + // `base.hi` is `{...}|`, subtract 1 byte (the length of '}') so that it points before the closing + // brace `{...|}` + Span::new(self.param.span.hi(), base.hi - BytePos(1), base.ctxt, base.parent) + } +} + /// A parsed `format_args!` expansion. #[derive(Debug)] pub struct FormatArgsExpn<'tcx> { diff --git a/src/docs.rs b/src/docs.rs index b8b4286b488..293b509dc6e 100644 --- a/src/docs.rs +++ b/src/docs.rs @@ -558,6 +558,7 @@ docs! { "unseparated_literal_suffix", "unsound_collection_transmute", "unused_async", + "unused_format_specs", "unused_io_amount", "unused_peekable", "unused_rounding", diff --git a/src/docs/unused_format_specs.txt b/src/docs/unused_format_specs.txt new file mode 100644 index 00000000000..77be3a2fb17 --- /dev/null +++ b/src/docs/unused_format_specs.txt @@ -0,0 +1,24 @@ +### What it does +Detects [formatting parameters] that have no effect on the output of +`format!()`, `println!()` or similar macros. + +### Why is this bad? +Shorter format specifiers are easier to read, it may also indicate that +an expected formatting operation such as adding padding isn't happening. + +### Example +``` +println!("{:.}", 1.0); + +println!("not padded: {:5}", format_args!("...")); +``` +Use instead: +``` +println!("{}", 1.0); + +println!("not padded: {}", format_args!("...")); +// OR +println!("padded: {:5}", format!("...")); +``` + +[formatting parameters]: https://doc.rust-lang.org/std/fmt/index.html#formatting-parameters \ No newline at end of file diff --git a/tests/ui/unused_format_specs.fixed b/tests/ui/unused_format_specs.fixed new file mode 100644 index 00000000000..2930722b42d --- /dev/null +++ b/tests/ui/unused_format_specs.fixed @@ -0,0 +1,18 @@ +// run-rustfix + +#![warn(clippy::unused_format_specs)] +#![allow(unused)] + +fn main() { + let f = 1.0f64; + println!("{}", 1.0); + println!("{f} {f:?}"); + + println!("{}", 1); +} + +fn should_not_lint() { + let f = 1.0f64; + println!("{:.1}", 1.0); + println!("{f:.w$} {f:.*?}", 3, w = 2); +} diff --git a/tests/ui/unused_format_specs.rs b/tests/ui/unused_format_specs.rs new file mode 100644 index 00000000000..ee192a000d4 --- /dev/null +++ b/tests/ui/unused_format_specs.rs @@ -0,0 +1,18 @@ +// run-rustfix + +#![warn(clippy::unused_format_specs)] +#![allow(unused)] + +fn main() { + let f = 1.0f64; + println!("{:.}", 1.0); + println!("{f:.} {f:.?}"); + + println!("{:.}", 1); +} + +fn should_not_lint() { + let f = 1.0f64; + println!("{:.1}", 1.0); + println!("{f:.w$} {f:.*?}", 3, w = 2); +} diff --git a/tests/ui/unused_format_specs.stderr b/tests/ui/unused_format_specs.stderr new file mode 100644 index 00000000000..7231c17e74c --- /dev/null +++ b/tests/ui/unused_format_specs.stderr @@ -0,0 +1,54 @@ +error: empty precision specifier has no effect + --> $DIR/unused_format_specs.rs:8: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:9: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:9: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:11:17 + | +LL | println!("{:.}", 1); + | ^ + | +help: remove the `.` + | +LL - println!("{:.}", 1); +LL + println!("{}", 1); + | + +error: aborting due to 4 previous errors + diff --git a/tests/ui/unused_format_specs_unfixable.rs b/tests/ui/unused_format_specs_unfixable.rs new file mode 100644 index 00000000000..78601a3483d --- /dev/null +++ b/tests/ui/unused_format_specs_unfixable.rs @@ -0,0 +1,30 @@ +#![warn(clippy::unused_format_specs)] +#![allow(unused)] + +macro_rules! format_args_from_macro { + () => { + format_args!("from macro") + }; +} + +fn main() { + // prints `.`, not ` .` + println!("{:5}.", format_args!("")); + //prints `abcde`, not `abc` + println!("{:.3}", format_args!("abcde")); + + println!("{:5}.", format_args_from_macro!()); + + let args = format_args!(""); + println!("{args:5}"); +} + +fn should_not_lint() { + println!("{}", format_args!("")); + // Technically the same as `{}`, but the `format_args` docs specifically mention that you can use + // debug formatting so allow it + println!("{:?}", format_args!("")); + + let args = format_args!(""); + println!("{args}"); +} diff --git a/tests/ui/unused_format_specs_unfixable.stderr b/tests/ui/unused_format_specs_unfixable.stderr new file mode 100644 index 00000000000..9f1890282e6 --- /dev/null +++ b/tests/ui/unused_format_specs_unfixable.stderr @@ -0,0 +1,69 @@ +error: format specifiers have no effect on `format_args!()` + --> $DIR/unused_format_specs_unfixable.rs:12:15 + | +LL | println!("{:5}.", format_args!("")); + | ^^^^ + | + = note: `-D clippy::unused-format-specs` implied by `-D warnings` +help: for the width to apply consider using `format!()` + | +LL | println!("{:5}.", format!("")); + | ~~~~~~ +help: if the current behavior is intentional, remove the format specifiers + | +LL - println!("{:5}.", format_args!("")); +LL + println!("{}.", format_args!("")); + | + +error: format specifiers have no effect on `format_args!()` + --> $DIR/unused_format_specs_unfixable.rs:14:15 + | +LL | println!("{:.3}", format_args!("abcde")); + | ^^^^^ + | +help: for the precision to apply consider using `format!()` + | +LL | println!("{:.3}", format!("abcde")); + | ~~~~~~ +help: if the current behavior is intentional, remove the format specifiers + | +LL - println!("{:.3}", format_args!("abcde")); +LL + println!("{}", format_args!("abcde")); + | + +error: format specifiers have no effect on `format_args!()` + --> $DIR/unused_format_specs_unfixable.rs:16:15 + | +LL | println!("{:5}.", format_args_from_macro!()); + | ^^^^ + | +help: for the width to apply consider using `format!()` + --> $DIR/unused_format_specs_unfixable.rs:16:17 + | +LL | println!("{:5}.", format_args_from_macro!()); + | ^ +help: if the current behavior is intentional, remove the format specifiers + | +LL - println!("{:5}.", format_args_from_macro!()); +LL + println!("{}.", format_args_from_macro!()); + | + +error: format specifiers have no effect on `format_args!()` + --> $DIR/unused_format_specs_unfixable.rs:19:15 + | +LL | println!("{args:5}"); + | ^^^^^^^^ + | +help: for the width to apply consider using `format!()` + --> $DIR/unused_format_specs_unfixable.rs:19:21 + | +LL | println!("{args:5}"); + | ^ +help: if the current behavior is intentional, remove the format specifiers + | +LL - println!("{args:5}"); +LL + println!("{args}"); + | + +error: aborting due to 4 previous errors +