From 61a2f972b3ae063512cdcd3e10fd2f95319fec24 Mon Sep 17 00:00:00 2001 From: Mario Carneiro Date: Sat, 2 Sep 2023 23:34:03 -0400 Subject: [PATCH 1/3] skip `todo` / `unimplemented` in `never_loop` --- clippy_lints/src/loops/never_loop.rs | 17 ++++++++-- tests/ui/needless_collect_indirect.rs | 6 ---- tests/ui/needless_collect_indirect.stderr | 7 ++--- tests/ui/never_loop.rs | 10 +++++- tests/ui/vec.fixed | 7 +---- tests/ui/vec.rs | 7 +---- tests/ui/vec.stderr | 38 +++++++++++------------ 7 files changed, 47 insertions(+), 45 deletions(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 5cdf0bd89..5845be870 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -2,6 +2,7 @@ use super::utils::make_iterator_snippet; use super::NEVER_LOOP; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::ForLoop; +use clippy_utils::macros::root_macro_call_first_node; use clippy_utils::source::snippet; use rustc_errors::Applicability; use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind}; @@ -263,13 +264,25 @@ fn never_loop_expr<'tcx>( | ExprKind::Lit(_) | ExprKind::Err(_) => NeverLoopResult::Normal, }; - combine_seq(result, || { + let result = combine_seq(result, || { if cx.typeck_results().expr_ty(expr).is_never() { NeverLoopResult::Diverging } else { NeverLoopResult::Normal } - }) + }); + if let NeverLoopResult::Diverging = result && + let Some(macro_call) = root_macro_call_first_node(cx, expr) && + let "todo" | "unimplemented" = cx.tcx.item_name(macro_call.def_id).as_str() + { + // We return MayContinueMainLoop here because we treat `todo!()` and + // `unimplemented!()` macros as potentially containing any code, + // including a continue of the main loop. This effectively silences the lint + // whenever a loop contains one of these macros anywhere. + NeverLoopResult::MayContinueMainLoop + } else { + result + } } fn never_loop_expr_all<'tcx, T: Iterator>>( diff --git a/tests/ui/needless_collect_indirect.rs b/tests/ui/needless_collect_indirect.rs index b4a536cb1..9d66c5f25 100644 --- a/tests/ui/needless_collect_indirect.rs +++ b/tests/ui/needless_collect_indirect.rs @@ -260,7 +260,6 @@ mod issue_8553 { let w = v.iter().collect::>(); //~^ ERROR: avoid using `collect()` when not needed // Do lint - #[allow(clippy::never_loop)] for _ in 0..w.len() { todo!(); } @@ -271,7 +270,6 @@ mod issue_8553 { let v: Vec = vec.iter().map(|i| i * i).collect(); let w = v.iter().collect::>(); // Do not lint, because w is used. - #[allow(clippy::never_loop)] for _ in 0..w.len() { todo!(); } @@ -285,7 +283,6 @@ mod issue_8553 { let mut w = v.iter().collect::>(); //~^ ERROR: avoid using `collect()` when not needed // Do lint - #[allow(clippy::never_loop)] while 1 == w.len() { todo!(); } @@ -296,7 +293,6 @@ mod issue_8553 { let mut v: Vec = vec.iter().map(|i| i * i).collect(); let mut w = v.iter().collect::>(); // Do not lint, because w is used. - #[allow(clippy::never_loop)] while 1 == w.len() { todo!(); } @@ -310,7 +306,6 @@ mod issue_8553 { let mut w = v.iter().collect::>(); //~^ ERROR: avoid using `collect()` when not needed // Do lint - #[allow(clippy::never_loop)] while let Some(i) = Some(w.len()) { todo!(); } @@ -321,7 +316,6 @@ mod issue_8553 { let mut v: Vec = vec.iter().map(|i| i * i).collect(); let mut w = v.iter().collect::>(); // Do not lint, because w is used. - #[allow(clippy::never_loop)] while let Some(i) = Some(w.len()) { todo!(); } diff --git a/tests/ui/needless_collect_indirect.stderr b/tests/ui/needless_collect_indirect.stderr index 47048b741..9337a7412 100644 --- a/tests/ui/needless_collect_indirect.stderr +++ b/tests/ui/needless_collect_indirect.stderr @@ -230,12 +230,11 @@ help: take the original Iterator's count instead of collecting it and finding th LL ~ LL | LL | // Do lint -LL | #[allow(clippy::never_loop)] LL ~ for _ in 0..v.iter().count() { | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:285:30 + --> $DIR/needless_collect_indirect.rs:283:30 | LL | let mut w = v.iter().collect::>(); | ^^^^^^^ @@ -248,12 +247,11 @@ help: take the original Iterator's count instead of collecting it and finding th LL ~ LL | LL | // Do lint -LL | #[allow(clippy::never_loop)] LL ~ while 1 == v.iter().count() { | error: avoid using `collect()` when not needed - --> $DIR/needless_collect_indirect.rs:310:30 + --> $DIR/needless_collect_indirect.rs:306:30 | LL | let mut w = v.iter().collect::>(); | ^^^^^^^ @@ -266,7 +264,6 @@ help: take the original Iterator's count instead of collecting it and finding th LL ~ LL | LL | // Do lint -LL | #[allow(clippy::never_loop)] LL ~ while let Some(i) = Some(v.iter().count()) { | diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index 33208364f..cee52dfd8 100644 --- a/tests/ui/never_loop.rs +++ b/tests/ui/never_loop.rs @@ -385,11 +385,19 @@ pub fn test31(b: bool) { } } -pub fn test32(b: bool) { +pub fn test32() { loop { //~^ ERROR: this loop never actually loops panic!("oh no"); } + loop { + // no error + unimplemented!("not yet"); + } + loop { + // no error + todo!("maybe later"); + } } fn main() { diff --git a/tests/ui/vec.fixed b/tests/ui/vec.fixed index de1eb613c..3ff2acbe2 100644 --- a/tests/ui/vec.fixed +++ b/tests/ui/vec.fixed @@ -1,10 +1,5 @@ #![warn(clippy::useless_vec)] -#![allow( - clippy::nonstandard_macro_braces, - clippy::never_loop, - clippy::uninlined_format_args, - unused -)] +#![allow(clippy::nonstandard_macro_braces, clippy::uninlined_format_args, unused)] use std::rc::Rc; diff --git a/tests/ui/vec.rs b/tests/ui/vec.rs index 1da10c288..2ab025f42 100644 --- a/tests/ui/vec.rs +++ b/tests/ui/vec.rs @@ -1,10 +1,5 @@ #![warn(clippy::useless_vec)] -#![allow( - clippy::nonstandard_macro_braces, - clippy::never_loop, - clippy::uninlined_format_args, - unused -)] +#![allow(clippy::nonstandard_macro_braces, clippy::uninlined_format_args, unused)] use std::rc::Rc; diff --git a/tests/ui/vec.stderr b/tests/ui/vec.stderr index a4c2b8984..5cd6d9fa8 100644 --- a/tests/ui/vec.stderr +++ b/tests/ui/vec.stderr @@ -1,5 +1,5 @@ error: useless use of `vec!` - --> $DIR/vec.rs:35:14 + --> $DIR/vec.rs:30:14 | LL | on_slice(&vec![]); | ^^^^^^^ help: you can use a slice directly: `&[]` @@ -7,109 +7,109 @@ LL | on_slice(&vec![]); = note: `-D clippy::useless-vec` implied by `-D warnings` error: useless use of `vec!` - --> $DIR/vec.rs:37:18 + --> $DIR/vec.rs:32:18 | LL | on_mut_slice(&mut vec![]); | ^^^^^^^^^^^ help: you can use a slice directly: `&mut []` error: useless use of `vec!` - --> $DIR/vec.rs:39:14 + --> $DIR/vec.rs:34:14 | LL | on_slice(&vec![1, 2]); | ^^^^^^^^^^^ help: you can use a slice directly: `&[1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:41:18 + --> $DIR/vec.rs:36:18 | LL | on_mut_slice(&mut vec![1, 2]); | ^^^^^^^^^^^^^^^ help: you can use a slice directly: `&mut [1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:43:14 + --> $DIR/vec.rs:38:14 | LL | on_slice(&vec![1, 2]); | ^^^^^^^^^^^ help: you can use a slice directly: `&[1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:45:18 + --> $DIR/vec.rs:40:18 | LL | on_mut_slice(&mut vec![1, 2]); | ^^^^^^^^^^^^^^^ help: you can use a slice directly: `&mut [1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:47:14 + --> $DIR/vec.rs:42:14 | LL | on_slice(&vec!(1, 2)); | ^^^^^^^^^^^ help: you can use a slice directly: `&[1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:49:18 + --> $DIR/vec.rs:44:18 | LL | on_mut_slice(&mut vec![1, 2]); | ^^^^^^^^^^^^^^^ help: you can use a slice directly: `&mut [1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:51:14 + --> $DIR/vec.rs:46:14 | LL | on_slice(&vec![1; 2]); | ^^^^^^^^^^^ help: you can use a slice directly: `&[1; 2]` error: useless use of `vec!` - --> $DIR/vec.rs:53:18 + --> $DIR/vec.rs:48:18 | LL | on_mut_slice(&mut vec![1; 2]); | ^^^^^^^^^^^^^^^ help: you can use a slice directly: `&mut [1; 2]` error: useless use of `vec!` - --> $DIR/vec.rs:79:19 + --> $DIR/vec.rs:74:19 | LL | let _x: i32 = vec![1, 2, 3].iter().sum(); | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:82:17 + --> $DIR/vec.rs:77:17 | LL | let mut x = vec![1, 2, 3]; | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:88:22 + --> $DIR/vec.rs:83:22 | LL | let _x: &[i32] = &vec![1, 2, 3]; | ^^^^^^^^^^^^^^ help: you can use a slice directly: `&[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:90:14 + --> $DIR/vec.rs:85:14 | LL | for _ in vec![1, 2, 3] {} | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:128:20 + --> $DIR/vec.rs:123:20 | LL | for _string in vec![repro!(true), repro!(null)] { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[repro!(true), repro!(null)]` error: useless use of `vec!` - --> $DIR/vec.rs:145:18 + --> $DIR/vec.rs:140:18 | LL | in_macro!(1, vec![1, 2], vec![1; 2]); | ^^^^^^^^^^ help: you can use an array directly: `[1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:145:30 + --> $DIR/vec.rs:140:30 | LL | in_macro!(1, vec![1, 2], vec![1; 2]); | ^^^^^^^^^^ help: you can use an array directly: `[1; 2]` error: useless use of `vec!` - --> $DIR/vec.rs:164:14 + --> $DIR/vec.rs:159:14 | LL | for a in vec![1, 2, 3] { | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:168:14 + --> $DIR/vec.rs:163:14 | LL | for a in vec![String::new(), String::new()] { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[String::new(), String::new()]` From 1317378b9e31306f45ece0be21104a798b4ba73c Mon Sep 17 00:00:00 2001 From: Mario Carneiro Date: Sun, 3 Sep 2023 17:16:06 +0200 Subject: [PATCH 2/3] fix todo item check, remove unimplemented --- clippy_lints/src/loops/never_loop.rs | 4 ++-- tests/ui/never_loop.rs | 2 +- tests/ui/never_loop.stderr | 11 ++++++++++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 5845be870..7b27c4132 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -7,7 +7,7 @@ use clippy_utils::source::snippet; use rustc_errors::Applicability; use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind}; use rustc_lint::LateContext; -use rustc_span::Span; +use rustc_span::{sym, Span}; use std::iter::{once, Iterator}; pub(super) fn check<'tcx>( @@ -273,7 +273,7 @@ fn never_loop_expr<'tcx>( }); if let NeverLoopResult::Diverging = result && let Some(macro_call) = root_macro_call_first_node(cx, expr) && - let "todo" | "unimplemented" = cx.tcx.item_name(macro_call.def_id).as_str() + let Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id) { // We return MayContinueMainLoop here because we treat `todo!()` and // `unimplemented!()` macros as potentially containing any code, diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index cee52dfd8..c67a6d449 100644 --- a/tests/ui/never_loop.rs +++ b/tests/ui/never_loop.rs @@ -391,7 +391,7 @@ pub fn test32() { panic!("oh no"); } loop { - // no error + //~^ ERROR: this loop never actually loops unimplemented!("not yet"); } loop { diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr index 37ccd63d2..50cec32ca 100644 --- a/tests/ui/never_loop.stderr +++ b/tests/ui/never_loop.stderr @@ -170,5 +170,14 @@ LL | | panic!("oh no"); LL | | } | |_____^ -error: aborting due to 15 previous errors +error: this loop never actually loops + --> $DIR/never_loop.rs:393:5 + | +LL | / loop { +LL | | +LL | | unimplemented!("not yet"); +LL | | } + | |_____^ + +error: aborting due to 16 previous errors From 4e0a3465d1cf3f7fee92168deef9668508d5b400 Mon Sep 17 00:00:00 2001 From: Mario Carneiro Date: Mon, 4 Sep 2023 06:35:51 +0200 Subject: [PATCH 3/3] fix vec.rs test, comment --- clippy_lints/src/loops/never_loop.rs | 7 +++---- tests/ui/vec.fixed | 1 + tests/ui/vec.rs | 1 + tests/ui/vec.stderr | 10 +++++----- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index 7b27c4132..3d8a4cd94 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -275,10 +275,9 @@ fn never_loop_expr<'tcx>( let Some(macro_call) = root_macro_call_first_node(cx, expr) && let Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id) { - // We return MayContinueMainLoop here because we treat `todo!()` and - // `unimplemented!()` macros as potentially containing any code, - // including a continue of the main loop. This effectively silences the lint - // whenever a loop contains one of these macros anywhere. + // We return MayContinueMainLoop here because we treat `todo!()` + // as potentially containing any code, including a continue of the main loop. + // This effectively silences the lint whenever a loop contains this macro anywhere. NeverLoopResult::MayContinueMainLoop } else { result diff --git a/tests/ui/vec.fixed b/tests/ui/vec.fixed index 3ff2acbe2..bcbca971a 100644 --- a/tests/ui/vec.fixed +++ b/tests/ui/vec.fixed @@ -120,6 +120,7 @@ fn issue11075() { stringify!($e) }; } + #[allow(clippy::never_loop)] for _string in [repro!(true), repro!(null)] { unimplemented!(); } diff --git a/tests/ui/vec.rs b/tests/ui/vec.rs index 2ab025f42..087425585 100644 --- a/tests/ui/vec.rs +++ b/tests/ui/vec.rs @@ -120,6 +120,7 @@ fn issue11075() { stringify!($e) }; } + #[allow(clippy::never_loop)] for _string in vec![repro!(true), repro!(null)] { unimplemented!(); } diff --git a/tests/ui/vec.stderr b/tests/ui/vec.stderr index 5cd6d9fa8..a133b4041 100644 --- a/tests/ui/vec.stderr +++ b/tests/ui/vec.stderr @@ -85,31 +85,31 @@ LL | for _ in vec![1, 2, 3] {} | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:123:20 + --> $DIR/vec.rs:124:20 | LL | for _string in vec![repro!(true), repro!(null)] { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[repro!(true), repro!(null)]` error: useless use of `vec!` - --> $DIR/vec.rs:140:18 + --> $DIR/vec.rs:141:18 | LL | in_macro!(1, vec![1, 2], vec![1; 2]); | ^^^^^^^^^^ help: you can use an array directly: `[1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:140:30 + --> $DIR/vec.rs:141:30 | LL | in_macro!(1, vec![1, 2], vec![1; 2]); | ^^^^^^^^^^ help: you can use an array directly: `[1; 2]` error: useless use of `vec!` - --> $DIR/vec.rs:159:14 + --> $DIR/vec.rs:160:14 | LL | for a in vec![1, 2, 3] { | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:163:14 + --> $DIR/vec.rs:164:14 | LL | for a in vec![String::new(), String::new()] { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[String::new(), String::new()]`