Lint usage of Debug-based formatting

Author: mcarton

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 117 lints included in this crate:
+There are 118 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -118,6 +118,7 @@ name
 [unstable_as_slice](https://github.com/Manishearth/rust-clippy/wiki#unstable_as_slice)                         | warn    | as_slice is not stable and can be replaced by & v[..]see https://github.com/rust-lang/rust/issues/27729
 [unused_collect](https://github.com/Manishearth/rust-clippy/wiki#unused_collect)                               | warn    | `collect()`ing an iterator without using the result; this is usually better written as a for loop
 [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_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!`

src/lib.rs

@@ -169,6 +169,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         mut_mut::MUT_MUT,
         mutex_atomic::MUTEX_INTEGER,
         print::PRINT_STDOUT,
+        print::USE_DEBUG,
         shadow::SHADOW_REUSE,
         shadow::SHADOW_SAME,
         shadow::SHADOW_UNRELATED,

src/print.rs

@@ -1,6 +1,8 @@
 use rustc::lint::*;
 use rustc_front::hir::*;
-use utils::{IO_PRINT_PATH, is_expn_of, match_path, span_lint};
+use rustc::front::map::Node::{NodeItem, NodeImplItem};
+use utils::{FMT_ARGUMENTV1_NEW_PATH, DEBUG_FMT_METHOD_PATH, IO_PRINT_PATH};
+use utils::{is_expn_of, match_path, span_lint};
 
 /// **What it does:** This lint warns whenever you print on *stdout*. The purpose of this lint is to catch debugging remnants.
 ///
@@ -16,21 +18,36 @@ declare_lint! {
     "printing on stdout"
 }
 
+/// **What it does:** This lint warns whenever you use `Debug` formatting. The purpose of this lint is to catch debugging remnants.
+///
+/// **Why is this bad?** The purpose of the `Debug` trait is to facilitate debugging Rust code. It
+/// should not be used in in user-facing output.
+///
+/// **Example:** `println!("{:?}", foo);`
+declare_lint! {
+    pub USE_DEBUG,
+    Allow,
+    "use `Debug`-based formatting"
+}
+
 #[derive(Copy, Clone, Debug)]
 pub struct PrintLint;
 
 impl LintPass for PrintLint {
     fn get_lints(&self) -> LintArray {
-        lint_array!(PRINT_STDOUT)
+        lint_array!(PRINT_STDOUT, USE_DEBUG)
     }
 }
 
 impl LateLintPass for PrintLint {
     fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
-        if let ExprCall(ref fun, _) = expr.node {
+        if let ExprCall(ref fun, ref args) = expr.node {
             if let ExprPath(_, ref path) = fun.node {
+                // Search for `std::io::_print(..)` which is unique in a
+                // `print!` expansion.
                 if match_path(path, &IO_PRINT_PATH) {
                     if let Some(span) = is_expn_of(cx, expr.span, "print") {
+                        // `println!` uses `print!`.
                         let (span, name) = match is_expn_of(cx, span, "println") {
                             Some(span) => (span, "println"),
                             None => (span, "print"),
@@ -39,7 +56,32 @@ impl LateLintPass for PrintLint {
                         span_lint(cx, PRINT_STDOUT, span, &format!("use of `{}!`", name));
                     }
                 }
+                // Search for something like
+                // `::std::fmt::ArgumentV1::new(__arg0, ::std::fmt::Debug::fmt)`
+                else if args.len() == 2 && match_path(path, &FMT_ARGUMENTV1_NEW_PATH) {
+                    if let ExprPath(None, ref path) = args[1].node {
+                        if match_path(path, &DEBUG_FMT_METHOD_PATH) &&
+                            !is_in_debug_impl(cx, expr) &&
+                            is_expn_of(cx, expr.span, "panic").is_none() {
+                            span_lint(cx, USE_DEBUG, args[0].span, "use of `Debug`-based formatting");
+                        }
+                    }
+                }
             }
         }
     }
 }
+
+fn is_in_debug_impl(cx: &LateContext, expr: &Expr) -> bool {
+    let map = &cx.tcx.map;
+
+    if let Some(NodeImplItem(item)) = map.find(map.get_parent(expr.id)) { // `fmt` method
+        if let Some(NodeItem(item)) = map.find(map.get_parent(item.id)) { // `Debug` impl
+            if let ItemImpl(_, _, _, Some(ref tr), _, _) = item.node {
+                return match_path(&tr.path, &["Debug"]);
+            }
+        }
+    }
+
+    false
+}

src/utils.rs

@@ -26,8 +26,10 @@ pub const BTREEMAP_PATH: [&'static str; 4] = ["collections", "btree", "map", "BT
 pub const CLONE_PATH: [&'static str; 3] = ["clone", "Clone", "clone"];
 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 DROP_PATH: [&'static str; 3] = ["core", "mem", "drop"];
+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"];
 pub const HASH_PATH: [&'static str; 2] = ["hash", "Hash"];

tests/compile-fail/print.rs

@@ -1,11 +1,41 @@
 #![feature(plugin)]
 #![plugin(clippy)]
+#![deny(print_stdout, use_debug)]
 
-#[deny(print_stdout)]
+use std::fmt::{Debug, Display, Formatter, Result};
+
+#[allow(dead_code)]
+struct Foo;
+
+impl Display for Foo {
+    fn fmt(&self, f: &mut Formatter) -> Result {
+        write!(f, "{:?}", 43.1415)
+        //~^ ERROR use of `Debug`-based formatting
+    }
+}
+
+impl Debug for Foo {
+    fn fmt(&self, f: &mut Formatter) -> Result {
+        // ok, we can use `Debug` formatting in `Debug` implementations
+        write!(f, "{:?}", 42.718)
+    }
+}
 
 fn main() {
     println!("Hello"); //~ERROR use of `println!`
     print!("Hello"); //~ERROR use of `print!`
 
+    print!("Hello {}", "World"); //~ERROR use of `print!`
+
+    print!("Hello {:?}", "World");
+    //~^ ERROR use of `print!`
+    //~| ERROR use of `Debug`-based formatting
+
+    print!("Hello {:#?}", "#orld");
+    //~^ ERROR use of `print!`
+    //~| ERROR use of `Debug`-based formatting
+
+    assert_eq!(42, 1337);
+
     vec![1, 2];
 }