Skip to content

Commit 293742b

Browse files
committed
Add format_in_format_args and to_string_in_format_args lints
Fixes rust-lang#7667 and rust-lang#7729
1 parent fe999e8 commit 293742b

15 files changed

+423
-46
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2734,6 +2734,7 @@ Released 2018-09-13
27342734
[`for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles
27352735
[`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
27362736
[`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
2737+
[`format_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args
27372738
[`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
27382739
[`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
27392740
[`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10
@@ -3011,6 +3012,7 @@ Released 2018-09-13
30113012
[`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
30123013
[`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
30133014
[`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
3015+
[`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
30143016
[`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
30153017
[`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
30163018
[`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines

clippy_lints/src/format.rs

Lines changed: 2 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::format::{check_unformatted, is_display_arg};
23
use clippy_utils::higher::FormatExpn;
3-
use clippy_utils::last_path_segment;
44
use clippy_utils::source::{snippet_opt, snippet_with_applicability};
55
use clippy_utils::sugg::Sugg;
66
use if_chain::if_chain;
77
use rustc_errors::Applicability;
8-
use rustc_hir::{BorrowKind, Expr, ExprKind, QPath};
8+
use rustc_hir::{Expr, ExprKind};
99
use rustc_lint::{LateContext, LateLintPass};
1010
use rustc_middle::ty;
1111
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -106,47 +106,3 @@ fn span_useless_format(cx: &LateContext<'_>, span: Span, mut sugg: String, mut a
106106
applicability,
107107
);
108108
}
109-
110-
fn is_display_arg(expr: &Expr<'_>) -> bool {
111-
if_chain! {
112-
if let ExprKind::Call(_, [_, fmt]) = expr.kind;
113-
if let ExprKind::Path(QPath::Resolved(_, path)) = fmt.kind;
114-
if let [.., t, _] = path.segments;
115-
if t.ident.name.as_str() == "Display";
116-
then { true } else { false }
117-
}
118-
}
119-
120-
/// Checks if the expression matches
121-
/// ```rust,ignore
122-
/// &[_ {
123-
/// format: _ {
124-
/// width: _::Implied,
125-
/// precision: _::Implied,
126-
/// ...
127-
/// },
128-
/// ...,
129-
/// }]
130-
/// ```
131-
fn check_unformatted(expr: &Expr<'_>) -> bool {
132-
if_chain! {
133-
if let ExprKind::AddrOf(BorrowKind::Ref, _, expr) = expr.kind;
134-
if let ExprKind::Array([expr]) = expr.kind;
135-
// struct `core::fmt::rt::v1::Argument`
136-
if let ExprKind::Struct(_, fields, _) = expr.kind;
137-
if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym::format);
138-
// struct `core::fmt::rt::v1::FormatSpec`
139-
if let ExprKind::Struct(_, fields, _) = format_field.expr.kind;
140-
if let Some(precision_field) = fields.iter().find(|f| f.ident.name == sym::precision);
141-
if let ExprKind::Path(ref precision_path) = precision_field.expr.kind;
142-
if last_path_segment(precision_path).ident.name == sym::Implied;
143-
if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym::width);
144-
if let ExprKind::Path(ref width_qpath) = width_field.expr.kind;
145-
if last_path_segment(width_qpath).ident.name == sym::Implied;
146-
then {
147-
return true;
148-
}
149-
}
150-
151-
false
152-
}

clippy_lints/src/format_args.rs

Lines changed: 262 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,262 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::format::{check_unformatted, is_display_arg};
3+
use clippy_utils::higher::{FormatArgsExpn, FormatExpn};
4+
use clippy_utils::source::snippet_opt;
5+
use clippy_utils::ty::implements_trait;
6+
use clippy_utils::{get_trait_def_id, match_def_path, paths};
7+
use if_chain::if_chain;
8+
use rustc_errors::Applicability;
9+
use rustc_hir::{Expr, ExprKind};
10+
use rustc_lint::{LateContext, LateLintPass};
11+
use rustc_session::{declare_lint_pass, declare_tool_lint};
12+
use rustc_span::{BytePos, ExpnKind, Span, Symbol};
13+
14+
declare_clippy_lint! {
15+
/// ### What it does
16+
/// Warns on `format!` within the arguments of another macro that does
17+
/// formatting such as `format!` itself, `write!` or `println!`. Suggests
18+
/// inlining the `format!` call.
19+
///
20+
/// ### Why is this bad?
21+
/// The recommended code is both shorter and avoids a temporary allocation.
22+
///
23+
/// ### Example
24+
/// ```rust
25+
/// # use std::panic::Location;
26+
/// println!("error: {}", format!("something failed at {}", Location::caller()));
27+
/// ```
28+
/// Use instead:
29+
/// ```rust
30+
/// # use std::panic::Location;
31+
/// println!("error: something failed at {}", Location::caller());
32+
/// ```
33+
pub FORMAT_IN_FORMAT_ARGS,
34+
perf,
35+
"`format!` used in a macro that does formatting"
36+
}
37+
38+
declare_lint_pass!(FormatInFormatArgs => [FORMAT_IN_FORMAT_ARGS]);
39+
40+
impl<'tcx> LateLintPass<'tcx> for FormatInFormatArgs {
41+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
42+
check_expr(cx, expr, format_in_format_args);
43+
}
44+
}
45+
46+
declare_clippy_lint! {
47+
/// ### What it does
48+
/// Checks for [`ToString::to_string`](https://doc.rust-lang.org/std/string/trait.ToString.html#tymethod.to_string)
49+
/// applied to a type that implements [`Display`](https://doc.rust-lang.org/std/fmt/trait.Display.html)
50+
/// in a macro that does formatting.
51+
///
52+
/// ### Why is this bad?
53+
/// Since the type implements `Display`, the use of `to_string` is
54+
/// unnecessary.
55+
///
56+
/// ### Example
57+
/// ```rust
58+
/// # use std::panic::Location;
59+
/// println!("error: something failed at {}", Location::caller().to_string());
60+
/// ```
61+
/// Use instead:
62+
/// ```rust
63+
/// # use std::panic::Location;
64+
/// println!("error: something failed at {}", Location::caller());
65+
/// ```
66+
pub TO_STRING_IN_FORMAT_ARGS,
67+
perf,
68+
"`to_string` applied to a type that implements `Display` in format args"
69+
}
70+
71+
declare_lint_pass!(ToStringInFormatArgs => [TO_STRING_IN_FORMAT_ARGS]);
72+
73+
impl<'tcx> LateLintPass<'tcx> for ToStringInFormatArgs {
74+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
75+
check_expr(cx, expr, to_string_in_format_args);
76+
}
77+
}
78+
79+
fn check_expr<'tcx, F>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, check_value: F)
80+
where
81+
F: Fn(&LateContext<'_>, &FormatArgsExpn<'_>, Span, Symbol, usize, &Expr<'_>) -> bool,
82+
{
83+
if_chain! {
84+
if let Some(format_args) = FormatArgsExpn::parse(expr);
85+
let call_site = expr.span.ctxt().outer_expn_data().call_site;
86+
if call_site.from_expansion();
87+
let expn_data = call_site.ctxt().outer_expn_data();
88+
if let ExpnKind::Macro(_, name) = expn_data.kind;
89+
if format_args.fmt_expr.map_or(true, check_unformatted);
90+
then {
91+
assert_eq!(format_args.args.len(), format_args.value_args.len());
92+
for (i, (arg, value)) in format_args.args.iter().zip(format_args.value_args.iter()).enumerate() {
93+
if !is_display_arg(arg) {
94+
continue;
95+
}
96+
if check_value(cx, &format_args, expn_data.call_site, name, i, value) {
97+
break;
98+
}
99+
}
100+
}
101+
}
102+
}
103+
104+
fn format_in_format_args(
105+
cx: &LateContext<'_>,
106+
format_args: &FormatArgsExpn<'_>,
107+
call_site: Span,
108+
name: Symbol,
109+
i: usize,
110+
value: &Expr<'_>,
111+
) -> bool {
112+
if_chain! {
113+
if let Some(FormatExpn{ format_args: inner_format_args, .. }) = FormatExpn::parse(value);
114+
if let Some(format_string) = snippet_opt(cx, format_args.format_string_span);
115+
if let Some(inner_format_string) = snippet_opt(cx, inner_format_args.format_string_span);
116+
if let Some((sugg, applicability)) = format_in_format_args_sugg(
117+
cx,
118+
name,
119+
&format_string,
120+
&format_args.value_args,
121+
i,
122+
&inner_format_string,
123+
&inner_format_args.value_args
124+
);
125+
then {
126+
span_lint_and_sugg(
127+
cx,
128+
FORMAT_IN_FORMAT_ARGS,
129+
trim_semicolon(cx, call_site),
130+
&format!("`format!` in `{}!` args", name),
131+
"inline the `format!` call",
132+
sugg,
133+
applicability,
134+
);
135+
// Report only the first instance.
136+
return true;
137+
}
138+
}
139+
false
140+
}
141+
142+
fn to_string_in_format_args(
143+
cx: &LateContext<'_>,
144+
_format_args: &FormatArgsExpn<'_>,
145+
_call_site: Span,
146+
name: Symbol,
147+
_i: usize,
148+
value: &Expr<'_>,
149+
) -> bool {
150+
if_chain! {
151+
if let ExprKind::MethodCall(_, _, args, _) = value.kind;
152+
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
153+
if match_def_path(cx, method_def_id, &paths::TO_STRING_METHOD);
154+
if let Some(receiver) = args.get(0);
155+
let ty = cx.typeck_results().expr_ty(receiver);
156+
if let Some(display_trait_id) = get_trait_def_id(cx, &paths::DISPLAY_TRAIT);
157+
if implements_trait(cx, ty, display_trait_id, &[]);
158+
if let Some(snippet) = snippet_opt(cx, value.span);
159+
if let Some(dot) = snippet.rfind('.');
160+
then {
161+
let span = value.span.with_lo(
162+
value.span.lo() + BytePos(u32::try_from(dot).unwrap())
163+
);
164+
span_lint_and_sugg(
165+
cx,
166+
TO_STRING_IN_FORMAT_ARGS,
167+
span,
168+
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
169+
"remove this",
170+
String::new(),
171+
Applicability::MachineApplicable,
172+
);
173+
}
174+
}
175+
false
176+
}
177+
178+
fn format_in_format_args_sugg(
179+
cx: &LateContext<'_>,
180+
name: Symbol,
181+
format_string: &str,
182+
values: &[&Expr<'_>],
183+
i: usize,
184+
inner_format_string: &str,
185+
inner_values: &[&Expr<'_>],
186+
) -> Option<(String, Applicability)> {
187+
let (left, right) = split_format_string(format_string, i);
188+
// If the inner format string is raw, the user is on their own.
189+
let (new_format_string, applicability) = if inner_format_string.starts_with('r') {
190+
(left + ".." + &right, Applicability::HasPlaceholders)
191+
} else {
192+
(
193+
left + &trim_quotes(inner_format_string) + &right,
194+
Applicability::MachineApplicable,
195+
)
196+
};
197+
let values = values
198+
.iter()
199+
.map(|value| snippet_opt(cx, value.span))
200+
.collect::<Option<Vec<_>>>()?;
201+
let inner_values = inner_values
202+
.iter()
203+
.map(|value| snippet_opt(cx, value.span))
204+
.collect::<Option<Vec<_>>>()?;
205+
let new_values = itertools::join(
206+
values
207+
.iter()
208+
.take(i)
209+
.chain(inner_values.iter())
210+
.chain(values.iter().skip(i + 1)),
211+
", ",
212+
);
213+
Some((
214+
format!(r#"{}!({}, {})"#, name, new_format_string, new_values),
215+
applicability,
216+
))
217+
}
218+
219+
fn split_format_string(format_string: &str, i: usize) -> (String, String) {
220+
let mut iter = format_string.chars();
221+
for j in 0..=i {
222+
assert!(advance(&mut iter) == '}' || j < i);
223+
}
224+
225+
let right = iter.collect::<String>();
226+
227+
let size = format_string.len();
228+
let right_size = right.len();
229+
let left_size = size - right_size;
230+
assert!(left_size >= 2);
231+
let left = std::str::from_utf8(&format_string.as_bytes()[..left_size - 2])
232+
.unwrap()
233+
.to_owned();
234+
(left, right)
235+
}
236+
237+
fn advance(iter: &mut std::str::Chars<'_>) -> char {
238+
loop {
239+
let first_char = iter.next().unwrap();
240+
if first_char != '{' {
241+
continue;
242+
}
243+
let second_char = iter.next().unwrap();
244+
if second_char == '{' {
245+
continue;
246+
}
247+
return second_char;
248+
}
249+
}
250+
251+
fn trim_quotes(string_literal: &str) -> String {
252+
let len = string_literal.chars().count();
253+
assert!(len >= 2);
254+
string_literal.chars().skip(1).take(len - 2).collect()
255+
}
256+
257+
fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
258+
snippet_opt(cx, span).map_or(span, |snippet| {
259+
let snippet = snippet.trim_end_matches(';');
260+
span.with_hi(span.lo() + BytePos(u32::try_from(snippet.len()).unwrap()))
261+
})
262+
}

clippy_lints/src/lib.mods.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ mod float_equality_without_abs;
6363
mod float_literal;
6464
mod floating_point_arithmetic;
6565
mod format;
66+
mod format_args;
6667
mod formatting;
6768
mod from_over_into;
6869
mod from_str_radix_10;

clippy_lints/src/lib.register_all.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
6060
LintId::of(float_equality_without_abs::FLOAT_EQUALITY_WITHOUT_ABS),
6161
LintId::of(float_literal::EXCESSIVE_PRECISION),
6262
LintId::of(format::USELESS_FORMAT),
63+
LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
64+
LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
6365
LintId::of(formatting::POSSIBLE_MISSING_COMMA),
6466
LintId::of(formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING),
6567
LintId::of(formatting::SUSPICIOUS_ELSE_FORMATTING),

clippy_lints/src/lib.register_lints.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ store.register_lints(&[
137137
floating_point_arithmetic::IMPRECISE_FLOPS,
138138
floating_point_arithmetic::SUBOPTIMAL_FLOPS,
139139
format::USELESS_FORMAT,
140+
format_args::FORMAT_IN_FORMAT_ARGS,
141+
format_args::TO_STRING_IN_FORMAT_ARGS,
140142
formatting::POSSIBLE_MISSING_COMMA,
141143
formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
142144
formatting::SUSPICIOUS_ELSE_FORMATTING,

clippy_lints/src/lib.register_perf.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
66
LintId::of(entry::MAP_ENTRY),
77
LintId::of(escape::BOXED_LOCAL),
8+
LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
9+
LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
810
LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
911
LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
1012
LintId::of(loops::MANUAL_MEMCPY),

clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
535535
store.register_late_pass(move || Box::new(feature_name::FeatureName));
536536
store.register_late_pass(move || Box::new(iter_not_returning_iterator::IterNotReturningIterator));
537537
store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic));
538+
store.register_late_pass(move || Box::new(format_args::FormatInFormatArgs));
539+
store.register_late_pass(move || Box::new(format_args::ToStringInFormatArgs));
538540
}
539541

540542
#[rustfmt::skip]

0 commit comments

Comments
 (0)