diff --git a/clippy_lints/src/format.rs b/clippy_lints/src/format.rs index 6e8d4585161e..1df6b9294d7b 100644 --- a/clippy_lints/src/format.rs +++ b/clippy_lints/src/format.rs @@ -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; @@ -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, "").to_string()) - } else { - ("consider using .to_string()", - format!("{}.to_string()", snippet(cx, format_arg.span, ""))) - } - }; + 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, "").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); } } } @@ -111,56 +70,102 @@ fn span_useless_format(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 { 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, ""); + 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 { 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 { + 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 } @@ -169,6 +174,7 @@ fn get_single_string_arg<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option /// &[_ { /// format: _ { /// width: _::Implied, +/// precision: _::Implied, /// ... /// }, /// ..., @@ -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; } diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 5c688a601cd8..55e1387fe99e 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -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"]; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 9abe4558bb91..b7a6e486f0bf 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -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", }, diff --git a/tests/ui/format.fixed b/tests/ui/format.fixed index e4217411f053..6e100230a3ad 100644 --- a/tests/ui/format.fixed +++ b/tests/ui/format.fixed @@ -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`. @@ -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"); } diff --git a/tests/ui/format.rs b/tests/ui/format.rs index 61ef3e7243d7..1fae6603ac09 100644 --- a/tests/ui/format.rs +++ b/tests/ui/format.rs @@ -13,6 +13,10 @@ fn main() { format!("foo"); format!("{{}}"); format!("{{}} abc {{}}"); + format!( + r##"foo {{}} +" bar"## + ); format!("{}", "foo"); format!("{:?}", "foo"); // Don't warn about `Debug`. @@ -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")); } diff --git a/tests/ui/format.stderr b/tests/ui/format.stderr index ec1253589f62..9736f34b03b4 100644 --- a/tests/ui/format.stderr +++ b/tests/ui/format.stderr @@ -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"` -error: aborting due to 11 previous errors +error: aborting due to 13 previous errors