Merge pull request #692 from mcarton/format

Lint about `format!("string lit")` with no argument

README.md

@@ -8,7 +8,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 122 lints included in this crate:
+There are 123 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -126,6 +126,7 @@ name
 [unused_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#unused_lifetimes)                           | warn    | unused lifetimes in function definitions
 [use_debug](https://github.com/Manishearth/rust-clippy/wiki#use_debug)                                         | allow   | use `Debug`-based formatting
 [used_underscore_binding](https://github.com/Manishearth/rust-clippy/wiki#used_underscore_binding)             | warn    | using a binding which is prefixed with an underscore
+[useless_format](https://github.com/Manishearth/rust-clippy/wiki#useless_format)                               | warn    | useless use of `format!`
 [useless_transmute](https://github.com/Manishearth/rust-clippy/wiki#useless_transmute)                         | warn    | transmutes that have the same to and from types
 [useless_vec](https://github.com/Manishearth/rust-clippy/wiki#useless_vec)                                     | warn    | useless `vec!`
 [while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop)                               | warn    | `loop { if let { ... } else break }` can be written as a `while let` loop

src/collapsible_if.rs

@@ -14,6 +14,7 @@
 
 use rustc::lint::*;
 use rustc_front::hir::*;
+use std::borrow::Cow;
 use syntax::codemap::Spanned;
 
 use utils::{in_macro, snippet, snippet_block, span_lint_and_then};
@@ -95,11 +96,11 @@ fn requires_brackets(e: &Expr) -> bool {
     }
 }
 
-fn check_to_string(cx: &LateContext, e: &Expr) -> String {
+fn check_to_string(cx: &LateContext, e: &Expr) -> Cow<'static, str> {
     if requires_brackets(e) {
-        format!("({})", snippet(cx, e.span, ".."))
+        format!("({})", snippet(cx, e.span, "..")).into()
     } else {
-        format!("{}", snippet(cx, e.span, ".."))
+        snippet(cx, e.span, "..")
     }
 }
 

src/format.rs

@@ -0,0 +1,112 @@
+use rustc::front::map::Node::NodeItem;
+use rustc::lint::*;
+use rustc_front::hir::*;
+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 worse `&format!("foo")` is often encountered in the
+/// wild.
+///
+/// **Known problems:** None.
+///
+/// **Example:** `format!("foo")`
+declare_lint! {
+    pub USELESS_FORMAT,
+    Warn,
+    "useless use of `format!`"
+}
+
+#[derive(Copy, Clone, Debug)]
+pub struct FormatMacLint;
+
+impl LintPass for FormatMacLint {
+    fn get_lints(&self) -> LintArray {
+        lint_array![USELESS_FORMAT]
+    }
+}
+
+impl LateLintPass for FormatMacLint {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        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
+}

src/lib.rs

@@ -35,59 +35,63 @@ extern crate rustc_plugin;
 
 use rustc_plugin::Registry;
 
+pub mod consts;
 #[macro_use]
 pub mod utils;
-pub mod copies;
+
-pub mod consts;
+// begin lints modules, do not remove this comment, it’s used in `update_lints`
-pub mod types;
-pub mod misc;
-pub mod enum_glob_use;
-pub mod eq_op;
-pub mod bit_mask;
-pub mod ptr_arg;
-pub mod needless_bool;
 pub mod approx_const;
-pub mod eta_reduction;
+pub mod array_indexing;
+pub mod attrs;
+pub mod bit_mask;
+pub mod block_in_if_condition;
+pub mod collapsible_if;
+pub mod copies;
+pub mod cyclomatic_complexity;
+pub mod derive;
+pub mod drop_ref;
+pub mod entry;
+pub mod enum_glob_use;
 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 minmax;
-pub mod mut_mut;
-pub mod mut_reference;
 pub mod len_zero;
-pub mod attrs;
-pub mod collapsible_if;
-pub mod block_in_if_condition;
-pub mod unicode;
-pub mod shadow;
-pub mod strings;
-pub mod methods;
-pub mod returns;
 pub mod lifetimes;
 pub mod loops;
-pub mod ranges;
 pub mod map_clone;
 pub mod matches;
-pub mod precedence;
+pub mod methods;
+pub mod minmax;
+pub mod misc;
+pub mod misc_early;
+pub mod mut_mut;
+pub mod mut_reference;
 pub mod mutex_atomic;
-pub mod zero_div_zero;
+pub mod needless_bool;
-pub mod open_options;
 pub mod needless_features;
 pub mod needless_update;
 pub mod no_effect;
+pub mod open_options;
+pub mod panic;
+pub mod precedence;
+pub mod print;
+pub mod ptr_arg;
+pub mod ranges;
+pub mod regex;
+pub mod returns;
+pub mod shadow;
+pub mod strings;
 pub mod temporary_assignment;
 pub mod transmute;
-pub mod cyclomatic_complexity;
+pub mod types;
-pub mod escape;
+pub mod unicode;
-pub mod entry;
-pub mod misc_early;
-pub mod array_indexing;
-pub mod panic;
-pub mod derive;
-pub mod print;
 pub mod vec;
-pub mod drop_ref;
+pub mod zero_div_zero;
-pub mod regex;
+// end lints modules, do not remove this comment, it’s used in `update_lints`
 
 mod reexport {
     pub use syntax::ast::{Name, NodeId};
@@ -160,6 +164,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box types::AbsurdExtremeComparisons);
     reg.register_late_lint_pass(box regex::RegexPass::default());
     reg.register_late_lint_pass(box copies::CopyAndPaste);
+    reg.register_late_lint_pass(box format::FormatMacLint);
 
     reg.register_lint_group("clippy_pedantic", vec![
         enum_glob_use::ENUM_GLOB_USE,
@@ -206,6 +211,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         eq_op::EQ_OP,
         escape::BOXED_LOCAL,
         eta_reduction::REDUNDANT_CLOSURE,
+        format::USELESS_FORMAT,
         identity_op::IDENTITY_OP,
         items_after_statements::ITEMS_AFTER_STATEMENTS,
         len_zero::LEN_WITHOUT_IS_EMPTY,

src/loops.rs

@@ -245,13 +245,13 @@ impl LateLintPass for LoopsPass {
                     let mut other_stuff = block.stmts
                                                .iter()
                                                .skip(1)
-                                               .map(|stmt| format!("{}", snippet(cx, stmt.span, "..")))
+                                               .map(|stmt| snippet(cx, stmt.span, ".."))
-                                               .collect::<Vec<String>>();
+                                               .collect::<Vec<Cow<_>>>();
                     if inner_stmt_expr.is_some() {
                         // if we have a statement which has a match,
                         if let Some(ref expr) = block.expr {
                             // then collect the expression (without semicolon) below it
-                            other_stuff.push(format!("{}", snippet(cx, expr.span, "..")));
+                            other_stuff.push(snippet(cx, expr.span, ".."));
                         }
                     }
 
@@ -317,8 +317,8 @@ impl LateLintPass for LoopsPass {
                     span_lint(cx,
                               UNUSED_COLLECT,
                               expr.span,
-                              &format!("you are collect()ing an iterator and throwing away the result. Consider \
+                              "you are collect()ing an iterator and throwing away the result. \
-                                        using an explicit for loop to exhaust the iterator"));
+                               Consider using an explicit for loop to exhaust the iterator");
                 }
             }
         }

src/methods.rs

@@ -530,10 +530,10 @@ fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P<Expr>])
             return;
         }
 
-        let sugg = match (fn_has_arguments, !or_has_args) {
+        let sugg: Cow<_> = match (fn_has_arguments, !or_has_args) {
-            (true, _) => format!("|_| {}", snippet(cx, arg.span, "..")),
+            (true, _) => format!("|_| {}", snippet(cx, arg.span, "..")).into(),
-            (false, false) => format!("|| {}", snippet(cx, arg.span, "..")),
+            (false, false) => format!("|| {}", snippet(cx, arg.span, "..")).into(),
-            (false, true) => format!("{}", snippet(cx, fun.span, "..")),
+            (false, true) => snippet(cx, fun.span, ".."),
         };
 
         span_lint(cx, OR_FUN_CALL, span, &format!("use of `{}` followed by a function call", name))
@@ -589,7 +589,7 @@ fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) {
         span_lint(cx,
                   EXTEND_FROM_SLICE,
                   expr.span,
-                  &format!("use of `extend` to extend a Vec by a slice"))
+                  "use of `extend` to extend a Vec by a slice")
             .span_suggestion(expr.span,
                              "try this",
                              format!("{}.extend_from_slice({}{})",

src/misc_early.rs

@@ -45,10 +45,7 @@ impl EarlyLintPass for MiscEarly {
     fn check_pat(&mut self, cx: &EarlyContext, pat: &Pat) {
         if let PatKind::Struct(ref npat, ref pfields, _) = pat.node {
             let mut wilds = 0;
-            let type_name = match npat.segments.last() {
+            let type_name = npat.segments.last().expect("A path must have at least one segment").identifier.name;
-                Some(elem) => format!("{}", elem.identifier.name),
-                None => String::new(),
-            };
 
             for field in pfields {
                 if field.node.pat.node == PatKind::Wild {

src/mut_reference.rs

@@ -39,13 +39,13 @@ impl LateLintPass for UnnecessaryMutPassed {
                                                            If this happened, the compiler would have \
                                                            aborted the compilation long ago");
                 if let ExprPath(_, ref path) = fn_expr.node {
-                    check_arguments(cx, &arguments, function_type, &format!("{}", path));
+                    check_arguments(cx, &arguments, function_type, &path.to_string());
                 }
             }
             ExprMethodCall(ref name, _, ref arguments) => {
                 let method_call = MethodCall::expr(e.id);
                 let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen.");
-                check_arguments(cx, &arguments, method_type.ty, &format!("{}", name.node.as_str()))
+                check_arguments(cx, &arguments, method_type.ty, &name.node.as_str())
             }
             _ => {}
         }

src/regex.rs

@@ -105,7 +105,7 @@ impl LateLintPass for RegexPass {
                         Ok(r) => {
                             if let Some(repl) = is_trivial_regex(&r) {
                                 span_help_and_lint(cx, TRIVIAL_REGEX, args[0].span,
-                                                   &"trivial regex",
+                                                   "trivial regex",
                                                    &format!("consider using {}", repl));
                             }
                         }
@@ -123,7 +123,7 @@ impl LateLintPass for RegexPass {
                     Ok(r) => {
                         if let Some(repl) = is_trivial_regex(&r) {
                             span_help_and_lint(cx, TRIVIAL_REGEX, args[0].span,
-                                               &"trivial regex",
+                                               "trivial regex",
                                                &format!("consider using {}", repl));
                         }
                     }

src/types.rs

@@ -491,12 +491,12 @@ fn check_type(cx: &LateContext, ty: &Ty) {
         visitor.visit_ty(ty);
         visitor.score
     };
-    // println!("{:?} --> {}", ty, score);
+
     if score > 250 {
         span_lint(cx,
                   TYPE_COMPLEXITY,
                   ty.span,
-                  &format!("very complex type used. Consider factoring parts into `type` definitions"));
+                  "very complex type used. Consider factoring parts into `type` definitions");
     }
 }
 

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

tests/compile-fail/format.rs

@@ -0,0 +1,15 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![deny(useless_format)]
+
+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);
+}

util/update_lints.py

@@ -60,6 +60,13 @@ def gen_group(lints, levels=None):
         yield '        %s::%s,\n' % (module, name.upper())
 
 
+def gen_mods(lints):
+    """Declare modules"""
+
+    for module in sorted(set(lint[0] for lint in lints)):
+        yield 'pub mod %s;\n' % module
+
+
 def replace_region(fn, region_start, region_end, callback,
                    replace_start=True, write_back=True):
     """Replace a region in a file delimited by two lines matching regexes.
@@ -128,6 +135,12 @@ def main(print_only=False, check=False):
         lambda: ['There are %d lints included in this crate:\n' % len(lints)],
         write_back=not check)
 
+    # update the `pub mod` list
+    changed |= replace_region(
+        'src/lib.rs', r'begin lints modules', r'end lints modules',
+        lambda: gen_mods(lints),
+        replace_start=False, write_back=not check)
+
     # same for "clippy" lint collection
     changed |= replace_region(
         'src/lib.rs', r'reg.register_lint_group\("clippy"', r'\]\);',