Merge pull request rust-lang#637 from mcarton/debug

llogiq · llogiq · commit 9ee462625bae · 2016-02-12T12:18:15.000+01:00
Lint usage of `Debug`-based formatting
diff --git a/README.md b/README.md
@@ -119,6 +119,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!`
diff --git a/src/consts.rs b/src/consts.rs
@@ -6,12 +6,10 @@ use rustc::middle::def::PathResolution;
 use rustc::middle::def::Def;
 use rustc_front::hir::*;
 use syntax::ptr::P;
-use std::char;
 use std::cmp::PartialOrd;
 use std::cmp::Ordering::{self, Greater, Less, Equal};
 use std::rc::Rc;
 use std::ops::Deref;
-use std::fmt;
 
 use syntax::ast::Lit_;
 use syntax::ast::LitIntType;
@@ -173,90 +171,6 @@ impl PartialOrd for Constant {
     }
 }
 
-fn format_byte(fmt: &mut fmt::Formatter, b: u8) -> fmt::Result {
-    if b == b'\\' {
-        write!(fmt, "\\\\")
-    } else if 0x20 <= b && b <= 0x7e {
-        write!(fmt, "{}", char::from_u32(b as u32).expect("all u8 are valid char"))
-    } else if b == 0x0a {
-        write!(fmt, "\\n")
-    } else if b == 0x0d {
-        write!(fmt, "\\r")
-    } else {
-        write!(fmt, "\\x{:02x}", b)
-    }
-}
-
-impl fmt::Display for Constant {
-    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
-            Constant::Str(ref s, _) => write!(fmt, "{:?}", s),
-            Constant::Byte(ref b) => {
-                write!(fmt, "b'")
-                    .and_then(|_| format_byte(fmt, *b))
-                    .and_then(|_| write!(fmt, "'"))
-            }
-            Constant::Binary(ref bs) => {
-                try!(write!(fmt, "b\""));
-                for b in bs.iter() {
-                    try!(format_byte(fmt, *b));
-                }
-                write!(fmt, "\"")
-            }
-            Constant::Char(ref c) => write!(fmt, "'{}'", c),
-            Constant::Int(ref i, ref ity) => {
-                let (sign, suffix) = match *ity {
-                    LitIntType::SignedIntLit(ref sity, ref sign) => {
-                        (if let Sign::Minus = *sign {
-                            "-"
-                        } else {
-                            ""
-                        },
-                         sity.ty_to_string())
-                    }
-                    LitIntType::UnsignedIntLit(ref uity) => ("", uity.ty_to_string()),
-                    LitIntType::UnsuffixedIntLit(ref sign) => {
-                        (if let Sign::Minus = *sign {
-                            "-"
-                        } else {
-                            ""
-                        },
-                         "".into())
-                    }
-                };
-                write!(fmt, "{}{}{}", sign, i, suffix)
-            }
-            Constant::Float(ref s, ref fw) => {
-                let suffix = match *fw {
-                    FloatWidth::Fw32 => "f32",
-                    FloatWidth::Fw64 => "f64",
-                    FloatWidth::FwAny => "",
-                };
-                write!(fmt, "{}{}", s, suffix)
-            }
-            Constant::Bool(ref b) => write!(fmt, "{}", b),
-            Constant::Repeat(ref c, ref n) => write!(fmt, "[{}; {}]", c, n),
-            Constant::Vec(ref v) => {
-                write!(fmt,
-                       "[{}]",
-                       v.iter()
-                        .map(|i| format!("{}", i))
-                        .collect::<Vec<_>>()
-                        .join(", "))
-            }
-            Constant::Tuple(ref t) => {
-                write!(fmt,
-                       "({})",
-                       t.iter()
-                        .map(|i| format!("{}", i))
-                        .collect::<Vec<_>>()
-                        .join(", "))
-            }
-        }
-    }
-}
-
-
 fn lit_to_constant(lit: &Lit_) -> Constant {
     match *lit {
         Lit_::LitStr(ref is, style) => Constant::Str(is.to_string(), style),
diff --git a/src/lib.rs b/src/lib.rs
@@ -170,6 +170,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,
diff --git a/src/loops.rs b/src/loops.rs
@@ -1,11 +1,12 @@
+use reexport::*;
+use rustc::front::map::Node::NodeBlock;
 use rustc::lint::*;
+use rustc::middle::const_eval::EvalHint::ExprTypeChecked;
+use rustc::middle::const_eval::{ConstVal, eval_const_expr_partial};
+use rustc::middle::def::Def;
+use rustc::middle::ty;
 use rustc_front::hir::*;
-use reexport::*;
 use rustc_front::intravisit::{Visitor, walk_expr, walk_block, walk_decl};
-use rustc::middle::ty;
-use rustc::middle::def::Def;
-use consts::{constant_simple, Constant};
-use rustc::front::map::Node::NodeBlock;
 use std::borrow::Cow;
 use std::collections::{HashSet, HashMap};
 
@@ -421,22 +422,36 @@ fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) {
     // if this for loop is iterating over a two-sided range...
     if let ExprRange(Some(ref start_expr), Some(ref stop_expr)) = arg.node {
         // ...and both sides are compile-time constant integers...
-        if let Some(start_idx @ Constant::Int(..)) = constant_simple(start_expr) {
-            if let Some(stop_idx @ Constant::Int(..)) = constant_simple(stop_expr) {
+        if let Ok(start_idx) = eval_const_expr_partial(&cx.tcx, start_expr, ExprTypeChecked, None) {
+            if let Ok(stop_idx) = eval_const_expr_partial(&cx.tcx, stop_expr, ExprTypeChecked, None) {
                 // ...and the start index is greater than the stop index,
                 // this loop will never run. This is often confusing for developers
                 // who think that this will iterate from the larger value to the
                 // smaller value.
-                if start_idx > stop_idx {
-                    span_help_and_lint(cx,
+                let (sup, eq) = match (start_idx, stop_idx) {
+                    (ConstVal::Int(start_idx), ConstVal::Int(stop_idx)) => (start_idx > stop_idx, start_idx == stop_idx),
+                    (ConstVal::Uint(start_idx), ConstVal::Uint(stop_idx)) => (start_idx > stop_idx, start_idx == stop_idx),
+                    _ => (false, false),
+                };
+
+                if sup {
+                    let start_snippet = snippet(cx, start_expr.span, "_");
+                    let stop_snippet = snippet(cx, stop_expr.span, "_");
+
+                    span_lint_and_then(cx,
                                        REVERSE_RANGE_LOOP,
                                        expr.span,
                                        "this range is empty so this for loop will never run",
-                                       &format!("Consider using `({}..{}).rev()` if you are attempting to iterate \
-                                                 over this range in reverse",
-                                                stop_idx,
-                                                start_idx));
-                } else if start_idx == stop_idx {
+                                       |db| {
+                                           db.span_suggestion(expr.span,
+                                                              "consider using the following if \
+                                                               you are attempting to iterate \
+                                                               over this range in reverse",
+                                                               format!("({}..{}).rev()` ",
+                                                                       stop_snippet,
+                                                                       start_snippet));
+                                       });
+                } else if eq {
                     // if they are equal, it's also problematic - this loop
                     // will never run.
                     span_lint(cx,
diff --git a/src/misc.rs b/src/misc.rs
@@ -11,7 +11,7 @@ use rustc::middle::const_eval::eval_const_expr_partial;
 use rustc::middle::const_eval::EvalHint::ExprTypeChecked;
 
 use utils::{get_item_name, match_path, snippet, get_parent_expr, span_lint};
-use utils::{span_help_and_lint, walk_ptrs_ty, is_integer_literal, implements_trait};
+use utils::{span_lint_and_then, walk_ptrs_ty, is_integer_literal, implements_trait};
 
 /// **What it does:** This lint checks for function arguments and let bindings denoted as `ref`.
 ///
@@ -62,16 +62,22 @@ impl LateLintPass for TopLevelRefPass {
             let Some(ref init) = l.init
             ], {
                 let tyopt = if let Some(ref ty) = l.ty {
-                    format!(": {:?} ", ty)
+                    format!(": {}", snippet(cx, ty.span, "_"))
                 } else {
                     "".to_owned()
                 };
-                span_help_and_lint(cx,
+                span_lint_and_then(cx,
                     TOPLEVEL_REF_ARG,
                     l.pat.span,
                     "`ref` on an entire `let` pattern is discouraged, take a reference with & instead",
-                    &format!("try `let {} {}= &{};`", snippet(cx, i.span, "_"),
-                             tyopt, snippet(cx, init.span, "_"))
+                    |db| {
+                        db.span_suggestion(s.span,
+                                           "try",
+                                           format!("let {}{} = &{};",
+                                                   snippet(cx, i.span, "_"),
+                                                   tyopt,
+                                                   snippet(cx, init.span, "_")));
+                    }
                 );
             }
         };
diff --git a/src/print.rs b/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
+}
diff --git a/src/utils.rs b/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"];
diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs
@@ -65,7 +65,7 @@ fn for_loop_over_option_and_result() {
         break;
     }
 
-    // while let false positive for Option
+    // while let false positive for Result
     while let Ok(x) = result {
         println!("{}", x);
         break;
@@ -85,8 +85,10 @@ impl Unrelated {
 
 #[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)]
 #[deny(unused_collect)]
-#[allow(linkedlist,shadow_unrelated,unnecessary_mut_passed, cyclomatic_complexity)]
+#[allow(linkedlist, shadow_unrelated, unnecessary_mut_passed, cyclomatic_complexity)]
 fn main() {
+    const MAX_LEN: usize = 42;
+
     let mut vec = vec![1, 2, 3, 4];
     let vec2 = vec![1, 2, 3, 4];
     for i in 0..vec.len() {
@@ -111,6 +113,11 @@ fn main() {
         println!("{}", vec[i]);
     }
 
+    for i in 0..MAX_LEN {
+        //~^ ERROR `i` is only used to index `vec`. Consider using `for item in vec.iter().take(MAX_LEN)`
+        println!("{}", vec[i]);
+    }
+
     for i in 5..10 {
         //~^ ERROR `i` is only used to index `vec`. Consider using `for item in vec.iter().take(10).skip(5)`
         println!("{}", vec[i]);
@@ -126,7 +133,16 @@ fn main() {
         println!("{} {}", vec[i], i);
     }
 
-    for i in 10..0 { //~ERROR this range is empty so this for loop will never run
+    for i in 10..0 {
+        //~^ERROR this range is empty so this for loop will never run
+        //~|HELP consider
+        //~|SUGGESTION (0..10).rev()
+        println!("{}", i);
+    }
+
+    for i in MAX_LEN..0 { //~ERROR this range is empty so this for loop will never run
+        //~|HELP consider
+        //~|SUGGESTION (0..MAX_LEN).rev()
         println!("{}", i);
     }
 
diff --git a/tests/compile-fail/print.rs b/tests/compile-fail/print.rs
diff --git a/tests/compile-fail/toplevel_ref_arg.rs b/tests/compile-fail/toplevel_ref_arg.rs
