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 versions of ptr::slice_from_raw_parts and slice::from_raw_parts. #94946

Conversation

tobz
Copy link

@tobz tobz commented Mar 15, 2022

This doesn't close the tracking issue (#67456) as there's still the mutable variants to consider, but the immutable variants should seemingly be fine to stabilize.

Apologies if this stabilization PR is lacking something: it wasn't immediately clear if there was a different process for partial stabilizations.

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 15, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dtolnay (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2022
@tobz
Copy link
Author

tobz commented Mar 15, 2022

@rustbot modify labels: +T-libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 15, 2022
#[unstable(feature = "const_slice_from_raw_parts", issue = "67456")]
#[rustc_const_unstable(feature = "const_slice_from_raw_parts", issue = "67456")]
#[rustc_const_stable(since = "1.61.0")]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly, this is a change I'm not 100% sure on: its callers are stable in a non-const context, and yet it was marked unstable. I have no real clue what unstable even implies for a non-exported function, so this was mostly a no-thinking-involved mechanical change.

@tobz
Copy link
Author

tobz commented Mar 15, 2022

Looking at the CI failure, it looks like this actually unwinds a bit to depending on ptr::from_raw_parts being const-stable, which makes sense on a surface level but it also seems to power non-const-fns, despite also being marked unstable?

It's not clear to me if it can also be made const stable safely? This is probably one of those "wait for help" scenarios, at this point. :)

@rust-log-analyzer

This comment has been minimized.

@dtolnay dtolnay removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 18, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything used in the implementation of a public stable const fn needs to be marked rustc_const_stable.

error: `ptr::metadata::from_raw_parts` is not yet stable as a const fn
    |
260 |     from_raw_parts(data.cast(), len)
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

#[rustc_const_unstable(feature = "ptr_metadata", issue = "81513")]
#[inline]
pub const fn from_raw_parts<T: ?Sized>(

@tobz
Copy link
Author

tobz commented Mar 19, 2022

Anything used in the implementation of a public stable const fn needs to be marked rustc_const_stable.

That's reasonable. Is that something we/I can do in this case? Admittedly, it's not clear to me.

@bors
Copy link
Contributor

bors commented Mar 26, 2022

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

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 3, 2022

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

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2022
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you will need to add rustc_const_stable attributes for all the private functions getting called by these public functions. Whether it's something we can do depends on what functions end up needing to get rustc_const_stable added. That would be reviewed after the changes are made.

@JohnCSimon
Copy link
Member

Ping from triage:
@tobz - can you please address the conflicts and build failures?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: Toby Lawrence <[email protected]>
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0711]: feature `ptr_metadata` is declared stable, but was previously declared unstable
   --> library/core/src/ptr/metadata.rs:108:1
    |
108 | #[rustc_const_stable(feature = "ptr_metadata", since = "1.62.0")]

error: could not compile `core` due to previous error
Build completed unsuccessfully in 0:01:25

@tobz
Copy link
Author

tobz commented May 8, 2022

I'm going to close this PR for now.

Working through adjusting the const stability for the transitive functions landed me at having to do so for the ptr_metadata feature.. which is obviously not ready to be const stable yet, and so spending any more time -- either me making the PR pass or someone reviewing it only to also come to the same conclusion -- wouldn't make much sense.

@tobz tobz closed this May 8, 2022
@tobz tobz deleted the stabilize-immut-const-ptr-slice-from-raw-parts branch May 8, 2022 02:14
@KamilaBorowska
Copy link
Contributor

Is there a problem with marking std::ptr::from_raw_parts as const stable? Note that marking a function as const stable doesn't mean having to stabilize it, see https://rustc-dev-guide.rust-lang.org/stability.html#rustc_const_stable.

This attribute can make sense even on an unstable function, if that function is called from another rustc_const_stable function.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 10, 2022
…r=dtolnay

Partially stabilize const_slice_from_raw_parts

This doesn't stabilize methods working on mutable pointers.

This pull request continues from rust-lang#94946.

Pinging `@rust-lang/wg-const-eval` this because I use `rustc_allow_const_fn_unstable`. I believe this is justifiable as it's already possible to use `slice::from_raw_parts` in stable by abusing `transmute`. The stable alternative to this would be to provide a stable const implementation of `std::ptr::from_raw_parts` (as it can already be implemented in stable).

```rust
use std::mem;

#[repr(C)]
struct Slice<T> {
    data: *const T,
    len: usize,
}

fn main() {
    let data: *const i32 = [1, 2, 3, 4].as_ptr();
    let len = 4;
    println!("{:?}", unsafe {
        mem::transmute::<Slice<i32>, &[i32]>(Slice { data, len })
    });
}
```

`@rustbot` modify labels: +T-libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants