Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[never_loop] Fix #10304 #10311

Merged
merged 4 commits into from Feb 14, 2023
Merged

[never_loop] Fix #10304 #10311

merged 4 commits into from Feb 14, 2023

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Feb 9, 2023

It is not sufficient to ignore break from a block inside the loop. Instructions after the break must be ignored, as they are unreachable. This is also true for all instructions in outer blocks and loops until the right block is reached.

Fixes #10304


changelog: FP: [never_loop]: No longer lints, for statements following break statements for outer blocks.
#10311

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2023

r? @Jarcho

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 9, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Feb 13, 2023

Overall looks good. While you're at changing this you can remove combine_both. Sub-expressions have always been executed in order, and it's been defined to be that way for quite a while now.

@samueltardieu
Copy link
Contributor Author

Overall looks good. While you're at changing this you can remove combine_both. Sub-expressions have always been executed in order, and it's been defined to be that way for quite a while now.

Indeed, will do.

@samueltardieu samueltardieu force-pushed the issue-10304 branch 4 times, most recently from 346a9f2 to 758e4ce Compare February 13, 2023 19:40
@samueltardieu
Copy link
Contributor Author

It is ready now. I first removed combine_both() and merged the changes into the existing commit, then I realized it probably deserved a commit on its own for traceability.

`combine_seq(x, NeverLoopResult::Otherwise)`  always returns `x`
All evaluations now happen in order.
Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

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

Took a closer second look at this and there are a few things which need to be fixed.

clippy_lints/src/loops/never_loop.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/never_loop.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops/never_loop.rs Outdated Show resolved Hide resolved
The following code

```
loop {
    'a: {
        { }
        break 'a;
    }
}
```

was detected as a never-looping loop.
It is not sufficient to ignore break from a block inside the loop.
Instructions after the break must be ignored, as they are unreachable.
This is also true for all instructions in outer blocks and loops
until the right block is reached.
@Jarcho
Copy link
Contributor

Jarcho commented Feb 14, 2023

All looks good. Thanks for the fix. @bors r+

@bors
Copy link
Collaborator

bors commented Feb 14, 2023

📌 Commit 657ee48 has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 14, 2023

⌛ Testing commit 657ee48 with merge a182a67...

@bors
Copy link
Collaborator

bors commented Feb 14, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing a182a67 to master...

@bors bors merged commit a182a67 into rust-lang:master Feb 14, 2023
@samueltardieu samueltardieu deleted the issue-10304 branch February 14, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clippy::never_loop has false positive with break from labeled blocks
4 participants