Skip to content

Commit f62c1e8

Browse files
committed
fix: needless_continue wrongly unmangled macros
1 parent c86f51f commit f62c1e8

File tree

3 files changed

+141
-29
lines changed

3 files changed

+141
-29
lines changed

clippy_lints/src/needless_continue.rs

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use clippy_utils::diagnostics::span_lint_and_help;
2-
use clippy_utils::source::{indent_of, snippet, snippet_block, snippet_with_context};
2+
use clippy_utils::macros::root_macro_call;
3+
use clippy_utils::source::{indent_of, snippet_block, snippet_with_context};
34
use clippy_utils::{higher, is_from_proc_macro};
45
use rustc_ast::Label;
56
use rustc_errors::Applicability;
@@ -132,7 +133,12 @@ declare_lint_pass!(NeedlessContinue => [NEEDLESS_CONTINUE]);
132133

133134
impl<'tcx> LateLintPass<'tcx> for NeedlessContinue {
134135
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
135-
if !expr.span.in_external_macro(cx.sess().source_map()) && !is_from_proc_macro(cx, expr) {
136+
// We cannot use `from_expansion` because for loops, while loops and while let loops are desugared
137+
// into `loop` expressions.
138+
if !expr.span.in_external_macro(cx.sess().source_map())
139+
&& !is_from_proc_macro(cx, expr)
140+
&& root_macro_call(expr.span).is_none()
141+
{
136142
check_and_warn(cx, expr);
137143
}
138144
}
@@ -193,15 +199,15 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessContinue {
193199
/// - The expression node is a block with the first statement being a `continue`.
194200
fn needless_continue_in_else(else_expr: &Expr<'_>, label: Option<&Label>) -> bool {
195201
match else_expr.kind {
196-
ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label),
202+
ExprKind::Block(else_block, _) => is_first_block_stmt_continue(else_block, label),
197203
ExprKind::Continue(l) => compare_labels(label, l.label.as_ref()),
198204
_ => false,
199205
}
200206
}
201207

202208
fn is_first_block_stmt_continue(block: &Block<'_>, label: Option<&Label>) -> bool {
203209
block.stmts.first().is_some_and(|stmt| match stmt.kind {
204-
StmtKind::Semi(ref e) | StmtKind::Expr(ref e) => {
210+
StmtKind::Semi(e) | StmtKind::Expr(e) => {
205211
if let ExprKind::Continue(ref l) = e.kind {
206212
compare_labels(label, l.label.as_ref())
207213
} else {
@@ -225,10 +231,10 @@ fn compare_labels(loop_label: Option<&Label>, continue_label: Option<&Label>) ->
225231
}
226232

227233
/// If `expr` is a loop expression (while/while let/for/loop), calls `func` with
228-
/// the AST object representing the loop block of `expr`.
229-
fn with_loop_block<'tcx, F>(expr: &Expr<'tcx>, mut func: F)
234+
/// the HIR object representing the loop block of `expr`.
235+
fn with_loop_block<F>(expr: &Expr<'_>, mut func: F)
230236
where
231-
F: FnMut(&Block<'tcx>, Option<&Label>),
237+
F: FnMut(&Block<'_>, Option<&Label>),
232238
{
233239
if let Some(higher::ForLoop { body, label, .. }) = higher::ForLoop::hir(expr)
234240
&& let ExprKind::Block(block, _) = &body.kind
@@ -264,9 +270,9 @@ where
264270
/// - The `if` condition expression,
265271
/// - The `then` block, and
266272
/// - The `else` expression.
267-
fn with_if_expr<'tcx, F>(expr: &Expr<'tcx>, mut func: F)
273+
fn with_if_expr<F>(expr: &Expr<'_>, mut func: F)
268274
where
269-
F: FnMut(&Expr<'tcx>, &Expr<'tcx>, &Block<'tcx>, &Expr<'tcx>),
275+
F: FnMut(&Expr<'_>, &Expr<'_>, &Block<'_>, &Expr<'_>),
270276
{
271277
if let Some(higher::If {
272278
cond,
@@ -288,20 +294,20 @@ enum LintType {
288294

289295
/// Data we pass around for construction of help messages.
290296
#[derive(Debug)]
291-
struct LintData<'tcx> {
297+
struct LintData<'hir> {
292298
/// The `if` expression encountered in the above loop.
293-
if_expr: &'tcx Expr<'tcx>,
299+
if_expr: &'hir Expr<'hir>,
294300
/// The condition expression for the above `if`.
295-
if_cond: &'tcx Expr<'tcx>,
301+
if_cond: &'hir Expr<'hir>,
296302
/// The `then` block of the `if` statement.
297-
if_block: &'tcx Block<'tcx>,
303+
if_block: &'hir Block<'hir>,
298304
/// The `else` block of the `if` statement.
299305
/// Note that we only work with `if` exprs that have an `else` branch.
300-
else_expr: &'tcx Expr<'tcx>,
306+
else_expr: &'hir Expr<'hir>,
301307
/// The 0-based index of the `if` statement in the containing loop block.
302308
stmt_idx: Option<usize>,
303309
/// The statements of the loop block.
304-
loop_block: &'tcx Block<'tcx>,
310+
loop_block: &'hir Block<'hir>,
305311
}
306312

307313
const MSG_REDUNDANT_CONTINUE_EXPRESSION: &str = "this `continue` expression is redundant";
@@ -366,7 +372,14 @@ fn suggestion_snippet_for_continue_inside_if(cx: &LateContext<'_>, data: &LintDa
366372
}
367373

368374
fn suggestion_snippet_for_continue_inside_else(cx: &LateContext<'_>, data: &LintData<'_>) -> String {
369-
let cond_code = snippet(cx, data.if_cond.span, "..");
375+
let mut applicability = Applicability::MachineApplicable;
376+
let (cond_code, _) = snippet_with_context(
377+
cx,
378+
data.if_cond.span,
379+
data.if_expr.span.ctxt(),
380+
"..",
381+
&mut applicability,
382+
);
370383

371384
// Region B
372385
let block_code = erode_from_back(&snippet_block(cx, data.if_block.span, "..", Some(data.if_expr.span)));
@@ -402,7 +415,7 @@ fn suggestion_snippet_for_continue_inside_else(cx: &LateContext<'_>, data: &Lint
402415
}
403416
lines.join("\n")
404417
} else {
405-
"".to_string()
418+
String::new()
406419
};
407420

408421
let indent_if = indent_of(cx, data.if_expr.span).unwrap_or(0);
@@ -417,7 +430,7 @@ fn check_last_stmt_in_expr<F>(cx: &LateContext<'_>, inner_expr: &Expr<'_>, func:
417430
where
418431
F: Fn(Option<&Label>, Span),
419432
{
420-
match &inner_expr.kind {
433+
match inner_expr.kind {
421434
ExprKind::Continue(continue_label) => {
422435
func(continue_label.label.as_ref(), inner_expr.span);
423436
},
@@ -432,7 +445,7 @@ where
432445
if !match_ty.is_unit() && !match_ty.is_never() {
433446
return;
434447
}
435-
for arm in arms.iter() {
448+
for arm in arms {
436449
check_last_stmt_in_expr(cx, arm.body, func);
437450
}
438451
},

tests/ui/needless_continue.rs

Lines changed: 92 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -246,16 +246,99 @@ mod issue_4077 {
246246
}
247247

248248
#[allow(clippy::let_unit_value)]
249-
fn issue14550(mut producer: impl Iterator<Item = Result<i32, u32>>) -> Result<u32, u32> {
250-
let mut counter = 2;
251-
loop {
252-
match producer.next().unwrap() {
253-
Ok(ok) => break Ok((ok + 1) as u32),
254-
Err(12) => {
255-
counter -= 1;
249+
mod issue14550 {
250+
fn match_with_value(mut producer: impl Iterator<Item = Result<i32, u32>>) -> Result<u32, u32> {
251+
let mut counter = 2;
252+
loop {
253+
match producer.next().unwrap() {
254+
Ok(ok) => break Ok((ok + 1) as u32),
255+
Err(12) => {
256+
counter -= 1;
257+
continue;
258+
},
259+
err => err?,
260+
};
261+
}
262+
}
263+
264+
fn inside_macro() {
265+
macro_rules! mac {
266+
($e:expr => $($rest:tt);*) => {
267+
loop {
268+
match $e {
269+
1 => continue,
270+
2 => break,
271+
n => println!("{n}"),
272+
}
273+
$($rest;)*
274+
}
275+
};
276+
}
277+
278+
mac!(2 => );
279+
mac!(1 => {println!("foobar")});
280+
}
281+
282+
mod partially_inside_macro {
283+
macro_rules! select {
284+
(
285+
$expr:expr,
286+
$( $pat:pat => $then:expr ),*
287+
) => {
288+
fn foo() {
289+
loop {
290+
match $expr {
291+
$(
292+
$pat => $then,
293+
)*
294+
}
295+
}
296+
}
297+
};
298+
}
299+
300+
select!(Some(1),
301+
Some(1) => {
302+
println!("one");
256303
continue;
257304
},
258-
err => err?,
259-
};
305+
Some(2) => {},
306+
None => break,
307+
_ => ()
308+
);
309+
310+
macro_rules! choose {
311+
(
312+
$expr:expr,
313+
$case:expr
314+
) => {
315+
fn bar() {
316+
loop {
317+
match $expr {
318+
$case => {
319+
println!("matched");
320+
continue;
321+
},
322+
_ => {
323+
println!("not matched");
324+
break;
325+
},
326+
}
327+
}
328+
}
329+
};
330+
}
331+
332+
choose!(todo!(), 5);
333+
}
334+
}
335+
336+
fn issue15548() {
337+
loop {
338+
if todo!() {
339+
} else {
340+
//~^ needless_continue
341+
continue;
342+
}
260343
}
261344
}

tests/ui/needless_continue.stderr

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,5 +220,21 @@ LL | | }
220220
do_something();
221221
}
222222

223-
error: aborting due to 15 previous errors
223+
error: this `else` block is redundant
224+
--> tests/ui/needless_continue.rs:339:16
225+
|
226+
LL | } else {
227+
| ________________^
228+
LL | |
229+
LL | | continue;
230+
LL | | }
231+
| |_________^
232+
|
233+
= help: consider dropping the `else` clause and merging the code that follows (in the loop) with the `if` block
234+
if todo!() {
235+
// merged code follows:
236+
237+
}
238+
239+
error: aborting due to 16 previous errors
224240

0 commit comments

Comments
 (0)