rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
10.89k stars 1.46k forks source link

Clippy gives unit_arg warning when using `black_box` #12707

Closed xobs closed 2 weeks ago

xobs commented 2 weeks ago

Summary

Consider the following program:

use std::{hint, thread, time};
fn main() {
    hint::black_box(thread::sleep(time::Duration::from_millis(10)));
}

The black_box() call is used to request that the function call is not removed. In this case, sleep() does not return anything. Even so, in order to give the black_box hint, it must be passed to the function.

The suggestion is to remove the black box, which is not an option.

Lint Name

unit_arg

Reproducer

I tried this code:

use std::{hint, thread, time};
fn main() {
    hint::black_box(thread::sleep(time::Duration::from_millis(10)));
}

I saw this happen:

    Checking playground v0.0.1 (/playground)
warning: passing a unit value to a function
 --> src/main.rs:3:5
  |
3 |     hint::black_box(thread::sleep(time::Duration::from_millis(10)));
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
  = note: `#[warn(clippy::unit_arg)]` on by default
help: move the expression in front of the call and replace it with the unit literal `()`
  |
3 ~     thread::sleep(time::Duration::from_millis(10));
4 ~     hint::black_box(());
  |

I expected to see this happen:

No warning, because the point of black_box is to be a hint.

Version

0.1.79 (2024-04-22 7f2fc33)

From https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=dbf98b9c0525acfe5280671a77fdf304

Additional Labels

No response

Alexendoo commented 2 weeks ago

black_box applies to the value you pass into it, I don't think applying it to a unit would achieve anything significant as there's only one possible value of unit

What is the intended effect of the black_box here?

xobs commented 2 weeks ago

I'm not sure that black_box applies to the value you pass into it.

The intended effect is for the function to not be optimized out or inlined. In this case, it's obvious that thread::sleep() won't get optmized out, but there may be cases where the compiler will decide to optimize or otherwise inline a call to a function.

For example, in the documentation, if black_box only applied to the value, then it would only apply to a bool rather than the function contains: https://doc.rust-lang.org/std/hint/fn.black_box.html#when-is-this-useful

The warning won't fire if you apply black_box to a function that returns a value, but it will fire if you try to black_box a function that returns no value.

y21 commented 2 weeks ago

std::hint::black_box is an ordinary function, so it's treated like any other function. The argument expression is still evaluated normally and optimized as per the usual rules. black_box(42 * 42) is still constant-folded to black_box(1764), so it does not prevent optimizations on its own in that regard.

Only the actual value produced by the expression in the function call is assumed to be used in arbitrary ways. To my knowledge, black_box has no effect on expressions producing ZSTs (e.g. ()), because zero-sized types carry no (runtime) information that needs to evaluate at runtime.

The example in the documentation is different: black_box(contains(...)) means "the bool value can be observed and might be shown to the user, so you cannot optimize out this bool". It does not say anything about contains -- it is still free to optimize out the contains() call if it could and simply end up with black_box(true). The other nested black_boxes ensure that this doesn't happen.

For thread::sleep this argument can't be made because no computation is needed to create a (). The compiler can make up () out of thin air. In practice, I don't think sleep will ever be optimized out, but for a different reason, the black_box does not change that.

So, IMO this lint is correct, even for black_box.

xobs commented 2 weeks ago

You're right, I appear to have a fundamental misunderstanding of the black_box documentation, and the lint here is, in fact, correct.

Closing, and perhaps this ticket's presence will help someone else in the future should they run into the same problem.