Skip to content

Re-factor format lint #4439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 23, 2019
Merged

Re-factor format lint #4439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09d302a
Re-factor useless_format lint
tesuji Aug 23, 2019
ce2d292
Add raw string regression test for useless_format lint
tesuji Aug 23, 2019
f717a77
Re-add false positive check
tesuji Aug 23, 2019
ab335ea
Run update_lints for Unicode lint
tesuji Aug 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 98 additions & 90 deletions clippy_lints/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use crate::utils::{
use if_chain::if_chain;
use rustc::hir::*;
use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass};
use rustc::ty;
use rustc::{declare_lint_pass, declare_tool_lint};
use rustc_errors::Applicability;
use syntax::ast::LitKind;
Expand Down Expand Up @@ -38,56 +37,16 @@ declare_lint_pass!(UselessFormat => [USELESS_FORMAT]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UselessFormat {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
if let Some(span) = is_expn_of(expr.span, "format") {
if span.from_expansion() {
return;
}
match expr.node {
// `format!("{}", foo)` expansion
ExprKind::Call(ref fun, ref args) => {
if_chain! {
if let ExprKind::Path(ref qpath) = fun.node;
if let Some(fun_def_id) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
let new_v1 = match_def_path(cx, fun_def_id, &paths::FMT_ARGUMENTS_NEWV1);
let new_v1_fmt = match_def_path(cx,
fun_def_id,
&paths::FMT_ARGUMENTS_NEWV1FORMATTED
);
if new_v1 || new_v1_fmt;
if check_single_piece(&args[0]);
if let Some(format_arg) = get_single_string_arg(cx, &args[1]);
if new_v1 || check_unformatted(&args[2]);
if let ExprKind::AddrOf(_, ref format_arg) = format_arg.node;
then {
let (message, sugg) = if_chain! {
if let ExprKind::MethodCall(ref path, _, _) = format_arg.node;
if path.ident.as_interned_str().as_symbol() == sym!(to_string);
then {
("`to_string()` is enough",
snippet(cx, format_arg.span, "<arg>").to_string())
} else {
("consider using .to_string()",
format!("{}.to_string()", snippet(cx, format_arg.span, "<arg>")))
}
};
let span = match is_expn_of(expr.span, "format") {
Some(s) if !s.from_expansion() => s,
_ => return,
};

span_useless_format(cx, span, message, sugg);
}
}
},
// `format!("foo")` expansion contains `match () { () => [], }`
ExprKind::Match(ref matchee, _, _) => {
if let ExprKind::Tup(ref tup) = matchee.node {
if tup.is_empty() {
let actual_snippet = snippet(cx, expr.span, "<expr>").to_string();
let actual_snippet = actual_snippet.replace("{{}}", "{}");
let sugg = format!("{}.to_string()", actual_snippet);
span_useless_format(cx, span, "consider using .to_string()", sugg);
}
}
},
_ => (),
}
// Operate on the only argument of `alloc::fmt::format`.
if let Some(sugg) = on_new_v1(cx, expr) {
span_useless_format(cx, span, "consider using .to_string()", sugg);
} else if let Some(sugg) = on_new_v1_fmt(cx, expr) {
span_useless_format(cx, span, "consider using .to_string()", sugg);
}
}
}
Expand All @@ -111,56 +70,102 @@ fn span_useless_format<T: LintContext>(cx: &T, span: Span, help: &str, mut sugg:
});
}

/// Checks if the expressions matches `&[""]`
fn check_single_piece(expr: &Expr) -> bool {
fn on_argumentv1_new<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, arms: &'a [Arm]) -> Option<String> {
if_chain! {
if let ExprKind::AddrOf(_, ref expr) = expr.node; // &[""]
if let ExprKind::Array(ref exprs) = expr.node; // [""]
if exprs.len() == 1;
if let ExprKind::Lit(ref lit) = exprs[0].node;
if let LitKind::Str(ref lit, _) = lit.node;
if let ExprKind::AddrOf(_, ref format_args) = expr.node;
if let ExprKind::Array(ref elems) = arms[0].body.node;
if elems.len() == 1;
if let ExprKind::Call(ref fun, ref args) = elems[0].node;
if let ExprKind::Path(ref qpath) = fun.node;
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
if match_def_path(cx, did, &paths::FMT_ARGUMENTV1_NEW);
// matches `core::fmt::Display::fmt`
if args.len() == 2;
if let ExprKind::Path(ref qpath) = args[1].node;
if let Some(did) = resolve_node(cx, qpath, args[1].hir_id).opt_def_id();
if match_def_path(cx, did, &paths::DISPLAY_FMT_METHOD);
if arms[0].pats.len() == 1;
// check `(arg0,)` in match block
if let PatKind::Tuple(ref pats, None) = arms[0].pats[0].node;
if pats.len() == 1;
then {
return lit.as_str().is_empty();
let ty = walk_ptrs_ty(cx.tables.pat_ty(&pats[0]));
if ty.sty != rustc::ty::Str && !match_type(cx, ty, &paths::STRING) {
return None;
}
if let ExprKind::Lit(ref lit) = format_args.node {
if let LitKind::Str(ref s, _) = lit.node {
return Some(format!("{:?}.to_string()", s.as_str()));
}
} else {
let snip = snippet(cx, format_args.span, "<arg>");
if let ExprKind::MethodCall(ref path, _, _) = format_args.node {
if path.ident.name == sym!(to_string) {
return Some(format!("{}", snip));
}
} else if let ExprKind::Binary(..) = format_args.node {
return Some(format!("{}", snip));
}
return Some(format!("{}.to_string()", snip));
}
}
}

false
None
}

/// Checks if the expressions matches
/// ```rust,ignore
/// &match (&"arg",) {
/// (__arg0,) => [::std::fmt::ArgumentV1::new(__arg0,
/// ::std::fmt::Display::fmt)],
/// }
/// ```
/// and that the type of `__arg0` is `&str` or `String`,
/// then returns the span of first element of the matched tuple.
fn get_single_string_arg<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option<&'a Expr> {
fn on_new_v1<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<String> {
if_chain! {
if let ExprKind::AddrOf(_, ref expr) = expr.node;
if let ExprKind::Match(ref match_expr, ref arms, _) = expr.node;
if arms.len() == 1;
if arms[0].pats.len() == 1;
if let PatKind::Tuple(ref pat, None) = arms[0].pats[0].node;
if pat.len() == 1;
if let ExprKind::Array(ref exprs) = arms[0].body.node;
if exprs.len() == 1;
if let ExprKind::Call(_, ref args) = exprs[0].node;
if let ExprKind::Call(ref fun, ref args) = expr.node;
if args.len() == 2;
if let ExprKind::Path(ref qpath) = args[1].node;
if let Some(fun_def_id) = resolve_node(cx, qpath, args[1].hir_id).opt_def_id();
if match_def_path(cx, fun_def_id, &paths::DISPLAY_FMT_METHOD);
if let ExprKind::Path(ref qpath) = fun.node;
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
if match_def_path(cx, did, &paths::FMT_ARGUMENTS_NEW_V1);
// Argument 1 in `new_v1()`
if let ExprKind::AddrOf(_, ref arr) = args[0].node;
if let ExprKind::Array(ref pieces) = arr.node;
if pieces.len() == 1;
if let ExprKind::Lit(ref lit) = pieces[0].node;
if let LitKind::Str(ref s, _) = lit.node;
// Argument 2 in `new_v1()`
if let ExprKind::AddrOf(_, ref arg1) = args[1].node;
if let ExprKind::Match(ref matchee, ref arms, MatchSource::Normal) = arg1.node;
if arms.len() == 1;
if let ExprKind::Tup(ref tup) = matchee.node;
then {
let ty = walk_ptrs_ty(cx.tables.pat_ty(&pat[0]));
if ty.sty == ty::Str || match_type(cx, ty, &paths::STRING) {
if let ExprKind::Tup(ref values) = match_expr.node {
return Some(&values[0]);
}
// `format!("foo")` expansion contains `match () { () => [], }`
if tup.is_empty() {
return Some(format!("{:?}.to_string()", s.as_str()));
} else if s.as_str().is_empty() {
return on_argumentv1_new(cx, &tup[0], arms);
}
}
}
None
}

fn on_new_v1_fmt<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) -> Option<String> {
if_chain! {
if let ExprKind::Call(ref fun, ref args) = expr.node;
if args.len() == 3;
if let ExprKind::Path(ref qpath) = fun.node;
if let Some(did) = resolve_node(cx, qpath, fun.hir_id).opt_def_id();
if match_def_path(cx, did, &paths::FMT_ARGUMENTS_NEW_V1_FORMATTED);
if check_unformatted(&args[2]);
// Argument 1 in `new_v1_formatted()`
if let ExprKind::AddrOf(_, ref arr) = args[0].node;
if let ExprKind::Array(ref pieces) = arr.node;
if pieces.len() == 1;
if let ExprKind::Lit(ref lit) = pieces[0].node;
if let LitKind::Str(..) = lit.node;
// Argument 2 in `new_v1_formatted()`
if let ExprKind::AddrOf(_, ref arg1) = args[1].node;
if let ExprKind::Match(ref matchee, ref arms, MatchSource::Normal) = arg1.node;
if arms.len() == 1;
if let ExprKind::Tup(ref tup) = matchee.node;
then {
return on_argumentv1_new(cx, &tup[0], arms);
}
}
None
}

Expand All @@ -169,6 +174,7 @@ fn get_single_string_arg<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option
/// &[_ {
/// format: _ {
/// width: _::Implied,
/// precision: _::Implied,
/// ...
/// },
/// ...,
Expand All @@ -179,15 +185,17 @@ fn check_unformatted(expr: &Expr) -> bool {
if let ExprKind::AddrOf(_, ref expr) = expr.node;
if let ExprKind::Array(ref exprs) = expr.node;
if exprs.len() == 1;
// struct `core::fmt::rt::v1::Argument`
if let ExprKind::Struct(_, ref fields, _) = exprs[0].node;
if let Some(format_field) = fields.iter().find(|f| f.ident.name == sym!(format));
// struct `core::fmt::rt::v1::FormatSpec`
if let ExprKind::Struct(_, ref fields, _) = format_field.expr.node;
if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym!(width));
if let ExprKind::Path(ref width_qpath) = width_field.expr.node;
if last_path_segment(width_qpath).ident.name == sym!(Implied);
if let Some(precision_field) = fields.iter().find(|f| f.ident.name == sym!(precision));
if let ExprKind::Path(ref precision_path) = precision_field.expr.node;
if last_path_segment(precision_path).ident.name == sym!(Implied);
if let Some(width_field) = fields.iter().find(|f| f.ident.name == sym!(width));
if let ExprKind::Path(ref width_qpath) = width_field.expr.node;
if last_path_segment(width_qpath).ident.name == sym!(Implied);
then {
return true;
}
Expand Down
5 changes: 3 additions & 2 deletions clippy_lints/src/utils/paths.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ pub const DROP: [&str; 3] = ["core", "mem", "drop"];
pub const DROP_TRAIT: [&str; 4] = ["core", "ops", "drop", "Drop"];
pub const DURATION: [&str; 3] = ["core", "time", "Duration"];
pub const EARLY_CONTEXT: [&str; 4] = ["rustc", "lint", "context", "EarlyContext"];
pub const FMT_ARGUMENTS_NEWV1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
pub const FMT_ARGUMENTS_NEWV1FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
pub const FMT_ARGUMENTS_NEW_V1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"];
pub const FMT_ARGUMENTS_NEW_V1_FORMATTED: [&str; 4] = ["core", "fmt", "Arguments", "new_v1_formatted"];
pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"];
pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"];
pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"];
pub const HASH: [&str; 2] = ["hash", "Hash"];
Expand Down
2 changes: 1 addition & 1 deletion src/lintlist/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1879,7 +1879,7 @@ pub const ALL_LINTS: [Lint; 311] = [
Lint {
name: "unicode_not_nfc",
group: "pedantic",
desc: "using a unicode literal not in NFC normal form (see [unicode tr15](http://www.unicode.org/reports/tr15/) for further information)",
desc: "using a Unicode literal not in NFC normal form (see [Unicode tr15](http://www.unicode.org/reports/tr15/) for further information)",
deprecation: None,
module: "unicode",
},
Expand Down
5 changes: 5 additions & 0 deletions tests/ui/format.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ fn main() {
"foo".to_string();
"{}".to_string();
"{} abc {}".to_string();
"foo {}\n\" bar".to_string();

"foo".to_string();
format!("{:?}", "foo"); // Don't warn about `Debug`.
Expand Down Expand Up @@ -59,4 +60,8 @@ fn main() {
42.to_string();
let x = std::path::PathBuf::from("/bar/foo/qux");
x.display().to_string();

// False positive
let a = "foo".to_string();
let _ = Some(a + "bar");
}
8 changes: 8 additions & 0 deletions tests/ui/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ fn main() {
format!("foo");
format!("{{}}");
format!("{{}} abc {{}}");
format!(
r##"foo {{}}
" bar"##
);

format!("{}", "foo");
format!("{:?}", "foo"); // Don't warn about `Debug`.
Expand Down Expand Up @@ -59,4 +63,8 @@ fn main() {
format!("{}", 42.to_string());
let x = std::path::PathBuf::from("/bar/foo/qux");
format!("{}", x.display().to_string());

// False positive
let a = "foo".to_string();
let _ = Some(format!("{}", a + "bar"));
}
37 changes: 26 additions & 11 deletions tests/ui/format.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,52 +19,67 @@ LL | format!("{{}} abc {{}}");
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"{} abc {}".to_string();`

error: useless use of `format!`
--> $DIR/format.rs:17:5
--> $DIR/format.rs:16:5
|
LL | / format!(
LL | | r##"foo {{}}
LL | | " bar"##
LL | | );
| |______^ help: consider using .to_string(): `"foo {}/n/" bar".to_string();`

error: useless use of `format!`
--> $DIR/format.rs:21:5
|
LL | format!("{}", "foo");
| ^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `"foo".to_string();`

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

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

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

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

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

error: useless use of `format!`
--> $DIR/format.rs:59:5
--> $DIR/format.rs:63:5
|
LL | format!("{}", 42.to_string());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: `to_string()` is enough: `42.to_string();`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `42.to_string();`

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

error: useless use of `format!`
--> $DIR/format.rs:69:18
|
LL | let _ = Some(format!("{}", a + "bar"));
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using .to_string(): `a + "bar"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this should be not linted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should. I forgot to removing the misleading comments.


error: aborting due to 11 previous errors
error: aborting due to 13 previous errors