Stream: t-libs

Topic: rfc2700


bstrie (Jan 17 2020 at 23:01, on Zulip):

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

bstrie (Jan 17 2020 at 23:04, on Zulip):

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!

BurntSushi (Jan 17 2020 at 23:12, on Zulip):

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.

Simon Sapin (Jan 17 2020 at 23:15, on Zulip):

Right, a good thing about #[rustc_deprecated(since = "…")] is that we can make the warnings start in releases arbitrarily far in the future

Simon Sapin (Jan 17 2020 at 23:20, on Zulip):

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

Simon Sapin (Jan 17 2020 at 23:21, on Zulip):

How those various cases should translate exactly in terms of when to emit deprecation warnings, I don’t have a strong opinion

Simon Sapin (Jan 17 2020 at 23:22, on Zulip):

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

Simon Sapin (Jan 17 2020 at 23:29, on Zulip):

@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

bstrie (Jan 17 2020 at 23:35, on Zulip):

@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

Simon Sapin (Jan 17 2020 at 23:35, on Zulip):

Implementation PR, not RFC

bstrie (Jan 17 2020 at 23:35, on Zulip):

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

bstrie (Jan 17 2020 at 23:39, on Zulip):

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

bstrie (Jan 17 2020 at 23:39, on Zulip):

after all, we're not that far out from a new edition... I can wait eighteen months :P

BurntSushi (Jan 18 2020 at 00:48, on Zulip):

i really like the idea of pushing some subset of deprecations to a new edition.

bstrie (Jan 18 2020 at 00:52, on Zulip):

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

Amanieu (Jan 18 2020 at 01:15, on Zulip):

Talking about deprecating stuff, I just remembered that we forgot to deprecate compare_and_swap after we added compare_exchange and compare_exchange_weak. This was back in ~1.12.0.

Steven Fackler (Jan 18 2020 at 23:42, on Zulip):

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.

faern (Jan 19 2020 at 18:10, on Zulip):

Ping? did this topic become empty now after the split?

faern (Jan 19 2020 at 18:11, on Zulip):

The RFC2700 thread is now empty? Or I'm not understanding Zulip.

davidtwco (Jan 19 2020 at 18:15, on Zulip):

I've moved messages around so that they're in the correct topics.

davidtwco (Jan 19 2020 at 18:16, on Zulip):

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.

faern (Jan 19 2020 at 18:18, on Zulip):

Ok. Copying my message over which was for this thread:

faern (Jan 19 2020 at 18:18, on Zulip):

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.

simulacrum (Jan 19 2020 at 18:58, on Zulip):

Yep, I agree that the docs should do so (nightly docs can essentially all point to the new locations)

faern (Jan 19 2020 at 19:03, on Zulip):

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 :)

simulacrum (Jan 19 2020 at 19:28, on Zulip):

hm, yes, though I imagine we'll want to move towards stabilization relatively quickly

simulacrum (Jan 19 2020 at 19:28, on Zulip):

certainly until they're stable I would feel odd even soft-deprecating the old ones

faern (Jan 19 2020 at 19:31, on Zulip):

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.

simulacrum (Jan 19 2020 at 19:34, on Zulip):

mhm

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

faern (Jan 19 2020 at 19:36, on Zulip):

Exactly! Except they redefine the old ones to use the values from the new ones in order to keep a single source of truth.

simulacrum (Jan 19 2020 at 19:36, on Zulip):

sure yes

simulacrum (Jan 19 2020 at 19:37, on Zulip):

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?

faern (Jan 19 2020 at 19:38, on Zulip):

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.

simulacrum (Jan 19 2020 at 19:39, on Zulip):

hm, yeah, trying to investigate the test itself now

simulacrum (Jan 19 2020 at 19:39, on Zulip):

I can't seem to find PI at all in that test

faern (Jan 19 2020 at 19:39, on Zulip):

I don't even know how to run it locally. So very slow to iterate towards the CI.

simulacrum (Jan 19 2020 at 19:40, on Zulip):

./x.py test --stage 1 src/test/rustdoc should work

simulacrum (Jan 19 2020 at 19:41, on Zulip):

you did modify the test case which is surprising to me

simulacrum (Jan 19 2020 at 19:41, on Zulip):

Maybe to try to debug?

faern (Jan 19 2020 at 19:42, on Zulip):

Well, yes. Currently in the PR I remove the test. I tried various ways to fix it before that.

simulacrum (Jan 19 2020 at 19:43, on Zulip):

Oh I see https://github.com/rust-lang/rust/commit/bd51e02c7ab5eb27787da407e95d5b37cd9ba0d2 was what CI comment is about

faern (Jan 19 2020 at 19:44, on Zulip):

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.

simulacrum (Jan 19 2020 at 19:44, on Zulip):

Yes, that's true looks like it should be irrelevant

simulacrum (Jan 19 2020 at 19:45, on Zulip):

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

faern (Jan 19 2020 at 19:52, on Zulip):

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:

simulacrum (Jan 19 2020 at 19:56, on Zulip):

Probably worth looking at the master-generated docs to try and see what that is looking for

simulacrum (Jan 19 2020 at 19:56, on Zulip):

might be e.g. testing that we don't show a comment or something like that

faern (Jan 19 2020 at 19:57, on Zulip):

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.

simulacrum (Jan 19 2020 at 20:01, on Zulip):

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.

faern (Jan 19 2020 at 20:01, on Zulip):

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

faern (Jan 19 2020 at 20:03, on Zulip):

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.

simulacrum (Jan 19 2020 at 20:19, on Zulip):

hm, okay, let's just drop the !has test for epsilon

faern (Jan 19 2020 at 22:15, on Zulip):

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?

Simon Sapin (Jan 20 2020 at 08:38, on Zulip):

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

gnzlbg (Jan 20 2020 at 12:42, on Zulip):

@Simon Sapin is there a #[rustc_deprecate(edition = "...")] that deprecates only after a particular edition ?

gnzlbg (Jan 20 2020 at 12:43, on Zulip):

that way people writing code using older editions won't have to deal with the warnings

Simon Sapin (Jan 20 2020 at 12:46, on Zulip):

There is not, that I know of

Simon Sapin (Jan 20 2020 at 12:46, on Zulip):

But I think it could be interesting to add

gnzlbg (Jan 20 2020 at 12:47, on Zulip):

I suppose it can be added when it is needed

gnzlbg (Jan 20 2020 at 12:48, on Zulip):

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

gnzlbg (Jan 20 2020 at 12:49, on Zulip):

what we could do in a future edition is make it an error to use those

gnzlbg (Jan 20 2020 at 12:49, on Zulip):

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)

gnzlbg (Jan 20 2020 at 12:50, on Zulip):

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

bstrie (Jan 21 2020 at 16:10, on Zulip):

@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

Simon Sapin (Jan 21 2020 at 16:10, on Zulip):

I don’t know, but either way I’d feel better with a test.

Simon Sapin (Jan 21 2020 at 16:12, on Zulip):

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

faern (Jan 23 2020 at 18:51, on Zulip):

@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.

Simon Sapin (Jan 23 2020 at 18:53, on Zulip):

Thanks for trying it out! It still sounds like a good test to add, if there isn’t one already :)

faern (Jan 23 2020 at 18:54, on Zulip):

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.

faern (Jan 23 2020 at 19:11, on Zulip):

@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

Simon Sapin (Jan 23 2020 at 19:12, on Zulip):

Yes exactly, thanks

faern (Jan 23 2020 at 19:15, on Zulip):

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?

Lukas Kalbertodt (Jan 23 2020 at 19:25, on Zulip):

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?

Lukas Kalbertodt (Jan 23 2020 at 19:26, on Zulip):

(And sorry, forgot to check Zulip this week :<)

Simon Sapin (Jan 23 2020 at 19:27, on Zulip):

Yes, go ahead. Merging a creating a tracking issue is a manual process.

Lukas Kalbertodt (Jan 23 2020 at 19:34, on Zulip):

Will do

centril (Jan 23 2020 at 19:39, on Zulip):

@Lukas Kalbertodt take a look at how I have merged RFCs in the past btw

Lukas Kalbertodt (Jan 23 2020 at 19:41, on Zulip):

@centril There is also this chapter in the Rust forge

Lukas Kalbertodt (Jan 23 2020 at 19:41, on Zulip):

I guess you always did it pretty similarly to that?

faern (Jan 23 2020 at 19:49, on Zulip):

@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.

Lukas Kalbertodt (Jan 23 2020 at 19:51, on Zulip):

Yeah, I know, I will fix everything eventually. I am slow :D

Lukas Kalbertodt (Jan 23 2020 at 20:00, on Zulip):

@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.

Simon Sapin (Jan 23 2020 at 20:24, on Zulip):

@centril, do we need a github "review" to merge an RFC?

centril (Jan 23 2020 at 20:24, on Zulip):

dont think so

Lukas Kalbertodt (Jan 23 2020 at 20:28, on Zulip):

I tried adding a review, but now I get "The base branch restricts merging to authorized users."

Lukas Kalbertodt (Jan 23 2020 at 20:28, on Zulip):

Could I even push to master?

Lukas Kalbertodt (Jan 23 2020 at 20:34, on Zulip):

@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 master :<

Simon Sapin (Jan 23 2020 at 20:37, on Zulip):

Clicked the green button

Lukas Kalbertodt (Jan 23 2020 at 20:38, on Zulip):

Thanks a bunch :)

Lukas Kalbertodt (Jan 23 2020 at 20:38, on Zulip):

For the future: should I get the permission to also merge? Not sure if this is intended

Simon Sapin (Jan 23 2020 at 20:45, on Zulip):

Maybe it’s tied to r+ permission?

simulacrum (Jan 23 2020 at 20:48, on Zulip):

I think permissions on the rfc repository are currently ad-hoc, not tied to rust-lang/team

centril (Jan 23 2020 at 21:02, on Zulip):

yeah; I have write access to rust-lang/rfcs

faern (Jan 23 2020 at 21:45, on Zulip):

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

Lukas Kalbertodt (Jan 23 2020 at 21:46, on Zulip):

@faern Yip, saw that. But don't have any time today anymore. Will re-review it in the coming days, hopefully tomorrow.

faern (Jan 23 2020 at 21:46, on Zulip):

Ok, sweet. Great we made some progress today :)

bstrie (Jan 24 2020 at 19:06, on Zulip):

yes, thanks everyone for helping to work this through!

faern (Jan 28 2020 at 20:38, on Zulip):

The crater run seems to be stuck. It has been on 120832 completed jobs and 69% for quite a while now.

faern (Jan 29 2020 at 07:26, on Zulip):

@simulacrum Ping. Since you started the crater run. It's now been stuck at the same job for almost a day.

Pietro Albini (Jan 29 2020 at 08:02, on Zulip):

fixed it

faern (Jan 29 2020 at 08:18, on Zulip):

Thanks! Oh, yeah. I see now that there was activity on the issue regarding this.

faern (Jan 30 2020 at 16:42, on Zulip):

Merged. Yay! Great work everyone. Hope shit does not hit the fan when the next nightly comes out :)

Lukas Kalbertodt (Jan 30 2020 at 19:39, on Zulip):

I sure hope so too! =D

faern (Feb 04 2020 at 22:47, on Zulip):

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.

faern (Feb 05 2020 at 17:42, on Zulip):

@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(?)

Lukas Kalbertodt (Feb 05 2020 at 18:42, on Zulip):

@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>".

Lukas Kalbertodt (Feb 05 2020 at 18:42, on Zulip):

But that's just my take on this ^_^

simulacrum (Feb 05 2020 at 18:45, on Zulip):

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.

faern (Feb 05 2020 at 18:47, on Zulip):

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.

simulacrum (Feb 05 2020 at 18:48, on Zulip):

:thumbs_up: on that plan.

faern (Feb 08 2020 at 12:07, on Zulip):

I have now published the PR stabilizing the constants.

Last update: Feb 25 2020 at 04:05UTC