Skip to content

Commit 1d08325

Browse files
committed
move lint to loops, emit proper suggestion, more tests
1 parent bcdcc34 commit 1d08325

File tree

10 files changed

+351
-150
lines changed

10 files changed

+351
-150
lines changed

clippy_lints/src/declared_lints.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
258258
crate::loops::WHILE_IMMUTABLE_CONDITION_INFO,
259259
crate::loops::WHILE_LET_LOOP_INFO,
260260
crate::loops::WHILE_LET_ON_ITERATOR_INFO,
261+
crate::loops::WHILE_POP_UNWRAP_INFO,
261262
crate::macro_use::MACRO_USE_IMPORTS_INFO,
262263
crate::main_recursion::MAIN_RECURSION_INFO,
263264
crate::manual_assert::MANUAL_ASSERT_INFO,
@@ -645,7 +646,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
645646
crate::useless_conversion::USELESS_CONVERSION_INFO,
646647
crate::vec::USELESS_VEC_INFO,
647648
crate::vec_init_then_push::VEC_INIT_THEN_PUSH_INFO,
648-
crate::while_pop_unwrap::WHILE_POP_UNWRAP_INFO,
649649
crate::wildcard_imports::ENUM_GLOB_USE_INFO,
650650
crate::wildcard_imports::WILDCARD_IMPORTS_INFO,
651651
crate::write::PRINTLN_EMPTY_STRING_INFO,

clippy_lints/src/lib.rs

-1
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,6 @@ mod use_self;
324324
mod useless_conversion;
325325
mod vec;
326326
mod vec_init_then_push;
327-
mod while_pop_unwrap;
328327
mod wildcard_imports;
329328
mod write;
330329
mod zero_div_zero;

clippy_lints/src/loops/mod.rs

+34-4
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ mod utils;
1717
mod while_immutable_condition;
1818
mod while_let_loop;
1919
mod while_let_on_iterator;
20+
mod while_pop_unwrap;
2021

2122
use clippy_utils::higher;
2223
use rustc_hir::{Expr, ExprKind, LoopSource, Pat};
@@ -25,8 +26,6 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
2526
use rustc_span::source_map::Span;
2627
use utils::{make_iterator_snippet, IncrementVisitor, InitializeVisitor};
2728

28-
use crate::while_pop_unwrap;
29-
3029
declare_clippy_lint! {
3130
/// ### What it does
3231
/// Checks for for-loops that manually copy items between
@@ -577,6 +576,36 @@ declare_clippy_lint! {
577576
"manual implementation of `Iterator::find`"
578577
}
579578

579+
declare_clippy_lint! {
580+
/// ### What it does
581+
/// Looks for loops that check for emptiness of a `Vec` in the condition and pop an element
582+
/// in the body as a separate operation.
583+
///
584+
/// ### Why is this bad?
585+
/// Such loops can be written in a more idiomatic way by using a while..let loop and directly
586+
/// pattern matching on the return value of `Vec::pop()`.
587+
///
588+
/// ### Example
589+
/// ```rust
590+
/// let mut numbers = vec![1, 2, 3, 4, 5];
591+
/// while !numbers.is_empty() {
592+
/// let number = numbers.pop().unwrap();
593+
/// // use `number`
594+
/// }
595+
/// ```
596+
/// Use instead:
597+
/// ```rust
598+
/// let mut numbers = vec![1, 2, 3, 4, 5];
599+
/// while let Some(number) = numbers.pop() {
600+
/// // use `number`
601+
/// }
602+
/// ```
603+
#[clippy::version = "1.70.0"]
604+
pub WHILE_POP_UNWRAP,
605+
style,
606+
"checking for emptiness of a `Vec` in the loop condition and popping an element in the body"
607+
}
608+
580609
declare_lint_pass!(Loops => [
581610
MANUAL_MEMCPY,
582611
MANUAL_FLATTEN,
@@ -596,6 +625,7 @@ declare_lint_pass!(Loops => [
596625
SINGLE_ELEMENT_LOOP,
597626
MISSING_SPIN_LOOP,
598627
MANUAL_FIND,
628+
WHILE_POP_UNWRAP
599629
]);
600630

601631
impl<'tcx> LateLintPass<'tcx> for Loops {
@@ -642,10 +672,10 @@ impl<'tcx> LateLintPass<'tcx> for Loops {
642672

643673
while_let_on_iterator::check(cx, expr);
644674

645-
if let Some(higher::While { condition, body }) = higher::While::hir(expr) {
675+
if let Some(higher::While { condition, body, span }) = higher::While::hir(expr) {
646676
while_immutable_condition::check(cx, condition, body);
647677
missing_spin_loop::check(cx, condition, body);
648-
while_pop_unwrap::check(cx, condition, body);
678+
while_pop_unwrap::check(cx, condition, body, span);
649679
}
650680
}
651681
}
+109
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
use clippy_utils::{
2+
diagnostics::{multispan_sugg_with_applicability, span_lint_and_then},
3+
match_def_path, paths,
4+
source::snippet,
5+
SpanlessEq,
6+
};
7+
use rustc_errors::Applicability;
8+
use rustc_hir::{Expr, ExprKind, Pat, Stmt, StmtKind, UnOp};
9+
use rustc_lint::LateContext;
10+
use rustc_span::Span;
11+
use std::borrow::Cow;
12+
13+
use super::WHILE_POP_UNWRAP;
14+
15+
/// The kind of statement that the `pop()` call appeared in.
16+
///
17+
/// Depending on whether the value was assigned to a variable or not changes what pattern
18+
/// we use for the suggestion.
19+
enum PopStmt<'hir> {
20+
/// `x.pop().unwrap()` was and assigned to a variable.
21+
/// The pattern of this local variable will be used and the local statement
22+
/// is deleted in the suggestion.
23+
Local(&'hir Pat<'hir>),
24+
/// `x.pop().unwrap()` appeared in an arbitrary expression and was not assigned to a variable.
25+
/// The suggestion will use some placeholder identifier and the `x.pop().unwrap()` expression
26+
/// is replaced with that identifier.
27+
Anonymous,
28+
}
29+
30+
fn report_lint(cx: &LateContext<'_>, pop_span: Span, pop_stmt_kind: PopStmt<'_>, loop_span: Span, receiver_span: Span) {
31+
span_lint_and_then(
32+
cx,
33+
WHILE_POP_UNWRAP,
34+
pop_span,
35+
"you seem to be trying to pop elements from a `Vec` in a loop",
36+
|diag| {
37+
let (pat, pop_replacement) = match &pop_stmt_kind {
38+
PopStmt::Local(pat) => (snippet(cx, pat.span, ".."), String::new()),
39+
PopStmt::Anonymous => (Cow::Borrowed("element"), "element".into()),
40+
};
41+
42+
let loop_replacement = format!("while let Some({}) = {}.pop()", pat, snippet(cx, receiver_span, ".."));
43+
multispan_sugg_with_applicability(
44+
diag,
45+
"consider using a `while..let` loop",
46+
Applicability::MachineApplicable,
47+
[(loop_span, loop_replacement), (pop_span, pop_replacement)],
48+
);
49+
},
50+
);
51+
}
52+
53+
fn match_method_call(cx: &LateContext<'_>, expr: &Expr<'_>, method: &[&str]) -> bool {
54+
if let ExprKind::MethodCall(..) = expr.kind
55+
&& let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
56+
{
57+
match_def_path(cx, id, method)
58+
} else {
59+
false
60+
}
61+
}
62+
63+
fn is_vec_pop_unwrap(cx: &LateContext<'_>, expr: &Expr<'_>, is_empty_recv: &Expr<'_>) -> bool {
64+
if (match_method_call(cx, expr, &paths::OPTION_UNWRAP) || match_method_call(cx, expr, &paths::OPTION_EXPECT))
65+
&& let ExprKind::MethodCall(_, unwrap_recv, ..) = expr.kind
66+
&& match_method_call(cx, unwrap_recv, &paths::VEC_POP)
67+
&& let ExprKind::MethodCall(_, pop_recv, ..) = unwrap_recv.kind
68+
{
69+
// make sure they're the same `Vec`
70+
SpanlessEq::new(cx).eq_expr(pop_recv, is_empty_recv)
71+
} else {
72+
false
73+
}
74+
}
75+
76+
fn check_local(cx: &LateContext<'_>, stmt: &Stmt<'_>, is_empty_recv: &Expr<'_>, loop_span: Span) {
77+
if let StmtKind::Local(local) = stmt.kind
78+
&& let Some(init) = local.init
79+
&& is_vec_pop_unwrap(cx, init, is_empty_recv)
80+
{
81+
report_lint(cx, stmt.span, PopStmt::Local(&local.pat), loop_span, is_empty_recv.span);
82+
}
83+
}
84+
85+
fn check_call_arguments(cx: &LateContext<'_>, stmt: &Stmt<'_>, is_empty_recv: &Expr<'_>, loop_span: Span) {
86+
if let StmtKind::Semi(expr) | StmtKind::Expr(expr) = stmt.kind {
87+
if let ExprKind::MethodCall(.., args, _) | ExprKind::Call(_, args) = expr.kind {
88+
let offending_arg = args
89+
.iter()
90+
.find_map(|arg| is_vec_pop_unwrap(cx, arg, is_empty_recv).then_some(arg.span));
91+
92+
if let Some(offending_arg) = offending_arg {
93+
report_lint(cx, offending_arg, PopStmt::Anonymous, loop_span, is_empty_recv.span);
94+
}
95+
}
96+
}
97+
}
98+
99+
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, full_cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>, loop_span: Span) {
100+
if let ExprKind::Unary(UnOp::Not, cond) = full_cond.kind
101+
&& let ExprKind::MethodCall(_, is_empty_recv, _, _) = cond.kind
102+
&& match_method_call(cx, cond, &paths::VEC_IS_EMPTY)
103+
&& let ExprKind::Block(body, _) = body.kind
104+
&& let Some(stmt) = body.stmts.first()
105+
{
106+
check_local(cx, stmt, is_empty_recv, loop_span);
107+
check_call_arguments(cx, stmt, is_empty_recv, loop_span);
108+
}
109+
}

clippy_lints/src/utils/author.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
333333

334334
#[allow(clippy::too_many_lines)]
335335
fn expr(&self, expr: &Binding<&hir::Expr<'_>>) {
336-
if let Some(higher::While { condition, body }) = higher::While::hir(expr.value) {
336+
if let Some(higher::While { condition, body, .. }) = higher::While::hir(expr.value) {
337337
bind!(self, condition, body);
338338
chain!(
339339
self,

clippy_lints/src/while_pop_unwrap.rs

-122
This file was deleted.

clippy_utils/src/higher.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,8 @@ pub struct While<'hir> {
311311
pub condition: &'hir Expr<'hir>,
312312
/// `while` loop body
313313
pub body: &'hir Expr<'hir>,
314+
/// Span of the loop header
315+
pub span: Span,
314316
}
315317

316318
impl<'hir> While<'hir> {
@@ -336,10 +338,10 @@ impl<'hir> While<'hir> {
336338
},
337339
_,
338340
LoopSource::While,
339-
_,
341+
span,
340342
) = expr.kind
341343
{
342-
return Some(Self { condition, body });
344+
return Some(Self { condition, body, span });
343345
}
344346
None
345347
}

0 commit comments

Comments
 (0)