Fix `never_loop` forget to remove `break` in suggestion #15064

New issue

Jump to bottom

Open

profetia wants to merge 1 commit into rust-lang:master from profetia:issue15007

+183 −26

Contributor

profetia commented Jun 16, 2025 •

edited by blyxyas

Loading

Closes #15007

Removes the break stmt and the dead code after it when appropriate.

changelog: [never_loop] Make sure to remove break in suggestions

rustbot assigned Alexendoo

Collaborator

rustbot commented Jun 16, 2025

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added the S-waiting-on-review label

profetia force-pushed the issue15007 branch 2 times, most recently from d2ee798 to 4f18d7f Compare

June 17, 2025 13:13


          fix: never_loop forget to remove break in suggestion

028a864

profetia force-pushed the issue15007 branch from 4f18d7f to 028a864 Compare

June 17, 2025 13:17

Member

blyxyas commented Jul 11, 2025

Alexendoo's a little bit over the top on capacity (32 assigned PRs)
I'll shave off his last uncommented PRs.

rustbot assigned blyxyas and unassigned Alexendoo

blyxyas requested changes

View reviewed changes

Member

blyxyas left a comment

Some starting review, mainly on readability.

clippy_lints/src/loops/never_loop.rs

                                       for_to_if_let_sugg(cx, iterator, pat),
+                                  )];
+                                  suggestions.extend(inner.iter().map(|span| (*span, String::new())));

Member

blyxyas Jul 14, 2025

(In tandem with the last suggestion)
How about a comment here?

Suggested change

      
                                suggestions.extend(inner.iter().map(|span| (*span, String::new())));
          
                                // Make sure to clear up the diverging sites when we remove a loopp.
          
                                suggestions.extend(break_spans.iter().map(|span| (*span, String::new())));

clippy_lints/src/loops/never_loop.rs

@@ @@ -20,7 +22,7 @@ pub(super) fn check<'tcx>( @@
                   for_loop: Option<&ForLoop<'_>>,
               ) {
                   match never_loop_block(cx, block, &mut Vec::new(), loop_id) {
-                      NeverLoopResult::Diverging => {
+                      NeverLoopResult::Diverging(ref inner) => {

Member

blyxyas Jul 14, 2025

A better variable name could help the readability here.

Suggested change

      
                    NeverLoopResult::Diverging(ref inner) => {
          
                    NeverLoopResult::Diverging(ref break_spans) => {

clippy_lints/src/loops/never_loop.rs

               enum NeverLoopResult {
                   /// A continue may occur for the main loop.
                   MayContinueMainLoop,
                   /// We have not encountered any main loop continue,
                   /// but we are diverging (subsequent control flow is not reachable)
-                  Diverging,
+                  Diverging(Vec<Span>),

Member

blyxyas Jul 14, 2025

What do you think about?

Suggested change

      
                Diverging(Vec<Span>),
          
                Diverging { break_spans: Vec<Span> },

clippy_lints/src/loops/never_loop.rs

                           NeverLoopResult::MayContinueMainLoop
                       },
                       (NeverLoopResult::Normal, _) | (_, NeverLoopResult::Normal) => NeverLoopResult::Normal,
-                      (NeverLoopResult::Diverging, NeverLoopResult::Diverging) => NeverLoopResult::Diverging,
+                      (NeverLoopResult::Diverging(mut inner1), NeverLoopResult::Diverging(mut inner2)) => {

Member

blyxyas Jul 14, 2025

Suggested change

      
                    (NeverLoopResult::Diverging(mut inner1), NeverLoopResult::Diverging(mut inner2)) => {
          
                    (NeverLoopResult::Diverging(mut break_spans1), NeverLoopResult::Diverging(mut break_spans2)) => {

clippy_lints/src/loops/never_loop.rs

                   }
               }
+              fn stmt_source_span(stmt: &Stmt<'_>) -> Span {
+                  let call_span = stmt.span.source_callsite();
+                  // if it is a macro call, the span will missing the trailing semicolon

Member

blyxyas Jul 14, 2025

Suggested change

      
                // if it is a macro call, the span will missing the trailing semicolon
          
                // if it is a macro call, the span will be missing the trailing semicolon

clippy_lints/src/loops/never_loop.rs

+                  if stmt.span == call_span {
+                      return call_span;
+                  }
+                  call_span.with_hi(call_span.hi() + BytePos(1))

Member

blyxyas Jul 14, 2025

You should probably check the statement kind, because there are statements with explicitly no semicolon.

clippy_lints/src/loops/never_loop.rs

+                  call_span.with_hi(call_span.hi() + BytePos(1))
+              }
+              fn expr_find_remove_span(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {

Member

blyxyas Jul 14, 2025

Suggested change

      
            fn expr_find_remove_span(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {
          
            /// Returns a Vec of all the individual spans after the highlighted expression in a block
          
            fn all_spans_after_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> Vec<Span> {

rustbot added S-waiting-on-author and removed S-waiting-on-review labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author