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

Clippy gives unit_arg warning when using black_box #12707

Closed
xobs opened this issue Apr 24, 2024 · 4 comments
Closed

Clippy gives unit_arg warning when using black_box #12707

xobs opened this issue Apr 24, 2024 · 4 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@xobs
Copy link

xobs commented Apr 24, 2024

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

@xobs xobs 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 Apr 24, 2024
@Alexendoo
Copy link
Member

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

xobs commented Apr 25, 2024

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

y21 commented Apr 25, 2024

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

xobs commented Apr 25, 2024

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.

@xobs xobs closed this as completed Apr 25, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 12, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
compiler-errors added a commit to compiler-errors/rust that referenced this issue Dec 13, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
Zalathar added a commit to Zalathar/rust that referenced this issue Dec 13, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 13, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 13, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 13, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 13, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
Zalathar added a commit to Zalathar/rust that referenced this issue Dec 14, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2024
Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 14, 2024
Rollup merge of rust-lang#133942 - BD103:black-box-docs, r=saethlin

Clarify how to use `black_box()`

Closes rust-lang#133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 15, 2024
Clarify how to use `black_box()`

Closes #133923.

r? libs
^ (I think that's the right group, this is my first time!)

This PR adds further clarification on the [`black_box()`](https://doc.rust-lang.org/stable/std/hint/fn.black_box.html) documentation. Specifically, it teaches _how_ to use it, instead of just _when_ to use it.

I tried my best to make it clear and accurate, but a lot of my information is sourced from rust-lang/rust-clippy#12707 and [manually inspecting assembly](https://godbolt.org/). Please tell me if I got anything wrong!
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 I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

No branches or pull requests

3 participants