`tracing::instrument` triggers `blocks_in_conditions`

[fenollp]

Summary

Basically https://github.com/rust-lang/rust-clippy/issues/12016#issuecomment-1935653049

Since 1.76.0, any async fn impl that is decorated with #[tracing::instrument(..)] will trigger https://rust-lang.github.io/rust-clippy/master/index.html#/blocks_in_conditions

Lint Name

blocks_in_conditions

Reproducer

I tried this code:

    #[tracing::instrument(skip(self), err)]
    async fn get_private_network(&mut self, private_network_id: Uuid) -> Result<PrivateNetwork> {
        let req = GetPrivateNetworkRequest { private_network_id: private_network_id.to_string() };
        match self.client.get_private_network(req).await {
            Ok(r) => Ok(r.into_inner().try_into()?),
            Err(s) => match s.code() {
                Code::NotFound => Err(Error::NotFound {
                    kind: "private_network",
                    id: private_network_id.to_string(),
                }),
                _ => Err(Error::ApiError(s.to_string())),
            },
        }
    }

I saw this happen:

error: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
  --> api_clients/src/clients/vpc_v2.rs:28:97
   |
28 |       async fn get_private_network(&mut self, private_network_id: Uuid) -> Result<PrivateNetwork> {
   |  _________________________________________________________________________________________________^
29 | |         let req = GetPrivateNetworkRequest { private_network_id: private_network_id.to_string() };
30 | |         match self.client.get_private_network(req).await {
31 | |             Ok(r) => Ok(r.into_inner().try_into()?),
...  |
39 | |         }
40 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions

I expected to see this happen: no warnings

Version

rustc 1.76.0 (07dca489a 2024-02-04)
binary: rustc
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce
commit-date: 2024-02-04
host: x86_64-unknown-linux-gnu
release: 1.76.0
LLVM version: 17.0.6

Additional Labels

No response

[Jarcho]

Needs either a call to is_from_proc_macro or in_external_macro.

[y21]

Did #12040 not also fix this? It added a call to is_from_proc_macro, and it already uses in_external_macro. Fix isn't on nightly yet though.

[okynos]

Hello!

I just came here to ask some questions. This issue is related to this clippy message in my repository? https://github.com/Achiefs/fim/security/code-scanning/59 Then there is something I can do to fix it?

Thanks

[y21]

@okynos I can't see the linked page, but I ran clippy on your project and I assume you mean clippy linting on this match? https://github.com/Achiefs/fim/blob/c3d1f23bf2df51f8b2a8d9a7c1402a2b7a0a65b9/src/monitor.rs#L146-L147

In that case, it doesn't look related to the issue here, and as far as I can tell your case seems like an expected warning. The lint seems to be explicitly looking for that pattern (match expression with a closure containing a block), and wants you to move the ctrlc::set_handler() call to its own variable.

[okynos]

Hi @y21,

Thanks for your explanation. I was confused because my local clippy doesn't report the same as Github. I updated it and now I'm working on a fix.

For those who comes with the same misunderstood or problem. The simplified message is that you shouldn't introduce {} inside complex match. I mean you should avoid to insert code inside a match. Let a function or variable do the job. For example I have:

        match ctrlc::set_handler(move || {
            for element in &copied_config.audit {
                let path = element["path"].as_str().unwrap();
                let rule = utils::get_audit_rule_permissions(element["rule"].as_str());
                utils::run_auditctl(&["-W", path, "-k", "fim", "-p", &rule]);
            }
            std::process::exit(0);
        }) {
            Ok(_v) => debug!("Handler Ctrl-C set and listening"),
            Err(e) => error!("Error setting Ctrl-C handler, the process will continue without signal handling, Error: '{}'", e)
        }

That contains code inside the match. I transform it into this:

fn clean_audit_rules(config: &config::Config){
    for element in config.audit.clone() {
        let path = element["path"].as_str().unwrap();
        let rule = utils::get_audit_rule_permissions(element["rule"].as_str());
        utils::run_auditctl(&["-W", path, "-k", "fim", "-p", &rule]);
    }
    std::process::exit(0);
}

        match ctrlc::set_handler(move || clean_audit_rules(&clone_config)) {
            Ok(_v) => debug!("Handler Ctrl-C set and listening"),
            Err(e) => error!("Error setting Ctrl-C handler, the process will continue without signal handling, Error: '{}'", e)
        }

[flokli]

I'm a bit confused - https://github.com/rust-lang/rust-clippy/pull/12173 seems related, and is not included in the rustc-1.76.0 sources, so I assumed it was a fix - however even after applying the patch it doesn't seem to prevent the warnings.

[y21]

It'd be good to have some kind of MCVE for this, because a simple async fn annotated with #[tracing::instrument] and a match is apparently not enough to trigger it: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2e011e037ed64ff5003b40b78c61605d

I'm surprised that #12040 and #12173 supposedly didn't fix it 🤔 (though it is worth noting that neither of those fixes are on the stable channel, so even if this was fixed, you would still see the warnings on stable. ~~It should be on beta however~~ nevermind, #12040 is not on beta)

[flokli]

Could this be functions that are also rewritten using other macros?

Thinking about

#[async_trait]
impl … for … {
  #[tracing::instrument(…)
  async fn foo() { … }
}

blocks.

Another place I saw this occuring is functions with #[async_recursion(?Send)] and #[instrument(…)] - that might be a smaller reproducer.

[flokli]

I was able to produce a small repro: https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=b1dbfd2daf9f4a2cb6c79c46472b8f14

use async_trait::async_trait;

#[async_trait]
pub trait FooService {
    async fn do_something(&self, r: i32) -> std::io::Result<i32>;
}

struct Foo {}

#[async_trait]
impl FooService for Foo {
    #[tracing::instrument(skip(self), ret, err)]
    async fn do_something(&self, _request: i32) -> std::io::Result<i32> {
        Err(std::io::Error::new(
            std::io::ErrorKind::Other,
            "some error",
        ))
    }
}

Clippy complains like this:

    Checking playground v0.0.1 (/playground)
warning: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
  --> src/lib.rs:13:73
   |
13 |       async fn do_something(&self, _request: i32) -> std::io::Result<i32> {
   |  _________________________________________________________________________^
14 | |         Err(std::io::Error::new(
15 | |             std::io::ErrorKind::Other,
16 | |             "some error",
17 | |         ))
18 | |     }
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions
   = note: `#[warn(clippy::blocks_in_conditions)]` on by default

warning: `playground` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.30s

[iajoiner]

Yup. I think at the very least we shouldn't get clippy issues for code in our dependencies.

[iajoiner]

Can we disable this lint by default??

[seanpianka]

In case anyone else is wondering, the fix for this should get released in the 1.80 release of rust/clippy.

[matze]

In case anyone else is wondering, the fix for this should get released in the 1.80 release of rust/clippy.

But it didn't, right?

[SimenB]

Still happens in my project using 1.80. as well. However, looking at the changelog, only commits up until May 30th were included, and the fix for this landed June 8th. So 1.81 presumably?

[y21]

Yes, the fix will be in 1.81. 1.80 branched on June 7th and the referenced PR was merged on June 8th. releases.rs can be useful for this.