-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Constify conversion traits #144289
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
base: master
Are you sure you want to change the base?
Constify conversion traits #144289
Conversation
This comment has been minimized.
This comment has been minimized.
21c84d1
to
cb3f0a6
Compare
This comment has been minimized.
This comment has been minimized.
cb3f0a6
to
5620209
Compare
Cc @rust-lang/wg-const-eval @rust-lang/libs-api
Adding these new methods seems orthogonal and needs an ACP (or libs-api signoff). Could it be split to a separate PR to be discussed independently?
|
I guess I could split this PR into several parts; my main justification was that all the new additions are unstable and if they were not accepted, could be kept that way and potentially left unstable. I figured it'd be better to just mark the methods as unstably const and leave them that way, than work around that to leave the code in an undesirable state just to avoid having to run extra votes. |
Also to clarify what I mean by leaving the code in a worse state: I could make a bunch of private methods with the same functionality and then update the stable ones to use that, thus sidestepping the bureaucracy of adding constness to existing API which is in libs-api' domain. or I could just mark these methods as unstably const or outright unstable knowing that they are very similar to existing methods, but still leave them open to libs-api to discuss in a dedicated forum, as I have. The issue here is, I know that whenever I submit a dedicated libs-api PR, I risk months of things not even being noticed, because there are several people in that review pool who seemingly do not actually review PRs. So, rather than doing things via hacky private methods, or waiting on libs-api to actually review these methods separately, I just tack them on this PR and let libs-api review them at their own pace. Knowing that they're all unstable anyway, since this isn't actually affecting a stable API. |
Nothing here does anything interesting const-wise, right? No new intrinsics or anything like that? This is entirely up to t-libs-api then, as far as I am concerned. It's not clear to me why these new methods were added (why does the old implementation of I know things taking a week or three to land can be frustrating, but please be mindful that many reviewers are volunteers, and trying to bypass processes is more harmful than helpful for the project as a whole. If a process is broken, it needs to be fixed. For tiny additions such as new public methods on existing types, all signs I am aware of indicate that the ACP process works just fine. |
You're right that I'm combining just adding constness to things with making them into actual public methods. I'll close this for now and open a few separate PRs + an ACP. |
Also, to fully clarify: the main issue here is the addition of the new methods. It has generally been okay to just mark some methods as const-unstable without an ACP, since the methods were already accepted, just not in const context. I'll resubmit this PR in a modified form once I'm feeling in a less venty mood. Sorry for not being entirely clear here and trying to bypass some bureaucracy. ACPs are still important. |
Yes, new unstable methods need ACP, new unstable constness does not. |
Okay, after spending more time to reflect, the changes I need to make to this PR are actually pretty small and I just marked the methods as private instead of unstable. Apologies for trying to bypass procedure: I understand why it's there, but sometimes when I get frustrated by things I get a bit overzealous. I can still split up the PR into the parts doing different things if you want (adding unstable constness, changing features for impls, and adding the impls) but it felt all related enough that it should go together and be reviewed by the same person. For now though, I'll at least reopen until we make that decision. (assuming that rustbot/bors can handle it…) |
5620209
to
5a74039
Compare
5a74039
to
316426e
Compare
libs-api, could you weigh in here? Specifically, do you want all the traits in the top post with all possible impls to become @rustbot label +I-libs-api-nominated @clarfonthey personally I think this needs to be split up, maybe having a PR that makes all the traits |
this has been delegated to wg-const-eval. Only stabilization is where libs is interested |
While folks have been thinking about what to do with this PR, I decided to actually do a proper audit of all the impls and have now gotten everything, except I'm okay splitting it, just, I do think that most of the review is verifying that the feature flags are reasonable for what I've marked, since everything is unstable. |
316426e
to
dd6ac17
Compare
This comment has been minimized.
This comment has been minimized.
dd6ac17
to
51d7138
Compare
This comment has been minimized.
This comment has been minimized.
51d7138
to
8a8d706
Compare
This comment has been minimized.
This comment has been minimized.
8a8d706
to
52ae276
Compare
This comment has been minimized.
This comment has been minimized.
52ae276
to
2642088
Compare
This comment has been minimized.
This comment has been minimized.
2642088
to
d50bfe6
Compare
This comment has been minimized.
This comment has been minimized.
I genuinely have no idea why those miri tests are failing, but I'm going to hope it's spurious and that a rebase will fix it. It seems unrelated and I couldn't replicate it locally, and I have no idea which script I can run to run it in a closer-to-CI container. |
d50bfe6
to
eeadfef
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -38,6 +38,7 @@ impl<'a> IoSlice<'a> { | |||
} | |||
|
|||
#[inline] | |||
#[rustc_const_unstable(feature = "const_convert", issue = "143773")] |
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.
These io_slice
methods aren't crate-public, seems like rustc_const_unstable
got added quite a few places it isn't needed
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.
I'm not 100% sure what the requirements are, although I believe that in general, const-unstable methods which use const-unstable features have to be marked with these as well.
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.
Hm - something like that does sound familiar, but I thought it was only at the crate-public level for things that are rustc_const_stable
. I'm not sure why these particular methods would need it in any case though, it's just a call to slice::from_raw_parts
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.
Yeah, that's incredibly fair: I got into the habit of copy-pasting them everywhere since I was tired of having to go back and add them all. Once we've decided what the plan is going to be for the full review, I'll go back and do a proper audit of the unnecessary ones.
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.
Fwiw if the std and alloc parts are dropped (like my last non-inline comment), I'm happy to review the whole thing as one. Most of the reason I was thinking it would be nicer to split in the first place was to get the baseline in before taking a closer look about whether or not everything else can even be reasonably used in const
, but if it's basically only core
then that pretty much goes away.
I think there still needs to be some coordination here, the usual questions still exist: what makes sense to be My thoughts, mostly with respect to this PR:
|
Tracking issue: #143773
Note that this renames the feature from
const_from
toconst_convert
. Several impls were wrongly included underconst_try
and those have been moved appropriately.Feature includes the constification of the following traits:
From
Into
TryFrom
TryInto
FromStr
AsRef
AsMut
Borrow
BorrowMut
Deref
DerefMut
Note that
Deref
andDerefMut
are currently listed under the now-closed #88955, but I decided it's worth just including that here instead. Since these all end up being needed for each other, it's best to bundle them all together, even though the semantics are different.There are a few methods that are intrinsically tied to these traits which I've included in the feature. Particularly, those which are wrappers over
AsRef
:ByteStr::new
(unstable underbstr
feature)OsStr::new
Path::new
Those which directly use
Into
:Result::into_ok
Result::into_err
And those which use
Deref
andDerefMut
:Pin::as_ref
Pin::as_mut
Pin::as_deref_mut
There are a few methods which were not const-stable before, but are used in the constification of these methods or tangential to them. I added them under a separate feature,
const_convert_methods
, and filed them under #144288. They are:Box::into_boxed_slice
(Box<T> -> Box<[T]>
)Box::into_pin
(Box<T> -> Pin<Box<T>>
)CStr::as_bytes
CStr::as_bytes_with_nul
CString::as_c_str
CString::into_bytes
CString::into_bytes_with_nul
CString::into_string
IntoStringError::into_cstring
IntoStringError::utf8_error
<Box<OsStr>>::into_os_string
OsString::as_os_str
<Box<Path>>::into_path_buf
PathBuf::as_path
path::{Component, PrefixComponent}::as_os_str
<Box<str>>::into_boxed_bytes
<Box<str>>::into_string
String::from_utf8
String::from_utf8_unchecked
FromUtf8Error::as_bytes
FromUtf8Error::into_bytes
FromUtf8Error::utf8_error
Box<[T]>::into_vec
Additionally, rust-lang/libs-team#624 proposes adding the following methods, which are added by this PR but marked private:
OsString::as_mut_os_str
PathBuf::as_mut_path
I've checked through the list in rustdoc to make sure I got all the important stuff, but here's what I missed:
~const Destruct
is required for destructured arrays and tuples #144280)VaList
(unfinished, unstable)path::{Components, Iter}
(derive_const(Clone)
needs some work, didn't feel like it)ptr::addr
)FromStr
impls (a lot of these use iterators and I didn't feel like converting them)