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.07k stars 1.49k forks source link

Lint for `-f64::NAN #11717

Open epage opened 8 months ago

epage commented 8 months ago

What it does

Warn users when negating a floating point NAN.

Advantage

The sign of the NAN constants is undefined, so people are likely expecting a negative NAN when they might end up with a positive NAN.

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

I cannot think of a legitimate use of this where it will do what the user wants. Therefore I recommend this be enabled by default

Example

let value = -f64::NAN;
let value = -f32::NAN;

Could be written as

let value = f64::NAN.copysign(-1.0);
let value = f32::NAN.copysign(-1.0);
RalfJung commented 8 months ago

I learned recently in https://github.com/rust-lang/miri/issues/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 https://github.com/rust-lang/rust/pull/116551 Miri has begun intentionally exercising these cases.

Miri is partially exercising this: f32::NAN.is_sign_positive() will currently always return true. The fact that it can return false is a matter of library API ability and rustc const-eval interpreter choices; this is not runtime non-determinism but merely underspecification.

On the other hand, as casts will indeed get a non-deterministic sign in Miri, and running the same code with different seeds can yield different results.

See https://github.com/rust-lang/rfcs/pull/3514 for a detailed discussion of float NAN semantics.