Skip to content

Commit c9599d7

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 4996e17 commit c9599d7

14 files changed

+908
-49
lines changed

CHANGELOG.md

+2
Original file line numberDiff line numberDiff line change
@@ -2736,6 +2736,7 @@ Released 2018-09-13
27362736
[`for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles
27372737
[`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
27382738
[`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
2739+
[`format_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args
27392740
[`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect
27402741
[`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into
27412742
[`from_str_radix_10`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_str_radix_10
@@ -3015,6 +3016,7 @@ Released 2018-09-13
30153016
[`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
30163017
[`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some
30173018
[`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display
3019+
[`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
30183020
[`todo`]: https://rust-lang.github.io/rust-clippy/master/index.html#todo
30193021
[`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
30203022
[`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines

clippy_lints/src/format.rs

+3-48
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
22
use clippy_utils::higher::FormatExpn;
3-
use clippy_utils::last_path_segment;
43
use clippy_utils::source::{snippet_opt, snippet_with_applicability};
54
use clippy_utils::sugg::Sugg;
65
use if_chain::if_chain;
76
use rustc_errors::Applicability;
8-
use rustc_hir::{BorrowKind, Expr, ExprKind, QPath};
7+
use rustc_hir::{Expr, ExprKind};
98
use rustc_lint::{LateContext, LateLintPass};
109
use rustc_middle::ty;
1110
use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -69,8 +68,8 @@ impl<'tcx> LateLintPass<'tcx> for UselessFormat {
6968
ty::Str => true,
7069
_ => false,
7170
};
72-
if format_args.args.iter().all(is_display_arg);
73-
if format_args.fmt_expr.map_or(true, check_unformatted);
71+
if let Some(args) = format_args.args();
72+
if args.iter().all(|arg| arg.is_display() && !arg.has_string_formatting());
7473
then {
7574
let is_new_string = match value.kind {
7675
ExprKind::Binary(..) => true,
@@ -106,47 +105,3 @@ fn span_useless_format(cx: &LateContext<'_>, span: Span, mut sugg: String, mut a
106105
applicability,
107106
);
108107
}
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 == sym::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

+223
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2+
use clippy_utils::higher::{FormatArgsArg, FormatArgsExpn, FormatExpn};
3+
use clippy_utils::source::snippet_opt;
4+
use clippy_utils::ty::implements_trait;
5+
use clippy_utils::{is_diag_trait_item, match_def_path, paths};
6+
use if_chain::if_chain;
7+
use rustc_errors::Applicability;
8+
use rustc_hir::{Expr, ExprKind};
9+
use rustc_lint::{LateContext, LateLintPass};
10+
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
11+
use rustc_middle::ty::Ty;
12+
use rustc_session::{declare_lint_pass, declare_tool_lint};
13+
use rustc_span::{sym, BytePos, ExpnData, ExpnKind, Span, Symbol};
14+
15+
declare_clippy_lint! {
16+
/// ### What it does
17+
/// Detects `format!` within the arguments of another macro that does
18+
/// formatting such as `format!` itself, `write!` or `println!`. Suggests
19+
/// inlining the `format!` call.
20+
///
21+
/// ### Why is this bad?
22+
/// The recommended code is both shorter and avoids a temporary allocation.
23+
///
24+
/// ### Example
25+
/// ```rust
26+
/// # use std::panic::Location;
27+
/// println!("error: {}", format!("something failed at {}", Location::caller()));
28+
/// ```
29+
/// Use instead:
30+
/// ```rust
31+
/// # use std::panic::Location;
32+
/// println!("error: something failed at {}", Location::caller());
33+
/// ```
34+
pub FORMAT_IN_FORMAT_ARGS,
35+
perf,
36+
"`format!` used in a macro that does formatting"
37+
}
38+
39+
declare_clippy_lint! {
40+
/// ### What it does
41+
/// Checks for [`ToString::to_string`](https://doc.rust-lang.org/std/string/trait.ToString.html#tymethod.to_string)
42+
/// applied to a type that implements [`Display`](https://doc.rust-lang.org/std/fmt/trait.Display.html)
43+
/// in a macro that does formatting.
44+
///
45+
/// ### Why is this bad?
46+
/// Since the type implements `Display`, the use of `to_string` is
47+
/// unnecessary.
48+
///
49+
/// ### Example
50+
/// ```rust
51+
/// # use std::panic::Location;
52+
/// println!("error: something failed at {}", Location::caller().to_string());
53+
/// ```
54+
/// Use instead:
55+
/// ```rust
56+
/// # use std::panic::Location;
57+
/// println!("error: something failed at {}", Location::caller());
58+
/// ```
59+
pub TO_STRING_IN_FORMAT_ARGS,
60+
perf,
61+
"`to_string` applied to a type that implements `Display` in format args"
62+
}
63+
64+
declare_lint_pass!(FormatArgs => [FORMAT_IN_FORMAT_ARGS, TO_STRING_IN_FORMAT_ARGS]);
65+
66+
const FORMAT_MACRO_PATHS: &[&[&str]] = &[
67+
&paths::FORMAT_ARGS_MACRO,
68+
&paths::ASSERT_EQ_MACRO,
69+
&paths::ASSERT_MACRO,
70+
&paths::ASSERT_NE_MACRO,
71+
&paths::EPRINT_MACRO,
72+
&paths::EPRINTLN_MACRO,
73+
&paths::PRINT_MACRO,
74+
&paths::PRINTLN_MACRO,
75+
&paths::WRITE_MACRO,
76+
&paths::WRITELN_MACRO,
77+
];
78+
79+
const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[sym::format_macro, sym::std_panic_macro];
80+
81+
impl<'tcx> LateLintPass<'tcx> for FormatArgs {
82+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
83+
if_chain! {
84+
if let Some(format_args) = FormatArgsExpn::parse(expr);
85+
let expr_expn_data = expr.span.ctxt().outer_expn_data();
86+
let outermost_expn_data = outermost_expn_data(expr_expn_data);
87+
if let Some(macro_def_id) = outermost_expn_data.macro_def_id;
88+
if FORMAT_MACRO_PATHS
89+
.iter()
90+
.any(|path| match_def_path(cx, macro_def_id, path))
91+
|| FORMAT_MACRO_DIAG_ITEMS
92+
.iter()
93+
.any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, macro_def_id));
94+
if let ExpnKind::Macro(_, name) = outermost_expn_data.kind;
95+
if let Some(args) = format_args.args();
96+
then {
97+
for (i, arg) in args.iter().enumerate() {
98+
if !arg.is_display() {
99+
continue;
100+
}
101+
if arg.has_string_formatting() {
102+
continue;
103+
}
104+
if is_aliased(&args, i) {
105+
continue;
106+
}
107+
check_format_in_format_args(cx, outermost_expn_data.call_site, name, arg);
108+
check_to_string_in_format_args(cx, name, arg);
109+
}
110+
}
111+
}
112+
}
113+
}
114+
115+
fn outermost_expn_data(expn_data: ExpnData) -> ExpnData {
116+
if expn_data.call_site.from_expansion() {
117+
outermost_expn_data(expn_data.call_site.ctxt().outer_expn_data())
118+
} else {
119+
expn_data
120+
}
121+
}
122+
123+
fn check_format_in_format_args(cx: &LateContext<'_>, call_site: Span, name: Symbol, arg: &FormatArgsArg<'_>) {
124+
if_chain! {
125+
if FormatExpn::parse(arg.value).is_some();
126+
if !arg.value.span.ctxt().outer_expn_data().call_site.from_expansion();
127+
then {
128+
span_lint_and_then(
129+
cx,
130+
FORMAT_IN_FORMAT_ARGS,
131+
trim_semicolon(cx, call_site),
132+
&format!("`format!` in `{}!` args", name),
133+
|diag| {
134+
diag.help(&format!(
135+
"combine the `format!(..)` arguments with the outer `{}!(..)` call",
136+
name
137+
));
138+
diag.help("or consider changing `format!` to `format_args!`");
139+
},
140+
);
141+
}
142+
}
143+
}
144+
145+
fn check_to_string_in_format_args<'tcx>(cx: &LateContext<'tcx>, name: Symbol, arg: &FormatArgsArg<'tcx>) {
146+
let value = arg.value;
147+
if_chain! {
148+
if !value.span.from_expansion();
149+
if let ExprKind::MethodCall(_, _, [receiver], _) = value.kind;
150+
if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(value.hir_id);
151+
if is_diag_trait_item(cx, method_def_id, sym::ToString);
152+
let receiver_ty = cx.typeck_results().expr_ty(receiver);
153+
if let Some(display_trait_id) = cx.tcx.get_diagnostic_item(sym::Display);
154+
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
155+
then {
156+
let (n_needed_derefs, target) = count_needed_derefs(
157+
receiver_ty,
158+
cx.typeck_results().expr_adjustments(receiver).iter(),
159+
);
160+
if implements_trait(cx, target, display_trait_id, &[]) {
161+
if n_needed_derefs == 0 {
162+
span_lint_and_sugg(
163+
cx,
164+
TO_STRING_IN_FORMAT_ARGS,
165+
value.span.with_lo(receiver.span.hi()),
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+
} else {
172+
span_lint_and_sugg(
173+
cx,
174+
TO_STRING_IN_FORMAT_ARGS,
175+
value.span,
176+
&format!("`to_string` applied to a type that implements `Display` in `{}!` args", name),
177+
"use this",
178+
format!("{:*>width$}{}", "", receiver_snippet, width = n_needed_derefs),
179+
Applicability::MachineApplicable,
180+
);
181+
}
182+
}
183+
}
184+
}
185+
}
186+
187+
// Returns true if `args[i]` "refers to" or "is referred to by" another argument.
188+
fn is_aliased(args: &[FormatArgsArg<'_>], i: usize) -> bool {
189+
let value = args[i].value;
190+
args.iter()
191+
.enumerate()
192+
.any(|(j, arg)| i != j && std::ptr::eq(value, arg.value))
193+
}
194+
195+
fn trim_semicolon(cx: &LateContext<'_>, span: Span) -> Span {
196+
snippet_opt(cx, span).map_or(span, |snippet| {
197+
let snippet = snippet.trim_end_matches(';');
198+
span.with_hi(span.lo() + BytePos(u32::try_from(snippet.len()).unwrap()))
199+
})
200+
}
201+
202+
fn count_needed_derefs<'tcx, I>(mut ty: Ty<'tcx>, mut iter: I) -> (usize, Ty<'tcx>)
203+
where
204+
I: Iterator<Item = &'tcx Adjustment<'tcx>>,
205+
{
206+
let mut n_total = 0;
207+
let mut n_needed = 0;
208+
loop {
209+
if let Some(Adjustment {
210+
kind: Adjust::Deref(overloaded_deref),
211+
target,
212+
}) = iter.next()
213+
{
214+
n_total += 1;
215+
if overloaded_deref.is_some() {
216+
n_needed = n_total;
217+
}
218+
ty = target;
219+
} else {
220+
return (n_needed, ty);
221+
}
222+
}
223+
}

clippy_lints/src/lib.register_all.rs

+2
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

+2
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ store.register_lints(&[
139139
floating_point_arithmetic::IMPRECISE_FLOPS,
140140
floating_point_arithmetic::SUBOPTIMAL_FLOPS,
141141
format::USELESS_FORMAT,
142+
format_args::FORMAT_IN_FORMAT_ARGS,
143+
format_args::TO_STRING_IN_FORMAT_ARGS,
142144
formatting::POSSIBLE_MISSING_COMMA,
143145
formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING,
144146
formatting::SUSPICIOUS_ELSE_FORMATTING,

clippy_lints/src/lib.register_perf.rs

+2
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

+2
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,7 @@ mod float_equality_without_abs;
218218
mod float_literal;
219219
mod floating_point_arithmetic;
220220
mod format;
221+
mod format_args;
221222
mod formatting;
222223
mod from_over_into;
223224
mod from_str_radix_10;
@@ -775,6 +776,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
775776
store.register_late_pass(move || Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(enable_raw_pointer_heuristic_for_send)));
776777
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
777778
store.register_late_pass(|| Box::new(match_str_case_mismatch::MatchStrCaseMismatch));
779+
store.register_late_pass(move || Box::new(format_args::FormatArgs));
778780
}
779781

780782
#[rustfmt::skip]

0 commit comments

Comments
 (0)