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

Weak's type parameter may dangle on drop #85535

Merged
merged 2 commits into from
May 26, 2021
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 21, 2021

Way back in 34076bc, #[may_dangle] was added to Rc<T> and Arc<T>'s Drop impls. That appears to have been because a test added in #28929 used Arc and Rc with dangling references at drop time. However, Weak was not covered by that test, and therefore no #[may_dangle] was forced to be added at the time.

As far as dropping, Weak has even less need to interact with the T than Rc and Arc do. Roughly speaking #[may_dangle] describes generic parameters that the outer type's Drop impl does not interact with except by possibly dropping them; no other interaction (such as trait method calls on the generic type) is permissible. It's clear this applies to Rc's and Arc's drop impl, which sometimes drop T but otherwise do not interact with one. It applies even more to Weak. Dropping a Weak cannot ever cause T's drop impl to run. Either there are strong references still in existence, in which case better not drop the T. Or there are no strong references still in existence, in which case the T would already have been dropped previously by the drop of the last strong count.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 21, 2021
@kennytm
Copy link
Member

kennytm commented May 25, 2021

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2021

📌 Commit 23a4050 has been approved by kennytm

@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 May 25, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 25, 2021
Weak's type parameter may dangle on drop

Way back in rust-lang@34076bc, #\[may_dangle\] was added to Rc\<T\> and Arc\<T\>'s Drop impls. That appears to have been because a test added in rust-lang#28929 used Arc and Rc with dangling references at drop time. However, Weak was not covered by that test, and therefore no #\[may_dangle\] was forced to be added at the time.

As far as dropping, Weak has *even less need* to interact with the T than Rc and Arc do. Roughly speaking #\[may_dangle\] describes generic parameters that the outer type's Drop impl does not interact with except by possibly dropping them; no other interaction (such as trait method calls on the generic type) is permissible. It's clear this applies to Rc's and Arc's drop impl, which sometimes drop T but otherwise do not interact with one. It applies *even more* to Weak. Dropping a Weak cannot ever cause T's drop impl to run. Either there are strong references still in existence, in which case better not drop the T. Or there are no strong references still in existence, in which case the T would already have been dropped previously by the drop of the last strong count.
@bors
Copy link
Contributor

bors commented May 26, 2021

⌛ Testing commit 23a4050 with merge 47a90f4...

@bors
Copy link
Contributor

bors commented May 26, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 47a90f4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 26, 2021
@bors bors merged commit 47a90f4 into rust-lang:master May 26, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 26, 2021
@dtolnay dtolnay deleted the weakdangle branch June 7, 2021 19:41
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants