Skip to content

Commit ba320c7

Browse files
committed
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}")`
1 parent 8b1ad17 commit ba320c7

22 files changed

+1507
-72
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -4274,6 +4274,7 @@ Released 2018-09-13
42744274
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
42754275
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
42764276
[`uninit_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec
4277+
[`uninlined_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
42774278
[`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
42784279
[`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
42794280
[`unit_hash`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_hash

Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ clap = { version = "3.1", features = ["derive"] }
4646
clippy_utils = { path = "clippy_utils" }
4747
derive-new = "0.5"
4848
if_chain = "1.0"
49+
indoc = "1.0"
4950
itertools = "0.10.1"
5051
quote = "1.0"
5152
serde = { version = "1.0.125", features = ["derive"] }

clippy_lints/src/format_args.rs

+134-7
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,18 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2-
use clippy_utils::is_diag_trait_item;
3-
use clippy_utils::macros::{is_format_macro, FormatArgsExpn};
4-
use clippy_utils::source::snippet_opt;
2+
use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred};
3+
use clippy_utils::macros::{is_format_macro, FormatArgsExpn, FormatParam, FormatParamUsage};
4+
use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
55
use clippy_utils::ty::implements_trait;
6+
use clippy_utils::{is_diag_trait_item, meets_msrv, msrvs};
67
use if_chain::if_chain;
78
use itertools::Itertools;
89
use rustc_errors::Applicability;
9-
use rustc_hir::{Expr, ExprKind, HirId};
10+
use rustc_hir::{Expr, ExprKind, HirId, Path, QPath};
1011
use rustc_lint::{LateContext, LateLintPass};
1112
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
1213
use rustc_middle::ty::Ty;
13-
use rustc_session::{declare_lint_pass, declare_tool_lint};
14+
use rustc_semver::RustcVersion;
15+
use rustc_session::{declare_tool_lint, impl_lint_pass};
1416
use rustc_span::{sym, ExpnData, ExpnKind, Span, Symbol};
1517

1618
declare_clippy_lint! {
@@ -64,7 +66,72 @@ declare_clippy_lint! {
6466
"`to_string` applied to a type that implements `Display` in format args"
6567
}
6668

67-
declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
69+
declare_clippy_lint! {
70+
/// ### What it does
71+
/// Detect when a variable is not inlined in a format string,
72+
/// and suggests to inline it.
73+
///
74+
/// ### Why is this bad?
75+
/// Non-inlined code is slightly more difficult to read and understand,
76+
/// as it requires arguments to be matched against the format string.
77+
/// The inlined syntax, where allowed, is simpler.
78+
///
79+
/// ### Example
80+
/// ```rust
81+
/// # let var = 42;
82+
/// # let width = 1;
83+
/// # let prec = 2;
84+
/// format!("{}", var); // implied variables
85+
/// format!("{0}", var); // positional variables
86+
/// format!("{v}", v=var); // named variables
87+
/// format!("{0} {0}", var); // aliased variables
88+
/// format!("{0:1$}", var, width); // width support
89+
/// format!("{0:.1$}", var, prec); // precision support
90+
/// format!("{:.*}", prec, var); // asterisk support
91+
/// ```
92+
/// Use instead:
93+
/// ```rust
94+
/// # let var = 42;
95+
/// # let width = 1;
96+
/// # let prec = 2;
97+
/// format!("{var}"); // implied, positional, and named variables
98+
/// format!("{var} {var}"); // aliased variables
99+
/// format!("{var:width$}"); // width support
100+
/// format!("{var:.prec$}"); // precision and asterisk support
101+
/// ```
102+
///
103+
/// ### Known Problems
104+
///
105+
/// * There may be a false positive if the format string is wrapped in a macro call:
106+
/// ```rust
107+
/// # let var = 42;
108+
/// macro_rules! no_param_str { () => { "{}" }; }
109+
/// macro_rules! pass_through { ($expr:expr) => { $expr }; }
110+
/// println!(no_param_str!(), var);
111+
/// println!(pass_through!("{}"), var);
112+
/// ```
113+
///
114+
/// * Format string uses an indexed argument that cannot be inlined.
115+
/// Supporting this case requires re-indexing of the format string.
116+
/// Until implemented, `print!("{0}={1}", var, 1+2)` should be changed to `print!("{var}={0}", 1+2)` by hand.
117+
#[clippy::version = "1.65.0"]
118+
pub UNINLINED_FORMAT_ARGS,
119+
pedantic,
120+
"using non-inlined variables in `format!` calls"
121+
}
122+
123+
impl_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, UNINLINED_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
124+
125+
pub struct FormatArgs {
126+
msrv: Option<RustcVersion>,
127+
}
128+
129+
impl FormatArgs {
130+
#[must_use]
131+
pub fn new(msrv: Option<RustcVersion>) -> Self {
132+
Self { msrv }
133+
}
134+
}
68135

69136
impl<'tcx> LateLintPass<'tcx> for FormatArgs {
70137
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
@@ -86,9 +153,69 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
86153
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg.param.value);
87154
check_to_string_in_format_args(cx, name, arg.param.value);
88155
}
156+
if meets_msrv(self.msrv, msrvs::FORMAT_ARGS_CAPTURE) {
157+
check_uninlined_args(cx, &format_args, outermost_expn_data.call_site);
158+
}
89159
}
90160
}
91161
}
162+
163+
extract_msrv_attr!(LateContext);
164+
}
165+
166+
fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_site: Span) {
167+
let mut fixes = Vec::new();
168+
// If any of the arguments are referenced by an index number,
169+
// and that argument is not a simple variable and cannot be inlined,
170+
// we cannot remove any other arguments in the format string,
171+
// because the index numbers might be wrong after inlining.
172+
// Example of an un-inlinable format: print!("{}{1}", foo, 2)
173+
if !args.params().all(|p| check_one_arg(cx, &p, &mut fixes)) || fixes.is_empty() {
174+
return;
175+
}
176+
177+
// FIXME: Properly ignore a rare case where the format string is wrapped in a macro.
178+
// Example: `format!(indoc!("{}"), foo);`
179+
// If inlined, they will cause a compilation error:
180+
// > to avoid ambiguity, `format_args!` cannot capture variables
181+
// > when the format string is expanded from a macro
182+
// @Alexendoo explanation:
183+
// > indoc! is a proc macro that is producing a string literal with its span
184+
// > set to its input it's not marked as from expansion, and since it's compatible
185+
// > tokenization wise clippy_utils::is_from_proc_macro wouldn't catch it either
186+
// This might be a relatively expensive test, so do it only we are ready to replace.
187+
// See more examples in tests/ui/uninlined_format_args.rs
188+
189+
span_lint_and_then(
190+
cx,
191+
UNINLINED_FORMAT_ARGS,
192+
call_site,
193+
"variables can be used directly in the `format!` string",
194+
|diag| {
195+
diag.multipart_suggestion("change this to", fixes, Applicability::MachineApplicable);
196+
},
197+
);
198+
}
199+
200+
fn check_one_arg(cx: &LateContext<'_>, param: &FormatParam<'_>, fixes: &mut Vec<(Span, String)>) -> bool {
201+
if matches!(param.kind, Implicit | Starred | Named(_) | Numbered)
202+
&& let ExprKind::Path(QPath::Resolved(None, path)) = param.value.kind
203+
&& let Path { span, segments, .. } = path
204+
&& let [segment] = segments
205+
{
206+
let replacement = match param.usage {
207+
FormatParamUsage::Argument => segment.ident.name.to_string(),
208+
FormatParamUsage::Width => format!("{}$", segment.ident.name),
209+
FormatParamUsage::Precision => format!(".{}$", segment.ident.name),
210+
};
211+
fixes.push((param.span, replacement));
212+
let arg_span = expand_past_previous_comma(cx, *span);
213+
fixes.push((arg_span, String::new()));
214+
true // successful inlining, continue checking
215+
} else {
216+
// if we can't inline a numbered argument, we can't continue
217+
param.kind != Numbered
218+
}
92219
}
93220

94221
fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
@@ -170,7 +297,7 @@ fn check_to_string_in_format_args(cx: &LateContext<'_>, name: Symbol, value: &Ex
170297
}
171298
}
172299

173-
// Returns true if `hir_id` is referred to by multiple format params
300+
/// Returns true if `hir_id` is referred to by multiple format params
174301
fn is_aliased(args: &FormatArgsExpn<'_>, hir_id: HirId) -> bool {
175302
args.params()
176303
.filter(|param| param.value.hir_id == hir_id)

clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ store.register_lints(&[
159159
format::USELESS_FORMAT,
160160
format_args::FORMAT_IN_FORMAT_ARGS,
161161
format_args::TO_STRING_IN_FORMAT_ARGS,
162+
format_args::UNINLINED_FORMAT_ARGS,
162163
format_impl::PRINT_IN_FORMAT_IMPL,
163164
format_impl::RECURSIVE_FORMAT_IMPL,
164165
format_push_string::FORMAT_PUSH_STRING,

clippy_lints/src/lib.register_pedantic.rs

+1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
2929
LintId::of(eta_reduction::REDUNDANT_CLOSURE_FOR_METHOD_CALLS),
3030
LintId::of(excessive_bools::FN_PARAMS_EXCESSIVE_BOOLS),
3131
LintId::of(excessive_bools::STRUCT_EXCESSIVE_BOOLS),
32+
LintId::of(format_args::UNINLINED_FORMAT_ARGS),
3233
LintId::of(functions::MUST_USE_CANDIDATE),
3334
LintId::of(functions::TOO_MANY_LINES),
3435
LintId::of(if_not_else::IF_NOT_ELSE),

clippy_lints/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -855,7 +855,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
855855
))
856856
});
857857
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks));
858-
store.register_late_pass(move || Box::new(format_args::FormatArgs));
858+
store.register_late_pass(move || Box::new(format_args::FormatArgs::new(msrv)));
859859
store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray));
860860
store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes));
861861
store.register_late_pass(|| Box::new(needless_late_init::NeedlessLateInit));

clippy_lints/src/utils/conf.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ define_Conf! {
213213
///
214214
/// Suppress lints whenever the suggested change would cause breakage for other crates.
215215
(avoid_breaking_exported_api: bool = true),
216-
/// 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.
216+
/// 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.
217217
///
218218
/// The minimum rust version that the project supports
219219
(msrv: Option<String> = None),

clippy_lints/src/write.rs

+2-9
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
22
use clippy_utils::macros::{root_macro_call_first_node, FormatArgsExpn, MacroCall};
3-
use clippy_utils::source::snippet_opt;
3+
use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
44
use rustc_ast::LitKind;
55
use rustc_errors::Applicability;
66
use rustc_hir::{Expr, ExprKind, HirIdMap, Impl, Item, ItemKind};
77
use rustc_lint::{LateContext, LateLintPass, LintContext};
88
use rustc_session::{declare_tool_lint, impl_lint_pass};
9-
use rustc_span::{sym, BytePos, Span};
9+
use rustc_span::{sym, BytePos};
1010

1111
declare_clippy_lint! {
1212
/// ### What it does
@@ -542,10 +542,3 @@ fn conservative_unescape(literal: &str) -> Result<String, UnescapeErr> {
542542

543543
if err { Err(UnescapeErr::Lint) } else { Ok(unescaped) }
544544
}
545-
546-
// Expand from `writeln!(o, "")` to `writeln!(o, "")`
547-
// ^^ ^^^^
548-
fn expand_past_previous_comma(cx: &LateContext<'_>, span: Span) -> Span {
549-
let extended = cx.sess().source_map().span_extend_to_prev_char(span, ',', true);
550-
extended.with_lo(extended.lo() - BytePos(1))
551-
}

clippy_utils/src/macros.rs

+58-11
Original file line numberDiff line numberDiff line change
@@ -545,19 +545,32 @@ fn span_from_inner(base: SpanData, inner: rpf::InnerSpan) -> Span {
545545
)
546546
}
547547

548+
/// How a format parameter is used in the format string
548549
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
549550
pub enum FormatParamKind {
550551
/// An implicit parameter , such as `{}` or `{:?}`.
551552
Implicit,
552-
/// A parameter with an explicit number, or an asterisk precision. e.g. `{1}`, `{0:?}`,
553-
/// `{:.0$}` or `{:.*}`.
553+
/// A parameter with an explicit number, e.g. `{1}`, `{0:?}`, or `{:.0$}`
554554
Numbered,
555+
/// A parameter with an asterisk precision. e.g. `{:.*}`.
556+
Starred,
555557
/// A named parameter with a named `value_arg`, such as the `x` in `format!("{x}", x = 1)`.
556558
Named(Symbol),
557559
/// An implicit named parameter, such as the `y` in `format!("{y}")`.
558560
NamedInline(Symbol),
559561
}
560562

563+
/// Where a format parameter is being used in the format string
564+
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
565+
pub enum FormatParamUsage {
566+
/// Appears as an argument, e.g. `format!("{}", foo)`
567+
Argument,
568+
/// Appears as a width, e.g. `format!("{:width$}", foo, width = 1)`
569+
Width,
570+
/// Appears as a precision, e.g. `format!("{:.precision$}", foo, precision = 1)`
571+
Precision,
572+
}
573+
561574
/// A `FormatParam` is any place in a `FormatArgument` that refers to a supplied value, e.g.
562575
///
563576
/// ```
@@ -573,6 +586,8 @@ pub struct FormatParam<'tcx> {
573586
pub value: &'tcx Expr<'tcx>,
574587
/// How this parameter refers to its `value`.
575588
pub kind: FormatParamKind,
589+
/// Where this format param is being used - argument/width/precision
590+
pub usage: FormatParamUsage,
576591
/// Span of the parameter, may be zero width. Includes the whitespace of implicit parameters.
577592
///
578593
/// ```text
@@ -585,6 +600,7 @@ pub struct FormatParam<'tcx> {
585600
impl<'tcx> FormatParam<'tcx> {
586601
fn new(
587602
mut kind: FormatParamKind,
603+
usage: FormatParamUsage,
588604
position: usize,
589605
inner: rpf::InnerSpan,
590606
values: &FormatArgsValues<'tcx>,
@@ -599,7 +615,12 @@ impl<'tcx> FormatParam<'tcx> {
599615
kind = FormatParamKind::NamedInline(name);
600616
}
601617

602-
Some(Self { value, kind, span })
618+
Some(Self {
619+
value,
620+
kind,
621+
usage,
622+
span,
623+
})
603624
}
604625
}
605626

@@ -618,22 +639,35 @@ pub enum Count<'tcx> {
618639

619640
impl<'tcx> Count<'tcx> {
620641
fn new(
642+
usage: FormatParamUsage,
621643
count: rpf::Count<'_>,
622644
position: Option<usize>,
623645
inner: Option<rpf::InnerSpan>,
624646
values: &FormatArgsValues<'tcx>,
625647
) -> Option<Self> {
626648
Some(match count {
627649
rpf::Count::CountIs(val) => Self::Is(val, span_from_inner(values.format_string_span, inner?)),
628-
rpf::Count::CountIsName(name, span) => Self::Param(FormatParam::new(
650+
rpf::Count::CountIsName(name, _) => Self::Param(FormatParam::new(
629651
FormatParamKind::Named(Symbol::intern(name)),
652+
usage,
630653
position?,
631-
span,
654+
inner?,
655+
values,
656+
)?),
657+
rpf::Count::CountIsParam(_) => Self::Param(FormatParam::new(
658+
FormatParamKind::Numbered,
659+
usage,
660+
position?,
661+
inner?,
662+
values,
663+
)?),
664+
rpf::Count::CountIsStar(_) => Self::Param(FormatParam::new(
665+
FormatParamKind::Starred,
666+
usage,
667+
position?,
668+
inner?,
632669
values,
633670
)?),
634-
rpf::Count::CountIsParam(_) | rpf::Count::CountIsStar(_) => {
635-
Self::Param(FormatParam::new(FormatParamKind::Numbered, position?, inner?, values)?)
636-
},
637671
rpf::Count::CountImplied => Self::Implied,
638672
})
639673
}
@@ -676,8 +710,20 @@ impl<'tcx> FormatSpec<'tcx> {
676710
fill: spec.fill,
677711
align: spec.align,
678712
flags: spec.flags,
679-
precision: Count::new(spec.precision, positions.precision, spec.precision_span, values)?,
680-
width: Count::new(spec.width, positions.width, spec.width_span, values)?,
713+
precision: Count::new(
714+
FormatParamUsage::Precision,
715+
spec.precision,
716+
positions.precision,
717+
spec.precision_span,
718+
values,
719+
)?,
720+
width: Count::new(
721+
FormatParamUsage::Width,
722+
spec.width,
723+
positions.width,
724+
spec.width_span,
725+
values,
726+
)?,
681727
r#trait: match spec.ty {
682728
"" => sym::Display,
683729
"?" => sym::Debug,
@@ -723,7 +769,7 @@ pub struct FormatArg<'tcx> {
723769
pub struct FormatArgsExpn<'tcx> {
724770
/// The format string literal.
725771
pub format_string: FormatString,
726-
// The format arguments, such as `{:?}`.
772+
/// The format arguments, such as `{:?}`.
727773
pub args: Vec<FormatArg<'tcx>>,
728774
/// Has an added newline due to `println!()`/`writeln!()`/etc. The last format string part will
729775
/// include this added newline.
@@ -797,6 +843,7 @@ impl<'tcx> FormatArgsExpn<'tcx> {
797843
// NamedInline is handled by `FormatParam::new()`
798844
rpf::Position::ArgumentNamed(name) => FormatParamKind::Named(Symbol::intern(name)),
799845
},
846+
FormatParamUsage::Argument,
800847
position.value,
801848
parsed_arg.position_span,
802849
&values,

0 commit comments

Comments
 (0)