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

tracing::instrument triggers blocks_in_conditions #12281

Open
fenollp opened this issue Feb 12, 2024 · 11 comments · May be fixed by #12805
Open

tracing::instrument triggers blocks_in_conditions #12281

fenollp opened this issue Feb 12, 2024 · 11 comments · May be fixed by #12805
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@fenollp
Copy link

fenollp commented Feb 12, 2024

Summary

Basically #12016 (comment)

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

@fenollp fenollp added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 12, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Feb 14, 2024

Needs either a call to is_from_proc_macro or in_external_macro.

@Jarcho Jarcho added the good-first-issue These issues are a good way to get started with Clippy label Feb 14, 2024
@y21
Copy link
Member

y21 commented Feb 14, 2024

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
Copy link

okynos commented Feb 18, 2024

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
Copy link
Member

y21 commented Feb 18, 2024

@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
Copy link

okynos commented Feb 18, 2024

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)
        }

dav1do added a commit to ceramicnetwork/rust-ceramic that referenced this issue Mar 8, 2024
All 500s are now `{"message": "some reason"}`. I implemented all the methods in the server impl with the new response as error, and in the trait just `or_else(|err| Ok(resp::InternalServerError(err))` so I didn't miss any places.

Note that on 1.76, I'm seeing a lot of clippy errors due to tracing instrument: rust-lang/rust-clippy#12281
@flokli
Copy link

flokli commented Mar 11, 2024

I'm a bit confused - #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
Copy link
Member

y21 commented Mar 11, 2024

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
Copy link

flokli commented Mar 15, 2024

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

Thinking about

#[async_trait]
implfor{
  #[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
Copy link

flokli commented Mar 15, 2024

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

tvlbot pushed a commit to tvlfyi/kit that referenced this issue Mar 17, 2024
- agenix has not been updated (ryantm/agenix#241).

- //tvix: regenerate protobuf files

- //tvix:clippy: work around rust-lang/rust-clippy#12281
  which exclusively causes false positives in our code at the moment.

Change-Id: I38d2f4c0e6d1abc92be360b06f58e6d40e7732a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11127
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
tvlbot pushed a commit to tvlfyi/tvix that referenced this issue Mar 17, 2024
- agenix has not been updated (ryantm/agenix#241).

- //tvix: regenerate protobuf files

- //tvix:clippy: work around rust-lang/rust-clippy#12281
  which exclusively causes false positives in our code at the moment.

Change-Id: I38d2f4c0e6d1abc92be360b06f58e6d40e7732a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11127
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
dav1do added a commit to ceramicnetwork/rust-ceramic that referenced this issue Mar 19, 2024
All 500s are now `{"message": "some reason"}`. I implemented all the methods in the server impl with the new response as error, and in the trait just `or_else(|err| Ok(resp::InternalServerError(err))` so I didn't miss any places.

Note that on 1.76, I'm seeing a lot of clippy errors due to tracing instrument: rust-lang/rust-clippy#12281
github-merge-queue bot pushed a commit to ceramicnetwork/rust-ceramic that referenced this issue Mar 19, 2024
All 500s are now `{"message": "some reason"}`. I implemented all the methods in the server impl with the new response as error, and in the trait just `or_else(|err| Ok(resp::InternalServerError(err))` so I didn't miss any places.

Note that on 1.76, I'm seeing a lot of clippy errors due to tracing instrument: rust-lang/rust-clippy#12281
@iajoiner
Copy link

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

@iajoiner
Copy link

Can we disable this lint by default??

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 good-first-issue These issues are a good way to get started with Clippy 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.

6 participants