Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jul 22, 2025

Tracking issue: #143773

Note that this renames the feature from const_from to const_convert. Several impls were wrongly included under const_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 and DerefMut 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 under bstr feature)
  • OsStr::new
  • Path::new

Those which directly use Into:

  • Result::into_ok
  • Result::into_err

And those which use Deref and DerefMut:

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

  • Tuple <-> array conversions (~const Destruct is required for destructured arrays and tuples #144280)
  • VaList (unfinished, unstable)
  • Collections' helper stuff (no const heap, not worth it)
  • Sync primitives (could ignore the mutexes in const context, not going to)
  • path::{Components, Iter} (derive_const(Clone) needs some work, didn't feel like it)
  • Slice iterators (requires const ptr::addr)
  • SIMD types (will do later; see Constified SIMD portable-simd#467)
  • Various heap-allocating (or heap-allocated) conversions
  • A few system-specific types
  • FromStr impls (a lot of these use iterators and I didn't feel like converting them)

@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 22, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor

Cc @rust-lang/wg-const-eval @rust-lang/libs-api

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:

* `CStr::as_bytes` (existing method, now const)
* `CStr::as_bytes_with_nul` (existing method, now const)
* `CStr::as_c_str` (existing method, now const)
* `CStr::as_mut_c_str` (new method; `DerefMut` implementation does not exist)
* `OsString::as_os_str` (existing method, now const)
* `OsString::as_mut_os_str` (new method; `DerefMut` implementation exists)
* `PathBuf::as_path` (existing method, now const)
* `PathBuf::as_mut_path` (new method; `DerefMut` implementation exists)
* `path::{Component, PrefixComponent}::as_os_str` (existing method, now const)

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?

const_try -> const_convert could optionally also be split off, to make this PR specifically about behavior that is new.

@clarfonthey
Copy link
Contributor Author

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.

@clarfonthey
Copy link
Contributor Author

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.

@RalfJung
Copy link
Member

RalfJung commented Jul 22, 2025

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 DerefMut for OsString not just work in const?), but using an omnibus PR to bypass the ACP process is definitely not appropriate. ACPs don't get assigned a single reviewer, they get team-nominated, so your point regarding the review pool makes no sense in this context. In fact, there is no such thing as a "libs-api review pool", only a libs review pool defined here, and AFAIK everyone there is active. And finally, if a PR doesn't get reviewed in 2 weeks, you can ask for a re-toll of the reviewer, you clearly know your way around the Rust project well enough to ping some appropriate reviewer. So nothing would get stuck for months.

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.

@clarfonthey
Copy link
Contributor Author

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.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2025
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Jul 22, 2025

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.

@RalfJung
Copy link
Member

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.

Yes, new unstable methods need ACP, new unstable constness does not.

@clarfonthey
Copy link
Contributor Author

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

@clarfonthey clarfonthey reopened this Jul 25, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2025
@tgross35
Copy link
Contributor

libs-api, could you weigh in here? Specifically, do you want all the traits in the top post with all possible impls to become const at this time?

@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 const and then chunk the rest into a few PRs by similar types. Most everything here is trivial, but a few hundred impls is still a lot to keep in cache.

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 29, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jul 29, 2025

libs-api, could you weigh in here? Specifically, do you want all the traits in the top post with all possible impls to become const at this time?

this has been delegated to wg-const-eval. Only stabilization is where libs is interested

@oli-obk oli-obk removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 29, 2025
@clarfonthey
Copy link
Contributor Author

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 FromStr, because I forgot that was included until the end and didn't feel like converting a bunch of iterator-based impls.

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.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Aug 1, 2025

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.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-miri failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
tests/pass/shims/x86/intrinsics-x86-avx2.rs ... ok
tests/pass/shims/x86/intrinsics-x86-gfni.rs ... ok

FAILED TEST: tests/pass/float_nan.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-m4ylHj" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/tmp/miri_ui/0/tests/pass" "tests/pass/float_nan.rs" "--edition" "2021" "--target" "i686-unknown-linux-gnu"

error: test got exit status: 101, but expected 0
 = note: the compiler panicked

error: no output was expected
Execute `./miri test --bless` to update `tests/pass/float_nan.stderr` to the actual output
+++ <stderr output>

thread 'main' panicked at tests/pass/float_nan.rs:450:5:
did not get value that should be possible: 0xfff0000000000001 (NaN: Neg, Signaling, payload = 0x1)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect


full stderr:

thread 'main' panicked at tests/pass/float_nan.rs:450:5:
did not get value that should be possible: 0xfff0000000000001 (NaN: Neg, Signaling, payload = 0x1)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect

full stdout:


FAILURES:
---

Location:
   /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.30.2/src/lib.rs:365

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-22b922dd454f6d1c pass` (exit status: 1)
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:57
  local time: Fri Aug  1 05:13:20 UTC 2025
  network time: Fri, 01 Aug 2025 05:13:20 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@@ -38,6 +38,7 @@ impl<'a> IoSlice<'a> {
}

#[inline]
#[rustc_const_unstable(feature = "const_convert", issue = "143773")]
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@tgross35
Copy link
Contributor

tgross35 commented Aug 1, 2025

libs-api, could you weigh in here? Specifically, do you want all the traits in the top post with all possible impls to become const at this time?

this has been delegated to wg-const-eval. Only stabilization is where libs is interested

I think there still needs to be some coordination here, the usual questions still exist: what makes sense to be const, what is maintainable as const, how much will possible churn in the const trait API or needing to un-const this affect things, etc.

My thoughts, mostly with respect to this PR:

  • Implementations in core that effectively wrap &, as, constructors/accessors, or a simple calls seems always fine
  • I'd be inclined to defer constifying any traits/impls in alloc or std, since the usefulness in const here is more fundamentally limited than by traits. If there isn't any specific experimentation-related thing to support, going for the completeness aspect seems like it can wait until after const traits are stable.
  • However, there are a handful of functions that don't have to do with traits and could independently become unstably const via the usual process (e.g. String::from_utf8, CString::as_bytes, etc)
  • Anything that has a FIXME(const-hack) should be looked at twice (I think some of the cases here aren't needed anymore)
  • There shouldn't be any substantial rewrites (e.g. no for to while conversions), that can wait until the needed support bits becomes const. (This is in response to the above FromStr comment, not anything changed here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for const Derefs
6 participants