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.

Sign up for GitHub

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

Jump to bottom

Make some Clone impls const #91804

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Make some Clone impls const #91804

merged 1 commit into from
Mar 10, 2022

Conversation

lilasta
Copy link
Contributor

@lilasta lilasta commented Dec 11, 2021 •
edited
Loading

Tracking issue: #91805
Clone::clone_from and some impls (Option, Result) bounded on ~const Drop.

// core::clone
impl const Clone for INTEGER
impl const Clone for FLOAT
impl const Clone for bool
impl const Clone for char
impl const Clone for !
impl<T: ?Sized> const Clone for *const T
impl<T: ?Sized> const Clone for *mut T
impl<T: ?Sized> const Clone for &T

// core::option
impl<T> const Clone for Option<T>
where
    T: ~const Clone + ~const Drop

// core::result
impl<T, E> const Clone for Result<T, E>
where
    T: ~const Clone + ~const Drop,
    E: ~const Clone + ~const Drop,

// core::convert
impl const Clone for Infallible

// core::ptr
impl<T: ?Sized> const Clone for NonNull<T>
impl<T: ?Sized> const Clone for Unique<T>

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2021
@lilasta lilasta changed the title Make Clone impls const Make some Clone impls const Dec 11, 2021
@lilasta lilasta mentioned this pull request Dec 11, 2021
Closed
3 tasks
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 14, 2021
@camelid
Copy link
Member

camelid commented Jan 25, 2022

r? @oli-obk maybe?

oli-obk
oli-obk reviewed Jan 26, 2022
View reviewed changes
library/core/src/option.rs
#[rustc_const_unstable(feature = "const_clone", issue = "91805")]
impl<T> const Clone for Option<T>
where
T: ~const Clone + ~const Drop,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change the way rustdoc renders it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before:
image
After:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yea, there's a pending fix for the missing space, but the additional Drop bound is really confusing for users and we should probably figure out out story there (both for documentation and explanation) before we expose it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will all such cases need to be explained in the future? It seems a bit hard... I think it would be nice if we could switch displaying of ~const (and ~const Drop) bound...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I'm unsure about ~const bounds in general, we should probably talk to the rustdoc team to figure out how it should display.

For ~const Drop I think we should just hide it from rustdoc. That bound is pretty much useless to see for anyone. We could potentially add a small symbol to the generic param that has a ~const Drop bound. Otherwise I fear that the useful bounds may get drowned out

Copy link
Contributor Author

@lilasta lilasta Jan 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

T: ~const Drop seems to mean "T doesn't require non-const Drop to destruct the value", not "T implements ~const Drop". For example, primitive types don't implement const Drop but are destructible at compile-time. Anyway, the point is that this is mostly needed in code that destructs values at compile-time and is implemented using generics.

The determination seems to be implemented here:
https://github.com/rust-lang/rust/blob/master/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs#L140

Copy link
Member

Choose a reason for hiding this comment

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

T: ~const Drop seems to mean "T doesn't require non-const Drop to destruct the value"

I'm a bit confused about what you mean. Do you mean "T must be able to be Droped at compile-time"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'm sorry for confusing you.

Copy link
Member

@camelid camelid Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What does the ~ mean? I thought ~const meant "maybe const" rather than "definitely const", but I may have misunderstood.

Copy link
Contributor

Choose a reason for hiding this comment

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

It means "const if whatever the bound is on is in a const context". So if it's on a generic param of a const fn, calling that const fn in a const item or other const context requires the generic param to implement the trait constly, in regular runtime code it just requires the trait to be implemented with no requirement about constness at all

@oli-obk oli-obk added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Mar 2, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Mar 9, 2022

This should be hidden from rustdoc now, and it doesn't introduce any new bounds that would not be hidden, so

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 9, 2022

📌 Commit 6620244 has been approved by oli-obk

@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 Mar 9, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 9, 2022
@matthiaskrgr
Rollup merge of rust-lang#91804 - woppopo:const_clone, r=oli-obk
0301ee8 
Make some `Clone` impls `const`

Tracking issue: rust-lang#91805
`Clone::clone_from` and some impls (Option, Result) bounded on `~const Drop`.

```rust
// core::clone
impl const Clone for INTEGER
impl const Clone for FLOAT
impl const Clone for bool
impl const Clone for char
impl const Clone for !
impl<T: ?Sized> const Clone for *const T
impl<T: ?Sized> const Clone for *mut T
impl<T: ?Sized> const Clone for &T

// core::option
impl<T> const Clone for Option<T>
where
    T: ~const Clone + ~const Drop

// core::result
impl<T, E> const Clone for Result<T, E>
where
    T: ~const Clone + ~const Drop,
    E: ~const Clone + ~const Drop,

// core::convert
impl const Clone for Infallible

// core::ptr
impl<T: ?Sized> const Clone for NonNull<T>
impl<T: ?Sized> const Clone for Unique<T>
```
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 9, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 9, 2022
@matthiaskrgr
Rollup merge of rust-lang#91804 - woppopo:const_clone, r=oli-obk
fe6ad3c 
Make some `Clone` impls `const`

Tracking issue: rust-lang#91805
`Clone::clone_from` and some impls (Option, Result) bounded on `~const Drop`.

```rust
// core::clone
impl const Clone for INTEGER
impl const Clone for FLOAT
impl const Clone for bool
impl const Clone for char
impl const Clone for !
impl<T: ?Sized> const Clone for *const T
impl<T: ?Sized> const Clone for *mut T
impl<T: ?Sized> const Clone for &T

// core::option
impl<T> const Clone for Option<T>
where
    T: ~const Clone + ~const Drop

// core::result
impl<T, E> const Clone for Result<T, E>
where
    T: ~const Clone + ~const Drop,
    E: ~const Clone + ~const Drop,

// core::convert
impl const Clone for Infallible

// core::ptr
impl<T: ?Sized> const Clone for NonNull<T>
impl<T: ?Sized> const Clone for Unique<T>
```
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 9, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 9, 2022
@Dylan-DPC
Rollup merge of rust-lang#91804 - woppopo:const_clone, r=oli-obk
ed0ed70 
Make some `Clone` impls `const`

Tracking issue: rust-lang#91805
`Clone::clone_from` and some impls (Option, Result) bounded on `~const Drop`.

```rust
// core::clone
impl const Clone for INTEGER
impl const Clone for FLOAT
impl const Clone for bool
impl const Clone for char
impl const Clone for !
impl<T: ?Sized> const Clone for *const T
impl<T: ?Sized> const Clone for *mut T
impl<T: ?Sized> const Clone for &T

// core::option
impl<T> const Clone for Option<T>
where
    T: ~const Clone + ~const Drop

// core::result
impl<T, E> const Clone for Result<T, E>
where
    T: ~const Clone + ~const Drop,
    E: ~const Clone + ~const Drop,

// core::convert
impl const Clone for Infallible

// core::ptr
impl<T: ?Sized> const Clone for NonNull<T>
impl<T: ?Sized> const Clone for Unique<T>
```
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 9, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2022
@bors
Auto merge of rust-lang#94787 - matthiaskrgr:rollup-yyou15f, r=matthi…
01ad0ad 
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#91804 (Make some `Clone` impls `const`)
 - rust-lang#92541 (Mention intent of `From` trait in its docs)
 - rust-lang#93057 (Add Iterator::collect_into)
 - rust-lang#94739 (Suggest `if let`/`let_else` for refutable pat in `let`)
 - rust-lang#94754 (Warn users about `||` in let chain expressions)
 - rust-lang#94763 (Add documentation about lifetimes to thread::scope.)
 - rust-lang#94768 (Ignore `close_read_wakes_up` test on SGX platform)
 - rust-lang#94772 (Add miri to the well known conditional compilation names and values)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c025962 into rust-lang:master Mar 10, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 10, 2022
@lilasta lilasta deleted the const_clone branch March 10, 2022 12:42
yvt added a commit to r3-os/r3 that referenced this pull request Mar 21, 2022
@yvt
refactor: use the default implementation of clone_from in `impl con…
388d3ce 
…st Clone`

`Clone::clone_from` gained `#[default_method_body_is_const]` in
[rust-lang/rust#91804][1].

n.b. `vec.rs` is shared by `r3_core` and `r3_kernel` (via a symbolic
link), hence the lack of a scope in the commit message headline.

[1]: rust-lang/rust#91804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
8 participants
@lilasta @rust-highfive @camelid @oli-obk @bors @joshtriplett @apiraino @rustbot