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

Reduce clippy's complaints about valid decimals #7691

Closed
wants to merge 1 commit into from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Sep 19, 2021

Followup to my hunch in #7666 (comment). I found it was indeed triggering too early in general by naively using the f{32,64}::DIGITS value.

changelog: [excessive_precision] should complain less about valid decimal strings that can be precisely parsed as floats, which clippy was erroneously recommending that you cut 1~3 digits short.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 19, 2021
@workingjubilee
Copy link
Member Author

Sigh. Of course the wrong answer is encoded in the test suite several times over.

@bors
Copy link
Contributor

bors commented Sep 27, 2021

☔ The latest upstream changes (presumably #7722) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines -1 to -5
error: float has excessive precision
--> $DIR/excessive_precision.rs:15:26
|
LL | const BAD32_1: f32 = 0.123_456_789_f32;
| ^^^^^^^^^^^^^^^^^ help: consider changing the type or truncating it to: `0.123_456_79_f32`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change is faithful to the purpose of this lint. Why would we not suggest replacing 0.123_456_789 with 0.123_456_79, which eliminates the extraneous precision and still represents the identical floating point quantity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I have been wondering about that a little to myself, on reading the code at first I was kind of stunned and confused by the naming of the vars and fn so I didn't examine it closely enough to find all the hidden catches. So let me explain my reasoning more thoroughly. Part of it is somewhat esoteric and may be a better case for recommending this be altered in category, from a style lint to a restriction lint.

It is actually considered "good style" in many situations to use 9 or 17 significant digits. It is necessary for encoding the longer sequences, and faithful replication of the binary value with those 9 or 17 digits is what IEEE754 guarantees. This also matters for, well, the reason that I was alarmed at just using the constant directly: it is common for the character strings of numbers to be blithely copied from language to language. Indeed, most of Rust's float-related constants seem to be lifted from C's <float.h>.

While most language's atoi or equivalent is fairly faithful, some languages have less correct float parsers than Rust's, especially at the edges. So it is possible to introduce floating point error due to variations in parsing, rounding, and formatting between compilers and runtimes. As a general guard against this problem, "over" specifying by a few chars is common, or at least using all 9 or 17 digits. For instance, consider the string values used in Boost, Rust, and OpenCL for pi:

let boost_pi = 3.141592653589793238462643383279502884e+0; // triggers excessive_precision
let rust_pi = 3.14159265358979323846264338327950288f64; // triggers excessive_precision also!
let opencl_pi = 3.141592653589793115998; // triggers excessive_precision AND approx_constant!

// They of course are the same binary value.
assert_eq!(rust_pi, boost_pi);
assert_eq!(rust_pi, opencl_pi);
assert_eq!(rust_pi, std::f64::consts::PI);

Thus, I suppose my disagreement is with the lint per se, or at least what it currently encourages and allows to be machine-applied, since it would likely be preferable to upgrade the type to f64 for many f32 triggers of this lint.

Comment on lines -75 to -79
error: float has excessive precision
--> $DIR/excessive_precision.rs:41:26
|
LL | let bad64_suf: f64 = 0.123_456_789_012_345_67f64;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing the type or truncating it to: `0.123_456_789_012_345_66_f64`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cases like this are fixed a different way by #7722.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. That change does make me considerably happier with the lint.

@llogiq
Copy link
Contributor

llogiq commented Oct 10, 2021

@workingjubilee Sorry for procrastinating on this for so long. What's the status on this PR? Do you intend to go forward with it?

@llogiq
Copy link
Contributor

llogiq commented Oct 26, 2021

@workingjubilee ping?

@workingjubilee
Copy link
Member Author

Augh, I don't know. I still have my dissatisfaction with this lint but my irritation has become more vague and I am not sure how I truly want to proceed anymore. It truly irks me that fairly standard code that appears in many codebases, and for good reasons, is linted on. But there are also useful reasons for the lint. Perhaps I want to change it to a restriction lint instead, but I can't seem to muster the internal quorum to agree with myself on that way forward either.

Closing for now, unless you have some particular feedback / opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants