Skip to content

Commit f52a5da

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 0c8799d commit f52a5da

11 files changed

+428
-46
lines changed

CHANGELOG.md

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

clippy_lints/src/lib.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ mod float_equality_without_abs;
217217
mod float_literal;
218218
mod floating_point_arithmetic;
219219
mod format;
220+
mod format_args;
220221
mod formatting;
221222
mod from_over_into;
222223
mod from_str_radix_10;
@@ -641,6 +642,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
641642
floating_point_arithmetic::IMPRECISE_FLOPS,
642643
floating_point_arithmetic::SUBOPTIMAL_FLOPS,
643644
format::USELESS_FORMAT,
645+
format_args::FORMAT_IN_FORMAT_ARGS,
646+
format_args::TO_STRING_IN_FORMAT_ARGS,
644647
formatting::POSSIBLE_MISSING_COMMA,
645648
formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
646649
formatting::SUSPICIOUS_ELSE_FORMATTING,
@@ -1780,6 +1783,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
17801783
store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
17811784
LintId::of(entry::MAP_ENTRY),
17821785
LintId::of(escape::BOXED_LOCAL),
1786+
LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
1787+
LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
17831788
LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
17841789
LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
17851790
LintId::of(loops::MANUAL_MEMCPY),
@@ -2142,6 +2147,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
21422147
store.register_late_pass(move || Box::new(feature_name::FeatureName));
21432148
store.register_late_pass(move || Box::new(iter_not_returning_iterator::IterNotReturningIterator));
21442149
store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic));
2150+
store.register_late_pass(move || Box::new(format_args::FormatInFormatArgs));
2151+
store.register_late_pass(move || Box::new(format_args::ToStringInFormatArgs));
21452152
}
21462153

21472154
#[rustfmt::skip]

0 commit comments

Comments
 (0)