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

Deprecate stdlib modules dedicated to numeric constants and move those constants to associated consts #2700

Merged
merged 13 commits into from Jan 23, 2020

Conversation

@bstrie
Copy link
Contributor

@bstrie bstrie commented May 13, 2019

Add the relevant associated constants to the numeric types in the standard library, and consider a timeline for the deprecation of the corresponding (and originally intended to be temporary) primitive numeric modules and associated functions.

Rendered

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented May 13, 2019

Thanks @bstrie for extensively documenting the intent that we collectively had all along. I don’t expect this to be controversial, but this is what the FCP period is for:

@rfcbot fcp merge

@rfcbot
Copy link

@rfcbot rfcbot commented May 13, 2019

Team member @SimonSapin 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.

@Ekleog
Copy link

@Ekleog Ekleog commented May 13, 2019

Does it really make sense to have f64::PI as an associated const? While I totally agree for min, max and the like (that really define the type), things like PI, LN_2, LN_10, FRAC_2_SQRT_PI etc. are less obvious to me.

Copy link
Contributor

@Centril Centril left a comment

Some thoughts...

Also, I think it's premature to jump to PFCP so quickly when no discussion has been had at all.

text/0000-associated-constants-on-ints.md Outdated Show resolved Hide resolved
text/0000-associated-constants-on-ints.md Outdated Show resolved Hide resolved
@liigo
Copy link
Contributor

@liigo liigo commented May 14, 2019

Replace each item with an associated const value on the appropriate type

a good rfc requires an explicite list

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented May 14, 2019

I think it's premature to jump to PFCP so quickly when no discussion has been had at all.

I took the links in the motivation section as showing there had been ample discussion elsewhere already. That said, let’s register this as a concern to give more time:

@rfcbot concern more discussion before FCP start

@bstrie
Copy link
Contributor Author

@bstrie bstrie commented May 14, 2019

Does it really make sense to have f64::PI as an associated const? While I totally agree for min, max and the like (that really define the type), things like PI, LN_2, LN_10, FRAC_2_SQRT_PI etc. are less obvious to me.

@Ekleog , I have added language to the section on alternatives further elaborating on the rationale here.

Updates to the text regarding other concerns are forthcoming, as my busy schedule playing Minecraft with my girlfriend permits.

@newpavlov
Copy link
Contributor

@newpavlov newpavlov commented May 14, 2019

While I support deprecation of module-level constants in favor of associated constants (also I think we should add std::time::UNIX_EPOCH to the list), I think we should first allow import of associated constants or otherwise it may be an ergonomic hit, e.g. use std::time::SystemTime::UNIX_EPOCH; will not work today.

@sfackler
Copy link
Member

@sfackler sfackler commented May 15, 2019

@newpavlov
Copy link
Contributor

@newpavlov newpavlov commented May 15, 2019

Yes, I know. My point is about marking std::time::UNIX_EPOCH as deprecated and allowing associated constants import before module-level constants deprecation.

@bstrie
Copy link
Contributor Author

@bstrie bstrie commented May 15, 2019

Though note that the inability to bring associated items into scope via use doesn't preclude aliasing them in other ways. If someone's using un-prefixed UNIX_EPOCH in their code, then that would require them to already have a line like:

use std::time::UNIX_EPOCH;

...which, if std::time::UNIX_EPOCH were deprecated in favor of the associated const std::time::SystemTime::UNIX_EPOCH, then the one and only change that would need to be made to any module with the aforementioned import would be to change the use line to:

use std::time::SystemTime;
const UNIX_EPOCH: SystemTime = SystemTime::UNIX_EPOCH;

Furthermore, because all primitive types are already in the prelude, doing the same transition for the numeric types as proposed in this RFC would be even easier, requiring only a line such as:

use std::i32::MAX;

...to become:

const MAX: i32 = i32::MAX;

Finally, whereas I can totally see someone wanting to use UNIX_EPOCH as an un-prefixed identifier, I think it is much less likely that someone would import constants like MAX or MIN to use un-prefixed, precisely because there are no less than twelve different maxes and mins in the stdlib, and the types are very useful for mental disambiguation (in addition, the types are extremely concise, unlike SystemTime).

So while it would be nice to be able to import associated consts, (and associated items in general; has anyone ever submitted an RFC for this?) I don't necessarily think it will be such a hindrance in this case if we chose not to wait until that were possible.

@Centril
Copy link
Contributor

@Centril Centril commented May 15, 2019

Finally, whereas I can totally see someone wanting to use UNIX_EPOCH as an un-prefixed identifier, I think it is much less likely that someone would import constants like MAX or MIN to use un-prefixed, precisely because there are no less than twelve different maxes and mins in the stdlib, and the types are very useful for mental disambiguation (in addition, the types are extremely concise, unlike SystemTime).

@bstrie Sure, it seems clear that u8::MAX is more understandable than MAX. For other constants the matter seems different. I would suggest cutting back to just MAX & MIN.

@bstrie
Copy link
Contributor Author

@bstrie bstrie commented May 15, 2019

@Centril, note that all the integral modules contain only MAX and MIN, so reducing the RFC's scope to those constants would be equivalent to the alternative discussed in the RFC where these changes would be made only to the integral modules and not for the float modules. (Although personally I find the visual difference between f32::PI and f64::PI to be just as important, to say nothing of the hypothetical f16::PI, f80::PI, and f128::PI that we may have someday).

I'll summarize this discussion and add it to the Drawbacks section.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

There is an alternative design where the proposed changes are only made to the integral numeric modules in the standard library, leaving alone `f32` and `f64`. Unlike the integral modules, these modules do not contain both constants and redundant associated items. In addition, these two modules contain submodules named `consts`, which contain constants of a more mathematical bent (the sort of thing other languages might put in a `std::math` module). This RFC argues for giving the float modules the same treatment as the integral modules, both since associated consts are "obviously the right thing" in this case and because we do not consider the mathematical/machine constant distinction to be particularly useful or intuitive. In particular, this distinction is not consistent with the existing set of associated functions implemented on `f32` and `f64`, which consist of a mix of both functions concerned with mathematical operations (e.g. `f32::atanh`) and functions concerned with machine representation (e.g. `f32::is_sign_negative`). As noted, if a `math` module existed in Rust's stdlib this would be the natural place to put them, however, such a module does not exist; further, any consideration of this hyptothetical module would, for the sake of consistency, want to also adopt not only the aforementioned mathematical associated functions that currently exist on `f32` and `f64`, but would also want to adopt the integral mathematical functions such as `i32::pow`--all while in some way recreating the module-level distinction between the operations as they exist on the various different numeric types. This is all to say that such a `std::math` module is out of scope for this proposal, in addition to lacking the technical motivation of this proposal. Ultimately, however, leaving `f32` and `f64` along and making the proposed changes only to the integral types would still be considered a success by this RFC.
Copy link
Contributor

@Centril Centril May 16, 2019

(please use linebreaks or semantic linefeeds...)

This RFC argues for giving the float modules the same treatment as the integral modules, both since associated consts are "obviously the right thing" in this case [...]

Please make an effort beyond "obviously the right thing". It's often not obvious, even if you think it is.

@scottmcm
Copy link
Member

@scottmcm scottmcm commented May 17, 2019

Yes please! I always just type u32::MAX then get sad when the compiler reminds me it doesn't work...

@avl
Copy link

@avl avl commented May 21, 2019

Also, f32::PI would be very nice! The chance that someone ever types that intending something other than the mathematical constant Pi seems infinitesimal. I think Pi and other mathematical constants are fundamental enough that they don't deserve being shuffled out of sight to a 'constants' namespace.

@avl
Copy link

@avl avl commented May 21, 2019

Couldn't deprecation wait til next edition (2021?). But introduce the new names now?

@Centril
Copy link
Contributor

@Centril Centril commented May 26, 2019

@SimonSapin I think it's fair to say that the discussion has slowed down at this point. :)

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented May 27, 2019

Indeed!

@rfcbot resolve more discussion before FCP start

However I think that the regression of not being able to import these constants is a valid concern. At least for the math constants, since for example MAX without context is arguably not a good idea.

@rfcbot concern importing

@bstrie, What do you think of having the RFC propose adding associated constants now, but only deprecating the other constants after associated items can be imported with use?

I couldn’t find an RFC for that, anyone would like to write it? https://internals.rust-lang.org/t/importing-associated-constants/6610 asked for it, and https://rust-lang.github.io/rfcs/0195-associated-items.html#scoping-of-trait-and-impl-items suggests it as a future extension.

@Centril
Copy link
Contributor

@Centril Centril commented May 28, 2019

cc @petrochenkov @rust-lang/lang ^----

@faern
Copy link
Contributor

@faern faern commented Jun 28, 2019

If this is blocked on the import issue I agree with some other people above that suggest we add the associated constants now and deprecate the module constants when imports are possible.

We can deprecate min_value/max_value now since they also can't be imported in the same way associated constants can't.

@faern
Copy link
Contributor

@faern faern commented Jan 18, 2020

Instead of inventing a new way of deprecating that does not produce warnings, we could just "soft deprecate" the old constants for a few versions first, the way Error::description was between 1.37 and 1.40. However, it feels like very little existing code using description was actually updated until very recently when it was fully deprecated on nightly. Then people suddenly got the energy to actually adapt. So I feel the only way to make a change is deprecation, and it seems we agree we want people to use the new way, so it's pretty obvious to me what to do 🤷‍♂

I agree with @bstrie that it would be absurd for libstd to provide three ways of doing the same thing. And remember that the first two were introduced explicitly as temporary solutions that people back in the day agreed were bad solutions. They should have been deprecated years ago, better late than never.

@Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jan 18, 2020

Deprecation is just a warning by default right? And all that a person does to fix this particular deprecation is search and replace some paths for some constants?

Just do it. Rip the band-aid off.

@Ixrec
Copy link
Contributor

@Ixrec Ixrec commented Jan 18, 2020

Is rustfix in a state where we can use it to make "trivial" migrations like this actually trivial?

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jan 18, 2020

To me "My primary reason for pushing this RFC is that I want writers to not have to think about this issue at all; what means do we have of encouraging writers to use these constants other than deprecation?" argues for not deprecating them immediately, as deprecation forces writers to think about which path they should be using.

I am not opposed to soft-deprecating these constants. I would not even be opposed to a deprecation date tied to an edition, with the lint being auto-fixable (with presumably some compiler magic); for that matter if we deprecate them (as I believe is usual) 2-3 releases after the replacements land so that crates on stable can transition without needing #![allow(deprecated)] or so, that would also be mostly fine with me.

But I don't think deprecating anything immediately makes sense, as that makes the transition painful for everyone.

@Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Jan 18, 2020

But it's (1) not actually painful to have a warning (2) totally solvable with search and replace.

People aren't being told "use this new function that works slightly differently", it's an identical value just at a new item path.

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jan 18, 2020

You're not wrong that it's likely solvable in an easy way, but to me that doesn't mean we should just deprecate immediately. I don't see any advantage to doing so -- indeed, would be fine with 3 ways to access max/min values (std::u32::MAX, u32::max_value() and u32::MAX) indefinitely. I don't really personally believe that this adds confusion to users -- it seems like when reading code, independent of which of these is used, it is obvious what is happening, the fact that there's 3 ways of doing it doesn't really hurt.

In any case, I think I've expressed my viewpoint sufficiently and we're spinning at this point.

I would personally be in favor of merging a PR implementing the new constants (I think there's one already up?) without deprecating the old ones immediately, and we can separately continue discussing, perhaps on an issue, when to deprecate the old variants.

@programmerjake
Copy link

@programmerjake programmerjake commented Jan 18, 2020

One concern with immediate deprecation is that people supporting multiple versions of Rust with the same source would be forced to either disable deprecation warnings or immediately switch to the new version which wouldn't compile on older versions (since it's not there yet). Both of those don't seem like a good migration path.

@programmerjake
Copy link

@programmerjake programmerjake commented Jan 18, 2020

I would be in favor of a PR implementing the new constants without marking the old constants
#[deprecated] just yet, but the documentation should say that the old constants are deprecated. They can be #[doc(hidden)] after adding the #[deprecated] annotation.

@faern
Copy link
Contributor

@faern faern commented Jan 19, 2020

I would personally be in favor of merging a PR implementing the new constants (I think there's one already up?)

Yes. I prepared and submitted rust-lang/rust#68325 with just this content.

@bstrie
Copy link
Contributor Author

@bstrie bstrie commented Jan 20, 2020

Am I correct in drawing the following conclusions from the deprecation thread above:

  1. We should not deprecate the old constants immediately.
  2. We should deprecate the old constants eventually.

If this seems agreeable, then I submit that this RFC could be approved and merged at any time. A timeframe for deprecation, however long it might be or whatever form it might take, can easily be left to the tracking issue.

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jan 20, 2020

It is my opinion that (presuming that was the unresolved issue), this RFC is ready for merging, per your comment's summary of the deprecation question.

@dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 20, 2020

I concur; we have accepted the RFC, we can add the associated constants now, and we loosely plan to deprecate but will negotiate the details later.

Personally I hold out some hope that we can figure out a way to make std::i32 etc be a reexport of the type i32 instead of a module, at which point there is no longer anything to deprecate.

@SimonSapin SimonSapin merged commit fa53bf8 into rust-lang:master Jan 23, 2020
@LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Jan 23, 2020

This RFC was approved!

Please use the tracking issue for further discussion. Thanks!

bors added a commit to rust-lang/rust that referenced this issue Jan 24, 2020
…-step1, r=<try>

Move numeric consts to associated consts step1

A subset of #67913. Implements the first step of RFC rust-lang/rfcs#2700

This PR adds the new constants as unstable constants and defines the old ones in terms of the new ones. Then fix a tiny bit of code that started having naming collisions because of the new assoc consts.

Removed a test that did not seem relevant any longer. Since doing just `u8::MIN` should now indeed be valid.
bors added a commit to rust-lang/rust that referenced this issue Jan 30, 2020
…-step1, r=LukasKalbertodt

Move numeric consts to associated consts step1

A subset of #67913. Implements the first step of RFC rust-lang/rfcs#2700

This PR adds the new constants as unstable constants and defines the old ones in terms of the new ones. Then fix a tiny bit of code that started having naming collisions because of the new assoc consts.

Removed a test that did not seem relevant any longer. Since doing just `u8::MIN` should now indeed be valid.
bors added a commit to rust-lang/rust that referenced this issue Jan 30, 2020
…-step1, r=LukasKalbertodt

Move numeric consts to associated consts step1

A subset of #67913. Implements the first step of RFC rust-lang/rfcs#2700

This PR adds the new constants as unstable constants and defines the old ones in terms of the new ones. Then fix a tiny bit of code that started having naming collisions because of the new assoc consts.

Removed a test that did not seem relevant any longer. Since doing just `u8::MIN` should now indeed be valid.
bors added a commit to rust-lang/rust that referenced this issue Mar 4, 2020
Stabilize assoc_int_consts associated int/float constants

The next step in RFC rust-lang/rfcs#2700 (tracking issue #68490). Stabilizing the associated constants that were added in #68325.

* Stabilize all constants under the `assoc_int_consts` feature flag.
* Update documentation on old constants to say they are soft-deprecated and the new ones should be preferred.
* Update documentation examples to use new constants.
* Remove `uint_macro` and use `int_macro` for all integer types since the macros were identical anyway.

r? @LukasKalbertodt
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 11, 2020
…, r=dtolnay

Migrate to numeric associated consts

The deprecation PR is rust-lang#72885

cc rust-lang#68490
cc rust-lang/rfcs#2700
RalfJung added a commit to RalfJung/rust that referenced this issue Jun 12, 2020
…, r=dtolnay

Migrate to numeric associated consts

The deprecation PR is rust-lang#72885

cc rust-lang#68490
cc rust-lang/rfcs#2700
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 12, 2020
…, r=dtolnay

Migrate to numeric associated consts

The deprecation PR is rust-lang#72885

cc rust-lang#68490
cc rust-lang/rfcs#2700
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 13, 2020
Migrate to numeric associated consts

The deprecation PR is #72885

cc #68490
cc rust-lang/rfcs#2700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet