There's been a little more activity on the proposed RFC for revamped numeric constants ( https://github.com/rust-lang/rfcs/pull/2700 ) , figured I'd make a thread here in case people find it easier to keep up with than on github. right now the question to be resolved seems to be whether or not to ever deprecate the constants that it proposes to replace; my stance is that I don't care if these constants get deprecated for a good long while, but I do want to guarantee that they will be deprecated eventually, though I'm not picky at how long that pre-deprecation period may be. cc @faern
ultimately my motivation is that I really care that people searching the docs for "max" know which constant we expect them to use; I'm not especially concerned with making existing code throw warnings or feel pressured to update. this is intended to be an improvement to ergonomics and beginner ease. if there is some established solution to this other than deprecation, I would love to know about it!
i think deprecating them eventually is probably fine, in accordance with whatever our normal procedures are. we've deprecated (what feels like) a lot of stuff at a fairly consistent pace, and i feel like not doing it for this would be a break from that norm.
i do still think it would be nice to stop being so deprecation happy, but can really appreciate the arguments in favor of doing it. a good middle road here might be to just make the period in which the replacement API exists without a corresponding deprecation be longer.
cc @Simon Sapin since i know he was expressing opinions on this.
Right, a good thing about
#[rustc_deprecated(since = "…")] is that we can make the warnings start in releases arbitrarily far in the future
I think there’s a whole spectrum of consequences to old code that keeps using something that has a better alternative. For example
std::mem::unitialized is often UB.
str::trim_right is named backwards if you’re doing Arabic text layout, so
trim_end can make code easier to understand. But
i32::MAX instead of
std::i32::MAX… IMO the difference feels very marginal and supperficial
How those various cases should translate exactly in terms of when to emit deprecation warnings, I don’t have a strong opinion
Maybe we should add a mechanism to make something deprecated only in a new edition, for the very low-impact ones. Edition migration is typically when we’re already doing busywork
@bstrie BTW when there’s only part of a proposal that has consensus (in this case the new constants) sending a first PR for that helps have it sooner and doesn’t mean rejecting the rest
@Simon Sapin by "sending a PR" are you referring to having two separate RFCs, one for adding the new constants and another for deprecating them, or are you referring to rustc PRs? if the latter, then I believe @faern has already updated their rustc PR to add in these new constants without deprecating the old ones
Implementation PR, not RFC
I'm just about to push an update to the RFC which shuffles around the steps to imply that we could add the new constants and then decide upon deprecation sometime in the future
I don't know if there currently exists a way to say "this item will only be seen as deprecated to users of an edition >= $EDITION_X", but I'm also all for that if we're worried about churn
after all, we're not that far out from a new edition... I can wait eighteen months :P
i really like the idea of pushing some subset of deprecations to a new edition.
would be good to keep that in mind as edition planning advances this year, although it might be out of scope to consider the exact mechanism for that right now :P
Talking about deprecating stuff, I just remembered that we forgot to deprecate
compare_and_swap after we added
compare_exchange_weak. This was back in ~1.12.0.
IIRC we weren't super gung-ho about deprecating compare_and_swap quickly when stabilizing compare_exchange due to the added complexity of thinking about success and failure orderings, and then it fell out of mind.
Ping? did this topic become empty now after the split?
The RFC2700 thread is now empty? Or I'm not understanding Zulip.
I've moved messages around so that they're in the correct topics.
When #t-libs > Deprecating compare_and_swap was forked off, it appears that messages from before the discussion shifted to deprecating
compare_and_swap were moved too, so I've moved those back.
Ok. Copying my message over which was for this thread:
I'm also fine with deprecating in the next edition. Just like @bstrie I only care about that it will eventually be done. However, I think we should update the docs for the old ways of reaching the consts immediately upon stabilizing them. Something like a soft deprecation. So people can immediately see which ones they should pick if they are writing new code, without giving warnings to existing code.
Yep, I agree that the docs should do so (nightly docs can essentially all point to the new locations)
A question not very discussed is when to stabilize the new constants? In my first PR I just stabilized them instantly since I also deprecated the old ones instantly. But in the new smaller PR that just introduces the constants, they are introduced as unstable. I can adapt my PR either way. But I assume we want to get it merged as unstable for at least a while first to make sure it seems to work at all :)
hm, yes, though I imagine we'll want to move towards stabilization relatively quickly
certainly until they're stable I would feel odd even soft-deprecating the old ones
Certainly. We of course can't say that the old ones are not the recommended ones until there are usable replacements on stable. I agree to that. As I wrote above, clearly indicate to users to use the new ones when we stabilize them.
So if I understand correctly we do have a PR which just adds the new constants, not changing the old ones, in any way, right? https://github.com/rust-lang/rust/pull/68325
Exactly! Except they redefine the old ones to use the values from the new ones in order to keep a single source of truth.
I imagine though that redefinition is at fault for the test failure (since rustdoc is plausibly not being recursive)
Maybe we can flip the "canonical" to be the old ones for now?
Or someone tell me how to fix the test? That feels more productive than doing work in this PR to reverse it and then having to fix the test anyway and reverse it once again further down the road.
hm, yeah, trying to investigate the test itself now
I can't seem to find
PI at all in that test
I don't even know how to run it locally. So very slow to iterate towards the CI.
./x.py test --stage 1 src/test/rustdoc should work
you did modify the test case which is surprising to me
Maybe to try to debug?
Well, yes. Currently in the PR I remove the test. I tried various ways to fix it before that.
Oh I see https://github.com/rust-lang/rust/commit/bd51e02c7ab5eb27787da407e95d5b37cd9ba0d2 was what CI comment is about
Yup. Tried to switch to some other constant which was not an associated one. Since I assume the test was just about re-exporting a constant. That it was EPSILON was irrelevant I guessed.
Yes, that's true looks like it should be irrelevant
I would run it locally, and look in
build/<triple>/test/rustdoc/show-const-contents/ in your web browser to try and figure out what should go there
Uhm. Ok. I got the @has test to work. But the @!has still fails. Not exactly sure what that tries to test for. That the documentation does not contain
; //? Well, the one for PI does :shrug:
Probably worth looking at the master-generated docs to try and see what that is looking for
might be e.g. testing that we don't show a comment or something like that
It would seem like that's what it tests for. However, it does show the comment in my generated html. Not sure why it would suddenly do that for PI if it did not for EPSILON before.
I don't know either :)
I don't have a lot of time to dig into this question myself, but we would likely want to answer it before changing the test.
I pushed a new fix. It does test that the output is the same as I can see on https://doc.rust-lang.org/nightly/std/f32/consts/constant.PI.html. So I guess all is well then? If the rustdoc people don't want the docs to contain what it currently contains that's not a problem for this PR
On nightly EPSILON does not generate a comment after the value, but PI does. I guess it's something about the definition of the value being short enough or something.
hm, okay, let's just drop the !has test for epsilon
I'm pretty happy with the PR. I think the only thing missing is an issue number for the unstable attribute. So we would need a tracking issue I assume. But I guess we should get the RFC merged first. Anything left there?
A question not very discussed is when to stabilize the new constants?
Is it possible to write code that works on older Stable Rust (with the old constants), in such a way that the new constants would shadow the old ones after they’re added? (Maybe
use std::i32; i32::MAX?) If adding unstable constant regress that code we may need to add them as insta-stable
@Simon Sapin is there a
#[rustc_deprecate(edition = "...")] that deprecates only after a particular edition ?
that way people writing code using older editions won't have to deal with the warnings
There is not, that I know of
But I think it could be interesting to add
I suppose it can be added when it is needed
I personally don't mind that much about whether the churn of deprecating things for consistency purposes is worth it or not, since it shouldn't really impact old code unless it has a
deny(warnings), in which case the authors wanted that code to break when these things happen
what we could do in a future edition is make it an error to use those
that feels more consistent with what we have been doing until now (deprecate in current edition, turn that into an error in the next one)
from that point-of-view, the deprecation warning "warns" that it will become an error in the next edition, and
cargo fix can trivially fix these
@Simon Sapin doesn't name resolution always prefer anything explicitly
used before checking in the prologue, which is where the new constants would be defined? in any case I don't see how even intermixing them would ever possibly be a problem, since not only are they the same type, the old constants would be changed to be just re-exports of the new ones
I don’t know, but either way I’d feel better with a test.
And yeah, previously-valid code starting to use the new constant is only a problem if the new ones are
#[unstable]. Once they are stable it’s not a problem since they have the same respective types and values
@Simon Sapin I tried what you suggested, and it seems to not be a problem. I used a build of the compiler/std from the branch in my PR and when I
use std::u16 and later access
u16::MAX it compiles without any warning. But if I remove the
use then I get an error that
u16::MAX is unstable. So existing code should not get errors that they use unstable library items.
Thanks for trying it out! It still sounds like a good test to add, if there isn’t one already :)
Add a test that tries to use
std::u16::MAX and make sure it works without unlocking a feature? Seems like a very temporary test to me. We will likely stabilize the constants very soon after adding them, they just need to reach nightly a little while so we see if it causes any problems or not.
@Simon Sapin I added it. Is this what you had in mind, or did I misinterpret you? https://github.com/rust-lang/rust/pull/68325/commits/6438938d450fe3abdb5abbead3ad14823e4e75f6
Yes exactly, thanks
Great. Then I'm not sure what we are waiting for any longer. No one seems to oppose the RFC and we seem to agree approximately how to implement it. Could we have the RFC merged? If not is there anything I can do now to help unblock that?
Yeah I'm a bit confused about why the RFC isn't merged yet. The FCP is finished and the discussion afterwards should happen on the tracking issue, right? @Simon Sapin Should I simply merge it and create a tracking issue?
(And sorry, forgot to check Zulip this week :<)
Yes, go ahead. Merging a creating a tracking issue is a manual process.
@Lukas Kalbertodt take a look at how I have merged RFCs in the past btw
@centril There is also this chapter in the Rust forge
I guess you always did it pretty similarly to that?
@Lukas Kalbertodt I think you linked to the wrong PR in the first bullet in the tracking issue.
Anyway. I will go ahead and update the PR so the unstable attribute points to the correct issue.
Yeah, I know, I will fix everything eventually. I am slow :D
@Simon Sapin So I already adjusted the links in the PR, but i cannot merge it on GitHub. It says "at least one review required". Am I expected to merge it locally? Because it seems like the others always merged it via GitHub.
@centril, do we need a github "review" to merge an RFC?
dont think so
I tried adding a review, but now I get "The base branch restricts merging to authorized users."
Could I even push to
@Simon Sapin Sorry for ping again, but seems like I need a helping hand with this. Don't want to leave this PR in this half-merged state for so long. I didn't know I couldn't merge into
Clicked the green button
Thanks a bunch :)
For the future: should I get the permission to also merge? Not sure if this is intended
Maybe it’s tied to r+ permission?
I think permissions on the rfc repository are currently ad-hoc, not tied to rust-lang/team
yeah; I have write access to rust-lang/rfcs
The PR adding the constants has been updated to point to the correct tracking issue. And the CI test is happy. I think the PR is ready for review/merge @Lukas Kalbertodt
@faern Yip, saw that. But don't have any time today anymore. Will re-review it in the coming days, hopefully tomorrow.
Ok, sweet. Great we made some progress today :)
yes, thanks everyone for helping to work this through!
The crater run seems to be stuck. It has been on 120832 completed jobs and 69% for quite a while now.
@simulacrum Ping. Since you started the crater run. It's now been stuck at the same job for almost a day.
Thanks! Oh, yeah. I see now that there was activity on the issue regarding this.
Merged. Yay! Great work everyone. Hope shit does not hit the fan when the next nightly comes out :)
I sure hope so too! =D
I have now prepared a stabilization PR. I won't submit it tonight, but it's basically ready now at least. A bit more work than it sounds. Since I also had to update most examples to use the associated constants instead of the old ones. Plus add to the documentation of the old ones that they are outdated and the assoc consts should be used.
@Lukas Kalbertodt What do you think is a suitable time to wait before stabilization? My take was that it's already decided that it will be stabilized unless we find any problems. And we have had a successful crater run plus it's been in nightly for a few days now without any complaints that I'm aware of. As such, we don't need to wait very long(?)
@faern I am not completely sure about the "minimum time to stabilization", but a few days are way too little I think. I usually expect features to cook on nightly for several release cycles at least before getting stabilized. I would advise you to push your stabilization commits to a branch of yours, to NOT create a PR and to link that branch in the tracking issue. Ala "I already did the code changes and am ready to create a PR. For anyone interested: <branch>".
But that's just my take on this ^_^
I think it is not bad to stabilize soon -- I don't think we'll see much movement on use of these features before that happens since they're near-zero improvements on existing stable features. But I would personally give it a week or two to give it a little more time to bake.
I agree with @simulacrum. It's a close to zero change basically. Very little impact. And no one will use it until we actually advise them to switch over.
I'll push the code and link it in the tracking PR then and submit a PR when a bit over one week has passed.
:thumbs_up: on that plan.
I have now published the PR stabilizing the constants.