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

Lint for sign-ambiguous NAN constants #11720

Open
epage opened this issue Oct 26, 2023 · 5 comments
Open

Lint for sign-ambiguous NAN constants #11720

epage opened this issue Oct 26, 2023 · 5 comments
Labels
A-lint Area: New lints

Comments

@epage
Copy link

epage commented Oct 26, 2023

What it does

Check if a f32::NAN or f64::NAN does not have a sign explicitly set (a more general version of #11717)

Advantage

In reviewing toml-rs/toml#637, I found a ambiguously-signed NAN was still left in. This would help catch that.

From toml-rs/toml#637

I learned recently in rust-lang/miri#3139 that as produces NaN with a nondeterministic sign, and separately that the sign of f64::NAN is not specified (may be a negative NaN). As of rust-lang/rust#116551 Miri has begun intentionally exercising these cases.

This PR fixes places where -nan would incorrectly be deserialized as a positive NaN, nan or +nan would be deserialized as a negative NaN, negative NaN would be serialized as nan, or positive NaN would be serialized as -nan. It adds tests to confirm the expected sign of NaN values, and improves existing tests related to NaN.

Drawbacks

This would be very noisy and most people won't care. This would likely be a pedantic lint

Example

let foo = f64::NAN;

Could be written as:

let foo = f64::NAN.copysign(1.0);
@RalfJung
Copy link
Member

RalfJung commented Oct 26, 2023

TBH this sounds more like a feature request to t-libs-api to guarantee that f32/64::NAN be defined to be positive? We could easily make this guarantee if there is sufficient motivation (though maybe there are objections I am not aware of).

That would then also imply that -f32/64::NAN is negative, since - is one of the operations that produces a well-defined sign even on NaN -- thus making #11717 unnecessary.

@dtolnay
Copy link
Member

dtolnay commented Oct 28, 2023

I am open to guaranteeing that f32/64::NAN has positive sign. I didn't immediately see a way to implement this, since copysign is not available in libcore. rust-lang/rust#50145 "they’re implemented by calling LLVM intrinsics, and it’s not clear whether those intrinsics are lowered on any platform to calls to C’s libm or something else that requires runtime support that we don’t want in libcore"

error[E0599]: no method named `copysign` found for type `f32` in the current scope
 --> src/lib.rs:1:31
  |
1 | pub const NAN: f32 = f32::NAN.copysign(1.0);
  |                               ^^^^^^^^ method not found in `f32`

f32::from_bits also does not work.

error[E0080]: evaluation of constant value failed
    --> ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/f32.rs:1216:21
     |
1216 |                     panic!("const-eval error: cannot use f32::from_bits on NaN")
     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'const-eval error: cannot use f32::from_bits on NaN', /rustc/2f1bd0729b74787f55d4cbc7818cfd787cd43a99/library/core/src/num/f32.rs:1216:21
     |
note: inside `core::f32::<impl f32>::from_bits::ct_u32_to_f32`
    --> ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/f32.rs:1216:21
     |
1216 |                     panic!("const-eval error: cannot use f32::from_bits on NaN")
     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `core::f32::<impl f32>::from_bits`
    --> ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/f32.rs:1233:18
     |
1233 |         unsafe { intrinsics::const_eval_select((v,), ct_u32_to_f32, rt_u32_to_f32) }
     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `NAN`
    --> src/lib.rs:2:22
     |
     | #![feature(const_float_bits_conv)]
2    | pub const NAN: f32 = f32::from_bits(0x7FC0_0000);
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)

@dtolnay
Copy link
Member

dtolnay commented Oct 28, 2023

⁉️

#![feature(const_mut_refs)]

pub const NAN: f32 = {
    let mut nan: f32 = 0.0f32 / 0.0f32;
    *unsafe { &mut *(&mut nan as *mut f32 as *mut u32) } &= !0x8000_0000;
    nan
};

@RalfJung
Copy link
Member

RalfJung commented Oct 28, 2023

I didn't immediately see a way to implement this, since copysign is not available in libcore.

Ah, I didn't realize that. Copysign is just bit manipulation though, we could totally make sure that it is available in libcore. We might just have to implement the intrinsic ourselves I guess? Or we can ask LLVM to promise it won't use libcalls.

f32::from_bits also does not work.

Yeah that's a safety hatch against NaN weirdness, I'm not sure we actually need it but until rust-lang/rfcs#3514 is accepted we're being as conservative as we can be while remaining backwards-compatible.

You can use transmute though...

@epage
Copy link
Author

epage commented Oct 29, 2023

Considering how rare caring about the sign of nan should be, I wonder if we can come up with good patterns of people doing it to warn them away from it by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

3 participants