Skip to content

Commit 09d302a

Browse files
tesujitesuji
committed
Re-factor useless_format lint
1 parent 6e4aa66 commit 09d302a

File tree

3 files changed

+106
-96
lines changed

3 files changed

+106
-96
lines changed

‎clippy_lints/src/format.rs‎

Lines changed: 101 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
use crate::utils::paths;
2-
use crate::utils::{
3-
is_expn_of, last_path_segment, match_def_path, match_type, resolve_node, snippet, span_lint_and_then, walk_ptrs_ty,
4-
};
2+
use crate::utils::{is_expn_of, last_path_segment, match_def_path, resolve_node, snippet, span_lint_and_then};
53
use if_chain::if_chain;
64
use rustc::hir::*;
75
use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass};
8-
use rustc::ty;
96
use rustc::{declare_lint_pass, declare_tool_lint};
107
use rustc_errors::Applicability;
118
use syntax::ast::LitKind;
@@ -38,56 +35,16 @@ declare_lint_pass!(UselessFormat => [USELESS_FORMAT]);
3835

3936
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UselessFormat {
4037
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
41-
if let Some(span) = is_expn_of(expr.span, "format") {
42-
if span.from_expansion() {
43-
return;
44-
}
45-
match expr.node {
46-
// `format!("{}", foo)` expansion
47-
ExprKind::Call(ref fun, ref args) => {
48-
if_chain! {
49-
if let ExprKind::Path(ref qpath) = fun.node;
50-
if let Some(fun_def_id) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
51-
let new_v1 = match_def_path(cx, fun_def_id, &paths::FMT_ARGUMENTS_NEWV1);
52-
let new_v1_fmt = match_def_path(cx,
53-
fun_def_id,
54-
&paths::FMT_ARGUMENTS_NEWV1FORMATTED
55-
);
56-
if new_v1 || new_v1_fmt;
57-
if check_single_piece(&args[0]);
58-
if let Some(format_arg) = get_single_string_arg(cx, &args[1]);
59-
if new_v1 || check_unformatted(&args[2]);
60-
if let ExprKind::AddrOf(_, ref format_arg) = format_arg.node;
61-
then {
62-
let (message, sugg) = if_chain! {
63-
if let ExprKind::MethodCall(ref path, _, _) = format_arg.node;
64-
if path.ident.as_interned_str().as_symbol() == sym!(to_string);
65-
then {
66-
("`to_string()` is enough",
67-
snippet(cx, format_arg.span, "<arg>").to_string())
68-
} else {
69-
("consider using .to_string()",
70-
format!("{}.to_string()", snippet(cx, format_arg.span, "<arg>")))
71-
}
72-
};
38+
let span = match is_expn_of(expr.span, "format") {
39+
Some(s) if !s.from_expansion() => s,
40+
_ => return,
41+
};
7342

74-
span_useless_format(cx, span, message, sugg);
75-
}
76-
}
77-
},
78-
// `format!("foo")` expansion contains `match () { () => [], }`
79-
ExprKind::Match(ref matchee, _, _) => {
80-
if let ExprKind::Tup(ref tup) = matchee.node {
81-
if tup.is_empty() {
82-
let actual_snippet = snippet(cx, expr.span, "<expr>").to_string();
83-
let actual_snippet = actual_snippet.replace("{{}}", "{}");
84-
let sugg = format!("{}.to_string()", actual_snippet);
85-
span_useless_format(cx, span, "consider using .to_string()", sugg);
86-
}
87-
}
88-
},
89-
_ => (),
90-
}
43+
// Operate on the only argument of `alloc::fmt::format`.
44+
if let Some(sugg) = on_new_v1(cx, expr) {
45+
span_useless_format(cx, span, "consider using .to_string()", sugg);
46+
} else if let Some(sugg) = on_new_v1_fmt(cx, expr) {
47+
span_useless_format(cx, span, "consider using .to_string()", sugg);
9148
}
9249
}
9350
}
@@ -111,56 +68,105 @@ fn span_useless_format<T: LintContext>(cx: &T, span: Span, help: &str, mut sugg:
11168
});
11269
}
11370

114-
/// Checks if the expressions matches `&[""]`
115-
fn check_single_piece(expr: &Expr) -> bool {
71+
fn on_argumentv1_new<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, arms: &'a [Arm]) -> Option<String> {
11672
if_chain! {
117-
if let ExprKind::AddrOf(_, ref expr) = expr.node; // &[""]
118-
if let ExprKind::Array(ref exprs) = expr.node; // [""]
119-
if exprs.len() == 1;
120-
if let ExprKind::Lit(ref lit) = exprs[0].node;
121-
if let LitKind::Str(ref lit, _) = lit.node;
73+
if let ExprKind::AddrOf(_, ref format_args) = expr.node;
74+
if let ExprKind::Array(ref elems) = arms[0].body.node;
75+
if elems.len() == 1;
76+
if let ExprKind::Call(ref fun, ref args) = elems[0].node;
77+
if let ExprKind::Path(ref qpath) = fun.node;
78+
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
79+
if match_def_path(cx, did, &paths::FMT_ARGUMENTV1_NEW);
80+
// matches `core::fmt::Display::fmt`
81+
if args.len() == 2;
82+
if let ExprKind::Path(ref qpath) = args[1].node;
83+
if let Some(did) = resolve_node(cx, qpath, args[1].hir_id).opt_def_id();
84+
if match_def_path(cx, did, &paths::DISPLAY_FMT_METHOD);
85+
if arms[0].pats.len() == 1;
86+
// check `(arg0,)` in match block
87+
if let PatKind::Tuple(ref pats, None) = arms[0].pats[0].node;
88+
if pats.len() == 1;
12289
then {
123-
return lit.as_str().is_empty();
90+
if let ExprKind::Lit(ref lit) = format_args.node {
91+
if let LitKind::Str(ref s, _) = lit.node {
92+
let snip = s.as_str().replace("{{}}", "{}");
93+
let sugg = format!("\"{}\".to_string()", snip);
94+
return Some(sugg);
95+
}
96+
return None;
97+
} else {
98+
let snip = snippet(cx, format_args.span, "<arg>");
99+
if let ExprKind::MethodCall(ref path, _, _) = format_args.node {
100+
if path.ident.name == sym!(to_string) {
101+
return Some(format!("{}", snip));
102+
}
103+
}
104+
return Some(format!("{}.to_string()", snip));
105+
}
124106
}
125107
}
126-
127-
false
108+
None
128109
}
129110

130-
/// Checks if the expressions matches
131-
/// ```rust,ignore
132-
/// &match (&"arg",) {
133-
/// (__arg0,) => [::std::fmt::ArgumentV1::new(__arg0,
134-
/// ::std::fmt::Display::fmt)],
135-
/// }
136-
/// ```
137-
/// and that the type of `__arg0` is `&str` or `String`,
138-
/// then returns the span of first element of the matched tuple.
139-
fn get_single_string_arg<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option<&'a Expr> {
111+
fn on_new_v1<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<String> {
140112
if_chain! {
141-
if let ExprKind::AddrOf(_, ref expr) = expr.node;
142-
if let ExprKind::Match(ref match_expr, ref arms, _) = expr.node;
143-
if arms.len() == 1;
144-
if arms[0].pats.len() == 1;
145-
if let PatKind::Tuple(ref pat, None) = arms[0].pats[0].node;
146-
if pat.len() == 1;
147-
if let ExprKind::Array(ref exprs) = arms[0].body.node;
148-
if exprs.len() == 1;
149-
if let ExprKind::Call(_, ref args) = exprs[0].node;
113+
if let ExprKind::Call(ref fun, ref args) = expr.node;
150114
if args.len() == 2;
151-
if let ExprKind::Path(ref qpath) = args[1].node;
152-
if let Some(fun_def_id) = resolve_node(cx, qpath, args[1].hir_id).opt_def_id();
153-
if match_def_path(cx, fun_def_id, &paths::DISPLAY_FMT_METHOD);
115+
if let ExprKind::Path(ref qpath) = fun.node;
116+
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
117+
if match_def_path(cx, did, &paths::FMT_ARGUMENTS_NEW_V1);
118+
// Argument 1 in `new_v1()`
119+
if let ExprKind::AddrOf(_, ref arr) = args[0].node;
120+
if let ExprKind::Array(ref pieces) = arr.node;
121+
if pieces.len() == 1;
122+
if let ExprKind::Lit(ref lit) = pieces[0].node;
123+
if let LitKind::Str(ref s, _) = lit.node;
124+
// Argument 2 in `new_v1()`
125+
if let ExprKind::AddrOf(_, ref arg1) = args[1].node;
126+
if let ExprKind::Match(ref matchee, ref arms, MatchSource::Normal) = arg1.node;
127+
if arms.len() == 1;
128+
if let ExprKind::Tup(ref tup) = matchee.node;
154129
then {
155-
let ty = walk_ptrs_ty(cx.tables.pat_ty(&pat[0]));
156-
if ty.sty == ty::Str || match_type(cx, ty, &paths::STRING) {
157-
if let ExprKind::Tup(ref values) = match_expr.node {
158-
return Some(&values[0]);
130+
// `format!("foo")` expansion contains `match () { () => [], }`
131+
if tup.is_empty() {
132+
let snip = s.as_str().replace("{{}}", "{}");
133+
let sugg = format!("\"{}\".to_string()", snip);
134+
return Some(sugg);
135+
} else {
136+
if s.as_str().is_empty() {
137+
return on_argumentv1_new(cx, &tup[0], arms);
138+
} else {
139+
return None;
159140
}
160141
}
161142
}
162143
}
144+
None
145+
}
163146

147+
fn on_new_v1_fmt<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<String> {
148+
if_chain! {
149+
if let ExprKind::Call(ref fun, ref args) = expr.node;
150+
if args.len() == 3;
151+
if let ExprKind::Path(ref qpath) = fun.node;
152+
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
153+
if match_def_path(cx, did, &paths::FMT_ARGUMENTS_NEW_V1_FORMATTED);
154+
if check_unformatted(&args[2]);
155+
// Argument 1 in `new_v1_formatted()`
156+
if let ExprKind::AddrOf(_, ref arr) = args[0].node;
157+
if let ExprKind::Array(ref pieces) = arr.node;
158+
if pieces.len() == 1;
159+
if let ExprKind::Lit(ref lit) = pieces[0].node;
160+
if let LitKind::Str(..) = lit.node;
161+
// Argument 2 in `new_v1_formatted()`
162+
if let ExprKind::AddrOf(_, ref arg1) = args[1].node;
163+
if let ExprKind::Match(ref matchee, ref arms, MatchSource::Normal) = arg1.node;
164+
if arms.len() == 1;
165+
if let ExprKind::Tup(ref tup) = matchee.node;
166+
then {
167+
return on_argumentv1_new(cx, &tup[0], arms);
168+
}
169+
}
164170
None
165171
}
166172

@@ -169,6 +175,7 @@ fn get_single_string_arg<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option
169175
/// &[_ {
170176
/// format: _ {
171177
/// width: _::Implied,
178+
/// precision: _::Implied,
172179
/// ...
173180
/// },
174181
/// ...,
@@ -179,15 +186,17 @@ fn check_unformatted(expr: &Expr) -> bool {
179186
if let ExprKind::AddrOf(_, ref expr) = expr.node;
180187
if let ExprKind::Array(ref exprs) = expr.node;
181188
if exprs.len() == 1;
189+
// struct `core::fmt::rt::v1::Argument`
182190
if let ExprKind::Struct(_, ref fields, _) = exprs[0].node;
183191
if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym!(format));
192+
// struct `core::fmt::rt::v1::FormatSpec`
184193
if let ExprKind::Struct(_, ref fields, _) = format_field.expr.node;
185-
if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym!(width));
186-
if let ExprKind::Path(ref width_qpath) = width_field.expr.node;
187-
if last_path_segment(width_qpath).ident.name == sym!(Implied);
188194
if let Some(precision_field) = fields.iter().find(|f| f.ident.name == sym!(precision));
189195
if let ExprKind::Path(ref precision_path) = precision_field.expr.node;
190196
if last_path_segment(precision_path).ident.name == sym!(Implied);
197+
if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym!(width));
198+
if let ExprKind::Path(ref width_qpath) = width_field.expr.node;
199+
if last_path_segment(width_qpath).ident.name == sym!(Implied);
191200
then {
192201
return true;
193202
}

‎clippy_lints/src/utils/paths.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,9 @@ pub const DROP: [&str; 3] = ["core", "mem", "drop"];
2727
pub const DROP_TRAIT: [&str; 4] = ["core", "ops", "drop", "Drop"];
2828
pub const DURATION: [&str; 3] = ["core", "time", "Duration"];
2929
pub const EARLY_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "EarlyContext"];
30-
pub const FMT_ARGUMENTS_NEWV1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
31-
pub const FMT_ARGUMENTS_NEWV1FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
30+
pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
31+
pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
32+
pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
3233
pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
3334
pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
3435
pub const HASH: [&str; 2] = ["hash", "Hash"];

‎tests/ui/format.stderr‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,13 @@ error: useless use of `format!`
5858
--> $DIR/format.rs:59:5
5959
|
6060
LL | format!("{}", 42.to_string());
61-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `42.to_string();`
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `42.to_string();`
6262

6363
error: useless use of `format!`
6464
--> $DIR/format.rs:61:5
6565
|
6666
LL | format!("{}", x.display().to_string());
67-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `x.display().to_string();`
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `x.display().to_string();`
6868

6969
error: aborting due to 11 previous errors
7070

0 commit comments

Comments
 (0)