X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Franges.rs;h=c9de88d9c83d2e157d059a7cc9281bd6322afb12;hb=2ff568d746e4641b992c0b74bea046e43a637997;hp=cd4e27110b3cbecbe58268e22cafcca084b14418;hpb=f7f85a0dcac857d35b621a736226fe3c73251c98;p=rust.git diff --git a/clippy_lints/src/ranges.rs b/clippy_lints/src/ranges.rs index cd4e27110b3..c9de88d9c83 100644 --- a/clippy_lints/src/ranges.rs +++ b/clippy_lints/src/ranges.rs @@ -1,34 +1,14 @@ use if_chain::if_chain; -use rustc::hir::*; -use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; -use rustc::{declare_lint_pass, declare_tool_lint}; +use rustc_ast::ast::RangeLimits; use rustc_errors::Applicability; -use syntax::ast::RangeLimits; -use syntax::source_map::Spanned; +use rustc_hir::{BinOpKind, Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Spanned; use crate::utils::sugg::Sugg; -use crate::utils::{get_trait_def_id, higher, implements_trait, SpanlessEq}; -use crate::utils::{is_integer_literal, paths, snippet, snippet_opt, span_lint, span_lint_and_then}; - -declare_clippy_lint! { - /// **What it does:** Checks for calling `.step_by(0)` on iterators, - /// which never terminates. - /// - /// **Why is this bad?** This very much looks like an oversight, since with - /// `loop { .. }` there is an obvious better way to endlessly loop. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```ignore - /// for x in (5..5).step_by(0) { - /// .. - /// } - /// ``` - pub ITERATOR_STEP_BY_ZERO, - correctness, - "using `Iterator::step_by(0)`, which produces an infinite iterator" -} +use crate::utils::{higher, SpanlessEq}; +use crate::utils::{is_integer_const, snippet, snippet_opt, span_lint, span_lint_and_then}; declare_clippy_lint! { /// **What it does:** Checks for zipping a collection with the range of @@ -40,7 +20,13 @@ /// /// **Example:** /// ```rust - /// x.iter().zip(0..x.len()) + /// # let x = vec![1]; + /// x.iter().zip(0..x.len()); + /// ``` + /// Could be written as + /// ```rust + /// # let x = vec![1]; + /// x.iter().enumerate(); /// ``` pub RANGE_ZIP_WITH_LEN, complexity, @@ -59,12 +45,20 @@ /// and ends with a closing one. /// I.e., `let _ = (f()+1)..(f()+1)` results in `let _ = ((f()+1)..=f())`. /// + /// Also in many cases, inclusive ranges are still slower to run than + /// exclusive ranges, because they essentially add an extra branch that + /// LLVM may fail to hoist out of the loop. + /// /// **Example:** - /// ```rust + /// ```rust,ignore /// for x..(y+1) { .. } /// ``` + /// Could be written as + /// ```rust,ignore + /// for x..=y { .. } + /// ``` pub RANGE_PLUS_ONE, - complexity, + pedantic, "`x..(y+1)` reads better as `x..=y`" } @@ -78,39 +72,30 @@ /// **Known problems:** None. /// /// **Example:** - /// ```rust + /// ```rust,ignore /// for x..=(y-1) { .. } /// ``` + /// Could be written as + /// ```rust,ignore + /// for x..y { .. } + /// ``` pub RANGE_MINUS_ONE, complexity, "`x..=(y-1)` reads better as `x..y`" } declare_lint_pass!(Ranges => [ - ITERATOR_STEP_BY_ZERO, RANGE_ZIP_WITH_LEN, RANGE_PLUS_ONE, RANGE_MINUS_ONE ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Ranges { - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if let ExprKind::MethodCall(ref path, _, ref args) = expr.node { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { + if let ExprKind::MethodCall(ref path, _, ref args) = expr.kind { let name = path.ident.as_str(); - - // Range with step_by(0). - if name == "step_by" && args.len() == 2 && has_step_by(cx, &args[0]) { - use crate::consts::{constant, Constant}; - if let Some((Constant::Int(0), _)) = constant(cx, cx.tables, &args[1]) { - span_lint( - cx, - ITERATOR_STEP_BY_ZERO, - expr.span, - "Iterator::step_by(0) will panic at runtime", - ); - } - } else if name == "zip" && args.len() == 2 { - let iter = &args[0].node; + if name == "zip" && args.len() == 2 { + let iter = &args[0].kind; let zip_arg = &args[1]; if_chain! { // `.iter()` call @@ -118,106 +103,107 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { 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(cx, zip_arg); - if is_integer_literal(start, 0); + if is_integer_const(cx, start, 0); // `.len()` call - if let ExprKind::MethodCall(ref len_path, _, ref len_args) = end.node; + 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].node; - if let ExprKind::Path(QPath::Resolved(_, ref len_path)) = len_args[0].node; + 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()", + &format!("It is more idiomatic to use `{}.iter().enumerate()`", snippet(cx, iter_args[0].span, "_"))); } } } } - // exclusive range plus one: `x..(y+1)` - if_chain! { - if let Some(higher::Range { - start, - end: Some(end), - limits: RangeLimits::HalfOpen - }) = higher::range(cx, expr); - if let Some(y) = y_plus_one(end); - then { - let span = expr.span + check_exclusive_range_plus_one(cx, expr); + check_inclusive_range_minus_one(cx, expr); + } +} + +// exclusive range plus one: `x..(y+1)` +fn check_exclusive_range_plus_one(cx: &LateContext<'_, '_>, expr: &Expr<'_>) { + if_chain! { + if let Some(higher::Range { + start, + end: Some(end), + limits: RangeLimits::HalfOpen + }) = higher::range(cx, expr); + if let Some(y) = y_plus_one(cx, end); + then { + let span = if expr.span.from_expansion() { + expr.span .ctxt() - .outer() - .expn_info() - .map_or(expr.span, |info| info.call_site); - span_lint_and_then( - cx, - RANGE_PLUS_ONE, - span, - "an inclusive range would be more readable", - |db| { - let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string()); - let end = Sugg::hir(cx, y, "y"); - if let Some(is_wrapped) = &snippet_opt(cx, span) { - if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') { - db.span_suggestion( - span, - "use", - format!("({}..={})", start, end), - Applicability::MaybeIncorrect, - ); - } else { - db.span_suggestion( - span, - "use", - format!("{}..={}", start, end), - Applicability::MachineApplicable, // snippet - ); - } + .outer_expn_data() + .call_site + } else { + expr.span + }; + span_lint_and_then( + cx, + RANGE_PLUS_ONE, + span, + "an inclusive range would be more readable", + |db| { + let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string()); + let end = Sugg::hir(cx, y, "y"); + if let Some(is_wrapped) = &snippet_opt(cx, span) { + if is_wrapped.starts_with('(') && is_wrapped.ends_with(')') { + db.span_suggestion( + span, + "use", + format!("({}..={})", start, end), + Applicability::MaybeIncorrect, + ); + } else { + db.span_suggestion( + span, + "use", + format!("{}..={}", start, end), + Applicability::MachineApplicable, // snippet + ); } - }, - ); - } - } - - // inclusive range minus one: `x..=(y-1)` - if_chain! { - if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr); - if let Some(y) = y_minus_one(end); - then { - span_lint_and_then( - cx, - RANGE_MINUS_ONE, - expr.span, - "an exclusive range would be more readable", - |db| { - let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string()); - let end = Sugg::hir(cx, y, "y"); - db.span_suggestion( - expr.span, - "use", - format!("{}..{}", start, end), - Applicability::MachineApplicable, // snippet - ); - }, - ); - } + } + }, + ); } } } -fn has_step_by(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { - // No need for `walk_ptrs_ty` here because `step_by` moves `self`, so it - // can't be called on a borrowed range. - let ty = cx.tables.expr_ty_adjusted(expr); - - get_trait_def_id(cx, &paths::ITERATOR) - .map_or(false, |iterator_trait| implements_trait(cx, ty, iterator_trait, &[])) +// inclusive range minus one: `x..=(y-1)` +fn check_inclusive_range_minus_one(cx: &LateContext<'_, '_>, expr: &Expr<'_>) { + if_chain! { + if let Some(higher::Range { start, end: Some(end), limits: RangeLimits::Closed }) = higher::range(cx, expr); + if let Some(y) = y_minus_one(cx, end); + then { + span_lint_and_then( + cx, + RANGE_MINUS_ONE, + expr.span, + "an exclusive range would be more readable", + |db| { + let start = start.map_or(String::new(), |x| Sugg::hir(cx, x, "x").to_string()); + let end = Sugg::hir(cx, y, "y"); + db.span_suggestion( + expr.span, + "use", + format!("{}..{}", start, end), + Applicability::MachineApplicable, // snippet + ); + }, + ); + } + } } -fn y_plus_one(expr: &Expr) -> Option<&Expr> { - match expr.node { +fn y_plus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> { + match expr.kind { ExprKind::Binary( Spanned { node: BinOpKind::Add, .. @@ -225,9 +211,9 @@ fn y_plus_one(expr: &Expr) -> Option<&Expr> { ref lhs, ref rhs, ) => { - if is_integer_literal(lhs, 1) { + if is_integer_const(cx, lhs, 1) { Some(rhs) - } else if is_integer_literal(rhs, 1) { + } else if is_integer_const(cx, rhs, 1) { Some(lhs) } else { None @@ -237,15 +223,15 @@ fn y_plus_one(expr: &Expr) -> Option<&Expr> { } } -fn y_minus_one(expr: &Expr) -> Option<&Expr> { - match expr.node { +fn y_minus_one<'t>(cx: &LateContext<'_, '_>, expr: &'t Expr<'_>) -> Option<&'t Expr<'t>> { + match expr.kind { ExprKind::Binary( Spanned { node: BinOpKind::Sub, .. }, ref lhs, ref rhs, - ) if is_integer_literal(rhs, 1) => Some(lhs), + ) if is_integer_const(cx, rhs, 1) => Some(lhs), _ => None, } }