Auto merge of rust-lang#9637 - Alexendoo:unused-format-specs, r=xFrednet

Add `unused_format_specs` lint Currently catches two cases: An empty precision specifier: ```rust // the same as {} println!("{:.}", x); ``` And using formatting specs on `format_args!()`: ```rust // prints `x.`, not `x .` println("{:5}.", format_args!("x")); ``` changelog: new lint: [`unused_format_specs`]
andreubotella · Oct 17, 2022 · 502e87c · 502e87c
2 parents 4eaf543 + 136c2cd
commit 502e87c
Show file tree

Hide file tree

Showing 13 changed files with 384 additions and 28 deletions.
diff --git 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
@@ -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<RustcVersion>,
@@ -132,34 +170,103 @@ 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);
             }
         }
     }
 
     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
@@ -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
@@ -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
@@ -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
@@ -627,7 +627,7 @@ pub enum Count<'tcx> {
     /// `FormatParamKind::Numbered`.
     Param(FormatParam<'tcx>),
     /// Not specified.
-    Implied,
+    Implied(Option<Span>),
 }
 
 impl<'tcx> Count<'tcx> {
@@ -638,8 +638,10 @@ impl<'tcx> Count<'tcx> {
         inner: Option<rpf::InnerSpan>,
         values: &FormatArgsValues<'tcx>,
     ) -> Option<Self> {
+        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<FormatParam<'tcx>> {
@@ -675,6 +677,14 @@ impl<'tcx> Count<'tcx> {
             _ => None,
         }
     }
+
+    pub fn span(self) -> Option<Span> {
+        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
@@ -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
@@ -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
diff --git a/tests/ui/unused_format_specs.fixed 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
@@ -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);
+}