Wrong clippy message or not?

1

I am a bit confused about a suggestion given by clippy.
In my code I have something like this:

  if self.kind == CheckType::Package {
      w0005!(dir_entry_str, fsize / 1024 / 1024);
  } else {
      w0006!(dir_entry_str, fsize / 1024 / 1024);
  }
  return false;

clippy says:

warning: all if blocks contain the same code at the start
  --> src/utils.rs:43:13
   |
43 | /             if self.kind == CheckType::Package {
44 | |                 w0005!(dir_entry_str, fsize / 1024 / 1024);
   | |___________________________________________________________^
   |
   = note: `#[warn(clippy::branches_sharing_code)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code
help: consider moving the start statements out like this
   |
43 |             w0005!(dir_entry_str, fsize / 1024 / 1024);
44 |             if self.kind == CheckType::Package {
   |

I don't agree that all if blocks contain the same at the start. w0005! and w0006! are different.

Anything I am overlooking?

2

A small experiment (click “Clippy” under “Tools”) seems to suggest that clippy considers the code after macro expansion in order to determine whether the “then” and “else” branch start the same (or are the same).

Maybe your macros w0005 and w0006 do the same thing? If this is intentional, e.g. dependent on conditional compilation or what not, feel free to silence the clippy warning; if it isn’t intentional, maybe it’s a bug?
Edit: Wait, the message says the branches “start the same”, so apparently both macro expansions start the same way, which is probably intentional. I’m not sure if this is something that clippy should change, probably yes, because the warning is not really actionable like that; I’m not sure how clippy works internally, i.e. if that’s simple to fix of whether macro expansion happening first is a fundamental part of how clippy operates. In any case, just put an #[allow(clippy::branches_sharing_code)] on the containing block or function; locally disabling clippy lints for the occasional false positive isn’t considered bad style.

3

Good idea with the macro expansion but although they start the same insofar as in both a println! happens the text to output starts with W0005 resp. W0006.

I am also not sure if this is something clippy should do better.

However, I agree that for the time being it might be best to tell clippy to accept it as you suggested.

4

That seems odd; sounds like it really shouldn’t produce a warning in that case? I cannot analyze this any further without seeing the definition of those macros, though, or an equivalent example reproducing that warning.

Edit: I just realized that println is a macro, too. I wonder if certain differing println calls might also be able to expand to code that starts the same.

Edit2:, no, println! can’t be the problem; it wraps its entire contents in a block and the branches_sharing_code lint doesn’t fire if stuff is wrapped in an additional layer of {}. Perhaps, if you have control over those, changing the definition of w0005, etc from something like

macro_rules! w0005 {
    ($foo: expr, $bar: expr) => {
        /* STATEMENTS */
    };
}

to

macro_rules! w0005 {
    ($foo: expr, $bar: expr) => {
        {
            /* STATEMENTS */
        }
    };
}

could be a good idea. This would also make the w0005 macro usable as an expression, while currently it’s probably only usable as a statement (i.e. I’d assume you can’t currently write things like let _ = w0005!(…);).

5

Here is a minimal example to make clippy unhappy:

macro_rules! error_occured {
    () => {
        println!("blabla");
    };
}

macro_rules! w0005 {
    ($fmt1:expr, $fmt2:expr) => {
        error_occured!();
        print!(
            "{}   some blabla {} and more blabla {} detected\n",
            "W0005",
            $fmt1,
            $fmt2
        );
    };
}

macro_rules! w0006 {
    ($fmt1:expr, $fmt2:expr) => {
        error_occured!();
        print!(
            "{}   some blabla {} and more blabla {} detected\n",
            "W0006",
            $fmt1,
            $fmt2
        );
    };
}


fn main() {
    if true {
        w0005!("parm1", "parm2");
    }
    else {
        w0006!("parm1", "parm2");

    }

}

Note: If I leave out the error_occured!(); invocation clippy is happy.

6

Right, I think it’s probably the best idea to add an additional layer of {} to each macro definition consisting of more statements than a single expression; furthermore I’d leave out the ; after println!("blablabla"), or wrap it into a block as-well.

The first layer of {} in a macro_rules definition just delimits the right-hand-side of the macro definition, it does not produce a block on its own. This is why e.g. a macro definition like

macro_rules! define {
   ($i:ident) => {
        let x = 42_i32;
    }
}

can be used to define a variable in the containing scope, e.g.

fn f() {
    define!(x);
    println!("{}", x); // -> prints 42
}

Generally in Rust, we want to have that “everything is an expression”, except for let-statements, and similarly except for macros that use let-statements to define something in their surrounding context. If your macro doesn’t define something to be used later, add a block to make it (usable as) an expression :slight_smile: Your minimal example fixed (link).

2 Likes
7

An additional layer of {} is indeed a very good idea. Very productive discussion.

Thanks for your help..

8

When I run rustfmt it gets formatted like this:

macro_rules! w0005 {
    ($fmt1:expr, $fmt2:expr) => {{
        error_occured!();
        print!(
            "{}   some blabla {} and more blabla {} detected\n",
            "W0005", $fmt1, $fmt2
        )
    }};
}

Not sure I like it. :slight_smile:

9

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.