Implement var inlining in format!()

Implement rust-lang#8368 - a new lint to inline format arguments such as `print!("{}", var)` into `print!("{var}")`. code | suggestion | comment ---|---|--- `print!("{}", var)` | `print!("{var}")` | simple variables `print!("{0}", var)` | `print!("{var}")` | positional variables `print!("{v}", v=var)` | `print!("{var}")` | named variables `print!("{0} {0}", var)` | `print!("{var} {var}")` | aliased variables `print!("{0:1$}", var, width)` | `print!("{var:width$}")` | width support `print!("{0:.1$}", var, prec)` | `print!("{var:.prec$}")` | precision support `print!("{:.*}", prec, var)` | `print!("{var:.prec$}")` | asterisk support code | suggestion | comment ---|---|--- `print!("{0}={1}", var, 1+2)` | `print!("{var}={0}", 1+2)` | Format string uses an indexed argument that cannot be inlined. Supporting this case requires re-indexing of the format string. changelog: [`inline-format-args`]: A new lint to inline format arguments, i.e. `print!("{}", var)` into `print!("{var}")`
nyurik · Sep 15, 2022 · 6584964 · 6584964
1 parent ba9afaf
commit 6584964
Show file tree

Hide file tree

Showing 13 changed files with 1,247 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3921,6 +3921,7 @@ Released 2018-09-13
 [`needless_continue`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue
 [`needless_doctest_main`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_doctest_main
 [`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each
+[`needless_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_format_args
 [`needless_late_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_late_init
 [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
 [`needless_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_match

diff --git a/clippy_lints/src/format_args.rs b/clippy_lints/src/format_args.rs
@@ -1,12 +1,13 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::is_diag_trait_item;
-use clippy_utils::macros::{is_format_macro, FormatArgsExpn};
-use clippy_utils::source::snippet_opt;
+use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred};
+use clippy_utils::macros::{is_format_macro, FormatArgsExpn, FormatParam};
+use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
 use clippy_utils::ty::implements_trait;
 use if_chain::if_chain;
 use itertools::Itertools;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, HirId};
+use rustc_hir::{Expr, ExprKind, HirId, Path, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::adjustment::{Adjust, Adjustment};
 use rustc_middle::ty::Ty;
@@ -64,7 +65,33 @@ declare_clippy_lint! {
     "`to_string` applied to a type that implements `Display` in format args"
 }
 
-declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detect when a variable is not inlined in a format string,
+    /// and suggests to inline it.
+    ///
+    /// ### Why is this bad?
+    /// Non-inlined code is slightly more difficult to read and understand,
+    /// as it requires arguments to be matched against the format string.
+    /// The inlined syntax, where allowed, is simpler.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # let foo = 42;
+    /// format!("{}", foo);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # let foo = 42;
+    /// format!("{foo}");
+    /// ```
+    #[clippy::version = "1.64.0"]
+    pub NEEDLESS_FORMAT_ARGS,
+    nursery,
+    "using non-inlined variables in `format!` calls"
+}
+
+declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, NEEDLESS_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
 
 impl<'tcx> LateLintPass<'tcx> for FormatArgs {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
@@ -76,7 +103,23 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
             if is_format_macro(cx, macro_def_id);
             if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
             then {
+                // if at least some of the arguments/format/precision are referenced by an index,
+                // e.g.  format!("{} {1}", foo, bar)  or  format!("{:1$}", foo, 2)
+                // we cannot remove an argument from a list until we support renumbering.
+                // We are OK if we inline all numbered arguments.
+                let mut do_inline = true;
+                // if we find one or more suggestions, this becomes a Vec of replacements
+                let mut inline_spans = None;
                 for arg in &format_args.args {
+                    if do_inline {
+                        do_inline = check_inline(cx, &arg.param, ParamType::Argument, &mut inline_spans);
+                    }
+                    if do_inline && let Some(p) = arg.format.width.param() {
+                        do_inline = check_inline(cx, &p, ParamType::Width, &mut inline_spans);
+                    }
+                    if do_inline && let Some(p) = arg.format.precision.param() {
+                        do_inline = check_inline(cx, &p, ParamType::Precision, &mut inline_spans);
+                    }
                     if !arg.format.is_default() {
                         continue;
                     }
@@ -86,11 +129,64 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
                     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 do_inline && let Some(inline_spans) = inline_spans  {
+                    span_lint_and_then(
+                        cx,
+                        NEEDLESS_FORMAT_ARGS,
+                        outermost_expn_data.call_site,
+                        "variables can be used directly in the `format!` string",
+                        |diag| {
+                            diag.multipart_suggestion("change this to", inline_spans, Applicability::MachineApplicable);
+                        },
+                    );
+                }
             }
         }
     }
 }
 
+#[derive(Debug, Clone, Copy)]
+enum ParamType {
+    Argument,
+    Width,
+    Precision,
+}
+
+fn check_inline(
+    cx: &LateContext<'_>,
+    param: &FormatParam<'_>,
+    ptype: ParamType,
+    inline_spans: &mut Option<Vec<(Span, String)>>,
+) -> bool {
+    if matches!(param.kind, Implicit | Starred | Named(_) | Numbered)
+        && let ExprKind::Path(QPath::Resolved(None, path)) = param.value.kind
+        && let Path { span, segments, .. } = path
+        && let [segment] = segments
+    {
+        let c = inline_spans.get_or_insert_with(Vec::new);
+        // TODO: Note the inconsistency here, that we may want to address separately:
+        // implicit, numbered, and starred `param.span` spans the whole relevant string:
+        // the empty space between `{}`, or the entire value `1$`, `.2$`, or `.*`
+        // but the named argument spans just the name itself, without the surrounding `.` and `$`.
+        let replacement = if param.kind == Numbered || param.kind == Starred {
+            match ptype {
+                ParamType::Argument => segment.ident.name.to_string(),
+                ParamType::Width => format!("{}$", segment.ident.name),
+                ParamType::Precision => format!(".{}$", segment.ident.name),
+            }
+        } else {
+            segment.ident.name.to_string()
+        };
+        c.push((param.span, replacement));
+        let arg_span = expand_past_previous_comma(cx, *span);
+        c.push((arg_span, String::new()));
+        true  // successful inlining, continue checking
+    } else {
+        // if we can't inline a numbered argument, we can't continue
+        param.kind != Numbered
+    }
+}
+
 fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
     if expn_data.call_site.from_expansion() {
         outermost_expn_data(expn_data.call_site.ctxt().outer_expn_data())
@@ -175,7 +271,7 @@ fn check_to_string_in_format_args(cx: &LateContext<'_>, name: Symbol, value: &Ex
     }
 }
 
-// Returns true if `hir_id` is referred to by multiple format params
+/// Returns true if `hir_id` is referred to by multiple format params
 fn is_aliased(args: &FormatArgsExpn<'_>, hir_id: HirId) -> bool {
     args.params()
         .filter(|param| param.value.hir_id == hir_id)

diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -158,6 +158,7 @@ store.register_lints(&[
     floating_point_arithmetic::SUBOPTIMAL_FLOPS,
     format::USELESS_FORMAT,
     format_args::FORMAT_IN_FORMAT_ARGS,
+    format_args::NEEDLESS_FORMAT_ARGS,
     format_args::TO_STRING_IN_FORMAT_ARGS,
     format_impl::PRINT_IN_FORMAT_IMPL,
     format_impl::RECURSIVE_FORMAT_IMPL,

diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs
@@ -10,6 +10,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
     LintId::of(fallible_impl_from::FALLIBLE_IMPL_FROM),
     LintId::of(floating_point_arithmetic::IMPRECISE_FLOPS),
     LintId::of(floating_point_arithmetic::SUBOPTIMAL_FLOPS),
+    LintId::of(format_args::NEEDLESS_FORMAT_ARGS),
     LintId::of(future_not_send::FUTURE_NOT_SEND),
     LintId::of(index_refutable_slice::INDEX_REFUTABLE_SLICE),
     LintId::of(let_if_seq::USELESS_LET_IF_SEQ),

diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs
@@ -1,12 +1,12 @@
 use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
 use clippy_utils::macros::{root_macro_call_first_node, FormatArgsExpn, MacroCall};
-use clippy_utils::source::snippet_opt;
+use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
 use rustc_ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, HirIdMap, Impl, Item, ItemKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
-use rustc_span::{sym, BytePos, Span};
+use rustc_span::{sym, BytePos};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -542,10 +542,3 @@ fn conservative_unescape(literal: &str) -> Result<String, UnescapeErr> {
 
     if err { Err(UnescapeErr::Lint) } else { Ok(unescaped) }
 }
-
-// Expand from `writeln!(o, "")` to `writeln!(o, "")`
-//                          ^^                 ^^^^
-fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
-    let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
-    extended.with_lo(extended.lo() - BytePos(1))
-}
diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs
@@ -549,9 +549,10 @@ fn span_from_inner(base: SpanData, inner: rpf::InnerSpan) -> Span {
 pub enum FormatParamKind {
     /// An implicit parameter , such as `{}` or `{:?}`.
     Implicit,
-    /// A parameter with an explicit number, or an asterisk precision. e.g. `{1}`, `{0:?}`,
-    /// `{:.0$}` or `{:.*}`.
+    /// A parameter with an explicit number, e.g. `{1}`, `{0:?}`, or `{:.0$}`
     Numbered,
+    /// A parameter with an asterisk precision. e.g. `{:.*}`.
+    Starred,
     /// A named parameter with a named `value_arg`, such as the `x` in `format!("{x}", x = 1)`.
     Named(Symbol),
     /// An implicit named parameter, such as the `y` in `format!("{y}")`.
@@ -631,9 +632,12 @@ impl<'tcx> Count<'tcx> {
                 span,
                 values,
             )?),
-            rpf::Count::CountIsParam(_) | rpf::Count::CountIsStar(_) => {
+            rpf::Count::CountIsParam(_) => {
                 Self::Param(FormatParam::new(FormatParamKind::Numbered, position?, inner?, values)?)
             },
+            rpf::Count::CountIsStar(_) => {
+                Self::Param(FormatParam::new(FormatParamKind::Starred, position?, inner?, values)?)
+            },
             rpf::Count::CountImplied => Self::Implied,
         })
     }
@@ -723,7 +727,7 @@ pub struct FormatArg<'tcx> {
 pub struct FormatArgsExpn<'tcx> {
     /// The format string literal.
     pub format_string: FormatString,
-    // The format arguments, such as `{:?}`.
+    /// The format arguments, such as `{:?}`.
     pub args: Vec<FormatArg<'tcx>>,
     /// Has an added newline due to `println!()`/`writeln!()`/etc. The last format string part will
     /// include this added newline.

diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs
@@ -13,6 +13,7 @@ macro_rules! msrv_aliases {
 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
     1,62,0 { BOOL_THEN_SOME }
+    1,58,0 { NEEDLESS_FORMAT_ARGS }
     1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
     1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
     1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS }

diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -392,6 +392,16 @@ pub fn trim_span(sm: &SourceMap, span: Span) -> Span {
     .span()
 }
 
+/// Expand a span to include a preceding comma
+/// ```rust,ignore
+/// writeln!(o, "")   ->   writeln!(o, "")
+///             ^^                   ^^^^
+/// ```
+pub fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
+    let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
+    extended.with_lo(extended.lo() - BytePos(1))
+}
+
 #[cfg(test)]
 mod test {
     use super::{reindent_multiline, without_block_comments};

diff --git a/src/docs.rs b/src/docs.rs
@@ -339,6 +339,7 @@ docs! {
     "needless_continue",
     "needless_doctest_main",
     "needless_for_each",
+    "needless_format_args",
     "needless_late_init",
     "needless_lifetimes",
     "needless_match",
@@ -548,7 +549,7 @@ docs! {
     "unseparated_literal_suffix",
     "unsound_collection_transmute",
     "unused_async",
-    "unused_io_amount",
+       "unused_io_amount",
     "unused_peekable",
     "unused_rounding",
     "unused_self",

diff --git a/src/docs/needless_format_args.txt b/src/docs/needless_format_args.txt
@@ -0,0 +1,17 @@
+### What it does
+Detect when a variable is not inlined in a format string,
+and suggests to inline it.
+
+### Why is this bad?
+Non-inlined code is slightly more difficult to read and understand,
+as it requires arguments to be matched against the format string.
+The inlined syntax, where allowed, is simpler.
+
+### Example
+```
+format!("{}", foo);
+```
+Use instead:
+```
+format!("{foo}");
+```