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

Stabilize const ptr::write* and mem::replace #130954

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Sep 27, 2024

Since const_mut_refs and const_refs_to_cell have been stabilized, we may now also stabilize the ability to write to places during const evaluation inside our library API. So, we now propose the const fn version of ptr::write and its variants. This allows us to also stabilize mem::replace and ptr::replace.

Their implementation requires an additional internal stabilization of const_intrinsic_forget, which is required for *::write* and thus *::replace. Thus we const-stabilize the internal intrinsics forget, write_bytes, and write_via_move.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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 Sep 27, 2024
@workingjubilee workingjubilee changed the title Stabilize Stabilize const_mut_refs-dependent API Sep 27, 2024
@workingjubilee workingjubilee added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Sep 27, 2024
@workingjubilee workingjubilee changed the title Stabilize const_mut_refs-dependent API Stabilize 7 const_mut_refs-dependent API Sep 27, 2024
@workingjubilee
Copy link
Member Author

Note that because the replace stabilization requires the write stabilization in its current implementation, these cannot really land sooner than "all together" even though mem::replace entered FCP earlier, on the 21st.

Also note that there are already existing PRs for most of the slice-related API, so I did not attempt to include any of them except the one that can be expressed via slice syntax: the rest have even harsher dependency orderings.

I will revise this PR appropriately if any of the FCPs garners objections.

@workingjubilee workingjubilee added F-const_mut_refs `#![feature(const_mut_refs)]` WG-const-eval Working group: Const evaluation F-const_refs_to_cell `#[feature(const_refs_to_cell)]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-const-eval Area: Constant evaluation (MIR interpretation) and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 27, 2024
@workingjubilee
Copy link
Member Author

workingjubilee commented Sep 27, 2024

cc @rust-lang/wg-const-eval As described in the PR message, the const_intrinsic_forget stabilization is part of the dependency chain for const_ptr_write and thus const_replace, and I believe this requires your approval. It's internal-only so it should not exactly be a big deal here.

@RalfJung
Copy link
Member

RalfJung commented Sep 28, 2024

There's in fact 3 intrinsics marked as const-stable here (meaning they are indirectly exposed to stable const code), which needs wg-const-eval approval, and t-lang should be in the loop as well:

  • forget
  • write_via_move
  • write_bytes

None of them do anything odd. In fact forget does nothing at all; the other two just copy some bytes around -- basic stuff we can confidently commit const-eval to support. The implementation of these has been battle-tested in Miri.

@rust-lang/lang this should be an easy one.

@RalfJung RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 28, 2024
@workingjubilee
Copy link
Member Author

Ah, I figured the const_ptr_write ones went without saying.

@RalfJung
Copy link
Member

It's not clear at all from the stable library surface whether new intrinsics are involved. :)

@workingjubilee workingjubilee added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. label Sep 28, 2024
@bors
Copy link
Contributor

bors commented Sep 28, 2024

☔ The latest upstream changes (presumably #130964) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee workingjubilee force-pushed the stabilize-const-mut-fn branch 2 times, most recently from a377b36 to 5f72cba Compare September 28, 2024 19:02
@RalfJung
Copy link
Member

@workingjubilee is there a reason #111774 is not included in this PR?

@traviscross traviscross removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 30, 2024
@traviscross
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Sep 30, 2024

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 30, 2024
@tgross35
Copy link
Contributor

tgross35 commented Oct 1, 2024

There is some interest in having some of these APIs available at the same time as const_mut_refs #83164 (comment). Unfortunately the merge cutoff is October 11th, which is 10 days from today. Would it be acceptable to merge this on the 10th/11th as long as lang FCP has started, even if it has not completed?

Considering libs-api already has complete or near-complete FCPs for these and lang already completed the FCP for const_mut_refs, it seems less like this is a decision point and more like a lang ack that we are exposing API related to const mutability. (I could be missing something here)

@RalfJung
Copy link
Member

RalfJung commented Oct 1, 2024

IMO we should just merge what's FCP'd until then. We can let everything that needs the intrinsics slip to the next release. (That's just const_replace and const_ptr_write, I think?)

@tgross35
Copy link
Contributor

tgross35 commented Oct 1, 2024

Ah, I did not realize that the lang FCP was related to the intrinsics, thanks for the clarification. Your suggestion sounds reasonable.

@scottmcm
Copy link
Member

scottmcm commented Oct 1, 2024

@rfcbot reviewed

Agreed there's nothing lang-concerning here. write_via_move can already be written without features on nightly using copy_nonoverlapping, for example, so isn't exposing anything new.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

Sounds reasonable to me.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 2, 2024
@rfcbot
Copy link

rfcbot commented Oct 2, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@tmandry
Copy link
Member

tmandry commented Oct 2, 2024

@rfcbot reviewed

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the meeting today... and it's now in FCP.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 2, 2024
@workingjubilee
Copy link
Member Author

workingjubilee commented Oct 2, 2024

Pursuant to my original remarks about modifying this PR if necessary as the FCPs it depends on develops, and to @tgross35's concern, I am going to chop this down to "stabilizations that depend on this T-lang FCP" and extract the quotient into a new PR: #131177

@workingjubilee workingjubilee changed the title Stabilize 7 const_mut_refs-dependent API Stabilize const ptr::write* and mem::replace Oct 2, 2024
@bors
Copy link
Contributor

bors commented Oct 5, 2024

☔ The latest upstream changes (presumably #131269) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Oct 5, 2024

☔ The latest upstream changes (presumably #131275) made this pull request unmergeable. Please resolve the merge conflicts.

This is an implicit requirement of stabilizing `const_ptr_write`.

Const-stabilizes the internal `core::intrinsics`:
- `forget`
Const-stabilizes:
- `write`
- `write_bytes`
- `write_unaligned`

In the following paths:
- `core::ptr`
- `core::ptr::NonNull`
- pointer `<*mut T>`

Const-stabilizes the internal `core::intrinsics`:
- `write_bytes`
- `write_via_move`
Depends on stabilizing `const_ptr_write`.

Const-stabilizes:
- `core::mem::replace`
- `core::ptr::replace`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. A-testsuite Area: The testsuite used to check the correctness of rustc disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-const_mut_refs `#![feature(const_mut_refs)]` F-const_refs_to_cell `#[feature(const_refs_to_cell)]` final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. WG-const-eval Working group: Const evaluation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for const_ptr_write Tracking Issue for const_replace