diff --git a/CHANGELOG.md b/CHANGELOG.md index 23f4f97ee07..da03f5c012b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5160,6 +5160,7 @@ Released 2018-09-13 [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop [`while_let_on_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_on_iterator +[`while_pop_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_pop_unwrap [`wildcard_dependencies`]: https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_dependencies [`wildcard_enum_match_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_enum_match_arm [`wildcard_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_imports diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 4aebd0b7d01..ddc4b139670 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -645,6 +645,7 @@ crate::useless_conversion::USELESS_CONVERSION_INFO, crate::vec::USELESS_VEC_INFO, crate::vec_init_then_push::VEC_INIT_THEN_PUSH_INFO, + crate::while_pop_unwrap::WHILE_POP_UNWRAP_INFO, crate::wildcard_imports::ENUM_GLOB_USE_INFO, crate::wildcard_imports::WILDCARD_IMPORTS_INFO, crate::write::PRINTLN_EMPTY_STRING_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 48dbecc9f6a..0d594612b79 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -324,6 +324,7 @@ mod useless_conversion; mod vec; mod vec_init_then_push; +mod while_pop_unwrap; mod wildcard_imports; mod write; mod zero_div_zero; diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 610a0233eee..90012694062 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -25,6 +25,8 @@ use rustc_span::source_map::Span; use utils::{make_iterator_snippet, IncrementVisitor, InitializeVisitor}; +use crate::while_pop_unwrap; + declare_clippy_lint! { /// ### What it does /// Checks for for-loops that manually copy items between @@ -643,6 +645,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if let Some(higher::While { condition, body }) = higher::While::hir(expr) { while_immutable_condition::check(cx, condition, body); missing_spin_loop::check(cx, condition, body); + while_pop_unwrap::check(cx, condition, body); } } } diff --git a/clippy_lints/src/while_pop_unwrap.rs b/clippy_lints/src/while_pop_unwrap.rs new file mode 100644 index 00000000000..fc77febad6b --- /dev/null +++ b/clippy_lints/src/while_pop_unwrap.rs @@ -0,0 +1,126 @@ +use clippy_utils::{diagnostics::span_lint_and_then, match_def_path, paths, source::snippet}; +use rustc_errors::Applicability; +use rustc_hir::*; +use rustc_lint::LateContext; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{symbol::Ident, Span}; + +declare_clippy_lint! { + /// ### What it does + /// Looks for loops that check for emptiness of a `Vec` in the condition and pop an element + /// in the body as a separate operation. + /// + /// ### Why is this bad? + /// Such loops can be written in a more idiomatic way by using a while..let loop and directly + /// pattern matching on the return value of `Vec::pop()`. + /// + /// ### Example + /// ```rust + /// let mut numbers = vec![1, 2, 3, 4, 5]; + /// while !numbers.is_empty() { + /// let number = numbers.pop().unwrap(); + /// // use `number` + /// } + /// ``` + /// Use instead: + /// ```rust + /// let mut numbers = vec![1, 2, 3, 4, 5]; + /// while let Some(number) = numbers.pop() { + /// // use `number` + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub WHILE_POP_UNWRAP, + style, + "checking for emptiness of a `Vec` in the loop condition and popping an element in the body" +} +declare_lint_pass!(WhilePopUnwrap => [WHILE_POP_UNWRAP]); + +fn report_lint<'tcx>( + cx: &LateContext<'tcx>, + pop_span: Span, + ident: Option, + loop_span: Span, + receiver_span: Span, +) { + span_lint_and_then( + cx, + WHILE_POP_UNWRAP, + pop_span, + "you seem to be trying to pop elements from a `Vec` in a loop", + |diag| { + diag.span_suggestion( + loop_span, + "try", + format!( + "while let Some({}) = {}.pop()", + ident.as_ref().map(Ident::as_str).unwrap_or("element"), + snippet(cx, receiver_span, "..") + ), + Applicability::MaybeIncorrect, + ) + .note("this while loop can be written in a more idiomatic way"); + }, + ); +} + +fn match_method_call(cx: &LateContext<'_>, expr: &Expr<'_>, method: &[&str]) -> bool { + if let ExprKind::MethodCall(..) = expr.kind + && let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && match_def_path(cx, id, method) + { + true + } else { + false + } +} + +fn is_vec_pop<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> bool { + match_method_call(cx, expr, &paths::VEC_POP) +} + +fn is_vec_pop_unwrap<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> bool { + if let ExprKind::MethodCall(_, inner, ..) = expr.kind + && (match_method_call(cx, expr, &paths::OPTION_UNWRAP) || match_method_call(cx, expr, &paths::OPTION_EXPECT)) + && is_vec_pop(cx, inner) + { + true + } else { + false + } +} + +fn check_local<'tcx>(cx: &LateContext<'tcx>, stmt: &Stmt<'_>, loop_span: Span, recv_span: Span) { + if let StmtKind::Local(local) = stmt.kind + && let PatKind::Binding(.., ident, _) = local.pat.kind + && let Some(init) = local.init + && let ExprKind::MethodCall(_, inner, ..) = init.kind + && is_vec_pop_unwrap(cx, init) + { + report_lint(cx, init.span.to(inner.span), Some(ident), loop_span, recv_span); + } +} + +fn check_call_arguments<'tcx>(cx: &LateContext<'tcx>, stmt: &Stmt<'_>, loop_span: Span, recv_span: Span) { + if let StmtKind::Semi(expr) | StmtKind::Expr(expr) = stmt.kind { + if let ExprKind::MethodCall(_, _, args, _) | ExprKind::Call(_, args) = expr.kind { + let offending_arg = args.iter().find_map(|arg| is_vec_pop_unwrap(cx, arg).then(|| arg.span)); + + if let Some(offending_arg) = offending_arg { + report_lint(cx, offending_arg, None, loop_span, recv_span); + } + } + } +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>) { + if let ExprKind::Unary(UnOp::Not, cond) = cond.kind + && let ExprKind::MethodCall(_, Expr { span: recv_span, .. }, _, _) = cond.kind + && match_method_call(cx, cond, &paths::VEC_IS_EMPTY) + && let ExprKind::Block(body, _) = body.kind + && let Some(stmt) = body.stmts.first() + { + check_local(cx, stmt, cond.span, *recv_span); + check_call_arguments(cx, stmt, cond.span, *recv_span); + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 9be2d0eae80..0f0792fdaa9 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -159,3 +159,7 @@ pub const PTR_NON_NULL: [&str; 4] = ["core", "ptr", "non_null", "NonNull"]; pub const INSTANT_NOW: [&str; 4] = ["std", "time", "Instant", "now"]; pub const INSTANT: [&str; 3] = ["std", "time", "Instant"]; +pub const VEC_IS_EMPTY: [&str; 4] = ["alloc", "vec", "Vec", "is_empty"]; +pub const VEC_POP: [&str; 4] = ["alloc", "vec", "Vec", "pop"]; +pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"]; +pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"]; diff --git a/tests/ui/while_pop_unwrap.rs b/tests/ui/while_pop_unwrap.rs new file mode 100644 index 00000000000..1b6de8f1f89 --- /dev/null +++ b/tests/ui/while_pop_unwrap.rs @@ -0,0 +1,47 @@ +#![allow(unused)] +#![warn(clippy::while_pop_unwrap)] + +struct VecInStruct { + numbers: Vec, + unrelated: String, +} + +fn accept_i32(_: i32) {} + +fn main() { + let mut numbers = vec![1, 2, 3, 4, 5]; + while !numbers.is_empty() { + let number = numbers.pop().unwrap(); + } + + let mut val = VecInStruct { + numbers: vec![1, 2, 3, 4, 5], + unrelated: String::new(), + }; + while !val.numbers.is_empty() { + let number = val.numbers.pop().unwrap(); + } + + while !numbers.is_empty() { + accept_i32(numbers.pop().unwrap()); + } + + while !numbers.is_empty() { + accept_i32(numbers.pop().expect("")); + } + + // This should not warn. It "conditionally" pops elements. + while !numbers.is_empty() { + if true { + accept_i32(numbers.pop().unwrap()); + } + } + + // This should also not warn. It conditionally pops elements. + while !numbers.is_empty() { + if false { + continue; + } + accept_i32(numbers.pop().unwrap()); + } +} diff --git a/tests/ui/while_pop_unwrap.stderr b/tests/ui/while_pop_unwrap.stderr new file mode 100644 index 00000000000..8dc77db8b5a --- /dev/null +++ b/tests/ui/while_pop_unwrap.stderr @@ -0,0 +1,43 @@ +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:14:22 + | +LL | while !numbers.is_empty() { + | ------------------ help: try: `while let Some(number) = numbers.pop()` +LL | let number = numbers.pop().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this while loop can be written in a more idiomatic way + = note: `-D clippy::while-pop-unwrap` implied by `-D warnings` + +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:22:22 + | +LL | while !val.numbers.is_empty() { + | ---------------------- help: try: `while let Some(number) = val.numbers.pop()` +LL | let number = val.numbers.pop().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this while loop can be written in a more idiomatic way + +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:26:20 + | +LL | while !numbers.is_empty() { + | ------------------ help: try: `while let Some(element) = numbers.pop()` +LL | accept_i32(numbers.pop().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this while loop can be written in a more idiomatic way + +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:30:20 + | +LL | while !numbers.is_empty() { + | ------------------ help: try: `while let Some(element) = numbers.pop()` +LL | accept_i32(numbers.pop().expect("")); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this while loop can be written in a more idiomatic way + +error: aborting due to 4 previous errors +