Skip to content

Commit 13bb22a

Browse files
mcartonmcarton
committed
Remove all ExprRange
Rustup to rustc 1.9.0-nightly (998a672 2016-03-07)
1 parent eb0a493 commit 13bb22a

File tree

7 files changed

+125
-45
lines changed

7 files changed

+125
-45
lines changed

‎src/loops.rs

+21-20
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@ use std::borrow::Cow;
1212
use std::collections::HashMap;
1313

1414
use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, in_external_macro,
15-
span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then, walk_ptrs_ty};
15+
span_help_and_lint, is_integer_literal, get_enclosing_block, span_lint_and_then,
16+
unsugar_range, walk_ptrs_ty};
1617
use utils::{BTREEMAP_PATH, HASHMAP_PATH, LL_PATH, OPTION_PATH, RESULT_PATH, VEC_PATH};
18+
use utils::UnsugaredRange;
1719

1820
/// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index.
1921
///
@@ -323,10 +325,9 @@ fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &E
323325
/// Check for looping over a range and then indexing a sequence with it.
324326
/// The iteratee must be a range literal.
325327
fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) {
326-
if let ExprRange(Some(ref l), ref r) = arg.node {
328+
if let Some(UnsugaredRange { start: Some(ref start), ref end, .. }) = unsugar_range(&arg) {
327329
// the var must be a single name
328330
if let PatKind::Ident(_, ref ident, _) = pat.node {
329-
330331
let mut visitor = VarVisitor {
331332
cx: cx,
332333
var: ident.node.name,
@@ -348,19 +349,19 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex
348349
return;
349350
}
350351

351-
let starts_at_zero = is_integer_literal(l, 0);
352+
let starts_at_zero = is_integer_literal(start, 0);
352353

353354
let skip: Cow<_> = if starts_at_zero {
354355
"".into()
355356
} else {
356-
format!(".skip({})", snippet(cx, l.span, "..")).into()
357+
format!(".skip({})", snippet(cx, start.span, "..")).into()
357358
};
358359

359-
let take: Cow<_> = if let Some(ref r) = *r {
360-
if is_len_call(&r, &indexed) {
360+
let take: Cow<_> = if let Some(ref end) = *end {
361+
if is_len_call(&end, &indexed) {
361362
"".into()
362363
} else {
363-
format!(".take({})", snippet(cx, r.span, "..")).into()
364+
format!(".take({})", snippet(cx, end.span, "..")).into()
364365
}
365366
} else {
366367
"".into()
@@ -416,27 +417,27 @@ fn is_len_call(expr: &Expr, var: &Name) -> bool {
416417

417418
fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) {
418419
// if this for loop is iterating over a two-sided range...
419-
if let ExprRange(Some(ref start_expr), Some(ref stop_expr)) = arg.node {
420+
if let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), .. }) = unsugar_range(&arg) {
420421
// ...and both sides are compile-time constant integers...
421-
if let Ok(start_idx) = eval_const_expr_partial(&cx.tcx, start_expr, ExprTypeChecked, None) {
422-
if let Ok(stop_idx) = eval_const_expr_partial(&cx.tcx, stop_expr, ExprTypeChecked, None) {
423-
// ...and the start index is greater than the stop index,
422+
if let Ok(start_idx) = eval_const_expr_partial(&cx.tcx, start, ExprTypeChecked, None) {
423+
if let Ok(end_idx) = eval_const_expr_partial(&cx.tcx, end, ExprTypeChecked, None) {
424+
// ...and the start index is greater than the end index,
424425
// this loop will never run. This is often confusing for developers
425426
// who think that this will iterate from the larger value to the
426427
// smaller value.
427-
let (sup, eq) = match (start_idx, stop_idx) {
428-
(ConstVal::Int(start_idx), ConstVal::Int(stop_idx)) => {
429-
(start_idx > stop_idx, start_idx == stop_idx)
428+
let (sup, eq) = match (start_idx, end_idx) {
429+
(ConstVal::Int(start_idx), ConstVal::Int(end_idx)) => {
430+
(start_idx > end_idx, start_idx == end_idx)
430431
}
431-
(ConstVal::Uint(start_idx), ConstVal::Uint(stop_idx)) => {
432-
(start_idx > stop_idx, start_idx == stop_idx)
432+
(ConstVal::Uint(start_idx), ConstVal::Uint(end_idx)) => {
433+
(start_idx > end_idx, start_idx == end_idx)
433434
}
434435
_ => (false, false),
435436
};
436437

437438
if sup {
438-
let start_snippet = snippet(cx, start_expr.span, "_");
439-
let stop_snippet = snippet(cx, stop_expr.span, "_");
439+
let start_snippet = snippet(cx, start.span, "_");
440+
let end_snippet = snippet(cx, end.span, "_");
440441

441442
span_lint_and_then(cx,
442443
REVERSE_RANGE_LOOP,
@@ -447,7 +448,7 @@ fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) {
447448
"consider using the following if \
448449
you are attempting to iterate \
449450
over this range in reverse",
450-
format!("({}..{}).rev()` ", stop_snippet, start_snippet));
451+
format!("({}..{}).rev()` ", end_snippet, start_snippet));
451452
});
452453
} else if eq {
453454
// if they are equal, it's also problematic - this loop

‎src/no_effect.rs

+7-4
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,11 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool {
2323
match expr.node {
2424
Expr_::ExprLit(..) |
2525
Expr_::ExprClosure(..) |
26-
Expr_::ExprRange(None, None) |
2726
Expr_::ExprPath(..) => true,
2827
Expr_::ExprIndex(ref a, ref b) |
29-
Expr_::ExprRange(Some(ref a), Some(ref b)) |
3028
Expr_::ExprBinary(_, ref a, ref b) => has_no_effect(cx, a) && has_no_effect(cx, b),
3129
Expr_::ExprVec(ref v) |
3230
Expr_::ExprTup(ref v) => v.iter().all(|val| has_no_effect(cx, val)),
33-
Expr_::ExprRange(Some(ref inner), None) |
34-
Expr_::ExprRange(None, Some(ref inner)) |
3531
Expr_::ExprRepeat(ref inner, _) |
3632
Expr_::ExprCast(ref inner, _) |
3733
Expr_::ExprType(ref inner, _) |
@@ -55,6 +51,13 @@ fn has_no_effect(cx: &LateContext, expr: &Expr) -> bool {
5551
_ => false,
5652
}
5753
}
54+
Expr_::ExprBlock(ref block) => {
55+
block.stmts.is_empty() && if let Some(ref expr) = block.expr {
56+
has_no_effect(cx, expr)
57+
} else {
58+
false
59+
}
60+
}
5861
_ => false,
5962
}
6063
}

‎src/ranges.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use rustc::lint::*;
22
use rustc_front::hir::*;
33
use syntax::codemap::Spanned;
4-
use utils::{is_integer_literal, match_type, snippet};
4+
use utils::{is_integer_literal, match_type, snippet, unsugar_range, UnsugaredRange};
55

66
/// **What it does:** This lint checks for iterating over ranges with a `.step_by(0)`, which never terminates.
77
///
@@ -47,17 +47,17 @@ impl LateLintPass for StepByZero {
4747
instead")
4848
} else if name.as_str() == "zip" && args.len() == 2 {
4949
let iter = &args[0].node;
50-
let zip_arg = &args[1].node;
50+
let zip_arg = &args[1];
5151
if_let_chain! {
5252
[
5353
// .iter() call
5454
let ExprMethodCall( Spanned { node: ref iter_name, .. }, _, ref iter_args ) = *iter,
5555
iter_name.as_str() == "iter",
5656
// range expression in .zip() call: 0..x.len()
57-
let ExprRange(Some(ref from), Some(ref to)) = *zip_arg,
58-
is_integer_literal(from, 0),
57+
let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), .. }) = unsugar_range(zip_arg),
58+
is_integer_literal(start, 0),
5959
// .len() call
60-
let ExprMethodCall(Spanned { node: ref len_name, .. }, _, ref len_args) = to.node,
60+
let ExprMethodCall(Spanned { node: ref len_name, .. }, _, ref len_args) = end.node,
6161
len_name.as_str() == "len" && len_args.len() == 1,
6262
// .iter() and .len() called on same Path
6363
let ExprPath(_, Path { segments: ref iter_path, .. }) = iter_args[0].node,

‎src/utils/hir.rs

-13
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,6 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
109109
!self.ignore_fn && lname.node == rname.node && ltys.is_empty() && rtys.is_empty() &&
110110
self.eq_exprs(largs, rargs)
111111
}
112-
(&ExprRange(ref lb, ref le), &ExprRange(ref rb, ref re)) => {
113-
both(lb, rb, |l, r| self.eq_expr(l, r)) && both(le, re, |l, r| self.eq_expr(l, r))
114-
}
115112
(&ExprRepeat(ref le, ref ll), &ExprRepeat(ref re, ref rl)) => self.eq_expr(le, re) && self.eq_expr(ll, rl),
116113
(&ExprRet(ref l), &ExprRet(ref r)) => both(l, r, |l, r| self.eq_expr(l, r)),
117114
(&ExprPath(ref lqself, ref lsubpath), &ExprPath(ref rqself, ref rsubpath)) => {
@@ -384,16 +381,6 @@ impl<'a, 'tcx: 'a> SpanlessHash<'a, 'tcx> {
384381
self.hash_name(&name.node);
385382
self.hash_exprs(args);
386383
}
387-
ExprRange(ref b, ref e) => {
388-
let c: fn(_, _) -> _ = ExprRange;
389-
c.hash(&mut self.s);
390-
if let Some(ref b) = *b {
391-
self.hash_expr(b);
392-
}
393-
if let Some(ref e) = *e {
394-
self.hash_expr(e);
395-
}
396-
}
397384
ExprRepeat(ref e, ref l) => {
398385
let c: fn(_, _) -> _ = ExprRepeat;
399386
c.hash(&mut self.s);

‎src/utils/mod.rs

+59-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use std::borrow::Cow;
99
use std::mem;
1010
use std::ops::{Deref, DerefMut};
1111
use std::str::FromStr;
12-
use syntax::ast::{self, LitKind};
12+
use syntax::ast::{self, LitKind, RangeLimits};
1313
use syntax::codemap::{ExpnInfo, Span, ExpnFormat};
1414
use syntax::errors::DiagnosticBuilder;
1515
use syntax::ptr::P;
@@ -40,6 +40,12 @@ pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedLis
4040
pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
4141
pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"];
4242
pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
43+
pub const RANGE_FROM_PATH: [&'static str; 3] = ["std", "ops", "RangeFrom"];
44+
pub const RANGE_FULL_PATH: [&'static str; 3] = ["std", "ops", "RangeFull"];
45+
pub const RANGE_INCLUSIVE_NON_EMPTY_PATH: [&'static str; 4] = ["std", "ops", "RangeInclusive", "NonEmpty"];
46+
pub const RANGE_PATH: [&'static str; 3] = ["std", "ops", "Range"];
47+
pub const RANGE_TO_INCLUSIVE_PATH: [&'static str; 3] = ["std", "ops", "RangeToInclusive"];
48+
pub const RANGE_TO_PATH: [&'static str; 3] = ["std", "ops", "RangeTo"];
4349
pub const REGEX_NEW_PATH: [&'static str; 3] = ["regex", "Regex", "new"];
4450
pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
4551
pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"];
@@ -673,3 +679,55 @@ pub fn camel_case_from(s: &str) -> usize {
673679
}
674680
last_i
675681
}
682+
683+
/// Represents a range akin to `ast::ExprKind::Range`.
684+
pub struct UnsugaredRange<'a> {
685+
pub start: Option<&'a Expr>,
686+
pub end: Option<&'a Expr>,
687+
pub limits: RangeLimits,
688+
}
689+
690+
/// Unsugar a `hir` range.
691+
pub fn unsugar_range(expr: &Expr) -> Option<UnsugaredRange> {
692+
// To be removed when ranges get stable.
693+
fn unwrap_unstable(expr: &Expr) -> &Expr {
694+
if let ExprBlock(ref block) = expr.node {
695+
if block.rules == BlockCheckMode::PushUnstableBlock || block.rules == BlockCheckMode::PopUnstableBlock {
696+
if let Some(ref expr) = block.expr {
697+
return expr;
698+
}
699+
}
700+
}
701+
702+
expr
703+
}
704+
705+
fn get_field<'a>(name: &str, fields: &'a [Field]) -> Option<&'a Expr> {
706+
let expr = &fields.iter()
707+
.find(|field| field.name.node.as_str() == name)
708+
.unwrap_or_else(|| panic!("missing {} field for range", name))
709+
.expr;
710+
711+
Some(unwrap_unstable(expr))
712+
}
713+
714+
if let ExprStruct(ref path, ref fields, None) = unwrap_unstable(&expr).node {
715+
if match_path(path, &RANGE_FROM_PATH) {
716+
Some(UnsugaredRange { start: get_field("start", fields), end: None, limits: RangeLimits::HalfOpen })
717+
} else if match_path(path, &RANGE_FULL_PATH) {
718+
Some(UnsugaredRange { start: None, end: None, limits: RangeLimits::HalfOpen })
719+
} else if match_path(path, &RANGE_INCLUSIVE_NON_EMPTY_PATH) {
720+
Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::Closed })
721+
} else if match_path(path, &RANGE_PATH) {
722+
Some(UnsugaredRange { start: get_field("start", fields), end: get_field("end", fields), limits: RangeLimits::HalfOpen })
723+
} else if match_path(path, &RANGE_TO_INCLUSIVE_PATH) {
724+
Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::Closed })
725+
} else if match_path(path, &RANGE_TO_PATH) {
726+
Some(UnsugaredRange { start: None, end: get_field("end", fields), limits: RangeLimits::HalfOpen })
727+
} else {
728+
None
729+
}
730+
} else {
731+
None
732+
}
733+
}

‎tests/compile-fail/copies.rs

+31-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#![feature(plugin)]
1+
#![feature(plugin, inclusive_range_syntax)]
22
#![plugin(clippy)]
33

44
#![allow(dead_code, no_effect)]
@@ -10,16 +10,46 @@
1010
fn bar<T>(_: T) {}
1111
fn foo() -> bool { unimplemented!() }
1212

13+
struct Foo {
14+
bar: u8,
15+
}
16+
1317
#[deny(if_same_then_else)]
1418
#[deny(match_same_arms)]
1519
fn if_same_then_else() -> Result<&'static str, ()> {
1620
if true {
21+
Foo { bar: 42 };
22+
0..10;
23+
..;
24+
0..;
25+
..10;
26+
0...10;
1727
foo();
1828
}
1929
else { //~ERROR this `if` has identical blocks
30+
Foo { bar: 42 };
31+
0..10;
32+
..;
33+
0..;
34+
..10;
35+
0...10;
2036
foo();
2137
}
2238

39+
if true {
40+
Foo { bar: 42 };
41+
}
42+
else {
43+
Foo { bar: 43 };
44+
}
45+
46+
if true {
47+
0..10;
48+
}
49+
else {
50+
0...10;
51+
}
52+
2353
if true {
2454
foo();
2555
foo();

‎tests/compile-fail/no_effect.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#![feature(plugin, box_syntax)]
1+
#![feature(plugin, box_syntax, inclusive_range_syntax)]
22
#![plugin(clippy)]
33

44
#![deny(no_effect)]
@@ -39,6 +39,7 @@ fn main() {
3939
5..; //~ERROR statement with no effect
4040
..5; //~ERROR statement with no effect
4141
5..6; //~ERROR statement with no effect
42+
5...6; //~ERROR statement with no effect
4243
[42, 55]; //~ERROR statement with no effect
4344
[42, 55][1]; //~ERROR statement with no effect
4445
(42, 55).1; //~ERROR statement with no effect

0 commit comments

Comments
 (0)