Macro-generated code triggers suspicious-else-formatting

[jplatte]

I tried this code:

impl Device {
    pub async fn set_status(id: Uuid, status: &str, db: impl PgExecutor<'_>) -> sqlx::Result<u64> {
        Ok(
            sqlx::query!("UPDATE devices SET status = $1 WHERE id = $2", status, id)
                .execute(db)
                .await?
                .rows_affected(),
        )
    }
}

(added some context because indentation level might matter? or maybe it's just entirely unformatted macro output)

I expected to see this happen: No warnings

Instead, this happened:

error: this looks like an `else if` but the `else` is missing
   --> src/db/models/device.rs:109:80
    |
109 |             sqlx::query!("UPDATE devices SET status = $1 WHERE id = $2", status, id)
    |                                                                                ^^
    |
    = note: `-D clippy::suspicious-else-formatting` implied by `-D warnings`
    = note: to remove this lint, add the missing `else` or add a new line before the second `if`
    = help: for further information visit rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting

Meta

(will fill this in once I can reproduce on a local computer again, right now fails on CI, probably clippy 0.0.212 (ffa2e7a 2020-10-24))

[xFrednet]

I can sadly not reproduce that currently by using the simple macro_rules! macro for defining my own macro (Playground). I guess that it has to do something with the use of quote_spanned! as this is used in the code that caused the lint. The documentation of that macro is sadly not clear enough for me to write my own minimal version.

The lint it self comes from /clippy_lints/src/formatting.rs. I would look further into this if someone could maybe provide a simple example 😅

[ebroto]

If I'm not mistaken quote is already a dependency in our test suite so you should be able to use quote_spanned there.

[xFrednet]

@rustbot claim

[xFrednet]

This issue originates from the use of quote_spanned! and is sadly unfixable for us. It is also not specific to this lint but can happen with any macro that uses the quote_spanned! macro.

The quote_spanned! sets the span of the produced tokens to a user defined span. (See docs for more information). sqlx set the token span to the actual input parameters. Our lint therefore determines that:

1. The code span is not the result of a macro (first.span.from_expansion() is false as the token uses the span of the input)
2. The formatting is wrong because there is no empty line between the if blocks

The first point is also the reason why this could happen with every lint as it bypasses our check if the code was generated via a macro. I would therefor think that this and potential other false positives from the quote_spanned! macro are currently unfixable for us. I've created a minimal example with quote_spanned! in case you want to test for your self: 31c287e

[Nugine]

I came into this bug and reproduced it by the code below.
I don't know whether it is a problem from tracing.

#![deny(clippy::suspicious_else_formatting)]

#[tracing::instrument]
async fn get(key: &str) -> String {
    if key.is_empty() {
        panic!()
    } else {
        todo!()
    }
}

error: this is an `else {..}` but the formatting might hide it
  --> src/lib.rs:4:35
   |
4  |   async fn get(key: &str) -> String {
   |  ___________________________________^
5  | |     if key.is_empty() {
6  | |         panic!()
7  | |     } else {
8  | |         todo!()
9  | |     }
10 | | }
   | |_^
   |
note: the lint level is defined here
  --> src/lib.rs:1:9
   |
1  | #![deny(clippy::suspicious_else_formatting)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: to remove this lint, remove the `else` or remove the new line between `else` and `{..}`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting

Versions:

tracing = "0.1.28"

rustc 1.57.0-nightly (f03eb6bef 2021-10-02)
clippy 0.1.57 (f03eb6b 2021-10-02)

[extrawurst]

@Nugine I have the same issue due to instruments, the weird part is, that it fails on our CI but not locally: https://github.com/gameroasters/atlasserver/runs/3779641152?check_suite_focus=true

[Nugine]

@Nugine I have the same issue due to instruments, the weird part is, that it fails on our CI but not locally: https://github.com/gameroasters/atlasserver/runs/3779641152?check_suite_focus=true

Our CI 166 also failed today. But CI 164 succeeded.

There is no commit during this week. The CI toolchain is locked to 1.55.0. tracing-attributes 0.1.17 released yesterday. So I'm sure that the change of tracing-attributes triggers this bug.

Try to update your lockfile and toolchain.

[extrawurst]

@Nugine right on, I did not have the tracing-attributes update locally yet, thanks for the hint!

[xFrednet]

I might have an idea how to avoid FPs like these in exchange for some false negatives. 🙃

@rustbot claim

once again ^^

[xFrednet]

My idea sadly didn't work out. #7707 also did some work with another approach, that seems promising. I'm closing this in the hopes that it's fixed 🙃