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
11.39k stars 1.54k forks source link

False positive `assertions_on_constants` #8159

Closed rukai closed 10 months ago

rukai commented 2 years ago

Summary

assert!(true) will be optimized out by the compiler is given when asserting on constants. Asserting on constants is very important for ensuring various kinds of invariants are upheld.

The example given in the recent announcement for panic in const contexts functionality doesn't reproduce the issue but the issue can still be reproduced in const contexts.

Lint Name

assertions_on_constants

Reproducer

I tried this code:

const FOO: usize = 100;
const BAR: usize = 0;

pub fn main() {
    assert!(FOO > BAR); // false positive
    const _: () = assert!(FOO > BAR); // false positive
    const _: () = assert!(std::mem::size_of::<u8>() > BAR); // true negative
    const _: () = assert!(std::mem::size_of::<u8>() == 1); // true negative
}

I saw this happen:

 1  warning: `assert!(true)` will be optimized out by the compiler
  --> src/main.rs:5:5
   |
 5 |     assert!(FOO > BAR); // false positive
   |     ^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::assertions_on_constants)]` on by default
   = help: remove it
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
   = note: this warning originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

 2  warning: `assert!(true)` will be optimized out by the compiler
  --> src/main.rs:6:19
   |
 6 |     const _: () = assert!(FOO > BAR); // false positive
   |                   ^^^^^^^^^^^^^^^^^^
   |
   = help: remove it
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants
   = note: this warning originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

 3  warning: `foo` (bin "foo") generated 2 warnings
 4  warning: `foo` (bin "foo" test) generated 2 warnings (2 duplicates)

I expected to see this happen: No warnings

Version

rustc 1.57.0 (f1edd0429 2021-11-29)
binary: rustc
commit-hash: f1edd0429582dd29cccacaf50fd134b05593bd9c
commit-date: 2021-11-29
host: x86_64-unknown-linux-gnu
release: 1.57.0
LLVM version: 13.0.0

Additional Labels

No response

flip1995 commented 2 years ago

Thanks for the report. This is currently expected behavior. The question is if it is the right behavior. I lean towards, that it isn't.

josephlr commented 2 years ago

I agree that this doesn't seem like the correct behavior inside constants. For example, if we want to check that usize is at least 32 bits (to check that u32 as usize is lossless), the simplest way is:

const _: () = assert!(usize::BITS >= 32);

But this triggers the assertions_on_constants lint.

ensc commented 1 year ago

imo, it is ok to trigger on code like

    assert!(FOO > BAR);

but compile-time assertion variants like

    const _: () = assert!(FOO > BAR);

should be accepted.

CAD97 commented 9 months ago

This isn't fully fixed (cc @StackOverflowExcept1on (wrote PR), @Jarcho (r+'d PR)):

const X1P_20: f64 = 9.536_743_164_062_5e-7; // 2^-20 ≅ 10^-6

const _: () = assert!(X1P_20 <= 1.0e-6); // no longer warns :+1:
const _: () = {
    assert!(X1P_20 <= 1.0e-6); // warns :disappointed:
};

As an additional aside, it's probably preferable to not suppress the lint when the const item is an associated item, since those constant evaluation errors only occur when forced (they're only actually evaluated post-monomorphization; never if unused).

StackOverflowExcept1on commented 9 months ago

@CAD97 Good catch. We need to somehow check if assert!() is in a const context. For now it just looks at the parent HIR items. If someone knows how to do this, then write about it. I don't know the entire rust compiler codebase. https://github.com/rust-lang/rust-clippy/blob/a71211d0b52c01f1b37fe544e0e13fd1bdc31979/clippy_lints/src/assertions_on_constants.rs#L48-L56

y21 commented 9 months ago

We should be able to get the enclosing body with tcx.hir().enclosing_body_owner(e.hir_id), then call tcx.hir().body_const_context() with that id. That should tell us if it's a const, a const fn or not const at all