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

False positive assertions_on_constants #8159

Closed
rukai opened this issue Dec 23, 2021 · 6 comments · Fixed by #11966
Closed

False positive assertions_on_constants #8159

rukai opened this issue Dec 23, 2021 · 6 comments · Fixed by #11966
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

@rukai
Copy link
Contributor

rukai commented Dec 23, 2021

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

@rukai rukai 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 Dec 23, 2021
@flip1995 flip1995 changed the title assert!(true) will be optimized out by the compiler False positive assertions_on_constants Dec 23, 2021
@flip1995
Copy link
Member

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

josephlr commented Oct 2, 2022

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.

robertknight added a commit to robertknight/rten that referenced this issue Jan 4, 2023
These values were taken from the Blis kernel configuration for Haswell [1],
Setting NR to use 2 AVX registers takes advantage of the 2 FMA execution ports
[2].

The clippy lint about the compiler optimizing away constant assertions
was disabled because that is exactly the behavior that we want. See also
rust-lang/rust-clippy#8159.

[1] https:/flame/blis/blob/f956b79922da412791e4c8b8b846b3aafc0a5ee0/kernels/haswell/bli_kernels_haswell.h#L55

[2] bluss/matrixmultiply#34 (comment)
@ensc
Copy link

ensc commented Jan 11, 2023

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

CAD97 commented Jan 13, 2024

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

@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.

if let ConstantSource::Constant = source
&& let Some(node) = cx.tcx.hir().find_parent(e.hir_id)
&& let Node::Item(Item {
kind: ItemKind::Const(..),
..
}) = node
{
return;
}

@y21
Copy link
Member

y21 commented Jan 13, 2024

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

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
7 participants