mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-27 07:00:55 +00:00
New lint: manual-range-contains
This commit is contained in:
parent
85959be606
commit
c693de350a
7 changed files with 354 additions and 31 deletions
|
@ -1793,6 +1793,7 @@ Released 2018-09-13
|
|||
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
|
||||
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
|
||||
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
|
||||
[`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
|
||||
[`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
|
||||
[`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip
|
||||
[`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap
|
||||
|
|
|
@ -785,6 +785,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
&ptr_eq::PTR_EQ,
|
||||
&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST,
|
||||
&question_mark::QUESTION_MARK,
|
||||
&ranges::MANUAL_RANGE_CONTAINS,
|
||||
&ranges::RANGE_MINUS_ONE,
|
||||
&ranges::RANGE_PLUS_ONE,
|
||||
&ranges::RANGE_ZIP_WITH_LEN,
|
||||
|
@ -1469,6 +1470,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
LintId::of(&ptr_eq::PTR_EQ),
|
||||
LintId::of(&ptr_offset_with_cast::PTR_OFFSET_WITH_CAST),
|
||||
LintId::of(&question_mark::QUESTION_MARK),
|
||||
LintId::of(&ranges::MANUAL_RANGE_CONTAINS),
|
||||
LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
|
||||
LintId::of(&ranges::REVERSED_EMPTY_RANGES),
|
||||
LintId::of(&redundant_clone::REDUNDANT_CLONE),
|
||||
|
@ -1624,6 +1626,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
|
|||
LintId::of(&ptr::PTR_ARG),
|
||||
LintId::of(&ptr_eq::PTR_EQ),
|
||||
LintId::of(&question_mark::QUESTION_MARK),
|
||||
LintId::of(&ranges::MANUAL_RANGE_CONTAINS),
|
||||
LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES),
|
||||
LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
|
||||
LintId::of(®ex::TRIVIAL_REGEX),
|
||||
|
|
|
@ -2,15 +2,19 @@ use crate::consts::{constant, Constant};
|
|||
use if_chain::if_chain;
|
||||
use rustc_ast::ast::RangeLimits;
|
||||
use rustc_errors::Applicability;
|
||||
use rustc_hir::{BinOpKind, Expr, ExprKind, QPath};
|
||||
use rustc_hir::{BinOpKind, Expr, ExprKind, PathSegment, QPath};
|
||||
use rustc_lint::{LateContext, LateLintPass};
|
||||
use rustc_middle::ty;
|
||||
use rustc_session::{declare_lint_pass, declare_tool_lint};
|
||||
use rustc_span::source_map::Spanned;
|
||||
use rustc_span::source_map::{Span, Spanned};
|
||||
use rustc_span::symbol::Ident;
|
||||
use std::cmp::Ordering;
|
||||
|
||||
use crate::utils::sugg::Sugg;
|
||||
use crate::utils::{get_parent_expr, is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then};
|
||||
use crate::utils::{
|
||||
get_parent_expr, is_integer_const, single_segment_path, snippet, snippet_opt, snippet_with_applicability,
|
||||
span_lint, span_lint_and_sugg, span_lint_and_then,
|
||||
};
|
||||
use crate::utils::{higher, SpanlessEq};
|
||||
|
||||
declare_clippy_lint! {
|
||||
|
@ -128,43 +132,51 @@ declare_clippy_lint! {
|
|||
"reversing the limits of range expressions, resulting in empty ranges"
|
||||
}
|
||||
|
||||
declare_clippy_lint! {
|
||||
/// **What it does:** Checks for expressions like `x >= 3 && x < 8` that could
|
||||
/// be more readably expressed as `(3..8).contains(x)`.
|
||||
///
|
||||
/// **Why is this bad?** `contains` expresses the intent better and has less
|
||||
/// failure modes (such as fencepost errors or using `||` instead of `&&`).
|
||||
///
|
||||
/// **Known problems:** None.
|
||||
///
|
||||
/// **Example:**
|
||||
///
|
||||
/// ```rust
|
||||
/// // given
|
||||
/// let x = 6;
|
||||
///
|
||||
/// assert!(x >= 3 && x < 8);
|
||||
/// ```
|
||||
/// Use instead:
|
||||
/// ```rust
|
||||
///# let x = 6;
|
||||
/// assert!((3..8).contains(&x));
|
||||
/// ```
|
||||
pub MANUAL_RANGE_CONTAINS,
|
||||
style,
|
||||
"manually reimplementing {`Range`, `RangeInclusive`}`::contains`"
|
||||
}
|
||||
|
||||
declare_lint_pass!(Ranges => [
|
||||
RANGE_ZIP_WITH_LEN,
|
||||
RANGE_PLUS_ONE,
|
||||
RANGE_MINUS_ONE,
|
||||
REVERSED_EMPTY_RANGES,
|
||||
MANUAL_RANGE_CONTAINS,
|
||||
]);
|
||||
|
||||
impl<'tcx> LateLintPass<'tcx> for Ranges {
|
||||
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
|
||||
if let ExprKind::MethodCall(ref path, _, ref args, _) = expr.kind {
|
||||
let name = path.ident.as_str();
|
||||
if name == "zip" && args.len() == 2 {
|
||||
let iter = &args[0].kind;
|
||||
let zip_arg = &args[1];
|
||||
if_chain! {
|
||||
// `.iter()` call
|
||||
if let ExprKind::MethodCall(ref iter_path, _, ref iter_args , _) = *iter;
|
||||
if iter_path.ident.name == sym!(iter);
|
||||
// range expression in `.zip()` call: `0..x.len()`
|
||||
if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(zip_arg);
|
||||
if is_integer_const(cx, start, 0);
|
||||
// `.len()` call
|
||||
if let ExprKind::MethodCall(ref len_path, _, ref len_args, _) = end.kind;
|
||||
if len_path.ident.name == sym!(len) && len_args.len() == 1;
|
||||
// `.iter()` and `.len()` called on same `Path`
|
||||
if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].kind;
|
||||
if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
|
||||
if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
|
||||
then {
|
||||
span_lint(cx,
|
||||
RANGE_ZIP_WITH_LEN,
|
||||
expr.span,
|
||||
&format!("it is more idiomatic to use `{}.iter().enumerate()`",
|
||||
snippet(cx, iter_args[0].span, "_")));
|
||||
}
|
||||
}
|
||||
}
|
||||
match expr.kind {
|
||||
ExprKind::MethodCall(ref path, _, ref args, _) => {
|
||||
check_range_zip_with_len(cx, path, args, expr.span);
|
||||
},
|
||||
ExprKind::Binary(ref op, ref l, ref r) => {
|
||||
check_possible_range_contains(cx, op.node, l, r, expr.span);
|
||||
},
|
||||
_ => {},
|
||||
}
|
||||
|
||||
check_exclusive_range_plus_one(cx, expr);
|
||||
|
@ -173,6 +185,148 @@ impl<'tcx> LateLintPass<'tcx> for Ranges {
|
|||
}
|
||||
}
|
||||
|
||||
fn check_possible_range_contains(cx: &LateContext<'_>, op: BinOpKind, l: &Expr<'_>, r: &Expr<'_>, span: Span) {
|
||||
let combine_and = match op {
|
||||
BinOpKind::And | BinOpKind::BitAnd => true,
|
||||
BinOpKind::Or | BinOpKind::BitOr => false,
|
||||
_ => return,
|
||||
};
|
||||
// value, name, order (higher/lower), inclusiveness
|
||||
if let (Some((lval, lname, name_span, lval_span, lord, linc)), Some((rval, rname, _, rval_span, rord, rinc))) =
|
||||
(check_range_bounds(cx, l), check_range_bounds(cx, r))
|
||||
{
|
||||
// we only lint comparisons on the same name and with different
|
||||
// direction
|
||||
if lname != rname || lord == rord {
|
||||
return;
|
||||
}
|
||||
let ord = Constant::partial_cmp(cx.tcx, cx.typeck_results().expr_ty(l), &lval, &rval);
|
||||
if combine_and && ord == Some(rord) {
|
||||
// order lower bound and upper bound
|
||||
let (l_span, u_span, l_inc, u_inc) = if rord == Ordering::Less {
|
||||
(lval_span, rval_span, linc, rinc)
|
||||
} else {
|
||||
(rval_span, lval_span, rinc, linc)
|
||||
};
|
||||
// we only lint inclusive lower bounds
|
||||
if !l_inc {
|
||||
return;
|
||||
}
|
||||
let (range_type, range_op) = if u_inc {
|
||||
("RangeInclusive", "..=")
|
||||
} else {
|
||||
("Range", "..")
|
||||
};
|
||||
let mut applicability = Applicability::MachineApplicable;
|
||||
let name = snippet_with_applicability(cx, name_span, "_", &mut applicability);
|
||||
let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability);
|
||||
let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability);
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
MANUAL_RANGE_CONTAINS,
|
||||
span,
|
||||
&format!("manual `{}::contains` implementation", range_type),
|
||||
"use",
|
||||
format!("({}{}{}).contains(&{})", lo, range_op, hi, name),
|
||||
applicability,
|
||||
);
|
||||
} else if !combine_and && ord == Some(lord) {
|
||||
// `!_.contains(_)`
|
||||
// order lower bound and upper bound
|
||||
let (l_span, u_span, l_inc, u_inc) = if lord == Ordering::Less {
|
||||
(lval_span, rval_span, linc, rinc)
|
||||
} else {
|
||||
(rval_span, lval_span, rinc, linc)
|
||||
};
|
||||
if l_inc {
|
||||
return;
|
||||
}
|
||||
let (range_type, range_op) = if u_inc {
|
||||
("Range", "..")
|
||||
} else {
|
||||
("RangeInclusive", "..=")
|
||||
};
|
||||
let mut applicability = Applicability::MachineApplicable;
|
||||
let name = snippet_with_applicability(cx, name_span, "_", &mut applicability);
|
||||
let lo = snippet_with_applicability(cx, l_span, "_", &mut applicability);
|
||||
let hi = snippet_with_applicability(cx, u_span, "_", &mut applicability);
|
||||
span_lint_and_sugg(
|
||||
cx,
|
||||
MANUAL_RANGE_CONTAINS,
|
||||
span,
|
||||
&format!("manual `!{}::contains` implementation", range_type),
|
||||
"use",
|
||||
format!("!({}{}{}).contains(&{})", lo, range_op, hi, name),
|
||||
applicability,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn check_range_bounds(cx: &LateContext<'_>, ex: &Expr<'_>) -> Option<(Constant, Ident, Span, Span, Ordering, bool)> {
|
||||
if let ExprKind::Binary(ref op, ref l, ref r) = ex.kind {
|
||||
let (inclusive, ordering) = match op.node {
|
||||
BinOpKind::Gt => (false, Ordering::Greater),
|
||||
BinOpKind::Ge => (true, Ordering::Greater),
|
||||
BinOpKind::Lt => (false, Ordering::Less),
|
||||
BinOpKind::Le => (true, Ordering::Less),
|
||||
_ => return None,
|
||||
};
|
||||
if let Some(id) = match_ident(l) {
|
||||
if let Some((c, _)) = constant(cx, cx.typeck_results(), r) {
|
||||
return Some((c, id, l.span, r.span, ordering, inclusive));
|
||||
}
|
||||
} else if let Some(id) = match_ident(r) {
|
||||
if let Some((c, _)) = constant(cx, cx.typeck_results(), l) {
|
||||
return Some((c, id, r.span, l.span, ordering.reverse(), inclusive));
|
||||
}
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn match_ident(e: &Expr<'_>) -> Option<Ident> {
|
||||
if let ExprKind::Path(ref qpath) = e.kind {
|
||||
if let Some(seg) = single_segment_path(qpath) {
|
||||
if seg.args.is_none() {
|
||||
return Some(seg.ident);
|
||||
}
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn check_range_zip_with_len(cx: &LateContext<'_>, path: &PathSegment<'_>, args: &[Expr<'_>], span: Span) {
|
||||
let name = path.ident.as_str();
|
||||
if name == "zip" && args.len() == 2 {
|
||||
let iter = &args[0].kind;
|
||||
let zip_arg = &args[1];
|
||||
if_chain! {
|
||||
// `.iter()` call
|
||||
if let ExprKind::MethodCall(ref iter_path, _, ref iter_args, _) = *iter;
|
||||
if iter_path.ident.name == sym!(iter);
|
||||
// range expression in `.zip()` call: `0..x.len()`
|
||||
if let Some(higher::Range { start: Some(start), end: Some(end), .. }) = higher::range(zip_arg);
|
||||
if is_integer_const(cx, start, 0);
|
||||
// `.len()` call
|
||||
if let ExprKind::MethodCall(ref len_path, _, ref len_args, _) = end.kind;
|
||||
if len_path.ident.name == sym!(len) && len_args.len() == 1;
|
||||
// `.iter()` and `.len()` called on same `Path`
|
||||
if let ExprKind::Path(QPath::Resolved(_, ref iter_path)) = iter_args[0].kind;
|
||||
if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].kind;
|
||||
if SpanlessEq::new(cx).eq_path_segments(&iter_path.segments, &len_path.segments);
|
||||
then {
|
||||
span_lint(cx,
|
||||
RANGE_ZIP_WITH_LEN,
|
||||
span,
|
||||
&format!("it is more idiomatic to use `{}.iter().enumerate()`",
|
||||
snippet(cx, iter_args[0].span, "_"))
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// exclusive range plus one: `x..(y+1)`
|
||||
fn check_exclusive_range_plus_one(cx: &LateContext<'_>, expr: &Expr<'_>) {
|
||||
if_chain! {
|
||||
|
|
|
@ -1159,6 +1159,13 @@ vec![
|
|||
deprecation: None,
|
||||
module: "manual_non_exhaustive",
|
||||
},
|
||||
Lint {
|
||||
name: "manual_range_contains",
|
||||
group: "style",
|
||||
desc: "manually reimplementing {`Range`, `RangeInclusive`}`::contains`",
|
||||
deprecation: None,
|
||||
module: "ranges",
|
||||
},
|
||||
Lint {
|
||||
name: "manual_saturating_arithmetic",
|
||||
group: "style",
|
||||
|
|
41
tests/ui/range_contains.fixed
Normal file
41
tests/ui/range_contains.fixed
Normal file
|
@ -0,0 +1,41 @@
|
|||
// run-rustfix
|
||||
|
||||
#[warn(clippy::manual_range_contains)]
|
||||
#[allow(unused)]
|
||||
#[allow(clippy::no_effect)]
|
||||
#[allow(clippy::short_circuit_statement)]
|
||||
#[allow(clippy::unnecessary_operation)]
|
||||
fn main() {
|
||||
let x = 9_u32;
|
||||
|
||||
// order shouldn't matter
|
||||
(8..12).contains(&x);
|
||||
(21..42).contains(&x);
|
||||
(1..100).contains(&x);
|
||||
|
||||
// also with inclusive ranges
|
||||
(9..=99).contains(&x);
|
||||
(1..=33).contains(&x);
|
||||
(1..=999).contains(&x);
|
||||
|
||||
// and the outside
|
||||
!(8..12).contains(&x);
|
||||
!(21..42).contains(&x);
|
||||
!(1..100).contains(&x);
|
||||
|
||||
// also with the outside of inclusive ranges
|
||||
!(9..=99).contains(&x);
|
||||
!(1..=33).contains(&x);
|
||||
!(1..=999).contains(&x);
|
||||
|
||||
// not a range.contains
|
||||
x > 8 && x < 12; // lower bound not inclusive
|
||||
x < 8 && x <= 12; // same direction
|
||||
x >= 12 && 12 >= x; // same bounds
|
||||
x < 8 && x > 12; // wrong direction
|
||||
|
||||
x <= 8 || x >= 12;
|
||||
x >= 8 || x >= 12;
|
||||
x < 12 || 12 < x;
|
||||
x >= 8 || x <= 12;
|
||||
}
|
41
tests/ui/range_contains.rs
Normal file
41
tests/ui/range_contains.rs
Normal file
|
@ -0,0 +1,41 @@
|
|||
// run-rustfix
|
||||
|
||||
#[warn(clippy::manual_range_contains)]
|
||||
#[allow(unused)]
|
||||
#[allow(clippy::no_effect)]
|
||||
#[allow(clippy::short_circuit_statement)]
|
||||
#[allow(clippy::unnecessary_operation)]
|
||||
fn main() {
|
||||
let x = 9_u32;
|
||||
|
||||
// order shouldn't matter
|
||||
x >= 8 && x < 12;
|
||||
x < 42 && x >= 21;
|
||||
100 > x && 1 <= x;
|
||||
|
||||
// also with inclusive ranges
|
||||
x >= 9 && x <= 99;
|
||||
x <= 33 && x >= 1;
|
||||
999 >= x && 1 <= x;
|
||||
|
||||
// and the outside
|
||||
x < 8 || x >= 12;
|
||||
x >= 42 || x < 21;
|
||||
100 <= x || 1 > x;
|
||||
|
||||
// also with the outside of inclusive ranges
|
||||
x < 9 || x > 99;
|
||||
x > 33 || x < 1;
|
||||
999 < x || 1 > x;
|
||||
|
||||
// not a range.contains
|
||||
x > 8 && x < 12; // lower bound not inclusive
|
||||
x < 8 && x <= 12; // same direction
|
||||
x >= 12 && 12 >= x; // same bounds
|
||||
x < 8 && x > 12; // wrong direction
|
||||
|
||||
x <= 8 || x >= 12;
|
||||
x >= 8 || x >= 12;
|
||||
x < 12 || 12 < x;
|
||||
x >= 8 || x <= 12;
|
||||
}
|
76
tests/ui/range_contains.stderr
Normal file
76
tests/ui/range_contains.stderr
Normal file
|
@ -0,0 +1,76 @@
|
|||
error: manual `Range::contains` implementation
|
||||
--> $DIR/range_contains.rs:12:5
|
||||
|
|
||||
LL | x >= 8 && x < 12;
|
||||
| ^^^^^^^^^^^^^^^^ help: use: `(8..12).contains(&x)`
|
||||
|
|
||||
= note: `-D clippy::manual-range-contains` implied by `-D warnings`
|
||||
|
||||
error: manual `Range::contains` implementation
|
||||
--> $DIR/range_contains.rs:13:5
|
||||
|
|
||||
LL | x < 42 && x >= 21;
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `(21..42).contains(&x)`
|
||||
|
||||
error: manual `Range::contains` implementation
|
||||
--> $DIR/range_contains.rs:14:5
|
||||
|
|
||||
LL | 100 > x && 1 <= x;
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `(1..100).contains(&x)`
|
||||
|
||||
error: manual `RangeInclusive::contains` implementation
|
||||
--> $DIR/range_contains.rs:17:5
|
||||
|
|
||||
LL | x >= 9 && x <= 99;
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `(9..=99).contains(&x)`
|
||||
|
||||
error: manual `RangeInclusive::contains` implementation
|
||||
--> $DIR/range_contains.rs:18:5
|
||||
|
|
||||
LL | x <= 33 && x >= 1;
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `(1..=33).contains(&x)`
|
||||
|
||||
error: manual `RangeInclusive::contains` implementation
|
||||
--> $DIR/range_contains.rs:19:5
|
||||
|
|
||||
LL | 999 >= x && 1 <= x;
|
||||
| ^^^^^^^^^^^^^^^^^^ help: use: `(1..=999).contains(&x)`
|
||||
|
||||
error: manual `!Range::contains` implementation
|
||||
--> $DIR/range_contains.rs:22:5
|
||||
|
|
||||
LL | x < 8 || x >= 12;
|
||||
| ^^^^^^^^^^^^^^^^ help: use: `!(8..12).contains(&x)`
|
||||
|
||||
error: manual `!Range::contains` implementation
|
||||
--> $DIR/range_contains.rs:23:5
|
||||
|
|
||||
LL | x >= 42 || x < 21;
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `!(21..42).contains(&x)`
|
||||
|
||||
error: manual `!Range::contains` implementation
|
||||
--> $DIR/range_contains.rs:24:5
|
||||
|
|
||||
LL | 100 <= x || 1 > x;
|
||||
| ^^^^^^^^^^^^^^^^^ help: use: `!(1..100).contains(&x)`
|
||||
|
||||
error: manual `!RangeInclusive::contains` implementation
|
||||
--> $DIR/range_contains.rs:27:5
|
||||
|
|
||||
LL | x < 9 || x > 99;
|
||||
| ^^^^^^^^^^^^^^^ help: use: `!(9..=99).contains(&x)`
|
||||
|
||||
error: manual `!RangeInclusive::contains` implementation
|
||||
--> $DIR/range_contains.rs:28:5
|
||||
|
|
||||
LL | x > 33 || x < 1;
|
||||
| ^^^^^^^^^^^^^^^ help: use: `!(1..=33).contains(&x)`
|
||||
|
||||
error: manual `!RangeInclusive::contains` implementation
|
||||
--> $DIR/range_contains.rs:29:5
|
||||
|
|
||||
LL | 999 < x || 1 > x;
|
||||
| ^^^^^^^^^^^^^^^^ help: use: `!(1..=999).contains(&x)`
|
||||
|
||||
error: aborting due to 12 previous errors
|
||||
|
Loading…
Reference in a new issue