From 34812e82d066dc1b3ef89df4272300662374f907 Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 7 Feb 2016 18:10:03 +0100 Subject: [PATCH 1/4] Use const_eval in loops --- src/loops.rs | 45 ++++++++++++++++++++++------------ tests/compile-fail/for_loop.rs | 22 ++++++++++++++--- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index 4620d6a3e81..cecf47daf55 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -1,11 +1,12 @@ -use rustc::lint::*; -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 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 rustc_front::intravisit::{Visitor, walk_expr, walk_block, walk_decl}; 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/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index e361ebe777f..4609c840836 100644 --- 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); } From d27aa960b605f69a22b83917ba3830bd2d5690ec Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 7 Feb 2016 18:14:03 +0100 Subject: [PATCH 2/4] Remove unused Display implementation for consts --- src/consts.rs | 86 --------------------------------------------------- 1 file changed, 86 deletions(-) diff --git a/src/consts.rs b/src/consts.rs index 5f40aff92cc..ddc8560c9b3 100644 --- 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::>() - .join(", ")) - } - Constant::Tuple(ref t) => { - write!(fmt, - "({})", - t.iter() - .map(|i| format!("{}", i)) - .collect::>() - .join(", ")) - } - } - } -} - - fn lit_to_constant(lit: &Lit_) -> Constant { match *lit { Lit_::LitStr(ref is, style) => Constant::Str(is.to_string(), style), From 1a8b8cd28f3e452a1c4bfc2208ada1a2f4a0ebda Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 7 Feb 2016 18:28:37 +0100 Subject: [PATCH 3/4] =?UTF-8?q?Don=E2=80=99t=20use=20`{:=3F}`=20and=20use?= =?UTF-8?q?=20span=5Fsuggestion=20in=20TOPLEVEL=5FREF=5FARG?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/misc.rs | 16 +++++++++++----- tests/compile-fail/toplevel_ref_arg.rs | 12 ++++++++---- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/misc.rs b/src/misc.rs index c0aed78225a..f570c18b742 100644 --- 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/tests/compile-fail/toplevel_ref_arg.rs b/tests/compile-fail/toplevel_ref_arg.rs index 05ad1af0034..de1556ed0e3 100644 --- a/tests/compile-fail/toplevel_ref_arg.rs +++ b/tests/compile-fail/toplevel_ref_arg.rs @@ -15,11 +15,15 @@ fn main() { let y = |ref x| { println!("{:?}", x) }; y(1u8); - let ref x = 1; //~ ERROR `ref` on an entire `let` pattern is discouraged - //~^ HELP try `let x = &1;` + let ref x = 1; + //~^ ERROR `ref` on an entire `let` pattern is discouraged + //~| HELP try + //~| SUGGESTION let x = &1; - let ref y = (&1, 2); //~ ERROR `ref` on an entire `let` pattern is discouraged - //~^ HELP try `let y = &(&1, 2);` + let ref y : (&_, u8) = (&1, 2); + //~^ ERROR `ref` on an entire `let` pattern is discouraged + //~| HELP try + //~| SUGGESTION let y: (&_, u8) = &(&1, 2); let (ref x, _) = (1,2); // okay, not top level println!("The answer is {}.", x); From 2db6965c81ee9f01b12874c115ec2a593b5f2c5f Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 7 Feb 2016 18:30:57 +0100 Subject: [PATCH 4/4] Lint usage of `Debug`-based formatting --- README.md | 3 ++- src/lib.rs | 1 + src/print.rs | 48 ++++++++++++++++++++++++++++++++++--- src/utils.rs | 2 ++ tests/compile-fail/print.rs | 32 ++++++++++++++++++++++++- 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 4d655c85951..87b58b320ff 100644 --- a/README.md +++ b/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!` diff --git a/src/lib.rs b/src/lib.rs index cd9ac226321..ba9236012d1 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/print.rs b/src/print.rs index d8f5fd488aa..3c10b4bed13 100644 --- 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 index a8890f31cb0..4c89b7f113d 100644 --- 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/print.rs b/tests/compile-fail/print.rs index 8141cdd2645..34c38dca286 100755 --- a/tests/compile-fail/print.rs +++ b/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]; }