Skip to content

Conversation

profetia
Copy link
Contributor

@profetia profetia commented Aug 23, 2025

Closes #14550
Closes #15548

changelog: [needless_continue] fix FP when match type is not unit or never

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 23, 2025
@profetia
Copy link
Contributor Author

Also closes #15548

Copy link

github-actions bot commented Aug 23, 2025

No changes for f62c1e8

Copy link
Member

@samueltardieu samueltardieu left a 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.

@profetia
Copy link
Contributor Author

Fixed. Thank you!

@profetia profetia requested a review from samueltardieu August 24, 2025 02:09
Copy link
Member

@samueltardieu samueltardieu left a 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.

@llogiq
Copy link
Contributor

llogiq commented Aug 24, 2025

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.

@llogiq
Copy link
Contributor

llogiq commented Aug 24, 2025

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.

@profetia
Copy link
Contributor Author

Those are FPs introduced when migrating the lint to late pass, and they are now fixed. Thank you! @samueltardieu @llogiq

@profetia profetia requested a review from samueltardieu August 24, 2025 16:17
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.

needless_continue wrongly unmangled macros needless_continue false positive
4 participants