-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix needless_continue
FP when match type is not unit or never
#15547
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
base: master
Are you sure you want to change the base?
Conversation
Also closes #15548 |
No changes for f62c1e8 |
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.
The macro handling is wrong. Take the following code:
#![warn(clippy::needless_continue)]
macro_rules! mac {
($e:expr => $($rest:tt);*) => {
loop {
match $e {
1 => continue,
//~^ needless_continue
2 => break,
n => println!("{n}"),
}
$($rest;)*
}
};
}
fn main() {
mac!(2 => );
mac!(1 => {println!("foobar")});
}
With your patch, it gives:
warning: this `continue` expression is redundant
--> /tmp/t.rs:7:22
|
7 | 1 => continue,
| ^^^^^^^^
...
18 | mac!(2 => );
| ----------- in this macro invocation
|
= help: consider dropping the `continue` expression
However, if you execute the program as-is, it loops without printing anything, while if we drop the continue
as suggested, it prints "foobar" in loop.
Linting the content of macros is dangerous as it may depend on how the macro is called and expanded.
Fixed. Thank you! |
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.
By looking at the lintcheck results, it looks like you should also ensure that loop
and continue
come from the same context: https://docs.rs/encoding_rs/0.8.34/src/encoding_rs/iso_2022_jp.rs.html#161
Otherwise, you risk having the same problem with another invocation of the same macro, similar to the case I have exposed when the continue
was in the macro.
At the very least we should have a few more macro test cases. I find dealing with macros sufficiently head-warping that I want to have some confidence we're not breaking user code containing them. |
Also is the PR title correct in that we fix a false positive? Lintcheck shows 44 new warnings, which seems to imply that the PR is increasing the lint surface, not reducing it. |
Those are FPs introduced when migrating the lint to late pass, and they are now fixed. Thank you! @samueltardieu @llogiq |
Closes #14550
Closes #15548
changelog: [
needless_continue
] fix FP when match type is not unit or never