new uninlined_format_args lint to inline explicit arguments

Implement #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: [`uninlined_format_args`]: A new lint to inline format arguments, i.e. `print!("{}", var)` into `print!("{var}")`
rust-lang · Sep 25, 2022 · 5a71bbd · 5a71bbd
1 parent 57c9daa
commit 5a71bbd
Show file tree

Hide file tree

Showing 15 changed files with 1,451 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4274,6 +4274,7 @@ Released 2018-09-13
 [`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
 [`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
 [`uninit_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec
+[`uninlined_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
 [`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
 [`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
 [`unit_hash`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_hash

diff --git a/clippy_lints/src/format_args.rs b/clippy_lints/src/format_args.rs
@@ -1,16 +1,18 @@
 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, FormatParamUsage};
+use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
 use clippy_utils::ty::implements_trait;
+use clippy_utils::{is_diag_trait_item, meets_msrv, msrvs};
 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;
-use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_semver::RustcVersion;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{sym, ExpnData, ExpnKind, Span, Symbol};
 
 declare_clippy_lint! {
@@ -64,7 +66,67 @@ 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 var = 42;
+    /// # let width = 1;
+    /// # let prec = 2;
+    /// format!("{}", var);
+    /// format!("{v:?}", v = var);
+    /// format!("{0} {0}", var);
+    /// format!("{0:1$}", var, width);
+    /// format!("{:.*}", prec, var);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # let var = 42;
+    /// # let width = 1;
+    /// # let prec = 2;
+    /// format!("{var}");
+    /// format!("{var:?}");
+    /// format!("{var} {var}");
+    /// format!("{var:width$}");
+    /// format!("{var:.prec$}");
+    /// ```
+    ///
+    /// ### Known Problems
+    ///
+    /// There may be a false positive if the format string is expanded from certain proc macros:
+    ///
+    /// ```ignore
+    /// println!(indoc!("{}"), var);
+    /// ```
+    ///
+    /// If a format string contains a numbered argument that cannot be inlined
+    /// nothing will be suggested, e.g. `println!("{0}={1}", var, 1+2)`.
+    #[clippy::version = "1.65.0"]
+    pub UNINLINED_FORMAT_ARGS,
+    pedantic,
+    "using non-inlined variables in `format!` calls"
+}
+
+impl_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, UNINLINED_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
+
+pub struct FormatArgs {
+    msrv: Option<RustcVersion>,
+}
+
+impl FormatArgs {
+    #[must_use]
+    pub fn new(msrv: Option<RustcVersion>) -> Self {
+        Self { msrv }
+    }
+}
 
 impl<'tcx> LateLintPass<'tcx> for FormatArgs {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
@@ -86,9 +148,73 @@ 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 meets_msrv(self.msrv, msrvs::FORMAT_ARGS_CAPTURE) {
+                    check_uninlined_args(cx, &format_args, outermost_expn_data.call_site);
+                }
             }
         }
     }
+
+    extract_msrv_attr!(LateContext);
+}
+
+fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_site: Span) {
+    if args.format_string.span.from_expansion() {
+        return;
+    }
+
+    let mut fixes = Vec::new();
+    // If any of the arguments are referenced by an index number,
+    // and that argument is not a simple variable and cannot be inlined,
+    // we cannot remove any other arguments in the format string,
+    // because the index numbers might be wrong after inlining.
+    // Example of an un-inlinable format:  print!("{}{1}", foo, 2)
+    if !args.params().all(|p| check_one_arg(cx, &p, &mut fixes)) || fixes.is_empty() {
+        return;
+    }
+
+    // FIXME: Properly ignore a rare case where the format string is wrapped in a macro.
+    // Example:  `format!(indoc!("{}"), foo);`
+    // If inlined, they will cause a compilation error:
+    //     > to avoid ambiguity, `format_args!` cannot capture variables
+    //     > when the format string is expanded from a macro
+    // @Alexendoo explanation:
+    //     > indoc! is a proc macro that is producing a string literal with its span
+    //     > set to its input it's not marked as from expansion, and since it's compatible
+    //     > tokenization wise clippy_utils::is_from_proc_macro wouldn't catch it either
+    // This might be a relatively expensive test, so do it only we are ready to replace.
+    // See more examples in tests/ui/uninlined_format_args.rs
+
+    span_lint_and_then(
+        cx,
+        UNINLINED_FORMAT_ARGS,
+        call_site,
+        "variables can be used directly in the `format!` string",
+        |diag| {
+            diag.multipart_suggestion("change this to", fixes, Applicability::MachineApplicable);
+        },
+    );
+}
+
+fn check_one_arg(cx: &LateContext<'_>, param: &FormatParam<'_>, fixes: &mut 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 replacement = match param.usage {
+            FormatParamUsage::Argument => segment.ident.name.to_string(),
+            FormatParamUsage::Width => format!("{}$", segment.ident.name),
+            FormatParamUsage::Precision => format!(".{}$", segment.ident.name),
+        };
+        fixes.push((param.span, replacement));
+        let arg_span = expand_past_previous_comma(cx, *span);
+        fixes.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 {
@@ -170,7 +296,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
@@ -159,6 +159,7 @@ store.register_lints(&[
     format::USELESS_FORMAT,
     format_args::FORMAT_IN_FORMAT_ARGS,
     format_args::TO_STRING_IN_FORMAT_ARGS,
+    format_args::UNINLINED_FORMAT_ARGS,
     format_impl::PRINT_IN_FORMAT_IMPL,
     format_impl::RECURSIVE_FORMAT_IMPL,
     format_push_string::FORMAT_PUSH_STRING,

diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs
@@ -29,6 +29,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     LintId::of(eta_reduction::REDUNDANT_CLOSURE_FOR_METHOD_CALLS),
     LintId::of(excessive_bools::FN_PARAMS_EXCESSIVE_BOOLS),
     LintId::of(excessive_bools::STRUCT_EXCESSIVE_BOOLS),
+    LintId::of(format_args::UNINLINED_FORMAT_ARGS),
     LintId::of(functions::MUST_USE_CANDIDATE),
     LintId::of(functions::TOO_MANY_LINES),
     LintId::of(if_not_else::IF_NOT_ELSE),

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -855,7 +855,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         ))
     });
     store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks));
-    store.register_late_pass(move || Box::new(format_args::FormatArgs));
+    store.register_late_pass(move || Box::new(format_args::FormatArgs::new(msrv)));
     store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray));
     store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes));
     store.register_late_pass(|| Box::new(needless_late_init::NeedlessLateInit));

diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -213,7 +213,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),

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
@@ -545,19 +545,32 @@ fn span_from_inner(base: SpanData, inner: rpf::InnerSpan) -> Span {
     )
 }
 
+/// How a format parameter is used in the format string
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 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}")`.
     NamedInline(Symbol),
 }
 
+/// Where a format parameter is being used in the format string
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum FormatParamUsage {
+    /// Appears as an argument, e.g. `format!("{}", foo)`
+    Argument,
+    /// Appears as a width, e.g. `format!("{:width$}", foo, width = 1)`
+    Width,
+    /// Appears as a precision, e.g. `format!("{:.precision$}", foo, precision = 1)`
+    Precision,
+}
+
 /// A `FormatParam` is any place in a `FormatArgument` that refers to a supplied value, e.g.
 ///
 /// ```
@@ -573,6 +586,8 @@ pub struct FormatParam<'tcx> {
     pub value: &'tcx Expr<'tcx>,
     /// How this parameter refers to its `value`.
     pub kind: FormatParamKind,
+    /// Where this format param is being used - argument/width/precision
+    pub usage: FormatParamUsage,
     /// Span of the parameter, may be zero width. Includes the whitespace of implicit parameters.
     ///
     /// ```text
@@ -585,6 +600,7 @@ pub struct FormatParam<'tcx> {
 impl<'tcx> FormatParam<'tcx> {
     fn new(
         mut kind: FormatParamKind,
+        usage: FormatParamUsage,
         position: usize,
         inner: rpf::InnerSpan,
         values: &FormatArgsValues<'tcx>,
@@ -599,7 +615,12 @@ impl<'tcx> FormatParam<'tcx> {
             kind = FormatParamKind::NamedInline(name);
         }
 
-        Some(Self { value, kind, span })
+        Some(Self {
+            value,
+            kind,
+            usage,
+            span,
+        })
     }
 }
 
@@ -618,22 +639,35 @@ pub enum Count<'tcx> {
 
 impl<'tcx> Count<'tcx> {
     fn new(
+        usage: FormatParamUsage,
         count: rpf::Count<'_>,
         position: Option<usize>,
         inner: Option<rpf::InnerSpan>,
         values: &FormatArgsValues<'tcx>,
     ) -> Option<Self> {
         Some(match count {
             rpf::Count::CountIs(val) => Self::Is(val, span_from_inner(values.format_string_span, inner?)),
-            rpf::Count::CountIsName(name, span) => Self::Param(FormatParam::new(
+            rpf::Count::CountIsName(name, _) => Self::Param(FormatParam::new(
                 FormatParamKind::Named(Symbol::intern(name)),
+                usage,
                 position?,
-                span,
+                inner?,
+                values,
+            )?),
+            rpf::Count::CountIsParam(_) => Self::Param(FormatParam::new(
+                FormatParamKind::Numbered,
+                usage,
+                position?,
+                inner?,
+                values,
+            )?),
+            rpf::Count::CountIsStar(_) => Self::Param(FormatParam::new(
+                FormatParamKind::Starred,
+                usage,
+                position?,
+                inner?,
                 values,
             )?),
-            rpf::Count::CountIsParam(_) | rpf::Count::CountIsStar(_) => {
-                Self::Param(FormatParam::new(FormatParamKind::Numbered, position?, inner?, values)?)
-            },
             rpf::Count::CountImplied => Self::Implied,
         })
     }
@@ -676,8 +710,20 @@ impl<'tcx> FormatSpec<'tcx> {
             fill: spec.fill,
             align: spec.align,
             flags: spec.flags,
-            precision: Count::new(spec.precision, positions.precision, spec.precision_span, values)?,
-            width: Count::new(spec.width, positions.width, spec.width_span, values)?,
+            precision: Count::new(
+                FormatParamUsage::Precision,
+                spec.precision,
+                positions.precision,
+                spec.precision_span,
+                values,
+            )?,
+            width: Count::new(
+                FormatParamUsage::Width,
+                spec.width,
+                positions.width,
+                spec.width_span,
+                values,
+            )?,
             r#trait: match spec.ty {
                 "" => sym::Display,
                 "?" => sym::Debug,
@@ -723,7 +769,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.
@@ -797,6 +843,7 @@ impl<'tcx> FormatArgsExpn<'tcx> {
                                 // NamedInline is handled by `FormatParam::new()`
                                 rpf::Position::ArgumentNamed(name) => FormatParamKind::Named(Symbol::intern(name)),
                             },
+                            FormatParamUsage::Argument,
                             position.value,
                             parsed_arg.position_span,
                             &values,

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 { FORMAT_ARGS_CAPTURE }
     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 }