-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
deprecate f{32,64}::DIGITS #89238
deprecate f{32,64}::DIGITS #89238
Conversation
r? @m-ou-se (rust-highfive has picked a reviewer for you, use r? to override) |
db901e9
to
4041f58
Compare
These constants are misleading: the number of significant digits varies for each value that these floating point numbers may encode but some programmers are taking them directly as an upper bound. This is wrong and is leading to programmers creating applications that directly mislead other users about their meaning, having a negative ecosystem-wide impact on mathematical accuracy. To contain the damage, deprecate them without replacement. It is hoped this will force programmers to reevaluate their use.
4041f58
to
4b8bef3
Compare
#[rustc_deprecated(since = "TBD", reason = "replaced by the `DIGITS` associated constant on `f32`")] | ||
#[rustc_deprecated( | ||
since = "TBD", | ||
reason = "this value is incorrect or misleading as it actually encompasses a range" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really tiny nit, but I feel like the word 'encompasses' is misleading here. I might read that and think that the deprecated value is the upper bound, and therefore safe.
Maybe change it to say "the actual number of significant digits varies over a range, and this value is the minimum number of significant digits" or something else that makes the problem clear.
What about all the other constants? (I guess some of them would need replacements though) |
@rfcbot merge |
Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Yeah, no need to handle them as a single blanket. These got entered as a group when they probably should have been evaluated on a more case-by-case basis in the first place, or even RFCed so let's not repeat that mistake. |
I haven't read the linked issue, but I've read this issue as well as the deprecation message and updated docs, and I'm still not sure I understand what's going on here. In particular, I think my beef with the docs is that it's a tease. They say "usage is likely incorrect," but don't really elaborate. Maybe we can get some more details, and even better, an example? |
Is there some particular reason these can't just be changed to the appropriate upper-bound values? |
Of course, @BurntSushi. An f32 can encode up to 9 significant decimal digits. And the current name and value can lead to misleading conclusions. For example: Both numbers also have a But then the documentation is not much help as it says it is "approximate", but doesn't specify how. As far as a practical example of why it is bad: fn main() {
// A sequence of unique floats, printed by Rust with varying numbers of significant digits.
let a: Vec<f32> = vec![
0.1000734,
0.100073405,
0.10007341,
0.10007342,
0.10007343,
0.100073434,
0.10007344,
0.10007345,
0.10007346,
0.100073464,
0.10007347,
0.10007348,
0.10007349,
0.100073494,
];
for f in a {
println!("{:.1$}", f, f32::DIGITS as usize); // All chopped down to the same string: 0.100073
}
} As this is a somewhat unusual formatting arg choice, you may wish to replace it with your choice of more custom string-length-evaluation code, et cetera, that uses @joshtriplett Note that a. allows d. to be done later. |
@rfcbot concern detailed explanation +1 from me on this change but I'd also like to see more detail added to the deprecation message and documentation as mentioned by @BurntSushi. Regarding the options you presented at the end of your last comment @workingjubilee I'd probably lean towards (c) or (d) through (a) as an initial step. IMO deprecate the constants now and then base any more resilient impl we add in the future on concrete usecases. |
The reason I can think of for these constants to be as they are is compatibility with the C standard library's |
@workingjubilee Any updates here? |
Since this is only waiting on a small update to the docs we're going to let the fcp move forward and use the github UI to track that changes still need to be made prior to merging. @rfcbot resolve detailed-explanation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just needs a minor clarification to the docs and then to set the since
fields I believe.
/// Approximate number of significant digits in base 10. | ||
/// Use [`f32::DIGITS`] instead. | ||
/// A minimum number of significant digits in base 10. | ||
/// This value is likely incorrect for usage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just need to elaborate on this and add a small example of how this can be incorrect.
@@ -381,8 +388,14 @@ impl f32 { | |||
#[stable(feature = "assoc_int_consts", since = "1.43.0")] | |||
pub const MANTISSA_DIGITS: u32 = 24; | |||
|
|||
/// Approximate number of significant digits in base 10. | |||
/// A minimum number of significant digits in base 10. | |||
/// This value is likely incorrect for usage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here as well
/// Approximate number of significant digits in base 10. | ||
/// Use [`f64::DIGITS`] instead. | ||
/// A minimum number of significant digits in base 10. | ||
/// This value is likely incorrect for usage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
/// Approximate number of significant digits in base 10. | ||
|
||
/// A minimum number of significant digits in base 10. | ||
/// This value is likely incorrect for usage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
Sorry, trying to catch up with everything and the part of my brain that was responsible for this particular PR is currently scheduling tasks, hope to catch up with this relatively soon and address concerns. |
☔ The latest upstream changes (presumably #95960) made this pull request unmergeable. Please resolve the merge conflicts. |
@workingjubilee any updates on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the docs changes, but +1 for the solution proposed!
Apologies. I began to question myself a bit reflectively in a normal fashion after initially opening this PR and enumerating the options more distinctly upon prompting, and then I became concerned about the quality of the current upgrade path ("none") following a few remarks, including @yaahc's. Then I became very busy. |
triage: Hi @workingjubilee, I just wanted to check on the status of this PR. |
Closing this as inactive. If you wish to continue you can open a new PR and we can move forward from there |
These constants are misleading: the number of significant digits
varies for each value that these floating point numbers may encode
but some programmers are taking them directly as an upper bound.
This is wrong and is leading to programmers creating applications
that directly mislead other users about their meaning, having
a negative ecosystem-wide impact on mathematical accuracy.
To contain the damage, deprecate them without replacement.
It is hoped this will force programmers to reevaluate their use.
Closes #89106.
This problem may seem trivial but instructions to alter valid float values being generated by an otherwise highly-regarded source (Clippy) is a bad result. While a PR is open against rust-clippy to fix this behavior, this pattern may have arisen in non-indexed code, even though it seems to be uncommon. As the constants don't seem to be greatly used in practice, while the damage they can do if misused is high, it seems reasonable to take this path.