diff --git a/src/loops.rs b/src/loops.rs index 600f22ea0016..723236c7ed73 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -12,8 +12,10 @@ use std::borrow::Cow; use std::collections::HashMap; 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 +325,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 +349,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 +417,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), .. }) = 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,7 +448,7 @@ 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 { // if they are equal, it's also problematic - this loop diff --git a/src/no_effect.rs b/src/no_effect.rs index 65dfeb0d4be6..59f7be94c237 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 895bd1801687..766d98b4e0b9 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 6b745c15e6fd..0bd054a839a7 100644 --- a/src/utils/hir.rs +++ b/src/utils/hir.rs @@ -109,9 +109,6 @@ 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)) => { @@ -384,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 f2b0c1f4db1a..b001d9530edd 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 7a17b345fa86..c1e1ba68b3e7 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/no_effect.rs b/tests/compile-fail/no_effect.rs index 52ea423a57d7..344c82f33075 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