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.47k stars 1.55k forks source link

`excessive_precision` inconsistency with exponential notation #6341

Open EndilWayfare opened 4 years ago

EndilWayfare commented 4 years ago

I love the idea of the excessive_precision lint. I'm working on a parser for a 3rd-party, closed-source file format, and they must have weird float-to-string serialization code. The value in the file doesn't match the one in the UI (e.g. 6.93 serialized to 6.9299999999999997E+00. I know floats are hard and weird, and 6.93 might not be an exact value, but to serialize out a string that's beyond double-precision? Maybe it's technically correct. lexical-core doesn't think so, though). It's really handy to know that my unit test values aren't being silently truncated.

Most cases work just fine. However, I came across a case where using decimal and exponential literals causes clippy to disagree with itself.

#[test]
fn clippy_excessive_precision() {
    let decimal: f64 = 0.004886506780521244;

    // This trips `clippy::excessive_precision`
    let exponential: f64 = 4.886506780521244E-03;

    // Truncated `exponential`
    let clippy: f64 = 4.88650678052124E-03;

    // NOTE: `clippy::float_cmp` warns about the `assert_eq`s, but I'm evaluating binary identity...
    //       so I think this is appropriate?

    assert_eq!(decimal, exponential);

    // `clippy` seems to suggest these should be equal, but they're not
    assert_ne!(exponential, clippy);
    // ... but they ARE within the error margin. Platform issue, maybe?
    assert!((exponential - clippy).abs() < f64::EPSILON);
}

Meta

EndilWayfare commented 4 years ago

OH WAIT. Maybe this is actually/additionally a documentation issue?

I used rust-analyzer's auto-fix thing, and it just inserted underscores without truncating. As if it tripped clippy::unreadable_literal instead? clippy no longer complains.

#[test]
fn clippy_excessive_precision() {
    let decimal: f64 = 0.004886506780521244;

    // This trips `clippy::excessive_precision`
    let exponential: f64 = 4.886506780521244E-03;

    // Auto-fixed `exponential`
    let clippy: f64 = 4.886_506_780_521_244E-3;

    assert_eq!(decimal, exponential);

    // These are equal now, as expected
    assert_eq!(exponential, clippy);
}

The documentation for clippy::excessive_precision doesn't say anything about underscores. The only "Why is this bad" is silent truncation. Also, it doesn't complain about decimal.

giraffate commented 4 years ago

@rustbot modify labels: +L-documentation -L-bug

flip1995 commented 3 years ago

So if you look at the error message carefully, you'll see, that this lint doesn't complain about the fractional part, but about the exponent part for some reason:

warning: float has excessive precision
 --> src/lib.rs:5:28
  |
5 |     let exponential: f64 = 4.886506780521244E-03;
  |                            ^^^^^^^^^^^^^^^^^^^^^ help: consider changing the type or truncating it to: `4.886_506_780_521_244E-3`
  |
  = note: `#[warn(clippy::excessive_precision)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#excessive_precision

If you change ...E-03 to ...E-3 it stops complaining. This is definitely a bug, because this lint should not get triggered by the exponential part.

EndilWayfare commented 3 years ago

OOOH, you're totally right. I was so fixated on the underscores that I didn't notice the zero.

I imagine it's because "precision" is being measured by "count all digit characters in literal after the decimal point"?

flip1995 commented 3 years ago

I imagine it's because "precision" is being measured by "count all digit characters in literal after the decimal point"?

I think we parse the literal and then compare their string representations or something like that, yeah.

ajtribick commented 3 years ago

Probably related to this, it also doesn't like the plus sign being used in exponents. E.g. trying to assign the following to an f64:

let x: f64 = 1.995_840_309_534_719_6e+292; // 0x1.fffffffffffffp+970

results in

let x: f64 = 1.995_840_309_534_719_6e+292; // 0x1.fffffffffffffp+970
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing the type or truncating it to: `1.995_840_309_534_719_6e292`

I personally prefer including the sign as it makes it more obvious where the separation between the sections is. Even if this is something that's worth linting, it's not a precision issue or a case where changing the type is appropriate, and the suggested fix should not be "truncation" (which at least for me suggests cutting off the end of the representation).