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_cstr_methods #107624

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Feb 3, 2023

This PR seeks to stabilize const_cstr_methods. Fixes most of #101719

New const stable API

impl CStr {
    // depends: memchr
    pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, FromBytesWithNulError> {...}
    // depends: const_slice_index
    pub const fn to_bytes(&self) -> &[u8] {}
    // depends: pointer casts
    pub const fn to_bytes_with_nul(&self) -> &[u8] {}
    // depends: str::from_utf8
    pub const fn to_str(&self) -> Result<&str, str::Utf8Error> {}
}

I don't think any of these methods will have any issue when CStr becomes a thin pointer as long as memchr is const (which also allows for const strlen) .

Notes

  • from_bytes_until_nul relies on const_slice_index, which relies on const_trait_impls, and generally this should be avoided. After talking with Oli, it should be OK in this case because we could replace the ranges with pointer tricks if needed (worst case being those feature gates disappear). Stabilize const_cstr_methods #107624 (comment)
  • Making from_ptr const is deferred because it depends on const_eval_select. I have moved this under the new flag const_cstr_from_ptr Stabilize const_cstr_methods #107624 (comment)

cc @oli-obk I think you're the const expert

@rustbot modify labels: +T-libs-api +needs-fcp

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

The Miri subtree was changed

cc @rust-lang/miri

@rustbot rustbot added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 3, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Feb 3, 2023

r? libs-api

@rustbot label -T-libs

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2023

Failed to set assignee to T-libs-api: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 3, 2023
@rustbot rustbot assigned dtolnay and unassigned thomcc Feb 3, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Feb 9, 2023

#107429 merged, so this is no longer blocked. It needs the wg-const-eval signoff, but should otherwise be good

@tgross35
Copy link
Contributor Author

tgross35 commented Feb 9, 2023

I'm not sure whether const stabilization needs an fcp or not, but with the const_slice_index and const_eval_select fixes I think this should be ready for the next step @dtolnay

(Unless you have anything else @oli-obk)

@bors
Copy link
Contributor

bors commented Feb 27, 2023

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

@dtolnay
Copy link
Member

dtolnay commented Feb 28, 2023

@rust-lang/libs-api:
@rfcbot fcp merge

This PR stabilizes the const on the following already stable functions, all of which have been stable since Rust 1.10 or older:

impl CStr {
    pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, FromBytesWithNulError>
    pub const fn to_bytes(&self) -> &[u8]
    pub const fn to_bytes_with_nul(&self) -> &[u8]
    pub const fn to_str(&self) -> Result<&str, str::Utf8Error>
}

Not all of these have a current implementation that is 100% on track for stabilization; for example the following, which uses https://doc.rust-lang.org/1.67.0/std/primitive.slice.html#method.get_unchecked, which doesn't even have a tracking issue but relies on being able to call trait methods inside const:

https://github.com/rust-lang/rust/blob/6c4269301e885c3407da8401f4ad43ee66f01352/library/core/src/ffi/c_str.rs#L569

However, I am confident that all of the functions for which const is being stabilized here could be reimplemented in a lower-level way using raw pointers, if necessary, to not call any trait methods.

@rfcbot
Copy link

rfcbot commented Feb 28, 2023

Team member @dtolnay 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!

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 Feb 28, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Feb 28, 2023

I believe the plan is still to make &CStr a thin pointer at some point. (So, strlen() to get the slice length.) Would that be harder if the to_ methods were const?

cc @rust-lang/wg-const-eval

@tgross35
Copy link
Contributor Author

tgross35 commented Jun 1, 2023

const_slice_index was removed so I updated to_bytes to just use from_raw_parts instead.

FCP needs one more check still @rust-lang/libs-api

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 1, 2023
@rfcbot
Copy link

rfcbot commented Jun 1, 2023

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 1, 2023
@BurntSushi
Copy link
Member

SGTM. One concern I have about "assuming memchr is const" is that it's plausible that the "const version of memchr" is not as fast as what one could do with non-const features (such as dynamically querying the features of the CPU at runtime). My understanding though is that there is an unstable way to say "run this code in a const context, and run this other code otherwise." That's enough to alleviate my concern, although, it would be nice if the underlying mechanism to make that work were also available to users of Rust and not just std.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 11, 2023
@rfcbot
Copy link

rfcbot commented Jun 11, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jun 11, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Jun 12, 2023

I believe const_eval_select is what you're referring to, which was discussed earlier #107624 (comment) (the reason CStr::from_ptr isn't in this PR). Based on that discussion, it seems like this will likely be intrinsic-stable by the time CStr becomes a thin pointer.

The rust implementation of memchr doesn't use that feature (link) and is what is used for CStr::from_bytes_x.

To summarize the possibilities:

Pointer Configuration from_ptr from_bytes_x len/to_bytes_x
Current (fat) O(1) best-strlen O(1) current-memchr Constant Time
Thin + const_eval_select CT O(1) best-memchr O(1) best-strlen
Thin, no const_eval_select CT O(1) current-memchr O(1) current-memchr (strlen)

So worst case, to_bytes_x will be as fast as our current implementation of from_bytes_x. If we add a const fn len it will have the same worst case.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 15, 2023
@tgross35
Copy link
Contributor Author

@dtolnay would you be able to merge this before the beta branch next week?

@dtolnay
Copy link
Member

dtolnay commented Jun 29, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2023

📌 Commit 5cb701f has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 29, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 30, 2023
…lnay

Stabilize `const_cstr_methods`

This PR seeks to stabilize `const_cstr_methods`. Fixes most of rust-lang#101719

## New const stable API

```rust
impl CStr {
    // depends: memchr
    pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, FromBytesWithNulError> {...}
    // depends: const_slice_index
    pub const fn to_bytes(&self) -> &[u8] {}
    // depends: pointer casts
    pub const fn to_bytes_with_nul(&self) -> &[u8] {}
    // depends: str::from_utf8
    pub const fn to_str(&self) -> Result<&str, str::Utf8Error> {}
}
```

I don't think any of these methods will have any issue when `CStr` becomes a thin pointer as long as `memchr` is const  (which also allows for const `strlen`) .

## Notes

- `from_bytes_until_nul` relies on `const_slice_index`, which relies on `const_trait_impls`, and generally this should be avoided. After talking with Oli, it should be OK in this case because we could replace the ranges with pointer tricks if needed (worst case being those feature gates disappear). rust-lang#107624 (comment)
- Making `from_ptr` const is deferred because it depends on `const_eval_select`. I have moved this under the new flag `const_cstr_from_ptr` rust-lang#107624 (comment)

cc `@oli-obk` I think you're the const expert

`@rustbot` modify labels: +T-libs-api +needs-fcp
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 30, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#107624 (Stabilize `const_cstr_methods`)
 - rust-lang#111403 (suggest `slice::swap` for `mem::swap(&mut x[0], &mut x[1])` borrowck error)
 - rust-lang#113071 (Account for late-bound vars from parent arg-position impl trait)
 - rust-lang#113165 (Encode item bounds for `DefKind::ImplTraitPlaceholder`)
 - rust-lang#113171 (Properly implement variances_of for RPITIT GAT)
 - rust-lang#113177 (Use structured suggestion when telling user about `for<'a>`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 016c306 into rust-lang:master Jun 30, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 30, 2023
@tgross35 tgross35 deleted the const-cstr-methods branch June 30, 2023 16:40
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Aug 24, 2023
Stabilize `const_cstr_methods`

This PR seeks to stabilize `const_cstr_methods`. Fixes most of #101719

## New const stable API

```rust
impl CStr {
    // depends: memchr
    pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, FromBytesWithNulError> {...}
    // depends: const_slice_index
    pub const fn to_bytes(&self) -> &[u8] {}
    // depends: pointer casts
    pub const fn to_bytes_with_nul(&self) -> &[u8] {}
    // depends: str::from_utf8
    pub const fn to_str(&self) -> Result<&str, str::Utf8Error> {}
}
```

I don't think any of these methods will have any issue when `CStr` becomes a thin pointer as long as `memchr` is const  (which also allows for const `strlen`) .

## Notes

- `from_bytes_until_nul` relies on `const_slice_index`, which relies on `const_trait_impls`, and generally this should be avoided. After talking with Oli, it should be OK in this case because we could replace the ranges with pointer tricks if needed (worst case being those feature gates disappear). rust-lang/rust#107624 (comment)
- Making `from_ptr` const is deferred because it depends on `const_eval_select`. I have moved this under the new flag `const_cstr_from_ptr` rust-lang/rust#107624 (comment)

cc ``@oli-obk`` I think you're the const expert

``@rustbot`` modify labels: +T-libs-api +needs-fcp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.