Skip to content

Commit 7a11843

Browse files
borsbors
committed
Auto merge of #4439 - lzutao:fix-format, r=phansch
Re-factor format lint cc #4432 changelog: none
2 parents 6e4aa66 + ab335ea commit 7a11843

File tree

6 files changed

+141
-104
lines changed

6 files changed

+141
-104
lines changed

‎clippy_lints/src/format.rs

Lines changed: 98 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ use crate::utils::{
55
use if_chain::if_chain;
66
use rustc::hir::*;
77
use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass};
8-
use rustc::ty;
98
use rustc::{declare_lint_pass, declare_tool_lint};
109
use rustc_errors::Applicability;
1110
use syntax::ast::LitKind;
@@ -38,56 +37,16 @@ declare_lint_pass!(UselessFormat => [USELESS_FORMAT]);
3837

3938
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UselessFormat {
4039
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-
};
40+
let span = match is_expn_of(expr.span, "format") {
41+
Some(s) if !s.from_expansion() => s,
42+
_ => return,
43+
};
7344

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-
}
45+
// Operate on the only argument of `alloc::fmt::format`.
46+
if let Some(sugg) = on_new_v1(cx, expr) {
47+
span_useless_format(cx, span, "consider using .to_string()", sugg);
48+
} else if let Some(sugg) = on_new_v1_fmt(cx, expr) {
49+
span_useless_format(cx, span, "consider using .to_string()", sugg);
9150
}
9251
}
9352
}
@@ -111,56 +70,102 @@ fn span_useless_format<T: LintContext>(cx: &T, span: Span, help: &str, mut sugg:
11170
});
11271
}
11372

114-
/// Checks if the expressions matches `&[""]`
115-
fn check_single_piece(expr: &Expr) -> bool {
73+
fn on_argumentv1_new<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, arms: &'a [Arm]) -> Option<String> {
11674
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;
75+
if let ExprKind::AddrOf(_, ref format_args) = expr.node;
76+
if let ExprKind::Array(ref elems) = arms[0].body.node;
77+
if elems.len() == 1;
78+
if let ExprKind::Call(ref fun, ref args) = elems[0].node;
79+
if let ExprKind::Path(ref qpath) = fun.node;
80+
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
81+
if match_def_path(cx, did, &paths::FMT_ARGUMENTV1_NEW);
82+
// matches `core::fmt::Display::fmt`
83+
if args.len() == 2;
84+
if let ExprKind::Path(ref qpath) = args[1].node;
85+
if let Some(did) = resolve_node(cx, qpath, args[1].hir_id).opt_def_id();
86+
if match_def_path(cx, did, &paths::DISPLAY_FMT_METHOD);
87+
if arms[0].pats.len() == 1;
88+
// check `(arg0,)` in match block
89+
if let PatKind::Tuple(ref pats, None) = arms[0].pats[0].node;
90+
if pats.len() == 1;
12291
then {
123-
return lit.as_str().is_empty();
92+
let ty = walk_ptrs_ty(cx.tables.pat_ty(&pats[0]));
93+
if ty.sty != rustc::ty::Str && !match_type(cx, ty, &paths::STRING) {
94+
return None;
95+
}
96+
if let ExprKind::Lit(ref lit) = format_args.node {
97+
if let LitKind::Str(ref s, _) = lit.node {
98+
return Some(format!("{:?}.to_string()", s.as_str()));
99+
}
100+
} else {
101+
let snip = snippet(cx, format_args.span, "<arg>");
102+
if let ExprKind::MethodCall(ref path, _, _) = format_args.node {
103+
if path.ident.name == sym!(to_string) {
104+
return Some(format!("{}", snip));
105+
}
106+
} else if let ExprKind::Binary(..) = format_args.node {
107+
return Some(format!("{}", snip));
108+
}
109+
return Some(format!("{}.to_string()", snip));
110+
}
124111
}
125112
}
126-
127-
false
113+
None
128114
}
129115

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> {
116+
fn on_new_v1<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<String> {
140117
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;
118+
if let ExprKind::Call(ref fun, ref args) = expr.node;
150119
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);
120+
if let ExprKind::Path(ref qpath) = fun.node;
121+
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
122+
if match_def_path(cx, did, &paths::FMT_ARGUMENTS_NEW_V1);
123+
// Argument 1 in `new_v1()`
124+
if let ExprKind::AddrOf(_, ref arr) = args[0].node;
125+
if let ExprKind::Array(ref pieces) = arr.node;
126+
if pieces.len() == 1;
127+
if let ExprKind::Lit(ref lit) = pieces[0].node;
128+
if let LitKind::Str(ref s, _) = lit.node;
129+
// Argument 2 in `new_v1()`
130+
if let ExprKind::AddrOf(_, ref arg1) = args[1].node;
131+
if let ExprKind::Match(ref matchee, ref arms, MatchSource::Normal) = arg1.node;
132+
if arms.len() == 1;
133+
if let ExprKind::Tup(ref tup) = matchee.node;
154134
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]);
159-
}
135+
// `format!("foo")` expansion contains `match () { () => [], }`
136+
if tup.is_empty() {
137+
return Some(format!("{:?}.to_string()", s.as_str()));
138+
} else if s.as_str().is_empty() {
139+
return on_argumentv1_new(cx, &tup[0], arms);
160140
}
161141
}
162142
}
143+
None
144+
}
163145

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

@@ -169,6 +174,7 @@ fn get_single_string_arg<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option
169174
/// &[_ {
170175
/// format: _ {
171176
/// width: _::Implied,
177+
/// precision: _::Implied,
172178
/// ...
173179
/// },
174180
/// ...,
@@ -179,15 +185,17 @@ fn check_unformatted(expr: &Expr) -> bool {
179185
if let ExprKind::AddrOf(_, ref expr) = expr.node;
180186
if let ExprKind::Array(ref exprs) = expr.node;
181187
if exprs.len() == 1;
188+
// struct `core::fmt::rt::v1::Argument`
182189
if let ExprKind::Struct(_, ref fields, _) = exprs[0].node;
183190
if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym!(format));
191+
// struct `core::fmt::rt::v1::FormatSpec`
184192
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);
188193
if let Some(precision_field) = fields.iter().find(|f| f.ident.name == sym!(precision));
189194
if let ExprKind::Path(ref precision_path) = precision_field.expr.node;
190195
if last_path_segment(precision_path).ident.name == sym!(Implied);
196+
if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym!(width));
197+
if let ExprKind::Path(ref width_qpath) = width_field.expr.node;
198+
if last_path_segment(width_qpath).ident.name == sym!(Implied);
191199
then {
192200
return true;
193201
}

‎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"];

‎src/lintlist/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1879,7 +1879,7 @@ pub const ALL_LINTS: [Lint; 311] = [
18791879
Lint {
18801880
name: "unicode_not_nfc",
18811881
group: "pedantic",
1882-
desc: "using a unicode literal not in NFC normal form (see [unicode tr15](http://www.unicode.org/reports/tr15/) for further information)",
1882+
desc: "using a Unicode literal not in NFC normal form (see [Unicode tr15](http://www.unicode.org/reports/tr15/) for further information)",
18831883
deprecation: None,
18841884
module: "unicode",
18851885
},

‎tests/ui/format.fixed

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ fn main() {
1313
"foo".to_string();
1414
"{}".to_string();
1515
"{} abc {}".to_string();
16+
"foo {}\n\" bar".to_string();
1617

1718
"foo".to_string();
1819
format!("{:?}", "foo"); // Don't warn about `Debug`.
@@ -59,4 +60,8 @@ fn main() {
5960
42.to_string();
6061
let x = std::path::PathBuf::from("/bar/foo/qux");
6162
x.display().to_string();
63+
64+
// False positive
65+
let a = "foo".to_string();
66+
let _ = Some(a + "bar");
6267
}

‎tests/ui/format.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ fn main() {
1313
format!("foo");
1414
format!("{{}}");
1515
format!("{{}} abc {{}}");
16+
format!(
17+
r##"foo {{}}
18+
" bar"##
19+
);
1620

1721
format!("{}", "foo");
1822
format!("{:?}", "foo"); // Don't warn about `Debug`.
@@ -59,4 +63,8 @@ fn main() {
5963
format!("{}", 42.to_string());
6064
let x = std::path::PathBuf::from("/bar/foo/qux");
6165
format!("{}", x.display().to_string());
66+
67+
// False positive
68+
let a = "foo".to_string();
69+
let _ = Some(format!("{}", a + "bar"));
6270
}

‎tests/ui/format.stderr

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,52 +19,67 @@ LL | format!("{{}} abc {{}}");
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"{} abc {}".to_string();`
2020

2121
error: useless use of `format!`
22-
--> $DIR/format.rs:17:5
22+
--> $DIR/format.rs:16:5
23+
|
24+
LL | / format!(
25+
LL | | r##"foo {{}}
26+
LL | | " bar"##
27+
LL | | );
28+
| |______^ help: consider using .to_string(): `"foo {}/n/" bar".to_string();`
29+
30+
error: useless use of `format!`
31+
--> $DIR/format.rs:21:5
2332
|
2433
LL | format!("{}", "foo");
2534
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();`
2635

2736
error: useless use of `format!`
28-
--> $DIR/format.rs:21:5
37+
--> $DIR/format.rs:25:5
2938
|
3039
LL | format!("{:+}", "foo"); // Warn when the format makes no difference.
3140
| ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();`
3241

3342
error: useless use of `format!`
34-
--> $DIR/format.rs:22:5
43+
--> $DIR/format.rs:26:5
3544
|
3645
LL | format!("{:<}", "foo"); // Warn when the format makes no difference.
3746
| ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();`
3847

3948
error: useless use of `format!`
40-
--> $DIR/format.rs:27:5
49+
--> $DIR/format.rs:31:5
4150
|
4251
LL | format!("{}", arg);
4352
| ^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string();`
4453

4554
error: useless use of `format!`
46-
--> $DIR/format.rs:31:5
55+
--> $DIR/format.rs:35:5
4756
|
4857
LL | format!("{:+}", arg); // Warn when the format makes no difference.
4958
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string();`
5059

5160
error: useless use of `format!`
52-
--> $DIR/format.rs:32:5
61+
--> $DIR/format.rs:36:5
5362
|
5463
LL | format!("{:<}", arg); // Warn when the format makes no difference.
5564
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `arg.to_string();`
5665

5766
error: useless use of `format!`
58-
--> $DIR/format.rs:59:5
67+
--> $DIR/format.rs:63:5
5968
|
6069
LL | format!("{}", 42.to_string());
61-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `42.to_string();`
70+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `42.to_string();`
6271

6372
error: useless use of `format!`
64-
--> $DIR/format.rs:61:5
73+
--> $DIR/format.rs:65:5
6574
|
6675
LL | format!("{}", x.display().to_string());
67-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `x.display().to_string();`
76+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `x.display().to_string();`
77+
78+
error: useless use of `format!`
79+
--> $DIR/format.rs:69:18
80+
|
81+
LL | let _ = Some(format!("{}", a + "bar"));
82+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `a + "bar"`
6883

69-
error: aborting due to 11 previous errors
84+
error: aborting due to 13 previous errors
7085

0 commit comments

Comments
 (0)