Auto merge of rust-lang#11455 - digama0:never_loop3, r=Centri3,dswij

skip `todo!()` in `never_loop` As promised in rust-lang#11450, here is an implementation which skips occurrences of the `todo!()` macro. changelog: [`never_loop`]: skip loops containing `todo!()`
flip1995 · Sep 4, 2023 · da882f0 · da882f0
2 parents f13e1f4 + 4e0a346
commit da882f0
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 47 deletions.
diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs
@@ -2,11 +2,12 @@ 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};
 use rustc_lint::LateContext;
-use rustc_span::Span;
+use rustc_span::{sym, Span};
 use std::iter::{once, Iterator};
 
 pub(super) fn check<'tcx>(
@@ -263,13 +264,24 @@ 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 Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id)
+    {
+        // 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
+    }
 }
 
 fn never_loop_expr_all<'tcx, T: Iterator<Item = &'tcx Expr<'tcx>>>(

diff --git 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::<Vec<_>>();
         //~^ 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<usize> = vec.iter().map(|i| i * i).collect();
         let w = v.iter().collect::<Vec<_>>();
         // 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::<Vec<_>>();
         //~^ 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<usize> = vec.iter().map(|i| i * i).collect();
         let mut w = v.iter().collect::<Vec<_>>();
         // 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::<Vec<_>>();
         //~^ 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<usize> = vec.iter().map(|i| i * i).collect();
         let mut w = v.iter().collect::<Vec<_>>();
         // 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
@@ -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::<Vec<_>>();
    |                              ^^^^^^^
@@ -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::<Vec<_>>();
    |                              ^^^^^^^
@@ -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
@@ -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 {
+        //~^ ERROR: this loop never actually loops
+        unimplemented!("not yet");
+    }
+    loop {
+        // no error
+        todo!("maybe later");
+    }
 }
 
 fn main() {

diff --git 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
 
diff --git 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;
 
@@ -125,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
@@ -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;
 
@@ -125,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
@@ -1,115 +1,115 @@
 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: `&[]`
    |
    = 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: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:145: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:145: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:164: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:168: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()]`