[needless_continue]: lint if the last stmt in for/while/loop is `co…

[lengyijun]

changelog: [needless_continue]: lint if the last stmt in for/while/loop is continue, recursively
Fixes: #4077

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[Alexendoo]

This would be another good one to look at @y21

[y21]

Just a few things, other than that it looks good to me!

[y21]

I believe the lint description could also use an update now that it lints additional cases and no longer just if exprs.

[xFrednet]

Hey @lengyijun, the CI failed on your last commit. Do you need any help to address the issue? :)

[y21]

Looks good to me!

[Alexendoo]

@bors delegate=y21

I think a good follow up to this if you're interested would be to split this lint up, this addition should be warn by default IMO but the lint is currently pedantic. The suggestions that require rearranging code instead of just removing a continue/else { continue } could go in their own lint

[bors]

✌️ @y21, you can now approve this pull request!

If @Alexendoo told you to "r=me" after making some further change, please make that change, then do @bors r=@Alexendoo

[y21]

I looked through this one last time and found a potential edge case I wanted to mention (sorry! 😅)

[y21]

Right now it's possible for the same lint to emit two warnings for the following code:

loop {
  if true {

  } else { // redundant `else`
    continue; // redundant `continue`
  }
}

Output

warning: this `continue` expression is redundant
   --> test.rs:171:13
    |
171 |             continue;
    |             ^^^^^^^^

warning: this `else` block is redundant
   --> test.rs:170:16
    |
170 |           } else {
    |  ________________^
171 | |             continue;
172 | |         }
    | |_________^
    |

I like the other warning a bit more in this case because it suggests removing the whole else block altogether.

@Alexendoo What do you think? Is this fine to ignore for now or would you consider it a potential blocker? Looks like it possibly has a bit of overlap with your suggestion to split up the lint

If this is a good idea and worth fixing in this PR, we could probably just move this check_last_stmt_in_expr call into the loop and guard it behind an i == stmts.len() - 1 check so that it only runs on the last statement, inline the with_if_expr function (it's really short and only used here, anyway) and add continue; in the one lint case that is emitted here so that it skips checking the new case.

[Alexendoo]

Yeah, good catch, we don't want to be linting the same thing twice

The else winning would be preferable, but if it's easier implementation wise to ignore it that's fine too, there's a lint for empty else blocks that would pick it up

[bors]

☔ The latest upstream changes (presumably #11750) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey @lengyijun, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

[xFrednet]

Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this!

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review