-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
new lint to detect infinite loop #11829
Conversation
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
62d9409
to
34a88f5
Compare
34a88f5
to
2d9fc6d
Compare
What about loops that have todo!() / panic!() / unreachable() / assert!() in them? We have at least a test imo 🙂 |
might not have time to get around to this soon |
1516cda
to
1d15de5
Compare
1d15de5
to
635dc92
Compare
635dc92
to
bb7a389
Compare
6ae6d02
to
3e9a6d1
Compare
and add more test cases
ad62f9e
to
0d26f91
Compare
Thanks @y21 for the hint!
clippy_lints/src/loops/mod.rs
Outdated
#[clippy::version = "1.75.0"] | ||
pub INFINITE_LOOP, | ||
restriction, | ||
"possibly unintended infinite loops" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loops -> loop?
I think this reads a bit better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondering about weird fn like
fn loopfn() -> Result<(), i32> {
Ok(loop {})
}
afaik something like -> Result <!, i32>
is not stabilized yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondering about weird fn like
Hmmm, Sorry I wasn't aware such cases, currently it would just prints help message suggesting adding a break condition...
What would be an ideal solution here 🤔 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, was on holiday :)
If it only suggests adding break/return, I think it should be fine then.
Another scenario that just came to my mind now:
loop {
continue;
}
This does have some kind of "control flow" but in this case it still loops indefinitely.
The linkt would lint here as well, right? (which is the correct solution I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy holiday 😄
As of your concern, yes it will lint, right now the visitor only looks for following occurrence in a loop block:
- A return or a break
- A function which never return, which was inspired be one of your early suggestion such as the one in
panic!
,unreachable!
, alsostd::process::exit
.
And if non of these could be find then... well, triggers the warning.
3093d53
to
758d0e8
Compare
& apply review suggestions;
Thanks! |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
…=xFrednet fix typo in infinite loop lint *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: This fixes a small typo introduced in #11829
closes: #11438
changelog: add new lint to detect infinite loop
I'll change the lint name. Should I name itinfinite_loop
orinfinite_loops
is fine? Ahhhh, English is hard...