Skip to content

Commit 64928fd

Browse files
nyuriknyurik
committed
Addressed review feedback
1 parent a2fb591 commit 64928fd

File tree

8 files changed

+197
-164
lines changed

8 files changed

+197
-164
lines changed

‎clippy_lints/src/format_args.rs

Lines changed: 58 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,18 @@
11
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2-
use clippy_utils::is_diag_trait_item;
32
use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred};
4-
use clippy_utils::macros::{is_format_macro, FormatArgsExpn, FormatParam};
3+
use clippy_utils::macros::{is_format_macro, FormatArgsExpn, FormatParam, FormatParamUsage};
54
use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
65
use clippy_utils::ty::implements_trait;
6+
use clippy_utils::{is_diag_trait_item, meets_msrv, msrvs};
77
use if_chain::if_chain;
88
use itertools::Itertools;
99
use rustc_errors::Applicability;
1010
use rustc_hir::{Expr, ExprKind, HirId, Path, QPath};
1111
use rustc_lint::{LateContext, LateLintPass};
1212
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
1313
use rustc_middle::ty::Ty;
14-
use rustc_session::{declare_lint_pass, declare_tool_lint};
14+
use rustc_semver::RustcVersion;
15+
use rustc_session::{declare_tool_lint, impl_lint_pass};
1516
use rustc_span::{sym, ExpnData, ExpnKind, Span, Symbol};
1617

1718
declare_clippy_lint! {
@@ -77,32 +78,34 @@ declare_clippy_lint! {
7778
///
7879
/// ### Example
7980
/// ```rust
80-
/// # let foo = 42;
81-
/// format!("{}", foo);
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
8291
/// ```
8392
/// Use instead:
8493
/// ```rust
85-
/// # let foo = 42;
86-
/// format!("{foo}");
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
87101
/// ```
88102
///
89-
/// ### Supported cases
103+
/// ### Known Problems
90104
///
91-
/// code | suggestion | comment
92-
/// ---|---|---
93-
/// `print!("{}", var)` | `print!("{var}")` | simple variables
94-
/// `print!("{0}", var)` | `print!("{var}")` | positional variables
95-
/// `print!("{v}", v=var)` | `print!("{var}")` | named variables
96-
/// `print!("{0} {0}", var)` | `print!("{var} {var}")` | aliased variables
97-
/// `print!("{0:1$}", var, width)` | `print!("{var:width$}")` | width support
98-
/// `print!("{0:.1$}", var, prec)` | `print!("{var:.prec$}")` | precision support
99-
/// `print!("{:.*}", prec, var)` | `print!("{var:.prec$}")` | asterisk support
105+
/// Format string uses an indexed argument that cannot be inlined.
106+
/// Supporting this case requires re-indexing of the format string.
100107
///
101-
/// ### Unsupported cases
102-
///
103-
/// code | suggestion | comment
104-
/// ---|---|---
105-
/// `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.
108+
/// Until implemnted, `print!("{0}={1}", var, 1+2)` should be changed to `print!("{var}={0}", 1+2)` by hand.
106109
///
107110
/// changelog: [`needless-format-args`]: A new lint to inline format arguments, i.e. `print!("{}", var)` into `print!("{var}")`
108111
#[clippy::version = "1.64.0"]
@@ -111,7 +114,20 @@ declare_clippy_lint! {
111114
"using non-inlined variables in `format!` calls"
112115
}
113116

114-
declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, NEEDLESS_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
117+
impl_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, NEEDLESS_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
118+
119+
pub struct FormatArgs {
120+
support_arg_capture: bool,
121+
}
122+
123+
impl FormatArgs {
124+
#[must_use]
125+
pub fn new(msrv: Option<RustcVersion>) -> Self {
126+
Self {
127+
support_arg_capture: meets_msrv(msrv, msrvs::FORMAT_ARGS_CAPTURE),
128+
}
129+
}
130+
}
115131

116132
impl<'tcx> LateLintPass<'tcx> for FormatArgs {
117133
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
@@ -123,23 +139,7 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
123139
if is_format_macro(cx, macro_def_id);
124140
if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
125141
then {
126-
// if at least some of the arguments/format/precision are referenced by an index,
127-
// e.g. format!("{} {1}", foo, bar) or format!("{:1$}", foo, 2)
128-
// we cannot remove an argument from a list until we support renumbering.
129-
// We are OK if we inline all numbered arguments.
130-
let mut do_inline = true;
131-
// if we find one or more suggestions, this becomes a Vec of replacements
132-
let mut inline_spans = None;
133142
for arg in &format_args.args {
134-
if do_inline {
135-
do_inline = check_inline(cx, &arg.param, ParamType::Argument, &mut inline_spans);
136-
}
137-
if do_inline && let Some(p) = arg.format.width.param() {
138-
do_inline = check_inline(cx, &p, ParamType::Width, &mut inline_spans);
139-
}
140-
if do_inline && let Some(p) = arg.format.precision.param() {
141-
do_inline = check_inline(cx, &p, ParamType::Precision, &mut inline_spans);
142-
}
143143
if !arg.format.is_default() {
144144
continue;
145145
}
@@ -149,7 +149,18 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
149149
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg.param.value);
150150
check_to_string_in_format_args(cx, name, arg.param.value);
151151
}
152-
if do_inline && let Some(inline_spans) = inline_spans {
152+
153+
// if at least some of the arguments/format/precision are referenced by an index,
154+
// e.g. format!("{} {1}", foo, bar) or format!("{:1$}", foo, 2)
155+
// we cannot remove an argument from a list until we support renumbering.
156+
// We are OK if we inline all numbered arguments.
157+
if !self.support_arg_capture {
158+
return;
159+
}
160+
let mut inline_spans = Vec::new();
161+
let do_inlining = format_args.params().all(|p| check_inline(cx, &p, &mut inline_spans));
162+
if do_inlining && !inline_spans.is_empty()
163+
{
153164
span_lint_and_then(
154165
cx,
155166
NEEDLESS_FORMAT_ARGS,
@@ -165,41 +176,28 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
165176
}
166177
}
167178

168-
#[derive(Debug, Clone, Copy)]
169-
enum ParamType {
170-
Argument,
171-
Width,
172-
Precision,
173-
}
174-
175-
fn check_inline(
176-
cx: &LateContext<'_>,
177-
param: &FormatParam<'_>,
178-
ptype: ParamType,
179-
inline_spans: &mut Option<Vec<(Span, String)>>,
180-
) -> bool {
179+
fn check_inline(cx: &LateContext<'_>, param: &FormatParam<'_>, inline_spans: &mut Vec<(Span, String)>) -> bool {
181180
if matches!(param.kind, Implicit | Starred | Named(_) | Numbered)
182181
&& let ExprKind::Path(QPath::Resolved(None, path)) = param.value.kind
183182
&& let Path { span, segments, .. } = path
184183
&& let [segment] = segments
185184
{
186-
let c = inline_spans.get_or_insert_with(Vec::new);
187185
// TODO: Note the inconsistency here, that we may want to address separately:
188186
// implicit, numbered, and starred `param.span` spans the whole relevant string:
189187
// the empty space between `{}`, or the entire value `1$`, `.2$`, or `.*`
190188
// but the named argument spans just the name itself, without the surrounding `.` and `$`.
191189
let replacement = if param.kind == Numbered || param.kind == Starred {
192-
match ptype {
193-
ParamType::Argument => segment.ident.name.to_string(),
194-
ParamType::Width => format!("{}$", segment.ident.name),
195-
ParamType::Precision => format!(".{}$", segment.ident.name),
190+
match param.usage {
191+
FormatParamUsage::Argument => segment.ident.name.to_string(),
192+
FormatParamUsage::Width => format!("{}$", segment.ident.name),
193+
FormatParamUsage::Precision => format!(".{}$", segment.ident.name),
196194
}
197195
} else {
198196
segment.ident.name.to_string()
199197
};
200-
c.push((param.span, replacement));
198+
inline_spans.push((param.span, replacement));
201199
let arg_span = expand_past_previous_comma(cx, *span);
202-
c.push((arg_span, String::new()));
200+
inline_spans.push((arg_span, String::new()));
203201
true // successful inlining, continue checking
204202
} else {
205203
// if we can't inline a numbered argument, we can't continue

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 1 deletion
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_utils/src/macros.rs

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,13 @@ pub enum FormatParamKind {
559559
NamedInline(Symbol),
560560
}
561561

562+
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
563+
pub enum FormatParamUsage {
564+
Argument,
565+
Width,
566+
Precision,
567+
}
568+
562569
/// A `FormatParam` is any place in a `FormatArgument` that refers to a supplied value, e.g.
563570
///
564571
/// ```
@@ -570,6 +577,9 @@ pub enum FormatParamKind {
570577
/// and a [`FormatParamKind::NamedInline("precision")`] `.kind` with a `.value` of `2`
571578
#[derive(Debug, Copy, Clone)]
572579
pub struct FormatParam<'tcx> {
580+
/// How this format param is being used - argument/width/precision
581+
pub usage: FormatParamUsage,
582+
573583
/// The expression this parameter refers to.
574584
pub value: &'tcx Expr<'tcx>,
575585
/// How this parameter refers to its `value`.
@@ -585,6 +595,7 @@ pub struct FormatParam<'tcx> {
585595

586596
impl<'tcx> FormatParam<'tcx> {
587597
fn new(
598+
usage: FormatParamUsage,
588599
mut kind: FormatParamKind,
589600
position: usize,
590601
inner: rpf::InnerSpan,
@@ -600,7 +611,12 @@ impl<'tcx> FormatParam<'tcx> {
600611
kind = FormatParamKind::NamedInline(name);
601612
}
602613

603-
Some(Self { value, kind, span })
614+
Some(Self {
615+
usage,
616+
value,
617+
kind,
618+
span,
619+
})
604620
}
605621
}
606622

@@ -619,6 +635,7 @@ pub enum Count<'tcx> {
619635

620636
impl<'tcx> Count<'tcx> {
621637
fn new(
638+
usage: FormatParamUsage,
622639
count: rpf::Count<'_>,
623640
position: Option<usize>,
624641
inner: Option<rpf::InnerSpan>,
@@ -627,17 +644,26 @@ impl<'tcx> Count<'tcx> {
627644
Some(match count {
628645
rpf::Count::CountIs(val) => Self::Is(val, span_from_inner(values.format_string_span, inner?)),
629646
rpf::Count::CountIsName(name, span) => Self::Param(FormatParam::new(
647+
usage,
630648
FormatParamKind::Named(Symbol::intern(name)),
631649
position?,
632650
span,
633651
values,
634652
)?),
635-
rpf::Count::CountIsParam(_) => {
636-
Self::Param(FormatParam::new(FormatParamKind::Numbered, position?, inner?, values)?)
637-
},
638-
rpf::Count::CountIsStar(_) => {
639-
Self::Param(FormatParam::new(FormatParamKind::Starred, position?, inner?, values)?)
640-
},
653+
rpf::Count::CountIsParam(_) => Self::Param(FormatParam::new(
654+
usage,
655+
FormatParamKind::Numbered,
656+
position?,
657+
inner?,
658+
values,
659+
)?),
660+
rpf::Count::CountIsStar(_) => Self::Param(FormatParam::new(
661+
usage,
662+
FormatParamKind::Starred,
663+
position?,
664+
inner?,
665+
values,
666+
)?),
641667
rpf::Count::CountImplied => Self::Implied,
642668
})
643669
}
@@ -680,8 +706,20 @@ impl<'tcx> FormatSpec<'tcx> {
680706
fill: spec.fill,
681707
align: spec.align,
682708
flags: spec.flags,
683-
precision: Count::new(spec.precision, positions.precision, spec.precision_span, values)?,
684-
width: Count::new(spec.width, positions.width, spec.width_span, values)?,
709+
precision: Count::new(
710+
FormatParamUsage::Precision,
711+
spec.precision,
712+
positions.precision,
713+
spec.precision_span,
714+
values,
715+
)?,
716+
width: Count::new(
717+
FormatParamUsage::Width,
718+
spec.width,
719+
positions.width,
720+
spec.width_span,
721+
values,
722+
)?,
685723
r#trait: match spec.ty {
686724
"" => sym::Display,
687725
"?" => sym::Debug,
@@ -795,6 +833,7 @@ impl<'tcx> FormatArgsExpn<'tcx> {
795833
.map(|(position, parsed_arg, arg_span)| {
796834
Some(FormatArg {
797835
param: FormatParam::new(
836+
FormatParamUsage::Argument,
798837
match parsed_arg.position {
799838
rpf::Position::ArgumentImplicitlyIs(_) => FormatParamKind::Implicit,
800839
rpf::Position::ArgumentIs(_) => FormatParamKind::Numbered,

‎clippy_utils/src/msrvs.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ macro_rules! msrv_aliases {
1313
// names may refer to stabilized feature flags or library items
1414
msrv_aliases! {
1515
1,62,0 { BOOL_THEN_SOME }
16-
1,58,0 { NEEDLESS_FORMAT_ARGS }
16+
1,58,0 { FORMAT_ARGS_CAPTURE }
1717
1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
1818
1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
1919
1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS }

‎src/docs/needless_format_args.txt

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,29 +9,27 @@ The inlined syntax, where allowed, is simpler.
99

1010
### Example
1111
```
12-
format!("{}", foo);
12+
format!("{}", var); // implied variables
13+
format!("{0}", var); // positional variables
14+
format!("{v}", v=var); // named variables
15+
format!("{0} {0}", var); // aliased variables
16+
format!("{0:1$}", var, width); // width support
17+
format!("{0:.1$}", var, prec); // precision support
18+
format!("{:.*}", prec, var); // asterisk support
1319
```
1420
Use instead:
1521
```
16-
format!("{foo}");
22+
format!("{var}"); // implied, positional, and named variables
23+
format!("{var} {var}"); // aliased variables
24+
format!("{var:width$}"); // width support
25+
format!("{var:.prec$}"); // precision and asterisk support
1726
```
1827

19-
### Supported cases
28+
### Known Problems
2029

21-
code | suggestion | comment
22-
---|---|---
23-
`print!("{}", var)` | `print!("{var}")` | simple variables
24-
`print!("{0}", var)` | `print!("{var}")` | positional variables
25-
`print!("{v}", v=var)` | `print!("{var}")` | named variables
26-
`print!("{0} {0}", var)` | `print!("{var} {var}")` | aliased variables
27-
`print!("{0:1$}", var, width)` | `print!("{var:width$}")` | width support
28-
`print!("{0:.1$}", var, prec)` | `print!("{var:.prec$}")` | precision support
29-
`print!("{:.*}", prec, var)` | `print!("{var:.prec$}")` | asterisk support
30+
Format string uses an indexed argument that cannot be inlined.
31+
Supporting this case requires re-indexing of the format string.
3032

31-
### Unsupported cases
32-
33-
code | suggestion | comment
34-
---|---|---
35-
`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.
33+
Until implemnted, `print!("{0}={1}", var, 1+2)` should be changed to `print!("{var}={0}", 1+2)` by hand.
3634

3735
changelog: [`needless-format-args`]: A new lint to inline format arguments, i.e. `print!("{}", var)` into `print!("{var}")`

‎tests/ui/needless_format_args.fixed

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,8 @@ fn tester(fn_arg: i32) {
1616
let val = 6;
1717

1818
// make sure this file hasn't been corrupted with tabs converted to spaces
19-
// TODO: ideally this should be a const compiler check, similar to
20-
// https://docs.rs/static_assertions/1.1.0/static_assertions/macro.const_assert_eq.html
21-
assert_eq!(" ", "\t \t");
19+
// let _ = ' '; // <- this is a single tab character
20+
let _: &[u8; 3] = b" "; // <- <tab><space><tab>
2221

2322
println!("val='{local_i32}'");
2423
println!("val='{local_i32}'"); // 3 spaces

‎tests/ui/needless_format_args.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,8 @@ fn tester(fn_arg: i32) {
1616
let val = 6;
1717

1818
// make sure this file hasn't been corrupted with tabs converted to spaces
19-
// TODO: ideally this should be a const compiler check, similar to
20-
// https://docs.rs/static_assertions/1.1.0/static_assertions/macro.const_assert_eq.html
21-
assert_eq!(" ", "\t \t");
19+
// let _ = ' '; // <- this is a single tab character
20+
let _: &[u8; 3] = b" "; // <- <tab><space><tab>
2221

2322
println!("val='{}'", local_i32);
2423
println!("val='{ }'", local_i32); // 3 spaces

0 commit comments

Comments
 (0)