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

Remove Drop impl of mpsc Receiver and (Sync)Sender #114965

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

benschulz
Copy link
Contributor

This change removes the empty Drop implementations for mpsc::Receiver, mpsc::Sender and mpsc::SyncSender. These implementations do not specify #[may_dangle], so by removing them we make mpsc types play nice with drop check.

This was previously attempted in #105243 but then abandoned due to a test failure. I've aligned the test with those for Mutex and RwLock.

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2023

r? @joshtriplett

(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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 18, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Sep 16, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 16, 2023

📌 Commit a38ea96 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 Sep 16, 2023
@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Sep 16, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#114965 (Remove Drop impl of mpsc Receiver and (Sync)Sender)
 - rust-lang#115434 (make `Debug` impl for `ascii::Char` match that of `char`)
 - rust-lang#115477 (Stabilize the `Saturating` type)
 - rust-lang#115611 (add diagnostic for raw identifiers in format string)
 - rust-lang#115654 (improve PassMode docs)
 - rust-lang#115862 (Migrate `compiler/rustc_hir_typeck/src/callee.rs` to translatable diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Sep 17, 2023

⌛ Testing commit a38ea96 with merge 4ce79c4...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2023
Remove Drop impl of mpsc Receiver and (Sync)Sender

This change removes the empty `Drop` implementations for `mpsc::Receiver`, `mpsc::Sender` and `mpsc::SyncSender`. These implementations do not specify `#[may_dangle]`, so by removing them we make `mpsc` types play nice with drop check.

This was previously attempted in [rust-lang#105243](rust-lang#105243 (comment)) but then [abandoned due to a test failure](rust-lang#105243 (comment)). I've aligned the test with those for `Mutex` and `RwLock`.
@bors bors merged commit 7cbe7fa into rust-lang:master Sep 17, 2023
11 of 12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 17, 2023
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2023
Rollup merge of rust-lang#114965 - benschulz:mpsc-drop, r=dtolnay

Remove Drop impl of mpsc Receiver and (Sync)Sender

This change removes the empty `Drop` implementations for `mpsc::Receiver`, `mpsc::Sender` and `mpsc::SyncSender`. These implementations do not specify `#[may_dangle]`, so by removing them we make `mpsc` types play nice with drop check.

This was previously attempted in [rust-lang#105243](rust-lang#105243 (comment)) but then [abandoned due to a test failure](rust-lang#105243 (comment)). I've aligned the test with those for `Mutex` and `RwLock`.
@steffahn
Copy link
Member

It seems like the expressed motivation mentioned here

#93563 (comment)

on why these Drop impls still existed was that some user might require you can use a type such as T = mpsc::Receiver<()> for a function like fn f<T: Drop>() {}. Of course, T: Drop bounds are pretty dumb, and maybe that's accepted breakage (I haven't looked at the guidelines on semver in Rust to find out more about removal of Drop implementations) or maybe it's easy to determine that this is non-existent breakage in crater. But I did want to mention it here nonetheless.

@benschulz
Copy link
Contributor Author

@steffahn, that point was raised in the PR linked above and deemed a non-issue, see #105243 (comment) ff.

That notwithstanding, I'm happy to file a PR that restores the Drop implementations with #[may_dangle] attributes. Just say the word. :)

@benschulz benschulz deleted the mpsc-drop branch September 20, 2023 07:09
@steffahn
Copy link
Member

steffahn commented Sep 20, 2023

Thanks for the link. I don't think they should be kept for to address this breakage. I believe it's very very unlikely someone would "rely" on this in the first place, and even then it'd likely not be very reasonable code to begin with. I just wanted it mentioned on this PR for context 😇. And also I was curious if perhaps removal of Drop is already "officially" accepted breakage anyways, or else I'm considering starting a discussion to make it officially acceptable.

@steffahn
Copy link
Member

steffahn commented Sep 20, 2023

Sorry for posting so many things here - as far as I can tell the contained mpmc::Sender/Receiver types still have an ordinary Drop implementation, so I'm questioning whether this PR actually changed anything to begin with, regarding drop check. I'm on a phone right now, so it's hard to verify this conclusively.

Even if it does make us accept code the was previously not accepted, there certainly is a lack of test for that, as this PR does not introduce any new tests.

@benschulz
Copy link
Contributor Author

Sorry for posting so many things here

No worries, I want to get this right. :)

as far as I can tell the contained mpmc::Sender/Receiver types still have an ordinary Drop implementation

The nightly documentation shows no such implementation, but the stable documentation does. Perhaps I'm misunderstanding your point?

Even if it does make us accept code the was previously not accepted, there certainly is a lack of test for that, as this PR does not introduce any new tests.

It's implicitly tested by the changed test, but I think you raise a good point in that there is no test whose sole purpose is to test that. I'll try to find the time to file a PR for it.

@dtolnay
Copy link
Member

dtolnay commented Sep 20, 2023

I was curious if perhaps removal of Drop is already "officially" accepted breakage anyways, or else I'm considering starting a discussion to make it officially acceptable.

Please do. I am confident that the standard library team would be open to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants