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

Revert #85176 addition of clone_from for ManuallyDrop #85758

Merged
merged 1 commit into from
Jun 14, 2021
Merged

Revert #85176 addition of clone_from for ManuallyDrop #85758

merged 1 commit into from
Jun 14, 2021

Conversation

petertodd
Copy link
Contributor

@petertodd petertodd commented May 27, 2021 •
edited
Loading

Forwarding clone_from to the inner value changes the observable behavior, as previously the inner value would not be dropped by the default implementation.

Frankly, this is a super-niche case, so #85176 is welcome to argue the behavior should be otherwise! But if we overrride it, IMO documenting the behavior would be good.

Example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=c5d0856686fa850c1d7ee16891014efb

@petertodd
Revert rust-lang#85176 addition of clone_from for ManuallyDrop
5b2076f 
Forwarding `clone_from` to the inner value changes the observable
behavior, as previously the inner value would *not* be dropped by the
default implementation.
@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 May 27, 2021
@nagisa
Copy link
Member

nagisa commented May 28, 2021

Could you add a test for this behaviour?

@petertodd
Copy link
Contributor Author

petertodd commented May 30, 2021 via email

@JohnCSimon
Copy link
Member

ping from triage:
@petertodd assigning back to you to address a reviewer's question...

@rustbot label: -S-waiting-on-review +S-waiting-on-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 Jun 14, 2021
@nagisa
Copy link
Member

nagisa commented Jun 14, 2021

The test can be written as either an unit test inside the crate that defines this datatype or as a run-pass ui test (src/test/ui).

@nagisa nagisa added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jun 14, 2021
@m-ou-se m-ou-se assigned m-ou-se and unassigned joshtriplett Jun 14, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 14, 2021

We should discuss this issue to decide what behaviour we want. I'll open an issue for it. Once that decision is made, we should add a test for that behaviour to make sure it stays that way.

But for now let's merge and backport this to avoid breakage.

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 14, 2021

📌 Commit 5b2076f has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 14, 2021
@m-ou-se m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 14, 2021
@bors
Copy link
Contributor

bors commented Jun 14, 2021

⌛ Testing commit 5b2076f with merge 7510b0c...

@m-ou-se m-ou-se mentioned this pull request Jun 14, 2021
Closed
@bors
Copy link
Contributor

bors commented Jun 14, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 7510b0c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 14, 2021
@bors bors merged commit 7510b0c into rust-lang:master Jun 14, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 14, 2021
@Mark-Simulacrum
Copy link
Member

Note: not backporting to 1.53, as #85176 is a 1.54 PR, so 1.53 did not have the override to revert.

@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.55.0, 1.54.0 Jun 17, 2021
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 17, 2021
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jun 17, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2021
@bors
Auto merge of rust-lang#86413 - Mark-Simulacrum:beta-next, r=Mark-Sim…
bf62f4d 
…ulacrum

[beta] Bootstrap from stable

This is the follow up to master/beta promotion, as well as the first round of backports:

* Revert "Allow specifying alignment for functions rust-lang#81234"
* Revert rust-lang#85176 addition of clone_from for ManuallyDrop rust-lang#85758
* rustdoc: revert deref recur to resume inclusion of impl ExtTrait<Local> for ExtType rust-lang#84867
*  [beta] Update cargo rust-lang#86563

r? `@Mark-Simulacrum`
@petertodd petertodd deleted the 2021-revert-manuallydrop-clone-from branch July 17, 2021 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
9 participants
@petertodd @rust-highfive @nagisa @JohnCSimon @m-ou-se @bors @Mark-Simulacrum @joshtriplett @rustbot