Skip to content

fix(while_let_loop): detect the pattern when the loop has a label - #17614

Open
durationextender wants to merge 1 commit into
rust-lang:masterfrom
durationextender:fix/while-let-loop-label-detection
Open

fix(while_let_loop): detect the pattern when the loop has a label#17614
durationextender wants to merge 1 commit into
rust-lang:masterfrom
durationextender:fix/while-let-loop-label-detection

Conversation

@durationextender

@durationextender durationextender commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

while_let_loop wasn't firing when the loop had a label, since the break check assumed no label. Now it also accepts break 'label when it matches the loop's own label, while still ignoring breaks meant for a different outer loop.

Covers if let/else, let else, and match versions.

fixes #17590

changelog: [while_let_loop]: detect the pattern even when the enclosing loop has a label

@rustbot rustbot added S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 23, 2026
@rustbot

rustbot commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews.

In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews.

@rustbot

This comment has been minimized.

@DanielEScherzer DanielEScherzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

community review: code seems fine, suggestions on the comments but overall LGTM

View changes since this review

/// Checks if `block` contains a single unlabeled `break` expression or statement, possibly embedded
/// inside other blocks.
fn is_simple_break_block(block: &Block<'_>) -> bool {
/// Checks if `block` contains a single unlabeled and importantly now also a labeled `break`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Checks if `block` contains a single unlabeled and importantly now also a labeled `break`
/// Checks if `block` contains a single (labeled or unlabeled) `break`

the "now" part is confusing, since we don't normally reference older versions of the code outside of regression tests; unclear why this is marked as "importantly"

/// Checks if `expr` contains a single unlabeled `break` expression or statement, possibly embedded
/// inside other blocks.
fn is_simple_break_expr(expr: &Expr<'_>) -> bool {
/// Checks if `expr` contains a single unlabeled and importantly now also a labeled `break`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Checks if `expr` contains a single unlabeled and importantly now also a labeled `break`
/// Checks if `expr` contains a single (labeled or unlabeled ) `break`

per above

@CommanderStorm CommanderStorm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if you apply these

Make sure to squash all commits into one.

(not +1 to not prematurely move this out of community review)

View changes since this review

@durationextender

Copy link
Copy Markdown
Contributor Author

Should be good now i hope.

@rustbot ready

@durationextender
durationextender force-pushed the fix/while-let-loop-label-detection branch from 3d03892 to 98939c0 Compare August 23, 2026 16:21
@rustbot

rustbot commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

@Gri-ffin Gri-ffin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include the label in the suggestion? I don't think silently dropping the label is a good idea.

View changes since this review

@Lokathor

Copy link
Copy Markdown

As I mentioned in #17590, a while-let loop is allowed to be labeled, so the label should be preserved even if it's only a "comment" about what the loop is doing.

@CommanderStorm

CommanderStorm commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Should be good now i hope.

Sorry, you still have fixes https://github.com/rust-lang/rust-clippy/issues/17590 in the commit, please remove this 😉

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 24, 2026
@rustbot

rustbot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

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

Labels

S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) S-waiting-on-community-reviews Status: This is awaiting for positive reviews from the community before a maintainer is assigned.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

while_let_loop seems to not fire if the loop has a label

6 participants