diff --git a/Cargo.toml b/Cargo.toml index bee10dccd..97dcb2039 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clippy" -version = "0.0.46" +version = "0.0.47" authors = [ "Manish Goregaokar ", "Andre Bogus ", diff --git a/src/loops.rs b/src/loops.rs index 600f22ea0..e41402ef8 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -10,10 +10,13 @@ use rustc_front::hir::*; use rustc_front::intravisit::{Visitor, walk_expr, walk_block, walk_decl}; use std::borrow::Cow; use std::collections::HashMap; +use syntax::ast; use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, in_external_macro, - span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, walk_ptrs_ty}; + span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, + unsugar_range, walk_ptrs_ty}; use utils::{BTREEMAP_PATH, HASHMAP_PATH, LL_PATH, OPTION_PATH, RESULT_PATH, VEC_PATH}; +use utils::UnsugaredRange; /// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. /// @@ -323,10 +326,9 @@ fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &E /// Check for looping over a range and then indexing a sequence with it. /// The iteratee must be a range literal. fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) { - if let ExprRange(Some(ref l), ref r) = arg.node { + if let Some(UnsugaredRange { start: Some(ref start), ref end, .. }) = unsugar_range(&arg) { // the var must be a single name if let PatKind::Ident(_, ref ident, _) = pat.node { - let mut visitor = VarVisitor { cx: cx, var: ident.node.name, @@ -348,19 +350,19 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex return; } - let starts_at_zero = is_integer_literal(l, 0); + let starts_at_zero = is_integer_literal(start, 0); let skip: Cow<_> = if starts_at_zero { "".into() } else { - format!(".skip({})", snippet(cx, l.span, "..")).into() + format!(".skip({})", snippet(cx, start.span, "..")).into() }; - let take: Cow<_> = if let Some(ref r) = *r { - if is_len_call(&r, &indexed) { + let take: Cow<_> = if let Some(ref end) = *end { + if is_len_call(&end, &indexed) { "".into() } else { - format!(".take({})", snippet(cx, r.span, "..")).into() + format!(".take({})", snippet(cx, end.span, "..")).into() } } else { "".into() @@ -416,27 +418,27 @@ fn is_len_call(expr: &Expr, var: &Name) -> bool { 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 { + if let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), limits }) = unsugar_range(&arg) { // ...and both sides are compile-time constant integers... - 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, + if let Ok(start_idx) = eval_const_expr_partial(&cx.tcx, start, ExprTypeChecked, None) { + if let Ok(end_idx) = eval_const_expr_partial(&cx.tcx, end, ExprTypeChecked, None) { + // ...and the start index is greater than the end 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. - let (sup, eq) = match (start_idx, stop_idx) { - (ConstVal::Int(start_idx), ConstVal::Int(stop_idx)) => { - (start_idx > stop_idx, start_idx == stop_idx) + let (sup, eq) = match (start_idx, end_idx) { + (ConstVal::Int(start_idx), ConstVal::Int(end_idx)) => { + (start_idx > end_idx, start_idx == end_idx) } - (ConstVal::Uint(start_idx), ConstVal::Uint(stop_idx)) => { - (start_idx > stop_idx, start_idx == stop_idx) + (ConstVal::Uint(start_idx), ConstVal::Uint(end_idx)) => { + (start_idx > end_idx, start_idx == end_idx) } _ => (false, false), }; if sup { - let start_snippet = snippet(cx, start_expr.span, "_"); - let stop_snippet = snippet(cx, stop_expr.span, "_"); + let start_snippet = snippet(cx, start.span, "_"); + let end_snippet = snippet(cx, end.span, "_"); span_lint_and_then(cx, REVERSE_RANGE_LOOP, @@ -447,9 +449,9 @@ fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) { "consider using the following if \ you are attempting to iterate \ over this range in reverse", - format!("({}..{}).rev()` ", stop_snippet, start_snippet)); + format!("({}..{}).rev()` ", end_snippet, start_snippet)); }); - } else if eq { + } else if eq && limits != ast::RangeLimits::Closed { // if they are equal, it's also problematic - this loop // will never run. span_lint(cx, diff --git a/src/no_effect.rs b/src/no_effect.rs index 65dfeb0d4..59f7be94c 100644 --- a/src/no_effect.rs +++ b/src/no_effect.rs @@ -23,15 +23,11 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool { match expr.node { Expr_::ExprLit(..) | Expr_::ExprClosure(..) | - Expr_::ExprRange(None, None) | Expr_::ExprPath(..) => true, Expr_::ExprIndex(ref a, ref b) | - Expr_::ExprRange(Some(ref a), Some(ref b)) | Expr_::ExprBinary(_, ref a, ref b) => has_no_effect(cx, a) && has_no_effect(cx, b), Expr_::ExprVec(ref v) | Expr_::ExprTup(ref v) => v.iter().all(|val| has_no_effect(cx, val)), - Expr_::ExprRange(Some(ref inner), None) | - Expr_::ExprRange(None, Some(ref inner)) | Expr_::ExprRepeat(ref inner, _) | Expr_::ExprCast(ref inner, _) | Expr_::ExprType(ref inner, _) | @@ -55,6 +51,13 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool { _ => false, } } + Expr_::ExprBlock(ref block) => { + block.stmts.is_empty() && if let Some(ref expr) = block.expr { + has_no_effect(cx, expr) + } else { + false + } + } _ => false, } } diff --git a/src/ranges.rs b/src/ranges.rs index 895bd1801..766d98b4e 100644 --- a/src/ranges.rs +++ b/src/ranges.rs @@ -1,7 +1,7 @@ use rustc::lint::*; use rustc_front::hir::*; use syntax::codemap::Spanned; -use utils::{is_integer_literal, match_type, snippet}; +use utils::{is_integer_literal, match_type, snippet, unsugar_range, UnsugaredRange}; /// **What it does:** This lint checks for iterating over ranges with a `.step_by(0)`, which never terminates. /// @@ -47,17 +47,17 @@ impl LateLintPass for StepByZero { instead") } else if name.as_str() == "zip" && args.len() == 2 { let iter = &args[0].node; - let zip_arg = &args[1].node; + let zip_arg = &args[1]; if_let_chain! { [ // .iter() call let ExprMethodCall( Spanned { node: ref iter_name, .. }, _, ref iter_args ) = *iter, iter_name.as_str() == "iter", // range expression in .zip() call: 0..x.len() - let ExprRange(Some(ref from), Some(ref to)) = *zip_arg, - is_integer_literal(from, 0), + let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), .. }) = unsugar_range(zip_arg), + is_integer_literal(start, 0), // .len() call - let ExprMethodCall(Spanned { node: ref len_name, .. }, _, ref len_args) = to.node, + let ExprMethodCall(Spanned { node: ref len_name, .. }, _, ref len_args) = end.node, len_name.as_str() == "len" && len_args.len() == 1, // .iter() and .len() called on same Path let ExprPath(_, Path { segments: ref iter_path, .. }) = iter_args[0].node, diff --git a/src/utils/hir.rs b/src/utils/hir.rs index 6231970a0..0bd054a83 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -109,14 +109,16 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { !self.ignore_fn && lname.node == rname.node && ltys.is_empty() && rtys.is_empty() && self.eq_exprs(largs, rargs) } - (&ExprRange(ref lb, ref le), &ExprRange(ref rb, ref re)) => { - both(lb, rb, |l, r| self.eq_expr(l, r)) && both(le, re, |l, r| self.eq_expr(l, r)) - } (&ExprRepeat(ref le, ref ll), &ExprRepeat(ref re, ref rl)) => self.eq_expr(le, re) && self.eq_expr(ll, rl), (&ExprRet(ref l), &ExprRet(ref r)) => both(l, r, |l, r| self.eq_expr(l, r)), (&ExprPath(ref lqself, ref lsubpath), &ExprPath(ref rqself, ref rsubpath)) => { both(lqself, rqself, |l, r| self.eq_qself(l, r)) && self.eq_path(lsubpath, rsubpath) } + (&ExprStruct(ref lpath, ref lf, ref lo), &ExprStruct(ref rpath, ref rf, ref ro)) => { + self.eq_path(lpath, rpath) && + both(lo, ro, |l, r| self.eq_expr(l, r)) && + over(lf, rf, |l, r| self.eq_field(l, r)) + } (&ExprTup(ref ltup), &ExprTup(ref rtup)) => self.eq_exprs(ltup, rtup), (&ExprTupField(ref le, li), &ExprTupField(ref re, ri)) => li.node == ri.node && self.eq_expr(le, re), (&ExprUnary(lop, ref le), &ExprUnary(rop, ref re)) => lop == rop && self.eq_expr(le, re), @@ -132,6 +134,10 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> { over(left, right, |l, r| self.eq_expr(l, r)) } + fn eq_field(&self, left: &Field, right: &Field) -> bool { + left.name.node == right.name.node && self.eq_expr(&left.expr, &right.expr) + } + /// Check whether two patterns are the same. pub fn eq_pat(&self, left: &Pat, right: &Pat) -> bool { match (&left.node, &right.node) { @@ -375,16 +381,6 @@ impl<'a, 'tcx: 'a> SpanlessHash<'a, 'tcx> { self.hash_name(&name.node); self.hash_exprs(args); } - ExprRange(ref b, ref e) => { - let c: fn(_, _) -> _ = ExprRange; - c.hash(&mut self.s); - if let Some(ref b) = *b { - self.hash_expr(b); - } - if let Some(ref e) = *e { - self.hash_expr(e); - } - } ExprRepeat(ref e, ref l) => { let c: fn(_, _) -> _ = ExprRepeat; c.hash(&mut self.s); diff --git a/src/utils/mod.rs b/src/utils/mod.rs index f2b0c1f4d..b001d9530 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -9,7 +9,7 @@ use std::borrow::Cow; use std::mem; use std::ops::{Deref, DerefMut}; use std::str::FromStr; -use syntax::ast::{self, LitKind}; +use syntax::ast::{self, LitKind, RangeLimits}; use syntax::codemap::{ExpnInfo, Span, ExpnFormat}; use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; @@ -40,6 +40,12 @@ pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedLis pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"]; pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"]; pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"]; +pub const RANGE_FROM_PATH: [&'static str; 3] = ["std", "ops", "RangeFrom"]; +pub const RANGE_FULL_PATH: [&'static str; 3] = ["std", "ops", "RangeFull"]; +pub const RANGE_INCLUSIVE_NON_EMPTY_PATH: [&'static str; 4] = ["std", "ops", "RangeInclusive", "NonEmpty"]; +pub const RANGE_PATH: [&'static str; 3] = ["std", "ops", "Range"]; +pub const RANGE_TO_INCLUSIVE_PATH: [&'static str; 3] = ["std", "ops", "RangeToInclusive"]; +pub const RANGE_TO_PATH: [&'static str; 3] = ["std", "ops", "RangeTo"]; pub const REGEX_NEW_PATH: [&'static str; 3] = ["regex", "Regex", "new"]; pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"]; pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"]; @@ -673,3 +679,55 @@ pub fn camel_case_from(s: &str) -> usize { } last_i } + +/// Represents a range akin to `ast::ExprKind::Range`. +pub struct UnsugaredRange<'a> { + pub start: Option<&'a Expr>, + pub end: Option<&'a Expr>, + pub limits: RangeLimits, +} + +/// Unsugar a `hir` range. +pub fn unsugar_range(expr: &Expr) -> Option { + // To be removed when ranges get stable. + fn unwrap_unstable(expr: &Expr) -> &Expr { + if let ExprBlock(ref block) = expr.node { + if block.rules == BlockCheckMode::PushUnstableBlock || block.rules == BlockCheckMode::PopUnstableBlock { + if let Some(ref expr) = block.expr { + return expr; + } + } + } + + expr + } + + fn get_field<'a>(name: &str, fields: &'a [Field]) -> Option<&'a Expr> { + let expr = &fields.iter() + .find(|field| field.name.node.as_str() == name) + .unwrap_or_else(|| panic!("missing {} field for range", name)) + .expr; + + Some(unwrap_unstable(expr)) + } + + if let ExprStruct(ref path, ref fields, None) = unwrap_unstable(&expr).node { + if match_path(path, &RANGE_FROM_PATH) { + Some(UnsugaredRange { start: get_field("start", fields), end: None, limits: RangeLimits::HalfOpen }) + } else if match_path(path, &RANGE_FULL_PATH) { + Some(UnsugaredRange { start: None, end: None, limits: RangeLimits::HalfOpen }) + } else if match_path(path, &RANGE_INCLUSIVE_NON_EMPTY_PATH) { + Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::Closed }) + } else if match_path(path, &RANGE_PATH) { + Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::HalfOpen }) + } else if match_path(path, &RANGE_TO_INCLUSIVE_PATH) { + Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::Closed }) + } else if match_path(path, &RANGE_TO_PATH) { + Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::HalfOpen }) + } else { + None + } + } else { + None + } +} diff --git a/tests/compile-fail/copies.rs b/tests/compile-fail/copies.rs index 7a17b345f..c1e1ba68b 100755 --- a/tests/compile-fail/copies.rs +++ b/tests/compile-fail/copies.rs @@ -1,4 +1,4 @@ -#![feature(plugin)] +#![feature(plugin, inclusive_range_syntax)] #![plugin(clippy)] #![allow(dead_code, no_effect)] @@ -10,16 +10,46 @@ fn bar(_: T) {} fn foo() -> bool { unimplemented!() } +struct Foo { + bar: u8, +} + #[deny(if_same_then_else)] #[deny(match_same_arms)] fn if_same_then_else() -> Result<&'static str, ()> { if true { + Foo { bar: 42 }; + 0..10; + ..; + 0..; + ..10; + 0...10; foo(); } else { //~ERROR this `if` has identical blocks + Foo { bar: 42 }; + 0..10; + ..; + 0..; + ..10; + 0...10; foo(); } + if true { + Foo { bar: 42 }; + } + else { + Foo { bar: 43 }; + } + + if true { + 0..10; + } + else { + 0...10; + } + if true { foo(); foo(); diff --git a/tests/compile-fail/for_loop.rs b/tests/compile-fail/for_loop.rs index 69ce68b17..0853ae83c 100644 --- a/tests/compile-fail/for_loop.rs +++ b/tests/compile-fail/for_loop.rs @@ -1,4 +1,4 @@ -#![feature(plugin, step_by)] +#![feature(plugin, step_by, inclusive_range_syntax)] #![plugin(clippy)] use std::collections::*; @@ -118,11 +118,21 @@ 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]); } + 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]); + } + for i in 5..vec.len() { //~^ ERROR `i` is used to index `vec`. Consider using `for (i, item) in vec.iter().enumerate().skip(5)` println!("{} {}", vec[i], i); @@ -140,6 +150,13 @@ fn main() { println!("{}", i); } + 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() @@ -150,6 +167,10 @@ fn main() { println!("{}", i); } + for i in 5...5 { // not an error, this is the range with only one element “5” + println!("{}", i); + } + for i in 0..10 { // not an error, the start index is less than the end index println!("{}", i); } @@ -158,10 +179,6 @@ fn main() { println!("{}", i); } - for i in (10..0).rev() { // not an error, this is an established idiom for looping backwards on a range - println!("{}", i); - } - for i in (10..0).map(|x| x * 2) { // not an error, it can't be known what arbitrary methods do to a range println!("{}", i); } diff --git a/tests/compile-fail/no_effect.rs b/tests/compile-fail/no_effect.rs index 52ea423a5..344c82f33 100644 --- a/tests/compile-fail/no_effect.rs +++ b/tests/compile-fail/no_effect.rs @@ -1,4 +1,4 @@ -#![feature(plugin, box_syntax)] +#![feature(plugin, box_syntax, inclusive_range_syntax)] #![plugin(clippy)] #![deny(no_effect)] @@ -39,6 +39,7 @@ fn main() { 5..; //~ERROR statement with no effect ..5; //~ERROR statement with no effect 5..6; //~ERROR statement with no effect + 5...6; //~ERROR statement with no effect [42, 55]; //~ERROR statement with no effect [42, 55][1]; //~ERROR statement with no effect (42, 55).1; //~ERROR statement with no effect