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

FP in collapsible_match on futures select! macro #7591

Closed
nipunn1313 opened this issue Aug 20, 2021 · 5 comments · Fixed by rust-lang/rust#88163
Closed

FP in collapsible_match on futures select! macro #7591

nipunn1313 opened this issue Aug 20, 2021 · 5 comments · Fixed by rust-lang/rust#88163
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@nipunn1313
Copy link
Contributor

nipunn1313 commented Aug 20, 2021

I tried this code:

use futures::executor::block_on;
use futures::future;
use futures::select;

fn main() {
    let mut a = future::ready(Some(4));
    let mut b = future::pending::<()>();

    block_on(async {
        let mut res = 0;
        select! {
            a_res = a => if let Some(a_res) = a_res { res = a_res + 1 },
            _ = b => (),
        };
        assert_eq!(res, 5);
    })
}

I expected to see no clippy error

Instead, this happened:

warning: unnecessary nested `if let` or `match`
  --> src/main.rs:12:26
   |
12 |             a_res = a => if let Some(a_res) = a_res { res = a_res + 1 },
   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::collapsible_match)]` on by default
help: the outer pattern can be modified to include the inner pattern
  --> src/main.rs:12:13
   |
12 |             a_res = a => if let Some(a_res) = a_res { res = a_res + 1 },
   |             ^^^^^               ^^^^^^^^^^^ with this pattern
   |             |
   |             replace this binding
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match

clippy seems to want a different code which is not valid:

 Some(a_res) = a => res = a_res + 1,

leading to

  --> src/main.rs:11:9
   |
11 | /         select! {
12 | |             Some(a_res) = a => res = a_res + 1,
13 | |             _ = b => (),
14 | |         };
   | |          ^
   | |          |
   | |          pattern `_0(None)` not covered
   | |__________`main::{closure#0}::__PrivResult<std::option::Option<i32>, ()>` defined here
   |            not covered

Meta

➜  clippy_test cargo clippy -V
clippy 0.1.56 (0035d9dc 2021-08-16)

It's a recent regression

Notably, I found that with

clippy 0.1.56 (2d2bc94c 2021-08-15)

things work fine

came up in the testsuite of rust-lang/futures-rs#2479

@nipunn1313 nipunn1313 added the C-bug Category: Clippy is not doing the correct thing label Aug 20, 2021
@xFrednet
Copy link
Member

Thank you for the report. At a first glance, it looks like the lint doesn't check if the code is inside a macro (I haven't looked at the linting code, though). 🙃

@rustbot label +I-false-positive

@rustbot rustbot added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Aug 20, 2021
@nipunn1313
Copy link
Contributor Author

#7565 looks potentially suspicious as it contains an August 16th change to the collapsible_match lint

https://github.com/rust-lang/rust-clippy/commits/master/clippy_lints/src/collapsible_match.rs

The timing lines up.

@xFrednet
Copy link
Member

There have also been some changes in the Rust repo on this specific lint. I wouldn't think that it originated from #7565 as your Clippy version stated a date before that PR was merged. 🙃

@nipunn1313
Copy link
Contributor Author

Yep - confirmed @xFrednet's hypothesis - by running master of clippy with various older/newer toolchains. It appears to be a change in the rust repo.

@xFrednet
Copy link
Member

Thank you, @nipunn1313, for the test 🙃.

cc. @camsteffen in case you want to work on another regression issue 😉

flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Aug 26, 2021
Fix clippy::collapsible_match with let expressions

This fixes rust-lang#7575 which is a regression from #80357. I am fixing the bug here instead of in the clippy repo (if that's okay) because a) the regression has not been synced yet and b) I would like to land the fix on nightly asap.

The fix is basically to re-generalize `match` and `if let` for the lint implementation (they were split because `if let` no longer desugars to `match` in the HIR).

Also fixes rust-lang#7586 and fixes rust-lang#7591
cc `@rust-lang/clippy`
`@xFrednet` do you want to review this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants