From ba3be834881a5a62c00cb49ac09b2d1b9f35fe5f Mon Sep 17 00:00:00 2001 From: mcarton Date: Sat, 20 Feb 2016 21:15:05 +0100 Subject: [PATCH] Lint about `format!("{}", foo)` --- src/format.rs | 86 +++++++++++++++++++++++++++++++++--- src/lib.rs | 1 + src/utils/mod.rs | 2 + tests/compile-fail/format.rs | 4 ++ 4 files changed, 86 insertions(+), 7 deletions(-) diff --git a/src/format.rs b/src/format.rs index ad72b38e1..704b4e8e8 100644 --- a/src/format.rs +++ b/src/format.rs @@ -1,11 +1,14 @@ +use rustc::front::map::Node::NodeItem; use rustc::lint::*; use rustc_front::hir::*; -use utils::{is_expn_of, span_lint}; +use syntax::ast::LitKind; +use utils::{DISPLAY_FMT_METHOD_PATH, FMT_ARGUMENTS_NEWV1_PATH}; +use utils::{is_expn_of, match_path, span_lint}; /// **What it does:** This lints about use of `format!("string literal with no argument")`. /// /// **Why is this bad?** There is no point of doing that. If you want a `String` you can use -/// `to_owned` on the string literal. The even worst `&format!("foo")` is often encountered in the +/// `to_owned` on the string literal. The even worse `&format!("foo")` is often encountered in the /// wild. /// /// **Known problems:** None. @@ -28,13 +31,82 @@ impl LintPass for FormatMacLint { impl LateLintPass for FormatMacLint { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { - // `format!("foo")` expansion contains `match () { () => [], }` - if let ExprMatch(ref matchee, _, _) = expr.node { - if let ExprTup(ref tup) = matchee.node { - if tup.is_empty() && is_expn_of(cx, expr.span, "format").is_some() { - span_lint(cx, USELESS_FORMAT, expr.span, &"useless use of `format!`"); + if let Some(span) = is_expn_of(cx, expr.span, "format") { + match expr.node { + // `format!("{}", foo)` expansion + ExprCall(ref fun, ref args) => { + if_let_chain!{[ + let ExprPath(_, ref path) = fun.node, + args.len() == 2, + match_path(path, &FMT_ARGUMENTS_NEWV1_PATH), + // ensure the format string is `"{..}"` with only one argument and no text + check_static_str(cx, &args[0]), + // ensure the format argument is `{}` ie. Display with no fancy option + check_arg_is_display(&args[1]) + ], { + span_lint(cx, USELESS_FORMAT, span, &"useless use of `format!`"); + }} } + // `format!("foo")` expansion contains `match () { () => [], }` + ExprMatch(ref matchee, _, _) => { + if let ExprTup(ref tup) = matchee.node { + if tup.is_empty() { + span_lint(cx, USELESS_FORMAT, span, &"useless use of `format!`"); + } + } + } + _ => (), } } } } + +/// Checks if the expressions matches +/// ``` +/// { static __STATIC_FMTSTR: &[""] = _; __STATIC_FMTSTR } +/// ``` +fn check_static_str(cx: &LateContext, expr: &Expr) -> bool { + if_let_chain! {[ + let ExprBlock(ref block) = expr.node, + block.stmts.len() == 1, + let StmtDecl(ref decl, _) = block.stmts[0].node, + let DeclItem(ref decl) = decl.node, + let Some(NodeItem(decl)) = cx.tcx.map.find(decl.id), + decl.name.as_str() == "__STATIC_FMTSTR", + let ItemStatic(_, _, ref expr) = decl.node, + let ExprAddrOf(_, ref expr) = expr.node, // &[""] + let ExprVec(ref expr) = expr.node, + expr.len() == 1, + let ExprLit(ref lit) = expr[0].node, + let LitKind::Str(ref lit, _) = lit.node, + lit.is_empty() + ], { + return true; + }} + + false +} + +/// Checks if the expressions matches +/// ``` +/// &match (&42,) { +/// (__arg0,) => [::std::fmt::ArgumentV1::new(__arg0, ::std::fmt::Display::fmt)], +/// }) +/// ``` +fn check_arg_is_display(expr: &Expr) -> bool { + if_let_chain! {[ + let ExprAddrOf(_, ref expr) = expr.node, + let ExprMatch(_, ref arms, _) = expr.node, + arms.len() == 1, + let ExprVec(ref exprs) = arms[0].body.node, + exprs.len() == 1, + let ExprCall(_, ref args) = exprs[0].node, + args.len() == 2, + let ExprPath(None, ref path) = args[1].node, + match_path(path, &DISPLAY_FMT_METHOD_PATH) + ], { + return true; + }} + + false +} diff --git a/src/lib.rs b/src/lib.rs index b690a5cf4..8348eb098 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,6 +56,7 @@ pub mod enum_variants; pub mod eq_op; pub mod escape; pub mod eta_reduction; +pub mod format; pub mod identity_op; pub mod items_after_statements; pub mod len_zero; diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 3a7c6c90d..68137fbbf 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -28,7 +28,9 @@ pub const CLONE_TRAIT_PATH: [&'static str; 2] = ["clone", "Clone"]; pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"]; pub const DEBUG_FMT_METHOD_PATH: [&'static str; 4] = ["std", "fmt", "Debug", "fmt"]; pub const DEFAULT_TRAIT_PATH: [&'static str; 3] = ["core", "default", "Default"]; +pub const DISPLAY_FMT_METHOD_PATH: [&'static str; 4] = ["std", "fmt", "Display", "fmt"]; pub const DROP_PATH: [&'static str; 3] = ["core", "mem", "drop"]; +pub const FMT_ARGUMENTS_NEWV1_PATH: [&'static str; 4] = ["std", "fmt", "Arguments", "new_v1"]; pub const FMT_ARGUMENTV1_NEW_PATH: [&'static str; 4] = ["std", "fmt", "ArgumentV1", "new"]; pub const HASHMAP_ENTRY_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "Entry"]; pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"]; diff --git a/tests/compile-fail/format.rs b/tests/compile-fail/format.rs index 6cdceefd0..0219771e0 100755 --- a/tests/compile-fail/format.rs +++ b/tests/compile-fail/format.rs @@ -4,7 +4,11 @@ fn main() { format!("foo"); //~ERROR useless use of `format!` + format!("{}", 42); //~ERROR useless use of `format!` + format!("{:?}", 42); // we only want to warn about `{}` + format!("{:+}", 42); // we only want to warn about `{}` format!("foo {}", 42); + format!("{} bar", 42); println!("foo"); println!("foo {}", 42);