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

Rename ~const Drop to ~const Destruct #94901

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Mar 13, 2022

Migration notes: We have renamed ~const Drop to ~const std::marker::Destruct. When you bump the pinned nightly version, please make sure that all usages of ~const Drop are changed to ~const Destruct!

r? @oli-obk

Completely switching to ~const Destructible would be rather complicated, so it seems best to add it for now and wait for it to be backported to beta in the next release.

The rationale is to prevent complications such as #92149 and #94803 by introducing an entirely new trait. And ~const Destructible reads a bit better than ~const Drop. Name Bikesheddable.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2022
@fee1-dead
Copy link
Member Author

fee1-dead commented Mar 13, 2022

wait, even if I just add this as a lang item now it wouldn't work because the logic for handling them would still need to be backported... I guess we could go with a compatible approach by ignoring ~const Drop first and do something like ~const Drop + ~const Destructible

@rustbot author

@rustbot rustbot 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 Mar 13, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Mar 13, 2022

You could implement Destructible for all T on bootstrap

@fee1-dead
Copy link
Member Author

yeah that works, which means we could land this now and I will implement the logic later

@shamatar
Copy link
Contributor

If the purpose of this trait to mark the types that can be trivially dropped for the purposes of CTFE, shouldn't it be "auto" trait or some other special "const auto" trait, or may be simultaneously marker trait (never manually implementable, but not sure about it)?

@fee1-dead
Copy link
Member Author

Yeah if we can implement it for all T in bootstrap it wouldn't make sense to just land this now. I will prepare an implementation when I have time.

@fmease
Copy link
Member

fmease commented Mar 14, 2022

Minor bikeshedding: In a lot of cases, trait names use the imperative form of a verb (Copy, Send, Unsize, Borrow, Display, CoerceUnsized) instead of an adjective ending in -able or -ible. The latter is quite discouraged in Rust AFAIK. I suggest renaming Destructible to Destruct while still possible.

@clarfonthey
Copy link
Contributor

Is there ever going to be a case where types don't implement Destructible, and only that they might not be const-destructible?

@fee1-dead
Copy link
Member Author

Don't think so. The rationale behind the use of the word destructible is as another word for drop, where any type can be dropped/destructed in runtime, but not in compile time.

@clarfonthey
Copy link
Contributor

My main concern is that it seems like this is just a renamed version of ~const Drop. Like, I get that it will internally work differently, but I fail to see the main differences for an end user, other than confusion.

@fee1-dead
Copy link
Member Author

~const Drop is as confusing as any other names. If we use another trait we can write the documentation to be specifically about dropping a type in compile time and say that it is implemented non-const-ly for all T. The main rationale is to prevent any more special casing to make it not conflict with : Drop bounds.

@fee1-dead fee1-dead changed the title Add Destructible trait for replacing ~const Drop bounds in the future Rename ~const Drop to ~const Destruct Mar 21, 2022
@fee1-dead fee1-dead added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2022
@rust-log-analyzer

This comment has been minimized.

if obligation.param_env.constness() == Constness::NotConst {
return Ok(ImplSourceConstDropData { nested: vec![] });
) -> Result<ImplSourceConstDestructData<PredicateObligation<'tcx>>, SelectionError<'tcx>> {
// `~const Destruct` in a non-const environment is always trivially true, since our type is `Drop`
Copy link
Contributor

Choose a reason for hiding this comment

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

Well... for now it's true because all types implement Destruct. Unless we get those weird kind of types that may never get destructed, outside of constness all types implement Destruct

@@ -1054,9 +1062,29 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let cause = obligation.derived_cause(BuiltinDerivedObligation);

// If we have a custom `impl const Drop`, then
// first check it like a regular impl candidate
// first check it like a regular impl candidate.
// This is copied from confirm_impl_candidate but remaps the predicate to `~const Drop` beforehand.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this code make work that wouldn't otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

So.. basically the nested obligation causes must be correctly ~const Destruct, but we want the obligation that is matched to actually be ~const Drop such that match_impl wouldn't fail.

@fee1-dead fee1-dead requested a review from oli-obk March 23, 2022 09:47
@oli-obk
Copy link
Contributor

oli-obk commented Mar 23, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2022

📌 Commit fe5b813 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 23, 2022
@bors
Copy link
Contributor

bors commented Mar 23, 2022

⌛ Testing commit fe5b813 with merge 9280445...

@bors
Copy link
Contributor

bors commented Mar 23, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9280445 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 23, 2022
@bors bors merged commit 9280445 into rust-lang:master Mar 23, 2022
@bors
Copy link
Contributor

bors commented Mar 23, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9280445 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9280445): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@fee1-dead fee1-dead deleted the destructable branch March 24, 2022 07:24
@fee1-dead fee1-dead mentioned this pull request Mar 24, 2022
yvt added a commit to r3-os/r3 that referenced this pull request Mar 29, 2022
This change was driven by [rust-lang/rust#94901][1]. It's actually more
than just renaming - it fixes the irregular meaning of `~const Drop`
(“the type can be destructed in a const context”, instead of the regular
meaning of “`<T as Drop>::drop is `const fn`”), which the compiler was
unable to reconcile in `Drop` bounds and thus necessitated the work-
around explained in `[ref:drop_const_bounds]`. This is no longer an
issue because `~const Destruct` has a regular meaning.

[1]: rust-lang/rust#94901
yvt added a commit to r3-os/r3 that referenced this pull request Mar 29, 2022
This change was driven by [rust-lang/rust#94901][1]. It's actually more
than just renaming - it fixes the irregular meaning of `~const Drop`
(“the type can be destructed in a const context”, instead of the regular
meaning of “`<T as Drop>::drop is `const fn`”), which the compiler was
unable to reconcile in `Drop` bounds and thus necessitated for us to 
implement the work-around explained in `[ref:drop_const_bounds]`. This
is no longer an issue because `~const Destruct` has a regular meaning.

[1]: rust-lang/rust#94901
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants